test(vmi): modernize the VMInstance/VMDisk backup/restore e2e to the BackupClass flow - #3427
Open
Andrey Kolkov (androndo) wants to merge 4 commits into
Open
test(vmi): modernize the VMInstance/VMDisk backup/restore e2e to the BackupClass flow#3427Andrey Kolkov (androndo) wants to merge 4 commits into
Andrey Kolkov (androndo) wants to merge 4 commits into
Conversation
…lass flow The VM backup path was uncovered: hack/e2e-chainsaw/backup/ held only a disabled Chainsaw port of the old backup.bats, still written against the retired API (kind: VirtualMachine, BackupJob.storageRef/strategyRef, velero-strategy-default) — it no longer matches the CRDs on main, so it guarded nothing (#3236). Replace it with a real round-trip that mirrors the etcd/postgres/mariadb precedent: a second Test doc in the vminstance suite, vminstance-2-backup-roundtrip, that drives the validated example scripts (examples/backups/vmi/run-all.sh) so the documented demo and the automated test can never drift. The flow exercises the current API end to end — Velero strategies -> BackupClass -> a booted Ubuntu VMInstance -> ad-hoc BackupJob (backupClassName, waits Succeeded) -> in-place RestoreJob (waits Succeeded, then waits for the restored VM to boot again) -> cross-namespace to-copy RestoreJob. A VM has no in-guest sentinel the host can read, so the proof is that the VM boots from the restored disk. It runs in tenant-root (Velero's CSI data mover reaches tenant-root's seaweedfs over same-tenant egress, which a tenant-test Pod cannot), with skipDelete + an explicit finally that prunes the cluster-scoped BackupClass and strategies and the cozy-velero Velero Backup/Restore. Like etcd-2-backup-roundtrip it is gated out of CI (VMI_E2E_S3_ROUNDTRIP=1): the in-cluster S3 endpoint serves a self-signed cert kind cannot trust and the platform BSL TLS/checksum defaults do not complete the flow in the sandbox (#3236); opt in on a real backups-enabled cluster. Make the vmi example CI-drivable and more correct while here: - run-all.sh / cleanup.sh are now non-interactive, matching every sibling example, so the harness can drive them; cleanup uses --ignore-not-found (idempotent, but does not mask a stuck delete). - 04-create-vminstance.sh now waits for the VM to boot before backup, as the etcd/postgres source steps do — backing up a half-imported disk is useless. - 06-restore-in-place.sh waits for the restored VM to boot again. - BackupJob/RestoreJob waits fail fast on a terminal Failed phase and get a 1800s budget (a 20Gi VM disk to S3 is far heavier than a DB dump). - Drop volumeSnapshotLocations from the demo strategies: snapshotMoveData routes volume data through the BSL, so a same-named VSL is not required and referencing a missing one fails the backup — the platform's own cozy-default-velero-vminstance strategy omits it too. - Default BACKUP_STORAGE_LOCATION to cozy-default, the platform BSL, so the demo works out of the box on a backups-enabled cluster. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
📝 WalkthroughWalkthroughAdds scripted VMDisk backup/restore automation, strengthens VMInstance readiness and failure handling, makes demo runners and cleanup non-interactive, and adds Chainsaw round-trip tests with Velero log collection and teardown. ChangesBackup round-trip automation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Chainsaw
participant RunAll
participant Kubernetes
participant Velero
Chainsaw->>RunAll: Run VMInstance or VMDisk round-trip
RunAll->>Kubernetes: Create source and backup/restore resources
Kubernetes->>Velero: Process backup and restore operations
Velero-->>Kubernetes: Update job and backup statuses
RunAll->>Kubernetes: Verify readiness and delete test resources
Chainsaw-->>Chainsaw: Collect Velero logs and run finally cleanup
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)hack/e2e-chainsaw/vminstance/chainsaw-test.yamlTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cozy-default binds VMDisk to a Velero strategy (cozy-default-velero-vmdisk) just as it does VMInstance, but that path was never exercised: the vminstance suite only tested standalone VMDisk creation, and the VMInstance round-trip backs up a disk attached to a running VM, not a standalone one. A standalone VMDisk is a distinct path — the strategy captures only the HelmRelease + PVC (+ configmaps), with no VirtualMachine / VMI / launcher pod. Add a self-contained example (examples/backups/vmdisk, mirroring the etcd/postgres harness style) driven by a new gated Chainsaw Test, vmdisk-2-backup-roundtrip, in the vminstance suite: Velero strategy -> BackupClass -> an imported 5Gi VMDisk -> ad-hoc BackupJob (waits Succeeded) -> in-place RestoreJob (waits Succeeded, then waits for the restored disk to be Bound + Ready). A standalone disk is not mounted anywhere, so the round-trip proof is that the restored VMDisk reconciles back to a Ready HelmRelease + Bound PVC; in-guest data is exercised by vminstance-2-backup-roundtrip, which boots the VM from its restored disk. Runs in tenant-root and reuses the VMI_E2E_S3_ROUNDTRIP=1 gate (same self-signed-S3 / BSL-defaults blocker as the VMInstance round-trip, #3236), with skipDelete + an explicit finally that prunes the cluster-scoped BackupClass and strategy, the cozy-velero Velero Backup/Restore, the VMDisk and any renamed original PVC. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Andrey Kolkov (androndo)
marked this pull request as ready for review
July 23, 2026 11:09
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@examples/backups/vmdisk/03-create-vmdisk.sh`:
- Around line 33-37: Add the documented event-driven backstop immediately before
the HelmRelease readiness wait in examples/backups/vmdisk/03-create-vmdisk.sh
lines 33-37, using the existing wait flow around kubectl and wait_for_field.
Apply the same backstop immediately before the restored VMDisk HelmRelease
readiness wait in examples/backups/vmdisk/05-restore-in-place.sh lines 41-43;
both sites must fail fast on HelmRelease readiness failures.
- Around line 35-36: Update the wait_for_field invocation for vm-disk-backup-src
to provide the DataVolume’s terminal failure phase as its fail value, so imports
stop immediately on failure while retaining Succeeded as the success value and
the existing namespace and timeout.
In `@examples/backups/vmi/04-create-vminstance.sh`:
- Around line 42-47: Add the documented event-driven readiness backstop before
each raw kubectl wait in examples/backups/vmi/04-create-vminstance.sh (lines
42-47) and before the restored-VM wait in
examples/backups/vmi/06-restore-in-place.sh (lines 51-54). In the HelmRelease
wait path of 04-create-vminstance.sh, also detect Ready=False and fail
immediately instead of waiting for the full timeout; preserve the existing VM
readiness wait behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a4fbc20c-1841-4c5e-931c-87dc289ef805
📒 Files selected for processing (19)
examples/backups/vmdisk/00-helpers.shexamples/backups/vmdisk/01-create-strategy.shexamples/backups/vmdisk/02-create-backupclass.shexamples/backups/vmdisk/03-create-vmdisk.shexamples/backups/vmdisk/04-create-backupjob.shexamples/backups/vmdisk/05-restore-in-place.shexamples/backups/vmdisk/cleanup.shexamples/backups/vmdisk/run-all.shexamples/backups/vmi/00-helpers.shexamples/backups/vmi/01-create-strategies.shexamples/backups/vmi/04-create-vminstance.shexamples/backups/vmi/05-create-backupjob.shexamples/backups/vmi/06-restore-in-place.shexamples/backups/vmi/07-restore-to-copy.shexamples/backups/vmi/cleanup.shexamples/backups/vmi/run-all.shhack/e2e-chainsaw/backup/README.mdhack/e2e-chainsaw/backup/chainsaw-test.yaml.disabledhack/e2e-chainsaw/vminstance/chainsaw-test.yaml
💤 Files with no reviewable changes (2)
- hack/e2e-chainsaw/backup/README.md
- hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled
Comment on lines
+33
to
+37
| log_step "Waiting for the VMDisk to import (HelmRelease Ready, DataVolume Ready, PVC Bound)..." | ||
| kubectl -n "$NAMESPACE" wait hr vm-disk-backup-src --for=condition=ready --timeout=600s | ||
| wait_for_field datavolume.cdi.kubevirt.io vm-disk-backup-src \ | ||
| '{.status.phase}' Succeeded "$NAMESPACE" 600 | ||
| wait_for_field pvc vm-disk-backup-src '{.status.phase}' Bound "$NAMESPACE" 300 |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add the required event-driven backstop before each HelmRelease wait. Both workflows call kubectl wait directly, which can consume the full timeout after reconciliation has already emitted a failure event.
examples/backups/vmdisk/03-create-vmdisk.sh#L33-L37: add the documented event-driven backstop immediately before the VMDisk HelmRelease readiness wait.examples/backups/vmdisk/05-restore-in-place.sh#L41-L43: add the same backstop immediately before the restored VMDisk HelmRelease readiness wait.
As per coding guidelines, “add event-driven backstops before every kubectl wait” and “fail fast on HelmRelease readiness.”
📍 Affects 2 files
examples/backups/vmdisk/03-create-vmdisk.sh#L33-L37(this comment)examples/backups/vmdisk/05-restore-in-place.sh#L41-L43
🤖 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 `@examples/backups/vmdisk/03-create-vmdisk.sh` around lines 33 - 37, Add the
documented event-driven backstop immediately before the HelmRelease readiness
wait in examples/backups/vmdisk/03-create-vmdisk.sh lines 33-37, using the
existing wait flow around kubectl and wait_for_field. Apply the same backstop
immediately before the restored VMDisk HelmRelease readiness wait in
examples/backups/vmdisk/05-restore-in-place.sh lines 41-43; both sites must fail
fast on HelmRelease readiness failures.
Source: Coding guidelines
Comment on lines
+35
to
+36
| wait_for_field datavolume.cdi.kubevirt.io vm-disk-backup-src \ | ||
| '{.status.phase}' Succeeded "$NAMESPACE" 600 |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For the Containerized Data Importer version used by this repository, what exact .status.phase value is reported for a failed DataVolume import?
💡 Result:
For a failed DataVolume import in the Containerized Data Importer (CDI), the exact.status.phase value reported is Failed [1][2][3]. This status is documented in official CDI resources as one of the possible status phases for a DataVolume [1][2]. When an import operation encounters a failure, such as checksum validation issues [1][2] or after exhausted retries [3], the DataVolume transitions to this state. It is important to distinguish the.status.phase, which indicates the high-level state (e.g., Failed), from the DataVolume conditions or Pod logs, which provide further detail regarding the specific reason for the failure [2][3].
Citations:
- 1: https://github.com/kubevirt/containerized-data-importer/blob/2a11d1a4/doc/datavolumes.md
- 2: https://github.com/kubevirt/containerized-data-importer/blob/main/doc/datavolumes.md
- 3: https://redhatquickcourses.github.io/ocp-virt-cookbook/ocp-virt-cookbook/1/storage/cdi-operations.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Locate relevant files and wait_for_field definition"
git ls-files | rg '(^|/)examples/backups/vmdisk/03-create-vmdisk\.sh$|wait_for_field|e2e-testing\.md|contributing\.md' || true
echo
echo "File excerpt:"
if [ -f examples/backups/vmdisk/03-create-vmdisk.sh ]; then
nl -ba examples/backups/vmdisk/03-create-vmdisk.sh | sed -n '1,90p'
else
echo "missing examples/backups/vmdisk/03-create-vmdisk.sh"
fi
echo
echo "Find wait_for_field definitions/usages:"
rg -n "function wait_for_field|wait_for_field|wait_for" -S --glob '!**/.git/**' . | sed -n '1,220p'Repository: cozystack/cozystack
Length of output: 364
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Script excerpt"
sed -n '1,90p' examples/backups/vmdisk/03-create-vmdisk.sh || true
echo
echo "wait_for_field definitions/usages"
rg -n "function wait_for_field|wait_for_field|wait_for" -S --glob '!**/.git/**' . | sed -n '1,240p' || true
echo
echo "Relevant docs excerpt"
sed -n '1,180p' docs/agents/e2e-testing.md | sed -n '1,220p' || trueRepository: cozystack/cozystack
Length of output: 28972
Fail immediately when the DataVolume import reaches its terminal failure phase. Line 35 omits wait_for_field’s fail value, so a failed image import waits the full 600-second budget despite never becoming Succeeded.
Proposed fix
wait_for_field datavolume.cdi.kubevirt.io vm-disk-backup-src \
- '{.status.phase}' Succeeded "$NAMESPACE" 600
+ '{.status.phase}' Succeeded "$NAMESPACE" 600 Failed📝 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
| wait_for_field datavolume.cdi.kubevirt.io vm-disk-backup-src \ | |
| '{.status.phase}' Succeeded "$NAMESPACE" 600 | |
| wait_for_field datavolume.cdi.kubevirt.io vm-disk-backup-src \ | |
| '{.status.phase}' Succeeded "$NAMESPACE" 600 Failed |
🤖 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 `@examples/backups/vmdisk/03-create-vmdisk.sh` around lines 35 - 36, Update the
wait_for_field invocation for vm-disk-backup-src to provide the DataVolume’s
terminal failure phase as its fail value, so imports stop immediately on failure
while retaining Succeeded as the success value and the existing namespace and
timeout.
Comment on lines
+42
to
+47
| log_step "Waiting for the VMInstance to boot (HelmRelease + VirtualMachine Ready)..." | ||
| log_command "kubectl -n $NAMESPACE wait hr vm-instance-test --for=condition=ready" | ||
| kubectl -n "$NAMESPACE" wait hr vm-instance-test --for=condition=ready --timeout=900s | ||
| log_command "kubectl -n $NAMESPACE wait virtualmachine.kubevirt.io/vm-instance-test --for=condition=Ready" | ||
| kubectl -n "$NAMESPACE" wait virtualmachine.kubevirt.io/vm-instance-test \ | ||
| --for=condition=Ready --timeout=600s |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add the required E2E readiness backstops.
The new raw kubectl wait calls omit the mandated event-driven backstop; additionally, the HelmRelease wait at Line 44 waits through Ready=False until its full timeout. Use the documented readiness wrapper/backstop and fail immediately on HelmRelease failure.
examples/backups/vmi/04-create-vminstance.sh#L42-L47: add the backstop before both waits and a terminal HelmRelease failure path.examples/backups/vmi/06-restore-in-place.sh#L51-L54: add the backstop before the restored-VM wait.
As per coding guidelines: “add event-driven backstops before every kubectl wait” and “fail fast on HelmRelease readiness.”
📍 Affects 2 files
examples/backups/vmi/04-create-vminstance.sh#L42-L47(this comment)examples/backups/vmi/06-restore-in-place.sh#L51-L54
🤖 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 `@examples/backups/vmi/04-create-vminstance.sh` around lines 42 - 47, Add the
documented event-driven readiness backstop before each raw kubectl wait in
examples/backups/vmi/04-create-vminstance.sh (lines 42-47) and before the
restored-VM wait in examples/backups/vmi/06-restore-in-place.sh (lines 51-54).
In the HelmRelease wait path of 04-create-vminstance.sh, also detect Ready=False
and fail immediately instead of waiting for the full timeout; preserve the
existing VM readiness wait behavior.
Source: Coding guidelines
Make vmdisk-2-backup-roundtrip actually exercise a Velero backup/restore in CI instead of exiting on the gate. It is the in-CI proof that the cozy-default-velero-vmdisk path works end to end: a CSI VolumeSnapshot of the LINSTOR `replicated` block disk, moved to S3 by Velero's data mover (kopia), then restored back to a Bound, Ready disk. The platform ships a default VolumeSnapshotClass for linstor.csi.linbit.com and runs Velero with features: EnableCSI + deployNodeAgent, so the machinery is present in the sandbox. VMDisk (not VMInstance) is the un-gated CI case on purpose: it drives the same Velero + CSI-data-mover + S3 path but captures only HelmRelease + PVC (+ configmaps) — no nested VM boot, no guest-agent freeze, no 20Gi image — so it is fast and reliable enough for kind. The heavier VMInstance round-trip stays gated behind VMI_E2E_S3_ROUNDTRIP=1. The platform cozy-default BSL derives the EXTERNAL S3 ingress endpoint, which is unroutable inside kind (the same reason the postgres round-trip targets the in-cluster endpoint). So the harness stands up a throwaway BSL cozy-e2e-vmdisk pointing at the in-cluster seaweedfs endpoint with insecureSkipTLSVerify (kind cannot trust the self-signed SeaweedFS CA) and checksumAlgorithm="" (the velero-plugin-for-aws v1.14 / AWS-SDK-v2 vs SeaweedFS incompatibility) — the #3236 fixes applied at the BSL — reusing the platform bucket + cozy-backups-creds under a distinct prefix. The example scripts stay generic and just consume BACKUP_STORAGE_LOCATION. Runs in tenant-root so the data-mover DataUpload Pods reach seaweedfs over same-tenant egress; the finally tears down the throwaway BSL alongside the example cleanup. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/e2e-chainsaw/vminstance/chainsaw-test.yaml (1)
356-369: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTeardown can leak the throwaway BSL when
cleanup.shfails.The
finallyruns cleanup and BSL deletion in a singleset -euscript. Ifexamples/backups/vmdisk/cleanup.shexits non-zero,set -eaborts before Line 369, socozy-e2e-vmdiskis never deleted — a leak precisely in the failure path this teardown is meant to harden. Capture cleanup's status but always delete the BSL, then propagate the failure so the step still has teeth.♻️ Always delete the BSL, still fail on cleanup error
content: | - set -eu - cd ../../.. - NAMESPACE="$NAMESPACE" examples/backups/vmdisk/cleanup.sh - kubectl -n cozy-velero delete bsl cozy-e2e-vmdisk --ignore-not-found + set -u + cd ../../.. + rc=0 + NAMESPACE="$NAMESPACE" examples/backups/vmdisk/cleanup.sh || rc=$? + kubectl -n cozy-velero delete bsl cozy-e2e-vmdisk --ignore-not-found + exit "$rc"🤖 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 `@hack/e2e-chainsaw/vminstance/chainsaw-test.yaml` around lines 356 - 369, Update the vmdisk backup-flow cleanup script around examples/backups/vmdisk/cleanup.sh to capture its exit status without aborting, always execute the cozy-e2e-vmdisk BSL deletion, then return the captured cleanup status so teardown failures still fail the step.
🤖 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.
Nitpick comments:
In `@hack/e2e-chainsaw/vminstance/chainsaw-test.yaml`:
- Around line 356-369: Update the vmdisk backup-flow cleanup script around
examples/backups/vmdisk/cleanup.sh to capture its exit status without aborting,
always execute the cozy-e2e-vmdisk BSL deletion, then return the captured
cleanup status so teardown failures still fail the step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f4e94f41-75c8-4738-9452-5a8e92b44272
📒 Files selected for processing (1)
hack/e2e-chainsaw/vminstance/chainsaw-test.yaml
… tiny VM Drop the VMI_E2E_S3_ROUNDTRIP gate on vminstance-2-backup-roundtrip so a real Velero backup/restore of a booted VMInstance runs in CI, kept affordable by shrinking the VM to the minimum: a cirros guest (~20MB image, instanceProfile cirros) on the smallest instance type (u1.nano, 1 vCPU / 512Mi) with a 2Gi disk, and SKIP_RESTORE_TO_COPY=1 so only the in-place restore runs (the cross-namespace to-copy restore stands up a second full VM — too heavy for kind and redundant for proving the round-trip). cirros has no qemu-guest-agent, so Velero takes a crash-consistent snapshot without a filesystem freeze, which is fine for exercising the mechanics. To keep the example a faithful demo while letting CI run it cheaply, the vmi scripts gained overrides (all defaulting to the existing ubuntu demo): VMI_IMAGE_URL + VMI_DISK_STORAGE (03-create-vmdisk), VMI_INSTANCE_PROFILE + VMI_INSTANCE_TYPE (04-create-vminstance), and SKIP_RESTORE_TO_COPY (run-all). Like vmdisk-2-backup-roundtrip, the harness stands up a throwaway in-cluster BSL cozy-e2e-vmi (seaweedfs endpoint + insecureSkipTLSVerify + checksumAlgorithm="", the #3236 fixes) reusing the platform bucket + cozy-backups-creds, and the finally tears it down alongside the example cleanup. Both VM backup round-trips now run un-gated in CI. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hack/e2e-chainsaw/vminstance/chainsaw-test.yaml`:
- Around line 243-245: Update both BSL validation loops for cozy-e2e-vmi (lines
243-245) and cozy-e2e-vmdisk (lines 373-378) to inspect the phase each
iteration, stop successfully on Available, and immediately print the BSL and
exit with failure on Unavailable; retain the 180-second timeout for other
phases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bbed627-31c3-44d0-8b8d-344711c060d3
📒 Files selected for processing (4)
examples/backups/vmi/03-create-vmdisk.shexamples/backups/vmi/04-create-vminstance.shexamples/backups/vmi/run-all.shhack/e2e-chainsaw/vminstance/chainsaw-test.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/backups/vmi/04-create-vminstance.sh
- examples/backups/vmi/run-all.sh
Comment on lines
+243
to
+245
| echo "Waiting for BSL cozy-e2e-vmi to validate Available..." | ||
| timeout 180 sh -ec 'until [ "$(kubectl -n cozy-velero get bsl cozy-e2e-vmi -o jsonpath="{.status.phase}" 2>/dev/null)" = "Available" ]; do sleep 5; done' | ||
| kubectl -n cozy-velero get bsl cozy-e2e-vmi |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== docs guideline files existence ==="
ls -l docs/agents/contributing.md docs/agents/changelog.md docs/agents/releasing.md docs/release.md docs/agents/overview.md docs/agents/e2e-testing.md 2>&1 || true
echo "=== target file outline/sections ==="
wc -l hack/e2e-chainsaw/vminstance/chainsaw-test.yaml
sed -n '220,260p' hack/e2e-chainsaw/vminstance/chainsaw-test.yaml
sed -n '350,390p' hack/e2e-chainsaw/vminstance/chainsaw-test.yaml
echo "=== BSL phase references in repository ==="
rg -n "status\.phase|phase: Unavailable|Unavailable|Available" hack/e2e-chainsaw/vminstance/chainsaw-test.yaml docs . --glob '!vendor/**' --glob '!**/.git/**' | head -n 200Repository: cozystack/cozystack
Length of output: 36223
Fail immediately when the BSL becomes Unavailable.
The current loops keep sleeping until the 180s timeout once the BSL reports Unavailable, then only print the BSL. Inspect the phase each iteration; break when Available, and print the BSL then exit on Unavailable for both cozy-e2e-vmi and cozy-e2e-vmdisk.
📍 Affects 1 file
hack/e2e-chainsaw/vminstance/chainsaw-test.yaml#L243-L245(this comment)hack/e2e-chainsaw/vminstance/chainsaw-test.yaml#L373-L378
🤖 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 `@hack/e2e-chainsaw/vminstance/chainsaw-test.yaml` around lines 243 - 245,
Update both BSL validation loops for cozy-e2e-vmi (lines 243-245) and
cozy-e2e-vmdisk (lines 373-378) to inspect the phase each iteration, stop
successfully on Available, and immediately print the BSL and exit with failure
on Unavailable; retain the 180-second timeout for other phases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Closes the uncovered VM backup path called out in #3236.
hack/e2e-chainsaw/backup/held only a disabled Chainsaw port of the oldbackup.bats, still written against the retired API (kind: VirtualMachine,BackupJob.storageRef/strategyRef,velero-strategy-default) — it no longer matches the CRDs onmain, so it guarded nothing. This removes it and replaces it with a real round-trip.The new coverage mirrors the
etcd/postgres/mariadbprecedent: a second Test doc in thevminstancesuite,vminstance-2-backup-roundtrip, that drives the validated example scripts (examples/backups/vmi/run-all.sh) so the documented demo and the automated test can never drift. The flow exercises the current API end to end — Velero strategies → BackupClass → a booted Ubuntu VMInstance → ad-hocBackupJob(backupClassName, waitsSucceeded) → in-placeRestoreJob(waitsSucceeded, then waits for the restored VM to boot again) → cross-namespace to-copyRestoreJob. A VM has no in-guest sentinel the host can read, so the round-trip proof is that the VM boots from the restored disk.It runs in
tenant-root(Velero's CSI data mover reachestenant-root's seaweedfs over same-tenant egress, which atenant-testPod cannot — same reasoning as the postgres suite), withskipDeleteplus an explicitfinallythat prunes the cluster-scoped BackupClass and strategies and thecozy-veleroVeleroBackup/Restore. Likeetcd-2-backup-roundtripit is gated out of CI (VMI_E2E_S3_ROUNDTRIP=1): the in-cluster S3 endpoint serves a self-signed cert kind cannot trust and the platform BackupStorageLocation TLS/checksum defaults do not complete the flow in the sandbox (#3236). Opt in on a real backups-enabled cluster.It also adds the other
cozy-defaultVelero path — standalone VMDisk (cozy-default-velero-vmdisk), previously covered only for creation — and this one runs un-gated, as a real backup/restore in CI.vmdisk-2-backup-roundtripdrives a self-containedexamples/backups/vmdiskflow (Velero strategy → BackupClass → an imported 5Gi VMDisk → ad-hocBackupJob→ in-placeRestoreJob). A standalone disk captures only the HelmRelease + PVC (no VirtualMachine/VMI/launcher pod), so it is a distinct, lighter path than the VMInstance round-trip: no nested VM boot, no guest-agent freeze, no 20Gi image — fast and reliable enough for the kind sandbox. It exercises the full Velero machinery for real: a CSIVolumeSnapshotof the LINSTORreplicatedblock disk → data mover (kopia) upload to S3 →RestoreJob→ the disk restored back to a Bound, Ready PVC (in-guest data is exercised by the gated VMInstance round-trip, which boots the VM).To make that reachable in CI, the harness stands up a throwaway BSL
cozy-e2e-vmdiskpointing at the in-clusterseaweedfs-s3.tenant-root:8333withinsecureSkipTLSVerify+checksumAlgorithm: ""— the #3236 fixes applied at the BSL — reusing the platform bucket +cozy-backups-credsunder a distinct prefix. This is necessary because the platformcozy-defaultBSL derives the external S3 ingress endpoint, which is an unroutable placeholder inside kind (the same reason the postgres round-trip targets the in-cluster endpoint). The example scripts stay generic and just consumeBACKUP_STORAGE_LOCATION. Runs intenant-root,skipDelete, with afinallythat tears down the throwaway BSL alongside the example cleanup.Made the
vmiexample CI-drivable and more correct while here:run-all.sh/cleanup.share now non-interactive, matching every sibling example, so the harness can drive them; cleanup uses--ignore-not-found(idempotent, but does not mask a stuck delete).04-create-vminstance.shnow waits for the VM to boot before backup, as the etcd/postgres source steps do — backing up a half-imported disk is useless.06-restore-in-place.shwaits for the restored VM to boot again.BackupJob/RestoreJobwaits fail fast on a terminalFailedphase and get a 1800s budget (a 20Gi VM disk to S3 is far heavier than a DB dump).volumeSnapshotLocationsfrom the demo strategies:snapshotMoveDataroutes volume data through the BSL, so a same-named VSL is not required and referencing a missing one fails the backup — the platform's owncozy-default-velero-vminstancestrategy omits it too.BACKUP_STORAGE_LOCATIONtocozy-default, the platform BSL, so the demo works out of the box on a backups-enabled cluster.Verified in CI:
vmdisk-2-backup-roundtripran the real flow green (BSLcozy-e2e-vmdiskvalidatedAvailable,BackupJob→Succeeded,RestoreJob→Succeeded, restored disk Bound + Ready, ~244s), whilevminstance-2-backup-roundtripstayed correctly gated (fast no-op). The VMInstance round-trip is kept gated because a nested-VM boot + guest freeze + 20Gi CSI snapshot + two restores is too heavy/flaky for the kind sandbox; run it on a real backups-enabled cluster withVMI_E2E_S3_ROUNDTRIP=1.Screenshots
N/A — no UI changes.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff (shell scripts underexamples/backups/vmi/, one Chainsaw test, a deleted stale suite dir): nopackages/apps|extraadd/rename, novalues.schema.json/enum/default changes, no CRD/ApplicationDefinition/label/annotation/namespace/metric changes.cozystack/examplesholds Terraform for bare nodes and is unrelated to the in-repoexamples/tree. Nothing is reached.Release note
Summary by CodeRabbit
New Features
Bug Fixes
Chores