Skip to content

Commit

Permalink
Merge pull request #101272 from Jiawei0227/deprecateflag
Browse files Browse the repository at this point in the history 
Remove CSIMigrationvSphereComplete flag
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jun 5, 2021
2 parents e6a8bc9 + fa1a410 commit 29a8105
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 114 deletions.
25 changes: 9 additions & 16 deletions cmd/kube-controller-manager/app/plugins_providers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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 {
Expand Down
24 changes: 8 additions & 16 deletions cmd/kubelet/app/plugins_providers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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 {
Expand Down
8 changes: 0 additions & 8 deletions pkg/features/kube_features.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,13 +375,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
//
Expand Down Expand Up @@ -802,7 +795,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},
Expand Down
16 changes: 4 additions & 12 deletions pkg/volume/csimigration/plugin_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
111 changes: 49 additions & 62 deletions pkg/volume/csimigration/plugin_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,89 +142,76 @@ 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 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 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 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: "plugin specific migration feature and CSI migration flag both enabled with plugin unregister disabled",
pluginFeature: features.CSIMigrationvSphere,
pluginFeatureEnabled: true,
csiMigrationEnabled: true,
pluginUnregsiterFeature: features.InTreePluginvSphereUnregister,
pluginUnregsiterEnabled: false,
expectMigrationComplete: false,
expectErr: false,
},
{
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: "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: "no plugin feature complete set",
pluginFeature: features.CSIMigrationGCE,
name: "all features enabled",
pluginFeature: features.CSIMigrationvSphere,
pluginFeatureEnabled: true,
csiMigrationEnabled: true,
pluginUnregsiterFeature: features.InTreePluginGCEUnregister,
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 {
t.Run(fmt.Sprintf("Testing %v", test.name), func(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 {
Expand Down

0 comments on commit 29a8105

Please sign in to comment.