ReplicaSet has onwer ref of the Deployment that created it #35676

krmayankk · 2016-10-27T07:04:27Z

What this PR does / why we need it:
This enabled garbage collection for ReplicaSets and ensures they are owned by their respective Deployment objects.

fixes #33845

This is an initial PR to get feedback. Will update this quickly with unit tests if this seems like in the right direction

This change is

krmayankk · 2016-10-27T07:05:52Z

@caesarxuchao @Kargakis @janetkuo ptal. If this is in the right direction, please provide more inputs as to how to test this locally. I just ran unit tests.

0xmichalis · 2016-10-27T09:49:56Z

pkg/controller/deployment/deployment_controller.go

@@ -355,6 +371,44 @@ func (dc *DeploymentController) syncDeployment(key string) error {
 		return err
 	}

+	if dc.garbageCollectorEnabled {
+		rsList, err := dc.rsLister.ReplicaSets(deployment.Namespace).List(labels.Everything())


Can you move all of this in a separate function dc.classifyReplicaSets or something so we can keep the main sync handler clean?

0xmichalis · 2016-10-27T09:51:44Z

We should also set owner references by default on creation of a replica set.

0xmichalis · 2016-10-27T09:51:59Z

@kubernetes/deployment

0xmichalis · 2016-10-27T09:54:04Z

pkg/controller/controller_ref_manager.go

+	return &ReplicaSetControllerRefManager{rsControl, controllerObject, controllerSelector, controllerKind}
+}
+
+// Classify first filters out inactive ReplicaSets, then it classify the remaining ReplicaSets


Inactive pods will be garbage-collected by the podGC controller. We don't have anything similar for replica sets so I don't think we should filter out any replica sets at this point - @caesarxuchao to keep me correct

In addition to what @Kargakis said, I filtered out inactive pods because that was the old behavior of replicaset controller , I did the filtering in my PR to respect the old behavior. So we don't need to classify inactive replicaset

Right, we don't need to filter inactive ReplicaSet. We don't define what "inactive" mean for RS.

We already use "inactive" in some places for replica sets with no pods but it's nothing we have agreed on.

I see. Still, no need to filter them.

yep removed the function and comment

0xmichalis · 2016-10-27T09:56:12Z

pkg/controller/deployment/deployment_controller.go

