Add Cinder to PersistentVolumeLabel Admission Controller#73102
Add Cinder to PersistentVolumeLabel Admission Controller#73102k8s-ci-robot merged 4 commits intokubernetes:masterfrom
Conversation
ac73063 to
c3dcf86
Compare
|
I like this trade-off @andrewsykim will look deeper when i get a chance. thanks! |
thanks! |
4457e3b to
3dac8d3
Compare
|
@dims comments addressed, PTAL! I ended up refactoring the unit tests a bit to account for other cloud providers. |
There was a problem hiding this comment.
might want to review this file in split mode
|
@andrewsykim |
|
It also caches the pvlabeler (by assigning it to a field). |
|
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. |
There was a problem hiding this comment.
this will panic if the interface is not implemented. seems like we want an error, as before
There was a problem hiding this comment.
do a conditional interface assertion so we can return an error instead of panicking?
24c7acc to
47871cb
Compare
47871cb to
ee1e9cf
Compare
|
@dims can I get another review please? |
|
/lgtm i am ok to leave the multiple pvlabler fields for now. thanks! |
There was a problem hiding this comment.
you can leave me off of this owners file, and there should probably be a storage reviewer, right?
There was a problem hiding this comment.
Fixed! @msau42 agreed to be in the OWNERS file :)
ee1e9cf to
070f4fd
Compare
070f4fd to
467a3e5
Compare
|
/retest |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm (re-applying) |
Due to kubernetes/kubernetes#72972 the alpha `Initializers` feature will be entirely removed with Kubernetes 1.14. We are currently relying on it as the cloud-controller-manager uses it to label persistent volumes before they become visible in the API. This functionality is also removed (see kubernetes/kubernetes#73102) with 1.14. As a consequence, in order to use PV labelling until the community has migrated all CCMs to use `MutatingWebhookConfiguration`s for that, the only way is to enable the (already deprecated) `PersistentVolumeLabel` admission controller in the kube-apiserver. That means that the kube-apiserver needs to talk to the cloud provider to figure out the region/zone of the PV (and later add them as label). --> We have to provide the cloud credentials + config to the API server in this case. Shoot clusters with Kubernetes version < 1.14 still have an enabled initializer feature.
Due to kubernetes/kubernetes#72972 the alpha `Initializers` feature will be entirely removed with Kubernetes 1.14. We are currently relying on it as the cloud-controller-manager uses it to label persistent volumes before they become visible in the API. This functionality is also removed (see kubernetes/kubernetes#73102) with 1.14. As a consequence, in order to use PV labelling until the community has migrated all CCMs to use `MutatingWebhookConfiguration`s for that, the only way is to enable the (already deprecated) `PersistentVolumeLabel` admission controller in the kube-apiserver. That means that the kube-apiserver needs to talk to the cloud provider to figure out the region/zone of the PV (and later add them as label). --> We have to provide the cloud credentials + config to the API server in this case. Shoot clusters with Kubernetes version < 1.14 still have an enabled initializer feature.
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
PersistentVolumeLabeladmission controller instead, the tradeoff is that we'll be able to remove usages of Initializers.Also refactors the admission controller to:
cloudprovider.PVLabelerfor consistency. Right now we have the labeling logic split in two places (the admission controller and the cloud providers).Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
/sig openstack
/sig cloud-provider