diff --git a/pkg/internal/polymorphichelpers/rollback.go b/pkg/internal/polymorphichelpers/rollback.go index 263b12b..478bfc3 100644 --- a/pkg/internal/polymorphichelpers/rollback.go +++ b/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 + // 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 + // 2 + + // 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 + // 3 + + // 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) } diff --git a/pkg/utils/misc.go b/pkg/utils/misc.go new file mode 100644 index 0000000..365cede --- /dev/null +++ b/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 +}