From fa1a4100c6c30f0053acf2547a4012b7d724503d Mon Sep 17 00:00:00 2001 From: Jiawei Wang Date: Tue, 20 Apr 2021 00:17:31 -0700 Subject: [PATCH] Remove CSIMigrationVSphereComplete flag --- .../app/plugins_providers.go | 25 ++-- cmd/kubelet/app/plugins_providers.go | 24 ++-- pkg/features/kube_features.go | 8 -- pkg/volume/csimigration/plugin_manager.go | 16 +-- .../csimigration/plugin_manager_test.go | 119 ++++++++---------- 5 files changed, 74 insertions(+), 118 deletions(-) diff --git a/cmd/kube-controller-manager/app/plugins_providers.go b/cmd/kube-controller-manager/app/plugins_providers.go index 915e1b715cb..69660eaaf41 100644 --- a/cmd/kube-controller-manager/app/plugins_providers.go +++ b/cmd/kube-controller-manager/app/plugins_providers.go @@ -36,20 +36,15 @@ import ( type probeFn func() []volume.VolumePlugin func appendPluginBasedOnFeatureFlags(plugins []volume.VolumePlugin, inTreePluginName string, featureGate featuregate.FeatureGate, pluginInfo pluginInfo) ([]volume.VolumePlugin, error) { - // Skip appending the in-tree plugin to the list of plugins to be probed/initialized - // if the CSIMigration feature flag and plugin specific feature flag indicating - // CSI migration is complete - migrationComplete, err := csimigration.CheckMigrationFeatureFlags(featureGate, pluginInfo.pluginMigrationFeature, - pluginInfo.pluginMigrationCompleteFeature, pluginInfo.pluginUnregisterFeature) + + _, err := csimigration.CheckMigrationFeatureFlags(featureGate, pluginInfo.pluginMigrationFeature, pluginInfo.pluginUnregisterFeature) 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 } - // TODO: This can be removed after feature flag CSIMigrationvSphereComplete is removed. - if migrationComplete { - klog.Infof("Skip registration of plugin %s since migration is complete", inTreePluginName) - return plugins, nil - } + + // Skip appending the in-tree plugin to the list of plugins to be probed/initialized + // if the plugin unregister feature flag is set if featureGate.Enabled(pluginInfo.pluginUnregisterFeature) { klog.Infof("Skip registration of plugin %s since feature flag %v is enabled", inTreePluginName, pluginInfo.pluginUnregisterFeature) return plugins, nil @@ -59,11 +54,9 @@ func appendPluginBasedOnFeatureFlags(plugins []volume.VolumePlugin, inTreePlugin } type pluginInfo struct { - pluginMigrationFeature featuregate.Feature - // deprecated, only to keep here for vSphere - pluginMigrationCompleteFeature featuregate.Feature - pluginUnregisterFeature featuregate.Feature - pluginProbeFunction probeFn + pluginMigrationFeature featuregate.Feature + pluginUnregisterFeature featuregate.Feature + pluginProbeFunction probeFn } func appendAttachableLegacyProviderVolumes(allPlugins []volume.VolumePlugin, featureGate featuregate.FeatureGate) ([]volume.VolumePlugin, error) { @@ -72,7 +65,7 @@ func appendAttachableLegacyProviderVolumes(allPlugins []volume.VolumePlugin, fea pluginMigrationStatus[plugins.GCEPDInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationGCE, pluginUnregisterFeature: features.InTreePluginGCEUnregister, pluginProbeFunction: gcepd.ProbeVolumePlugins} pluginMigrationStatus[plugins.CinderInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationOpenStack, pluginUnregisterFeature: features.InTreePluginOpenStackUnregister, pluginProbeFunction: cinder.ProbeVolumePlugins} pluginMigrationStatus[plugins.AzureDiskInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationAzureDisk, pluginUnregisterFeature: features.InTreePluginAzureDiskUnregister, pluginProbeFunction: azuredd.ProbeVolumePlugins} - pluginMigrationStatus[plugins.VSphereInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationvSphere, pluginMigrationCompleteFeature: features.CSIMigrationvSphereComplete, pluginUnregisterFeature: features.InTreePluginvSphereUnregister, pluginProbeFunction: vsphere_volume.ProbeVolumePlugins} + pluginMigrationStatus[plugins.VSphereInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationvSphere, pluginUnregisterFeature: features.InTreePluginvSphereUnregister, pluginProbeFunction: vsphere_volume.ProbeVolumePlugins} var err error for pluginName, pluginInfo := range pluginMigrationStatus { diff --git a/cmd/kubelet/app/plugins_providers.go b/cmd/kubelet/app/plugins_providers.go index 7261cd13b6a..98564409de3 100644 --- a/cmd/kubelet/app/plugins_providers.go +++ b/cmd/kubelet/app/plugins_providers.go @@ -42,20 +42,14 @@ type probeFn func() []volume.VolumePlugin func appendPluginBasedOnFeatureFlags(plugins []volume.VolumePlugin, inTreePluginName string, featureGate featuregate.FeatureGate, pluginInfo pluginInfo) ([]volume.VolumePlugin, error) { - // Skip appending the in-tree plugin to the list of plugins to be probed/initialized - // if the CSIMigration feature flag and plugin specific feature flag indicating - // CSI migration is complete - migrationComplete, err := csimigration.CheckMigrationFeatureFlags(featureGate, pluginInfo.pluginMigrationFeature, - pluginInfo.pluginMigrationCompleteFeature, pluginInfo.pluginUnregisterFeature) + _, err := csimigration.CheckMigrationFeatureFlags(featureGate, pluginInfo.pluginMigrationFeature, pluginInfo.pluginUnregisterFeature) if err != nil { klog.InfoS("Unexpected CSI Migration Feature Flags combination detected, CSI Migration may not take effect", "err", err) // TODO: fail and return here once alpha only tests can set the feature flags for a plugin correctly } - // TODO: This can be removed after feature flag CSIMigrationvSphereComplete is removed. - if migrationComplete { - klog.InfoS("Skipped registration of plugin since migration is completed", "pluginName", inTreePluginName) - return plugins, nil - } + + // Skip appending the in-tree plugin to the list of plugins to be probed/initialized + // if the plugin unregister feature flag is set if featureGate.Enabled(pluginInfo.pluginUnregisterFeature) { klog.InfoS("Skipped registration of plugin since feature flag is enabled", "pluginName", inTreePluginName, "featureFlag", pluginInfo.pluginUnregisterFeature) return plugins, nil @@ -66,11 +60,9 @@ func appendPluginBasedOnFeatureFlags(plugins []volume.VolumePlugin, inTreePlugin } type pluginInfo struct { - pluginMigrationFeature featuregate.Feature - // deprecated, only to keep here for vSphere - pluginMigrationCompleteFeature featuregate.Feature - pluginUnregisterFeature featuregate.Feature - pluginProbeFunction probeFn + pluginMigrationFeature featuregate.Feature + pluginUnregisterFeature featuregate.Feature + pluginProbeFunction probeFn } func appendLegacyProviderVolumes(allPlugins []volume.VolumePlugin, featureGate featuregate.FeatureGate) ([]volume.VolumePlugin, error) { @@ -80,7 +72,7 @@ func appendLegacyProviderVolumes(allPlugins []volume.VolumePlugin, featureGate f pluginMigrationStatus[plugins.CinderInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationOpenStack, pluginUnregisterFeature: features.InTreePluginOpenStackUnregister, pluginProbeFunction: cinder.ProbeVolumePlugins} pluginMigrationStatus[plugins.AzureDiskInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationAzureDisk, pluginUnregisterFeature: features.InTreePluginAzureDiskUnregister, pluginProbeFunction: azuredd.ProbeVolumePlugins} pluginMigrationStatus[plugins.AzureFileInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationAzureFile, pluginUnregisterFeature: features.InTreePluginAzureFileUnregister, pluginProbeFunction: azure_file.ProbeVolumePlugins} - pluginMigrationStatus[plugins.VSphereInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationvSphere, pluginMigrationCompleteFeature: features.CSIMigrationvSphereComplete, pluginUnregisterFeature: features.InTreePluginvSphereUnregister, pluginProbeFunction: vsphere_volume.ProbeVolumePlugins} + pluginMigrationStatus[plugins.VSphereInTreePluginName] = pluginInfo{pluginMigrationFeature: features.CSIMigrationvSphere, pluginUnregisterFeature: features.InTreePluginvSphereUnregister, pluginProbeFunction: vsphere_volume.ProbeVolumePlugins} var err error for pluginName, pluginInfo := range pluginMigrationStatus { diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index ec3c7afcbaf..edb82b04865 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -376,13 +376,6 @@ const ( // Enables the vSphere in-tree driver to vSphere CSI Driver migration feature. CSIMigrationvSphere featuregate.Feature = "CSIMigrationvSphere" - // owner: @divyenpatel - // beta: v1.19 (requires: vSphere vCenter/ESXi Version: 7.0u1, HW Version: VM version 15) - // - // Disables the vSphere in-tree driver. - // Expects vSphere CSI Driver to be installed and configured on all nodes. - CSIMigrationvSphereComplete featuregate.Feature = "CSIMigrationvSphereComplete" - // owner: @divyenpatel // alpha: v1.21 // @@ -770,7 +763,6 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS CSIMigrationAzureFile: {Default: false, PreRelease: featuregate.Beta}, // Off by default (requires Azure File CSI driver) InTreePluginAzureFileUnregister: {Default: false, PreRelease: featuregate.Alpha}, CSIMigrationvSphere: {Default: false, PreRelease: featuregate.Beta}, // Off by default (requires vSphere CSI driver) - CSIMigrationvSphereComplete: {Default: false, PreRelease: featuregate.Beta}, // remove in 1.22 InTreePluginvSphereUnregister: {Default: false, PreRelease: featuregate.Alpha}, CSIMigrationOpenStack: {Default: true, PreRelease: featuregate.Beta}, InTreePluginOpenStackUnregister: {Default: false, PreRelease: featuregate.Alpha}, diff --git a/pkg/volume/csimigration/plugin_manager.go b/pkg/volume/csimigration/plugin_manager.go index 8870d12c3b3..ed0259df99d 100644 --- a/pkg/volume/csimigration/plugin_manager.go +++ b/pkg/volume/csimigration/plugin_manager.go @@ -71,7 +71,7 @@ func (pm PluginManager) IsMigrationCompleteForPlugin(pluginName string) bool { case csilibplugins.CinderInTreePluginName: return pm.featureGate.Enabled(features.InTreePluginOpenStackUnregister) case csilibplugins.VSphereInTreePluginName: - return pm.featureGate.Enabled(features.CSIMigrationvSphereComplete) || pm.featureGate.Enabled(features.InTreePluginvSphereUnregister) + return pm.featureGate.Enabled(features.InTreePluginvSphereUnregister) default: return false } @@ -152,21 +152,13 @@ func TranslateInTreeSpecToCSI(spec *volume.Spec, podNamespace string, translator // CheckMigrationFeatureFlags checks the configuration of feature flags related // to CSI Migration is valid. It will return whether the migration is complete -// by looking up the pluginMigrationComplete and pluginUnregister flag +// by looking up the pluginUnregister flag func CheckMigrationFeatureFlags(f featuregate.FeatureGate, pluginMigration, - pluginMigrationComplete, pluginUnregister featuregate.Feature) (migrationComplete bool, err error) { + pluginUnregister featuregate.Feature) (migrationComplete bool, err error) { if f.Enabled(pluginMigration) && !f.Enabled(features.CSIMigration) { return false, fmt.Errorf("enabling %q requires CSIMigration to be enabled", pluginMigration) } - // TODO: Remove the following two checks once the CSIMigrationXXComplete flag is removed - if pluginMigrationComplete != "" && f.Enabled(pluginMigrationComplete) && !f.Enabled(pluginMigration) { - return false, fmt.Errorf("enabling %q requires %q to be enabled", pluginMigrationComplete, pluginMigration) - } - // This is only needed for vSphere since we will deprecate the CSIMigrationvSphereComplete flag soon - if pluginMigrationComplete != "" && f.Enabled(features.CSIMigration) && - f.Enabled(pluginMigration) && f.Enabled(pluginMigrationComplete) { - return true, nil - } + // This is for other in-tree plugin that get migration finished if f.Enabled(pluginMigration) && f.Enabled(pluginUnregister) { return true, nil diff --git a/pkg/volume/csimigration/plugin_manager_test.go b/pkg/volume/csimigration/plugin_manager_test.go index 5844367f254..f2f4d2ce26c 100644 --- a/pkg/volume/csimigration/plugin_manager_test.go +++ b/pkg/volume/csimigration/plugin_manager_test.go @@ -142,74 +142,64 @@ func TestIsMigratable(t *testing.T) { func TestCheckMigrationFeatureFlags(t *testing.T) { testCases := []struct { - name string - pluginFeature featuregate.Feature - pluginFeatureEnabled bool - csiMigrationEnabled bool - pluginFeatureComplete featuregate.Feature - pluginFeatureCompleteEnabled bool - pluginUnregsiterFeature featuregate.Feature - pluginUnregsiterEnabled bool - expectMigrationComplete bool - expectErr bool + name string + pluginFeature featuregate.Feature + pluginFeatureEnabled bool + csiMigrationEnabled bool + pluginUnregsiterFeature featuregate.Feature + pluginUnregsiterEnabled bool + expectMigrationComplete bool + expectErr bool }{ { - name: "plugin specific feature flag enabled with migration flag disabled", - pluginFeature: features.CSIMigrationvSphere, - pluginFeatureEnabled: true, - csiMigrationEnabled: false, - pluginFeatureComplete: features.CSIMigrationvSphereComplete, - pluginFeatureCompleteEnabled: false, - pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, - pluginUnregsiterEnabled: false, - expectMigrationComplete: false, - expectErr: false, + name: "plugin specific feature flag enabled with migration flag disabled", + pluginFeature: features.CSIMigrationvSphere, + pluginFeatureEnabled: true, + csiMigrationEnabled: false, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, + pluginUnregsiterEnabled: false, + expectMigrationComplete: false, + expectErr: true, }, { - name: "plugin specific complete flag enabled but plugin specific feature flag disabled", - pluginFeature: features.CSIMigrationvSphere, - pluginFeatureEnabled: false, - csiMigrationEnabled: true, - pluginFeatureComplete: features.CSIMigrationvSphereComplete, - pluginFeatureCompleteEnabled: true, - pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, - pluginUnregsiterEnabled: false, - expectMigrationComplete: false, - expectErr: false, - }, - { - name: "plugin specific complete feature disabled but plugin specific migration feature and CSI migration flag enabled", - pluginFeature: features.CSIMigrationvSphere, - pluginFeatureEnabled: true, - csiMigrationEnabled: true, - pluginFeatureComplete: features.CSIMigrationvSphereComplete, - pluginFeatureCompleteEnabled: false, - pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, - pluginUnregsiterEnabled: false, - expectMigrationComplete: false, - expectErr: true, - }, - { - name: "all features enabled", - pluginFeature: features.CSIMigrationvSphere, - pluginFeatureEnabled: true, - csiMigrationEnabled: true, - pluginFeatureComplete: features.CSIMigrationvSphereComplete, - pluginFeatureCompleteEnabled: true, - pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, - pluginUnregsiterEnabled: true, - expectMigrationComplete: true, - expectErr: true, - }, - { - name: "no plugin feature complete set", - pluginFeature: features.CSIMigrationGCE, + name: "plugin specific migration feature and CSI migration flag both enabled with plugin unregister disabled", + pluginFeature: features.CSIMigrationvSphere, pluginFeatureEnabled: true, csiMigrationEnabled: true, - pluginUnregsiterFeature: features.InTreePluginGCEUnregister, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, + pluginUnregsiterEnabled: false, + expectMigrationComplete: false, + expectErr: false, + }, + { + name: "plugin specific migration feature and plugin unregister disabled and CSI migration flag enabled", + pluginFeature: features.CSIMigrationvSphere, + pluginFeatureEnabled: false, + csiMigrationEnabled: true, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, + pluginUnregsiterEnabled: false, + expectMigrationComplete: false, + expectErr: false, + }, + { + name: "all features enabled", + pluginFeature: features.CSIMigrationvSphere, + pluginFeatureEnabled: true, + csiMigrationEnabled: true, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, pluginUnregsiterEnabled: true, expectMigrationComplete: true, - expectErr: true, + expectErr: false, + }, + { + name: "all features disabled", + pluginFeature: features.CSIMigrationvSphere, + pluginFeatureEnabled: false, + csiMigrationEnabled: false, + pluginUnregsiterFeature: features.InTreePluginvSphereUnregister, + pluginUnregsiterEnabled: false, + expectMigrationComplete: false, + expectErr: false, }, } for _, test := range testCases { @@ -217,14 +207,11 @@ func TestCheckMigrationFeatureFlags(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIMigration, test.csiMigrationEnabled)() defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, test.pluginFeature, test.pluginFeatureEnabled)() defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, test.pluginUnregsiterFeature, test.pluginUnregsiterEnabled)() - if test.pluginFeatureComplete != "" { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, test.pluginFeatureComplete, test.pluginFeatureCompleteEnabled)() - } - migrationComplete, err := CheckMigrationFeatureFlags(utilfeature.DefaultFeatureGate, test.pluginFeature, test.pluginFeatureComplete, test.pluginUnregsiterFeature) - if err != nil && test.expectErr == true { + migrationComplete, err := CheckMigrationFeatureFlags(utilfeature.DefaultFeatureGate, test.pluginFeature, test.pluginUnregsiterFeature) + if err != nil && test.expectErr == false { t.Errorf("Unexpected error: %v", err) } - if err == nil && test.expectErr == false { + if err == nil && test.expectErr == true { t.Errorf("Unexpected validation pass") } if migrationComplete != test.expectMigrationComplete {