Merge https://github.com/velero-io/velero:main (7d300ea) into oadp-dev

[oadp-rebasebot-app]

This is an automated rebase PR generated by RebaseBot.

Summary

• Source: https://github.com/velero-io/velero:main
• Destination: https://github.com/openshift/velero:oadp-dev
• 17 new upstream commits

[openshift-ci]

Hi @oadp-rebasebot-app[bot]. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: oadp-rebasebot-app[bot]
Once this PR has been reviewed and has the lgtm label, please assign jmontleon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• DOWNSTREAM_OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Walkthrough

This PR adds backup type support, centralizes data mover handling, implements Linux block-device operations, preserves pod-volume results during cancellation, scopes repository automation, updates Kopia-focused documentation, and adds design proposals and changelog entries.

Changes

Backup type support

Layer / File(s)	Summary
API, CRD, and controller defaults pkg/apis/..., config/crd/..., pkg/controller/...	Adds backupType with Full and Incremental values, CRD schemas, and Incremental defaulting.
CLI and builder wiring pkg/cmd/cli/backup/..., pkg/builder/...	Adds --backup-type, validation, builder support, and tests.

Data mover selection

Layer / File(s)	Summary
Shared utility and policy validation pkg/util/datamover/..., internal/resourcepolicies/...	Centralizes mover identifiers and validates snapshot action parameters.
Caller migration pkg/datamover/..., pkg/controller/..., pkg/exposer/...	Updates callers to use the shared utility package.

Block uploader and restore handling

Layer / File(s)	Summary
Linux block-device operations pkg/uploader/block/...	Resolves symlinks, validates block devices, opens handles, and closes them during backup and restore.
Pod-volume and restore matching pkg/podvolume/..., pkg/restore/...	Returns tracked PVBs after cancellation and matches only completed backups with snapshot IDs.

Repository automation and documentation

Layer / File(s)	Summary
Repository-scoped automation .github/..., go.mod, restic	Adds ownership, repository guards, module replacements, and a subproject revision update.
Designs and documentation design/..., site/content/docs/..., changelogs/unreleased/...	Adds design proposals and updates backup, repository, Kopia, and maintenance documentation.

Estimated code review effort: 5 (Critical) | ~90+ minutes

Suggested reviewers: lyndon-li

🚥 Pre-merge checks | ✅ 12 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.69% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	⚠️ Warning	No Ginkgo tests were added, but new unit tests still ignore setup/cleanup errors (Close() and ToUnstructured), so quality checks fail.	Use require.NoError for all temp file closes and handle the ToUnstructured error in TestHasPodVolumeBackup.
Description check	⚠️ Warning	The description has a summary, but it omits the issue-fix field and the required checklist items from the template.	Add the missing issue reference or state none, and include the DCO/changelog/docs checklist items required by the template.

