Proposal of selector generation #256

mengqiy · 2017-01-06T19:24:01Z

Selector generation for Deployments, ReplicaSet, DaemonSet, StatefulSet and ReplicationController.

Originally discussed in #187.

enisoc · 2017-01-07T01:19:11Z

The controllerRef proposal already claims to solve the "selector overlap" problem, and it was chosen over generated labels for that purpose because of the points discussed here: kubernetes/kubernetes#36859 (comment).

I'm working on finishing the implementation of controllerRef. If that works to solve selector overlap, is there an additional benefit to expanding selector generation to all the workload controllers?

0xmichalis · 2017-01-09T10:50:21Z

The controllerRef proposal already claims to solve the "selector overlap" problem, and it was chosen over generated labels for that purpose because of the points discussed here: kubernetes/kubernetes#36859 (comment).

I'm working on finishing the implementation of controllerRef. If that works to solve selector overlap, is there an additional benefit to expanding selector generation to all the workload controllers?

Owner references don't fully solve selector overlap. Between two overlapping deployments A and B (assuming A is older thus works as expected), B cannot adopt the replica sets owned by A but B is still blocked. A unique selector key would address that issue.

0xmichalis · 2017-01-09T10:58:55Z

Owner references don't fully solve selector overlap. Between two overlapping deployments A and B (assuming A is older thus works as expected), B cannot adopt the replica sets owned by A but B is still blocked. A unique selector key would address that issue.

And even if this worked, we could end up in weird situations:
Deployment A selects RS-A1, RS-A2, RS-A3
Deployment B selects RS-B1, RS-B2

Deployment A is deleted.
The garbage collector now races with Deployment B, either it will get to the replica sets first and delete them or Deployment B adopts the additional replica sets and ends up with RS-B1, RS-B2, RS-A1, RS-A2, RS-A3 in the worst case.

EDIT Nevermind this example, if Deployment A gets deleted (cascade=true), Deployment B will not be able to adopt the replica sets.

0xmichalis · 2017-01-09T11:02:55Z

contributors/design-proposals/selector-generation-for-all.md

+
+`extension/v1beta1 Deployment|ReplicaSet|DaemonSet`, `apps/v1beta1 StatefulSet` and `v1 ReplicationController` change as follows.
+
+Add field `spec.manualSelector`. It controls if using automatic generated selectors.


I am not sure I like the additional field. I suggest we always default a key - is there a backwards-compatibility concern here?

Users already hardly understand how to use selectors, adding one more field to work-around that will probably cause more confusion than less.

0xmichalis · 2017-01-09T12:14:24Z

Owner references don't fully solve selector overlap. Between two overlapping deployments A and B (assuming A is older thus works as expected), B cannot adopt the replica sets owned by A but B is still blocked. A unique selector key would address that issue.

Thinking about it more, if we change our listers to take into account owner references, we may not need selector generation. Haven't looked closely at that but it sounds plausible.

enisoc · 2017-01-09T18:17:07Z

Thinking about it more, if we change our listers to take into account owner references, we may not need selector generation. Haven't looked closely at that but it sounds plausible.

I had originally thought this was included in the controllerRef proposal, but upon re-reading I see that it's not quite clear what the intention was. The question specifically is what the implications of "ownership" are in this passage:

Most controllers (all that manage collections of things defined by label selector) will have slightly changed semantics: currently controller owns an object if its selector matches object’s labels and if it doesn't notice an older controller of the same kind that also matches the object's labels, but after introduction of ControllerReference a controller will own an object iff selector matches labels and the OwnerReference with Controller=true points to it.

If the controller ignores an object because its controllerRef points to someone else, I thought that means it will create its own object, so no controller will get stuck. I'll try to get clarification from the reviewer of that proposal.

lavalamp · 2017-01-09T18:40:10Z

Owner references don't fully solve selector overlap. Between two overlapping deployments A and B (assuming A is older thus works as expected), B cannot adopt the replica sets owned by A but B is still blocked. A unique selector key would address that issue.

