-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Remove restore-wait initcontainer if not needed on restore #8880
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
base: main
Are you sure you want to change the base?
Conversation
41d24ee
to
eab86f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the pod volume restore action to conditionally add the restore-wait init container only when volumes actually need a file system restore. Key changes include:
- Adding a check in pkg/restore/actions/pod_volume_restore_action.go that iterates over volume snapshots and PodVolumeBackups to decide if an init container is necessary.
- Updating and expanding tests in pkg/restore/actions/pod_volume_restore_action_test.go to cover scenarios with and without file system restores.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
pkg/restore/actions/pod_volume_restore_action.go | Added logic to determine whether file system restoration is required before adding the init container. |
pkg/restore/actions/pod_volume_restore_action_test.go | Updated tests and added new cases to verify the behavior of the file system restore check. |
Files not reviewed (1)
- changelogs/unreleased/8880-kaovilai: Language not supported
Comments suppressed due to low confidence (1)
pkg/restore/actions/pod_volume_restore_action_test.go:545
- [nitpick] The test case name indicates that a matching pod volume backup should result in an init container, but the expected init container count is set to 0. Please update the test name or expected outcome to clearly reflect the intended behavior.
name: "matching pod volume backup results in init container",
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8880 +/- ##
==========================================
+ Coverage 59.70% 59.74% +0.04%
==========================================
Files 370 370
Lines 40326 40353 +27
==========================================
+ Hits 24075 24109 +34
+ Misses 14770 14765 -5
+ Partials 1481 1479 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make some changes.
To me, the current implementation cannot fix the issue.
Please see my comments.
test passing, rebasing. |
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 <tkaovila@redhat.com>
- 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 <tkaovila@redhat.com>
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
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:
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.Testing
Signed-off-by: Tiger Kaovilai tkaovila@redhat.com
Thank you for contributing to Velero!
Does your change fix a particular issue?
Fixes #8870
Please indicate you've done the following:
make new-changelog
) or comment/kind changelog-not-required
on this PR.site/content/docs/main
.