feat(backups): restore vmi to copy in another namespace - #2329

internal/backupcontroller/velerostrategy_controller_test.go (1)

1102-1113: Use strings.Contains from the standard library.

The custom containsString helper reimplements functionality available in the strings package.

♻️ Suggested refactor

+import "strings"
+
 // containsString reports whether substr appears in s.
 func containsString(s, substr string) bool {
-	if len(substr) == 0 || len(s) < len(substr) {
-		return false
-	}
-	for i := 0; i <= len(s)-len(substr); i++ {
-		if s[i:i+len(substr)] == substr {
-			return true
-		}
-	}
-	return false
+	return strings.Contains(s, substr)
 }

Or simply replace calls to containsString(s, substr) with strings.Contains(s, substr) and remove this helper entirely.

🤖 Prompt for AI Agents

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

In `@internal/backupcontroller/velerostrategy_controller_test.go` around lines
1102 - 1113, The helper containsString reimplements standard behavior; replace
all calls to containsString(s, substr) with strings.Contains(s, substr), remove
the containsString function, and add the "strings" import where needed (or
remove it if already present); ensure any tests compile by updating imports and
cleaning up the removed helper references (function name: containsString).

Fix all unresolved CodeRabbit comments on this PR:

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

examples/backups/vmi/cleanup.sh (1)

33-64: ⚠️ Potential issue | 🟠 Major

Don’t suppress real deletion failures.

The 2>/dev/null || log_warning ... pattern hides API/auth/connectivity errors and makes cleanup look successful when it isn’t. Prefer --ignore-not-found so only true “not found” is tolerated.

Suggested fix

-kubectl delete restorejob restore-to-copy-test -n "$NAMESPACE" 2>/dev/null || log_warning "RestoreJob restore-to-copy-test not found"
-kubectl delete restorejob restore-in-place-test -n "$NAMESPACE" 2>/dev/null || log_warning "RestoreJob restore-in-place-test not found"
+kubectl delete restorejob restore-to-copy-test -n "$NAMESPACE" --ignore-not-found
+kubectl delete restorejob restore-in-place-test -n "$NAMESPACE" --ignore-not-found

-kubectl delete backupjob test-backup -n "$NAMESPACE" 2>/dev/null || log_warning "BackupJob test-backup not found"
-kubectl delete backup test-backup -n "$NAMESPACE" 2>/dev/null || log_warning "Backup test-backup not found"
+kubectl delete backupjob test-backup -n "$NAMESPACE" --ignore-not-found
+kubectl delete backup test-backup -n "$NAMESPACE" --ignore-not-found

-kubectl delete vminstance test -n "$NAMESPACE" 2>/dev/null || log_warning "VMInstance test not found"
+kubectl delete vminstance test -n "$NAMESPACE" --ignore-not-found

-kubectl delete vmdisk ubuntu-source -n "$NAMESPACE" 2>/dev/null || log_warning "VMDisk ubuntu-source not found"
+kubectl delete vmdisk ubuntu-source -n "$NAMESPACE" --ignore-not-found

-kubectl delete backupclass velero 2>/dev/null || log_warning "BackupClass velero not found"
+kubectl delete backupclass velero --ignore-not-found

-kubectl delete velero.strategy.backups.cozystack.io vminstance-strategy 2>/dev/null || log_warning "Strategy vminstance-strategy not found"
-kubectl delete velero.strategy.backups.cozystack.io vmdisk-strategy 2>/dev/null || log_warning "Strategy vmdisk-strategy not found"
+kubectl delete velero.strategy.backups.cozystack.io vminstance-strategy --ignore-not-found
+kubectl delete velero.strategy.backups.cozystack.io vmdisk-strategy --ignore-not-found