That's not how it works :)

B is not blocked-- if A wants 10 pod and B wants 10 pods, you'll get total 20 pods. A and B will each construct their pods with the correct controllerRef already specified, so there will never be any un-adopted pods and no opportunity for the two controllers to fight over anything.

lavalamp · 2017-01-09T18:43:25Z

contributors/design-proposals/selector-generation-for-all.md

+=============
+
+# Goals
+Make selector easy to use and less error-prone for Deployments, ReplicaSet, DaemonSet, StatefulSet and ReplicationController. Make `kubectl apply` work well with selector.


Can we remove all wording in this doc that's about correctness? controllerRef should take care of the existing problems. I think there is something that needs to be done to fix kubectl apply, but this is a really invasive change and we should think about whether anything else can be done.

lavalamp · 2017-01-09T18:45:20Z

contributors/design-proposals/selector-generation-for-all.md

+# Problem Description
+The field `spec.selector` of Deployments, ReplicaSet, DaemonSet are defaulted from `spec.template.metadata.labels`, if it is unspecified.
+And there is validation to make sure `spec.selector` always selects the pod template.
+The defaulting of selector may prevent from `kubectl apply` from working when updating the `spec.selector` field. e.g. [kubernetes/kubernetes#26202 (comment)](https://github.com/kubernetes/kubernetes/issues/26202#issuecomment-221421254)


Can we add an "alternatives considered" section to this doc? I would at least like to see what it would look like to fix this problem by e.g. modifying kubectl apply instead of every other component :)

lavalamp · 2017-01-09T18:46:01Z

contributors/design-proposals/selector-generation-for-all.md

+
+## Garbage collector
+
+Garbage collector is responsible for cleaning up the extra generated labels when orphaning.


Why would this be necessary? I would prefer for GC to not have to do this.

lavalamp · 2017-01-09T18:46:48Z

contributors/design-proposals/selector-generation-for-all.md

+
+## Controllers
+
+Controller managers are responsible to set generated labels when adopting.


This doesn't make a lot of sense to me, controllers wouldn't adopt anything that didn't already have the generated label set, no?

pwittrock

I think there are some good ideas here, and generally I do like how they permit hiding some controller implementation details from the user. I fear introducing these sorts of changes to the API in a backwards compatible way may be challenging, and create confusion for existing users that already understand how the current implementation functions.

I can think of a couple different problems trying to be solved here.

Simplify specification of controller objects.

I imagine the most common case users want is for Pods created by a controller (and only Pods created by that controller) to be managed / owned by it.
Adopting and Orphaning Pods should be the exception, and the details of how to do such need not be part of the spec

Generally Simplify updating labels on Pods created by a controller

Changing labels on Pods contained in the controller selector is a bit of a nightmare. Pod controller references help, but it is confusing what is going on behind the scenes. IIUC they also make querying which Pods are owned by a controller and orphaning more complicated.
Do controller references solve the case where the PodTemplate labels are updated, but the controller selectors are not? I guess this is caught in Validation instead?

Reduce the number of (user) errors caused by changing PodTemplate labels with kubectl apply

If we just focus on this case, we can probably do a lot in the client.

pwittrock · 2017-01-09T18:53:29Z

contributors/design-proposals/selector-generation-for-all.md

+### Automatic Mode
+
+- User does not specify `spec.selector`.
+- User does not specify `spec.manualSelector` field or set it to `false`.


This seems like it changes the api in a manner that is not backwards compatible. Is that correct?

pwittrock · 2017-01-09T18:54:37Z

contributors/design-proposals/selector-generation-for-all.md

+
+## Garbage collector
+
+Garbage collector is responsible for cleaning up the extra generated labels when orphaning.


Isn't the process of orphaning itself what cleans up the labels? I guess a Pod could be orphaned by deleting only 1 of the labels.

Yeah, I really really don't want to build knowledge of various labels into the GC's orphaning process.

pwittrock · 2017-01-09T18:56:16Z

contributors/design-proposals/selector-generation-for-all.md

