Fix the issue where the BatchRelease gets stuck in Verifying when the Deployment's PodTemplate already contains pod-template-hash label

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>

Author: AiRanthem

pkg/controller/batchrelease/labelpatch/patcher.go

@@ -18,6 +18,7 @@ package labelpatch
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strconv"
 
@@ -84,18 +85,31 @@ func (r *realPatcher) patchPodBatchLabel(pods []*corev1.Pod, ctx *batchcontext.B
 			// on the pod-template-hash is not recommended. Thus, we use the CloneSet algorithm to compute the
 			// controller-revision-hash: this method is fully defined by OpenKruise and can ensure that the same
 			// ReplicaSet produces the same value.
-			owner := metav1.GetControllerOf(pod)
+			owner := metav1.GetControllerOfNoCopy(pod)
 			if owner != nil && owner.Kind == "ReplicaSet" {
 				var hash string
 				if cache, ok := revisionHashCache[owner.UID]; ok {
 					hash = cache
 				} else {
 					rs := &v1.ReplicaSet{}
 					if err := r.Get(context.Background(), types.NamespacedName{Namespace: pod.Namespace, Name: owner.Name}, rs); err != nil {
-						klog.ErrorS(err, "Failed to get ReplicaSet", "pod", klog.KObj(pod), "rollout", r.logKey, "owner", owner.Name, "namespace", pod.Namespace)
+						klog.ErrorS(err, "Failed to get ReplicaSet when patching pod", "pod", klog.KObj(pod), "rollout", r.logKey, "owner", owner.Name, "namespace", pod.Namespace)
 						return err
 					}
-					delete(rs.Spec.Template.ObjectMeta.Labels, v1.DefaultDeploymentUniqueLabelKey)
+					rsOwner := metav1.GetControllerOfNoCopy(rs)
+					if rsOwner == nil || rsOwner.Kind != "Deployment" {
+						klog.ErrorS(nil, "ReplicaSet has no deployment owner, skip patching")
+						return errors.New("ReplicaSet has no deployment owner")
+					}
+					deploy := &v1.Deployment{}
+					if err := r.Get(context.Background(), types.NamespacedName{Namespace: pod.Namespace, Name: rsOwner.Name}, deploy); err != nil {
+						klog.ErrorS(err, "Failed to get Deployment when patching pod", "pod", klog.KObj(pod), "rollout", r.logKey, "owner", owner.Name, "namespace", pod.Namespace)
+						return err
+					}
+
+					if deploy.Spec.Template.Labels[v1.DefaultDeploymentUniqueLabelKey] == "" {
+						delete(rs.Spec.Template.ObjectMeta.Labels, v1.DefaultDeploymentUniqueLabelKey)
+					}
 					hash = util.ComputeHash(&rs.Spec.Template, nil)
 					revisionHashCache[owner.UID] = hash
 				}

pkg/controller/batchrelease/labelpatch/patcher_test.go

@@ -28,10 +28,12 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -269,6 +271,16 @@ func TestDeploymentPatch(t *testing.T) {
 	rs := &appsv1.ReplicaSet{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "rs-1",
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion:         "apps/v1",
+					Kind:               "Deployment",
+					Name:               "deploy-1",
+					UID:                types.UID("deploy-1"),
+					Controller:         ptr.To(true),
+					BlockOwnerDeletion: ptr.To(true),
+				},
+			},
 		},
 		Spec: appsv1.ReplicaSetSpec{
 			Template: corev1.PodTemplateSpec{
@@ -288,6 +300,34 @@ func TestDeploymentPatch(t *testing.T) {
 			},
 		},
 	}
+	deploy := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "deploy-1",
+		},
+		Spec: appsv1.DeploymentSpec{
+			Replicas: ptr.To(int32(10)),
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"app": "nginx",
+				},
+			},
+			Template: corev1.PodTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"app": "nginx",
+					},
+				},
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name:  "nginx",
+							Image: "nginx:1.14.2",
+						},
+					},
+				},
+			},
+		},
+	}
 	revision := util.ComputeHash(&rs.Spec.Template, nil)
 	// randomly inserted to test cases, pods with this revision should be skipped and
 	// the result should not be influenced
@@ -296,6 +336,8 @@ func TestDeploymentPatch(t *testing.T) {
 		batchContext    func() *batchcontext.BatchContext
 		Batches         []v1beta1.ReleaseBatch
 		expectedPatched []int
+		modifier        func(rs *appsv1.ReplicaSet, deploy *appsv1.Deployment)
+		expectErr       bool
 	}{
 		"10 pods, 0 patched, 5 new patched": {
 			batchContext: func() *batchcontext.BatchContext {
@@ -476,6 +518,54 @@ func TestDeploymentPatch(t *testing.T) {
 			},
 			expectedPatched: []int{3, 2},
 		},
+		"rs with no controller": {
+			batchContext: func() *batchcontext.BatchContext {
+				ctx := &batchcontext.BatchContext{
+					RolloutID:              "rollout-1",
+					UpdateRevision:         revision,
+					PlannedUpdatedReplicas: 5,
+					CurrentBatch:           1,
+					Replicas:               10,
+				}
+				pods := generateDeploymentPods(1, 5, 3,
+					"rollout-1", strconv.Itoa(1))
+				ctx.Pods = pods
+				return ctx
+			},
+			Batches: []v1beta1.ReleaseBatch{
+				{CanaryReplicas: intstr.FromInt(2)},
+				{CanaryReplicas: intstr.FromInt(5)},
+			},
+			expectedPatched: []int{3, 2},
+			expectErr:       true,
+			modifier: func(rs *appsv1.ReplicaSet, deploy *appsv1.Deployment) {
+				rs.OwnerReferences = nil
+			},
+		},
+		"failed to get deployment": {
+			batchContext: func() *batchcontext.BatchContext {
+				ctx := &batchcontext.BatchContext{
+					RolloutID:              "rollout-1",
+					UpdateRevision:         revision,
+					PlannedUpdatedReplicas: 5,
+					CurrentBatch:           1,
+					Replicas:               10,
+				}
+				pods := generateDeploymentPods(1, 5, 3,
+					"rollout-1", strconv.Itoa(1))
+				ctx.Pods = pods
+				return ctx
+			},
+			Batches: []v1beta1.ReleaseBatch{
+				{CanaryReplicas: intstr.FromInt(2)},
+				{CanaryReplicas: intstr.FromInt(5)},
+			},
+			expectedPatched: []int{3, 2},
+			expectErr:       true,
+			modifier: func(rs *appsv1.ReplicaSet, deploy *appsv1.Deployment) {
+				deploy.Name = "not-exist"
+			},
+		},
 	}
 	for name, cs := range cases {
 		t.Run(name, func(t *testing.T) {
@@ -504,12 +594,22 @@ func TestDeploymentPatch(t *testing.T) {
 			for _, pod := range ctx.Pods {
 				objects = append(objects, pod)
 			}
-			objects = append(objects, rs)
+			rsCopy, deployCopy := rs.DeepCopy(), deploy.DeepCopy()
+			if cs.modifier != nil {
+				cs.modifier(rsCopy, deployCopy)
+			}
+			objects = append(objects, rsCopy, deployCopy)
 			cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
 			patcher := NewLabelPatcher(cli, klog.ObjectRef{Name: "test"}, cs.Batches)
 			if err := patcher.patchPodBatchLabel(ctx.Pods, ctx); err != nil {
+				if cs.expectErr {
+					return
+				}
 				t.Fatalf("failed to patch pods: %v", err)
 			}
+			if cs.expectErr {
+				t.Fatalf("errors should occur")
+			}
 
 			podList := &corev1.PodList{}
 			if err := cli.List(context.TODO(), podList); err != nil {

test/e2e/rollout_test.go

@@ -414,6 +414,7 @@ var _ = SIGDescribe("Rollout", func() {
 			workload.Spec.Template.Annotations = map[string]string{
 				"pod": "stable",
 			}
+			workload.Spec.Template.Labels[apps.DefaultDeploymentUniqueLabelKey] = "abcdefg"
 			CreateObject(workload)
 			WaitDeploymentAllPodsReady(workload)