jump: nextStep Index default value from 0 to -1

Signed-off-by: yunbo <[email protected]>

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() {

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)

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

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)
+		}
+	}
+}

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())