Repository navigation
test(vminstance): dry-run evict the VM launcher pod - #4585
Conversation
github-actions
Bot
added
area/uncategorized
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe vminstance Chainsaw test submits a dry-run eviction for the VM’s running launcher pod and checks the response against the VMI’s Changesvminstance eviction test
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Merge Risk: ⚪ Minimal · up to The added eviction test aligns with KubeVirt’s dry-run behavior. No merge-blocking issue was identified. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
44eceb1 to
02d9616
Compare
No suite evicts a virt-launcher pod, so the platform's eviction behaviour is known only from reading KubeVirt. A server-side dry-run eviction goes through the same KubeVirt admission webhook as a drain and returns its decision without deleting anything. The test reads the VMI's LiveMigratable condition rather than assuming it, because the answer depends on the storage class: under LiveMigrateIfPossible a migratable VMI is refused with the evacuation message, and a non-migratable one is let through. It also pins the evictionStrategy that virt-api stamped into the VMI spec, since the webhook passes an eviction under strategy None as well, and checks that the dry run left the launcher pod and the VMI untouched. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
02d9616 to
966fccf
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM
The dry-run eviction step does real work; it isn't green by accident. It gates on two things that fail loudly instead of passing in silence. The chainsaw assert requires spec.evictionStrategy: LiveMigrateIfPossible and a LiveMigratable condition to be present, and that premise holds up: packages/system/kubevirt/templates/kubevirt-cr.yaml:126 sets that cluster default and packages/system/kubevirt/tests/kubevirt_cr_test.yaml:41 pins it. The shell step then asserts positively in both branches (eviction allowed with a Status/Success body when the VMI is non-migratable, refused with the evacuation message when it's migratable) and, the part that carries the weight, checks afterward that the launcher pod still exists with the same uid and no deletionTimestamp, and that the VMI carries no evacuationNodeName. Those two guards are what make "dry-run changed nothing" an actual assertion rather than a hope.
The negated checks are effective shell (set -eu, if ! …; then exit 1), not the inert BATS run + ! cmd shape. Names are pinned. The script op is bounded at timeout: 1m, and the one untimed assert inherits the global assert: 5m from .chainsaw.yaml. It's a pure addition, no existing assertion got rewritten, so nothing was quietly dropped from coverage.
Two non-blocking notes inline. Neither holds up the merge.
| echo "FAIL: dry-run eviction of migratable VMI $vmi was allowed; the webhook should refuse it and start an evacuation" >&2 | ||
| exit 1 | ||
| fi | ||
| if ! printf '%s\n' "$err" | grep -Fq "Eviction triggered evacuation of VMI \"$NAMESPACE/$vmi\""; then |
There was a problem hiding this comment.
[MINOR] True branch (eviction refusal) never runs on the CI lane
On the PR lane the post-install prep pins StorageProfile/local to RWO, so the VMI is non-migratable and LiveMigratable=False always holds, which means only the False branch runs in CI. The True branch, including this grep for Eviction triggered evacuation of VMI "<ns>/<vmi>", is never exercised by the lane, so that exact message string isn't covered by CI. A wording change in KubeVirt's evacuation path would only surface on a migratable setup, not here. That's a property of the dual-path design, not a defect; I'm flagging it so the dead-in-CI branch is a known fact for future edits.
There was a problem hiding this comment.
IvanHunters Yes, the refusal branch runs only where the VMI is migratable, and no CI lane gives it RWX storage today. I kept the branch so the step checks the right answer on such a cluster instead of failing there.
| spec: | ||
| evictionStrategy: LiveMigrateIfPossible | ||
| status: | ||
| (conditions[?type == 'LiveMigratable']): |
There was a problem hiding this comment.
[NIT] Possible transient Unknown between assert and script read
The assert waits only for a LiveMigratable condition to exist (any status). The script then reads its status once and fails the * branch on anything other than True/False. If virt-handler is mid-transition and the condition reads Unknown in that window, the step fails with "expected True or False". The risk is low, since wait-vm-running already asserted the VM Ready and the domain is synced by then, so the condition should be settled. Still a possible flake point worth knowing about.
There was a problem hiding this comment.
In KubeVirt v1.9.0 this window does not exist: virt-handler is the only writer of LiveMigratable, and calculateLiveMigrationCondition returns only True or False. The * branch would catch a future KubeVirt change, and I want that to fail loudly.
What this PR does
Part of #4422.
The e2e suite now asks KubeVirt whether it would let a VM launcher pod be evicted, without evicting it. Until now nothing in e2e evicted a launcher pod, so the drain behaviour set up in #4403 was known only from reading KubeVirt. Once the
vminstanceVM is running, the suite sends a dry-run Eviction (dryRun=All) for its virt-launcher pod and checks the answer. Nothing gets deleted.A dry run still goes through the KubeVirt eviction webhook, which is registered with
sideEffects: NoneOnDryRunin v1.9.0. WithLiveMigrateIfPossibleits answer follows the VMI'sLiveMigratablecondition. That condition depends on storage, so the test reads it instead of assuming it:LiveMigratable=True: the eviction must be refused with the webhook'sEviction triggered evacuation of VMI "<ns>/<name>"message (HTTP 429). A drain retries on that while the VM migrates away.LiveMigratable=False: the eviction must be allowed. The oldLiveMigratevalue refused this case with "is not live-migratable". KubeVirt v1.9.0 creates no PodDisruptionBudget for a VMI, so no PDB can refuse it either.The allowed answer alone proves little, because the webhook also lets an eviction through under
None. So the test also assertsevictionStrategy: LiveMigrateIfPossiblein the VMI spec, where virt-api copies the cluster value at creation. After the dry run it checks that the launcher pod is the same object and not terminating, and that the VMI has noevacuationNodeName.On the PR lane only the
Falsebranch runs. Post-install prep pinsStorageProfile/localto RWO because DRBD is off there, and an RWO disk makes the VMI non-migratable. Nightly and tag runs usereplicated, where I expect theTruebranch, but I have not seen this suite run on that lane yet. A real drain and a workload update after a KubeVirt bump are still open in #4422.I ran the script against a stub
kubectl. Both expected answers pass, and it fails on every wrong answer I tried, such as a refusal for a non-migratable VMI, a PDB-style 429, a missing condition, two launcher pods, or a replaced pod after the dry run.chainsaw lint(v0.2.15) and shellcheck pass.Screenshots
Not a UI change.
Downstream repositories
The diff touches only
hack/e2e-chainsaw/vminstance/and the suite README, and nothing in the trigger map covers either.Release note
Summary by CodeRabbit
LiveMigratablecondition, expecting eviction to be refused when the VM is live-migratable and accepted otherwise.jqrequirement for running it.