Merge pull request #4039 from hashicorp/b-reschedule-validate

Adds additional validation for ambiguous settings

nomad/structs/structs.go

@@ -2897,6 +2897,16 @@ func (r *ReschedulePolicy) Validate() error {
 	}
 	var mErr multierror.Error
 	// Check for ambiguous/confusing settings
+	if r.Attempts > 0 {
+		if r.Interval <= 0 {
+			multierror.Append(&mErr, fmt.Errorf("Interval must be a non zero value if Attempts > 0"))
+		}
+		if r.Unlimited {
+			multierror.Append(&mErr, fmt.Errorf("Reschedule Policy with Attempts = %v, Interval = %v, "+
+				"and Unlimited = %v is ambiguous", r.Attempts, r.Interval, r.Unlimited))
+			multierror.Append(&mErr, errors.New("If Attempts >0, Unlimited cannot also be set to true"))
+		}
+	}
 
 	delayPreCheck := true
 	// Delay should be bigger than the default
@@ -2914,11 +2924,11 @@ func (r *ReschedulePolicy) Validate() error {
 	// Validate MaxDelay if not using linear delay progression
 	if r.DelayFunction != "linear" {
 		if r.MaxDelay.Nanoseconds() < ReschedulePolicyMinDelay.Nanoseconds() {
-			multierror.Append(&mErr, fmt.Errorf("Delay Ceiling cannot be less than %v (got %v)", ReschedulePolicyMinDelay, r.Delay))
+			multierror.Append(&mErr, fmt.Errorf("Max Delay cannot be less than %v (got %v)", ReschedulePolicyMinDelay, r.Delay))
 			delayPreCheck = false
 		}
 		if r.MaxDelay < r.Delay {
-			multierror.Append(&mErr, fmt.Errorf("Delay Ceiling cannot be less than Delay %v (got %v)", r.Delay, r.MaxDelay))
+			multierror.Append(&mErr, fmt.Errorf("Max Delay cannot be less than Delay %v (got %v)", r.Delay, r.MaxDelay))
 			delayPreCheck = false
 		}
 
@@ -5674,7 +5684,11 @@ func (a *Allocation) RescheduleEligible(reschedulePolicy *ReschedulePolicy, fail
 	if !enabled {
 		return false
 	}
-	if (a.RescheduleTracker == nil || len(a.RescheduleTracker.Events) == 0) && attempts > 0 || reschedulePolicy.Unlimited {
+	if reschedulePolicy.Unlimited {
+		return true
+	}
+	// Early return true if there are no attempts yet and the number of allowed attempts is > 0
+	if (a.RescheduleTracker == nil || len(a.RescheduleTracker.Events) == 0) && attempts > 0 {
 		return true
 	}
 	attempted := 0

nomad/structs/structs_test.go

@@ -2157,7 +2157,7 @@ func TestReschedulePolicy_Validate(t *testing.T) {
 				Delay:         15 * time.Second,
 				MaxDelay:      5 * time.Second},
 			errors: []error{
-				fmt.Errorf("Delay Ceiling cannot be less than Delay %v (got %v)",
+				fmt.Errorf("Max Delay cannot be less than Delay %v (got %v)",
 					15*time.Second, 5*time.Second),
 			},
 		},
@@ -2226,7 +2226,7 @@ func TestReschedulePolicy_Validate(t *testing.T) {
 			},
 		},
 		{
-			desc: "Valid Unlimited config",
+			desc: "Ambiguous Unlimited config, has both attempts and unlimited set",
 			ReschedulePolicy: &ReschedulePolicy{
 				Attempts:      1,
 				Unlimited:     true,
@@ -2234,6 +2234,10 @@ func TestReschedulePolicy_Validate(t *testing.T) {
 				Delay:         5 * time.Minute,
 				MaxDelay:      1 * time.Hour,
 			},
+			errors: []error{
+				fmt.Errorf("Interval must be a non zero value if Attempts > 0"),
+				fmt.Errorf("Reschedule Policy with Attempts = %v, Interval = %v, and Unlimited = %v is ambiguous", 1, time.Duration(0), true),
+			},
 		},
 		{
 			desc: "Invalid Unlimited config",
@@ -2245,7 +2249,16 @@ func TestReschedulePolicy_Validate(t *testing.T) {
 			},
 			errors: []error{
 				fmt.Errorf("Delay cannot be less than %v (got %v)", ReschedulePolicyMinDelay, 0*time.Second),
-				fmt.Errorf("Delay Ceiling cannot be less than %v (got %v)", ReschedulePolicyMinDelay, 0*time.Second),
+				fmt.Errorf("Max Delay cannot be less than %v (got %v)", ReschedulePolicyMinDelay, 0*time.Second),
+			},
+		},
+		{
+			desc: "Valid Unlimited config",
+			ReschedulePolicy: &ReschedulePolicy{
+				Unlimited:     true,
+				DelayFunction: "exponential",
+				Delay:         5 * time.Second,
+				MaxDelay:      1 * time.Hour,
 			},
 		},
 	}
@@ -2538,6 +2551,14 @@ func TestAllocation_ShouldReschedule(t *testing.T) {
 			ReschedulePolicy: nil,
 			ShouldReschedule: false,
 		},
+		{
+			Desc:             "Reschedule with unlimited and attempts >0",
+			ClientStatus:     AllocClientStatusFailed,
+			DesiredStatus:    AllocDesiredStatusRun,
+			FailTime:         fail,
+			ReschedulePolicy: &ReschedulePolicy{Attempts: 1, Unlimited: true},
+			ShouldReschedule: true,
+		},
 		{
 			Desc:             "Reschedule when client status is complete",
 			ClientStatus:     AllocClientStatusComplete,