CSI Migration phase 2: disable probing of in-tree plugins#83098
CSI Migration phase 2: disable probing of in-tree plugins#83098k8s-ci-robot merged 1 commit intokubernetes:masterfrom ddebroy:disable-intree
Conversation
|
/assign |
|
This is not fully ready yet - pending some design discussions. |
|
/remove-sig api-machinery |
|
@jennybuckley: Those labels are not set on the issue: DetailsIn response to this:
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. |
|
@jennybuckley: Those labels are not set on the issue: DetailsIn response to this:
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. |
|
@jennybuckley: Those labels are not set on the issue: DetailsIn response to this:
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. |
|
@jennybuckley: Those labels are not set on the issue: DetailsIn response to this:
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. |
|
@jennybuckley: Those labels are not set on the issue: DetailsIn response to this:
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. |
There was a problem hiding this comment.
granted the "turn on all alpha features" CI job is an unusual case, but I'm reconsidering making this a failure given the gymnastics https://github.com/kubernetes/test-infra/pull/15286/files has to go through. Maybe just warning in these cases would be sufficient, to tell someone they've turned on a particular feature or feature complete flag that doesn't make a lot of sense
There was a problem hiding this comment.
start by calling if !IsMigrationEnabledForPlugin(plugin) { return false }?
|
/retest |
|
/test pull-kubernetes-integration |
There was a problem hiding this comment.
nothing in this method uses pm... this could be a function instead of a method, then be used by kube-controller-manager and kubelet so they don't have to maintain their own plugin -> feature name mapping
There was a problem hiding this comment.
@liggitt I completely agree that in the context of the above, IsMigrationCompleteForPlugin/IsMigrationEnabledForPlugin could be a function instead of method. However, the reason I did not make it a function is: enabling the unit testing of modules using these methods (such as the pv_controller) and align with the overall pattern introduced in #82683.
So having these as methods allows an easy way for unit tests for the controller using these to create an interface implementing IsMigrationCompleteForPlugin/IsMigrationEnabledForPlugin and use fake plugins (for example for pv_controller).
There was a problem hiding this comment.
having the feature mapping in multiple places is going to be hard to maintain if more things are added to migration (especially if implementations fall back to reporting false if an unknown plugin is asked about). I still think these should collapse to a single authoritative spot in the code, but that can be a follow up. David opened #85313 to track that.
There was a problem hiding this comment.
Totally agree about single spot in the code for the mappings rather than multiple places.
There was a problem hiding this comment.
see comment in pkg/volume/csimigration/plugin_manager.go about how to make the helper function useful so we don't have to maintain the pluginName->feature mappings in multiple places
There was a problem hiding this comment.
these could use the helper methods as well and not need to maintain feature mappings
cmd/kubelet/app/plugins_providers.go
Outdated
There was a problem hiding this comment.
and these could use the helper methods
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidz627, ddebroy, liggitt, Random-Liu, saad-ali 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 |
There was a problem hiding this comment.
I think just an unnecessary change. But maybe it's a gofmt thing.
|
/lgtm |
|
/retest |
Signed-off-by: Deep Debroy <ddebroy@docker.com>
|
/lgtm |
|
/retest |
|
@ddebroy: The following tests failed, say
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. DetailsInstructions 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. |
|
|
| // | ||
| // Kubelet does not currently need to configure volume plugins. | ||
| // If/when it does, see kube-controller-manager/app/plugins.go for example of using volume.VolumeConfig | ||
| allPlugins = appendLegacyProviderVolumes(allPlugins) |
There was a problem hiding this comment.
we didn't update the providerless build to match :(
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces a
CSIMigrationInTreeOfffeature flag. When the feature flag is set (along withCSIMigrationand plugin-specific feature flags), the in-tree storage plugins migrated to CSI are no longer probed/registered by the different storage controllers and are effectively disabled.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Allowing the storage controllers to function with unregistered plugins required a refactoring of the CSI migration framework especially for the Attach/Detach controller as detailed in kubernetes/community#4093
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: