Allow deployment rollback when in the process of cannary/partition (#91)

add comment



an improvement for canary rollback during another rollback

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

pkg/internal/polymorphichelpers/rollback.go

@@ -28,6 +28,7 @@ import (
 	kruiseclientsets "github.com/openkruise/kruise-api/client/clientset/versioned"
 	internalapps "github.com/openkruise/kruise-tools/pkg/internal/apps"
 
+	utils "github.com/openkruise/kruise-tools/pkg/utils"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
@@ -143,9 +144,6 @@ func (r *DeploymentRollbacker) Rollback(obj runtime.Object, updatedAnnotations m
 	if dryRunStrategy == cmdutil.DryRunClient {
 		return printTemplate(&rsForRevision.Spec.Template)
 	}
-	if deployment.Spec.Paused {
-		return "", fmt.Errorf("you cannot rollback a paused deployment; resume it first with 'kubectl rollout resume deployment/%s' and try again", name)
-	}
 
 	// Skip if the revision already matches current Deployment
 	if equalIgnoreHash(&rsForRevision.Spec.Template, &deployment.Spec.Template) {
@@ -157,13 +155,18 @@ func (r *DeploymentRollbacker) Rollback(obj runtime.Object, updatedAnnotations m
 
 	// compute deployment annotations
 	annotations := map[string]string{}
-	for k := range annotationsToSkip {
-		if v, ok := deployment.Annotations[k]; ok {
+
+	// In the same vein as annotationsToSkip, which records annotations to exclude,
+	// IsKruiseRolloutsAnnotation checks whether an annotation is generated by kruise-rollout.
+	// Annotations identified as generated by kruise-rollout
+	// will be skipped when copying from ReplicaSet annotations to Deployment annotations.
+	for k, v := range deployment.Annotations {
+		if annotationsToSkip[k] || utils.IsKruiseRolloutsAnnotation(&k) {
 			annotations[k] = v
 		}
 	}
 	for k, v := range rsForRevision.Annotations {
-		if !annotationsToSkip[k] {
+		if !annotationsToSkip[k] && !utils.IsKruiseRolloutsAnnotation(&k) {
 			annotations[k] = v
 		}
 	}
@@ -270,6 +273,50 @@ func deploymentRevision(deployment *appsv1.Deployment, c kubernetes.Interface, t
 		return nil, revisionNotFoundErr(toRevision)
 	}
 
+	// even a deployment is paused, rollout history can update if the current rollout progress is
+	// actually a rollback, for example:
+
+	// Step1. Initially, there's only one revision for the deployment:
+	// REVISION  CHANGE-CAUSE
+	// 1         <none>
+	// before the first canary release is completed, the rollout history won't update since the original deployment
+	// is paused. If someone decides to rollback during this release (before it is completed),
+	// we should return the latestReplicaSet instead of the previousReplicaSet.
+
+	// Step2. After a canary release completed, the rollout history will be like:
+	// REVISION  CHANGE-CAUSE
+	// 1       	 <none>
+	// 2         <none>
+
+	// Step3. Someone decides to rollback after this release using rollout undo.
+	// It is ok, and it will be seen as a new canary release.
+	// However, the rollout history will update though the original deployment is paused, because
+	// the "new" canary revision can be found from the rollout history, which will be like:
+	// REVISION  CHANGE-CAUSE
+	// 2         <none>
+	// 3         <none>
+
+	// Step4. Someone decides to rollback during this rollback progress (before it is completed),
+	// that means rolling back to the revision referred in the Step2.
+	// We should return the previousReplicaSet instead of the latestReplicaSet.
+	if utils.InCanaryProgress(deployment) {
+		if latestReplicaSet == nil {
+			return nil, fmt.Errorf("no rollout history found for deployment %q", deployment.Name)
+		}
+		// only one revison found, then return it, equalIgnoreHash will be called later in Rollback function
+		if previousReplicaSet == nil {
+			return latestReplicaSet, nil
+		}
+		// possibly attempting to rollback during a rollback progress, so we return previousReplicaSet,
+		// since the rollout history is updated
+		if equalIgnoreHash(&latestReplicaSet.Spec.Template, &deployment.Spec.Template) {
+			return previousReplicaSet, nil
+		}
+		// attempting to rollback during a normal publication,
+		// so we return latestReplicaSet since the rollout history hasn't updated
+		return latestReplicaSet, nil
+	}
+
 	if previousReplicaSet == nil {
 		return nil, fmt.Errorf("no rollout history found for deployment %q", deployment.Name)
 	}

pkg/utils/misc.go

@@ -0,0 +1,33 @@
+package utils
+
+import (
+	"strings"
+
+	appsv1 "k8s.io/api/apps/v1"
+)
+
+const InRolloutProgressingAnnotation = "rollouts.kruise.io/in-progressing"
+const DeploymentStrategyAnnotation = "rollouts.kruise.io/deployment-strategy"
+
+func IsKruiseRolloutsAnnotation(s *string) bool {
+	if s == nil {
+		return false
+	}
+	const prefix = "rollouts.kruise.io/"
+	return strings.Contains(*s, prefix)
+}
+
+func InCanaryProgress(deployment *appsv1.Deployment) bool {
+	if !deployment.Spec.Paused {
+		return false
+	}
+	//if deployment has InRolloutProgressingAnnotation, it is under kruise-rollout control
+	if _, ok := deployment.Annotations[InRolloutProgressingAnnotation]; !ok {
+		return false
+	}
+	// only if deployment strategy is 'partition', webhook would add DeploymentStrategyAnnotation
+	if _, ok := deployment.Annotations[DeploymentStrategyAnnotation]; ok {
+		return false
+	}
+	return true
+}