Remove scheduler-side retry policy re-validation

internal/scheduler/retry/convert.go

@@ -6,9 +6,10 @@ import (
 	"github.com/armadaproject/armada/pkg/api"
 )
 
-// ConvertPolicy translates an api.RetryPolicy proto into the internal Policy,
-// validating fields. Returns an error for any malformed field (unknown action,
-// missing category, etc.).
+// ConvertPolicy translates an api.RetryPolicy proto into the internal Policy.
+// It only parses. The CRUD service validates policies at write time, so
+// conversion assumes the stored policy is valid. It still returns an error
+// for data it cannot map, for example an unknown action enum value.
 func ConvertPolicy(p *api.RetryPolicy) (*Policy, error) {
 	if p == nil {
 		return nil, fmt.Errorf("retry policy is nil")
@@ -34,9 +35,6 @@ func ConvertPolicy(p *api.RetryPolicy) (*Policy, error) {
 		DefaultAction: defaultAction,
 		Rules:         rules,
 	}
-	if err := ValidatePolicy(*policy); err != nil {
-		return nil, fmt.Errorf("policy %q: %w", p.Name, err)
-	}
 	return policy, nil
 }
 

internal/scheduler/retry/engine_test.go

@@ -4,7 +4,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 
 	"github.com/armadaproject/armada/pkg/armadaevents"
 )
@@ -243,58 +242,3 @@ func TestEngine_Evaluate(t *testing.T) {
 		})
 	}
 }
-
-func TestValidatePolicy(t *testing.T) {
-	tests := map[string]struct {
-		policy      Policy
-		expectError string
-	}{
-		"valid policy with Retry default": {
-			policy: Policy{
-				Name:          "test",
-				DefaultAction: ActionRetry,
-			},
-		},
-		"valid policy with OnCategory rule": {
-			policy: Policy{
-				Name:          "test",
-				DefaultAction: ActionFail,
-				Rules: []Rule{
-					{Action: ActionRetry, OnCategory: "transient"},
-				},
-			},
-		},
-		"empty name rejected": {
-			policy:      Policy{Name: "", DefaultAction: ActionRetry},
-			expectError: "policy name must not be empty",
-		},
-		"empty DefaultAction rejected": {
-			policy:      Policy{Name: "test", DefaultAction: ""},
-			expectError: "DefaultAction must be",
-		},
-		"unknown DefaultAction rejected": {
-			policy:      Policy{Name: "test", DefaultAction: "Skip"},
-			expectError: "DefaultAction must be",
-		},
-		"rule without OnCategory rejected": {
-			policy: Policy{
-				Name:          "test",
-				DefaultAction: ActionRetry,
-				Rules:         []Rule{{Action: ActionRetry, OnCategory: ""}},
-			},
-			expectError: "rule 0: OnCategory must be set",
-		},
-	}
-
-	for name, tc := range tests {
-		t.Run(name, func(t *testing.T) {
-			err := ValidatePolicy(tc.policy)
-			if tc.expectError != "" {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), tc.expectError)
-			} else {
-				assert.NoError(t, err)
-			}
-		})
-	}
-}

internal/scheduler/retry/types.go

@@ -1,9 +1,5 @@
 package retry
 
-import (
-	"fmt"
-)
-
 // Action is what a rule or a policy's default prescribes for a failed run:
 // retry it or fail it permanently.
 type Action string
@@ -53,29 +49,3 @@ type Result struct {
 	// always set.
 	Decision Decision
 }
-
-// ValidatePolicy checks that a policy has valid fields.
-func ValidatePolicy(p Policy) error {
-	if p.Name == "" {
-		return fmt.Errorf("policy name must not be empty")
-	}
-	if p.DefaultAction != ActionFail && p.DefaultAction != ActionRetry {
-		return fmt.Errorf("DefaultAction must be %q or %q, got %q", ActionFail, ActionRetry, p.DefaultAction)
-	}
-	for i := range p.Rules {
-		if err := validateRule(i, p.Rules[i]); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func validateRule(index int, rule Rule) error {
-	if rule.Action != ActionFail && rule.Action != ActionRetry {
-		return fmt.Errorf("rule %d: Action must be %q or %q, got %q", index, ActionFail, ActionRetry, rule.Action)
-	}
-	if rule.OnCategory == "" {
-		return fmt.Errorf("rule %d: OnCategory must be set", index)
-	}
-	return nil
-}