Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about implications on selector updates #3877

Merged
merged 1 commit into from May 27, 2017
Merged

Warn about implications on selector updates #3877

merged 1 commit into from May 27, 2017

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented May 23, 2017

Fixes #1938


This change is Reviewable

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 23, 2017
@0xmichalis
Copy link
Contributor Author

@kubernetes/sig-apps-misc


* Selector additions require the pod template labels in the Deployment spec to be updated with the new label, too,
otherwise a validation error is returned. This change is a non-overlapping one, meaning that the new selector does
not select ReplicaSets and Pods created with the old selector, resulting in orphaning all of those objects. Also,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The orphaning will also trigger garbage collection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the Deployment controller removes the owner refs from children that no longer match the parent label selector as per the owner ref proposal.

not select ReplicaSets and Pods created with the old selector, resulting in orphaning all of those objects. Also,
note that changes in the PodTemplateSpec of the Deployment result in new rollouts, but a new ReplicaSet with Pods
will be created anyway since the Deployment selector is not matching anything anymore.
* Selector updates, ie. changing the existing value in a selector key, result in the same behavior as additions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/ie./i.e./

* Selector additions require the pod template labels in the Deployment spec to be updated with the new label, too,
otherwise a validation error is returned. This change is a non-overlapping one, meaning that the new selector does
not select ReplicaSets and Pods created with the old selector, resulting in orphaning all of those objects. Also,
note that changes in the PodTemplateSpec of the Deployment result in new rollouts, but a new ReplicaSet with Pods
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes in the PodTemplateSpec of the Deployment result in new rollouts

This could be a separate point entirely? A bit confusing when placed with creation of new RS-es.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The points here are about selector {additions, updates, removals}, unrelated to creation of RSs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant that these were separate points:

  1. Changes to PodTemplateSpec results in new rollouts
  2. New ReplicaSet will be created since it isn't matching anything because of selector addition.

I think it just seemed a bit difficult to comprehend why those were stated together on my first pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate, a selector addition will require from users to also update their Deployment pod templates too. That will result in the creation of a new ReplicaSet but the new RS will be created anyway since the Deployment is not matching anything due to having a new selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will drop 2. entirely and just mention that the new selector not matching anything will result in a new RS.

@foxish
Copy link
Contributor

foxish commented May 23, 2017

LGTM after comments

@0xmichalis
Copy link
Contributor Author

@foxish thanks for the review, comments addressed and I also made it explicit during selector key removals that no orphaning or creation of RS is involved.

Signed-off-by: Michail Kargakis <mkargaki@redhat.com>
@chenopis
Copy link
Contributor

@Kargakis Thanks for working on this!

@chenopis chenopis merged commit 8a27d98 into kubernetes:master May 27, 2017
@0xmichalis 0xmichalis deleted the label-selector-updates branch June 2, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants