[AI Generated] BugFix: stabilize libvirt device passthrough

[Vivek Yadav (vyadavmsft)]

Preserve allocated devices original IOMMU groups so cleanup does not require a live host connection. Pin SSH return traffic to the management NIC before eligible NICs are detached, and remove the temporary route during platform cleanup.

Description

Related Issue

Type of Change

• Bug fix
• New feature
• Breaking change
• Refactoring
• Documentation update

Checklist

• Description is filled in above
• No credentials, secrets, or internal details are included
• Peer review requested (if not, add required peer reviewers after raising PR)
• Tests executed and results posted below

Test Validation

Key Test Cases:

Impacted LISA Features:

Tested Azure Marketplace Images:

Test Results

Image	VM Size	Result
		PASSED / FAILED / SKIPPED

[Copilot]

Pull request overview

This PR hardens libvirt PCI device passthrough teardown by (1) recording the original IOMMU group for allocated devices so they can be returned to the pool without querying a possibly-disconnected host, and (2) pinning SSH return traffic to the management NIC before detaching eligible passthrough NICs, with best-effort cleanup during platform shutdown.

Changes:

• Track allocated PCI device → IOMMU group mappings during request_devices() and use them in release_devices() to avoid host-side discovery during cleanup.
• Add a management-route “guard” that installs a temporary per-peer route for SSH return traffic when the current return path uses a passthrough-candidate NIC; remove it during cleanup (including in platform._cleanup()).
• Add selftests covering management route pinning/cleanup behavior and disconnected-host teardown scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
selftests/test_libvirt_device_pool.py	Adds unit tests for the new management-route stabilization and offline-safe device release behaviors.
lisa/sut_orchestrator/libvirt/platform.py	Ensures device-pool cleanup runs in a finally block during platform cleanup.
lisa/sut_orchestrator/libvirt/libvirt_device_pool.py	Implements management-route stabilization, stores allocated device group mappings, and uses them during release/cleanup.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

lisa/sut_orchestrator/libvirt/libvirt_device_pool.py:143

• Major: release_devices() will raise an unhandled KeyError if _allocated_device_groups has no entry for a device (e.g., if the pool instance was recreated, the context was built without request_devices(), or the same device appears twice). Please fail deterministically with a LisaException (or fall back to _get_device_iommu_group() when the host is reachable) instead of allocated_device_groups.pop(device_id).

            allocated_device_groups = self._allocated_device_groups.get(pool_type, {})
            for device in devices_list:
                device_id = self._get_pci_address_str(device)
                iommu_grp = allocated_device_groups.pop(device_id)
                pool_devices = pool.get(iommu_grp, [])

[LiliDeng]

Purna Pavan Chandra (@pupacha) Anirudh Rayabharam (@anirudhrb) please review, thanks.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

lisa/sut_orchestrator/libvirt/platform.py:1293

• The timeout exception message doesn’t give actionable next steps. Consider including what to check (e.g., console log, libvirt domain state, DHCP/bridge config) and/or where logs are captured so operators know how to investigate.

                raise GuestBootTimeoutError(
                    f"no IP addresses found for {node_context.vm_name}."
                    " Guest OS might have failed to boot"
                )

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:87

• use_new_environment=True significantly increases test cost/runtime. Please add a brief justification (why a fresh environment is required for this case) or consider a cheaper reset approach if possible.

        use_new_environment=True,

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (14)

Previously missed (1) — in code that hasn't changed since the last review.

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:87

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

This issue also appears in the following locations of the same file:

• line 124
• line 163
• line 200
• line 236
• line 294
• ...and 6 more

        use_new_environment=True,

lisa/sut_orchestrator/libvirt/libvirt_device_pool.py:142

• Major: allocated_device_groups.pop(device_id) can raise a KeyError if the device ID was not recorded (e.g., unexpected node_context contents or a partially-initialized pool), which would break device release/cleanup. Consider using pop(device_id, None) and raising a LisaException with a clear message when the mapping is missing.

            allocated_device_groups = self._allocated_device_groups.get(pool_type, {})
            for device in devices_list:
                device_id = self._get_pci_address_str(device)
                iommu_grp = allocated_device_groups.pop(device_id)

lisa/sut_orchestrator/libvirt/ch_platform.py:357

• Major: Avoid time.sleep(1) here; fixed sleeps can make shutdown handling flaky/slow. Use a bounded retry/polling approach (with a maximum timeout) to wait for the domain/process to be reaped before re-running _run_bounded_domain_stop().

        # Let the timed-out libvirt destroy job reap the process and release its
        # host devices before checking the domain state again.
        time.sleep(1)
        cleanup_result = self._run_bounded_domain_stop(node_context.vm_name)

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:124

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:163

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:200

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:236

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:294

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:344

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:379

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:417

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:453

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:488

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:541

• Major: use_new_environment=True significantly increases cost/runtime. Please add a brief justification (e.g., in the test description) explaining why a fresh environment is required instead of reusing/rebooting.

        use_new_environment=True,

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (1)

lisa/sut_orchestrator/libvirt/ch_platform.py:358

• Major: Avoid time.sleep(1) here; a fixed delay can slow runs and still be flaky (too short on busy hosts). Prefer a bounded poll (with a timeout) that checks whether the domain/process has actually exited or whether virsh destroy is now succeeding, so behavior is deterministic without an unconditional sleep.

        # Let the timed-out libvirt destroy job reap the process and release its
        # host devices before checking the domain state again.
        time.sleep(1)
        cleanup_result = self._run_bounded_domain_stop(node_context.vm_name)

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/sut_orchestrator/libvirt/libvirt_device_pool.py:143

• release_devices() assumes every device being released has an entry in _allocated_device_groups[pool_type] and unconditionally does allocated_device_groups.pop(device_id). If release_devices() is called for a context that wasn't allocated via request_devices() (or if the mapping is already partially cleared), this will raise KeyError and can break cleanup.

Consider using allocated_device_groups.pop(device_id, fallback) (e.g., fallback to _get_device_iommu_group(device)) or explicitly checking membership and raising a clearer LisaException with context.

            pool = self.available_host_devices.get(pool_type, {})
            allocated_device_groups = self._allocated_device_groups.get(pool_type, {})
            for device in devices_list:
                device_id = self._get_pci_address_str(device)
                iommu_grp = allocated_device_groups.pop(device_id)
                pool_devices = pool.get(iommu_grp, [])

lisa/microsoft/testsuites/performance/networkperf_passthrough.py:93

• These passthrough performance cases now set use_new_environment=True. This forces a fresh deployment per test case and can significantly increase cost/time compared to reusing an environment (especially for multi-node cases).

If this is required to avoid stale passthrough state, please add a short in-code note explaining why it’s necessary; otherwise consider limiting it to the specific scenarios that need isolation.

        """,
        priority=3,
        timeout=TIMEOUT,
        use_new_environment=True,
        requirement=simple_requirement(

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

lisa/schema.py:1918

• PR hygiene: the PR description still contains template placeholders (Description/Related Issue/Tests executed). Please replace the placeholders with a short explanation of what changed and why, and link the related issue (if any) for traceability.

    # Exit at the first failed test case, useful for reproducing issues.
    # When True, all not started test cases will be marked as skipped.
    exit_on_first_failure: bool = False
    # If set, only a failed result containing this text triggers early termination.
    exit_on_first_failure_message: str = ""

lisa/sut_orchestrator/libvirt/libvirt_device_pool.py:143

• release_devices() assumes every device being released exists in _allocated_device_groups; if the mapping is missing (e.g., partial allocation failure, unexpected node_context state, or a prior in-memory reset), allocated_device_groups.pop(device_id) will raise KeyError and abort platform cleanup. Raise a LisaException with a clear message instead of leaking KeyError so cleanup failures are actionable.

            allocated_device_groups = self._allocated_device_groups.get(pool_type, {})
            for device in devices_list:
                device_id = self._get_pci_address_str(device)
                iommu_grp = allocated_device_groups.pop(device_id)
                pool_devices = pool.get(iommu_grp, [])

lisa/sut_orchestrator/libvirt/ch_platform.py:361

• Using time.sleep(1) adds a fixed delay and can still be flaky if libvirt needs more (or less) time to reap the timed-out destroy job. Prefer a bounded wait helper (e.g., check_till_timeout) that retries the cleanup destroy until it succeeds or a small timeout elapses, so the code is both deterministic and avoids hard-coded sleeps.

        # Let the timed-out libvirt destroy job reap the process and release its
        # host devices before checking the domain state again.
        time.sleep(1)
        cleanup_result = self._run_bounded_domain_stop(node_context.vm_name)
        if not self._domain_stop_succeeded(cleanup_result):

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

lisa/sut_orchestrator/libvirt/libvirt_device_pool.py:142

• release_devices() can raise a KeyError when _allocated_device_groups does not contain the device (or the pool type), because it uses allocated_device_groups.pop(device_id) without a default. This would turn a cleanup path into an unhandled exception (e.g., during environment deletion) and is harder to diagnose than the previous host-query behavior.

            pool = self.available_host_devices.get(pool_type, {})
            allocated_device_groups = self._allocated_device_groups.get(pool_type, {})
            for device in devices_list:
                device_id = self._get_pci_address_str(device)
                iommu_grp = allocated_device_groups.pop(device_id)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

lisa/sut_orchestrator/libvirt/libvirt_device_pool.py:143

• Major: release_devices() uses allocated_device_groups.pop(device_id) without a default. If the device->IOMMU mapping wasn't recorded (or was already removed), this will raise KeyError and can break environment cleanup/deletion. Make this resilient by using a default and falling back to _get_device_iommu_group() (which provides a clearer LisaException) when the mapping is missing.

            allocated_device_groups = self._allocated_device_groups.get(pool_type, {})
            for device in devices_list:
                device_id = self._get_pci_address_str(device)
                iommu_grp = allocated_device_groups.pop(device_id)
                pool_devices = pool.get(iommu_grp, [])

[Vivek Yadav (vyadavmsft)]

Copilot Fix test issues.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

selftests/test_libvirt_device_pool.py:30

• Minor: _load_device_pool_module() inserts stub modules into sys.modules and never removes them, which can leak state across other selftests in the same process. Track which modules were stubbed and remove them in a finally block after importing the target module to keep the test isolated.

def _load_device_pool_module() -> ModuleType:
    for module_name in ("libvirt", "libvirtaio"):
        try:
            importlib.import_module(module_name)
        except ModuleNotFoundError as identifier:
            if identifier.name != module_name:

lisa/sut_orchestrator/libvirt/libvirt_device_pool.py:143

• Major: release_devices() does allocated_device_groups.pop(device_id) unconditionally. If the mapping is missing (e.g., devices were not allocated via request_devices(), or state was lost), this raises a KeyError and prevents cleanup. Raise a LisaException with actionable context instead of leaking a KeyError.

            allocated_device_groups = self._allocated_device_groups.get(pool_type, {})
            for device in devices_list:
                device_id = self._get_pci_address_str(device)
                iommu_grp = allocated_device_groups.pop(device_id)
                pool_devices = pool.get(iommu_grp, [])