Fix StatefulSet set-based selector bug by ayushpateria · Pull Request #59365 · kubernetes/kubernetes

ayushpateria · 2018-02-05T19:52:26Z

What this PR does / why we need it:
ControllerRevisions were using selectors as the labels, in case of set-based selectors, the helper function to convert selectors to labels would break. This PR uses pod labels for ControllerRevision labels instead of selectors.
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 #59266

Special notes for your reviewer:
I'm trying to learn Kubernetes codebase and would be happy to make changes if anything is off.
Release note:

Fix StatefulSet to work with set-based selectors.

lavalamp · 2018-02-05T21:11:14Z

Please add a test :)

lavalamp · 2018-02-05T21:11:25Z

/assign @cheftako

cheftako · 2018-02-06T01:09:40Z

pkg/controller/history/controller_history.go

+func getPodLabelsFromData(data runtime.RawExtension) map[string]string {
+	var dat map[string]interface{}
+	labelMap := make(map[string]string)
+	if err := json.Unmarshal(data.Raw, &dat); err != nil {


How do we know that the raw extension will always be json and not say protobuf?

@cheftako Please have a look at my recent commit :)

cheftako · 2018-02-06T01:10:20Z

pkg/controller/history/controller_history.go

+	var dat map[string]interface{}
+	labelMap := make(map[string]string)
+	if err := json.Unmarshal(data.Raw, &dat); err != nil {
+		return labelMap


If the raw data does not parse we should not be silently swallowing that error. Additionally we clearly are missing tests for cases where either the key or value are invalid.

Will a log warning work? I didn't want it to break like previously if the problem is only with parsing of labels, as they aren't being used anywhere anyway.

cheftako · 2018-02-06T01:10:59Z

pkg/controller/history/controller_history.go

+		return labelMap
+	}
+	for key, value := range labels {
+		labelMap[key] = value.(string)


We should make sure to validate both the key and value to make sure they are valid label key and label values.

Are we required to validate key and values again? Because they must have already been validated when creating the StatefulSet (or pods).

0xmichalis · 2018-02-06T08:52:45Z

@kubernetes/sig-apps-pr-reviews

enisoc · 2018-02-06T19:25:54Z

pkg/controller/history/controller_history.go

-	if err != nil {
-		return nil, err
-	}
+	labelMap := getPodLabelsFromData(data)


I think we should require the caller of NewControllerRevision to provide the labels, rather than making assumptions about the contents of data.

cc @kow3ns @janetkuo

@enisoc please check :)

0xmichalis · 2018-02-09T09:16:01Z

pkg/controller/history/controller_history.go

Remove this diff

0xmichalis · 2018-02-09T09:21:35Z

pkg/controller/statefulset/stateful_set_utils.go

podLabels should never be nil

The statefulset selector is required to be non-empty

The podtemplate labels are required to match the selector.

I had this doubt, did the check anyway to be on safer side. Removed the check now. Please see the recent commit.

0xmichalis · 2018-02-09T09:35:07Z

lgtm

also needs a unit test that uses a set-based selector

ayushpateria · 2018-02-11T23:47:30Z

@Kargakis I've modified newStatefulSet() to include a set based selector, this should ensure controller revisions don't fail when stateful sets are having set-based selectors.

ayushpateria · 2018-02-12T00:01:10Z

pkg/controller/history/controller_history_test.go

TestSortControllerRevisions was failing because of the incorrect version number (2 was being used for ss1Rev3).

enisoc · 2018-02-19T09:04:50Z

/ok-to-test

ayushpateria · 2018-02-19T22:36:45Z

/retest

ayushpateria · 2018-02-20T13:41:38Z

Why is pull-kubernetes-bazel-test failing? Is it a flake?

janetkuo · 2018-02-20T18:18:11Z

The failure looks relevant to the change made in this PR.

//pkg/controller/statefulset:go_default_test 0.00s
bazel test //pkg/controller/statefulset:go_default_test
E0219 23:03:17.207307      14 stateful_pod_control.go:125] error getting updated Pod default/foo-0 from lister: pod "foo-0" not found
Terminated

janetkuo · 2018-02-20T18:51:49Z

pkg/controller/history/controller_history.go

-	if err != nil {
-		return nil, err
-	}
+	labelMap := podLabels


Need to make a copy here; otherwise, podLabels gets mutated when cr.Labels is mutated in L91.

And this means template labels (of the given StatefulSet) gets mutated.

janetkuo · 2018-02-20T18:56:28Z

pkg/controller/history/controller_history.go


 // NewControllerRevision returns a ControllerRevision with a ControllerRef pointing to parent and indicating that
-// parent is of parentKind. The ControllerRevision has labels matching selector, contains Data equal to data, and
+// parent is of parentKind. The ControllerRevision has labels matching pod, contains Data equal to data, and


CR's labels is a copy of template labels; "matching" is between selector and labels.

janetkuo · 2018-02-20T18:56:47Z