✅ Passed checks (12 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	No Ginkgo titles were added; the changed tests use static, descriptive subtest names and no dynamic name construction was found.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the changed test files are plain testing unit tests with no ginkgo/gomega imports or MicroShift-sensitive OpenShift APIs.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the PR changes are docs, workflows, CRDs, and unit tests only, with no test/e2e SNO-sensitive assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	PASS: The only visible change is the restic submodule pointer; no manifests/controllers/replicas/affinity/node selectors were added.
Ote Binary Stdout Contract	✅ Passed	No changed main/init/TestMain/suite setup code writes to stdout; the only stdout hits are in regular command logic or test cases.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the diff only updates the restic submodule, so there are no IPv4 or disconnected-network concerns.
No-Weak-Crypto	✅ Passed	Only change is a restic submodule pointer update; no weak-crypto APIs, custom crypto, or secret compares were added.
Container-Privileges	✅ Passed	No changed file adds privileged/hostPID/hostNetwork/hostIPC/SYS_ADMIN/allowPrivilegeEscalation settings; workflow edits are only repo guards.
No-Sensitive-Data-In-Logs	✅ Passed	Inspected changed Go files; logs only emit resource names, phases, and generic errors, with no secrets/PII/internal hostnames/customer data.
Title check	✅ Passed	The title accurately describes an automated merge/rebase of upstream Velero main into oadp-dev.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[Joeavaikath]

/ok-to-test

[coderabbitai]

Actionable comments posted: 12

🧹 Nitpick comments (3)

pkg/uploader/block/dev_linux_test.go (1)

182-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add successful read and write opening cases.

The tests never verify that read mode passes O_RDONLY|O_DIRECT and write mode passes O_RDWR|O_DIRECT. Capture the injected openFileFunc arguments and return a temporary file for both positive cases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/uploader/block/dev_linux_test.go` around lines 182 - 327, Extend
TestOpenBlockDevice with successful read and write cases that use a fake
block-device lstat result, capture the flags passed to injectOpenFile, and
return a temporary file handle. Assert read mode uses O_RDONLY|O_DIRECT and
write mode uses O_RDWR|O_DIRECT, while preserving the existing error-case
coverage.

design/cli-dynamic-resource-completion_design.md (2)

117-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Specify comma-aware completion for repeated flag values.

--volume-snapshot-locations only completes the first value; subsequent values receive the full comma-delimited string. Define splitting and prefix replacement, or explicitly narrow the feature scope and add coverage.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@design/cli-dynamic-resource-completion_design.md` around lines 117 - 119,
Update the design for --volume-snapshot-locations to define comma-aware
completion: split the comma-delimited input, complete the current value, and
replace only that value while preserving the others. If this behavior is
intentionally out of scope, explicitly narrow the completion requirements and
add coverage for the documented limitation.

---

113-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Define maximum-argument handling before implementation.

Single-argument commands can receive suggestions after an argument is already present, producing invalid completions. Add a shared maximum-argument check (or per-command wrapper) and test backup logs and backup download.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@design/cli-dynamic-resource-completion_design.md` around lines 113 - 115,
Update the dynamic resource completion design to define maximum-argument
handling before implementation: ensure the completion function or per-command
wrappers stop returning suggestions once the command’s allowed positional
argument count is reached. Apply this to the single-argument `backup logs` and
`backup download` commands, and specify tests covering completion after one
argument is provided.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@changelogs/unreleased/9994-Lyndon-Li`:
- Line 1: Update the changelog entry to mention that block-device operations
support both block uploader backup and restore paths, not backup alone.

In `@design/default-resource-modifier_design.md`:
- Around line 277-289: The restore configuration currently persists only the
skip flag, so the describer cannot reliably represent the resolved default
modifier later. Persist resolved default state in the restore data, including
the applied ConfigMap name/source and whether it was skipped or ignored due to
retrieval or validation failure, then update the restore describer in
restore_describer.go to use that persisted state: show the ConfigMap for applied
defaults, the skip reason for SkipDefaultResourceModifier, and no extra output
for per-restore overrides or failed defaults.

In `@internal/resourcepolicies/resource_policies.go`:
- Around line 60-64: The validDataMovers allow-list must recognize the
documented empty-string default alias. Add the empty string to validDataMovers,
and add regression coverage showing Action.validate accepts dataMover "" and
GetDataMover returns it unchanged.

In `@pkg/cmd/cli/backup/create.go`:
- Around line 240-246: Update CreateOptions.validateBackupType to assign the
trimmed backupType back to o.BackupType after successful validation, so
BuildBackup submits the canonical enum value. Add an assertion covering
generated Backup.Spec.BackupType for whitespace-padded input and verifying it
contains the expected trimmed value.

In `@pkg/restore/restore_test.go`:
- Line 4264: Update the fixture conversion in the test helper around
runtime.DefaultUnstructuredConverter.ToUnstructured to capture its error and
assert require.NoError(t, err) before using the converted object, preventing a
failed conversion from producing a broken PV fixture.

In `@pkg/uploader/block/dev_linux_test.go`:
- Around line 65-68: Update every temporary-file setup and cleanup block in the
test to assert close results with require.NoError(t, f.Close()). Apply this
consistently to the file handles created in the affected test cases, including
the blocks around the visible os.CreateTemp call, without leaving any bare
f.Close() calls.

In `@pkg/uploader/block/dev_linux.go`:
- Around line 35-56: In the device-opening flow after openFileFunc returns, call
device.Stat() and verify the opened descriptor’s mode identifies a block device,
rather than relying only on the earlier lstatFunc check. On Stat failure or
non-block results, close the descriptor and return an appropriate error;
preserve the existing successfully opened device path otherwise.

In `@pkg/uploader/block/snapshot.go`:
- Line 70: Update the deferred device cleanup in Restore and the other affected
function around sourceInfo.dev.Close so Close errors are not discarded. Use
deferred closures that access the named return error and join or wrap any close
failure into it, preserving existing errors while returning device-finalization
failures.

In `@pkg/util/datamover/datamover.go`:
- Around line 32-42: The built-in classification in IsBuiltInDataMover does not
recognize the resolved default or specialized built-in movers. Separate legacy
empty/“velero” alias handling from built-in classification, and update
IsBuiltInDataMover to accept DataMoverTypeVeleroFs and DataMoverTypeVeleroBlock
while preserving existing aliases. Add regression tests covering
GetDefaultBuiltInDataMover’s result and both specialized movers.

In `@site/content/docs/main/api-types/backup.md`:
- Around line 181-187: Update the backupType documentation near the BackupType
field to match the implemented and tested default of Incremental, changing only
the omitted-value default description unless the project explicitly chooses Full
as the contract. Keep the documented backup behavior and supported backup modes
unchanged.

In `@site/content/docs/main/backup-repository-configuration.md`:
- Line 21: Revise the backup repository configuration documentation to clarify
that ConfigMap entries, rather than the ConfigMap itself, are keyed by
repository type such as <kopia> or <other-repository-type>. State that each
entry applies to all BackupRepository CRs of the matching type, while
repositoryConfig changes remain specific to individual CRs.

In `@site/content/docs/main/performance-guidance.md`:
- Around line 124-125: Revise the performance conclusions around the Kopia
uploader to match the measured timings: remove the claim that it becomes faster,
avoid unsupported “less time-consuming” and timeout statements, and describe
only the observed runtime increases and memory behavior unless the missing
comparison measurements are added.

---

Nitpick comments:
In `@design/cli-dynamic-resource-completion_design.md`:
- Around line 117-119: Update the design for --volume-snapshot-locations to
define comma-aware completion: split the comma-delimited input, complete the
current value, and replace only that value while preserving the others. If this
behavior is intentionally out of scope, explicitly narrow the completion
requirements and add coverage for the documented limitation.
- Around line 113-115: Update the dynamic resource completion design to define
maximum-argument handling before implementation: ensure the completion function
or per-command wrappers stop returning suggestions once the command’s allowed
positional argument count is reached. Apply this to the single-argument `backup
logs` and `backup download` commands, and specify tests covering completion
after one argument is provided.

In `@pkg/uploader/block/dev_linux_test.go`:
- Around line 182-327: Extend TestOpenBlockDevice with successful read and write
cases that use a fake block-device lstat result, capture the flags passed to
injectOpenFile, and return a temporary file handle. Assert read mode uses
O_RDONLY|O_DIRECT and write mode uses O_RDWR|O_DIRECT, while preserving the
existing error-case coverage.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6bfedd07-566c-4d11-b963-65d2c55748e3

📥 Commits

Reviewing files that changed from the base of the PR and between e2178e7 and c009b2f.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (56)

• .github/CODEOWNERS
• .github/workflows/auto_assign_prs.yml
• .github/workflows/auto_label_prs.yml
• .github/workflows/auto_request_review.yml
• .github/workflows/pr-changelog-check.yml
• .github/workflows/pr-codespell.yml
• .github/workflows/pr-filepath-check.yml
• .github/workflows/prow-action.yml
• .github/workflows/rebase.yml
• .github/workflows/stale-issues.yml
• changelogs/unreleased/10004-chlins
• changelogs/unreleased/9921-shubham-pampattiwar
• changelogs/unreleased/9954-blackpiglet
• changelogs/unreleased/9994-Lyndon-Li
• changelogs/unreleased/9995-shubham-pampattiwar
• config/crd/v1/bases/velero.io_backups.yaml
• config/crd/v1/bases/velero.io_schedules.yaml
• config/crd/v1/crds/crds.go
• design/cli-dynamic-resource-completion_design.md
• design/default-resource-modifier_design.md
• go.mod
• internal/resourcepolicies/resource_policies.go
• internal/resourcepolicies/resource_policies_test.go
• internal/resourcepolicies/volume_resources_validator.go
• internal/resourcepolicies/volume_resources_validator_test.go
• pkg/apis/velero/v1/backup_types.go
• pkg/builder/backup_builder.go
• pkg/cmd/cli/backup/create.go
• pkg/cmd/cli/backup/create_test.go
• pkg/controller/backup_controller.go
• pkg/controller/backup_controller_test.go
• pkg/controller/data_download_controller.go
• pkg/controller/data_upload_controller.go
• pkg/datamover/dataupload_delete_action.go
• pkg/datamover/util.go
• pkg/datamover/util_test.go
• pkg/exposer/csi_snapshot.go
• pkg/exposer/csi_snapshot_test.go
• pkg/exposer/generic_restore.go
• pkg/exposer/generic_restore_test.go
• pkg/podvolume/backupper.go
• pkg/podvolume/backupper_test.go
• pkg/restore/restore.go
• pkg/restore/restore_test.go
• pkg/uploader/block/dev_linux.go
• pkg/uploader/block/dev_linux_test.go
• pkg/uploader/block/snapshot.go
• pkg/util/datamover/datamover.go
• pkg/util/datamover/datamover_test.go
• restic
• site/content/docs/main/api-types/backup.md
• site/content/docs/main/backup-repository-configuration.md
• site/content/docs/main/file-system-backup.md
• site/content/docs/main/performance-guidance.md
• site/content/docs/main/repository-maintenance.md
• site/content/docs/main/self-signed-certificates.md

💤 Files with no reviewable changes (1)

• pkg/datamover/util_test.go

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Mention restore support in the changelog.

The implementation adds block-device handling to both backup and restore paths.

-Add block device operations for block uploader backup
+Add block device operations for block uploader backup and restore

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	Add block device operations for block uploader backup
	Add block device operations for block uploader backup and restore

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@changelogs/unreleased/9994-Lyndon-Li` at line 1, Update the changelog entry
to mention that block-device operations support both block uploader backup and
restore paths, not backup alone.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Persist resolved default state before promising describe output.

Only the skip flag is stored; the ConfigMap name, source, and whether loading succeeded are not persisted. The describer therefore cannot reliably report applied, skipped, or failed default modifiers after configuration changes. Add status fields/events for the resolved state, or remove this output requirement.

🧰 Tools

🪛 LanguageTool

[style] ~289-~289: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e existing Resource Modifier field. - When the default was ignored due to a valida...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🪛 markdownlint-cli2 (0.23.0)

[warning] 281-281: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

[warning] 285-285: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@design/default-resource-modifier_design.md` around lines 277 - 289, The
restore configuration currently persists only the skip flag, so the describer
cannot reliably represent the resolved default modifier later. Persist resolved
default state in the restore data, including the applied ConfigMap name/source
and whether it was skipped or ignored due to retrieval or validation failure,
then update the restore describer in restore_describer.go to use that persisted
state: show the ConfigMap for applied defaults, the skip reason for
SkipDefaultResourceModifier, and no extra output for per-restore overrides or
failed defaults.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Allow the documented empty-string alias.

GetDataMover documents "" as a default alias (Lines 78-80), but this allow-list rejects it. Since Action.validate uses the same map, a policy with dataMover: "" cannot be accepted or returned unchanged. Add "": {} and regression cases for both validation and GetDataMover.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/resourcepolicies/resource_policies.go` around lines 60 - 64, The
validDataMovers allow-list must recognize the documented empty-string default
alias. Add the empty string to validDataMovers, and add regression coverage
showing Action.validate accepts dataMover "" and GetDataMover returns it
unchanged.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Normalize the validated backup type before building the request.

validateBackupType() trims into a local variable but leaves o.BackupType unchanged. Therefore, " Incremental " passes validation and BuildBackup() submits the untrimmed value, which violates the CRD enum and causes the create request to fail.

Assign the trimmed value back after successful validation, and add an assertion that the generated Backup.Spec.BackupType contains the canonical enum value.

Proposed fix

  switch backupType {
  case "", "Incremental", "Full":
  default:
    return fmt.Errorf("invalid backup type %s - valid values are 'Incremental', and 'Full'", backupType)
  }

+ o.BackupType = backupType
  return nil

Also applies to: 414-415

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cmd/cli/backup/create.go` around lines 240 - 246, Update
CreateOptions.validateBackupType to assign the trimmed backupType back to
o.BackupType after successful validation, so BuildBackup submits the canonical
enum value. Add an assertion covering generated Backup.Spec.BackupType for
whitespace-padded input and verifying it contains the expected trimmed value.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant section of the test file with line numbers.
sed -n '4235,4295p' pkg/restore/restore_test.go | cat -n

# Find the ToUnstructured signature in the local module cache if available via go env.
python3 - <<'PY'
import os, subprocess, sys, json
try:
    modcache = subprocess.check_output(["go", "env", "GOMODCACHE"], text=True).strip()
    print("GOMODCACHE=", modcache)
except Exception as e:
    print("go env failed:", e)
PY

# Search repository for ToUnstructured usages and any handling pattern.
rg -n "DefaultUnstructuredConverter\.ToUnstructured|ToUnstructured\(" pkg -g '!**/vendor/**'

# Locate the imported runtime package and surrounding test helper definitions.
rg -n "runtime\.DefaultUnstructuredConverter|func .*pv|PersistentVolume|unstructured\.Unstructured" pkg/restore/restore_test.go

Repository: openshift/velero

Length of output: 28680

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the converter implementation/signature in the local module cache.
modcache="$(go env GOMODCACHE)"
rg -n "func .*ToUnstructured|ToUnstructured\(in interface" "$modcache"/k8s.io/apimachinery@* -g 'converter.go' -g '*.go' | head -n 20

# Show the implementation if found.
impl=$(rg -l "ToUnstructured" "$modcache"/k8s.io/apimachinery@* -g 'converter.go' | head -n 1 || true)
if [ -n "${impl:-}" ]; then
  echo "IMPLEMENTATION_FILE=$impl"
  sed -n '1,220p' "$impl" | cat -n
fi

Repository: openshift/velero

Length of output: 11337

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

modcache="$(go env GOMODCACHE)"
rg -n "func .*ToUnstructured|ToUnstructured\(in interface" "$modcache"/k8s.io/apimachinery@* -g 'converter.go' -g '*.go' | head -n 20

impl=$(rg -l "ToUnstructured" "$modcache"/k8s.io/apimachinery@* -g 'converter.go' | head -n 1 || true)
if [ -n "${impl:-}" ]; then
  echo "IMPLEMENTATION_FILE=$impl"
  sed -n '1,220p' "$impl" | cat -n
fi

Repository: openshift/velero

Length of output: 11337

---

Handle the unstructured conversion error — runtime.DefaultUnstructuredConverter.ToUnstructured returns an error here; if conversion fails, this fixture helper will build a broken PV and mask the real problem. Capture it and require.NoError(t, err).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/restore/restore_test.go` at line 4264, Update the fixture conversion in
the test helper around runtime.DefaultUnstructuredConverter.ToUnstructured to
capture its error and assert require.NoError(t, err) before using the converted
object, preventing a failed conversion from producing a broken PV fixture.

Source: Path instructions

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Propagate device-close failures.

Both deferred calls discard Close() errors. In particular, Restore can return success despite a device finalization failure. Use a deferred closure with a named return error and join/wrap any close failure.

As per path instructions, “Never ignore error returns.”

Also applies to: 223-223

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/uploader/block/snapshot.go` at line 70, Update the deferred device
cleanup in Restore and the other affected function around sourceInfo.dev.Close
so Close errors are not discarded. Use deferred closures that access the named
return error and join or wrap any close failure into it, preserving existing
errors while returning device-finalization failures.

Source: Path instructions

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== datamover utility ==\n'
sed -n '1,220p' pkg/util/datamover/datamover.go

printf '\n== search for built-in mover predicate usages ==\n'
rg -n "IsBuiltInDataMover|GetDefaultBuiltInDataMover|DataMoverTypeVeleroFs|DataMoverTypeVelero|velero-block" pkg -g'*.go'

printf '\n== inspect likely callers ==\n'
rg -n "IsBuiltInDataMover\(" -g'*.go' .

Repository: openshift/velero

Length of output: 6234

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== pkg/datamover/util.go ==\n'
sed -n '1,220p' pkg/datamover/util.go

printf '\n== pkg/controller/data_upload_controller.go (relevant region) ==\n'
sed -n '130,220p' pkg/controller/data_upload_controller.go
sed -n '920,980p' pkg/controller/data_upload_controller.go

printf '\n== pkg/controller/data_download_controller.go (relevant region) ==\n'
sed -n '130,190p' pkg/controller/data_download_controller.go

printf '\n== pkg/datamover/dataupload_delete_action.go ==\n'
sed -n '1,160p' pkg/datamover/dataupload_delete_action.go

printf '\n== pkg/util/datamover/datamover_test.go ==\n'
sed -n '1,220p' pkg/util/datamover/datamover_test.go

Repository: openshift/velero

Length of output: 16203

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== search for DataMover defaulting/assignment ==\n'
rg -n "DataMover.*Default|default.*DataMover|GetDefaultBuiltInDataMover|DataMoverTypeVeleroFs|DataMoverTypeVeleroBlock|DataMoverTypeVelero" pkg config . -g'*.go' -g'*.yaml' -g'*.yml' -g'*.json'

printf '\n== locate API type definitions for DataMover fields ==\n'
rg -n "DataMover string|DataMover .*string" pkg/apis -g'*.go'

printf '\n== inspect v2alpha1 DataUpload/DataDownload types ==\n'
sed -n '1,260p' pkg/apis/velero/v2alpha1/types.go

printf '\n== inspect any defaulting webhook files ==\n'
rg -n "Defaulting|SetDefaults|defaulter|Webhook|DataMover" pkg/apis pkg -g'*.go'

Repository: openshift/velero

Length of output: 6266

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== internal/resourcepolicies/resource_policies.go ==\n'
sed -n '1,180p' internal/resourcepolicies/resource_policies.go

printf '\n== internal/resourcepolicies/volume_resources_validator.go ==\n'
sed -n '1,180p' internal/resourcepolicies/volume_resources_validator.go

printf '\n== search for IsBuiltInDataMover combined with default resolution ==\n'
rg -n "GetDefaultBuiltInDataMover\(\).*IsBuiltInDataMover|IsBuiltInDataMover\([^)]*GetDefaultBuiltInDataMover|GetDefaultBuiltInDataMover\(\)" . -g'*.go'

Repository: openshift/velero

Length of output: 12104

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== search for GetDataMover call sites ==\n'
rg -n "\.GetDataMover\(" . -g'*.go'

printf '\n== search for DataUpload/DataDownload creation with dataMover ==\n'
rg -n "DataMover:|Spec\.DataMover|dataMover =" pkg internal -g'*.go'

printf '\n== inspect expose/restoration paths around mover selection ==\n'
sed -n '1,260p' pkg/exposer/generic_restore.go
sed -n '1,260p' pkg/exposer/csi_snapshot.go

Repository: openshift/velero

Length of output: 24462

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== pkg/backup/actions/csi/pvc_action.go (data mover assignment) ==\n'
sed -n '520,620p' pkg/backup/actions/csi/pvc_action.go

printf '\n== pkg/restore/actions/csi/pvc_action.go (data mover assignment) ==\n'
sed -n '400,470p' pkg/restore/actions/csi/pvc_action.go

printf '\n== pkg/restore/actions/dataupload_retrieve_action.go ==\n'
sed -n '1,140p' pkg/restore/actions/dataupload_retrieve_action.go

printf '\n== internal/volume/volumes_information.go (data mover extraction) ==\n'
sed -n '600,900p' internal/volume/volumes_information.go

Repository: openshift/velero

Length of output: 20729

---

Expand built-in mover detection
GetDefaultBuiltInDataMover() resolves to velero-fs, but IsBuiltInDataMover() still rejects velero-fs and velero-block. That can make the data upload/download controllers skip valid CRs once the default is stored in Spec.DataMover. Split the legacy ""/velero alias check from built-in classification, and add regression tests for the resolved default and specialized movers.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/util/datamover/datamover.go` around lines 32 - 42, The built-in
classification in IsBuiltInDataMover does not recognize the resolved default or
specialized built-in movers. Separate legacy empty/“velero” alias handling from
built-in classification, and update IsBuiltInDataMover to accept
DataMoverTypeVeleroFs and DataMoverTypeVeleroBlock while preserving existing
aliases. Add regression tests covering GetDefaultBuiltInDataMover’s result and
both specialized movers.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Document the actual default backup type.

The documentation says an omitted backupType defaults to Full, but pkg/controller/backup_controller.go Lines 413-416 and its tests default it to Incremental. Update the documentation to say Incremental, or change the implementation and tests consistently if Full is the intended contract.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@site/content/docs/main/api-types/backup.md` around lines 181 - 187, Update
the backupType documentation near the BackupType field to match the implemented
and tested default of Incremental, changing only the omitted-value default
description unless the project explicitly chooses Full as the contract. Keep the
documented backup behavior and supported backup modes unchanged.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clarify the scope of the ConfigMap versus its entries.

The example below places <kopia> and <other-repository-type> keys in one ConfigMap, so saying the ConfigMap itself is repository-type-specific is misleading. Say that configuration entries are keyed by repository type and apply to all matching CRs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@site/content/docs/main/backup-repository-configuration.md` at line 21, Revise
the backup repository configuration documentation to clarify that ConfigMap
entries, rather than the ConfigMap itself, are keyed by repository type such as
<kopia> or <other-repository-type>. State that each entry applies to all
BackupRepository CRs of the matching type, while repositoryConfig changes remain
specific to individual CRs.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align the conclusions with the measured data.

The reported times increase from 2m34s to 3m45s to 5m06s, so “increasingly faster” is unsupported. The summary also claims Kopia is “less time-consuming” without a comparison baseline and mentions timeouts that are not shown in these results. State only conclusions supported by the tables, or add the missing measurements.

Also applies to: 145-148

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@site/content/docs/main/performance-guidance.md` around lines 124 - 125,
Revise the performance conclusions around the Kopia uploader to match the
measured timings: remove the claim that it becomes faster, avoid unsupported
“less time-consuming” and timeout statements, and describe only the observed
runtime increases and memory behavior unless the missing comparison measurements
are added.

[openshift-ci]

@oadp-rebasebot-app[bot]: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.