From fccb995bff947f977ff2465da99d8ab2117844e1 Mon Sep 17 00:00:00 2001 From: yunbo Date: Fri, 7 Jun 2024 16:42:33 +0800 Subject: [PATCH] jump: nextStep Index default value from 0 to -1 Signed-off-by: yunbo --- api/v1beta1/rollout_types.go | 4 ++++ pkg/controller/rollout/rollout_canary.go | 14 +---------- pkg/controller/rollout/rollout_progressing.go | 24 +++++++++++-------- pkg/util/rollout_utils.go | 21 ++++++++++++---- test/e2e/rollout_v1beta1_test.go | 6 ++--- 5 files changed, 38 insertions(+), 31 deletions(-) diff --git a/api/v1beta1/rollout_types.go b/api/v1beta1/rollout_types.go index 89f7a2bc..a37b2d83 100644 --- a/api/v1beta1/rollout_types.go +++ b/api/v1beta1/rollout_types.go @@ -102,6 +102,10 @@ func (r *RolloutStrategy) IsCanaryStragegy() bool { return r.GetRollingStyle() == CanaryRollingStyle || r.GetRollingStyle() == PartitionRollingStyle } +func (r *RolloutStrategy) IsEmptyRelease() bool { + return r.BlueGreen == nil && r.Canary == nil +} + // Get the steps based on the rolling style func (r *RolloutStrategy) GetSteps() []CanaryStep { switch r.GetRollingStyle() { diff --git a/pkg/controller/rollout/rollout_canary.go b/pkg/controller/rollout/rollout_canary.go index 0b93d76d..0c3e62f4 100644 --- a/pkg/controller/rollout/rollout_canary.go +++ b/pkg/controller/rollout/rollout_canary.go @@ -242,19 +242,7 @@ func (m *canaryReleaseManager) doCanaryJump(c *RolloutContext) (jumped bool) { canaryStatus := c.NewStatus.CanaryStatus currentStep := c.Rollout.Spec.Strategy.Canary.Steps[canaryStatus.CurrentStepIndex-1] nextIndex := canaryStatus.NextStepIndex - if nextIndex != util.NextBatchIndex(c.Rollout, canaryStatus.CurrentStepIndex) && nextIndex != 0 || nextIndex < 0 { - /* - * The NextStepIndex should typically be assigned a non-negative integer. - * However, assigning it a negative value acts as a sentinel to trigger an APPROVE. - * For instance, if currentStepIndex is 2 and nextStepIndex is 3, - * directly jumping to step 3 by patching nextStepIndex should not be possible since the value remains unchanged. - * By permitting the assignment of a negative value, such as -1, we can jump to step 3, which essentially - * means to approve current batch. - * Might be useful someday in the future. - */ - if nextIndex < 0 { - nextIndex = util.NextBatchIndex(c.Rollout, canaryStatus.CurrentStepIndex) - } + if nextIndex != util.NextBatchIndex(c.Rollout, canaryStatus.CurrentStepIndex) && nextIndex > 0 { currentIndexBackup := canaryStatus.CurrentStepIndex canaryStatus.CurrentStepIndex = nextIndex canaryStatus.NextStepIndex = util.NextBatchIndex(c.Rollout, nextIndex) diff --git a/pkg/controller/rollout/rollout_progressing.go b/pkg/controller/rollout/rollout_progressing.go index c57bc752..1928d432 100644 --- a/pkg/controller/rollout/rollout_progressing.go +++ b/pkg/controller/rollout/rollout_progressing.go @@ -276,14 +276,19 @@ func (r *RolloutReconciler) handleRolloutPlanChanged(c *RolloutContext) error { klog.Errorf("rollout(%s/%s) reCalculate Canary StepIndex failed: %s", c.Rollout.Namespace, c.Rollout.Name, err.Error()) return err } - // canary step configuration change causes current step index change - // if not changed, we should use a special value to notify jump + // if the target step index is the same as the NextStepIndex + // we simply set the CurrentStepState to Ready if c.NewStatus.GetSubStatus().NextStepIndex == newStepIndex { - c.NewStatus.GetSubStatus().NextStepIndex = -1 - } else { - c.NewStatus.GetSubStatus().NextStepIndex = newStepIndex + c.NewStatus.GetSubStatus().CurrentStepState = v1beta1.CanaryStepStateReady + c.NewStatus.GetSubStatus().LastUpdateTime = &metav1.Time{Time: time.Now()} + c.NewStatus.GetSubStatus().RolloutHash = c.Rollout.Annotations[util.RolloutHashAnnotation] + klog.Infof("rollout(%s/%s) canary step configuration change, and NextStepIndex(%d) state(%s)", + c.Rollout.Namespace, c.Rollout.Name, c.NewStatus.GetSubStatus().NextStepIndex, c.NewStatus.GetSubStatus().CurrentStepState) + return nil } - // directly jump to step paused to process jump + + // otherwise, we jump to step paused, where the "jump" logic exists + c.NewStatus.GetSubStatus().NextStepIndex = newStepIndex c.NewStatus.GetSubStatus().CurrentStepState = v1beta1.CanaryStepStatePaused c.NewStatus.GetSubStatus().LastUpdateTime = &metav1.Time{Time: time.Now()} c.NewStatus.GetSubStatus().RolloutHash = c.Rollout.Annotations[util.RolloutHashAnnotation] @@ -299,10 +304,9 @@ func (r *RolloutReconciler) handleNormalRolling(c *RolloutContext) error { progressingStateTransition(c.NewStatus, corev1.ConditionTrue, v1alpha1.ProgressingReasonFinalising, "Rollout has been completed and some closing work is being done") return nil } - // modifying NextStepIndex to 0 is not allowed - if c.NewStatus.GetSubStatus().NextStepIndex == 0 { - c.NewStatus.GetSubStatus().NextStepIndex = util.NextBatchIndex(c.Rollout, c.Rollout.Status.GetSubStatus().CurrentStepIndex) - } + // in case user modifies it with inappropriate value + util.CheckNextBatchIndexWithCorrect(c.Rollout) + releaseManager, err := r.getReleaseManager(c.Rollout) if err != nil { return err diff --git a/pkg/util/rollout_utils.go b/pkg/util/rollout_utils.go index 8709cda6..33a2b0d9 100644 --- a/pkg/util/rollout_utils.go +++ b/pkg/util/rollout_utils.go @@ -165,17 +165,28 @@ func EncodeHash(data string) string { return fmt.Sprintf("%x", sha256.Sum256([]byte(data))) } +// calculate the next batch index func NextBatchIndex(rollout *rolloutv1beta1.Rollout, CurrentStepIndex int32) int32 { if rollout == nil { - return 0 + return -1 } allSteps := int32(len(rollout.Spec.Strategy.GetSteps())) - // In initilization, CurrentStep is set to Completed, and NextStepIndex is set to 0 - // when Current Step is the last step (Progeessing or Completed), we set NextStepIndex to 0 - // Patching NextStepIndex to 0 is meaningless for user, if doing so, nothing happen - // and step jump won't happen if CurrentStepIndex >= allSteps { return -1 } return CurrentStepIndex + 1 } + +// check if NextStepIndex is legal, if not, correct it +func CheckNextBatchIndexWithCorrect(rollout *rolloutv1beta1.Rollout) { + if rollout == nil { + return + } + nextStep := rollout.Status.GetSubStatus().NextStepIndex + if nextStep <= 0 || nextStep > int32(len(rollout.Spec.Strategy.GetSteps())) { + rollout.Status.GetSubStatus().NextStepIndex = NextBatchIndex(rollout, rollout.Status.GetSubStatus().CurrentStepIndex) + if nextStep != rollout.Status.GetSubStatus().NextStepIndex { + klog.Infof("rollout(%s/%s) invalid nextStepIndex(%d), reset to %d", rollout.Namespace, rollout.Name, nextStep, rollout.Status.GetSubStatus().NextStepIndex) + } + } +} diff --git a/test/e2e/rollout_v1beta1_test.go b/test/e2e/rollout_v1beta1_test.go index f76030fc..31b1c1dc 100644 --- a/test/e2e/rollout_v1beta1_test.go +++ b/test/e2e/rollout_v1beta1_test.go @@ -726,7 +726,7 @@ var _ = SIGDescribe("Rollout v1beta1", func() { klog.Infof("rollout(%s) completed, and check", namespace) // rollout Expect(GetObject(rollout.Name, rollout)).NotTo(HaveOccurred()) - Expect(rollout.Status.CanaryStatus.NextStepIndex).Should(BeNumerically("==", 0)) + Expect(rollout.Status.CanaryStatus.NextStepIndex).Should(BeNumerically("==", -1)) // check service & ingress & deployment // ingress Expect(GetObject(ingress.Name, ingress)).NotTo(HaveOccurred()) @@ -1182,7 +1182,7 @@ var _ = SIGDescribe("Rollout v1beta1", func() { klog.Infof("rollout(%s) completed, and check", namespace) // rollout Expect(GetObject(rollout.Name, rollout)).NotTo(HaveOccurred()) - Expect(rollout.Status.CanaryStatus.NextStepIndex).Should(BeNumerically("==", 0)) + Expect(rollout.Status.CanaryStatus.NextStepIndex).Should(BeNumerically("==", -1)) // check service & ingress & deployment // ingress Expect(GetObject(ingress.Name, ingress)).NotTo(HaveOccurred()) @@ -1587,7 +1587,7 @@ var _ = SIGDescribe("Rollout v1beta1", func() { klog.Infof("rollout(%s) completed, and check", namespace) // rollout Expect(GetObject(rollout.Name, rollout)).NotTo(HaveOccurred()) - Expect(rollout.Status.CanaryStatus.NextStepIndex).Should(BeNumerically("==", 0)) + Expect(rollout.Status.CanaryStatus.NextStepIndex).Should(BeNumerically("==", -1)) // check service & ingress // ingress Expect(GetObject(ingress.Name, ingress)).NotTo(HaveOccurred())