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

Add Cinder to PersistentVolumeLabel Admission Controller #73102

Merged
merged 4 commits into from Jan 24, 2019

Conversation

@andrewsykim
Copy link
Member

andrewsykim commented Jan 19, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Despite the fact that the PersistentVolumeLabel admission controller is deprecated, we need to add support for the Cinder volume plugin here given the out-of-tree equivalent relies on Initializers that are being removed (see #72972). We're going to need a longer-term solution for this that is out-of-tree and does not depend on Initializers but until then, labeling PVs for Cinder will have to be done via the admission controller. Note that there is a good chance that users are not even using this feature because it would have required them to enable the alpha Initializers feature. If users have been using this feature, they will have to use the PersistentVolumeLabel admission controller instead, the tradeoff is that we'll be able to remove usages of Initializers.

Also refactors the admission controller to:

  • call cloudprovider.PVLabeler for consistency. Right now we have the labeling logic split in two places (the admission controller and the cloud providers).
  • use test tables for unit tests (also includes tests for other providers)

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

If you are running the cloud-controller-manager and you have the `pvlabel.kubernetes.io` alpha Initializer enabled, you must now enable PersistentVolume labeling using the `PersistentVolumeLabel` admission controller instead. You can do this by adding `PersistentVolumeLabel` in the `--enable-admission-plugins` kube-apiserver flag. 

/sig openstack
/sig cloud-provider

@andrewsykim andrewsykim changed the title Add Cinder PersistentVolumeLabel Admission Controller Add Cinder to PersistentVolumeLabel Admission Controller Jan 19, 2019

@k8s-ci-robot k8s-ci-robot requested review from deads2k and derekwaynecarr Jan 19, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Jan 19, 2019

/assign @liggitt @dims

@andrewsykim andrewsykim force-pushed the andrewsykim:add-openstack-pvl-admission branch from ac73063 to c3dcf86 Jan 19, 2019

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 19, 2019

I like this trade-off @andrewsykim will look deeper when i get a chance. thanks!

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 19, 2019

@andrewsykim

  • Let us please split this into 2 commits, one that switches the impl to PVLabeler and another that adds the cinder support. That will help evaluate the PVLabeler changes independent of the cinder thing
  • Can we please add the cinder stuff to the test case as well.

thanks!

@andrewsykim andrewsykim force-pushed the andrewsykim:add-openstack-pvl-admission branch 2 times, most recently from 4457e3b to 3dac8d3 Jan 21, 2019

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/L labels Jan 22, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Jan 22, 2019

@dims comments addressed, PTAL! I ended up refactoring the unit tests a bit to account for other cloud providers.

@@ -17,68 +17,33 @@ limitations under the License.
package label

This comment has been minimized.

@andrewsykim

andrewsykim Jan 22, 2019

Author Member

might want to review this file in split mode

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 22, 2019

@andrewsykim getAzurePVLabeler / getGCEPVLabeler / getAWSPVLabeler / getOpenStackPVLabeler are all exactly the same except for the cloud type parameter. no?

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Jan 22, 2019

It also caches the pvlabeler (by assigning it to a field).

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Jan 22, 2019

I think it would be cleaner to have 1 field (pvlabler) as opposed to one per cloud provider but that assumes that the admission controller only admits PVs from 1 provider. I think in most clusters this is true but it seems there is an assumption before this patch that the apiserver can admit PVs from any provider.

}
l.ebsVolumes = awsCloudProvider

l.awsPVLabeler = cloudProvider.(cloudprovider.PVLabeler)

This comment has been minimized.

@liggitt

liggitt Jan 22, 2019

Member

this will panic if the interface is not implemented. seems like we want an error, as before

return nil, fmt.Errorf("error retrieving GCE cloud provider")
}
l.gceCloudProvider = gceCloudProvider
l.gcePVLabeler = cloudProvider.(cloudprovider.PVLabeler)

This comment has been minimized.

@liggitt

liggitt Jan 22, 2019

Member

do a conditional interface assertion so we can return an error instead of panicking?

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Jan 23, 2019

/assign @saad-ali @msau42

@andrewsykim andrewsykim force-pushed the andrewsykim:add-openstack-pvl-admission branch from 8f35188 to 24c7acc Jan 23, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 23, 2019

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.

@andrewsykim andrewsykim force-pushed the andrewsykim:add-openstack-pvl-admission branch from 24c7acc to 47871cb Jan 23, 2019

@andrewsykim andrewsykim force-pushed the andrewsykim:add-openstack-pvl-admission branch from 47871cb to ee1e9cf Jan 23, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Jan 24, 2019

@dims can I get another review please?

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 24, 2019

/lgtm

i am ok to leave the multiple pvlabler fields for now. thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 24, 2019

reviewers:
- andrewsykim
- dims
- liggit

This comment has been minimized.

@liggitt

liggitt Jan 24, 2019

Member

you can leave me off of this owners file, and there should probably be a storage reviewer, right?

This comment has been minimized.

@andrewsykim

andrewsykim Jan 24, 2019

Author Member

Makes sense! @msau42 @saad-ali can I add one of you here? (Or recommend someone who can) :)

This comment has been minimized.

@andrewsykim

andrewsykim Jan 24, 2019

Author Member

Fixed! @msau42 agreed to be in the OWNERS file :)

@andrewsykim andrewsykim force-pushed the andrewsykim:add-openstack-pvl-admission branch from ee1e9cf to 070f4fd Jan 24, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 24, 2019

@andrewsykim andrewsykim force-pushed the andrewsykim:add-openstack-pvl-admission branch from 070f4fd to 467a3e5 Jan 24, 2019

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 24, 2019

/retest
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 24, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, liggitt

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

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 24, 2019

/lgtm

(re-applying)

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 24, 2019

@k8s-ci-robot k8s-ci-robot merged commit 5fc286f into kubernetes:master Jan 24, 2019

18 checks passed

cla/linuxfoundation andrewsykim 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-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
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
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment