Merge pull request moby#2105 from aaronlehmann/rollback-regression

updater: Fix rollback regression

manager/orchestrator/replicated/update_test.go

@@ -16,14 +16,18 @@ import (
 )
 
 func TestUpdaterRollbackAndPause(t *testing.T) {
-	testUpdaterRollback(t, api.UpdateConfig_PAUSE)
+	testUpdaterRollback(t, api.UpdateConfig_PAUSE, true)
+}
+
+func TestUpdaterRollbackAndPauseNoMonitor(t *testing.T) {
+	testUpdaterRollback(t, api.UpdateConfig_PAUSE, false)
 }
 
 func TestUpdaterRollbackAndContinue(t *testing.T) {
-	testUpdaterRollback(t, api.UpdateConfig_CONTINUE)
+	testUpdaterRollback(t, api.UpdateConfig_CONTINUE, true)
 }
 
-func testUpdaterRollback(t *testing.T, rollbackFailureAction api.UpdateConfig_FailureAction) {
+func testUpdaterRollback(t *testing.T, rollbackFailureAction api.UpdateConfig_FailureAction, setMonitor bool) {
 	ctx := context.Background()
 	s := store.NewMemoryStore(nil)
 	assert.NotNil(t, s)
@@ -114,19 +118,22 @@ func testUpdaterRollback(t *testing.T, rollbackFailureAction api.UpdateConfig_Fa
 					FailureAction:   api.UpdateConfig_ROLLBACK,
 					Parallelism:     1,
 					Delay:           10 * time.Millisecond,
-					Monitor:         gogotypes.DurationProto(500 * time.Millisecond),
 					MaxFailureRatio: 0.4,
 				},
 				Rollback: &api.UpdateConfig{
 					FailureAction:   rollbackFailureAction,
 					Parallelism:     1,
 					Delay:           10 * time.Millisecond,
-					Monitor:         gogotypes.DurationProto(500 * time.Millisecond),
 					MaxFailureRatio: 0.4,
 				},
 			},
 		}
 
+		if setMonitor {
+			s1.Spec.Update.Monitor = gogotypes.DurationProto(500 * time.Millisecond)
+			s1.Spec.Rollback.Monitor = gogotypes.DurationProto(500 * time.Millisecond)
+		}
+
 		assert.NoError(t, store.CreateService(tx, s1))
 		return nil
 	})
@@ -204,6 +211,12 @@ func testUpdaterRollback(t *testing.T, rollbackFailureAction api.UpdateConfig_Fa
 	assert.Equal(t, observedTask.Status.State, api.TaskStateNew)
 	assert.Equal(t, observedTask.Spec.GetContainer().Image, "image1")
 
+	if !setMonitor {
+		// Exit early in this case, since it would take a long time for
+		// the service to reach the "*_COMPLETED" states.
+		return
+	}
+
 	// Should end up in ROLLBACK_COMPLETED state
 	for {
 		e := <-watchServiceUpdate

manager/orchestrator/update/updater.go

@@ -155,18 +155,23 @@ func (u *Updater) Run(ctx context.Context, slots []orchestrator.Slot) {
 		u.startUpdate(ctx, service.ID)
 	}
 
-	var monitoringPeriod time.Duration
+	var (
+		monitoringPeriod time.Duration
+		updateConfig     *api.UpdateConfig
+	)
 
-	updateConfig := service.Spec.Update
 	if service.UpdateStatus != nil && service.UpdateStatus.State == api.UpdateStatus_ROLLBACK_STARTED {
 		monitoringPeriod, _ = gogotypes.DurationFromProto(defaults.Service.Rollback.Monitor)
 		updateConfig = service.Spec.Rollback
 		if updateConfig == nil {
 			updateConfig = defaults.Service.Rollback
 		}
-	} else if updateConfig == nil {
+	} else {
 		monitoringPeriod, _ = gogotypes.DurationFromProto(defaults.Service.Update.Monitor)
-		updateConfig = defaults.Service.Update
+		updateConfig = service.Spec.Update
+		if updateConfig == nil {
+			updateConfig = defaults.Service.Update
+		}
 	}
 
 	parallelism := int(updateConfig.Parallelism)

manager/orchestrator/update/updater_test.go

@@ -340,7 +340,6 @@ func TestUpdaterFailureAction(t *testing.T) {
 
 	assert.Equal(t, 0, v2Counter)
 	assert.Equal(t, instances, v3Counter)
-
 }
 
 func TestUpdaterTaskTimeout(t *testing.T) {