Ensure Deployment labels adopted ReplicaSets and pods #21030

janetkuo · 2016-02-11T01:51:31Z

Ensure Deployment labels adopted ReplicaSets and pods. Otherwise, new RCs produced by Deployment will overlap we pre-existing ones that aren't constrained by the pod-template-hash.

Fixes #20755

Ignore the first commit (from #20994)
cc @bgrant0607 @nikhiljindal @ironcladlou @Kargakis @kubernetes/sig-config @madhusudancs @mqliang

k8s-github-robot · 2016-02-11T01:57:40Z

Labelling this PR as size/L

k8s-bot · 2016-02-11T02:21:58Z

GCE e2e test build/test passed for commit 00d2a46e858927e0cad4670872a082ca0f51a213.

bgrant0607 · 2016-02-11T07:28:57Z

pkg/util/labels/labels.go

@@ -93,3 +106,30 @@ func CloneSelectorAndAddLabel(selector *unversioned.LabelSelector, labelKey stri

 	return newSelector
 }
+
+// AddLabelToSelector returns a selector with the given key and value added to the given selector's MatchLabels.
+func AddLabelToSelector(selector *unversioned.LabelSelector, labelKey string, labelValue uint32) *unversioned.LabelSelector {


In general, label values are strings, so this function should probably take a string labelValue.

janetkuo · 2016-02-12T01:14:46Z

It's ready for review now. PTAL @bgrant0607 @Kargakis

Addressed comments. Added e2e tests. I also add pod-template-hash to rs's labels (the first commit only added to rs's selector and template label).

k8s-bot · 2016-02-12T01:29:36Z

GCE e2e test build/test passed for commit 449f81b764785120960e213b9b4edf6d8cab8d96.

0xmichalis · 2016-02-12T10:38:26Z

pkg/util/deployment/deployment.go

+		return nil, err
+	}
+	for _, pod := range podList.Items {
+		pod.Labels = labelsutil.AddLabel(pod.Labels, extensions.DefaultDeploymentUniqueLabelKey, hash)


Should we check if the pod already has the label? It may happen that we start labeling some pods and then fail. I think we wouldn't want to relabel those pods.

0xmichalis · 2016-02-12T10:41:18Z

@bgrant0607 I am worried that we are running the pod relabeling as part of an utility for deployments instead of in the rs controller loop. Can't we look up the pod-template-hash label in the rs controller and trigger the relabeling there?

mqliang · 2016-02-12T12:40:54Z

pkg/util/deployment/deployment.go

@@ -25,9 +25,11 @@ import (
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apis/extensions"
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
+	extensions_unversioned "k8s.io/kubernetes/pkg/client/typed/generated/extensions/unversioned"


IIRC, we should not use under_scores in imported package names. see #15319

@mqliang noted. cc @caesarxuchao

Thanks. I'll revise the other occurrences.

Fixed in #21325

bgrant0607 · 2016-02-12T15:46:44Z

@Kargakis Regarding doing the pod relabeling in the RS controller:

My immediate motivation for relabeling is so that adopted ReplicaSets don't overlap with ones created by a Deployment due to lack of the unique label. kubectl rolling-update --image did something similar.

An alternative would be to implement controllerRef in Deployment and ReplicaSet, which is described in #2210 and #14961. In that case, synchronizing controllerRef backpointers of the pods would be delegated to ReplicaSet.

That said, I think we'll want to propagate pod template label updates in the future. This is just one example of an in-place update: #9043. In-place updates make sense for cases where we want to minimize disruption.

Even label updates can disrupt containers, in the case that they are visible to the containers via the downward API. In the future we may need to roll out such changes progressively, as we do for the regular rollout process.

I don't want ReplicaSet to need progressive rollout logic, as well. That logic should live in Deployment.

If we think this is too tricky/risky, we could wait until the next release. With pause, adoption shouldn't be needed as often. We would need to document the problem this was trying to solve as a known issue with the beta implementation.

bgrant0607 · 2016-02-12T18:10:48Z

I just wrote up a controllerRef proposal on #14961.

k8s-github-robot · 2016-02-14T11:36:08Z

PR needs rebase

janetkuo · 2016-02-20T00:55:13Z

@bgrant0607 @Kargakis PTAL

k8s-bot · 2016-02-20T01:04:56Z

GCE e2e test build/test passed for commit 4c361d326014720f86153d9e38041ef64a38333b.

k8s-bot · 2016-02-20T01:10:44Z

GCE e2e test build/test passed for commit 4b4609f55781b4c863b18e59febf276a15dfb1a9.

bgrant0607 · 2016-02-20T03:47:12Z

# k8s.io/kubernetes/pkg/util/deployment
_output/local/go/src/k8s.io/kubernetes/pkg/util/deployment/deployment_test.go:30:2: cannot find package "k8s.io/kubernetes/pkg/client/testing/fake" in any of:
    /usr/local/go/src/k8s.io/kubernetes/pkg/client/testing/fake (from $GOROOT)
    /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/client/testing/fake (from $GOPATH)
    /workspace/kubernetes/Godeps/_workspace/src/k8s.io/kubernetes/pkg/client/testing/fake
FAIL    k8s.io/kubernetes/pkg/util/deployment [setup failed]

bgrant0607 · 2016-02-20T04:45:45Z

pkg/util/deployment/deployment.go

+		}
+	}
+	// Make sure rs pod template is updated so that it won't create pods without the new label (orphaned pods).
+	if updatedRS.Generation != updatedRS.Status.ObservedGeneration {


Should be >.

bgrant0607 · 2016-02-20T06:02:19Z

A couple minor comments and a test failure, but otherwise looks pretty good. Thanks.

0xmichalis · 2016-02-22T11:01:59Z

pkg/util/deployment/deployment.go

+	return updatedRS, nil
+}
+
+func waitForReplicaSetUpdated(c clientset.Interface, desiredGeneration int64, namespace, name string) error {


Move this to pkg/client/conditions.go

Do you mean pkg/client/unversioned/conditions.go? We use the generated client instead of the unversioned one in this function, so this function should not be there. cc @caesarxuchao

Yes, sorry. How about creating pkg/client/clientset_generated/internalclientset/conditions.go then?

No, because code in pkg/client/clientset_generated should be auto-generated. How about making a standalone condition package at pkg/client/conditions/conditions.go?

Let's figure out the best place in another PR.

… too

k8s-bot · 2016-02-22T19:28:16Z

GCE e2e test build/test passed for commit 4699a6d.

janetkuo · 2016-02-22T19:41:00Z

@bgrant0607 @Kargakis Addressed comments and fixed the unit test failure; PTAL

bgrant0607 · 2016-02-22T20:50:10Z

LGTM

k8s-github-robot · 2016-02-23T07:15:33Z

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

k8s-bot · 2016-02-23T07:44:25Z

GCE e2e test build/test passed for commit 4699a6d.

k8s-github-robot · 2016-02-23T07:45:42Z

Automatic merge from submit-queue

Auto commit by PR queue bot

janetkuo added area/app-lifecycle team/ux labels Feb 11, 2016

janetkuo added this to the v1.2 milestone Feb 11, 2016

googlebot added the cla: yes label Feb 11, 2016

k8s-github-robot assigned bprashanth Feb 11, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 11, 2016

bgrant0607 assigned bgrant0607 and unassigned bprashanth Feb 11, 2016

bgrant0607 reviewed Feb 11, 2016
View reviewed changes

janetkuo force-pushed the deployment-label-adopted branch from 00d2a46 to 449f81b Compare February 12, 2016 01:00

janetkuo changed the title ~~WIP: Ensure Deployment labels adopted ReplicaSets and pods~~ Ensure Deployment labels adopted ReplicaSets and pods Feb 12, 2016

0xmichalis reviewed Feb 12, 2016
View reviewed changes

mqliang reviewed Feb 12, 2016
View reviewed changes

bgrant0607 mentioned this pull request Feb 12, 2016

Validate no replicationController overlap. #2210

Closed

janetkuo force-pushed the deployment-label-adopted branch from 4c361d3 to 4b4609f Compare February 20, 2016 00:42

janetkuo removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2016

bgrant0607 reviewed Feb 20, 2016
View reviewed changes

0xmichalis reviewed Feb 22, 2016
View reviewed changes

janetkuo added 6 commits February 22, 2016 10:58

Ensure Deployment labels adopted ReplicaSets and pods

da58172

Address comments; fix test failures; add e2e tests; update RS's label…

11fdbff

… too

Address comments

14bab2b

Address comments

0e5da84

Address comments; fix incorrect hash

dc78af9

Address comments; fix test failure

4699a6d

janetkuo force-pushed the deployment-label-adopted branch from 4b4609f to 4699a6d Compare February 22, 2016 18:59

bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Feb 22, 2016

bgrant0607 mentioned this pull request Feb 23, 2016

e2e flake: Deployment deployment should support rollover [minUnavailable/maxSurge violated] #19299

Closed

k8s-github-robot pushed a commit that referenced this pull request Feb 23, 2016

Merge pull request #21030 from janetkuo/deployment-label-adopted

0afbc71

Auto commit by PR queue bot

k8s-github-robot merged commit 0afbc71 into kubernetes:master Feb 23, 2016

janetkuo added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 11, 2016

Ensure Deployment labels adopted ReplicaSets and pods #21030

Ensure Deployment labels adopted ReplicaSets and pods #21030

Conversation

janetkuo commented Feb 11, 2016

k8s-github-robot commented Feb 11, 2016

k8s-bot commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented Feb 12, 2016

k8s-bot commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Feb 12, 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 Feb 12, 2016

bgrant0607 commented Feb 12, 2016

k8s-github-robot commented Feb 14, 2016

janetkuo commented Feb 20, 2016

k8s-bot commented Feb 20, 2016

k8s-bot commented Feb 20, 2016

bgrant0607 commented Feb 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Feb 20, 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

k8s-bot commented Feb 22, 2016

janetkuo commented Feb 22, 2016

bgrant0607 commented Feb 22, 2016

k8s-github-robot commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

k8s-github-robot commented Feb 23, 2016