Merge pull request #742 from hashicorp/b-restart-attempts

Restart policy handling / validation adjustments

client/restarts.go

@@ -26,6 +26,11 @@ type RestartTracker struct {
 }
 
 func (r *RestartTracker) NextRestart(exitCode int) (bool, time.Duration) {
+	// Hot path if no attempts are expected
+	if r.policy.Attempts == 0 {
+		return false, 0
+	}
+
 	// Check if we have entered a new interval.
 	end := r.startTime.Add(r.policy.Interval)
 	now := time.Now()

client/restarts_test.go

@@ -77,5 +77,14 @@ func TestClient_RestartTracker_NoRestartOnSuccess(t *testing.T) {
 	if shouldRestart, _ := rt.NextRestart(0); shouldRestart {
 		t.Fatalf("NextRestart() returned %v, expected: %v", shouldRestart, false)
 	}
+}
 
+func TestClient_RestartTracker_ZeroAttempts(t *testing.T) {
+	t.Parallel()
+	p := testPolicy(true, structs.RestartPolicyModeFail)
+	p.Attempts = 0
+	rt := newRestartTracker(p)
+	if actual, when := rt.NextRestart(1); actual {
+		t.Fatalf("expect no restart, got restart/delay: %v", when)
+	}
 }

nomad/structs/structs.go

@@ -1199,6 +1199,11 @@ func (r *RestartPolicy) Validate() error {
 		return fmt.Errorf("Unsupported restart mode: %q", r.Mode)
 	}
 
+	// Check for ambiguous/confusing settings
+	if r.Attempts == 0 && r.Mode != RestartPolicyModeFail {
+		return fmt.Errorf("Restart policy %q with %d attempts is ambiguous", r.Mode, r.Attempts)
+	}
+
 	if r.Interval == 0 {
 		return nil
 	}

nomad/structs/structs_test.go

@@ -654,3 +654,43 @@ func TestPeriodicConfig_NextCron(t *testing.T) {
 		}
 	}
 }
+
+func TestRestartPolicy_Validate(t *testing.T) {
+	// Policy with acceptable restart options passes
+	p := &RestartPolicy{
+		Mode:     RestartPolicyModeFail,
+		Attempts: 0,
+	}
+	if err := p.Validate(); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Policy with ambiguous restart options fails
+	p = &RestartPolicy{
+		Mode:     RestartPolicyModeDelay,
+		Attempts: 0,
+	}
+	if err := p.Validate(); err == nil || !strings.Contains(err.Error(), "ambiguous") {
+		t.Fatalf("expect ambiguity error, got: %v", err)
+	}
+
+	// Bad policy mode fails
+	p = &RestartPolicy{
+		Mode:     "nope",
+		Attempts: 1,
+	}
+	if err := p.Validate(); err == nil || !strings.Contains(err.Error(), "mode") {
+		t.Fatalf("expect mode error, got: %v", err)
+	}
+
+	// Fails when attempts*delay does not fit inside interval
+	p = &RestartPolicy{
+		Mode:     RestartPolicyModeDelay,
+		Attempts: 3,
+		Delay:    5 * time.Second,
+		Interval: time.Second,
+	}
+	if err := p.Validate(); err == nil || !strings.Contains(err.Error(), "can't restart") {
+		t.Fatalf("expect restart interval error, got: %v", err)
+	}
+}