From 9810e446fe1c017fbf651cfb0e84e8956d9613ab Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Mon, 19 Aug 2019 09:00:24 -0700 Subject: [PATCH] Honor MaxSurge and MaxUnavailable after last step --- rollout/canary.go | 15 ++++++---- rollout/canary_test.go | 49 +++++++++++++++++++++++++++++---- utils/replicaset/canary.go | 24 +++++++++++++--- utils/replicaset/canary_test.go | 18 +++++++++++- 4 files changed, 90 insertions(+), 16 deletions(-) diff --git a/rollout/canary.go b/rollout/canary.go index 7a6851a5b9..66026d2aec 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -91,7 +91,7 @@ func (c *RolloutController) rolloutCanary(rollout *v1alpha1.Rollout, rsList []*a func (c *RolloutController) reconcileStableRS(olderRSs []*appsv1.ReplicaSet, newRS *appsv1.ReplicaSet, stableRS *appsv1.ReplicaSet, rollout *v1alpha1.Rollout) (bool, error) { logCtx := logutil.WithRollout(rollout) if !replicasetutil.CheckStableRSExists(newRS, stableRS) { - logCtx.Info("No StableRS exists to reconcile") + logCtx.Info("No StableRS exists to reconcile or matches newRS") return false, nil } _, stableRSReplicaCount := replicasetutil.CalculateReplicaCountsForCanary(rollout, newRS, stableRS, olderRSs) @@ -253,8 +253,11 @@ func (c *RolloutController) syncRolloutStatusCanary(olderRSs []*appsv1.ReplicaSe } if stepCount == 0 { - logCtx.Info("Rollout has no steps so setting stableRS status to currentPodHash") - newStatus.Canary.StableRS = newStatus.CurrentPodHash + logCtx.Info("Rollout has no steps") + if newRS != nil && newRS.Status.AvailableReplicas == defaults.GetRolloutReplicasOrDefault(r) { + logCtx.Info("New RS has successfully progressed") + newStatus.Canary.StableRS = newStatus.CurrentPodHash + } newStatus = c.calculateRolloutConditions(r, newStatus, allRSs, newRS) return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) } @@ -262,7 +265,10 @@ func (c *RolloutController) syncRolloutStatusCanary(olderRSs []*appsv1.ReplicaSe if *currentStepIndex == stepCount { logCtx.Info("Rollout has executed every step") newStatus.CurrentStepIndex = &stepCount - newStatus.Canary.StableRS = newStatus.CurrentPodHash + if newRS != nil && newRS.Status.AvailableReplicas == defaults.GetRolloutReplicasOrDefault(r) { + logCtx.Info("New RS has successfully progressed") + newStatus.Canary.StableRS = newStatus.CurrentPodHash + } newStatus = c.calculateRolloutConditions(r, newStatus, allRSs, newRS) return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) } @@ -272,7 +278,6 @@ func (c *RolloutController) syncRolloutStatusCanary(olderRSs []*appsv1.ReplicaSe newStatus.CurrentStepIndex = currentStepIndex if int(*currentStepIndex) == len(r.Spec.Strategy.CanaryStrategy.Steps) { c.recorder.Event(r, corev1.EventTypeNormal, "SettingStableRS", "Completed all steps") - newStatus.Canary.StableRS = newStatus.CurrentPodHash } logCtx.Infof("Incrementing the Current Step Index to %d", *currentStepIndex) c.recorder.Eventf(r, corev1.EventTypeNormal, "SetStepIndex", "Set Step Index to %d", int(*currentStepIndex)) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 3f57130787..598149b5a6 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -220,9 +220,6 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatchTemplate := `{ "status":{ - "canary": { - "stableRS":"` + rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `" - }, "pauseStartTime": null, "conditions" : %s, "currentStepIndex": 1 @@ -620,6 +617,49 @@ func TestRollBackToStable(t *testing.T) { assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } +func TestGradualShiftToNewStable(t *testing.T) { + f := newFixture(t) + defer f.Close() + + steps := []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }} + r1 := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(3), intstr.FromInt(0)) + + r2 := bumpVersion(r1) + rs2 := newReplicaSetWithStatus(r2, 4, 4) + + rs1 := newReplicaSetWithStatus(r1, 9, 9) + rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + f.kubeobjects = append(f.kubeobjects, rs1, rs2) + f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) + + r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 13, 4, 13, false) + maxSurge := intstr.FromInt(3) + r2.Spec.Strategy.CanaryStrategy.MaxSurge = &maxSurge + r2.Status.CurrentPodHash = rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + f.rolloutLister = append(f.rolloutLister, r2) + f.objects = append(f.objects, r2) + + updatedR2SIndex := f.expectUpdateReplicaSetAction(rs1) + patchIndex := f.expectPatchRolloutAction(r1) + f.run(getKey(r2, t)) + + updatedRS2 := f.getUpdatedReplicaSet(updatedR2SIndex) + assert.Equal(t, rs1.Name, updatedRS2.Name) + assert.Equal(t, int32(6), *updatedRS2.Spec.Replicas) + + expectedPatchWithoutSub := `{ + "status":{ + "conditions": %s + } + }` + newConditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) + expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, newConditions) + patch := f.getPatchedRollout(patchIndex) + assert.Equal(t, calculatePatch(r2, expectedPatch), patch) +} + func TestRollBackToStableAndStepChange(t *testing.T) { f := newFixture(t) defer f.Close() @@ -695,9 +735,6 @@ func TestCanaryRolloutIncrementStepIfSetWeightsAreCorrect(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatch := `{ "status":{ - "canary":{ - "stableRS":"` + rs3.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `" - }, "currentStepIndex":1, "conditions": %s } diff --git a/utils/replicaset/canary.go b/utils/replicaset/canary.go index 7fdc7056c5..34471da457 100644 --- a/utils/replicaset/canary.go +++ b/utils/replicaset/canary.go @@ -140,15 +140,15 @@ func CalculateReplicaCountsForCanary(rollout *v1alpha1.Rollout, newRS *appsv1.Re } minAvailableReplicaCount := rolloutSpecReplica - MaxUnavailable(rollout) - totalCurrentAvailableReplicaCount := GetAvailableReplicaCountForReplicaSets(allRSs) - scaleDownCount := totalCurrentAvailableReplicaCount - minAvailableReplicaCount + + totalAvailableOlderReplicaCount := GetAvailableReplicaCountForReplicaSets(oldRSs) + scaleDownCount := GetReplicasForScaleDown(newRS) + GetReplicasForScaleDown(stableRS) + totalAvailableOlderReplicaCount - minAvailableReplicaCount + if scaleDownCount <= 0 { // Cannot scale down stableRS or newRS without going below min available replica count return newRSReplicaCount, stableRSReplicaCount } - totalAvailableOlderReplicaCount := GetAvailableReplicaCountForReplicaSets(oldRSs) - if scaleDownCount <= totalAvailableOlderReplicaCount { //Need to scale down older replicas before scaling down the newRS or stableRS. return newRSReplicaCount, stableRSReplicaCount @@ -196,6 +196,22 @@ func CheckStableRSExists(newRS, stableRS *appsv1.ReplicaSet) bool { return true } +// GetReplicasForScaleDown returns the number of replicas to consider for scaling down. +func GetReplicasForScaleDown(rs *appsv1.ReplicaSet) int32 { + if rs == nil { + return int32(0) + } + if *rs.Spec.Replicas < rs.Status.AvailableReplicas { + // The ReplicaSet is already going to scale down replicas since the availableReplica count is bigger + // than the spec count. The controller uses the .Spec.Replicas to prevent the controller from + // assuming the extra replicas (availableReplica - .Spec.Replicas) are going to remain available. + // Otherwise, the controller use those extra replicas to scale down more replicas and potentially + // violate the min available. + return *rs.Spec.Replicas + } + return rs.Status.AvailableReplicas +} + // extraReplicaAdded checks if an extra replica is added because the stable and canary replicas count are both // rounded up. The controller rounds both of the replica counts when the setWeight does not distribute evenly // in order to prevent either from having a 0 replica count. diff --git a/utils/replicaset/canary_test.go b/utils/replicaset/canary_test.go index 64d656d2a2..dbc6b05357 100644 --- a/utils/replicaset/canary_test.go +++ b/utils/replicaset/canary_test.go @@ -69,6 +69,22 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) { olderRS *appsv1.ReplicaSet }{ + { + name: "Do not add extra RSs in scaleDownCount when .Spec.Replica < AvailableReplicas", + rolloutSpecReplicas: 10, + setWeight: 100, + maxSurge: intstr.FromInt(3), + maxUnavailable: intstr.FromInt(0), + + stableSpecReplica: 7, + stableAvailableReplica: 8, + + canarySpecReplica: 6, + canaryAvailableReplica: 3, + + expectedStableReplicaCount: 7, + expectedCanaryReplicaCount: 6, + }, { name: "Use max surge int to scale up canary", rolloutSpecReplicas: 10, @@ -296,7 +312,7 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) { expectedCanaryReplicaCount: 3, }, { - name: "Do not scale down newRS or stable when older RS count >= minAvailable", + name: "Do not scale down newRS or stable when older RS count >= scaleDownCount", rolloutSpecReplicas: 10, setWeight: 30, maxSurge: intstr.FromInt(0),