-kubectl delete namespace "$TARGET_NAMESPACE" 2>/dev/null || log_warning "Namespace $TARGET_NAMESPACE not found"
+kubectl delete namespace "$TARGET_NAMESPACE" --ignore-not-found

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/cleanup.sh` around lines 33 - 64, The cleanup script
currently suppresses all deletion errors by redirecting stderr and falling back
to log_warning (e.g., the kubectl delete lines for restorejob, backupjob,
backup, vminstance, vmdisk, backupclass, velero.strategy and namespace); change
each kubectl delete invocation to use the --ignore-not-found flag instead of
`2>/dev/null || log_warning ...` so kubectl only tolerates NotFound, and remove
the stderr redirection; keep explicit log_step calls and add a conditional
log_warning only when kubectl exits with a non-zero status that is not a
NotFound error to ensure API/auth/connectivity failures are visible (apply this
change for the commands targeting restore-to-copy-test/restore-in-place-test,
test-backup, ubuntu-source, vminstance-strategy/vmdisk-strategy, velero, and the
namespace deletion).

examples/backups/vmi/03-create-vmdisk.sh (1)

39-39: ⚠️ Potential issue | 🟡 Minor

Avoid claiming readiness before readiness is checked.

Line 39 still reports “ready” even though image download may still be ongoing. Please keep this status truthful (or add a real readiness wait first).

Suggested fix

-log_success "VMDisk is ready (image download may still be in progress)"
+log_success "VMDisk created (image download may still be in progress)"

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/03-create-vmdisk.sh` at line 39, The script currently
calls log_success "VMDisk is ready (image download may still be in progress)"
which falsely asserts readiness; either change that message to a truthful status
(e.g., "VMDisk created; image download may still be in progress") or implement a
real readiness check before calling log_success. Locate the log_success
invocation in 03-create-vmdisk.sh, and either update the text to avoid claiming
readiness or add a wait function (e.g., wait_for_image_download /
wait_for_vmdisk_ready) that polls the image/disk state and only calls
log_success once the download/ready condition is confirmed.

internal/backupcontroller/velerostrategy_controller_test.go (1)

1202-1213: Consider using strings.Contains from standard library.

The custom containsString function replicates strings.Contains from the standard library, which is already imported elsewhere in the codebase.

Proposed simplification

-// containsString reports whether substr appears in s.
-func containsString(s, substr string) bool {
-	if len(substr) == 0 || len(s) < len(substr) {
-		return false
-	}
-	for i := 0; i <= len(s)-len(substr); i++ {
-		if s[i:i+len(substr)] == substr {
-			return true
-		}
-	}
-	return false
-}
+import "strings"
+
+// Then use strings.Contains(s, substr) directly in tests

🤖 Prompt for AI Agents

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

In `@internal/backupcontroller/velerostrategy_controller_test.go` around lines
1202 - 1213, The containsString helper duplicates standard library behavior;
remove the custom containsString function and replace its usages with
strings.Contains from the standard library (add "strings" to imports where
needed), ensuring all callers that referenced containsString(s, substr) now call
strings.Contains(s, substr) instead.

internal/backupcontroller/velerostrategy_controller.go (1)

1016-1082: postRestoreRename only handles VMInstance, not VMDisk.

The function hardcodes vmNamePrefix for HelmRelease naming, so it won't work correctly if target.AppKind is VMDisk. If VMDisk renames are needed in the future, this would silently use the wrong HelmRelease name.

Consider using helmReleaseNameForApp for consistency:

Proposed improvement

 func (r *RestoreJobReconciler) postRestoreRename(ctx context.Context, restoreJob *backupsv1alpha1.RestoreJob, backup *backupsv1alpha1.Backup, target restoreTarget) error {
 	logger := getLogger(ctx)
 	sourceAppName := backup.Spec.ApplicationRef.Name
-	sourceHRName := vmNamePrefix + sourceAppName
-	targetHRName := vmNamePrefix + target.AppName
+	sourceHRName := helmReleaseNameForApp(backup.Spec.ApplicationRef.Kind, sourceAppName)
+	targetHRName := helmReleaseNameForApp(target.AppKind, target.AppName)
+
+	// Skip rename for unsupported kinds
+	if sourceHRName == "" || targetHRName == "" {
+		logger.Debug("skipping post-restore rename for unsupported kind", "kind", target.AppKind)
+		return nil
+	}
 
 	logger.Debug("post-restore rename", "from", sourceHRName, "to", targetHRName, "namespace", target.Namespace)

🤖 Prompt for AI Agents

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

In `@internal/backupcontroller/velerostrategy_controller.go` around lines 1016 -
1082, postRestoreRename currently constructs HelmRelease names with the
hardcoded vmNamePrefix which only works for VMInstance; replace that logic to
call helmReleaseNameForApp(target.AppKind, sourceAppName) and
helmReleaseNameForApp(target.AppKind, target.AppName) when computing
sourceHRName and targetHRName so VMDisk (or other kinds) use the correct
HelmRelease naming; update any references that used vmNamePrefix (in the debug
log and creation/deletion flows) to use these computed names and keep the rest
of the function unchanged.

Fix all unresolved CodeRabbit comments on this PR:

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

examples/backups/vmi/05-create-backupjob.sh (1)

38-39: ⚠️ Potential issue | 🟡 Minor

Logged command and executed command are inconsistent.

Line 38 logs -o yaml but Line 39 executes -o wide. This makes debugging confusing.

Suggested fix

-log_command "kubectl get backupjob test-backup -n $NAMESPACE -o yaml"
-kubectl get backupjob test-backup -n "$NAMESPACE" -o wide
+log_command "kubectl get backupjob test-backup -n $NAMESPACE -o wide"
+kubectl get backupjob test-backup -n "$NAMESPACE" -o wide

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/05-create-backupjob.sh` around lines 38 - 39, The logged
command and the executed command are inconsistent: log_command logs "kubectl get
backupjob test-backup -n $NAMESPACE -o yaml" while the actual execution uses "-o
wide"; update one so both match (choose either YAML or wide) by changing the
argument in either the log_command call or the kubectl invocation so that
log_command and the kubectl command use the same "-o" flag (e.g., both "-o yaml"
or both "-o wide") to avoid confusion when debugging; refer to the existing
log_command invocation and the kubectl get backupjob test-backup -n "$NAMESPACE"
-o wide line to make the change.

examples/backups/vmi/04-create-vminstance.sh (1)

43-43: ⚠️ Potential issue | 🟡 Minor

Don't mark VMInstance as ready without a readiness check.

Line 43 claims readiness, but the script only verified existence via kubectl get. Consider changing wording to "verified" or adding a real readiness wait.

Suggested fix

-log_success "VMInstance is ready"
+log_success "VMInstance created and verified"

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/04-create-vminstance.sh` at line 43, The script
currently calls log_success "VMInstance is ready" after only running kubectl
get, which only verifies existence; change this to either (A) update the message
to "VMInstance verified" or (B) implement a real readiness check before logging
success — e.g., use kubectl wait --for=condition=Ready (or poll the VMInstance
status field) with a sensible timeout and error handling, then call log_success
"VMInstance is ready" only after the wait/poll for readiness succeeds; locate
the call to log_success "VMInstance is ready" and the preceding kubectl get
invocation to apply the fix.

examples/backups/vmi/03-create-vmdisk.sh (1)

39-39: ⚠️ Potential issue | 🟡 Minor

Misleading "ready" status without actual readiness check.

The message claims the VMDisk is "ready" but only existence was verified. The parenthetical disclaimer suggests this isn't actually ready. Consider changing to "VMDisk verified" or adding a real readiness wait.

Suggested fix

-log_success "VMDisk is ready (image download may still be in progress)"
+log_success "VMDisk created (image download may still be in progress)"

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/03-create-vmdisk.sh` at line 39, The log message
currently emitted by log_success ("VMDisk is ready (image download may still be
in progress)") is misleading because the script only verifies existence; change
the message to accurately reflect verification (e.g., "VMDisk verified (image
download may still be in progress)") or implement a real readiness check before
calling log_success — for a readiness check add a loop that inspects the
disk/image state (the place where the script currently verifies existence before
the log_success call) and only call log_success once the image download/state
indicates completion; update the invocation of log_success accordingly.

examples/backups/vmi/06-restore-in-place.sh (1)

14-31: ⚠️ Potential issue | 🔴 Critical

Backticks in unquoted heredoc break shell parsing.

Line 29 uses backticks inside an unquoted <<EOF block, so bash treats <name>-orig-<hash> as command substitution.

Proposed fix

-    keepOriginalPVC: true # renames original VMI PVC before restore to `<name>-orig-<hash>`, only for in-place restore
+    keepOriginalPVC: true # renames original VMI PVC before restore to '<name>-orig-<hash>', only for in-place restore

#!/bin/bash
set -euo pipefail

# Verify current parse status
bash -n examples/backups/vmi/06-restore-in-place.sh

# Inspect the affected heredoc lines
sed -n '27,31p' examples/backups/vmi/06-restore-in-place.sh

Expected result: bash -n succeeds after replacing backticks.

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/06-restore-in-place.sh` around lines 14 - 31, The
heredoc used to render the RestoreJob contains backticks in the comment for
keepOriginalPVC ("<name>-orig-<hash>") which causes command substitution when
using an unquoted <<EOF; change the heredoc to a single-quoted delimiter
(<<'EOF') so the contents are treated literally (or alternatively escape the
backticks in the comment), e.g., update the kubectl apply -f - <<EOF block to
kubectl apply -f - <<'EOF' so the RestoreJob YAML (including the keepOriginalPVC
comment) parses correctly.

examples/backups/vmi/cleanup.sh (1)

33-64: ⚠️ Potential issue | 🟠 Major

Deletion pattern masks real kubectl errors.

The 2>/dev/null || log_warning "…not found" pattern suppresses all stderr and can misreport genuine failures as “not found”.

Proposed fix pattern (apply to all delete calls in this block)

-kubectl delete restorejob restore-to-copy-test -n "$NAMESPACE" 2>/dev/null || log_warning "RestoreJob restore-to-copy-test not found"
+kubectl delete restorejob restore-to-copy-test -n "$NAMESPACE" --ignore-not-found=true

#!/bin/bash
set -euo pipefail

# Find remaining masked delete calls in cleanup script
rg -n 'kubectl delete .*2>/dev/null \|\| log_warning' examples/backups/vmi/cleanup.sh

Expected result: no matches after migration to --ignore-not-found=true.

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/cleanup.sh` around lines 33 - 64, Replace the unsafe
stderr-suppression pattern in cleanup.sh where delete calls use "2>/dev/null ||
log_warning ..." with kubectl's built-in ignore flag so real errors aren't
masked; for each kubectl delete (e.g., commands deleting restorejob
restore-to-copy-test, backupjob test-backup, backup test-backup, vminstance
test, vmdisk ubuntu-source, backupclass velero,
velero.strategy.backups.cozystack.io vminstance-strategy/vmdisk-strategy, and
namespace "$TARGET_NAMESPACE"), remove the "2>/dev/null || log_warning" idiom
and add "--ignore-not-found=true" to the kubectl delete invocation and keep
log_warning only for unexpected failures if you still want explicit warnings.

examples/backups/vmi/07-restore-to-copy.sh (1)

27-45: ⚠️ Potential issue | 🔴 Critical

Backticks in unquoted heredoc can break this script.

Line 43 uses backticks inside <<EOF; bash treats that as command substitution.

Proposed fix

-    keepOriginalPVC: false # renames original VMI PVC before restore to `<name>-orig-<hash>`, only for in-place restore
+    keepOriginalPVC: false # renames original VMI PVC before restore to '<name>-orig-<hash>', only for in-place restore

#!/bin/bash
set -euo pipefail

# Parse-check the script
bash -n examples/backups/vmi/07-restore-to-copy.sh

# Show affected heredoc lines
sed -n '40,45p' examples/backups/vmi/07-restore-to-copy.sh

Expected result: bash -n succeeds once backticks are removed/escaped.

🤖 Prompt for AI Agents

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

In `@examples/backups/vmi/07-restore-to-copy.sh` around lines 27 - 45, The heredoc
used in the kubectl apply block (started with kubectl apply -f - <<EOF) contains
backticks in the comment on the keepOriginalPVC option (the string
`<name>-orig-<hash>` in the RestoreJob options) which causes command
substitution; fix it by making the heredoc literal (change <<EOF to <<'EOF') or
by escaping/backslash-quoting the backticks so the shell does not perform
command substitution, ensuring the RestoreJob options/comments remain unchanged
at runtime.

Fix all unresolved CodeRabbit comments on this PR:

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