pkg/controller/history/controller_history.go

 func NewControllerRevision(parent metav1.Object,
 	parentKind schema.GroupVersionKind,
-	selector labels.Selector,
+	podLabels map[string]string,


Name it templateLabels

janetkuo · 2018-02-20T18:57:13Z

pkg/controller/statefulset/stateful_set_utils.go

 	cr, err := history.NewControllerRevision(set,
 		controllerKind,
-		selector,
+		podLabels,


templateLabels

enisoc · 2018-02-20T18:54:23Z

pkg/controller/history/controller_history_test.go

-		t.Fatal(err)
-	}
-	ss1Rev1, err := NewControllerRevision(ss1, parentKind, sel1, rawTemplate(&ss1.Spec.Template), 1, nil)
+	sel1 := labels.Set(ss1.Spec.Template.Labels).AsSelector()


I think sel1 should remain what it was (pulling the actual selector from the StatefulSet). This selector ends up being used to list ControllerRevisions. If the selector has MatchExpressions, it's important that sel1 includes those as well.

The selector selects ControllerRevisions on the basis of labels, earlier because the labels of controller revisions and selectors of StatefulSet were same, we could use StatefulSet selector. But now the labels of CRs are the template labels, so I changed the CR's selector to that.
Every StatefulSet selector should have a matching pod label (which is also CR's label), so I think the previous version would work as well, but it's not consistent with labels logic.

It's always safe to use the StatefulSet's selector, because we ensure through validation that it selects the pod template labels. Since we now apply the pod template labels to revisions too, we can also expect that the StatefulSet's selector will match those revisions.

On the other hand, generating a selector purely from the template labels will not always give the right result. Here is a contrived example of that:

... metadata: name: ss1 spec: selector: matchLabels: component: mything matchExpressions: - {key: role, operator: DoesNotExist} template: metadata: labels: component: mything ... --- ... metadata: name: ss2 spec: selector: matchLabels: component: mything role: test template: metadata: labels: component: mything role: test

If you generate a selector directly from the pod template labels, ss1 would end up also selecting ss2's revisions, whereas ss1's actual selector means that it will avoid selecting ss2's pods. We want the behavior for selecting pods and revisions to be consistent with each other, so we respect the user's intention.

I called this example contrived because it's probably a bad idea for the user to do this. However, the API allows it, so we need to make sure we do the right thing.

enisoc · 2018-02-20T18:58:31Z

pkg/controller/history/controller_history.go

-	if err != nil {
-		return nil, err
-	}
+	labelMap := podLabels


~~This variable isn't needed anymore since there's no conversion happening. We can just directly use the input parameter as Labels: podLabels.~~

Nevermind, see comment above by @janetkuo.

podLabels is not being used anywhere later, I agree with what you struck out. Am I missing something?

@janetkuo pointed out in the other thread that we mutate this map below on this line:

cr.Labels[ControllerRevisionHashLabel] = strconv.FormatInt(int64(hash), 10)

Since map types are references, this means we're mutating the map that the caller passed in to us. So it turns out we do need to make our own labelMap var after all, to hold a deep copy of podLabels. Unless you can find a utility helper for this, you'll need to just allocate a new map and then range loop over the input map to copy each entry.

Okay, I was thinking that even if it gets mutated it's not affecting anything. But yeah, we should not mutate anything that caller passes.

it's not affecting anything

It actually mutates template labels (set.Spec.Template.Labels) the caller passes.

Got it. Thanks.

enisoc · 2018-02-20T19:01:26Z

pkg/controller/history/controller_history_test.go

-		t.Fatal(err)
-	}
-	ss1Rev1, err := NewControllerRevision(ss1, parentKind, sel1, rawTemplate(&ss1.Spec.Template), 1, nil)
+	sel1 := labels.Set(ss1.Spec.Template.Labels).AsSelector()


enisoc · 2018-02-20T19:24:25Z

pkg/controller/history/controller_history_test.go

 		Spec: apps.StatefulSetSpec{
 			Selector: &metav1.LabelSelector{
 				MatchLabels: labels,
+				MatchExpressions: []metav1.LabelSelectorRequirement{


I agree that this change to the test would have prevented the exact manifestation of the bug we hit (converting a new-style, set-based selector into a string and then trying to parse it as an old-style, map-only selector). However, there are other possible manifestations of the general problem (forgetting about set-based selectors) that this test won't prevent in the future. For example, if we regress to pulling the labels from MatchLabels without converting via string format/parse, this test won't catch that.

I think we'll catch more potential regressions if we include test cases with no MatchLabels at all, so we're sure it will break if any link in the chain ignores the set-based MatchExpressions. We could do that by converting all entries in labels into MatchExpressions (instead of just the first pair), and leaving MatchLabels empty.

Also, we should leave a comment here saying that we are intentionally using a selector with empty MatchLabels to make sure MatchExpressions works as expected, to reduce the chance that someone will weaken our test in the future.

@enisoc Updated! :)

enisoc · 2018-02-20T19:28:48Z

pkg/controller/statefulset/stateful_set_utils.go

 	cr, err := history.NewControllerRevision(set,
 		controllerKind,
-		selector,
+		podLabels,


You can directly put set.Spec.Template.Labels here.

kow3ns · 2018-02-26T20:57:55Z

/approve

janetkuo · 2018-02-26T21:00:14Z

/lgtm

enisoc

/lgtm

k8s-ci-robot · 2018-02-26T21:00:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayushpateria, enisoc, janetkuo, kow3ns

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/controller/history/OWNERS~~ [enisoc,janetkuo,kow3ns]
~~pkg/controller/statefulset/OWNERS~~ [enisoc,janetkuo,kow3ns]

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

enisoc · 2018-02-26T21:48:16Z

The verify failure looks like #60447. Looks like e2e-gce didn't even run.

/test pull-kubernetes-e2e-gce

BenTheElder · 2018-02-26T22:18:13Z

fix PR for verify merged
/test pull-kubernetes-verify

k8s-github-robot · 2018-02-27T17:39:11Z

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

k8s-github-robot · 2018-02-27T18:21:00Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

lavalamp · 2018-02-27T23:18:06Z

pkg/controller/statefulset/stateful_set_utils.go

 	cr, err := history.NewControllerRevision(set,
 		controllerKind,
-		selector,
+		set.Spec.Template.Labels,


What happens if the selector doesn't match these labels?

Validation prevents a StatefulSet with a selector that doesn't match its own template labels from being created. apps/v1 StatefulSet's selector cannot be changed after creation.

Fix StatefulSet set selector bug

8de89d9

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 5, 2018

k8s-ci-robot requested review from smarterclayton and tnozicka February 5, 2018 19:52

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 5, 2018

k8s-ci-robot assigned cheftako Feb 5, 2018

cheftako reviewed Feb 6, 2018

View reviewed changes

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Feb 6, 2018

enisoc reviewed Feb 6, 2018

View reviewed changes

k8s-ci-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 Feb 8, 2018

ayushpateria force-pushed the patch-sts branch from a89d796 to 16a1e75 Compare February 8, 2018 23:16

0xmichalis reviewed Feb 9, 2018

View reviewed changes

pkg/controller/history/controller_history.go Outdated

Copy link

Contributor

0xmichalis Feb 9, 2018

Choose a reason for hiding this comment

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

Remove this diff

0xmichalis reviewed Feb 9, 2018

View reviewed changes

Pass pod labels to controller revision

4f84a1c

ayushpateria force-pushed the patch-sts branch from 16a1e75 to 4f84a1c Compare February 9, 2018 09:24

ayushpateria force-pushed the patch-sts branch from b8ec12a to eed99b8 Compare February 11, 2018 23:45

ayushpateria commented Feb 12, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 19, 2018

ayushpateria force-pushed the patch-sts branch from eed99b8 to 52ed164 Compare February 19, 2018 21:11

janetkuo suggested changes Feb 20, 2018

View reviewed changes

enisoc suggested changes Feb 20, 2018

View reviewed changes

ayushpateria force-pushed the patch-sts branch from 84bc5b7 to c6dfd51 Compare February 21, 2018 23:07

Modify tests

a269491

ayushpateria force-pushed the patch-sts branch from c6dfd51 to a269491 Compare February 22, 2018 13:49

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2018

janetkuo approved these changes Feb 26, 2018

View reviewed changes

k8s-ci-robot assigned janetkuo Feb 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018

k8s-ci-robot assigned enisoc Feb 26, 2018

enisoc approved these changes Feb 26, 2018

View reviewed changes

kow3ns added this to the v1.10 milestone Feb 26, 2018

kow3ns added approved-for-milestone area/stateful-apps kind/bug Categorizes issue or PR as related to a bug. labels Feb 26, 2018

k8s-ci-robot added status/approved-for-milestone and removed approved-for-milestone labels Feb 26, 2018

k8s-github-robot merged commit 249ecab into kubernetes:master Feb 27, 2018

lavalamp reviewed Feb 27, 2018

View reviewed changes

tnozicka mentioned this pull request Mar 1, 2018

UPSTREAM: 59365: Fix StatefulSet set-based selector bug openshift/origin#18797

Merged

Conversation

ayushpateria commented Feb 5, 2018 • edited by enisoc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lavalamp commented Feb 5, 2018

Uh oh!

lavalamp commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheftako Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushpateria Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushpateria Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Feb 9, 2018

Uh oh!

ayushpateria commented Feb 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enisoc commented Feb 19, 2018

Uh oh!

ayushpateria commented Feb 19, 2018

Uh oh!

ayushpateria commented Feb 20, 2018

Uh oh!

janetkuo commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janetkuo Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushpateria Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enisoc Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushpateria commented Feb 5, 2018 •

edited by enisoc

Loading

cheftako Feb 6, 2018 •

edited

Loading

ayushpateria Feb 6, 2018 •

edited

Loading

ayushpateria Feb 8, 2018 •

edited

Loading

janetkuo Feb 20, 2018 •

edited

Loading

ayushpateria Feb 20, 2018 •

edited

Loading

enisoc Feb 20, 2018 •

edited

Loading