Merge https://github.com/vmware-tanzu/velero:main (66ac235) into oadp-dev

[oadp-rebasebot-app]

Summary by CodeRabbit

• Bug Fixes

  • Fixed Data Block Restore getting stuck when CSI snapshots are unavailable in cloud provider.
  • Improved error detection and cleanup for snapshot content failures.

• Documentation

  • Added itemOperationTimeout field to Schedule API type documentation.
  • Removed DocSearch integration from documentation site.

• Tests

  • Added end-to-end test coverage for restore with multiple exec hooks.

[coderabbitai]

Walkthrough

The pull request addresses a CSI snapshot cleanup issue by adding error handling and fast-fail logic to VolumeSnapshotContent processing, includes corresponding unit and end-to-end tests, removes DocSearch integration from documentation, adds itemOperationTimeout field documentation, and updates the changelog.

Changes

Cohort / File(s)	Summary
CSI VolumeSnapshotContent Error Handling internal/delete/actions/csi/volumesnapshotcontent_action.go, internal/delete/actions/csi/volumesnapshotcontent_action_test.go	Added cleanup logic to delete created VSC on timeout in Execute method; added fast-fail path in checkVSCReadiness to detect Status.Error.Message. Includes test cases for error scenarios and helper functions (boolPtr, stringPtr).
DocSearch Integration Removal site/algolia-crawler.json, site/layouts/docs/docs.html, site/layouts/partials/head-docs.html	Removed Algolia DocSearch crawler configuration, client-side search UI initialization, and external DocSearch resources (preconnect and CSS script) from documentation site.
Documentation and Changelog changelogs/unreleased/9581-shubham-pampattiwar, site/content/docs/main/api-types/schedule.md	Added changelog entry for CSI snapshot fix and documented new itemOperationTimeout field in Schedule API spec.template with 4h default.
End-to-End Test for Restore Exec Hooks test/e2e/basic/restore_exec_hooks.go, test/e2e/e2e_suite_test.go	Introduced new RestoreExecHooks test type with Init, CreateResources, and Verify methods to validate restore with multiple exec hooks. Registered test in e2e suite.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 4

❌ Failed checks (4 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title describes a merge operation rather than the substantive changes in the changeset. It does not clearly summarize the main technical changes (VSC cleanup logic, error handling, doc updates, or test additions).	Replace the generic merge title with a specific summary of the primary changes, such as 'Fix DBR stuck when CSI snapshot no longer exists in cloud provider' which reflects the core bugfix and is already documented in the changelog.
Description check	⚠️ Warning	The pull request has no description provided, making it impossible to verify compliance with the required template sections including summary, issue reference, DCO acceptance, changelog creation, and documentation updates.	Add a detailed pull request description following the template: include a summary of changes, reference the issue being fixed, confirm DCO acceptance and changelog creation, and verify documentation has been updated.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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	CSI unit test assertions lack meaningful failure messages, violating requirement #4; E2E test properly includes context-specific messages in all assertions.	Add meaningful failure messages to all assertions in volumesnapshotcontent_action_test.go, such as 'require.NoError(t, err, "VSC should be created successfully")' to match the pattern demonstrated in the E2E test.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Stable And Deterministic Test Names	✅ Passed	The Ginkgo test title is stable and deterministic, containing only static descriptive language without dynamic values like pod names, timestamps, UUIDs, or node names. Dynamic values are appropriately placed in the test body, following best practices.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

[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.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

internal/delete/actions/csi/volumesnapshotcontent_action_test.go (2)

97-109: Test coverage could be more explicit about cleanup verification.

The test case validates that an error is returned when CSI reports an error, but it doesn't explicitly verify that the dangling VSC was cleaned up (deleted). Consider adding an assertion to check that the VSC no longer exists in the fake client after Execute returns, to fully validate the cleanup behavior described in the test name.

💡 Suggested enhancement to verify cleanup

After p.Execute(...) returns, you could add:

// Verify VSC was cleaned up
vscList := &snapshotv1api.VolumeSnapshotContentList{}
require.NoError(t, crClient.List(context.Background(), vscList))
require.Empty(t, vscList.Items, "Expected VSC to be cleaned up")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go` around
lines 97 - 109, The test should explicitly verify that the dangling
VolumeSnapshotContent was deleted after Execute; after calling p.Execute(...)
use the test client (crClient) to List VolumeSnapshotContent objects
(snapshotv1api.VolumeSnapshotContentList) and assert no items remain by calling
require.NoError for the List result and require.Empty on the list.Items
(referencing the existing vsc variable and the Execute call) so the test
confirms cleanup behavior rather than only checking expectErr.

---

242-248: Consider reusing existing pointer utilities.

The codebase already has pkg/util/boolptr (used in the main implementation file). For consistency, consider reusing that package for boolPtr. For stringPtr, this local helper is acceptable since there doesn't appear to be a corresponding utility in the codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go` around
lines 242 - 248, The local test helper boolPtr should be replaced with the
existing utility to ensure consistency: remove the local boolPtr function and
use the package pkg/util/boolptr (used in the implementation) wherever boolPtr()
is referenced in this test (e.g., in volumesnapshotcontent_action_test.go); keep
the local stringPtr helper as-is since no shared string pointer utility exists.
Ensure the import for the util boolptr package is added and update references
from boolPtr(...) to the util's API.

test/e2e/basic/restore_exec_hooks.go (1)

51-54: Unusual format string for namespace numbering.

The format %00000d is unusual and likely not producing the intended result. The sequence 00000 means the minimum width is 0 (the rightmost digit), so there's no zero-padding. If the intent is 5-digit zero-padded numbers, use %05d instead.

💡 Suggested fix

 	for nsNum := 0; nsNum < r.NamespacesTotal; nsNum++ {
-		createNSName := fmt.Sprintf("%s-%00000d", r.CaseBaseName, nsNum)
+		createNSName := fmt.Sprintf("%s-%05d", r.CaseBaseName, nsNum)
 		*r.NSIncluded = append(*r.NSIncluded, createNSName)
 	}

Note: This also appears at lines 78 and 142 in CreateResources() and Verify().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/basic/restore_exec_hooks.go` around lines 51 - 54, The namespace
name formatting in the loop that builds createNSName uses the incorrect format
string "%00000d" which doesn't produce zero-padded numbers; change it to "%05d"
in the loop that constructs createNSName (inside CreateResources()/the
namespace-building loop) and update the other occurrences of the same format
(the instances referenced in Verify() and the second CreateResources()
occurrence) to use "%05d" so namespace numbers are zero-padded to 5 digits.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@site/content/docs/main/api-types/schedule.md`:
- Around line 66-69: Update the grammatical errors in the documentation values:
change "The default value is 4 hour." to "The default value is 4 hours." for the
itemOperationTimeout entry (referenced by the symbol itemOperationTimeout) and
also change "10 minute" to "10 minutes" in the related schedule/config entry
mentioned earlier (ensure the corresponding field name remains correct in the
same doc), keeping plural "minutes" and "hours" for the default value
descriptions.

---

Nitpick comments:
In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go`:
- Around line 97-109: The test should explicitly verify that the dangling
VolumeSnapshotContent was deleted after Execute; after calling p.Execute(...)
use the test client (crClient) to List VolumeSnapshotContent objects
(snapshotv1api.VolumeSnapshotContentList) and assert no items remain by calling
require.NoError for the List result and require.Empty on the list.Items
(referencing the existing vsc variable and the Execute call) so the test
confirms cleanup behavior rather than only checking expectErr.
- Around line 242-248: The local test helper boolPtr should be replaced with the
existing utility to ensure consistency: remove the local boolPtr function and
use the package pkg/util/boolptr (used in the implementation) wherever boolPtr()
is referenced in this test (e.g., in volumesnapshotcontent_action_test.go); keep
the local stringPtr helper as-is since no shared string pointer utility exists.
Ensure the import for the util boolptr package is added and update references
from boolPtr(...) to the util's API.

In `@test/e2e/basic/restore_exec_hooks.go`:
- Around line 51-54: The namespace name formatting in the loop that builds
createNSName uses the incorrect format string "%00000d" which doesn't produce
zero-padded numbers; change it to "%05d" in the loop that constructs
createNSName (inside CreateResources()/the namespace-building loop) and update
the other occurrences of the same format (the instances referenced in Verify()
and the second CreateResources() occurrence) to use "%05d" so namespace numbers
are zero-padded to 5 digits.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: 9dacb3df-e509-4af3-8614-195975c0171b

📥 Commits

Reviewing files that changed from the base of the PR and between ca5442f and 3e35877.

📒 Files selected for processing (9)

• changelogs/unreleased/9581-shubham-pampattiwar
• internal/delete/actions/csi/volumesnapshotcontent_action.go
• internal/delete/actions/csi/volumesnapshotcontent_action_test.go
• site/algolia-crawler.json
• site/content/docs/main/api-types/schedule.md
• site/layouts/docs/docs.html
• site/layouts/partials/head-docs.html
• test/e2e/basic/restore_exec_hooks.go
• test/e2e/e2e_suite_test.go

💤 Files with no reviewable changes (3)

• site/layouts/docs/docs.html
• site/layouts/partials/head-docs.html
• site/algolia-crawler.json

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix grammatical error in documentation.

Line 68 contains a grammatical error: "The default value is 4 hour." should be "The default value is 4 hours." (plural).

Note: The same grammatical issue exists in line 65 ("10 minute" should be "10 minutes"). Consider fixing both for consistency.

📝 Proposed fix

     # ItemOperationTimeout specifies the time used to wait for
     # asynchronous BackupItemAction operations
-    # The default value is 4 hour.
+    # The default value is 4 hours.
     itemOperationTimeout: 4h

If fixing line 65 as well:

     # CSISnapshotTimeout specifies the time used to wait for
     # CSI VolumeSnapshot status turns to ReadyToUse during creation, before
-    # returning error as timeout. The default value is 10 minute.
+    # returning error as timeout. The default value is 10 minutes.
     csiSnapshotTimeout: 10m

📝 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

	# ItemOperationTimeout specifies the time used to wait for
	# asynchronous BackupItemAction operations
	# The default value is 4 hour.
	itemOperationTimeout: 4h
	# ItemOperationTimeout specifies the time used to wait for
	# asynchronous BackupItemAction operations
	# The default value is 4 hours.
	itemOperationTimeout: 4h

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@site/content/docs/main/api-types/schedule.md` around lines 66 - 69, Update
the grammatical errors in the documentation values: change "The default value is
4 hour." to "The default value is 4 hours." for the itemOperationTimeout entry
(referenced by the symbol itemOperationTimeout) and also change "10 minute" to
"10 minutes" in the related schedule/config entry mentioned earlier (ensure the
corresponding field name remains correct in the same doc), keeping plural
"minutes" and "hours" for the default value descriptions.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oadp-rebasebot-app[bot], sseago

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• DOWNSTREAM_OWNERS [sseago]

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