Repository navigation
[AI Generated] BugFix: Retain passthrough allocations on cleanup failure - #4830
Vivek Yadav (vyadavmsft) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup failures can strand retained allocations because environments are marked deleted before teardown completes, while lost undefine responses leave stale domain handles that block recovery on later attempts.
2 open issues
What changed in this PR
This PR isolates the libvirt cleanup fix from #4827, retaining passthrough allocations when domain teardown fails rather than releasing devices with uncertain ownership.
Changes:
- Propagate libvirt destroy and undefine errors.
- Add regression coverage for allocation retention, cleanup retry, and subsequent reuse.
- Document cleanup ownership and recovery expectations.
Suggested integration validation on a custom passthrough host:
Key Test Cases:
perf_tcp_max_pps_passthrough_host_guest|perf_tcp_ntttcp_passthrough_host_guest
Impacted LISA Features:
N/A — libvirt lifecycle and device-pool ownership; no feature class directly changed.
Tested Azure Marketplace Images:
N/A — custom libvirt hosts with Ubuntu 22.04 LTS cloud-image guests; no Marketplace validation claimed.
| File | Description |
|---|---|
| selftests/test_libvirt_device_pool.py | Tests retained allocations after cleanup failure and release after retry. |
| lisa/sut_orchestrator/libvirt/platform.py | Re-raises destroy and undefine errors to prevent premature device release. |
| docs/run_test/platform.rst | Documents allocation retention and cleanup retry expectations. |
Review effort: Balanced
AI Test Case SelectionSelected 1 test case(s): smoke_test Marketplace image: Result: Succeeded |
01bb4f9 to
6ba1ec1
Compare
AI Test Case SelectionSelected 1 test case(s): smoke_test Marketplace image: Result: Succeeded |
6ba1ec1 to
29eb888
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Port-forward cleanup cannot recover from a transient NAT deletion failure because retries fail on the already-removed FORWARD rule and completed entries remain pending, blocking subsequent prepare/deploy calls.
1 open issue
2 resolved since last review
Review effort: Balanced
AI Test Case SelectionSelected 1 test case(s): smoke_test Marketplace image: Result: Succeeded |
29eb888 to
fb4332c
Compare
AI Test Case SelectionSelected 1 test case(s): smoke_test Marketplace image: Result: Succeeded |
Fix cleanup ownership and recovery gaps after connectivity loss during libvirt passthrough operations. This is follow-up hardening to #4689 (cached IOMMU allocation mappings and management-route protection) and #4729 (bounded Cloud Hypervisor passthrough stopping), not a replacement for those fixes. Previously, libvirt destroy/undefine errors could be logged and swallowed, allowing cleanup to return devices to the available pool even though the domain might still own them. Simply rethrowing is insufficient: the platform marks an environment Deleted before teardown, and the runner can then discard it or reset its nodes without scheduling another cleanup. Preserve failed teardown ownership inside the libvirt platform: - Propagate destroy and undefine failures before releasing devices. - Retain snapshots of the original nodes, disk directory, and forwarding rules independently of runner environment resets. - Retry deletion up to three times, retry pending cleanup before preparing or deploying environments, and retry again during platform shutdown. - Report unresolved final cleanup explicitly while preserving allocations and the device pool's management-route protection. - Track disk directories per environment so delayed cleanup cannot remove a different environment's current directory. Make teardown recoverable after a lost undefine response or stale handle. Reconcile domain absence through a fresh lookup and require a healthy libvirt connection before treating a missing domain as successful cleanup. Retain allocations when lookup fails or the domain state remains uncertain. Cloud Hypervisor passthrough stop failures use CloudHypervisorDomainStopError, not libvirtError, so base-class reconciliation alone missed the actual subclass path. Handle that error in cleanup too: a confirmed absent domain can release its devices, and an existing but confirmed stopped domain can continue through undefine. Active domains remain allocated. Keep the previously-failed-stop safety latch unchanged in the restart/reuse path; cleanup recovery must not make an unsafe guest reusable. Make libvirt port-forward cleanup retry-safe after partial deletion. If a FORWARD rule is deleted but NAT deletion fails, retry previously failed on the already-absent FORWARD rule and stranded the pending environment. Delete each rule independently, accept only the explicit missing-rule diagnostic with exit code 1, and propagate other command/transport errors. Remove completed forwarding entries immediately so later failures cannot force retries of earlier completed entries. Keep this handling local to libvirt; the shared firewall tool is unchanged. No runner, shared-platform, firewall-tool, documentation, or repository test-file changes. Validation: - 82 targeted checks passed: 50 existing tests plus 32 out-of-tree cleanup regression checks, including the actual Cloud Hypervisor subclass. - Verified missing domains, previously failed stops, inactive and active domains, unhealthy connections, permission/state-query failures, retained cleanup after runner reset, and offline cached device release. - Verified missing forwarding rules, partial NAT deletion failure, removal of completed entries, pending-owner recovery, forced privileged execution, and propagation of permission, lock, command, and transport failures. - Black, flake8, mypy, and git diff --check passed. - A focused physical run before this forwarding change passed both host-guest NIC passthrough performance cases and normal domain teardown. It used different devices for successive environments and did not encounter or inject the original cleanup connectivity failure. - The forwarding retry change has local fault-regression coverage only; no physical rerun or hardware proof of failure recovery is claimed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fb4332c to
00b01b2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup may attempt to install iptables even when no forwarding rules exist, allowing an installation failure to leave an empty environment pending and block subsequent deployments.
2 open issues
1 resolved since last review
Review effort: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
AI Test Case SelectionSelected 1 test case(s): smoke_test Marketplace image: Result: Canceled |
AI Test Case SelectionSelected 1 test case(s): smoke_test Marketplace image: Result: Succeeded |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because cleanup retries can repeat completed deletions after overlapping lifecycle operations or partial failures, potentially removing rules the cleanup does not own.
No open issues 🎉
Review effort: Balanced
AI Test Case SelectionSelected 1 test case(s): smoke_test Marketplace image: Result: Succeeded |
Description
Propagate libvirt destroy/undefine errors so uncertain passthrough allocations and domain context remain retained until cleanup succeeds. Allow a later successful retry to release resources rather than silently losing ownership.
This is the focused cleanup replacement for part of #4827. It changes only the libvirt lifecycle, its device-pool regression tests, and lifecycle documentation.
Related Issue
No public issue is linked.
Type of Change
Checklist
Test Validation
Key Test Cases:
perf_tcp_max_pps_passthrough_host_guest|perf_tcp_ntttcp_passthrough_host_guest|test_failed_domain_cleanup_retains_devices_until_retry
Impacted LISA Features:
N/A: libvirt lifecycle and passthrough allocation ownership, not a feature-class change.
Tested Azure Marketplace Images:
N/A: Ubuntu 22.04 LTS cloud-image guests on a custom passthrough host.
Test Results
Eighteen native device-pool tests, seven process regressions and the applicable Black, flake8, pylint and mypy checks passed on validated aggregate
b6fe4c76c3d65a7178e609de52f7bbea21104864. Native fault controls prove destroy/undefine error propagation, retained allocations, retry, release and reallocation.The healthy physical run did not inject or encounter the original cleanup fault. Its successive allocations used different devices, not same-device reuse; no final whole-host inventory is claimed. Image-cache identity and performance-summary attribution are outside this change.
Every changed file blob and mode at branch head
01bb4f9f75b9d3811248c0b6e48550a38c91ff95matches that aggregate. The integration run used the aggregate, not this standalone branch; no new run is claimed for the split.