traffic: Refactor continous logic (#222)

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

api/v1alpha1/trafficrouting_types.go

@@ -30,6 +30,7 @@ type TrafficRoutingRef struct {
 	// Service holds the name of a service which selects pods with stable version and don't select any pods with canary version.
 	Service string `json:"service"`
 	// Optional duration in seconds the traffic provider(e.g. nginx ingress controller) consumes the service, ingress configuration changes gracefully.
+	// +kubebuilder:default=3
 	GracePeriodSeconds int32 `json:"gracePeriodSeconds,omitempty"`
 	// Ingress holds Ingress specific configuration to route traffic, e.g. Nginx, Alb.
 	Ingress *IngressTrafficRouting `json:"ingress,omitempty"`

api/v1beta1/rollout_types.go

@@ -556,7 +556,7 @@ const (
 	// Restore the GatewayAPI/Ingress/Istio
 	FinalisingStepTypeGateway FinalisingStepType = "RestoreGateway"
 	// Delete Canary Service
-	FinalisingStepTypeCanaryService FinalisingStepType = "DeleteCanayService"
+	FinalisingStepTypeDeleteCanaryService FinalisingStepType = "DeleteCanaryService"
 	// Delete Batch Release
 	FinalisingStepTypeDeleteBR FinalisingStepType = "DeleteBatchRelease"
 )

api/v1beta1/trafficrouting.go

@@ -21,6 +21,7 @@ type TrafficRoutingRef struct {
 	// Service holds the name of a service which selects pods with stable version and don't select any pods with canary version.
 	Service string `json:"service"`
 	// Optional duration in seconds the traffic provider(e.g. nginx ingress controller) consumes the service, ingress configuration changes gracefully.
+	// +kubebuilder:default=3
 	GracePeriodSeconds int32 `json:"gracePeriodSeconds,omitempty"`
 	// Ingress holds Ingress specific configuration to route traffic, e.g. Nginx, Alb.
 	Ingress *IngressTrafficRouting `json:"ingress,omitempty"`

config/crd/bases/rollouts.kruise.io_rollouts.yaml

@@ -373,6 +373,7 @@ spec:
                                   type: string
                               type: object
                             gracePeriodSeconds:
+                              default: 3
                               description: Optional duration in seconds the traffic
                                 provider(e.g. nginx ingress controller) consumes the
                                 service, ingress configuration changes gracefully.
@@ -943,6 +944,7 @@ spec:
                                   type: string
                               type: object
                             gracePeriodSeconds:
+                              default: 3
                               description: Optional duration in seconds the traffic
                                 provider(e.g. nginx ingress controller) consumes the
                                 service, ingress configuration changes gracefully.
@@ -1350,6 +1352,7 @@ spec:
                                   type: string
                               type: object
                             gracePeriodSeconds:
+                              default: 3
                               description: Optional duration in seconds the traffic
                                 provider(e.g. nginx ingress controller) consumes the
                                 service, ingress configuration changes gracefully.

config/crd/bases/rollouts.kruise.io_trafficroutings.yaml

@@ -82,6 +82,7 @@ spec:
                           type: string
                       type: object
                     gracePeriodSeconds:
+                      default: 3
                       description: Optional duration in seconds the traffic provider(e.g.
                         nginx ingress controller) consumes the service, ingress configuration
                         changes gracefully.

pkg/controller/rollout/rollout_progressing.go

@@ -406,26 +406,98 @@ func isRollingBackInBatches(rollout *v1beta1.Rollout, workload *util.Workload) b
 	return workload.IsInRollback && workload.CanaryRevision != status.GetCanaryRevision() && inBatch
 }
 
-// 1. modify network api(ingress or gateway api) configuration, and route 100% traffic to stable pods
-// 2. remove batchRelease CR.
+// 1. restore network api(ingress/gatewayAPI/Istio) configuration, potentially route all traffic to stable pods
+// 2. remove batchRelease CR
+// 3. remove canary Service
 func (r *RolloutReconciler) doProgressingReset(c *RolloutContext) (bool, error) {
-	if len(c.Rollout.Spec.Strategy.Canary.TrafficRoutings) > 0 {
-		// modify network api(ingress or gateway api) configuration, and route 100% traffic to stable pods
-		tr := newTrafficRoutingContext(c)
-		done, err := r.trafficRoutingManager.FinalisingTrafficRouting(tr, false)
-		c.NewStatus.CanaryStatus.LastUpdateTime = tr.LastUpdateTime
-		if err != nil || !done {
-			return done, err
-		}
-	}
-	done, err := r.canaryManager.removeBatchRelease(c)
+	releaseManager, err := r.getReleaseManager(c.Rollout)
 	if err != nil {
-		klog.Errorf("rollout(%s/%s) DoFinalising batchRelease failed: %s", c.Rollout.Namespace, c.Rollout.Name, err.Error())
 		return false, err
-	} else if !done {
-		return false, nil
 	}
-	return true, nil
+	// if no trafficRouting exists, simply remove batchRelease
+	if !c.Rollout.Spec.Strategy.HasTrafficRoutings() {
+		done, err := releaseManager.removeBatchRelease(c)
+		if err != nil {
+			klog.Errorf("rollout(%s/%s) DoFinalising batchRelease failed: %s", c.Rollout.Namespace, c.Rollout.Name, err.Error())
+			return false, err
+		} else if !done {
+			return false, nil
+		}
+		return true, nil
+	}
+	// else, do reset logic under next sequence:
+	// restore the gateway -> remove the batchRelease -> remove the canary Service
+	subStatus := c.NewStatus.GetSubStatus()
+	if subStatus == nil {
+		return true, nil
+	}
+	gracePeriodSeconds := c.Rollout.Spec.Strategy.GetTrafficRouting()[0].GracePeriodSeconds
+	// To ensure respect for graceful time between these steps, we set start timer before the first step
+	if len(subStatus.FinalisingStep) == 0 {
+		subStatus.LastUpdateTime = &metav1.Time{Time: time.Now()}
+	}
+	tr := newTrafficRoutingContext(c)
+	klog.Infof("rollout(%s/%s) Finalising Step is %s", c.Rollout.Namespace, c.Rollout.Name, subStatus.FinalisingStep)
+	switch subStatus.FinalisingStep {
+	default:
+		// start from FinalisingStepTypeGateway
+		subStatus.FinalisingStep = v1beta1.FinalisingStepTypeGateway
+		fallthrough
+	// firstly, restore the gateway resources (ingress/gatewayAPI/Istio), that means
+	// only stable Service will accept the traffic
+	case v1beta1.FinalisingStepTypeGateway:
+		//TODO - RestoreGateway returns (bool, error) pair instead of error only.
+		// return (fasle, nil): gateway is patched successfully, but we need time to observe; recheck later
+		// return (true, nil): gateway is patched successfully, and accepts the update successfully; go to next step then
+		// return (false, error): gateway encounters error when patched, or the update is not accepted; recheck later
+		err := r.trafficRoutingManager.RestoreGateway(tr)
+		if err != nil {
+			subStatus.LastUpdateTime = tr.LastUpdateTime
+			return false, err
+		}
+		// usually, GracePeriodSeconds means duration to wait after an operation is done,
+		// we use defaultGracePeriodSeconds+1 here because the timer started before the RestoreGateway step
+		if subStatus.LastUpdateTime != nil && time.Since(subStatus.LastUpdateTime.Time) < time.Second*time.Duration(gracePeriodSeconds+1) {
+			klog.Infof("rollout(%s/%s) in step (%s), and wait %d seconds", c.Rollout.Namespace, c.Rollout.Name, subStatus.FinalisingStep, gracePeriodSeconds+1)
+			return false, nil
+		}
+		klog.Infof("rollout(%s/%s) in step (%s), and success", c.Rollout.Namespace, c.Rollout.Name, subStatus.FinalisingStep)
+		subStatus.LastUpdateTime = &metav1.Time{Time: time.Now()}
+		subStatus.FinalisingStep = v1beta1.FinalisingStepTypeDeleteBR
+		fallthrough
+	// secondly, remove the batchRelease. For canary release, it means the immediate deletion of
+	// canary deployment, for other release, the v2 pods won't be deleted immediately
+	// in both cases, only the stable pods (v1) accept the traffic
+	case v1beta1.FinalisingStepTypeDeleteBR:
+		done, err := releaseManager.removeBatchRelease(c)
+		if err != nil {
+			klog.Errorf("rollout(%s/%s) Finalize batchRelease failed: %s", c.Rollout.Namespace, c.Rollout.Name, err.Error())
+			return false, err
+		} else if !done {
+			return false, nil
+		}
+		klog.Infof("rollout(%s/%s) in step (%s), and success", c.Rollout.Namespace, c.Rollout.Name, subStatus.FinalisingStep)
+		subStatus.LastUpdateTime = &metav1.Time{Time: time.Now()}
+		subStatus.FinalisingStep = v1beta1.FinalisingStepTypeDeleteCanaryService
+		fallthrough
+	// finally, remove the canary service. This step can swap with the last step.
+	/*
+		NOTE: we only remove the canary service, while leaving stable service unchanged, that
+		means the stable service may still has the selector of stable revision. Consider this senario:
+		continuous release v1->v2->3, and currently we are in step x, which expects
+		to route 0% traffic to v2 (or simply A/B test), if we release v3 in step x and remove the
+		stable service selector, then the traffic will route to both v1 and v2 before executing the
+		first step of v3 release.
+	*/
+	case v1beta1.FinalisingStepTypeDeleteCanaryService:
+		err := r.trafficRoutingManager.RemoveCanaryService(tr)
+		if err != nil {
+			subStatus.LastUpdateTime = tr.LastUpdateTime
+			return false, err
+		}
+		klog.Infof("rollout(%s/%s) in step (%s), and success", c.Rollout.Namespace, c.Rollout.Name, subStatus.FinalisingStep)
+		return true, nil
+	}
 }
 
 func (r *RolloutReconciler) recalculateCanaryStep(c *RolloutContext) (int32, error) {

pkg/controller/rollout/rollout_progressing_test.go

@@ -533,7 +533,7 @@ func TestReconcileRolloutProgressing(t *testing.T) {
 			},
 		},
 		{
-			name: "ReconcileRolloutProgressing rolling -> continueRelease",
+			name: "ReconcileRolloutProgressing rolling -> continueRelease1",
 			getObj: func() ([]*apps.Deployment, []*apps.ReplicaSet) {
 				dep1 := deploymentDemo.DeepCopy()
 				dep1.Spec.Template.Spec.Containers[0].Image = "echoserver:v3"
@@ -578,10 +578,72 @@ func TestReconcileRolloutProgressing(t *testing.T) {
 			},
 			expectStatus: func() *v1beta1.RolloutStatus {
 				s := rolloutDemo.Status.DeepCopy()
-				s.CanaryStatus = nil
+				s.CanaryStatus.ObservedWorkloadGeneration = 2
+				s.CanaryStatus.RolloutHash = "f55bvd874d5f2fzvw46bv966x4bwbdv4wx6bd9f7b46ww788954b8z8w29b7wxfd"
+				s.CanaryStatus.StableRevision = "pod-template-hash-v1"
+				s.CanaryStatus.CanaryRevision = "6f8cc56547"
+				s.CanaryStatus.CurrentStepIndex = 3
+				s.CanaryStatus.CanaryReplicas = 5
+				s.CanaryStatus.CanaryReadyReplicas = 3
+				s.CanaryStatus.FinalisingStep = v1beta1.FinalisingStepTypeGateway
+				s.CanaryStatus.PodTemplateHash = "pod-template-hash-v2"
+				s.CanaryStatus.CurrentStepState = v1beta1.CanaryStepStateUpgrade
 				cond := util.GetRolloutCondition(*s, v1beta1.RolloutConditionProgressing)
-				cond.Reason = v1alpha1.ProgressingReasonInitializing
+				cond.Reason = v1alpha1.ProgressingReasonInRolling
 				util.SetRolloutCondition(s, *cond)
+				s.CurrentStepIndex = s.CanaryStatus.CurrentStepIndex
+				s.CurrentStepState = s.CanaryStatus.CurrentStepState
+				return s
+			},
+		},
+		{
+			name: "ReconcileRolloutProgressing rolling -> continueRelease2",
+			getObj: func() ([]*apps.Deployment, []*apps.ReplicaSet) {
+				dep1 := deploymentDemo.DeepCopy()
+				dep1.Spec.Template.Spec.Containers[0].Image = "echoserver:v3"
+				dep2 := deploymentDemo.DeepCopy()
+				dep2.UID = "1ca4d850-9ec3-48bd-84cb-19f2e8cf4180"
+				dep2.Name = dep1.Name + "-canary"
+				dep2.Labels[util.CanaryDeploymentLabel] = dep1.Name
+				rs1 := rsDemo.DeepCopy()
+				rs2 := rsDemo.DeepCopy()
+				rs2.Name = "echoserver-canary-2"
+				rs2.OwnerReferences = []metav1.OwnerReference{
+					{
+						APIVersion: "apps/v1",
+						Kind:       "Deployment",
+						Name:       dep2.Name,
+						UID:        "1ca4d850-9ec3-48bd-84cb-19f2e8cf4180",
+						Controller: utilpointer.BoolPtr(true),
+					},
+				}
+				rs2.Labels["pod-template-hash"] = "pod-template-hash-v2"
+				rs2.Spec.Template.Spec.Containers[0].Image = "echoserver:v2"
+				return []*apps.Deployment{dep1, dep2}, []*apps.ReplicaSet{rs1, rs2}
+			},
+			getNetwork: func() ([]*corev1.Service, []*netv1.Ingress) {
+				return []*corev1.Service{demoService.DeepCopy()}, []*netv1.Ingress{demoIngress.DeepCopy()}
+			},
+			getRollout: func() (*v1beta1.Rollout, *v1beta1.BatchRelease, *v1alpha1.TrafficRouting) {
+				obj := rolloutDemo.DeepCopy()
+				obj.Status.CanaryStatus.ObservedWorkloadGeneration = 2
+				obj.Status.CanaryStatus.RolloutHash = "f55bvd874d5f2fzvw46bv966x4bwbdv4wx6bd9f7b46ww788954b8z8w29b7wxfd"
+				obj.Status.CanaryStatus.StableRevision = "pod-template-hash-v1"
+				obj.Status.CanaryStatus.CanaryRevision = "6f8cc56547"
+				obj.Status.CanaryStatus.CurrentStepIndex = 3
+				obj.Status.CanaryStatus.CanaryReplicas = 5
+				obj.Status.CanaryStatus.CanaryReadyReplicas = 3
+				obj.Status.CanaryStatus.PodTemplateHash = "pod-template-hash-v2"
+				obj.Status.CanaryStatus.CurrentStepState = v1beta1.CanaryStepStateUpgrade
+				obj.Status.CanaryStatus.FinalisingStep = v1beta1.FinalisingStepTypeDeleteCanaryService
+				cond := util.GetRolloutCondition(obj.Status, v1beta1.RolloutConditionProgressing)
+				cond.Reason = v1alpha1.ProgressingReasonInRolling
+				util.SetRolloutCondition(&obj.Status, *cond)
+				return obj, nil, nil
+			},
+			expectStatus: func() *v1beta1.RolloutStatus {
+				s := rolloutDemo.Status.DeepCopy()
+				s.Clear()
 				return s
 			},
 		},

pkg/controller/rollout/rollout_releaseManager.go

@@ -1,13 +1,34 @@
+/*
+Copyright 2022 The Kruise Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package rollout
 
 import (
 	"github.com/openkruise/rollouts/api/v1beta1"
 )
 
 type ReleaseManager interface {
+	// execute the core logic of a release step by step
 	runCanary(c *RolloutContext) error
+	// check the NextStepIndex field in status, if modifies detected, jump to target step
 	doCanaryJump(c *RolloutContext) bool
+	// called when user accomplishes a release / does a rollback, or disables/removes the Rollout Resource
 	doCanaryFinalising(c *RolloutContext) (bool, error)
+	// fetch the BatchRelease object
 	fetchBatchRelease(ns, name string) (*v1beta1.BatchRelease, error)
+	// remove the BatchRelease object
 	removeBatchRelease(c *RolloutContext) (bool, error)
 }