Add the design of cleaning artifacts generated during CSI B/R #8676

blackpiglet · 2025-02-11T03:32:59Z

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

This is the design to resolve issue #7979.

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file (make new-changelog) or comment /kind changelog-not-required on this PR.
Updated the corresponding documentation in site/content/docs/main.

codecov · 2025-02-11T03:46:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.43%. Comparing base (5d9a4e8) to head (a6ae21e).
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8676      +/-   ##
==========================================
+ Coverage   59.39%   59.43%   +0.03%     
==========================================
  Files         370      370              
  Lines       39988    40031      +43     
==========================================
+ Hits        23752    23791      +39     
- Misses      14744    14747       +3     
- Partials     1492     1493       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

design/clean_artifacts_in_csi_flow.md

kaovilai · 2025-02-11T16:47:49Z

design/clean_artifacts_in_csi_flow.md

+	if boolptr.IsSetToFalse(input.Restore.Spec.RestorePVs) {
+		p.log.Infof("Restore did not request for PVs to be restored %s/%s",
+			input.Restore.Namespace, input.Restore.Name)
+		return &velero.RestoreItemActionExecuteOutput{SkipRestore: true}, nil
+	}


What about scenarios where cluster admin backup entire cluster expecting VSC to be in backup and be restored?
Will admin be able to restore VSCs with RestorePVs: false?

First, yes. The check will return early in the action, but the VSC will still be created in the cluster.

Second, please check this design's Non Goals first bulletin:
There were some discussions about whether Velero backup should include VSs and VSCs not generated in during the backup. By far, the conclusion is not including them is a better option. Although that is a useful enhancement, that is not included in this design.

IMO, Velero backups don't need to include the VolumeSnapshotContents not created by the running backup.
The reason is the VolumeSnapshots and VolumeSnapshotContents only take effect when they are created.

The results of VS and VSC creation are either snapshots in the storage provider or the volumes created from the snapshot specified in the VSC.

If they are included in the backup, the restore will recreate them. The recreation may not be intended.

I thought you can create VSC without resulting in new snapshot if there's already an existing snapshot id in the cloud provider

The check will return early in the action, but the VSC will still be created in the cluster.

but the return adds SkipRestore here? Just want to confirm intended behavior. I'm ok either way.

return &velero.RestoreItemActionExecuteOutput{SkipRestore: true}, nil

I thought you can create VSC without resulting in new snapshot if there's already an existing snapshot id in the cloud provider

The static VolumeSnapshot will not trigger dynamically creating a snapshot, but the dynamic VolumeSnapshot will create a new snapshot.

but the return adds SkipRestore here? Just want to confirm intended behavior. I'm ok either way.

Sorry, in this case, the actual result is skipping restore of the VS and the VSC, but that is the existing behavior. It's not changed in this design.

design/clean_artifacts_in_csi_flow.md

anshulahuja98 · 2025-02-12T06:06:43Z

design/clean_artifacts_in_csi_flow.md