+		cm := controller.NewReplicaSetControllerRefManager(dc.rsControl, deployment.ObjectMeta, deploymentSelector, getDeploymentControllerKind())
+		matchesAndControlled, matchesNeedsController, controlledDoesNotMatch := cm.Classify(rsList)
+		for _, replicaSet := range matchesNeedsController {
+			err := cm.AdoptReplicaSet(replicaSet)


These replica sets are coming from a shared cache so you need to deep-copy before mutating them.

This function should be readonly, but @krmayankk could you double check? If it's readonly, could you add a comment here?

AdoptReplicaSet seems to patch the owner reference in the replica set.

It generates a patch and sends the patch to server. It doesn't change anything locally.

yes, its readonly

Now I see it is used just for constructing the patch. ok

0xmichalis · 2016-10-27T09:56:36Z

pkg/controller/deployment/deployment_controller.go

+		// remove the controllerRef for the RS that no longer have matching labels
+		var errlist []error
+		for _, replicaSet := range controlledDoesNotMatch {
+			err := cm.ReleaseReplicaSet(replicaSet)


Same comment as above. Deep-copy before mutating.

Both Adopt/Release are readonly for the object being passed to its arguments

0xmichalis · 2016-10-27T09:58:06Z

pkg/controller/controller_ref_manager.go

+// are the ReplicaSets that have a controllerRef pointing to the controller, but their
+// labels no longer match the selector.
+func (m *ReplicaSetControllerRefManager) Classify(replicaSets []*extensions.ReplicaSet) (
+	matchesAndControlled []*extensions.ReplicaSet,


I would advise against the use of named returns since it's harder to process the function if it has multiple return calls.

I went through a long debate with lavalamp and wojtek-t last time. The names make the signature more readable. I remember we agreed if we make all returns explicit then having named return variables is not a problem.

0xmichalis · 2016-10-27T09:58:27Z

pkg/controller/controller_ref_manager.go

+	controlledDoesNotMatch []*extensions.ReplicaSet) {
+	for i := range replicaSets {
+		replicaSet := replicaSets[i]
+		// TODO: Do we need to ignore inactive RS


I don't think so, see https://github.com/kubernetes/kubernetes/pull/35676/files#r85303683

removed the comment

0xmichalis · 2016-10-27T10:11:05Z

pkg/controller/controller_utils.go

@@ -705,6 +725,10 @@ func FilterActivePods(pods []*api.Pod) []*api.Pod {
 	return result
 }

+func IsReplicaSetActive(r *extensions.ReplicaSet) bool {


You probably don't need this.

0xmichalis · 2016-10-27T10:14:30Z

pkg/controller/deployment/deployment_controller.go

@@ -56,9 +59,14 @@ const (
 	MaxRetries = 5
 )

+func getDeploymentControllerKind() unversioned.GroupVersionKind {
+	return v1beta1.SchemeGroupVersion.WithKind("DeploymentController")


Probably needs to be "Deployment"

Yes, should be "Deployment".

0xmichalis · 2016-10-27T10:23:25Z

pkg/controller/deployment/deployment_controller.go

+	if dc.garbageCollectorEnabled {
+		rsList, err := dc.rsLister.ReplicaSets(deployment.Namespace).List(labels.Everything())
+		if err != nil {
+			glog.Errorf("Error getting ReplicaSets for deployment %q: %v", deployment, err)


Change deployment to d.Name in the arguments.

0xmichalis · 2016-10-27T10:24:54Z

pkg/controller/deployment/deployment_controller.go

@@ -56,9 +59,14 @@ const (
 	MaxRetries = 5
 )

+func getDeploymentControllerKind() unversioned.GroupVersionKind {


Instead of using a separate function, inline the body in the caller site.

not sure i understand the reason for this. Are we saying its just used in one place and hence we should do this way. Keeping it consistent with getRSControllerKind() for now

This is an unnecessary indirection in the code and the deployment controller isn't the easiest one to maintain so the simpler things are the better.

I see you now use it elsewhere too. ok

caesarxuchao

I'll finish the rest tonight.

caesarxuchao · 2016-10-27T22:19:03Z

pkg/controller/controller_ref_manager.go

+	return &ReplicaSetControllerRefManager{rsControl, controllerObject, controllerSelector, controllerKind}
+}
+
+// Classify first filters out inactive ReplicaSets, then it classify the remaining ReplicaSets


In addition to what @Kargakis said, I filtered out inactive pods because that was the old behavior of replicaset controller , I did the filtering in my PR to respect the old behavior. So we don't need to classify inactive replicaset

caesarxuchao · 2016-10-28T00:30:19Z

pkg/controller/controller_ref_manager.go

+	return &ReplicaSetControllerRefManager{rsControl, controllerObject, controllerSelector, controllerKind}
+}
+
+// Classify first filters out inactive ReplicaSets, then it classify the remaining ReplicaSets


Right, we don't need to filter inactive ReplicaSet. We don't define what "inactive" mean for RS.

caesarxuchao · 2016-10-28T00:33:56Z

pkg/controller/controller_ref_manager.go

+// are the ReplicaSets that have a controllerRef pointing to the controller, but their
+// labels no longer match the selector.
+func (m *ReplicaSetControllerRefManager) Classify(replicaSets []*extensions.ReplicaSet) (
+	matchesAndControlled []*extensions.ReplicaSet,


I went through a long debate with lavalamp and wojtek-t last time. The names make the signature more readable. I remember we agreed if we make all returns explicit then having named return variables is not a problem.

caesarxuchao · 2016-10-28T05:36:54Z

pkg/controller/controller_ref_manager.go

+func (m *ReplicaSetControllerRefManager) AdoptReplicaSet(replicaSet *extensions.ReplicaSet) error {
+	// we should not adopt any ReplicaSets if the controller is about to be deleted
+	if m.controllerObject.DeletionTimestamp != nil {
+		return fmt.Errorf("cancel the adopt attempt for RS %s because the controlller is being deleted",


Could you print out the controller's name as well? It'd be useful for debugging.

caesarxuchao · 2016-10-28T05:40:48Z

pkg/controller/controller_ref_manager.go

+// ReleaseReplicaSet sends a patch to free the ReplicaSet from the control of the Deployment controller.
+// It returns the error if the patching fails. 404 and 422 errors are ignored.
+func (m *ReplicaSetControllerRefManager) ReleaseReplicaSet(replicaSet *extensions.ReplicaSet) error {
+	glog.V(2).Infof("patching pod %s_%s to remove its controllerRef to %s/%s:%s",


s/pod/replicaset

caesarxuchao · 2016-10-28T05:41:42Z

pkg/controller/controller_ref_manager.go

+	err := m.rsControl.PatchReplicaSet(replicaSet.Namespace, replicaSet.Name, []byte(deleteOwnerRefPatch))
+	if err != nil {
+		if errors.IsNotFound(err) {
+			// If the ReplicaSet  no longer exists, ignore it.


nit: remove the extra space

caesarxuchao · 2016-10-28T05:46:08Z

pkg/controller/controller_ref_manager.go

+			// match, which means the ReplicaSet is deleted and then recreated.
+			// In both cases, the error can be ignored.
+
+			// TODO: If the ReplicaSet has owner references, but none of them


nit: no need to copy the TODO

caesarxuchao · 2016-10-28T05:48:06Z

pkg/controller/deployment/deployment_controller.go

@@ -56,9 +59,14 @@ const (
 	MaxRetries = 5
 )

+func getDeploymentControllerKind() unversioned.GroupVersionKind {
+	return v1beta1.SchemeGroupVersion.WithKind("DeploymentController")


Yes, should be "Deployment".

caesarxuchao · 2016-10-28T05:52:28Z

pkg/controller/deployment/deployment_controller.go

@@ -84,10 +92,13 @@ type DeploymentController struct {

 	// Deployments that need to be synced
 	queue workqueue.RateLimitingInterface
+
+	// garbageCollectorEnabled denotes if the garbage collector is enabled.
+	garbageCollectorEnabled bool


I don't think garbageCollectorEnabled is needed anymore. If GC is disabled, adding controllerRef won't break the system, and it still has the benefit of preventing multiple controllers contend for a controllee.

I added garbageCollectorEnabled in RS controller because GC was still alpha (now it's beta) at that time, and we wanted to have a safety button to disable every GC related features.

caesarxuchao · 2016-10-28T05:57:55Z

pkg/controller/deployment/deployment_controller.go

+		cm := controller.NewReplicaSetControllerRefManager(dc.rsControl, deployment.ObjectMeta, deploymentSelector, getDeploymentControllerKind())
+		matchesAndControlled, matchesNeedsController, controlledDoesNotMatch := cm.Classify(rsList)
+		for _, replicaSet := range matchesNeedsController {
+			err := cm.AdoptReplicaSet(replicaSet)


This function should be readonly, but @krmayankk could you double check? If it's readonly, could you add a comment here?

caesarxuchao · 2016-10-28T06:03:35Z

pkg/controller/deployment/deployment_controller.go

+			return utilerrors.NewAggregate(errlist)
+		}
+
+	}


I don't know deployement controller well enough. If the number of controlled replicaset is greater than the max rollback history, will another routine try to delete the surplus? @janetkuo @Kargakis

caesarxuchao · 2016-10-28T06:10:52Z

Thanks @krmayankk! Generally looks good. OwnerReferences affects garbage collection behavior (user doc), so could you check these:

Could you add test to check if garbage collection work as expected? i.e., when deleting the deployement, will the RS and RS's pods be deleted? * If DeleteOptions.OrphanDependents=true, will the RS be orphaned? See https://github.com/kubernetes/kubernetes/blob/master/test/e2e/garbage_collector.go as a reference.
You need to change the registry for the deployment, see the similar changes for RS.

0xmichalis · 2016-10-28T12:31:58Z

I don't think garbageCollectorEnabled is needed anymore. If GC is disabled, adding controllerRef won't break the system, and it still has the benefit of preventing multiple controllers contend for a controllee.

+1. We need to use owner references in a couple of places apart from garbage collecction.

krmayankk · 2016-12-21T17:43:20Z

@caesarxuchao @Kargakis the second e2e test i added is failing. i am debugging this. If any of you have some pointers let me know. It seems to always timing out the Poll.

caesarxuchao · 2016-12-21T19:32:01Z

Is this flaky or does it always fail? Can you print out the deployment as well? This could be a bug in the garbage collector itself.

krmayankk · 2016-12-22T09:40:20Z

@caesarxuchao its always failing. I have added debug infor for deployment as well. hoping to see some clue there.

k8s-ci-robot · 2016-12-22T10:18:27Z

Jenkins GCE etcd3 e2e failed for commit 363caa8b19c5cd416e0f570b1f66658b52b4077f. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-22T10:20:07Z

Jenkins GCI GCE e2e failed for commit 363caa8b19c5cd416e0f570b1f66658b52b4077f. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao · 2016-12-22T19:47:31Z

@k8s-bot cvm gce e2e test this

k8s-ci-robot · 2016-12-22T20:26:20Z

Jenkins GCE e2e failed for commit 363caa8b19c5cd416e0f570b1f66658b52b4077f. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-22T23:06:15Z

Jenkins Bazel Build failed for commit ceed344. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao

Some nits, otherwise lgtm. Hopefully we can get this lgtm'ed today!

caesarxuchao · 2016-12-22T23:11:59Z

test/e2e/garbage_collector.go

 		},
 	}
 }

+// verifyRemainingDeploymentsAndReplicaSets verifies if the number of the remaining deployment


nit: can you realign the newlines?

caesarxuchao · 2016-12-22T23:16:21Z

test/e2e/garbage_collector.go

+		return false, fmt.Errorf("Failed to list rs: %v", err)
+	}
+
+	var ret = true


nit^2: move this to the start of the function, remove the newline, so that the code looks symmetric.

caesarxuchao · 2016-12-22T23:18:58Z

test/e2e/garbage_collector.go

+				if err != nil {
+					return false, fmt.Errorf("Failed to list rs: %v", err)
+				}
+				// We know there is at least one RS


Can you make a for loop over rs.Items? and fail if any RS's ownerRefs are not cleaned up.

caesarxuchao · 2016-12-22T23:22:32Z

test/e2e/garbage_collector.go

+		err = wait.PollImmediate(5*time.Second, 2*time.Minute, func() (bool, error) {
+			ret, err := verifyRemainingDeploymentsAndReplicaSets(f, clientSet, deployment, 0, 1)
+			if err != nil {
+				return ret, err


nit: I think the convention is return false, err.

caesarxuchao · 2016-12-22T23:24:26Z

test/e2e/garbage_collector.go

+			if err != nil {
+				return ret, err
+			}
+			if ret {


Can you move this entire block out of the Poll()? Deployment shouldn't be deleted if the RS.ownerRefs are not cleaned up yet.

caesarxuchao · 2016-12-22T23:56:48Z

This PR lgtm after the nits are addressed. I'll be OOO for the next couple of weeks. @Kargakis could you give the lgtm if I don't get the chance? Thank you all for getting this done!

caesarxuchao · 2016-12-23T00:54:39Z

LGTM. Thank you!

k8s-ci-robot · 2016-12-23T00:59:28Z

Jenkins GKE smoke e2e failed for commit 7779776. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-12-23T02:50:08Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-12-23T02:50:31Z

Automatic merge from submit-queue (batch tested with PRs 39059, 39175, 35676, 38655)

googlebot added the cla: yes label Oct 27, 2016

k8s-github-robot assigned derekwaynecarr Oct 27, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 27, 2016

0xmichalis assigned 0xmichalis and unassigned derekwaynecarr Oct 27, 2016

0xmichalis reviewed Oct 27, 2016

View reviewed changes

0xmichalis added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 27, 2016

0xmichalis reviewed Oct 27, 2016

View reviewed changes

caesarxuchao reviewed Oct 28, 2016

View reviewed changes

caesarxuchao suggested changes Oct 28, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2016

krmayankk closed this Nov 1, 2016

krmayankk deleted the contr-ref branch November 1, 2016 06:52

krmayankk restored the contr-ref branch November 1, 2016 06:53

krmayankk reopened this Nov 1, 2016

caesarxuchao mentioned this pull request Dec 22, 2016

v1beta1/deployment: delete dependents kubernetes/client-go#50

Closed

krmayankk force-pushed the contr-ref branch from f345aaf to 363caa8 Compare December 22, 2016 09:39

krmayankk force-pushed the contr-ref branch 2 times, most recently from 64c5741 to ceed344 Compare December 22, 2016 23:05

caesarxuchao reviewed Dec 22, 2016

View reviewed changes

krmayankk force-pushed the contr-ref branch from ceed344 to 7779776 Compare December 23, 2016 00:43

ReplicaSet has owner ref of the Deployment that created it

7779776

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 23, 2016

k8s-github-robot merged commit af54124 into kubernetes:master Dec 23, 2016

krmayankk mentioned this pull request Dec 24, 2016

append errors while releasing and adopting pods and replicasets during gc #39215

Closed

0xmichalis mentioned this pull request Jan 2, 2017

Actually delete the dummy Pod in kubeadm setups #39248

Closed

0xmichalis mentioned this pull request Mar 10, 2017

Garbage collector should orphan RS created by deployment when deleteOptions.OrphanDependents is true #42639

Closed

caesarxuchao mentioned this pull request Mar 13, 2017

Deployment E2E tests failing in upgrade test jobs #42449

Closed

InAnimaTe mentioned this pull request Mar 29, 2017

Cascading Deployment Removal kubernetes-client/python#162

Closed

derekchiang mentioned this pull request Apr 19, 2017

Remove worker's ReplicaSet when its Deployment has been removed pachyderm/pachyderm#1694

Merged

KnVerey mentioned this pull request Jun 5, 2017

Replicaset awareness Shopify/krane#107

Merged

seh mentioned this pull request Jun 29, 2017

Helm upgrade with --wait is waiting even when all resources are ready helm/helm#2426

Closed

ReplicaSet has onwer ref of the Deployment that created it #35676

ReplicaSet has onwer ref of the Deployment that created it #35676

Conversation

krmayankk commented Oct 27, 2016 • edited by k8s-oncall Loading

krmayankk commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Oct 27, 2016

0xmichalis commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Oct 28, 2016

0xmichalis commented Oct 28, 2016

krmayankk commented Dec 21, 2016

caesarxuchao commented Dec 21, 2016

krmayankk commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

caesarxuchao commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Dec 22, 2016

caesarxuchao commented Dec 23, 2016

k8s-ci-robot commented Dec 23, 2016

k8s-github-robot commented Dec 23, 2016

krmayankk commented Oct 27, 2016 •

edited by k8s-oncall

Loading

caesarxuchao Dec 22, 2016 •

edited

Loading