Design to add support for Multiple VolumeSnapshotClasses in CSI Plugin

[anshulahuja98]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Proposal for #5750

Please indicate you've done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
• Updated the corresponding documentation in site/content/docs/main.

[codecov-commenter]

Codecov Report

Merging #5774 (22c1f9f) into main (598333d) will decrease coverage by 0.71%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5774      +/-   ##
==========================================
- Coverage   40.67%   39.96%   -0.71%     
==========================================
  Files         243      256      +13     
  Lines       21035    23037    +2002     
==========================================
+ Hits         8555     9206     +651     
- Misses      11857    13139    +1282     
- Partials      623      692      +69     

see 77 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sseago]

For now, just a couple high level comments. I think we should not consider the explicit csi-specific fields on the Backup plugin. As long as the CSI plugin is implemented as an external plugin, we really don't want to embed specific knowledge of the plugin's internals in the core velero APIs.

Of the other two approaches, the annotation one is the most straightforward. If we go that way, we may want to allow annotations on both PVCs and Backup/Schedule CRs. That way a user can set one policy for the backup as a whole, allowing overrides per PVC. If we only do one of the two, probably the Backup level is most useful.

The "generic property bag" is the most interesting option, as it would allow other plugins to also add their own configuration parameters without having to rely on annotations or external config maps. It would be more work, but probably worthwhile if we can identify other existing plugin use cases that would also benefit from this. But the annotation approach is also perfectly acceptable, as there is precedent in using annotations for plugin configuration in other plugins.

[blackpiglet]

@anshulahuja98
Thanks for your contribution.
I prefer not to modify the Velero backup and schedule CRD to introduce the VolumeSnapshotClass chosen feature.
By far, this is only used for the CSI scenario, and CRD modification could make the Velero version upgrade process more complicated.
I think we can use the annotation on PVC and backup way, or the ConfigMap way. Velero can use some specific label or annotation to find out the correct ConfigMap.

[anshulahuja98]

@anshulahuja98 Thanks for your contribution. I prefer not to modify the Velero backup and schedule CRD to introduce the VolumeSnapshotClass chosen feature. By far, this is only used for the CSI scenario, and CRD modification could make the Velero version upgrade process more complicated. I think we can use the annotation on PVC and backup way, or the ConfigMap way. Velero can use some specific label or annotation to find out the correct ConfigMap.

"I think we can use the annotation on PVC and backup way, or the ConfigMap way. Velero can use some specific label or annotation to find out the correct ConfigMap."

@blackpiglet what are your thoughts on introducing a generic Plugin parameter field in the backup/schedule CRD?

the issue with annotating configmaps is that they always end up remaining a global setting and hard to get determinism for what is to be used for each backup

Introducing another generic field in the CRD can help bring in that determinism and be extensible to other plugins where similar disambiguation of global settings is required.

Annotations are not a very friendly way from a regular customer POV. I believe we can't even use velero CLI for such scenarios.

We discussed this in the last Community meeting - there seemed to be general consensus over the generic plugin param approach
Would love to hear your thoughts more specifically around that

[blackpiglet]

@anshulahuja98
I agree that annotation is not an ideal method.
There is an ongoing PR that also enriches the backup function by adding advanced filter rules. #5773
It uses the ConfigMap way because the configuration could inflate big enough that makes the CR modification and creation have a notifiable latency, although your case may not be that obvious.
Prefer to split the advanced configuration into a separate element from Velero CRD, but also open to the community's opinion.

[anshulahuja98]

@blackpiglet I completely understand your point of the amount of inputs to be very high and being created as a separte configmap.

keeping the advanced configuration separate from CRD I also agree with.

The point I am trying to drive here is just that, the reference to that configmap instead of through annotations, we pass it in a generic input field.

For example:

apiVersion: velero.io/v1
kind: Backup
metadata:
name: backup-1
spec:
    pluginInputs:
    - name: velero.io/advanced-filter
    - properties:
        - key: advanceFilterConfigMapReference
        - value: configMapNamespace/configMapName

In above example, the advance filter code will know which value to look for in the pluginInputs field, whicch key to search on to get the configmap reference.
TOday we might be doing things like this through annotations which are a pain to manage.

