openshift / openshift/oadp-operator

e2e follow-ups: checksum-verification race and BSL-availability retries, namespace mapping

Open
#2,421 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
92
Forks
93
Avg merge
1d 23h
Merged PRs (30d)
43

Description

Tracking issue for e2e follow-ups identified while validating openshift/oadp-operator#2404.

1. DONE -- Checksum-verification race in kdm restore specs (never actually verified data integrity)

Fixed in openshift/oadp-operator#2404 (commit 7bd0c0d8). The restore-side "hard" data-integrity checksum was silently skipped on effectively every run: it only trusted the read if the VM was still Halted immediately before and after, but the restored VM was already Running by the very first status read after restore, every time observed.

VirtOperator.EnsureVmHaltedForExclusivePVCAccess (tests/e2e/lib/virt_helpers.go) now deterministically stops the VM and waits for its virt-launcher pod to actually disappear, instead of hoping to catch a naturally-occurring halted window. Unconditional by design (Option A from below) -- a bypass keyed on "no pod right now" has the same race shape as the bug it closes, since the restored VM's spec.running stays true and KubeVirt could create a new launcher pod moments later.

Validated live end-to-end on a real bare-metal KVM cluster (not just locally reasoned), and this caught two further real bugs along the way that a synthetic-only pass wouldn't have exercised:

  • Trusting the VM's printableStatus string instead of the virt-launcher pod's actual presence (Paused/Starting/Stopping all still have an attached pod, same as Running).
  • GetAllPodsWithLabel returns an error on a genuinely empty list ("no Pod found") instead of a clean empty result -- every poll tick was misreading "the pod is actually gone" as a transient failure worth retrying, so the wait never succeeded on its own. Fixed by calling Pods().List() directly instead of routing through that helper.

Final live run: both kdm restore specs passed, hard assertions genuinely executed with real matching checksums (not skipped), stop-to-pod-gone taking ~5-36s in practice -- well inside the existing 5-minute budget once the actual bug was fixed.

2. RETRACTED -- "BSL-availability transient flakiness" was a misdiagnosis

Previously claimed here (and on 3 separate retest comments on migtools/kubevirt-datamover-controller#205): a PartiallyFailed backup phase with empty FailureReason/ValidationErrors was caused by a transient BSL-availability race, evidenced by a co-occurring "BackupStorageLocation is in unavailable state, skip syncing backup from it." log line.

This was wrong. That log line comes from velero's periodic backup-sync controller (pkg/controller/backup_sync_controller.go) -- a completely unrelated reconcile loop that syncs backup metadata from bucket to Kubernetes, nothing to do with the backup actually being created. It was coincidental noise present in the same log window purely by chance, all 3 times, because all 3 occurrences were the same real bug: migtools/kubevirt-datamover-controller#205's new quiesce-by-default logic marks the DataUpload Failed whenever a guest-agent-less VM's freeze attempt fails (which it always does on CirrOS), and that DataUpload failure is what velero's own async-operation tracking logs as a genuine Error()-level line in velero's own pod, which is what actually flips the Backup to PartiallyFailed (via velero's per-backup logCounter.GetCount(logrus.ErrorLevel) > 0 in backup_controller.go) -- unrelated to BSL availability at all. Full writeup: migtools/kubevirt-datamover-controller#205 (comment).

No action needed here as a result -- lib.IsBackupCompletedSuccessfully/lib.BSLsAreAvailable's single-shot-no-retry design may still be worth hardening on its own general merits, but there's no longer any confirmed evidence tying it to a real failure in this repo's own e2e runs.

3. GAP -- No e2e coverage for Restore.Spec.NamespaceMapping with kdm backups

Background: velero's NamespaceMapping lets a Restore recreate a backup's resources into a different namespace than they were backed up from. Velero core auto-rewrites an item's own metadata.namespace and (for PVs) spec.claimRef.namespace, but does not auto-rewrite namespace references embedded inside a resource's own spec fields pointing at other objects -- velero's own built-in CSI RestoreItemActions (pvc_action.go, volumesnapshotcontent_action.go) each manually consult input.Restore.Spec.NamespaceMapping[originalNamespace] for exactly this reason. Any custom RestoreItemAction (like kubevirt-datamover-plugin) that creates its own additional CRs referencing another namespaced object must do the same lookup itself -- velero doesn't do it automatically on a plugin's behalf.

There was a real bug here, already found and fixed upstream, independent of this issue: kubevirt-datamover-plugin's design is namespace-mapping-aware by intent -- it deliberately stashes the VM's original (source) namespace in the kubevirt-datamover.io/vm-namespace annotation, since the backup's S3 manifest lookup is keyed by the source namespace regardless of restore target (pkg/common/validation.go:56-58, intentional, not a bug). But KubeVirtDataDownloadReconciler.restoreVMRunStateIfAllSiblingsCompleted (internal/controller/kubevirt_datadownload_controller.go) reused that same source-namespace value for a live cluster lookup/update of the actual restored VirtualMachine -- which under NamespaceMapping lives in the target namespace, not the source one. Result: restored VMs stayed stuck Halted forever whenever NamespaceMapping was used (the plugin halts the VM during restore to avoid racing the disk restore, then relies on this reconcile step to flip it back). Reported as migtools/kubevirt-datamover-controller#222, fixed by migtools/kubevirt-datamover-controller#223 (merged, commit bea33e37): use dd.Spec.TargetVolume.Namespace for the live VM lookup/update, keep the stashed source-namespace annotation only for sibling-correlation/S3-manifest lookup.

What's actually missing, and what this item tracks: oadp-operator's own e2e suite has zero mentions of NamespaceMapping anywhere in tests/e2e/lib/virt_helpers.go or tests/e2e/virt_backup_restore_suite_test.go -- so even though #223 fixes the real bug upstream, nothing in this repo's own e2e would catch a regression of it. Also worth separately guarding: velero's per-namespace ResourcePolicy/resource include-exclude filtering (item_collector.go) could independently desync a VM from its VirtualMachineBackup/VirtualMachineBackuptracker/DataUpload CRDs if a filter narrow enough to include the VM but exclude those CRD kinds (or vice versa) were ever configured within an otherwise-included namespace -- velero has no built-in guard coupling those kinds together, so that coupling (if desired) would need to live in this repo's own test/plugin code.

Concrete action: add a kdm restore spec that backs up a VM and restores it via Restore.Spec.NamespaceMapping into a different namespace, asserting the VM resumes to its original run state -- this exact scenario (the one #222/#223 fixed) is currently untested end-to-end through the real operator.

[!Note]
Responses generated with Claude

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in tests/e2e/lib/virt_helpers.go and tests/e2e/virt_backup_restore_suite_test.go, then inspect existing kdm backup and restore specs. Add coverage that backs up a VM, restores it with Restore.Spec.NamespaceMapping into another namespace, and verifies the restored VM resumes its original run state.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing-qa
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
62/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.