Proposal for ControllerReference #25256

gmarek · 2016-05-06T10:34:50Z

Proposal for including the reference pointing to the owning "collection" (controller) for objects that can be grouped. The goal is to prevent a situation when two controllers are fighting over some resources.

cc @bgrant0607 @lavalamp @caesarxuchao @davidopp @fgrzadkowski @wojtek-t @kubernetes/sig-api-machinery

gmarek · 2016-05-06T10:37:18Z

There's a couple questions that need answers:

do we care about determinism between controllers of the same kind
what do we want to do with invalid setReferences that don't have corresponding controllerManagers running (because of downgrade, crash or some bug)

lavalamp · 2016-05-06T17:35:35Z

docs/proposals/set-ref.md

+```
+ObjectMeta {
+    …
+    setReference *SetReference


I think "set" is a bad name, it sounds like a verb. "collection"? "organizer"? "boss"? "manager"? "director"?

@gmarek's original idea was collection. I suggested set, on the argument that it is no more vague than collection (except to former Borg users) and it contains the suffix we're kinda trying to use for API objects (I say kinda because we did ReplicaSet and DaemonSet, but then fell off the wagon with Job and Deployment). You make a good point that "set" might be interpreted as a verb. I'm fine collection. I don't think the other terms you listed are good, because I think we want to covey that this is the set, not the logic that manages the set (which AIUI is part of the reason we moved away from calling collections *Controller, to calling them *Set).

Renamed to CollectionRef.

lavalamp · 2016-05-06T18:17:02Z

@smarterclayton I'd like to get an LGTM from you or one of your minions on this before we accept the design.

caesarxuchao · 2016-05-06T22:08:15Z

docs/proposals/set-ref.md

+
+# Upgrade/downgrade procedure
+
+During system upgrade controllers will adopt objects that match their label selector using ordinary adoption procedure. Adoption, by default, will set *IsParent* to *false,* so the behavior of objects existing before the update won’t change (i.e. deleting owners won't cause cascading deletion - see OwnerReference design).


I think by default IsParent should set true, so if the user later want to do cascading deletion, it will be possible.

We can keep the backward compatibility by defaulting the DeleteOptions.OrphanDependents to true.

lavalamp · 2016-05-09T16:50:31Z

docs/proposals/set-ref.md

+    // UID of the referent.
+    UID types.UID
+    // Should this CollectionReference be treated as an owner for cascading deletion purposes
+    Owner bool


As an alternative to this, we could instead add a CollectionOwner bool field to OwnerRef, making this a special OwnerReference. Did we discuss that approach?

I think if you want the garbage collector to be responsible for clearing these, then we should really consider making these less different from the things the garbage collector actually looks at.

We did not. We were talking about how to do this with @caesarxuchao but run out of time. What you propose is that a collectionRef is always an owner. I'm fine with that, if everyone are (@bgrant0607).

Then we can have collectionRef of an *OwnerRef type as a copy of the value from OwnerReferences. Everything that modifies OwnereReferences would be responsible for keeping collectionRef up to date. Other solution would require iterating over all owner every time we need to get to the "managing collection" (I'm running out of words...), and as it's done on every update it may be quite expensive (@wojtek-t)

bgrant0607 · 2016-05-09T22:32:42Z

@gmarek The title of this PR is misleading.

It's also not helpful for others to omit the PR description.

bgrant0607 · 2016-05-09T22:34:12Z

docs/proposals/set-ref.md

+- [Implementation plan (sketch)](#implementation-plan-sketch)
+- [Considered alternatives](#considered-alternatives)
+
+# Goal of CollectionReference


We don't use the term "Collection" ANYWHERE. Please don't introduce a new term.

Do you have a better suggestion? AFAIK we don't have a term for the set of pods belonging to a single controller.

We were planning to use "Set" as in SetReference but @lavalamp pointed out that SetReference sounds like Set is being used as a verb.

gmarek · 2016-07-01T14:39:29Z

@lavalamp PTAL

gmarek · 2016-07-07T08:12:58Z

Ping @lavalamp

gmarek · 2016-07-07T09:17:05Z

@smarterclayton - can someone from RH take a look.
@davidopp - can you find some approver for this proposal?

davidopp · 2016-07-07T22:35:04Z

I think @bgrant0607 or @lavalamp or @caesarxuchao are the best approvers for this. I don't mind doing it but I wasn't involved in all the design discussions so I might not notice something that was omitted.

caesarxuchao · 2016-07-07T22:42:11Z

I'll take another pass tonight.

smarterclayton · 2016-07-08T02:28:29Z

I have no further comments

caesarxuchao · 2016-07-08T03:40:51Z

docs/proposals/controller-ref.md

+* there are no `OwnerReferences` with `Controller` set to true in `OwnerReferences` array
+* `DeletionTimestamp` is not set
+and
+* Controller is the oldest controller of a given kind from ones which has a selector that matches labels of adopted object that does not have `DeletionTimestamp` set.


Could you remind me why we want this determinism?

Testability and general complexity of the system. It's more like philosophy than anything else, but I believe that deterministic behavior is way simpler for people to understand, and various interaction between different components are slightly clearer. Maybe @bgrant0607 can give more reasons.

That being said I agree that the system will work mostly the same from the user's perspective without this assumption.

I'm asking this because to enforce this rule will make the controller code complex and less efficient based on my experience of refactoring the rc manager. Let's say rc1 and rc2 both have matching selectors, and rc1 is older. If rc2 gets processed by the rm manager's syncHandler before rc1, the only way to prevent rc2 to adopt a pod is checking if there is older matching rc in the system. This is time consuming (if the lookupCache is invalidated) and make the code complex. I'm not convince we should do this for "testability and general complexity". Having multiple controllers matching a pod is still an anti-pattern. We are not obligated to make the system behavior simple to understand if this happens.

Testability implies robustness. Let me think about the implementation next week, if I won't think of something I may remove this requirement.

I'm with @caesarxuchao here. How about using the pattern @erictune introduced in Jobs, where upon creation we add an additional selector which guarantees pod selection uniqueness? See this code.

I think it's an orthogonal to the issue here and is something that should be discussed in much wider audience and we'd need to overwrite it for RS created by the Deployment.

Not quite, since #14961, which this proposal addresses mentioned #12298 as one of the topics.

OK, I didn't knew about that.

wojtek-t · 2016-07-14T17:02:20Z

We are finishing implementation - can we have the proposal finally merged?

@lavalamp

Automatic merge from submit-queue [GarbageCollector] Let the RC manager set/remove ControllerRef What's done: * RC manager sets Controller Ref when creating new pods * RC manager sets Controller Ref when adopting pods with matching labels but having no controller * RC manager clears Controller Ref when pod labels change * RC manager clears pods' Controller Ref when rc's selector changes * RC manager stops adoption/creating/deleting pods when rc's DeletionTimestamp is set * RC manager bumps up ObservedGeneration: The [original code](https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/replication/replication_controller_utils.go#L36) will do this. * Integration tests: * verifies that changing RC's selector or Pod's Labels triggers adoption/abandoning * e2e tests (separated to #27151): * verifies GC deletes the pods created by RC if DeleteOptions.OrphanDependents=false, and orphans the pods if DeleteOptions.OrphanDependents=true. TODO: - [x] we need to be able to select Pods that have a specific ControllerRef. Then each time we sync the RC, we will iterate through all the Pods that has a controllerRef pointing the RC, event if the labels of the Pod doesn't match the selector of RC anymore. This will prevent a Pod from stuck with a stale controllerRef, which could be caused by the race between abandoner (the goroutine that removes controllerRef) and worker the goroutine that add controllerRef to pods). - [ ] use controllerRef instead of calling `getPodController`. This might be carried out by the control-plane team. - [ ] according to the controllerRef proposal (#25256): "For debugging purposes we want to add an adoptionTime annotation prefixed with kubernetes.io/ which will keep the time of last controller ownership transfer." This might be carried out by the control-plane team. cc @lavalamp @gmarek

gmarek · 2016-07-18T15:10:54Z

@caesarxuchao - removed the 'oldest controller' part.

soltysh · 2016-07-18T18:24:57Z

Current version LGTM. Let's get this in and we can iterate further.

gmarek · 2016-07-18T20:47:41Z

I'll squash when I get home.

caesarxuchao · 2016-07-19T04:14:11Z

It seems @soltysh's comment is lost on this thread. I found it in the email notification and @soltysh's suggestions is using server generated selector upon controller creation to make the pod selection unique (code). Does this make adoption impossible?

gmarek · 2016-07-19T05:04:26Z

Automatic adoption yes. But user always can set the label by hand.

k8s-bot · 2016-07-19T05:40:18Z

GCE e2e build/test passed for commit 8e0ff44.

caesarxuchao · 2016-07-20T17:46:29Z

LGTM. Thanks, @gmarek! Since we are already implementing it, let's merge and iterate if there are issues.

k8s-github-robot · 2016-07-20T18:11:27Z

Automatic merge from submit-queue

@bgrant0607

Automatic merge from submit-queue Proposal for ControllerReference Proposal for including the reference pointing to the owning "collection" (controller) for objects that can be grouped. The goal is to prevent a situation when two controllers are fighting over some resources. cc @bgrant0607 @lavalamp @caesarxuchao @davidopp @fgrzadkowski @wojtek-t @kubernetes/sig-api-machinery

gmarek added kind/design Categorizes issue or PR as related to design. kind/documentation Categorizes issue or PR as related to documentation. labels May 6, 2016

googlebot added the cla: yes label May 6, 2016

k8s-github-robot assigned smarterclayton May 6, 2016

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

gmarek assigned lavalamp and unassigned smarterclayton May 6, 2016

gmarek force-pushed the proposal branch 2 times, most recently from 208b69b to 4d026d9 Compare May 6, 2016 14:33

lavalamp reviewed May 6, 2016
View reviewed changes

caesarxuchao reviewed May 6, 2016
View reviewed changes

gmarek force-pushed the proposal branch 2 times, most recently from ba08332 to c9e3d4c Compare May 9, 2016 09:26

gmarek added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 9, 2016

lavalamp reviewed May 9, 2016
View reviewed changes

bgrant0607 added the e2e-not-required label May 9, 2016

bgrant0607 reviewed May 9, 2016
View reviewed changes

gmarek assigned smarterclayton and davidopp and unassigned lavalamp Jul 7, 2016

This was referenced Jul 8, 2016

[GarbageCollector] Let the RC manager set/remove ControllerRef #27600

Merged

[Garbage Collector] Umbrella: known issues of garbage collector #26120

Closed

caesarxuchao reviewed Jul 8, 2016
View reviewed changes

gmarek force-pushed the proposal branch from 8753914 to 5a664ac Compare July 18, 2016 14:55

Proposal for ControllerReference

8e0ff44

gmarek force-pushed the proposal branch from 5a664ac to 8e0ff44 Compare July 19, 2016 05:05

caesarxuchao self-assigned this Jul 20, 2016

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

k8s-github-robot merged commit b7490d5 into kubernetes:master Jul 20, 2016

gmarek deleted the proposal branch August 30, 2016 09:48


		# Upgrade/downgrade procedure

		During system upgrade controllers will adopt objects that match their label selector using ordinary adoption procedure. Adoption, by default, will set IsParent to false, so the behavior of objects existing before the update won’t change (i.e. deleting owners won't cause cascading deletion - see OwnerReference design).

Proposal for ControllerReference #25256

Proposal for ControllerReference #25256

Conversation

gmarek commented May 6, 2016 • edited

gmarek commented May 6, 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

lavalamp commented May 6, 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

bgrant0607 commented May 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented Jul 1, 2016

gmarek commented Jul 7, 2016

gmarek commented Jul 7, 2016

davidopp commented Jul 7, 2016

caesarxuchao commented Jul 7, 2016

smarterclayton commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao Jul 8, 2016 • edited

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

wojtek-t commented Jul 14, 2016

gmarek commented Jul 18, 2016

soltysh commented Jul 18, 2016

gmarek commented Jul 18, 2016

caesarxuchao commented Jul 19, 2016

gmarek commented Jul 19, 2016

k8s-bot commented Jul 19, 2016

caesarxuchao commented Jul 20, 2016

k8s-github-robot commented Jul 20, 2016

gmarek commented May 6, 2016 •

edited

caesarxuchao Jul 8, 2016 •

edited