+### Rationale
+
+Label combination of `resource name` and `resource kind` is unique across all kind of resources.
+They are more predictable and human-friendly than UID.


I wonder if this is a good thing. Having the labels that tie controller-to-pods be scary looking might not be terrible :)

It is not actually unique unless you include the group... I would really recommend against doing this.

…ternatives considered section

mengqiy · 2017-01-11T01:35:29Z

Updated the proposal:

propose a client-side design
move the original server-side design to Alternatives considered section.

mengqiy · 2017-01-13T08:02:17Z

Updated the proposal:

propose selector generation as a plug-in in admission controller

bgrant0607 · 2017-01-18T21:26:49Z

Please do not merge this without my approval.

bgrant0607

Counterproposal:

Warn about selector changes that could orphan pods, for all updates, not just apply
Warn about label changes in the case of an empty selector in apply

bgrant0607 · 2017-02-21T20:13:51Z

contributors/design-proposals/selector-generation-from-kubectl.md

+
+# Goals
+
+Make `kubectl apply` work well with selector.


This goal isn't achievable with only client-side changes. In general, changing the selector requires orchestration to ensure that pods aren't accidentally orphaned.

The selector generation has to be done on server, and that should be the goal.

Agreed with @soltysh. Also I believe selector generation is going to be useful for more than just making kubectl apply work. For example, in Deployments we already need to generate a label for making the underlying ReplicaSets non-overlapping.

I'm inclined to say that everything should be using generated selectors, by default. With the ability to turn it off by a user, when needed. Selectors are for controllers to work with underlying resources, usually. There's nothing stooping users from adding additional labels, if needed.

So I skimmed through the proposal and it seems to target the apply use case in the client. It will most likely be superseded if apply is moved server-side. I need to read the selector generation proposal for Jobs.

bgrant0607 · 2017-02-22T04:45:23Z

contributors/design-proposals/selector-generation-from-kubectl.md

+
+### Rationale
+
+Label combination of `resource group`, `resource kind` and `resource name` is unique across all kind of resources. They are predictable, human-friendly and self-documenting, but we need all of them to assure uniqueness.


The problem with resource name is that it's a single attribute. In practice, in order to make it unique, users will concatenate several attributes. However, if they don't plan for all attributes they'll eventually need in advance, they'll need to delete the resource and recreate it in order to add more attributes. Labels representing individual attributes, OTOH, could be added later. Or, in the case that a new controller needed new distinguishing attributes, the selector(s) of the original controller(s) could be updated to select the absence of the new label key.

I think we'd be better off providing conventions and tools to set a number (3-5) of common attributes by default.

bgrant0607 · 2017-02-22T04:50:12Z

contributors/design-proposals/selector-generation-from-kubectl.md

+
+Label combination of `resource group`, `resource kind` and `resource name` is unique across all kind of resources. They are predictable, human-friendly and self-documenting, but we need all of them to assure uniqueness.
+
+Label `UID` itself is unique. It is simple but unpredictable.


It should be made clearer that because uid is unpredictable, it's not part of this proposal.

bgrant0607 · 2017-02-22T04:57:47Z

contributors/design-proposals/selector-generation-from-kubectl.md

+
+# Proposed changes
+
+We can let `kubectl` do something similar to [Selector Generation](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/selector-generation.md) on creation. We can let `kubectl` add a set of labels to assure non-overlapping. 


We can't change default behavior.

bgrant0607 · 2017-02-22T05:00:36Z

contributors/design-proposals/selector-generation-from-kubectl.md

+The second set of labels is:
+
+- Add 1 more label, which concatenates all the three in the first set, to `spec.selector`
+  - `matchLabels["controllers.k8s.io/selector"]="$GROUP/$KIND/$NAME"`


Why is this needed if we have the other 3 labels?

Also, I try to avoid label keys with prefixes.

bgrant0607 · 2017-02-22T05:02:17Z

contributors/design-proposals/selector-generation-from-kubectl.md