Similarly if let's say the VolumeSNapshotClassInputs increase in size we can pass reference to configmap.

apiVersion: velero.io/v1
kind: Backup
metadata:
name: backup-1
spec:
    pluginInputs:
    - name : velero.io/csi
    - properties:
        - key: snapshotClassMappingConfigMapReference
        - value: configMapNamespace/configMapName

This can further be extended to pass in StorageClass mapping configmap

Plugin inputs is a generic field which can maps a "PluginName" to a map[string]string which can hold plugin specific params. Now this can be specific parameters in case the payload is small. BUt in advanced cases we can just pass references to other objects such as configmaps/secrets

[blackpiglet]

@anshulahuja98
I totally agree with you that the annotation way is not an ideal way, and the proposed method is a better alternative.
But I'm not sure whether we should also provide the annotation way for users.
Just my thought, maybe given both annotation and CRD reference ways are better, but if there is not enough buffer for both, I think just implementing the CRD reference way is good enough.

[anshulahuja98]

I agree with your point @blackpiglet , we can do both annotations and CRD way.

WIll keep CRD route to be P0, and annotations P1, should not take too much effort to light up annotations as well.

We anyways need annotations if we wish to support VSC support at per PVC level. For overrides of global settings.
I'll work towards a more granular proposal now covering only things which we need to work on and remove other approaches from the doc.

Thank you for your comments and initial review!

[anshulahuja98]

Meanwhile are there any thoughts on how to expose the input of such "plugin inputs" through the CLI?
Any top of mind concerns/ thoughts on what might be a better user experience.
CC: @blackpiglet / @sseago

[anshulahuja98]

CC @reasonerjt / @qiuming-best for review.
If we can take any inspiration from here for the PR: #5773

[anshulahuja98]

Have updated the PR with the final design based on inputs.
Pl review @sseago / @blackpiglet

[blackpiglet]

Please add a changelog file to this PR, and fix the typo to make the Github action check to pass.

For the "plugin inputs" setting part, the Velero team had some discussion. Since there is no procedure for the Velero CRD update, we don't prefer to modify the Velero CRDs in v1.11. Because, if the CRD is changed, after the Velero version upgrade, the Velero CRD is not changed accordingly, it will complain that fails to find the updated CRD in the cluster.

[anshulahuja98]

@blackpiglet
I believe we have the upgrade-crds Job, isn't that enough in most cases to ensure CRD upgrades.

Major version should be enough to push through a CR update, I see that has happened often in the past. Is there anything specific about 1.11 due to which we don't wish to change them?

Also does this mean that PRs such as #5838 also blocked for 1.11 since they have CRD changes?

For steps forward do you wish for this design to be checked in as is and worked upon post 1.11. Or come up with alternative proposal?

[sseago]

While the new resource filtering mentioned above (with CRD changes) won't make it into 1.11, there will be CRD changes for the new async BIA/RIA plugin operations, some of which are already merged on main, so I don't think that "we don't want CRD changes for 1.11" would really apply here. For z streams (1.10.1, etc.), we definitely want to minimize, hopefully completely eliminate CRD changes, but for releases like 1.8, 1.9, 1.10, 1.11, etc. some CRD changes are always expected. The main concern is making sure that the changes are backwards-compatible, especially with regard to existing backups/PVBs, etc.

[blackpiglet]

Right. Although it's better to limit the CRD change, I also found it hard to avoid CRD change, so the CRD change is not a blocker of the PR.

For the pluginInput format, IMO, it's better to integrate configurations into one ConfigMap. What's your opinion?

[anshulahuja98]

Right. Although it's better to limit the CRD change, I also found it hard to avoid CRD change, so the CRD change is not a blocker of the PR.

For the pluginInput format, IMO, it's better to integrate configurations into one ConfigMap. What's your opinion?

Even if it's one Configmap, we still need a field to pass in that input of which coonfigmap to use which anyways needs a CRD change

For plugins which are lightweight and do not require heavy inputs, the inputs can passed in the generic property bag itself similar to BSL params or VolumeSnapshotClass params

For plugins which require heavy inputs, they can take input of a configmap reference (namespace / name) and parse it to fetch the config map. Similar to the example

In short the design does support the configmap route. While being generic to type of input.

In future plugins may need reference of other objects such as secrets etc. THis extensible design takes care of all scenarios.

Would also request you to go through the alternatives considered section in the design.

I will fix the PR with changelog / typo shortly. Hoping to go through the final review meanwhile.
Please add any other folks which can help reviewing/signing off

[anshulahuja98]

@shubham-pampattiwar can you please review this PR since you have more CSI related context.

[shubham-pampattiwar]

@anshulahuja98 Thanks for the design, added a few comments.

[shubham-pampattiwar]

+1 on the proposed behaviour

[shubham-pampattiwar]

Maybe add a note so that we update docs on how we are using the delimiters for the pluginsInputs in velero CLI

[shubham-pampattiwar]

I believe the main downside of this approach would be that we would be increasing the size of the backup CR

[anshulahuja98]

What all concerns emerge from this? Have we seen any issues earlier due to this?

[blackpiglet]

Not from the CR size, but the Restore CRD size was big before, and in some low-bandwidth environments, Velero installation fails due to API-server connection timeout.

[anshulahuja98]

I see..
We could consider reducing the comments/ description per CRD field as well in those cases.

[shubham-pampattiwar]

I wonder would it make sense to add another spec called type (plugintype) along the level of name spec.

[anshulahuja98]

here I have used name for the plugin Name
Isn't plugin type same as plugin name?

[anshulahuja98]

Went through the code, do you mean PluginKind (ObjectStore/ VolumeSnapshotter etc)

Would make the customer experience a bit more complicated is what I feel

[sseago]

@shubham-pampattiwar I'm thinking that we're dealing with limited types here, so instead of a type field, lets have a single spec field for backup item actions. If we need to support this for volume snapshotters (and we may not, since there's not really a lot of active development there) we can add a separate field for that later. Velero already calls BIA and VolumeSnapshotter plugins in completely different places, so it probably makes more sense to separate the configuration fields too.

[shubham-pampattiwar]

Maybe an implementation detail, but we should add a backup validation as well when pluginInputs are specified like they should only be accepted only if the corresponding plugin is registered with the velero controller.

[anshulahuja98]

Makes sense. Will check feasibility of such check in code and add it.

[anshulahuja98]

In the code, the plugins today seem to be queryable using the PluginKind (List for ObjectStore/ VolumeSnapshotter etc).
Querying purely based on name does not seem to be available.
most likely because the same plugin name may hold for snapshotter and object store.

So for adding this validation, we might need to take PluginKind as input. Which is slightly a bad end user experience.

[sseago]

@shubham-pampattiwar @anshulahuja98 Actually, I'm not 100% sure we need that validation. Should backups with plugin inputs fail if the plugin isn't registered? In any case, if you want this validation you'll need PluginKind. clientmgmt.Manager does have methods for retrieving registered plugins by name, but there is no grand unified list of plugins. Each PluginKind has a separate list of registered plugins. However, since we're talking about plgins for a backup or for a restore) here, we should already know the plugin kind. We're only dealing with BackupItemAction plugin inputs here. We may want to eventually support volumesnapshotter inputs too, but maybe that's a separate spec section. Call this biaPluginInputs instead of pluginInputs (and riaPluginInputs on restore), and then we already know the plugin kind. It would be BackupItemAction in the context of velero 1.10, but since this won't be implemented until post-1.11, it would be BackupItemActionV2 Basically, validation could call pluginManager.GetBackupItemActionV2(input.Name) and if it errors out, then the plugin isn't registered.

[anshulahuja98]

I think it makes sense to evaluate if we really need validation
We can stage it if needed.

[shubham-pampattiwar]

Overall the design/use-case looks sane to me but I would like to know thoughts from other maintainers as well ?
@anshulahuja98 There might be some intersection with the ongoing design regarding Volume Policies (#5773)
I am just spit-balling here but can we not integrate your use case with the Volume Policies design ? like the Condition would be the CSI driver and Action.Type could be Override and Action.Parameters could consist of the VolumeSnapshotClass that you want to use for those volumes.

(cc @blackpiglet @Lyndon-Li @sseago @reasonerjt @qiuming-best )

[anshulahuja98]

Overall the design/use-case looks sane to me but I would like to know thoughts from other maintainers as well ? @anshulahuja98 There might be some intersection with the ongoing design regarding Volume Policies (#5773) I am just spit-balling here but can we not integrate your use case with the Volume Policies design ? like the Condition would be the CSI driver and Action.Type could be Override and Action.Parameters could consist of the VolumeSnapshotClass that you want to use for those volumes.

(cc @blackpiglet @Lyndon-Li @sseago @reasonerjt @qiuming-best )

I understand where you are coming from on the intersection.
But I feel VolumeSnapshotting is a very core functionality with CSI being the path forward. And VSC param selection is a regular flow, as compared to fine grained volume control which might be used by niche set of customers.

The volumefilter approach seems very involved for a customer to setup as compared to having dedicated entries for CSI VSCs.
Additionally they will be 2 separate plugins.

However I feel that the VOlume Policy Design could benefit from the pluginInputs field that I am proposing. THey could leverage the same.

[reasonerjt]

I'm not sure if it's right thing to do to expose plugin name to the end user, that couples the interface to the implementation.

For example, if we choose to move CSI plugin into the main backup flow (not likely to happen though), it will be confusing if we continuing letting user select the vsclass via plugin input.

I would rather add a CSI field into the spec of backup and let the user choose it explicitly.

[anshulahuja98]

On the point of "couples the interface to the implementation.", even today customer has to understand which plugins to enable to add the appropriate plugin containers to the deployment. User is already in some sense expected to be aware of which component is providing which functionality.

If CSI is merged with current codebase, we would have to anyways educated brownfield customers to migrate from using the CSI plugin containers in their deployments. This change is much bigger than just honoring / not honoring the values in the pluginInputs.

The overall purpose of this spec is to introduce pluginInputs which can be leverage by other plugins without having to bloat up the backup CR, this approach is mostly plugin agnostic. CSI related functionalities are just a part of the requirement.

For CSI you might agree on backup spec, but I don't think for features such as storageclass mapping this approach would make sense.
Any suggestions as to how those scenarios can be handled?

In the alternatives consdiered below you can check other approaches we evaluated.
Introducing CSI specific fields in backup CR turned to be strong no from discussion.

[reasonerjt]

The overall purpose of this spec is to introduce pluginInputs which can be leverage by other plugins without having to bloat up the backup CR

I agree we don't want to bloat up the backup CR, that's why we want a balance between customizability and complexity. At this moment, I'm still not quite convinced we should provide a general framework so that users can customize the behavior of any plugin.

For CSI you might agree on backup spec, but I don't think for features such as storageclass mapping this approach would make sense.
Any suggestions as to how those scenarios can be handled?

As for storageclass mapping, if you are talking about modifying the resources before they are restored, user can write RIA, if we are not happy with RIA, I think the JSON-substitution proposal may be helpful, though it may not have to be an RIA.

[anshulahuja98]

"I agree we don't want to bloat up the backup CR, that's why we want a balance between customizability and complexity. At this moment, I'm still not quite convinced we should provide a general framework so that users can customize the behavior of any plugin."

Users are anyways required to do this through annotations for certain plugins - the main issue is that often these are often set as global behaviour impacting all backup/restores. Additionally annotations are not a user friendly approach to this.

For storage class mapping the existing RIA is dependent on global behavior. It fetches configmap on basis of certain label / annotation. The same issue we are facing in VolumeSnapshotClass selection.
Expecting the end user to write plugins for each scenario, understand velero code, maintaining the RIA code seems like a very big overhead.

[reasonerjt]

There could be a mismatch between the driver name in the annotation and the one in the actual vsclass, in that case the plugin will also fallback to the default one, right?

[anshulahuja98]

Yes, that is correct.

[reasonerjt]

An alternative is to extend the resource policy:
https://github.com/vmware-tanzu/velero/blob/main/design/handle-backup-of-volumes-by-resources-filters.md

So it matches certain PVCs and pass the vsclass as parameters of action csi-snapshot

But to avoid confusion I think annotations may be good enough.

[anshulahuja98]

I believe CSI snapshotting is a very common use case as compared to resource filters for volumes which are more specific in nature of their purpose.
And hence from a better customer experience POV, I'll prefer an annotations approach.

For performing basic snapshotting operations, user should not have to create and manage configmaps.

[reasonerjt]

So this implies velero should always use the same vsclass to take snapshot of PVCxxx

[anshulahuja98]

Yes, consider this an override behavior for a PVC.
In case user has configured VSC at backup level, this will override the VSC for the particular PVC.

I am not sure if you this comment something else?

[anshulahuja98]

@reasonerjt can you please add this to the 1.12 milestone.

[draghuram]

Coming a bit late to the party. I would really appreciate it if someone can summarize the change planned for 1.12 as far as this feature is concerned. Has it been decided to implement annotations at both backup and PVC level? I did try to go through all the comments but it is not easy to process as there are so many of them.

Just for reference, I would like to highlight few points:

1. It is not entirely true that CSI is implemented only through the plug-in. There is some CSI implementation in the core Velero as well. Add to this the fact that CSI is the future of storage in Kubernetes so there should not be a problem in introducing CSI specific fields in any CR. I am not saying we should but I don't think it should be summarily dismissed. In fact, I don't see much point in having CSI plug-in installed separately as snapshots are central to backups and including it in the core reduces scope for error (such as version mismatch).
2. Having annotations at PVC level or other Kubernetes objects level can lead to trouble as they have unintended consequences. Imagine a PVC included in two different backups in which case the annotation will take effect for both backups which may not be what the user intended. I have recently seen a user implementing admission webhook to remove any FSB annotations added on PVCs because he didn't want any inadvertent addition of annotations messing up his backups. Annotations at Backup level or some other way of specifying in Backup CR is fine though.

[anshulahuja98]

@draghuram,

1. We did discuss about making CSI part of the core code base - but that will be a long term item and would need more discussion and closure with the community, people did agree that over long term it might make sense to expose CSI specific params in CRD since it is a important flow for users. Hence overall the community is inclined to that, but it is not something which is prioritized as of now. And we wished to decouple it from current design of annotations.

2. The design supports both annotations on the backup and on the PVC. For most users the expectation is that they would only use the backup route. PVC annotations are for power users - if they need it, they can override it. Having that additional feature of annotation on PVC should not cause issues since it is opt in.

[blackpiglet]

The PR basically looks good to me. Need other reviewers' opinions.

[shubham-pampattiwar]

LGTM !

[anshulahuja98]

Thank you for approving the PR folks
Can one of you please help with the merge ?
@shubham-pampattiwar / @blackpiglet / @sseago

[achyut-deshpande]

Is it testable in v1.12 RC1?

[anshulahuja98]

yes @achyut-deshpande you can use v0.6.0-rc.1 release of CSI plugin to test out this functionality. Please use it in conjunction with 1.12 RC1 of core velero.

[achyut-deshpande]

Thanks, however This annotation format in backup-cr is not working
velero.io/csi-volumesnapshot-class/csi.cloud.disk.driver: csi-diskdriver-snapclass

The Backup "backup3" is invalid: metadata.annotations: Invalid value: "velero.io/csi-volumesnapshot-class/csi.cloud.disk.driver": a qualified name must consist of alphanumeric characters, '-', '_' or '.'

[anshulahuja98]

Hi @achyut-deshpande we fixed this issue later

"velero.io/csi-volumesnapshot-class_bar.csi.k8s.io": "bar2",

use an "_" instead of the "/"
vmware-tanzu/velero-plugin-for-csi#193 this got updated as part of this PR and we missed updating the design doc it seems like.

[draghuram]

I checked CSI section of Velero docs and it still mentions single volume snapshot class. Is there a docs PR for this feature?

[anshulahuja98]

@draghuram missed the docs PR due to time crunch.
Let me add now
I see overall 3 things - updating design doc, updating CSI plugin README, updating docs.

[anshulahuja98]

CSI README was already updated with basic docs, just need the bug fix
https://github.com/vmware-tanzu/velero-plugin-for-csi/blob/main/README.md