Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify handling of CSI migration feature gate #3139

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Unify handling of CSI migration feature gate #3139

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d38489
Unify handling of CSI migration feature gate
gnufied Dec 17, 2024
2a5de93
Fix the unit tests
gnufied Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/csi/service/common/commonco/coagnostic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
restclient "k8s.io/client-go/rest"

cnsvolume "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco/k8sorchestrator"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco/types"
Expand All @@ -47,6 +48,9 @@ type COCommonInterface interface {
IsPVCSIFSSEnabled(ctx context.Context, featureName string) bool
// EnableFSS helps enable feature state switch in the FSS config map
EnableFSS(ctx context.Context, featureName string) error
// Check if CSI migration is enabled. In true multi-VC clusters CSI
// migration will be disabled by default
IsCSIMigrationEnabled(ctx context.Context, cfg *config.Config) bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSI migration is already GA. We are not supposed to disable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't work in multi-VC clusters though, regardless of feature gate state. That is why syncer logs those stacktraces in logs. So, the right thing to do here IMO is to disable the state when multiple vCenters are present.

// DisableFSS helps disable feature state switch in the FSS config map
// This method is added for Unit tests coverage
DisableFSS(ctx context.Context, featureName string) error
Expand Down
17 changes: 17 additions & 0 deletions pkg/csi/service/common/commonco/k8sorchestrator/k8sorchestrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
cnsoperatorv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
cnsvolume "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
cnsconfig "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/logger"
Expand Down Expand Up @@ -1359,6 +1360,22 @@ func (c *K8sOrchestrator) IsFakeAttachAllowed(ctx context.Context, volumeID stri
return false, nil
}

func (c *K8sOrchestrator) IsCSIMigrationEnabled(ctx context.Context, cfg *config.Config) bool {
log := logger.GetLogger(ctx)
isMultiVCenterFssEnabled := c.IsFSSEnabled(ctx, common.MultiVCenterCSITopology)
isMigrationEnabled := c.IsFSSEnabled(ctx, common.CSIMigration)

if !isMultiVCenterFssEnabled {
return isMigrationEnabled
} else {
if len(cfg.VirtualCenter) > 1 {
log.Infof("Disabling CSI migration in multi-vc clusters")
return false
}
return isMigrationEnabled
}
}

// MarkFakeAttached updates the pvc corresponding to volume to have a fake
// attach annotation.
func (c *K8sOrchestrator) MarkFakeAttached(ctx context.Context, volumeID string) error {
Expand Down
23 changes: 17 additions & 6 deletions pkg/csi/service/vanilla/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/node"
cnsvolume "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
cnsvsphere "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
cnsconfig "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
csifault "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/fault"
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/prometheus"
Expand Down Expand Up @@ -131,7 +132,8 @@ func (c *controller) Init(config *cnsconfig.Config, version string) error {
// Check if the feature states are enabled.
multivCenterCSITopologyEnabled = commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx,
common.MultiVCenterCSITopology)
csiMigrationEnabled = commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration)
csiMigrationEnabled = commonco.ContainerOrchestratorUtility.IsCSIMigrationEnabled(ctx, config)

filterSuspendedDatastores = commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx,
common.CnsMgrSuspendCreateVolume)
isTopologyAwareFileVolumeEnabled = commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx,
Expand Down Expand Up @@ -328,6 +330,7 @@ func (c *controller) Init(config *cnsconfig.Config, version string) error {
log.Errorf("failed to watch on path: %q. err=%v", cfgDirPath, err)
return err
}

if commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration) {
if !multivCenterCSITopologyEnabled {
log.Info("CSI Migration Feature is Enabled. Loading Volume Migration Service")
Expand Down Expand Up @@ -510,6 +513,14 @@ func (c *controller) filterDatastores(ctx context.Context, sharedDatastores []*c
return filteredDatastores, nil
}

func (c *controller) getCNSConfig() *config.Config {
if multivCenterCSITopologyEnabled {
return c.managers.CnsConfig
} else {
return c.manager.CnsConfig
}
}

// createBlockVolume creates a block volume based on the CreateVolumeRequest.
func (c *controller) createBlockVolume(ctx context.Context, req *csi.CreateVolumeRequest) (
*csi.CreateVolumeResponse, string, error) {
Expand All @@ -523,7 +534,7 @@ func (c *controller) createBlockVolume(ctx context.Context, req *csi.CreateVolum

// Check if the feature states are enabled.
isBlockVolumeSnapshotEnabled := commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.BlockVolumeSnapshot)
csiMigrationFeatureState := commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration)
csiMigrationFeatureState := commonco.ContainerOrchestratorUtility.IsCSIMigrationEnabled(ctx, c.getCNSConfig())

// Check if requested volume size and source snapshot size matches
volumeSource := req.GetVolumeContentSource()
Expand Down Expand Up @@ -1587,7 +1598,7 @@ func (c *controller) createFileVolume(ctx context.Context, req *csi.CreateVolume

// Fetching the feature state for csi-migration before parsing storage class
// params.
csiMigrationFeatureState := commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration)
csiMigrationFeatureState := commonco.ContainerOrchestratorUtility.IsCSIMigrationEnabled(ctx, c.getCNSConfig())
scParams, err := common.ParseStorageClassParams(ctx, req.Parameters, csiMigrationFeatureState)
// TODO: Need to figure out the fault returned by ParseStorageClassParams.
// Currently, just return "csi.fault.Internal".
Expand Down Expand Up @@ -2075,7 +2086,7 @@ func (c *controller) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequ
volumeType = prometheus.PrometheusBlockVolumeType
cnsVolumeType = common.BlockVolumeType
// In-tree volume support.
if !commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration) {
if !commonco.ContainerOrchestratorUtility.IsCSIMigrationEnabled(ctx, c.getCNSConfig()) {
// Migration feature switch is disabled.
return nil, csifault.CSIInternalFault, logger.LogNewErrorCodef(log, codes.Internal,
"volume-migration feature switch is disabled. Cannot use volume with vmdk path :%q", req.VolumeId)
Expand Down Expand Up @@ -2257,7 +2268,7 @@ func (c *controller) ControllerPublishVolume(ctx context.Context, req *csi.Contr
volumeType = prometheus.PrometheusBlockVolumeType
if strings.Contains(req.VolumeId, ".vmdk") {
// In-tree volume support.
if !commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration) {
if !commonco.ContainerOrchestratorUtility.IsCSIMigrationEnabled(ctx, c.getCNSConfig()) {
// Migration feature switch is disabled.
return nil, csifault.CSIInternalFault, logger.LogNewErrorCodef(log, codes.Internal,
"volume-migration feature switch is disabled. Cannot use volume with vmdk path :%q", req.VolumeId)
Expand Down Expand Up @@ -2392,7 +2403,7 @@ func (c *controller) ControllerUnpublishVolume(ctx context.Context, req *csi.Con
} else {
// In-tree volume support.
volumeType = prometheus.PrometheusBlockVolumeType
if !commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration) {
if !commonco.ContainerOrchestratorUtility.IsCSIMigrationEnabled(ctx, c.getCNSConfig()) {
// Migration feature switch is disabled.
return nil, csifault.CSIInternalFault, logger.LogNewErrorCodef(log, codes.Internal,
"volume-migration feature switch is disabled. Cannot use volume with vmdk path: %q", req.VolumeId)
Expand Down
3 changes: 1 addition & 2 deletions pkg/syncer/fullsync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ func CsiFullSync(ctx context.Context, metadataSyncer *metadataSyncInformer, vc s
var err error
// Fetch CSI migration feature state, before performing full sync operations.
if metadataSyncer.clusterFlavor == cnstypes.CnsClusterFlavorVanilla {
if metadataSyncer.coCommonInterface.IsFSSEnabled(ctx, common.CSIMigration) &&
len(metadataSyncer.configInfo.Cfg.VirtualCenter) == 1 {
if metadataSyncer.coCommonInterface.IsCSIMigrationEnabled(ctx, metadataSyncer.configInfo.Cfg) {
migrationFeatureStateForFullSync = true
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/syncer/metadatasyncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func InitMetadataSyncer(ctx context.Context, clusterFlavor cnstypes.CnsClusterFl

if clusterFlavor == cnstypes.CnsClusterFlavorVanilla {
isMultiVCenterFssEnabled = commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.MultiVCenterCSITopology)
IsMigrationEnabled = commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.CSIMigration)
IsMigrationEnabled = commonco.ContainerOrchestratorUtility.IsCSIMigrationEnabled(ctx, configInfo.Cfg)
}
isStorageQuotaM2FSSEnabled = commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.StorageQuotaM2)
// Create the kubernetes client from config.
Expand Down
8 changes: 4 additions & 4 deletions pkg/syncer/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func getPVsInBoundAvailableOrReleased(ctx context.Context,
}
for _, pv := range allPVs {
if (pv.Spec.CSI != nil && pv.Spec.CSI.Driver == csitypes.Name) ||
(metadataSyncer.coCommonInterface.IsFSSEnabled(ctx, common.CSIMigration) && pv.Spec.VsphereVolume != nil &&
(metadataSyncer.coCommonInterface.IsCSIMigrationEnabled(ctx, metadataSyncer.configInfo.Cfg) && pv.Spec.VsphereVolume != nil &&
isValidvSphereVolume(ctx, pv)) {
log.Debugf("FullSync: pv %v is in state %v", pv.Name, pv.Status.Phase)
if pv.Status.Phase == v1.VolumeBound || pv.Status.Phase == v1.VolumeAvailable ||
Expand Down Expand Up @@ -156,7 +156,7 @@ func IsValidVolume(ctx context.Context, volume v1.Volume, pod *v1.Pod,
// Verify volume is a in-tree VCP volume.
if pv.Spec.VsphereVolume != nil {
// Check if migration feature switch is enabled.
if metadataSyncer.coCommonInterface.IsFSSEnabled(ctx, common.CSIMigration) {
if metadataSyncer.coCommonInterface.IsCSIMigrationEnabled(ctx, metadataSyncer.configInfo.Cfg) {
if !isValidvSphereVolume(ctx, pv) {
return false, nil, nil
}
Expand Down Expand Up @@ -599,7 +599,7 @@ func getPVsInBoundAvailableOrReleasedForVc(ctx context.Context, metadataSyncer *
for _, pv := range allPvs {
// Check if the PV is an in-tree volume.
if pv.Spec.CSI == nil {
if metadataSyncer.coCommonInterface.IsFSSEnabled(ctx, common.CSIMigration) &&
if metadataSyncer.coCommonInterface.IsCSIMigrationEnabled(ctx, metadataSyncer.configInfo.Cfg) &&
pv.Spec.VsphereVolume != nil {
return nil, logger.LogNewErrorf(log,
"In-tree volumes are not supported on a multi VC set up."+
Expand Down Expand Up @@ -785,7 +785,7 @@ func createMissingFileVolumeInfoCrs(ctx context.Context, metadataSyncer *metadat
fileVolumes := make([]*v1.PersistentVolume, 0)
for _, pv := range allPvs {
if pv.Spec.CSI == nil {
if metadataSyncer.coCommonInterface.IsFSSEnabled(ctx, common.CSIMigration) &&
if metadataSyncer.coCommonInterface.IsCSIMigrationEnabled(ctx, metadataSyncer.configInfo.Cfg) &&
pv.Spec.VsphereVolume != nil {
log.Errorf("In-tree volumes are not supported on a multi VC set up."+
"Found in-tree volume %s.", pv.Name)
Expand Down