Remove restore-wait initcontainer if not needed on restore #8880

kaovilai · 2025-04-25T00:19:17Z

Please add a summary of your change

This PR fixes issue #8870 where Velero was unnecessarily adding the restore-wait init container when restoring pods with volumes that were backed up using native datamover or CSI.

When restoring pods with volumes, Velero was always adding the restore-wait init container, even when the volumes were backed up using native datamover or CSI and didn't need file system restores. This was causing unnecessary overhead and potential issues.

Solution

Modified the pod volume restore action to check if any volumes actually need file system restores before adding the init container:

Added a check in pkg/restore/actions/pod_volume_restore_action.go to determine if any volumes need file system restores by checking if there are matching PodVolumeBackups for the pod's volumes.
Only add the restore-wait init container if there are volumes that need file system restores.
Updated tests to match the new logic and renamed the test function to be more descriptive.

Testing

Added test cases to verify that the init container is only added when needed
Updated existing test cases to work with the new logic
All tests are passing

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

Thank you for contributing to Velero!

Does your change fix a particular issue?

Fixes #8870

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-04-25T00:35:42Z

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.09%. Comparing base (805237a) to head (22372b3).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/restore/actions/pod_volume_restore_action.go	85.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8880      +/-   ##
==========================================
+ Coverage   60.07%   60.09%   +0.01%     
==========================================
  Files         378      378              
  Lines       42900    42911      +11     
==========================================
+ Hits        25771    25786      +15     
+ Misses      15583    15580       -3     
+ Partials     1546     1545       -1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pkg/restore/actions/pod_volume_restore_action.go

blackpiglet

We need to make some changes.
To me, the current implementation cannot fix the issue.
Please see my comments.

kaovilai · 2025-05-29T18:34:12Z

test passing, rebasing.

pkg/restore/actions/pod_volume_restore_action.go

Change looks good to me, but we still need to fix some linter issue and DCO issue.

blackpiglet · 2025-06-30T08:08:06Z

@kaovilai
Hi Tiger, the PR change looks good to me, but we still need to fix some linter issues found after rebasing. Please replace assert.NoError with require.NoError.
Please also fix the DCO check error.
It's better to squash the commits, too.

kaovilai · 2025-06-30T13:16:24Z

Will do

Lyndon-Li · 2025-07-09T06:25:24Z

pkg/restore/actions/pod_volume_restore_action.go

 	for i := range podVolumeBackupList.Items {
 		podVolumeBackups = append(podVolumeBackups, &podVolumeBackupList.Items[i])
 	}
 	volumeSnapshots := podvolume.GetVolumeBackupsForPod(podVolumeBackups, &pod, podFromBackup.Namespace)


I would propose another solution:

At the beginning of this action, remove the restore-wait init-container blindly without checking needsFileSystemRestore

The let it go with the existing logics, if fs-backup is involved, the existing logic will add the restore-wait init-container back

This way looks more simpler.

Thanks. Incorporated.

Copilot

Pull Request Overview

This PR ensures the restore-wait init container is only added when needed and removed if no file system restores are required.

Filters out existing restore-wait init containers before deciding to add
Adds the init container only when matching PodVolumeBackups exist
Updates and enhances tests to cover both adding and removing scenarios

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
pkg/restore/actions/pod_volume_restore_action.go	Remove existing wait containers, check for volume backups, prepend new init container only when needed
pkg/restore/actions/pod_volume_restore_action_test.go	Add new test suite for file-system restore cases and update existing tests
changelogs/unreleased/8880-kaovilai	Changelog entry for the fix

Comments suppressed due to low confidence (2)

pkg/restore/actions/pod_volume_restore_action_test.go:42

The velerotest import is not used in this file; consider removing it to clean up unused dependencies.

	velerotest "github.com/vmware-tanzu/velero/pkg/test"

pkg/restore/actions/pod_volume_restore_action_test.go:43

The kube import is unused in this test; remove it to avoid unused imports.

	"github.com/vmware-tanzu/velero/pkg/util/kube"

pkg/restore/actions/pod_volume_restore_action.go

reasonerjt · 2025-07-11T09:08:13Z

@kaovilai Would you squash this PR?

kaovilai · 2025-07-11T14:05:57Z

Sure. You can also squash during merge anyway?

This PR fixes issue vmware-tanzu#8870 where Velero was unnecessarily adding the restore-wait init container when restoring pods with volumes that were backed up using native datamover or CSI. When restoring pods with volumes, Velero was always adding the restore-wait init container, even when the volumes were backed up using native datamover or CSI and didn't need file system restores. This was causing unnecessary overhead and potential issues. PVR action to remove restore-wait init container on restore 1. Added a check in `pkg/restore/actions/pod_volume_restore_action.go` to determine if any volumes need file system restores by checking if there are matching PodVolumeBackups for the pod's volumes. 2. Only add the restore-wait init container if there are volumes that need file system restores. 3. Updated tests to match the new logic and renamed the test function to be more descriptive. - Added test cases to verify that the init container is only added when needed - Updated existing test cases to work with the new logic - All tests are passing Signed-off-by: Tiger Kaovilai <[email protected]> Refactor init container assertions in pod volume restore tests for clarity Signed-off-by: Tiger Kaovilai <[email protected]> Fix restore-wait init container removal to handle multiple containers Address PR feedback by implementing comprehensive removal of ALL restore-wait init containers (both current and legacy names) to prevent duplicates. Previously, the code only removed the first init container if it matched the restore-wait names. This could leave behind multiple restore-wait containers or fail to remove them if they weren't at position 0. Changes: - Remove ALL existing restore-wait init containers before deciding whether to add a new one - This covers both scenarios: when no file system restore is needed AND when preventing duplicates - Simplify the add logic since we've already cleaned up existing containers - Add better logging to show how many containers were removed Signed-off-by: Tiger Kaovilai <[email protected]> Address PR feedback: clarify comments and fix test logic - Clarify comments explaining when restore-wait init container is needed - Fix test expectations for matching PodVolumeBackups (should add init container) - Add backup labels to PodVolumeBackups in tests for proper filtering - Remove incorrect test assertion that checked init containers should not be restore-wai Signed-off-by: Tiger Kaovilai <[email protected]>

…anzu#8880) This PR fixes issue vmware-tanzu#8870 where Velero was unnecessarily adding the restore-wait init container when restoring pods with volumes that were backed up using native datamover or CSI. When restoring pods with volumes, Velero was always adding the restore-wait init container, even when the volumes were backed up using native datamover or CSI and didn't need file system restores. This was causing unnecessary overhead and potential issues. PVR action to remove restore-wait init container on restore Changes: - Remove ALL existing restore-wait init containers before deciding whether to add a new one - This covers both scenarios: when no file system restore is needed AND when preventing duplicates - Simplify the add logic since we've already cleaned up existing containers - Add better logging to show how many containers were removed Signed-off-by: Tiger Kaovilai <[email protected]>

pawcykca · 2025-07-30T09:56:50Z

When do you plan to release this fix?

kaovilai · 2025-07-30T23:14:05Z

See milestone assigned to #8870

github-actions bot assigned kaovilai Apr 25, 2025

github-actions bot requested review from blackpiglet and reasonerjt April 25, 2025 00:19

github-actions bot added has-unit-tests has-changelog labels Apr 25, 2025

kaovilai force-pushed the 8870 branch 2 times, most recently from 41d24ee to eab86f1 Compare April 25, 2025 00:27

kaovilai requested a review from Copilot April 25, 2025 00:32

This comment was marked as outdated.

Sign in to view

blackpiglet reviewed Apr 27, 2025

View reviewed changes

pkg/restore/actions/pod_volume_restore_action.go Outdated Show resolved Hide resolved

blackpiglet requested changes Apr 27, 2025

View reviewed changes

kaovilai force-pushed the 8870 branch from eab86f1 to 0a94249 Compare April 28, 2025 02:08

kaovilai changed the title ~~Modified the pod volume restore action to check if any volumes actually need file system restores before adding the init container:~~ Remove restore-wait initcontainer if not needed on restore Apr 28, 2025

kaovilai force-pushed the 8870 branch from 0a94249 to d4d245f Compare April 28, 2025 02:30

blackpiglet force-pushed the 8870 branch from 1edcff5 to 6c5a78a Compare June 3, 2025 07:48

blackpiglet reviewed Jun 3, 2025

View reviewed changes

pkg/restore/actions/pod_volume_restore_action.go Outdated Show resolved Hide resolved

kaovilai requested a review from blackpiglet June 24, 2025 14:58

blackpiglet force-pushed the 8870 branch from d2e9556 to 4792f5c Compare June 30, 2025 07:53

blackpiglet previously approved these changes Jun 30, 2025

View reviewed changes

Lyndon-Li reviewed Jul 9, 2025

View reviewed changes

kaovilai force-pushed the 8870 branch from 27a1017 to 9d32c56 Compare July 10, 2025 01:32

kaovilai requested review from Lyndon-Li and Copilot July 10, 2025 01:33

Copilot AI reviewed Jul 10, 2025

View reviewed changes

pkg/restore/actions/pod_volume_restore_action.go Show resolved Hide resolved

kaovilai force-pushed the 8870 branch from 9d32c56 to e58cb32 Compare July 11, 2025 22:11

blackpiglet force-pushed the 8870 branch from e58cb32 to 9ac7ab3 Compare July 14, 2025 05:25

kaovilai force-pushed the 8870 branch from 9ac7ab3 to 39b6925 Compare July 16, 2025 20:22

blackpiglet force-pushed the 8870 branch from 907264e to 22372b3 Compare July 17, 2025 06:22

blackpiglet approved these changes Jul 17, 2025

View reviewed changes

reasonerjt removed their request for review July 18, 2025 05:52

Lyndon-Li approved these changes Jul 21, 2025

View reviewed changes

blackpiglet merged commit 2b787f5 into vmware-tanzu:main Jul 21, 2025
46 checks passed

MaloLelandais mentioned this pull request Jul 28, 2025

Rebase velero clcondorcet/velero#1

Closed

3 tasks

Remove restore-wait initcontainer if not needed on restore #8880

Remove restore-wait initcontainer if not needed on restore #8880

Uh oh!

Conversation

kaovilai commented Apr 25, 2025

Please add a summary of your change

Solution

Testing

Does your change fix a particular issue?

Please indicate you've done the following:

Uh oh!

This comment was marked as outdated.

Uh oh!

codecov bot commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

blackpiglet left a comment

Choose a reason for hiding this comment

Uh oh!

kaovilai commented May 29, 2025

Uh oh!

Uh oh!

blackpiglet commented Jun 30, 2025

Uh oh!

kaovilai commented Jun 30, 2025

Uh oh!

Lyndon-Li Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

kaovilai Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

reasonerjt commented Jul 11, 2025

Uh oh!

kaovilai commented Jul 11, 2025

Uh oh!

Uh oh!

pawcykca commented Jul 30, 2025

Uh oh!

kaovilai commented Jul 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Apr 25, 2025 •

edited

Loading