remove apps/v1beta2 defaulting codes for obj.Spec.Selector and obj.Labels #50164

crimsonfaith91 · 2017-08-04T18:57:35Z

What this PR does / why we need it:
This PR removes defaulting codes for obj.Spec.Selector. Currently, obj.Spec.Selector.MatchLabels is set to obj.Spec.Template.Labels if obj.Spec.Template.Labels != nil && obj.Spec.Selector == nil. We should not perform this defaulting operation as controllers selectors are immutable.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #50339

Special notes for your reviewer:
This PR removes defaulting codes for apps/v1beta2 only. The defaulting codes for validation will be removed in another PR.

Release note:

crimsonfaith91 · 2017-08-04T19:04:07Z

pkg/apis/apps/v1beta2/defaults_test.go

@@ -416,13 +406,6 @@ func TestSetDefaultReplicaSet(t *testing.T) {
 			t.Errorf("unexpected object: %v", rs2)
 			t.FailNow()
 		}
-		if test.expectSelector != reflect.DeepEqual(rs2.Spec.Selector.MatchLabels, rs2.Spec.Template.Labels) {


@kow3ns I spent some time thinking whether to set obj.Spec.Selector to nil or &metav1.LabelSelector{MatchLabels: map[string]string{}}. In the end, i chose nil for simplicity. As a result, the test to compare rs2.Spec.Selector.MatchLabels and rs2.Spec.Template.Labels became unnecessary and was removed.

Do not default the selector to anything. We want to eliminate setting the Selector, or any field for that matter, based on another field of the same object. Doing so has caused us pain wrt apply and patch.

Do not default the selector to anything.
Did you mean obj.Spec.Selector should be nil? obj.Spec.Selector will be nil without explicitly setting it.

kow3ns

Don't set the labels on the specification objects based on the PodTemplateSpecs.

kow3ns · 2017-08-04T21:46:55Z

pkg/apis/apps/v1beta2/defaults.go

-		if len(obj.Labels) == 0 {
-			obj.Labels = labels
-		}
+	if labels != nil && len(obj.Labels) == 0 {


Delete this. You are setting the labels of the DaemonSet to the labels of the PodTemplateSpec. You do not want to do this.

@kow3ns Noted! I will remove this. Same applies for other comments. :)

kow3ns · 2017-08-04T21:48:03Z

pkg/apis/apps/v1beta2/defaults.go

-			obj.Labels = labels
-		}
+	if labels != nil && len(obj.Labels) == 0 {
+		obj.Labels = labels


Delete this. You are setting the labels of the StatefulSet to the labels of the PodTemplateSpec. You do not want to do this.

kow3ns · 2017-08-04T21:51:23Z

pkg/apis/apps/v1beta2/defaults.go

-		if len(obj.Labels) == 0 {
-			obj.Labels = labels
-		}
+	if labels != nil && len(obj.Labels) == 0 {


Delete this. You are setting the labels of the Deployment to the labels of the PodTemplateSpec. You do not want to do this.

kow3ns · 2017-08-04T21:51:26Z

pkg/apis/apps/v1beta2/defaults.go

-		if len(obj.Labels) == 0 {
-			obj.Labels = labels
-		}
+	if labels != nil && len(obj.Labels) == 0 {


Delete this. You are setting the labels of the ReplicaSet to the labels of the PodTemplateSpec. You do not want to do this.

kow3ns · 2017-08-04T21:51:30Z

pkg/apis/apps/v1beta2/defaults_test.go

@@ -416,13 +406,6 @@ func TestSetDefaultReplicaSet(t *testing.T) {
 			t.Errorf("unexpected object: %v", rs2)
 			t.FailNow()
 		}
-		if test.expectSelector != reflect.DeepEqual(rs2.Spec.Selector.MatchLabels, rs2.Spec.Template.Labels) {


Do not default the selector to anything. We want to eliminate setting the Selector, or any field for that matter, based on another field of the same object. Doing so has caused us pain wrt apply and patch.

crimsonfaith91 · 2017-08-07T19:39:27Z

test/integration/etcd/etcd_storage_path_test.go

@@ -133,7 +133,7 @@ var etcdStorageData = map[schema.GroupVersionResource]struct {

 	// k8s.io/kubernetes/pkg/apis/apps/v1beta1
 	gvr("apps", "v1beta1", "statefulsets"): {
-		stub:             `{"metadata": {"name": "ss1"}, "spec": {"template": {"metadata": {"labels": {"a": "b"}}}}}`,
+		stub:             `{"metadata": {"name": "ss1"}, "spec": {"selector": {"matchLabels": {"a": "b"}}, "template": {"metadata": {"labels": {"a": "b"}}}}}`,


@kow3ns The test case for statefulset did not have spec.selector, but similar test cases for deployments, daemonsets, and replicasets all have spec.selector. Hence, i modified the test case.

crimsonfaith91 · 2017-08-07T19:39:38Z

test/integration/etcd/etcd_storage_path_test.go

@@ -149,7 +149,7 @@ var etcdStorageData = map[schema.GroupVersionResource]struct {

 	// k8s.io/kubernetes/pkg/apis/apps/v1beta2
 	gvr("apps", "v1beta2", "statefulsets"): {
-		stub:             `{"metadata": {"name": "ss2"}, "spec": {"template": {"metadata": {"labels": {"a": "b"}}}}}`,
+		stub:             `{"metadata": {"name": "ss2"}, "spec": {"selector": {"matchLabels": {"a": "b"}}, "template": {"metadata": {"labels": {"a": "b"}}}}}`,


@kow3ns Same here.

kow3ns · 2017-08-07T20:00:39Z

/lgtm

kow3ns · 2017-08-08T20:57:05Z

/lgtm

erictune · 2017-08-08T21:00:25Z

/approve

liggitt · 2017-08-09T13:08:49Z

We still ensure the pod spec has labels and the selector matches the labels in validation, right?

kow3ns · 2017-08-09T14:24:24Z

Current validation for apps and extensions is still in place. We will issue a subsequent PR for validation changes (where necessary) in the apps and extensions groups. As none of the controllers handle selector mutation in a sane way, we intend to make selectors immutable for v1. We may lift this restriction at a future date when controllers can handle coordinated label updates and selector mutation. This PR for defaults has been done separately because it is isolated to the defaults in the v1beta2 group.

kow3ns · 2017-08-09T17:28:21Z

/lgtm

…bels

kow3ns · 2017-08-09T20:17:24Z

/lgtm

k8s-github-robot · 2017-08-09T20:18:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crimsonfaith91, erictune, kow3ns

Associated issue: 50339

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/apis/OWNERS~~ [erictune]
~~test/OWNERS~~ [erictune,kow3ns]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-10T01:19:06Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-10T02:24:54Z

Automatic merge from submit-queue

0xmichalis · 2017-08-17T08:51:37Z

@mfojtik @tnozicka

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

k8s-github-robot assigned jianhuiz and tallclair Aug 4, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 4, 2017

k8s-ci-robot assigned kow3ns and unassigned jianhuiz and tallclair Aug 4, 2017

crimsonfaith91 commented Aug 4, 2017

View reviewed changes

kow3ns suggested changes Aug 4, 2017

View reviewed changes

crimsonfaith91 force-pushed the default branch from 0fb4f5e to b7468b0 Compare August 5, 2017 00:24

crimsonfaith91 changed the title ~~remove defaulting for obj.Spec.Selector~~ remove defaulting codes for obj.Spec.Selector and obj.Labels Aug 5, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 5, 2017

crimsonfaith91 force-pushed the default branch from b7468b0 to 89611d2 Compare August 7, 2017 18:27

crimsonfaith91 commented Aug 7, 2017

View reviewed changes

kow3ns approved these changes Aug 7, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017

crimsonfaith91 force-pushed the default branch from 89611d2 to 04dfbfa Compare August 7, 2017 21:24

crimsonfaith91 changed the title ~~remove defaulting codes for obj.Spec.Selector and obj.Labels~~ remove apps/v1beta2 defaulting codes for obj.Spec.Selector and obj.Labels Aug 7, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 8, 2017

crimsonfaith91 force-pushed the default branch from 04dfbfa to 7d4f677 Compare August 9, 2017 17:19

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 9, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017

remove apps/v1beta2 defaulting codes for obj.Spec.Selector and obj.La…

a5987fe

…bels

crimsonfaith91 force-pushed the default branch from 7d4f677 to a5987fe Compare August 9, 2017 18:45

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017

k8s-github-robot merged commit 17ec91c into kubernetes:master Aug 10, 2017

crimsonfaith91 mentioned this pull request Aug 17, 2017

Controller selectors should be immutable after creation #50808

Closed

enisoc mentioned this pull request Aug 22, 2017

Confusing error message with apps/v1beta2.statefulset #51133

Closed

This was referenced Sep 11, 2017

Revamped concepts doc for ReplicaSet kubernetes/website#5396

Closed

Revamped concepts doc for ReplicaSet kubernetes/website#5463

Merged

Updated Deployments concepts doc kubernetes/website#5491

Merged

This was referenced Oct 16, 2017

Selector on DaemonSet is not filled automatically #53976

Closed

should update out of date code comment for apps/v1beta2 #54139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove apps/v1beta2 defaulting codes for obj.Spec.Selector and obj.Labels #50164

remove apps/v1beta2 defaulting codes for obj.Spec.Selector and obj.Labels #50164

crimsonfaith91 commented Aug 4, 2017 •

edited

Loading

crimsonfaith91 Aug 4, 2017

kow3ns Aug 4, 2017

crimsonfaith91 Aug 4, 2017

kow3ns left a comment

kow3ns Aug 4, 2017

crimsonfaith91 Aug 4, 2017

kow3ns Aug 4, 2017

kow3ns Aug 4, 2017

kow3ns Aug 4, 2017

kow3ns Aug 4, 2017

crimsonfaith91 Aug 7, 2017

crimsonfaith91 Aug 7, 2017

kow3ns commented Aug 7, 2017

kow3ns commented Aug 8, 2017

erictune commented Aug 8, 2017

liggitt commented Aug 9, 2017

kow3ns commented Aug 9, 2017

kow3ns commented Aug 9, 2017

kow3ns commented Aug 9, 2017

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 10, 2017

k8s-github-robot commented Aug 10, 2017

0xmichalis commented Aug 17, 2017

remove apps/v1beta2 defaulting codes for obj.Spec.Selector and obj.Labels #50164

remove apps/v1beta2 defaulting codes for obj.Spec.Selector and obj.Labels #50164

Conversation

crimsonfaith91 commented Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kow3ns 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

kow3ns commented Aug 7, 2017

kow3ns commented Aug 8, 2017

erictune commented Aug 8, 2017

liggitt commented Aug 9, 2017

kow3ns commented Aug 9, 2017

kow3ns commented Aug 9, 2017

kow3ns commented Aug 9, 2017

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 10, 2017

k8s-github-robot commented Aug 10, 2017

0xmichalis commented Aug 17, 2017

crimsonfaith91 commented Aug 4, 2017 •

edited

Loading