New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Aug 14, 2018

Conversation

@kangarlou
Contributor

kangarlou commented Jul 30, 2018

What this PR does / why we need it:
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 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

Changed admission controller to allow volume expansion for all volume…
… 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

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Jul 30, 2018

Contributor

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.


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.

Contributor

k8s-ci-robot commented Jul 30, 2018

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.


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

This comment has been minimized.

Show comment
Hide comment
@wongma7

wongma7 Jul 30, 2018

Contributor

/ok-to-test

Contributor

wongma7 commented Jul 30, 2018

/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

This comment has been minimized.

Show comment
Hide comment
@wongma7

wongma7 Jul 30, 2018

Contributor

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

Contributor

wongma7 commented Jul 30, 2018

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

@gnufied

This comment has been minimized.

Show comment
Hide comment
@gnufied

gnufied Jul 30, 2018

Member

@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.

Member

gnufied commented Jul 30, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Jul 30, 2018

Contributor

@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.

Contributor

kangarlou commented Jul 30, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Jul 30, 2018

Contributor

/retest

Contributor

kangarlou commented Jul 30, 2018

/retest

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Jul 31, 2018

Contributor

@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.

Contributor

kangarlou commented Jul 31, 2018

@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.

@kangarlou kangarlou referenced this pull request Aug 1, 2018

Closed

Resizing of NFS share. #21

@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.

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

This comment has been minimized.

@gnufied

gnufied Aug 2, 2018

Member

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.

@gnufied

gnufied Aug 2, 2018

Member

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.

This comment has been minimized.

@wongma7

wongma7 Aug 2, 2018

Contributor

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

@wongma7

wongma7 Aug 2, 2018

Contributor

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

This comment has been minimized.

@gnufied

gnufied Aug 2, 2018

Member

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.

@gnufied

gnufied Aug 2, 2018

Member

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.

This comment has been minimized.

@kangarlou

kangarlou Aug 2, 2018

Contributor

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

kangarlou Aug 2, 2018

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 2, 2018

Contributor

@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.

Contributor

kangarlou commented Aug 2, 2018

@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.

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 3, 2018

Contributor

/retest

Contributor

kangarlou commented Aug 3, 2018

/retest

@gnufied

This comment has been minimized.

Show comment
Hide comment
@gnufied

gnufied Aug 3, 2018

Member

/lgtm

Member

gnufied commented Aug 3, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 3, 2018

@gnufied

This comment has been minimized.

Show comment
Hide comment
@gnufied

gnufied Aug 3, 2018

Member

@kangarlou can you please squash your commits?

Member

gnufied commented Aug 3, 2018

@kangarlou can you please squash your commits?

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 3, 2018

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 3, 2018

Contributor

@gnufied squashed and rebased. Thanks!

Contributor

kangarlou commented Aug 3, 2018

@gnufied squashed and rebased. Thanks!

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 3, 2018

Contributor

/retest

Contributor

kangarlou commented Aug 3, 2018

/retest

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 6, 2018

Contributor

/assign @derekwaynecarr

Contributor

kangarlou commented Aug 6, 2018

/assign @derekwaynecarr

@gnufied

This comment has been minimized.

Show comment
Hide comment
@gnufied

gnufied Aug 8, 2018

Member

/lgtm

Member

gnufied commented Aug 8, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 8, 2018

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 8, 2018

Contributor

/retest

Contributor

kangarlou commented Aug 8, 2018

/retest

@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Aug 9, 2018

Member

/approve

/assign @deads2k @derekwaynecarr
for admission approval.

Member

jsafrane commented Aug 9, 2018

/approve

/assign @deads2k @derekwaynecarr
for admission approval.

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 13, 2018

Contributor

/retest

Contributor

kangarlou commented Aug 13, 2018

/retest

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Aug 14, 2018

Contributor

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

Contributor

deads2k commented Aug 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 14, 2018

Contributor

[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:

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

Contributor

k8s-ci-robot commented Aug 14, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 14, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 14, 2018

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

@kangarlou

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 14, 2018

Contributor

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.

Contributor

kangarlou commented Aug 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@kangarlou

kangarlou Aug 14, 2018

Contributor

/retest

Contributor

kangarlou commented Aug 14, 2018

/retest

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 14, 2018

Contributor

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.

Contributor

k8s-merge-robot commented Aug 14, 2018

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-merge-robot k8s-merge-robot merged commit 76434bd into kubernetes:master Aug 14, 2018

16 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-integration
Details
pull-kubernetes-integration Job triggered.
Details
cla/linuxfoundation kangarlou authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 14, 2018

Contributor

@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.

Contributor

k8s-ci-robot commented Aug 14, 2018

@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.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment