Populate NodeAffinity on top of labels for cloud based PersistentVolumes by ddebroy · Pull Request #64226 · kubernetes/kubernetes

ddebroy · 2018-05-23T20:22:03Z

What this PR does / why we need it:

This PR populates the NodeAffinity field (on top of the existing labels) for PVs backed by cloud providers like EC2 EBS and GCE PD.

Special notes for your reviewer:
Related to #63232

Sample describe pv output for EBS with node affinity field populated:

kubectl describe pv pv0001
Name:              pv0001
Labels:            failure-domain.beta.kubernetes.io/region=us-west-2
                   failure-domain.beta.kubernetes.io/zone=us-west-2a
Annotations:       <none>
Finalizers:        [kubernetes.io/pv-protection]
StorageClass:      
Status:            Available
Claim:             
Reclaim Policy:    Retain
Access Modes:      RWO
Capacity:          5Gi
Node Affinity:     
  Required Terms:  
    Term 0:        failure-domain.beta.kubernetes.io/zone in [us-west-2a]
                   failure-domain.beta.kubernetes.io/region in [us-west-2]
Message:           
Source:
    Type:       AWSElasticBlockStore (a Persistent Disk resource in AWS)
    VolumeID:   vol-00cf03a068c62cbe6
    FSType:     ext4
    Partition:  0
    ReadOnly:   false
Events:         <none>

/sig storage
/assign @msau42

Release note:
NONE

msau42 · 2018-05-23T20:23:52Z

/ok-to-test

msau42 · 2018-05-23T20:52:00Z

/assign @verult

msau42 · 2018-05-23T21:35:04Z

pkg/controller/cloud/pvlcontroller.go

So on GCE, we support Regional Disks, which are replicated in two zones. To represent that, we actually stuffed both zones in a single label.

You'll have to treat the zone label specially. And you'll need to use volumeutil.LabelZonesToSet in order to parse this label format. And then, each zone should be included as a separate element in the Values. See the example pv node affinity here

Drawing parallels with the find*Labels() methods, we could have something like translate*Labels() that implement plugin-specific translation logic like this.

msau42 · 2018-05-23T21:38:42Z

pkg/controller/cloud/pvlcontroller.go

You can use append on requirements instead of keeping track of i

msau42 · 2018-05-23T21:40:34Z

pkg/controller/cloud/pvlcontroller.go

We should only process/set NodeAffinity if VolumeScheduling feature is enabled.

krmayankk · 2018-05-23T22:25:18Z

pkg/controller/cloud/pvlcontroller.go

how are we verifying this doesnt conflict with user provided labels and overwrites them ?

We should make sure these labels passed in are the ones given by the cloud provider.

You also bring up a good point. If the user already specified a NodeAffinity in the PV, we should probably not override it.

Good point indeed. I did think about potential conflicts between user-provided labels and those applied by the system but based on this unit-test I thought overriding is(/was) preferred way: https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go#L161

I discussed this briefly with @saad-ali and @verult. We think it's better if there's any conflict between user provided and cloud provider provided values, then we should error instead of blindly overwriting. Users should know that they made a configuration mistake because it could indicate that they are accidentally using the wrong volume than intended.

Sounds good. Thinking about how to detect conflicts and "merge" with an arbitrary pre-specified affinity in a safe way, the following scheme should work. Thoughts?

Scan through each NodeSelectorTerm in NodeAffinity.Required: Create a map <NodeSelectorRequirement, bool> populated with NodeSelectorRequirement s corresponding to cloud specific labels to record if a matching NodeSelectorRequirement has been found in current NodeSelectorTerm Scan through each NodeSelectorRequirement in the NodeSelectorTerm: If NodeSelectorRequirement.Key matches but operator or values do not match, return error due to conflict. If key, operator, value of NodeSelectorRequirement matches entry in map, mark it in map. Append NodeSelectorRequirement entries from map that have not yet been marked/matched with NodeSelectorRequirements (corresponding to cloud specific label) in current NodeSelectorTerm

This should result in the NodeSelectorRequirement entries corresponding to cloud specific labels being "and"ed to each NodeSelectorTerm entry of an arbitrarily specified NodeAffinity.Required block.

This could work; alternatively maybe we just want to leave the NodeAffinity alone if the user already specified something?

verult

Is there a need for a way to convert from PV NodeAffinity back to zone labels? For example, when a new volume is created with VolumeScheduling enabled, AFAIU only has PV NodeAffinity, no labels. If the feature is then turned off, the cluster still needs labels to schedule correctly.

verult · 2018-05-24T21:30:03Z

pkg/controller/cloud/pvlcontroller.go

Drawing parallels with the find*Labels() methods, we could have something like translate*Labels() that implement plugin-specific translation logic like this.

verult · 2018-05-24T21:44:29Z

pkg/controller/cloud/pvlcontroller.go

This could work; alternatively maybe we just want to leave the NodeAffinity alone if the user already specified something?

msau42 · 2018-05-24T21:59:15Z

For example, when a new volume is created with VolumeScheduling enabled, AFAIU only has PV NodeAffinity, no labels. If the feature is then turned off, the cluster still needs labels to schedule correctly.

We should still keep the old behavior with labels for backwards compatibility exactly for that reason

ddebroy · 2018-05-30T16:19:38Z

alternatively maybe we just want to leave the NodeAffinity alone if the user already specified something

@verult I will update the code with the above logic - it is certainly simple. I did want to confirm a couple of things around this:

If the user specifies a NodeAffinity with any NodeSelectorRequirement (even completely unrelated to the cloud zones/regions) for a PV, we will completely skip appending/merging any NodeSelectorRequirement related to the clouds into the PV.NodeAffinity. The user in this case is therefore expected to manually specify NodeSelectorRequirements appropriately for the cloud.
Is it possible, that just like the PV labelling controllers, there maybe another component/controller that tries to assign certain NodeSelectorRequirements to PV.NodeAffinity? If so, without any coordination between the controllers, there may be inconsistent behavior as the controller that gets to process the PVs first will append it's own NodeSelectorRequirements while the other ones back off upon detecting a NodeAffinity already populated. One way to resolve this would be to have a common "merging-of-NodeSelectorRequirements" mechanism for affinity that all controllers use so that the user will end up with consistent results. Let me know if this is too advanced of a scenario right now and okay to postpone for later.

verult · 2018-06-04T21:23:48Z

Thanks for the great points

If the user specifies a NodeAffinity with any NodeSelectorRequirement (even completely unrelated to the cloud zones/regions) for a PV, we will completely skip appending/merging any NodeSelectorRequirement related to the clouds into the PV.NodeAffinity. The user in this case is therefore expected to manually specify NodeSelectorRequirements appropriately for the cloud.

Right, an argument could be made that if a user is already aware of PV NodeAffinity and is working with it, it's reasonable to expect them specify it correctly and completely.

As for (2), regardless of whether a controller exists today, it'd be nice to be future-proof here and avoid headaches if another controller writing to PV NodeAffinity is created in the future. Now I'm leaning towards your original proposed merging method for this reason. As long as the process is only additive to the field we should be OK. One thing to consider, as @msau42 pointed out offline, is there are multiple ways to express the same topology using NodeAffinity. See here for an example with CSI, but this issue could come up with Regional PDs.

msau42 · 2018-06-05T19:08:18Z

pkg/controller/cloud/pvlcontroller_test.go

Check nil ptrs when dereferencing each field. Same for all tests

msau42 · 2018-06-05T19:10:05Z

pkg/controller/cloud/pvlcontroller_test.go

You could sort the two lists and do a DeepEqual

ddebroy · 2018-06-05T22:03:24Z

I will go ahead and implement the additive/merge logic for NodeSelectorRequirement elements corresponding to the zone and regions labels as discussed above. Will update this PR.

ddebroy · 2018-06-12T22:54:40Z

I have implemented the merge mechanism for NodeSelectorRequirements corresponding to the cloud labels as discussed earlier.

msau42 · 2018-06-18T18:53:11Z

lgtm, go ahead and squash the commits

@verult could you test this out with repd?

ddebroy · 2018-06-18T20:34:55Z

commits squashed

verult · 2018-06-21T00:03:13Z

Manually tested using regional PDs for the following cases:

Dynamically provisioned regional PD
Pre-provisioned PV without NodeAffinity
Pre-provisioned PV with only zone in NodeAffinity; works as expected - region information is not filled in
Pre-provisioned PV with both zone and region info.

Everything looks good!

msau42 · 2018-06-21T13:19:27Z

/lgtm

ddebroy · 2018-06-21T18:24:27Z

