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.

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

Remove Restic dependencies in Velero plugins #5274

Closed
Lyndon-Li opened this issue Sep 1, 2022 · 9 comments · Fixed by vmware-tanzu/velero-plugin-for-csi#124 or vmware-tanzu/velero-plugin-for-gcp#105

Comments

@Lyndon-Li
Copy link
Contributor

Lyndon-Li commented Sep 1, 2022

Some Velero BackupItemAction/RestoreItemAction plugins, for example, CSI plugin, vSphere plugin, contain some code to actively check and exclude the volumes handled by Velero's Pod Volume Backup/Restore. As a result, these plugins depend on Velero's code in PodVolume/Restic package.

In v1.10, we've refactored the code, plugins need to call the new function.
It is recommended to remove the irrational "Restic" text in plugins' code.

Moreover, it is not reasonable for the plugins to depend on Velero's logic, the plugins should have no knowledge of what Velero does, for example, Pod Volume Backup/Restore. Moreover, whenever the related code changes in Velero, changes are required in the plugins.
Therefore, let's refactor the BackupItemAction/RestoreItemAction plugin interface, so that this kind of dependencies can be removed.

@reasonerjt reasonerjt added this to the 1.10 milestone Sep 5, 2022
ywk253100 added a commit to ywk253100/velero-plugin-for-microsoft-azure that referenced this issue Sep 30, 2022
1. Update the snapshotter to support VSL credential
2. Bump up golang to v1.18
3. Remove restic related

Fixes vmware-tanzu/velero#5392
Also related to vmware-tanzu/velero#5313 and vmware-tanzu/velero#5274

Signed-off-by: Wenkai Yin(尹文开) <[email protected]>
@sseago
Copy link
Collaborator

sseago commented Sep 30, 2022

@Lyndon-Li Is the suggestion that we add explicit information to the BIA/RIA interface about restic/PodVolume stuff? It's not clear to me what we're suggesting that Velero would want to provide to the plugins. Are there additional parameters that velero would supply to the plugin? What exactly would those be? (and note, that if this is to be a change to the BIA/RIA interface, it can't really be type-specific, since all resource types use the same plugin interface.

@shubham-pampattiwar Since you've done more CSI work than I have, what sort of generic plugin interface additions would eliminate the problem described above?

@Lyndon-Li
Copy link
Contributor Author

@sseago
The direct problem we see here is that some BIA/RIA involve some restic/PodVolume logics to check the volumes handled by restic/PodVolume, it is weird for some BIA/RIA that have nothing to do with restic/PodVolume to have restic/PodVolume logics.
Therefore, we want to remove these restic/PodVolume specific logics from the BIA/RIA.

I don't have a specific design for the interface change. Generally speaking, this requires the BIA/RIA interface to expose some way to exclude some volumes, so that Velero could have the logics in itself and deliver the result to BIA/RIA as volumes to be excluded.

@sseago
Copy link
Collaborator

sseago commented Oct 3, 2022

@Lyndon-Li OK. It sounds like we need to get a design proposed (and approved) for that before we could consider adding that to the API. If that happens before we're ready to approve the current v2 API, then I can add it to that design. Otherwise it can go in its own design -- still in v2 if it's in the same release cycle (i.e. we won't do v2 and v3 before Velero 1.11 releases), although it would probably be in v3 if it's not ready to implement in the 1.11 dev cycle.

@sseago
Copy link
Collaborator

sseago commented Oct 3, 2022

In any case, the main challenge would be that whatever new inputs are added to the Execute() API method need to be generic. These APIs apply to all resource types, so any API methods added should probably not be volume-specific.

@shubham-pampattiwar
Copy link
Collaborator

@Lyndon-Li
Copy link
Contributor Author

@sseago
so any API methods added should probably not be volume-specific
Agree

@Lyndon-Li
Copy link
Contributor Author

@shubham-pampattiwar
Are you talking about instances like these https://github.com/vmware-tanzu/velero-plugin-for-csi/blob/main/internal/backup/pvc_action.go#L92
Yes

@reasonerjt
Copy link
Contributor

In v1.10 we'll only consider renaming.
cc @blackpiglet
After that we'll close this issue and open another one to track the work to update the interface.

IMHO, that's not very high priority

@reasonerjt
Copy link
Contributor

AWS and Azure plugins are set
@blackpiglet let's resolve this one once GCP and CSI plugins are cleared

msdolbey pushed a commit to msdolbey/velero-plugin-for-microsoft-azure that referenced this issue Oct 28, 2022
1. Update the snapshotter to support VSL credential
2. Bump up golang to v1.18
3. Remove restic related

Fixes vmware-tanzu/velero#5392
Also related to vmware-tanzu/velero#5313 and vmware-tanzu/velero#5274

Signed-off-by: Wenkai Yin(尹文开) <[email protected]>
Signed-off-by: msdolbey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment