Changed admission controller to allow volume expansion for all volume plugins #66780

kangarlou · 2018-07-30T14:44:44Z

What this PR does / why we need it:
There are two motivations for this change:

CSI plugins are soon going to support volume expansion. For such plugins, admission controller doesn't know whether the plugins are capabale of supporting volume expansion or not.
Currently, admission controller rejects PVC updates for in-tree plugins that don't support volume expansion (e.g., NFS, iSCSI). This change allows external controllers to expand volumes similar to how external provisioners are accommodated.

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 #

Special notes for your reviewer:
This PR mimics the behavior of the PV controller when PVs are provisioned externally by logging and setting a new event for PVs that are being expanded externally. As SIG Storage is planning new types of operations on PVs, it may make more sense to a have a single event for all actions taken by external controllers.

Release note:

The check for unsupported plugins during volume resize has been moved from the admission controller to the two controllers that handle volume resize.

/sig storage
/assign @gnufied @jsafrane @wongma7

… plugins There are two motivations for this change: (1) CSI plugins are soon going to support volume expansion. For such plugins, admission controller doesn't know whether the plugins are capabale of supporting volume expansion or not. (2) Currently, admission controller rejects PVC updates for in-tree plugins that don't support volume expansion (e.g., NFS, iSCSI). This change allows external controllers to expand volumes similar to how external provisioners operate.

k8s-ci-robot · 2018-07-30T14:44:47Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wongma7 · 2018-07-30T14:50:44Z

/ok-to-test

wongma7

lgtm
i dont like how we have duplciate the event from two places, seems impossible to avoid with how the expand controller is written though...

wongma7 · 2018-07-30T14:57:18Z

you made need to run hack/update-bazel.sh

gnufied · 2018-07-30T16:43:06Z

@wongma7 @kangarlou it should be possible to reject the volume resize request in AddPVCUpdate to avoid code duplication in two places? the VolumeResizeMap may need VolumePluginMgr but it should not be too hard.

kangarlou · 2018-07-30T17:16:47Z

@gnufied That's actually how I implemented the code first; however, that complicated the unit tests for AddPVCUpdate. More specifically, initializing VolumePluginMgr for unit tests is not straightforward.
@wongma7 I rebased this morning and it seems the bazel tests for some recent PRs are failing. I'll look into it.

kangarlou · 2018-07-30T18:37:23Z

/retest

kangarlou · 2018-07-31T19:25:57Z

@gnufied @wongma7 The tests seem to be passing now. I'll squash the commits if everyone is ok with the current implementation.

I agree having the same logic to check valid plugins in two controllers isn't ideal, but short of a major refactoring, I don't see a better solution. Implementing this logic inside AddPVCUpdate also results in code duplication of a different kind and complicates unit tests.

One outstanding question is whether we should use a single generic event (e.g., ExternalController) for all actions handled by external controllers (e.g., provisioning, resize, snapshot). Having an event for every action supported on PVs will lead to unnecessary proliferation of events.

gnufied

I do no see why we can't just stop volume/pod from being added to resizemap inside AddPVCUpdate. Initializing volume plugin manager in tests is not very difficult. For example - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_test.go#L322

Also - preferably - we should not implement VolumeHost interface again for pvc populator.

gnufied · 2018-08-02T01:45:37Z

pkg/controller/volume/expand/pvc_populator.go

 		kubeClient: kubeClient,
 	}