/assign @thockin @derekwaynecarr

verult

Minor comments, LGTM otherwise

verult · 2018-06-21T20:28:26Z

pkg/controller/cloud/pvlcontroller.go

nit: This block can be moved to line 249

verult · 2018-06-21T20:29:26Z

pkg/controller/cloud/pvlcontroller.go

nit: populateAffinity := utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) && len(volLabels) != 0, so you don't need the populateAffinity = true later

verult · 2018-06-21T20:39:24Z

pkg/apis/core/v1/helper/helpers_test.go

nit: What about if reqs is empty?

added case to return false for empty reqs

verult · 2018-06-21T20:47:28Z

pkg/controller/cloud/pvlcontroller_test.go

nit:

if tc.affinityMerged { if obj.Spec.NodeAffinity == nil { .... } if obj.Spec.NodeAffinity.Required == nil { ...

block removed as no longer necessary

verult · 2018-06-21T20:56:51Z

pkg/controller/cloud/pvlcontroller_test.go

I'd avoid this logic here - it's part of the same logic we want to test. Instead of affinityMerged, what if we had expectedNodeAffinity? We should be able to directly verify the result without having to analyze implementation details in the test like merging.

Hardcoded expectations and matching with target using deepequal.

ddebroy · 2018-06-26T23:31:25Z

Addressed CR comments from @verult, rebased and squashed commits

Signed-off-by: Deep Debroy <ddebroy@docker.com>

verult · 2018-06-29T00:26:31Z

/lgtm

Thanks Deep!

ddebroy · 2018-06-29T18:15:43Z

/assign @thockin

ddebroy · 2018-07-02T15:45:39Z

/assign @erictune @smarterclayton @luxas @wlan0 @deads2k

Adding some more folks for review/approval based on the OWNERS files mentioned above.

thockin · 2018-07-09T16:49:21Z

/approve

k8s-ci-robot · 2018-07-09T16:50:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, msau42, thockin, verult

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/apis/core/OWNERS~~ [thockin]
~~pkg/controller/cloud/OWNERS~~ [thockin]
~~plugin/pkg/admission/OWNERS~~ [thockin]

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 · 2018-07-09T19:16:02Z

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

k8s-ci-robot assigned msau42 May 23, 2018

k8s-ci-robot requested review from derekwaynecarr and thockin May 23, 2018 20:22

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 May 23, 2018

k8s-ci-robot assigned verult May 23, 2018

msau42 reviewed May 23, 2018

View reviewed changes

krmayankk reviewed May 23, 2018

View reviewed changes

verult reviewed May 24, 2018

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 30, 2018

msau42 reviewed Jun 5, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 12, 2018

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

k8s-ci-robot assigned derekwaynecarr and thockin Jun 21, 2018

verult suggested changes Jun 21, 2018

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and 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 Jun 24, 2018

Populate NodeAffinity on top of labels for cloud based PersistentVolumes

074dc64

Signed-off-by: Deep Debroy <ddebroy@docker.com>

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

k8s-ci-robot assigned deads2k, erictune, luxas, smarterclayton and wlan0 Jul 2, 2018

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

k8s-github-robot merged commit 349d6a6 into kubernetes:master Jul 9, 2018

ddebroy mentioned this pull request Aug 28, 2018

REQUEST: New membership for ddebroy kubernetes/org#46

Closed

6 tasks

Conversation

ddebroy commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msau42 commented May 23, 2018

Uh oh!

msau42 commented May 23, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddebroy May 23, 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!

verult left a comment

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!

msau42 commented May 24, 2018

Uh oh!

ddebroy commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

verult commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddebroy commented Jun 5, 2018

Uh oh!

ddebroy commented Jun 12, 2018

Uh oh!

msau42 commented Jun 18, 2018

Uh oh!

ddebroy commented Jun 18, 2018

Uh oh!

verult commented Jun 21, 2018

Uh oh!

msau42 commented Jun 21, 2018

Uh oh!

ddebroy commented Jun 21, 2018

Uh oh!

verult left a comment

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!

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!

Choose a reason for hiding this comment

ddebroy commented May 23, 2018 •

edited

Loading

ddebroy May 23, 2018 •

edited

Loading

ddebroy commented May 30, 2018 •

edited

Loading

ddebroy commented Jul 2, 2018 •

edited

Loading