Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize #7046

kaovilai · 2023-11-01T17:14:14Z

Signed-off-by: Tiger Kaovilai [email protected]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #7047

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.

kaovilai · 2023-11-01T17:16:35Z

/kind changelog-not-required

Signed-off-by: Tiger Kaovilai <[email protected]>

sseago · 2023-11-02T19:23:19Z

@kaovilai I think we'd want a changelog here since this fixes a regression introduced in 1.12

kaovilai · 2023-11-02T19:25:49Z

pkg/controller/backup_finalizer_controller.go

@@ -186,7 +191,16 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	}
 	backup.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()}
 	recordBackupMetrics(log, backup, outBackupFile, r.metrics, true)
-
+	CSISnapshotsNotReady := errors.New("CSI snapshots not ready")
+	retry.OnError(retry.DefaultBackoff, func(err error) bool {


The question here is how long do we wanna wait for CSI to complete.

At this phase it is also probably safe to unblock a new backup, not sure if that's the case already.

@kaovilai no reason to wait here. All the waiting happens during WaitingForPluginOperations phase in the backup operations controller. By the time finalizer controller reconciles a backup, we're no longer waiting for snapshot completion. They're all either completed, failed explicitly, or marked failed via timeout.

We should be able to just check status a single time like we've always done in the backup controller.

…tus() and run in backup_finalizer_controller Signed-off-by: Tiger Kaovilai <[email protected]>

codecov · 2023-11-02T19:34:53Z

Codecov Report

Merging #7046 (8c72742) into main (705a3bc) will decrease coverage by 0.10%.
Report is 17 commits behind head on main.
The diff coverage is 4.54%.

@@            Coverage Diff             @@
##             main    #7046      +/-   ##
==========================================
- Coverage   61.14%   61.04%   -0.10%     
==========================================
  Files         252      256       +4     
  Lines       26916    27076     +160     
==========================================
+ Hits        16457    16528      +71     
- Misses       9295     9373      +78     
- Partials     1164     1175      +11

Files	Coverage Δ
pkg/backup/request.go	`100.00% <ø> (ø)`
pkg/controller/backup_controller.go	`61.96% <100.00%> (+0.91%)`	⬆️
pkg/controller/backup_finalizer_controller.go	`58.26% <100.00%> (+0.33%)`	⬆️
pkg/controller/backup_operations_controller.go	`46.09% <ø> (ø)`
pkg/cmd/server/server.go	`22.03% <0.00%> (ø)`
pkg/backup/snapshots.go	`0.00% <0.00%> (ø)`

... and 6 files with indirect coverage changes

kaovilai · 2023-11-02T19:36:44Z

/kind changelog-not-required
does this undo changelog required label?

sseago · 2023-11-02T19:59:01Z

pkg/controller/backup_finalizer_controller.go

@@ -186,7 +191,16 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	}
 	backup.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()}
 	recordBackupMetrics(log, backup, outBackupFile, r.metrics, true)
-
+	CSISnapshotsNotReady := errors.New("CSI snapshots not ready")
+	retry.OnError(retry.DefaultBackoff, func(err error) bool {


@kaovilai no reason to wait here. All the waiting happens during WaitingForPluginOperations phase in the backup operations controller. By the time finalizer controller reconciles a backup, we're no longer waiting for snapshot completion. They're all either completed, failed explicitly, or marked failed via timeout.

sseago · 2023-11-02T19:59:48Z

pkg/controller/backup_finalizer_controller.go

@@ -186,7 +191,16 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	}
 	backup.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()}
 	recordBackupMetrics(log, backup, outBackupFile, r.metrics, true)