-	return populator
+	if err := populator.volumePluginMgr.InitPlugins(plugins, nil, populator); err != nil {


Is there any advantage over initializing plugin manager here again rather than just passing the one that already has been initialzied in expand_controller ?

@gnufied govet doesn't like passing volumePluginMgr as an argument (see the first commit in my PR) because it results in passing a lock by value:

2018/07/30 19:02:56 Code for OpenAPI definitions generated # k8s.io/kubernetes/pkg/controller/volume/expand pkg/controller/volume/expand/expand_controller.go:145: call of NewPVCPopulator copies lock value: k8s.io/kubernetes/pkg/volume.VolumePluginMgr contains sync.Mutex pkg/controller/volume/expand/pvc_populator.go:64: NewPVCPopulator passes lock by value: k8s.io/kubernetes/pkg/volume.VolumePluginMgr contains sync.Mutex pkg/controller/volume/expand/pvc_populator.go:71: literal copies lock value from volumePluginMgr: k8s.io/kubernetes/pkg/volume.VolumePluginMgr contains sync.Mutex make[1]: *** [vet] Error 1

PVC populator implementing the VolumeHost interface is precisely to handle this situation. As I said, I'm not a fan of code duplication, but this is a side-effect of the original implementation where we have two controllers more or less doing the same job.

Can this not be worked around by passing the volume plugin manager as a pointer rather than by value?

gnufied · 2018-08-02T17:11:24Z

pkg/controller/volume/events/event.go

@@ -30,4 +30,5 @@ const (
 	ProvisioningCleanupFailed = "ProvisioningCleanupFailed"
 	ProvisioningSucceeded     = "ProvisioningSucceeded"
 	WaitForFirstConsumer      = "WaitForFirstConsumer"
+	ExternalExpanding         = "ExternalExpanding"


I think ExternalExpanding is bit vague. Can we rename this to something like VolumeResizePending or VolumeExpansionPending ? Once a external controller picks up the PVC it can go ahead and finish the process.

ExternalProvisioning is the same, I prefer ExternalExpanding for consistency. VolumeResizePending wouldn't tell you kubernetes is just waiting for an external actor to do something, so the user wouldn't know who to blame

Yeah I see your point. It can go either way. I am not sure if ExternalProvisioning name makes sense either. Kubernetes can not know for sure that a volume is being externally provisioned or resized. All it knows is - it can't do this itself.

I also went for consistency. This is how events stack up for externally provisioned volumes after my changes:

Events: Type Reason Age From Message ---- ------ ---- ---- ------- Normal ExternalProvisioning 44s persistentvolume-controller waiting for a volume to be created, either by external provisioner "netapp.io/trident" or manually created by system administrator Normal ProvisioningSuccess 36s netapp.io/trident Kubernetes frontend provisioned a volume and a PV for the PVC. Normal ExternalExpanding 14s volume_expand Ignoring the PVC: didn't find a plugin capable of expanding the volume; waiting for an external controller to process this PVC. Mounted By: <none>

kangarlou · 2018-08-02T22:32:30Z

@gnufied Good idea regarding passing *VolumePluginMgr. I made the change. I didn't change AddPVCUpdate as expand controller already needs a recorder to set events and already has to initialize VolumePluginMgr. I also don't think it makes sense to set events inside AddPVCUpdate as it unnecessarily complicates unit tests.

gnufied · 2018-08-02T23:00:24Z

pkg/controller/volume/expand/pvc_populator.go

+			glog.V(3).Infof("Ignoring the PVC %q (uid: %q) : %v.",
+				strings.JoinQualifiedName(pvc.GetNamespace(), pvc.GetName()),
+				pvc.UID, err)
+			continue


Can we make a util function or something out of this?

Done. Please see the third commit.

kangarlou · 2018-08-03T16:48:32Z

/retest

gnufied · 2018-08-03T19:34:46Z

/lgtm

gnufied · 2018-08-03T19:37:15Z

@kangarlou can you please squash your commits?

kangarlou · 2018-08-03T20:00:36Z

@gnufied squashed and rebased. Thanks!

kangarlou · 2018-08-03T22:46:31Z

/retest

kangarlou · 2018-08-06T13:54:40Z

/assign @derekwaynecarr

gnufied · 2018-08-08T17:04:48Z

/lgtm

kangarlou · 2018-08-08T18:26:36Z

/retest

jsafrane · 2018-08-09T07:33:43Z

/approve

/assign @deads2k @derekwaynecarr
for admission approval.

kangarlou · 2018-08-13T14:26:49Z

/retest

deads2k · 2018-08-14T17:17:09Z

The admission chain change removes the protection for plugins which don't have this ability. I'd like to see that in the release note. Otherwise, assuming reasonable status reporting in the controller, this ought to be ok.

/approve

k8s-ci-robot · 2018-08-14T17:17:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, gnufied, jsafrane, kangarlou

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:

~~pkg/controller/volume/OWNERS~~ [deads2k,jsafrane]
~~pkg/volume/util/OWNERS~~ [gnufied,jsafrane]
~~plugin/pkg/admission/OWNERS~~ [deads2k]

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-08-14T17:32:12Z

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

kangarlou · 2018-08-14T17:40:41Z

Thanks @deads2k. Yes, the PR moves the checks for unsupported plugins from admission controller to the two controllers that handle volume resize. These controllers log proper error messages and set the right events on PVCs.

kangarlou · 2018-08-14T19:14:54Z

/retest

k8s-github-robot · 2018-08-14T19:22:00Z

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

k8s-ci-robot · 2018-08-14T23:46:36Z

@kangarlou: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`ee747b8`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot assigned gnufied Jul 30, 2018

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 30, 2018

k8s-ci-robot assigned jsafrane Jul 30, 2018

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 30, 2018

k8s-ci-robot assigned wongma7 Jul 30, 2018

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 30, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2018

k8s-ci-robot requested review from deads2k and rootfs July 30, 2018 14:44

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 30, 2018

wongma7 approved these changes Jul 30, 2018

View reviewed changes

kangarlou mentioned this pull request Aug 1, 2018

Resizing of NFS share. NetApp/trident#21

Closed

kangarlou force-pushed the volumeresize branch from 0055f4b to 6bb1704 Compare August 1, 2018 14:02

gnufied reviewed Aug 2, 2018

View reviewed changes

kangarlou force-pushed the volumeresize branch from 6bb1704 to d0a1a95 Compare August 2, 2018 22:19

gnufied reviewed Aug 2, 2018

View reviewed changes

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

kangarlou force-pushed the volumeresize branch from 8e490c3 to ee747b8 Compare August 3, 2018 19:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2018

k8s-ci-robot assigned derekwaynecarr Aug 6, 2018

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

k8s-ci-robot assigned deads2k Aug 9, 2018

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

k8s-github-robot merged commit 76434bd into kubernetes:master Aug 14, 2018

aniket-s-kulkarni pushed a commit to aniket-s-kulkarni/kubernetes that referenced this pull request Aug 16, 2018

Adding support for flex drivers after merging kubernetes#66780

05233b2

aniket-s-kulkarni pushed a commit to aniket-s-kulkarni/kubernetes that referenced this pull request Aug 20, 2018

Adding support for flex drivers after merging kubernetes#66780

4271af0

Changed admission controller to allow volume expansion for all volume plugins #66780

Changed admission controller to allow volume expansion for all volume plugins #66780

Conversation

kangarlou commented Jul 30, 2018 • edited Loading

k8s-ci-robot commented Jul 30, 2018

wongma7 commented Jul 30, 2018

wongma7 left a comment

Choose a reason for hiding this comment

wongma7 commented Jul 30, 2018

gnufied commented Jul 30, 2018

kangarlou commented Jul 30, 2018 • edited Loading

kangarlou commented Jul 30, 2018

kangarlou commented Jul 31, 2018 • edited Loading

gnufied 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

kangarlou commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kangarlou commented Aug 3, 2018

gnufied commented Aug 3, 2018

gnufied commented Aug 3, 2018

kangarlou commented Aug 3, 2018

kangarlou commented Aug 3, 2018

kangarlou commented Aug 6, 2018

gnufied commented Aug 8, 2018

kangarlou commented Aug 8, 2018

jsafrane commented Aug 9, 2018

kangarlou commented Aug 13, 2018

deads2k commented Aug 14, 2018

k8s-ci-robot commented Aug 14, 2018

k8s-github-robot commented Aug 14, 2018

kangarlou commented Aug 14, 2018

kangarlou commented Aug 14, 2018

k8s-github-robot commented Aug 14, 2018

k8s-ci-robot commented Aug 14, 2018

kangarlou commented Jul 30, 2018 •

edited

Loading

kangarlou commented Jul 30, 2018 •

edited

Loading

kangarlou commented Jul 31, 2018 •

edited

Loading

kangarlou commented Aug 2, 2018 •

edited

Loading