+- Add 1 more label to `spec.selector`
+  - `matchLabels["resource-uid"]="$UIDOFRESOURCE"`
+- Append 1 label to `spec.template.metadata.labels`
+  - `resource-uid=$UIDOFRESOURCE`


Don't see why uid is needed.

soltysh

We should rather go in the direction Jobs has implemented, iow. using uuid as a selector, although this will make the selectors hard to read and use by regular users.

soltysh · 2017-02-22T16:22:35Z

contributors/design-proposals/selector-generation-from-kubectl.md

+
+# Goals
+
+Make `kubectl apply` work well with selector.


The selector generation has to be done on server, and that should be the goal.

bgrant0607 · 2017-07-17T16:55:33Z

This approach isn't aligned with the intent of labels, so I'm closing this proposal.

cc @kow3ns

* Fix peribolos config * Add readme and makefile * Grab dep * Remove sync command

proposal of selector generation for all

892ab48

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 6, 2017

mengqiy mentioned this pull request Jan 6, 2017

Proposal: Use Selector Generation for Deployments, ReplicaSet, DaemonSet, StatefulSet and ReplicationController #187

Closed

0xmichalis reviewed Jan 9, 2017

View reviewed changes

lavalamp reviewed Jan 9, 2017

View reviewed changes

pwittrock reviewed Jan 9, 2017

View reviewed changes

0xmichalis mentioned this pull request Jan 10, 2017

Deployments with overlapping selectors thrash pods kubernetes/kubernetes#24152

Closed

propose selector generation in kubectl and move server-side one to Al…

af0c375

…ternatives considered section

add admission controller design and minor updates

80db270

mengqiy force-pushed the selector_generation branch from 63c66b4 to 80db270 Compare January 13, 2017 08:00

bgrant0607 self-assigned this Jan 18, 2017

bgrant0607 reviewed Feb 22, 2017

View reviewed changes

soltysh suggested changes Feb 22, 2017

View reviewed changes

0xmichalis mentioned this pull request Feb 22, 2017

Rapid actions on a DC end up messing its scale openshift/origin#12589

Closed

bgrant0607 closed this Jul 17, 2017

mengqiy deleted the selector_generation branch February 26, 2018 07:15

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

Setup org automation based on config file (kubernetes#256)

b4e6aa0

* Fix peribolos config * Add readme and makefile * Grab dep * Remove sync command


		`extension/v1beta1 Deployment\|ReplicaSet\|DaemonSet`, `apps/v1beta1 StatefulSet` and `v1 ReplicationController` change as follows.

		Add field `spec.manualSelector`. It controls if using automatic generated selectors.


		## Garbage collector

		Garbage collector is responsible for cleaning up the extra generated labels when orphaning.


		## Controllers

		Controller managers are responsible to set generated labels when adopting.


		### Rationale

		Label combination of `resource group`, `resource kind` and `resource name` is unique across all kind of resources. They are predictable, human-friendly and self-documenting, but we need all of them to assure uniqueness.


		Label combination of `resource group`, `resource kind` and `resource name` is unique across all kind of resources. They are predictable, human-friendly and self-documenting, but we need all of them to assure uniqueness.

		Label `UID` itself is unique. It is simple but unpredictable.


		# Proposed changes

		We can let `kubectl` do something similar to [Selector Generation](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/selector-generation.md) on creation. We can let `kubectl` add a set of labels to assure non-overlapping.

Proposal of selector generation #256

Proposal of selector generation #256

Conversation

mengqiy commented Jan 6, 2017 • edited

enisoc commented Jan 7, 2017

0xmichalis commented Jan 9, 2017

0xmichalis commented Jan 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jan 9, 2017

enisoc commented Jan 9, 2017

lavalamp commented Jan 9, 2017

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

pwittrock 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

mengqiy commented Jan 11, 2017

mengqiy commented Jan 13, 2017

bgrant0607 commented Jan 18, 2017

bgrant0607 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

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Jul 17, 2017

mengqiy commented Jan 6, 2017 •

edited

0xmichalis commented Jan 9, 2017 •

edited