Skip to content

Reject restores from backups not in a completed or partially failed phase#9792

Merged
sseago merged 10 commits into
velero-io:mainfrom
subhramit:reject-unusable-backups
Jun 8, 2026
Merged

Reject restores from backups not in a completed or partially failed phase#9792
sseago merged 10 commits into
velero-io:mainfrom
subhramit:reject-unusable-backups

Conversation

@subhramit

@subhramit subhramit commented May 14, 2026

Copy link
Copy Markdown
Contributor

Reject restores from backups not in a completed or partially failed phase.

Fixes #9790

Please indicate you've done the following:

subhramit added 4 commits May 15, 2026 02:19
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: Subhramit Basu <subhramit.bb@live.in>
@subhramit subhramit changed the title Reject unusable backups Issue #9790: Reject restores from backups not in a completed or partially failed phase May 14, 2026
@subhramit subhramit changed the title Issue #9790: Reject restores from backups not in a completed or partially failed phase Reject restores from backups not in a completed or partially failed phase May 14, 2026
sseago
sseago previously approved these changes May 19, 2026
@subhramit

Copy link
Copy Markdown
Contributor Author

@sseago @Lyndon-Li hey, could I get an approval for the awaiting CI workflows?

@netlify

netlify Bot commented May 23, 2026

Copy link
Copy Markdown

👷 Deploy request for velero pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 7f68077

kaovilai
kaovilai previously approved these changes Jun 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown

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 tightens restore validation to ensure a Restore cannot start from a Backup that is not yet usable (or is being deleted), addressing the race described in issue #9790.

Changes:

  • Add restore validation to only allow source backups in Completed or PartiallyFailed phase.
  • Update restore workflow documentation to describe the new validation requirement.
  • Extend restore controller tests with coverage for allowed/disallowed backup phases and adjust some existing tests to set backup phase explicitly.

Reviewed changes

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

File Description
site/content/docs/main/restore-reference.md Documents that restore validation requires the referenced backup to be in a usable phase (Completed/PartiallyFailed).
pkg/controller/restore_controller.go Enforces backup phase validation during restore admission/validation.
pkg/controller/restore_controller_test.go Adds/updates test cases for the new validation logic (but still needs further updates for consistency).
changelogs/unreleased/9792-subhramit Adds an unreleased changelog note for the new restore validation behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 305 to 309
name: "restorer throwing an error causes the restore to fail",
location: defaultStorageLocation,
restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(),
backup: defaultBackup().StorageLocation("default").Result(),
backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(),
restorerError: errors.New("blarg"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix available as Cherry pick from #9874

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all the tests are passing, have to verify the merit of copilot's suggestion. Will take a look soon.

@subhramit subhramit Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaovilai I had already adapted the existing tests in a3a33c3, but copilot was not totally wrong here - I had missed one case which I adapted now in 3247e00 (it was passing before for the wrong reason of the restore being rejected before checking for the phase).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests running.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All green :)

@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread pkg/controller/restore_controller_test.go Outdated
Signed-off-by: Subhramit Basu <subhramit.bb@live.in>
@subhramit subhramit dismissed stale reviews from kaovilai and sseago via 3247e00 June 2, 2026 06:33
@subhramit subhramit requested review from kaovilai and sseago June 2, 2026 06:40
@subhramit

Copy link
Copy Markdown
Contributor Author

@kaovilai @sseago gentle ping since PR is gtg

@kaovilai

kaovilai commented Jun 5, 2026

Copy link
Copy Markdown
Member

this repo require 2 maintainer approvals :) Need another not me to go in.

@sseago sseago merged commit dda779d into velero-io:main Jun 8, 2026
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Restore validation should reject restores from backups in Deleting phase

5 participants