-
+	CSISnapshotsNotReady := errors.New("CSI snapshots not ready")
+	retry.OnError(retry.DefaultBackoff, func(err error) bool {


We should be able to just check status a single time like we've always done in the backup controller.

pkg/controller/backup_controller.go

sseago · 2023-11-02T20:03:08Z

pkg/backup/snapshots.go

+	backup.Status.CSIVolumeSnapshotsAttempted = len(volumeSnapshots)
+	for _, vs := range volumeSnapshots {
+		if vs.Status != nil && boolptr.IsSetToTrue(vs.Status.ReadyToUse) {
+			backup.Status.CSIVolumeSnapshotsCompleted++


Since this is now run both during initial processing and finalizing, instead of incrementing the field directly, we should increment a local var and set the Spec field to this value at the end of the loop. Otherwise we'll end up double counting things.

sseago · 2023-11-02T20:04:14Z

@kaovilai I removed the changelog-not-required label.

https://github.com/vmware-tanzu/velero/pull/7046/files#r1380708644 Signed-off-by: Tiger Kaovilai <[email protected]>

Signed-off-by: Tiger Kaovilai <[email protected]>

github-actions bot requested review from shubham-pampattiwar and sseago November 1, 2023 17:14

github-actions bot assigned kaovilai Nov 1, 2023

github-actions bot added the has-unit-tests label Nov 1, 2023

kaovilai force-pushed the backup-patch-status-unittest branch from f570cf7 to a249fd6 Compare November 1, 2023 17:15

kaovilai force-pushed the backup-patch-status-unittest branch from a249fd6 to f8444e1 Compare November 1, 2023 17:16

github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Nov 1, 2023

kaovilai force-pushed the backup-patch-status-unittest branch from f8444e1 to ca5c3d2 Compare November 1, 2023 17:17

Add PatchResource unit test for backup status

886e074

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai force-pushed the backup-patch-status-unittest branch from ca5c3d2 to 886e074 Compare November 1, 2023 19:29

kaovilai marked this pull request as draft November 1, 2023 21:15

kaovilai force-pushed the backup-patch-status-unittest branch from de3ea52 to 7efe36b Compare November 2, 2023 19:12

kaovilai changed the title ~~Add PatchResource unit test for backup status~~ Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize Nov 2, 2023

kaovilai force-pushed the backup-patch-status-unittest branch from 7efe36b to 60a0232 Compare November 2, 2023 19:20

kaovilai changed the title ~~Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize~~ Update and wait for Backup.Status.CSIVolumeSnapshotsCompleted during finalize Nov 2, 2023

kaovilai force-pushed the backup-patch-status-unittest branch from 60a0232 to 3209650 Compare November 2, 2023 19:21

kaovilai marked this pull request as ready for review November 2, 2023 19:22

github-actions bot added the has-changelog label Nov 2, 2023

github-actions bot requested review from anshulahuja98 and ywk253100 November 2, 2023 19:22

kaovilai commented Nov 2, 2023

View reviewed changes

kaovilai force-pushed the backup-patch-status-unittest branch from 3209650 to 1dbcd1f Compare November 2, 2023 19:27

refactor backup snapshot status updates into UpdateBackupSnapshotsSta…

9311a42

…tus() and run in backup_finalizer_controller Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai force-pushed the backup-patch-status-unittest branch from 1dbcd1f to 9311a42 Compare November 2, 2023 19:30

sseago requested changes Nov 2, 2023

View reviewed changes

sseago removed the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Nov 2, 2023

kaovilai added 3 commits November 2, 2023 16:14

skip this if SnapshotMoveData

f9e716a

https://github.com/vmware-tanzu/velero/pull/7046/files#r1380708644 Signed-off-by: Tiger Kaovilai <[email protected]>

remove waiting during finalize

18acf00

Signed-off-by: Tiger Kaovilai <[email protected]>

CSIFeatureFlag enable check

1c138b8

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai requested a review from sseago November 2, 2023 20:21

kaovilai changed the title ~~Update and wait for Backup.Status.CSIVolumeSnapshotsCompleted during finalize~~ Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize Nov 2, 2023

update changelog to reflect removed waits

6896a1f

Signed-off-by: Tiger Kaovilai <[email protected]>

sseago previously approved these changes Nov 2, 2023

View reviewed changes

make update

cd0ad74

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai dismissed sseago’s stale review via cd0ad74 November 2, 2023 20:46

kaovilai requested a review from sseago November 2, 2023 20:46

revert test changes

8c72742

Signed-off-by: Tiger Kaovilai <[email protected]>

sseago approved these changes Nov 2, 2023

View reviewed changes

sseago approved these changes Nov 6, 2023

View reviewed changes

ywk253100 approved these changes Nov 15, 2023

View reviewed changes

ywk253100 merged commit 2a533d0 into vmware-tanzu:main Nov 15, 2023
22 of 24 checks passed

kaovilai mentioned this pull request Nov 16, 2023

release-1.12: Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize #7111

Merged

3 tasks

shubham-pampattiwar mentioned this pull request Oct 21, 2024

Nominate Tiger Kaovilai as a maintainer #8328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize #7046

Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize #7046

kaovilai commented Nov 1, 2023 •

edited

Loading

kaovilai commented Nov 1, 2023

sseago commented Nov 2, 2023

kaovilai Nov 2, 2023

sseago Nov 2, 2023

sseago Nov 2, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading

kaovilai commented Nov 2, 2023

sseago Nov 2, 2023

sseago Nov 2, 2023

sseago Nov 2, 2023

sseago commented Nov 2, 2023

Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize #7046

Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize #7046

Conversation

kaovilai commented Nov 1, 2023 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

kaovilai commented Nov 1, 2023

sseago commented Nov 2, 2023

kaovilai Nov 2, 2023

Choose a reason for hiding this comment

sseago Nov 2, 2023

Choose a reason for hiding this comment

sseago Nov 2, 2023

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

kaovilai commented Nov 2, 2023

sseago Nov 2, 2023

Choose a reason for hiding this comment

sseago Nov 2, 2023

Choose a reason for hiding this comment

sseago Nov 2, 2023

Choose a reason for hiding this comment

sseago commented Nov 2, 2023

kaovilai commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading