Disable in-tree plugin without enabling CSI migration #98243

Jiawei0227 · 2021-01-20T23:00:54Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This commit replaces all the alpha CSIMigrationXXXComplete
flag with InTreePluginXXUnregister flag. This new flag will
be a superset of the CSIMigrationXXXComplete. But this
decouple the plugin unregister from CSI migration. So
if a K8s distribution want to go directly with CSI and
do not support in-tree, they can use this flag directly.

For the only beta feature CSIMigrationvSphereComplete, we
deprecate it and it will be removed in 1.22.

Testing:

Enable the InTreePluginXXUnregister and not CSIMigrationXXX,
verify that the PVC using old plugin name will have error
saying cannot find the plugin
Enable both the InTreePluginXXUnregister and CSIMigrationXXX
verify that the PVC using old plugin name will start to use
the migrated CSI plugin

Which issue(s) this PR fixes:

Fixes #96730

Special notes for your reviewer:
We will deprecate the CSIMigrationvSphereComplete feature flag.

Does this PR introduce a user-facing change?:

ACTION REQUIRED: Remove alpha CSIMigrationXXComplete flag and add alpha InTreePluginXXUnregister flag. Deprecate CSIMigrationvSphereComplete flag and it will be removed in 1.22.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

TODO:
Remove CSIMigrationvSphereComplete flag after deprecation over.

/sig storage
/assign @msau42 @mattcary

Jiawei0227 · 2021-01-21T01:47:23Z

/cc @gnufied @xing-yang

xing-yang · 2021-01-21T16:18:39Z

/cc @divyenpatel

fedebongio · 2021-01-21T21:09:02Z

/remove-sig api-machinery

mattcary · 2021-01-21T21:29:17Z

cmd/kubelet/app/plugins_providers.go

 	if err != nil {
 		klog.Warningf("Unexpected CSI Migration Feature Flags combination detected: %v. CSI Migration may not take effect", err)
 		// TODO: fail and return here once alpha only tests can set the feature flags for a plugin correctly
 	}
-	if featureGate.Enabled(features.CSIMigration) && featureGate.Enabled(pluginMigration) && featureGate.Enabled(pluginMigrationComplete) {
-		klog.Infof("Skip registration of plugin %s since feature flag %v is enabled", inTreePluginName, pluginMigrationComplete)
+	if featureGate.Enabled(pluginInfo.pluginUnregisterFeature) {


Can we combine this and the vSphere-specific check at line 53 into a single function in csimigration? Then instead of copying these two stanzas across cmd/kubelet, cmd/kube-controller-manager, etc etc we have the logic consolidated into one place. That will make it easier to remove the vSphere flag anyway.

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

…omplete for Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

…re{Disk,File}Complete See for more details: kubernetes/kubernetes#98243

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

…re{Disk,File}Complete See for more details: kubernetes/kubernetes#98243

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

…re{Disk,File}Complete See for more details: kubernetes/kubernetes#98243

…omplete for Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

A bunch of flags were removed/renamed by kubernetes/kubernetes#98243

andyzhangx · 2021-07-19T02:15:44Z

@Jiawei0227 @msau42 We(Azure disk & file drivers) would like to Enable both the InTreePluginXXUnregister and CSIMigrationXXX, while block in-tree Azure Disk & file dynamic PV creation, is that possible? One solution could be add one special tag in TranslateInTreePVToCSI func, and return error in CSI CreateVolume func if it's in-tree PV request, does it work? We want to add such a tag in 1.22 first, and finally CSI driver could control whether we could block in-tree PV request or not.

andyzhangx · 2021-07-19T12:22:11Z

@Jiawei0227 @msau42 We(Azure disk & file drivers) would like to Enable both the InTreePluginXXUnregister and CSIMigrationXXX, while block in-tree Azure Disk & file dynamic PV creation, is that possible? One solution could be add one special tag in TranslateInTreePVToCSI func, and return error in CSI CreateVolume func if it's in-tree PV request, does it work? We want to add such a tag in 1.22 first, and finally CSI driver could control whether we could block in-tree PV request or not.

anyway, I worked out a PR to add a migration flag in csi migration logic: #103768, and then CSI driver could decide whether disable CreateVolume for csi migrated PV requests.

Jiawei0227 · 2021-07-19T17:38:53Z

We(Azure disk & file drivers) would like to Enable both the InTreePluginXXUnregister and CSIMigrationXXX, while block in-tree Azure Disk & file dynamic PV creation, is that possible?

If you enable both InTreePluginXXUnregister and CSIMigrationXXX, the XXX intree plugin will not be registered anymore. So there will not be any intree disk & file dynamic PV creation for sure. In fact, if you only enable InTreePluginXXUnregister, it will unregister the intree plugin XXX. Adding CSIMigrationXXX to it will enable the CSI migration part for the intree so that if anyone is still using the intree plugin, it can get redirect to CSI driver.

One solution could be add one special tag in TranslateInTreePVToCSI func, and return error in CSI CreateVolume func if it's in-tree PV request, does it work?

I dont think this is necessary, why do you want to do this? Enabling InTreePluginXXUnregister and CSIMigrationXXX means CSI migration is complete. There will be no intree plugin code used any more

andyzhangx · 2021-07-19T23:58:47Z

InTreePluginXXUnregister

@Jiawei0227 thanks for the clarification, so if both InTreePluginXXUnregister and CSIMigrationXXX are enabled, existing in-tree PVs still works while we cannot create dynamic in-tree volumes by the in-tree driver anymore, right?

msau42 · 2021-07-23T17:48:27Z

Enabling both flags means provisioning with in-tree API will go through the CSI driver.

Jiawei0227 · 2021-07-26T18:32:49Z

@Jiawei0227 thanks for the clarification, so if both InTreePluginXXUnregister and CSIMigrationXXX are enabled, existing in-tree PVs still works while we cannot create dynamic in-tree volumes by the in-tree driver anymore, right?

@andyzhangx , Sorry for the late reply. Enable both means, all intree traffic will be redirected to CSI driver. The intree will not even be registered. So the intree code will never be able to execute anymore.

andyzhangx · 2022-02-14T08:30:33Z

We(Azure disk & file drivers) would like to Enable both the InTreePluginXXUnregister and CSIMigrationXXX, while block in-tree Azure Disk & file dynamic PV creation, is that possible?

If you enable both InTreePluginXXUnregister and CSIMigrationXXX, the XXX intree plugin will not be registered anymore. So there will not be any intree disk & file dynamic PV creation for sure. In fact, if you only enable InTreePluginXXUnregister, it will unregister the intree plugin XXX. Adding CSIMigrationXXX to it will enable the CSI migration part for the intree so that if anyone is still using the intree plugin, it can get redirect to CSI driver.

One solution could be add one special tag in TranslateInTreePVToCSI func, and return error in CSI CreateVolume func if it's in-tree PV request, does it work?

I dont think this is necessary, why do you want to do this? Enabling InTreePluginXXUnregister and CSIMigrationXXX means CSI migration is complete. There will be no intree plugin code used any more

@Jiawei0227 I tried --feature-gates="CSIMigration=true,CSIMigrationAzureDisk=true,InTreePluginAzureDiskUnregister=true" with kube-controller-manager, and it could still create dynamic PV by in-tree storage class, I think that's not my original intention. Is there a way to only disallow dynamic PV by in-tree storage class, while make existing in-tree PV attach/mount working? Looks like I still needs this PR? #103768
cc @msau42

andyzhangx · 2022-02-14T08:32:28Z

the IsMigrationCompleteForPlugin func is only called in attach/detach scenario, it won't disallow dynamic PV by in-tree storage class:

kubernetes/pkg/controller/volume/attachdetach/util/util.go

Lines 333 to 338 in c592bd4

    
           if csiMigratedPluginManager.IsMigrationCompleteForPlugin(pluginName) { 
        
           	// All nodes are expected to have migrated CSI plugin installed and 
        
           	// configured when CSI Migration Complete flag is enabled for a plugin. 
        
           	// CSI migration is supported even if there is version skew between 
        
           	// managers and node. 
        
           	return true, nil

k8s-ci-robot assigned mattcary and msau42 Jan 20, 2021

k8s-ci-robot requested review from brendandburns and humblec January 20, 2021 23:02

ehashman added this to Waiting on Author in SIG Node PR Triage Jan 20, 2021

Jiawei0227 force-pushed the unregister-plugin branch from 4efdb6b to 8cead17 Compare January 21, 2021 01:44

Jiawei0227 changed the title ~~[WIP]Disable in-tree plugin without enabling CSI migration~~ Disable in-tree plugin without enabling CSI migration Jan 21, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2021

k8s-ci-robot requested review from gnufied and xing-yang January 21, 2021 01:47

k8s-ci-robot requested a review from divyenpatel January 21, 2021 16:18

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 21, 2021

mattcary reviewed Jan 21, 2021

View reviewed changes

Jiawei0227 force-pushed the unregister-plugin branch from 8cead17 to e48e367 Compare January 22, 2021 02:24

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 22, 2021

SIG Node PR Triage automation moved this from Needs Approver to Done Jan 29, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 29, 2021

xmudrii mentioned this pull request Feb 19, 2021

Add CCM/CSI migration logic kubermatic/kubermatic#6535

Merged

Jiawei0227 mentioned this pull request Mar 9, 2021

Remove CSIMigrationvSphereComplete flag and use InTreePluginvSphereUnregister instead #99982

Closed

rfranzke mentioned this pull request Apr 9, 2021

☂️-Issue for "Support for Kubernetes v1.21” gardener/gardener#3851

Closed

rfranzke added a commit to gardener/gardener-extension-provider-aws that referenced this pull request Apr 9, 2021

Use InTreePluginAWSUnregister instead of CSIMigrationAWSComplete for …

0a6da48

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-aws that referenced this pull request Apr 9, 2021

Use InTreePluginAWSUnregister instead of CSIMigrationAWSComplete for …

89e4837

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-openstack that referenced this pull request Apr 9, 2021

Use InTreePluginOpenStackUnregister instead of CSIMigrationOpenStackC…

a27e216

…omplete for Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-gcp that referenced this pull request Apr 9, 2021

Use InTreePluginGCEUnregister instead of CSIMigrationGCEComplete for …

067e389

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-azure that referenced this pull request Apr 9, 2021

Use InTreePluginAzure{Disk,File}Unregister instead of CSIMigrationAzu…

2aed6b5

…re{Disk,File}Complete See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-aws that referenced this pull request Apr 9, 2021

Use InTreePluginAWSUnregister instead of CSIMigrationAWSComplete for …

1d147b8

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-gcp that referenced this pull request Apr 9, 2021

Use InTreePluginGCEUnregister instead of CSIMigrationGCEComplete for …

47976ac

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-azure that referenced this pull request Apr 9, 2021

Use InTreePluginAzure{Disk,File}Unregister instead of CSIMigrationAzu…

7ce21dc

…re{Disk,File}Complete See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-aws that referenced this pull request Apr 14, 2021

Use InTreePluginAWSUnregister instead of CSIMigrationAWSComplete for …

d2af058

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-gcp that referenced this pull request Apr 14, 2021

Use InTreePluginGCEUnregister instead of CSIMigrationGCEComplete for …

39abe31

…Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-azure that referenced this pull request Apr 14, 2021

Use InTreePluginAzure{Disk,File}Unregister instead of CSIMigrationAzu…

a9a9651

…re{Disk,File}Complete See for more details: kubernetes/kubernetes#98243

rfranzke added a commit to gardener/gardener-extension-provider-openstack that referenced this pull request Apr 14, 2021

Use InTreePluginOpenStackUnregister instead of CSIMigrationOpenStackC…

2401d1d

…omplete for Kubernetes >= 1.21 See for more details: kubernetes/kubernetes#98243

divyenpatel mentioned this pull request May 5, 2021

remove CSIMigrationvSphereComplete flag #101735

Closed

Porges added a commit to Porges/website that referenced this pull request May 31, 2021

Update feature-gates.md

f6572b4

A bunch of flags were removed/renamed by kubernetes/kubernetes#98243

Porges mentioned this pull request May 31, 2021

Update feature-gates.md to document renaming of feature gates kubernetes/website#28199

Closed

Jiawei0227 mentioned this pull request Jul 19, 2021

add migration flag in Azure volume CSI migration logic #103768

Closed

andyzhangx mentioned this pull request Feb 24, 2022

add migration flag in Azure volume CSI migration logic #108317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable in-tree plugin without enabling CSI migration #98243

Disable in-tree plugin without enabling CSI migration #98243

Jiawei0227 commented Jan 20, 2021 •

edited

Jiawei0227 commented Jan 21, 2021

xing-yang commented Jan 21, 2021

fedebongio commented Jan 21, 2021

mattcary Jan 21, 2021

Jiawei0227 Jan 22, 2021

andyzhangx commented Jul 19, 2021 •

edited

andyzhangx commented Jul 19, 2021

Jiawei0227 commented Jul 19, 2021

andyzhangx commented Jul 19, 2021

msau42 commented Jul 23, 2021

Jiawei0227 commented Jul 26, 2021

andyzhangx commented Feb 14, 2022

andyzhangx commented Feb 14, 2022

Disable in-tree plugin without enabling CSI migration #98243

Disable in-tree plugin without enabling CSI migration #98243

Conversation

Jiawei0227 commented Jan 20, 2021 • edited

Jiawei0227 commented Jan 21, 2021

xing-yang commented Jan 21, 2021

fedebongio commented Jan 21, 2021

mattcary Jan 21, 2021

Choose a reason for hiding this comment

Jiawei0227 Jan 22, 2021

Choose a reason for hiding this comment

andyzhangx commented Jul 19, 2021 • edited

andyzhangx commented Jul 19, 2021

Jiawei0227 commented Jul 19, 2021

andyzhangx commented Jul 19, 2021

msau42 commented Jul 23, 2021

Jiawei0227 commented Jul 26, 2021

andyzhangx commented Feb 14, 2022

andyzhangx commented Feb 14, 2022

Jiawei0227 commented Jan 20, 2021 •

edited

andyzhangx commented Jul 19, 2021 •

edited