+```
+
+### Backup Sync
+csi-volumesnapshotclasses.json, csi-volumesnapshotcontents.json, and csi-volumesnapshots.json are CSI-related metadata files in the BSL for each backup.


please ensure these files are not removed altogether from velero, we rely on these files for record keeping of snapshots taken.
Just putting this to make sure the design intent is not to remove these files and only called in doc to explain the need for them in delete/ restore flow

OK. Deleting the existing metadata files is considered a breaking change.
Even if this design proposes to delete them, we should follow the deprecation policy.

I don't think deprecation process is needed, b/c it's not APIs. but for v1.16 we'll keep it as is and open an issue to remove it in future release.

@anshulahuja98
Apart from whether the deprecation policy is relevant or not, it's OK to not delete the metadata files in v1.16.
Because we also have the Backup's VolumeInfo metadata, I think the downstream consumer, such as Azure, can use that to replace the CSI VSC and VS metadata files.
https://github.com/vmware-tanzu/velero/blob/main/design/Implemented/pv_backup_info.md

What's your opinion on making a change to adopt the backup VolumeInfo in the Azure future releases?

that seems reasonable, we'll work on migrating to use VolumeInfo instead of CSI JSONs.
We can target removal of files in 1.17 / 1.17+

design/clean_artifacts_in_csi_flow.md

Signed-off-by: Xun Jiang/Bruce Jiang <[email protected]>

blackpiglet · 2025-02-14T05:36:52Z

@kaovilai @anshulahuja98
PTAL.

anshulahuja98 · 2025-02-14T05:48:47Z

Replied to the pending comment, feel free to resolve after reading.
Overall while we go for impl PR, please make sure back compat for restore / Delete Backup does not break in any way.

github-actions bot added the Area/Design Design Documents label Feb 11, 2025

github-actions bot requested review from shubham-pampattiwar and ywk253100 February 11, 2025 03:33

github-actions bot assigned blackpiglet Feb 11, 2025

blackpiglet added kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes and removed Area/Design Design Documents labels Feb 11, 2025

blackpiglet force-pushed the 7979_design branch from fe4e8d2 to 43bbfa8 Compare February 11, 2025 03:36

github-actions bot added the Area/Design Design Documents label Feb 11, 2025

blackpiglet force-pushed the 7979_design branch from 43bbfa8 to c369e45 Compare February 11, 2025 03:38

Lyndon-Li reviewed Feb 11, 2025

View reviewed changes

blackpiglet force-pushed the 7979_design branch 3 times, most recently from 0b0a727 to b72f660 Compare February 11, 2025 09:53

kaovilai reviewed Feb 11, 2025

View reviewed changes

draghuram reviewed Feb 11, 2025

View reviewed changes

design/clean_artifacts_in_csi_flow.md Show resolved Hide resolved

anshulahuja98 reviewed Feb 12, 2025

View reviewed changes

design/clean_artifacts_in_csi_flow.md Outdated Show resolved Hide resolved

blackpiglet force-pushed the 7979_design branch 4 times, most recently from 7413507 to d948e76 Compare February 13, 2025 07:04

Lyndon-Li previously approved these changes Feb 13, 2025

View reviewed changes

Add the design of cleaning artifacts generated during CSI B/R

a6ae21e

Signed-off-by: Xun Jiang/Bruce Jiang <[email protected]>

blackpiglet dismissed Lyndon-Li’s stale review via a6ae21e February 13, 2025 07:45

blackpiglet force-pushed the 7979_design branch from d948e76 to a6ae21e Compare February 13, 2025 07:45

Lyndon-Li approved these changes Feb 13, 2025

View reviewed changes

anshulahuja98 approved these changes Feb 14, 2025

View reviewed changes

kaovilai approved these changes Feb 14, 2025

View reviewed changes

kaovilai merged commit 9235fe1 into vmware-tanzu:main Feb 14, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the design of cleaning artifacts generated during CSI B/R #8676

Add the design of cleaning artifacts generated during CSI B/R #8676

blackpiglet commented Feb 11, 2025 •

edited

Loading

codecov bot commented Feb 11, 2025 •

edited

Loading

kaovilai Feb 11, 2025

blackpiglet Feb 12, 2025 •

edited

Loading

kaovilai Feb 12, 2025

kaovilai Feb 12, 2025

blackpiglet Feb 13, 2025

blackpiglet Feb 13, 2025

anshulahuja98 Feb 12, 2025

blackpiglet Feb 12, 2025

reasonerjt Feb 12, 2025

blackpiglet Feb 13, 2025 •

edited

Loading

anshulahuja98 Feb 14, 2025

blackpiglet commented Feb 14, 2025

anshulahuja98 commented Feb 14, 2025

Add the design of cleaning artifacts generated during CSI B/R #8676

Add the design of cleaning artifacts generated during CSI B/R #8676

Conversation

blackpiglet commented Feb 11, 2025 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov bot commented Feb 11, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

blackpiglet Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackpiglet Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackpiglet commented Feb 14, 2025

anshulahuja98 commented Feb 14, 2025

blackpiglet commented Feb 11, 2025 •

edited

Loading

codecov bot commented Feb 11, 2025 •

edited

Loading

blackpiglet Feb 12, 2025 •

edited

Loading

blackpiglet Feb 13, 2025 •

edited

Loading