Remove the EnableCSI feature flag

[reasonerjt]

Since v1.12 velero provides the default datamover and a framework for customized datamover, therefore the EnableCSI may not be needed anymore, we should remove this feathre flag, update the code and doc.

After that, as long as CSI plugin is installed velero should be able to snapshot and restore pv provisioned by CSI drivers.

[shawn-hurley]

Does this have to follow some deprecation policy?

[blackpiglet]

There is another issue opened to address the similar scenario:
#6585

[sseago]

@shawn-hurley First thing we need to do is actually have a deprecation policy.

The good news is we have one proposed. Now might be a good time to revisit the PR and merge it or fix it (if it still needs work):
#5532

[sseago]

Although -- if a feature flag is needed to enable (not-yet-GA) functionality, how does removing a feature flag fit in with deprecation policies? I'm imagining that once the feature is GA, the first thing to do is to enable it always and the feature flag becomes a no-op. At the same time we should probably deprecate the feature flag. Once deprecated, it would then follow the deprecation policy for when to remove it.

@shubham-pampattiwar should the deprecation policy PR explicitly address removing feature flags? Removing it because behavior has been deprecated (and eventually removed) is one use case, but removing it because the flag is no longer needed (because it's always enabled) is another one. I don't know whether those details are needed in the policy, or whether the standard "deprecate it, then remove in N releases" general policy is sufficient here.

[reasonerjt]

I used to think the feature flag is a temporary switch due to lack of confidence about the feature.

removing it because the flag is no longer needed (because it's always enabled) is another one. I don't know whether those details are needed in the policy, or whether the standard "deprecate it, then remove in N releases" general policy is sufficient here.

If we agree with this, we probably don't need to follow deprecation policy for feature flags?

[reasonerjt]

If user wants to disable CSI snapshot of velero, he/she can just skip installing the CSI plugin when velero is installed.
I don't think of a good use case where the CSI plugin installed but not turning on EnableCSI flag.

[shubham-pampattiwar]

@sseago @reasonerjt I can add a note about feature flags in the deprecation policy PR.

[sseago]

I think the right process is as I described earlier. First, always enable CSI. Feature flag does nothing, and remove all of the "if enableCSI" checks in the code. At the same time, deprecate the flag. If a user specifies it, nothing breaks, but it's no longer required to enable CSI -- as you suggested, installing the CSI plugin is all that's needed to enable CSI. Then, 2 releases later (according to the deprecation policy) we remove the flag entirely.

The reason we still need to deprecate it is that when we eventually remove it, users who are specifying the feature flag will get an error message saying that the feature they enabled is invalid. If they have 2 releases of deprecation, they'll know to stop using that flag before it's actually removed.

Essentially, any user-visible feature (via flag, CRD, etc.) should never be removed without notice. It must always be deprecated first, and then removed in a future version.

[shawn-hurley]

Another thing to add/consider, you may want to consider if someone turns it off, but has the plugin installed, exposing some warning that this flag has no semantic meaning.

[sseago]

@shawn-hurley Yes. I imagine that should be part of the deprecation warning. If a user enables that flag in the installer, we'd want to notify the user that this flag is now deprecated, the feature is always enabled, so the flag does not change any functionality, and that if they do not want to use CSI, they simply need to not add the CSI plugin. I guess a similar warning could be logged (or created as an event) in the velero pod on startup if this flag is present.

[draghuram]

I think the right process is as I described earlier. First, always enable CSI. Feature flag does nothing, and remove all of the "if enableCSI" checks in the code. At the same time, deprecate the flag. If a user

@sseago, when you say "remove "enableCSI" code, do you mean it should be replaced by a check for the CSI plug-in? If not, code to skip such as this will not work?

https://github.com/vmware-tanzu/velero/blob/main/pkg/backup/item_backupper.go#L518

[blackpiglet]

I think the code will be equal to this, after the change.
CSI is assumed to turn on by default.

	if pv.Spec.CSI != nil {
		log.Infof("Skipping snapshot of persistent volume %s, because it's handled by CSI plugin.", pv.Name)
		return nil
	}

[draghuram]

But what if CSI plugin is not installed? The above code will skip the volume and it will not be processed further.

[sseago]

@draghuram hmm. What happens if you add CSI feature flag today without CSI plugin? I don't remember whether we error our there or not. But yes, if there is no feature flag, we may need some sort of check to see whether the CSI plugin is installed, because the decision between "use CSI snapshots" vs. "use native snapshots" would be based on whether or not the CSI plugin is installed in the absence of a feature flag.

[draghuram]

Based on discussion here and also on extensive discussion in today's community call, I would like to propose the following:

• Keep the flag as is
• Package CSI plug-in with Velero obviating the need to install it separately.

I think this combination takes care of multiple issues such as:

• Enabling the flag but not installing the plug-in and vice versa.
• Installing wrong version of CSI plug-in (as we have repeatedly seen in Slack).

I can't think of any downside to packaging CSI plug-in as long as the feature flag is there. OTOH, there is a serious problem when the feature is enabled but plug-in is not installed where Velero proper will skip some PVCs, assuming they will be handled by CSI plug-in and that will not happen if the plug-in is not present.

if the CSI plug-in is not going to be packaged, backup logic should validate that CSI plugin is installed if EnableCSI flag is specified.

[shawn-hurley]

I was not on the community call, but I think that makes sense, just wanted to put my two cents here :)

[reasonerjt]

Package CSI plug-in with Velero obviating the need to install it separately.

We probably don't wanna do it in velero's binary installer, b/c it will couple the velero binary and plugin release. Currently it's possible to release a CSI plugin without bumping up velero's binary.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. If a Velero team member has requested log or more information, please provide the output of the shared commands.

[blackpiglet]

Not staled

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. If a Velero team member has requested log or more information, please provide the output of the shared commands.

[github-actions]

This issue was closed because it has been stalled for 14 days with no activity.

[GeorgiIvanovITGix]

Hi everyone,

I wanted to share our recent experience with Velero and the EnableCSI feature flag, which might be relevant to this discussion. We encountered an issue where Velero was skipping volume backups due to the presence of the --features=EnableCSI argument in the Velero deployment. This was unexpected as we hadn't installed the CSI plugin separately.

After investigating, we found that removing the EnableCSI flag from the deployment arguments and conducting a manual backup of a namespace allowed Velero to perform native snapshots of each volume successfully. We verified the snapshots were both present in Velero and visible in the Azure console.

This issue highlights the importance of ensuring that if EnableCSI is enabled, the corresponding CSI plugin must be installed and correctly configured. In our case, the presence of the flag without the plugin led to Velero skipping native snapshot backups, relying instead on an unavailable CSI mechanism.

I believe this experience underscores the need for clarity around the use of feature flags and their dependencies on plugins, as discussed here.