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

Deployment to stop adding pod-template-hash labels/selector on adoption #61615

Merged
merged 4 commits into from Mar 28, 2018

Conversation

@janetkuo
Copy link
Member

janetkuo commented Mar 23, 2018

What this PR does / why we need it: This is a blocker for #55714, because ReplicaSet selector becomes immutable in apps/v1. With controller ref, Deployment's ReplicaSets and Pods can avoid fighting with each others without unique label/selector (pod-template-hash), so it's safe to stop adding hash label/selector on adoption.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #61433

Special notes for your reviewer: This is a behavioral change to Deployment controller that will affect all versions of Deployment APIs (apps/v1, extensions/v1beta1, apps/v1beta1, apps/v1beta2).

Release note:

Deployment will stop adding pod-template-hash labels/selector to ReplicaSets and Pods it adopts. Resources created by Deployments are not affected (will still have pod-template-hash labels/selector). 
@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Mar 23, 2018

/lgtm
/hold

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Mar 23, 2018

You may have to fix e2e tests

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Mar 26, 2018

/lgtm
/hold cancel

Are you planning on making Deployment selectors immutable in a future release?

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Mar 26, 2018

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Mar 26, 2018

Are you planning on making Deployment selectors immutable in a future release?

apps/v1 Deployment selectors are immutable (ref).

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Mar 26, 2018

Awesome, thanks!

cc @tnozicka @mfojtik

@janetkuo janetkuo force-pushed the janetkuo:rm-adopt-hash branch from 9b401ac to 18ec38a Mar 26, 2018

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Mar 26, 2018

Found some functions not referenced by anyone after removing addHashKeyToRSAndPods. Removing them in the last commit.

@janetkuo janetkuo force-pushed the janetkuo:rm-adopt-hash branch from 18ec38a to 7899fb4 Mar 27, 2018

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Mar 27, 2018

It seems that pkg/util/labels is ready-only:

Found pkg/util/labels/.readonly, but files changed compared to "master" branch.

So I'll leave func AddLabels there even though it's not referenced in the repo.

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Mar 27, 2018

Integration test failure is observed in other PRs too. Should be a flake unrelated to this PR. Filed #61748.

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Mar 27, 2018

/test pull-kubernetes-integration

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Mar 27, 2018

All green. @kargakis PTAL

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Mar 28, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 28, 2018

@tnozicka
Copy link
Contributor

tnozicka left a comment

/lgtm

we might want to have a issue tracking deprecation and removal for RSs created directly by the deployment

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 28, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, kargakis, tnozicka

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 28, 2018

Automatic merge from submit-queue (batch tested with PRs 61790, 61808, 60339, 61615, 61757). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 2ba2cc1 into kubernetes:master Mar 28, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation janetkuo authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@kow3ns kow3ns added this to Backlog in Workloads via automation Mar 29, 2018

@kow3ns kow3ns moved this from Backlog to In Progress in Workloads Mar 29, 2018

@kow3ns kow3ns moved this from In Progress to Done in Workloads Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.