diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 497e82cb1d9..cfd147b5072 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -318,35 +318,40 @@ func (dc *DeploymentController) getNewReplicaSet(d *extensions.Deployment, rsLis createdRS, err := dc.client.ExtensionsV1beta1().ReplicaSets(d.Namespace).Create(&newRS) switch { // We may end up hitting this due to a slow cache or a fast resync of the Deployment. - // Fetch a copy of the ReplicaSet. If its PodTemplateSpec is semantically deep equal - // with the PodTemplateSpec of the Deployment, then that is our new ReplicaSet. Otherwise, - // this is a hash collision and we need to increment the collisionCount field in the - // status of the Deployment and try the creation again. case errors.IsAlreadyExists(err): alreadyExists = true + + // Fetch a copy of the ReplicaSet. rs, rsErr := dc.rsLister.ReplicaSets(newRS.Namespace).Get(newRS.Name) if rsErr != nil { return nil, rsErr } + + // If the Deployment owns the ReplicaSet and the ReplicaSet's PodTemplateSpec is semantically + // deep equal to the PodTemplateSpec of the Deployment, it's the Deployment's new ReplicaSet. + // Otherwise, this is a hash collision and we need to increment the collisionCount field in + // the status of the Deployment and requeue to try the creation in the next sync. + controllerRef := metav1.GetControllerOf(rs) + if controllerRef != nil && controllerRef.UID == d.UID && deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template) { + createdRS = rs + err = nil + break + } + // Matching ReplicaSet is not equal - increment the collisionCount in the DeploymentStatus // and requeue the Deployment. - if !deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template) { - if d.Status.CollisionCount == nil { - d.Status.CollisionCount = new(int32) - } - preCollisionCount := *d.Status.CollisionCount - *d.Status.CollisionCount++ - // Update the collisionCount for the Deployment and let it requeue by returning the original - // error. - _, dErr := dc.client.ExtensionsV1beta1().Deployments(d.Namespace).UpdateStatus(d) - if dErr == nil { - glog.V(2).Infof("Found a hash collision for deployment %q - bumping collisionCount (%d->%d) to resolve it", d.Name, preCollisionCount, *d.Status.CollisionCount) - } - return nil, err + if d.Status.CollisionCount == nil { + d.Status.CollisionCount = new(int32) } - // Pass through the matching ReplicaSet as the new ReplicaSet. - createdRS = rs - err = nil + preCollisionCount := *d.Status.CollisionCount + *d.Status.CollisionCount++ + // Update the collisionCount for the Deployment and let it requeue by returning the original + // error. + _, dErr := dc.client.ExtensionsV1beta1().Deployments(d.Namespace).UpdateStatus(d) + if dErr == nil { + glog.V(2).Infof("Found a hash collision for deployment %q - bumping collisionCount (%d->%d) to resolve it", d.Name, preCollisionCount, *d.Status.CollisionCount) + } + return nil, err case err != nil: msg := fmt.Sprintf("Failed to create new replica set %q: %v", newRS.Name, err) if d.Spec.ProgressDeadlineSeconds != nil { diff --git a/test/integration/deployment/deployment_test.go b/test/integration/deployment/deployment_test.go index bfa16d0d521..f663741b5b2 100644 --- a/test/integration/deployment/deployment_test.go +++ b/test/integration/deployment/deployment_test.go @@ -1379,3 +1379,122 @@ func TestDeploymentScaleSubresource(t *testing.T) { // Use the scale subresource to scale the deployment down to 0 testScalingUsingScaleSubresource(t, tester, 0) } + +// This test verifies that the Deployment does orphan a ReplicaSet when the ReplicaSet's +// .Labels field is changed to no longer match the Deployment's selector. It also partially +// verifies that collision avoidance mechanism is triggered when a Deployment's new ReplicaSet +// is orphaned, even without PodTemplateSpec change. Refer comment below for more info: +// https://github.com/kubernetes/kubernetes/pull/59212#discussion_r166465113 +func TestReplicaSetOrphaningAndAdoptionWhenLabelsChange(t *testing.T) { + s, closeFn, rm, dc, informers, c := dcSetup(t) + defer closeFn() + name := "test-replicaset-orphaning-and-adoption-when-labels-change" + ns := framework.CreateTestingNamespace(name, s, t) + defer framework.DeleteTestingNamespace(ns, s, t) + + deploymentName := "deployment" + replicas := int32(1) + tester := &deploymentTester{t: t, c: c, deployment: newDeployment(deploymentName, ns.Name, replicas)} + var err error + tester.deployment, err = c.ExtensionsV1beta1().Deployments(ns.Name).Create(tester.deployment) + if err != nil { + t.Fatalf("failed to create deployment %q: %v", deploymentName, err) + } + + // Start informer and controllers + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + go rm.Run(5, stopCh) + go dc.Run(5, stopCh) + + // Wait for the Deployment to be updated to revision 1 + if err := tester.waitForDeploymentRevisionAndImage("1", fakeImage); err != nil { + t.Fatal(err) + } + + // Ensure the deployment completes while marking its pods as ready simultaneously + if err := tester.waitForDeploymentCompleteAndMarkPodsReady(); err != nil { + t.Fatal(err) + } + + // Orphaning: deployment should remove OwnerReference from a RS when the RS's labels change to not match its labels + + // Get replicaset of the deployment + rs, err := deploymentutil.GetNewReplicaSet(tester.deployment, c.ExtensionsV1beta1()) + if err != nil { + t.Fatalf("failed to get replicaset of deployment %q: %v", deploymentName, err) + } + if rs == nil { + t.Fatalf("unable to find replicaset of deployment %q", deploymentName) + } + + // Verify controllerRef of the replicaset is not nil and pointing to the deployment + controllerRef := metav1.GetControllerOf(rs) + if controllerRef == nil { + t.Fatalf("controllerRef of replicaset %q is nil", rs.Name) + } + if controllerRef.UID != tester.deployment.UID { + t.Fatalf("controllerRef of replicaset %q has a different UID: Expected %v, got %v", rs.Name, tester.deployment.UID, controllerRef.UID) + } + + // Change the replicaset's labels to not match the deployment's labels + labelMap := map[string]string{"new-name": "new-test"} + rs, err = tester.updateReplicaSet(rs.Name, func(update *v1beta1.ReplicaSet) { + update.Labels = labelMap + }) + if err != nil { + t.Fatalf("failed to update replicaset %q: %v", rs.Name, err) + } + + // Wait for the controllerRef of the replicaset to become nil + rsClient := tester.c.ExtensionsV1beta1().ReplicaSets(ns.Name) + if err = wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { + rs, err = rsClient.Get(rs.Name, metav1.GetOptions{}) + if err != nil { + return false, err + } + return metav1.GetControllerOf(rs) == nil, nil + }); err != nil { + t.Fatalf("failed to wait for controllerRef of replicaset %q to become nil: %v", rs.Name, err) + } + + // Wait for the deployment to create a new replicaset + // This will trigger collision avoidance due to deterministic nature of replicaset name + // i.e., the new replicaset will have a name with different hash to preserve name uniqueness + var newRS *v1beta1.ReplicaSet + if err = wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { + newRS, err = deploymentutil.GetNewReplicaSet(tester.deployment, c.ExtensionsV1beta1()) + if err != nil { + return false, fmt.Errorf("failed to get new replicaset of deployment %q after orphaning: %v", deploymentName, err) + } + return newRS != nil, nil + }); err != nil { + t.Fatalf("failed to wait for deployment %q to create a new replicaset after orphaning: %v", deploymentName, err) + } + if newRS.UID == rs.UID { + t.Fatalf("expect deployment %q to create a new replicaset different from the orphaned one, but it isn't", deploymentName) + } + + // Adoption: deployment should add controllerRef to a RS when the RS's labels change to match its labels + + // Change the old replicaset's labels to match the deployment's labels + rs, err = tester.updateReplicaSet(rs.Name, func(update *v1beta1.ReplicaSet) { + update.Labels = testLabels() + }) + if err != nil { + t.Fatalf("failed to update replicaset %q: %v", rs.Name, err) + } + + // Wait for the deployment to adopt the old replicaset + if err = wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { + rs, err := rsClient.Get(rs.Name, metav1.GetOptions{}) + if err != nil { + return false, err + } + controllerRef = metav1.GetControllerOf(rs) + return controllerRef != nil && controllerRef.UID == tester.deployment.UID, nil + }); err != nil { + t.Fatalf("failed waiting for replicaset adoption by deployment %q to complete: %v", deploymentName, err) + } +}