Dpdk: roll back broken installs and reuse downloaded assets

[mcgov (mcgov)]

Part 7 of 9 of a stacked series that reworks the DPDK SRIOV hot plug tests. Stacked on #4659, review only the last commit.

• A failure part way through a source installation left the node with a half installed dpdk or rdma-core, and every later test on that node failed for an unrelated reason. The install steps are now wrapped so a failure uninstalls what was applied, removes the extracted source, marks the node dirty if even the cleanup fails, and re-raises the original error.
• The asset removal guards that were specific to the rdma-core installer now live on the base Installer as _delete_assets, so every installer is protected against deleting / or the working path.
• Downloads and extraction are skipped when the asset is already on the node, and dpdk-stable is fetched from the github mirror, which is far more reliable than dpdk.org from Azure.

Key Test Cases:
verify_dpdk_build_netvsc|verify_dpdk_build_failsafe|verify_dpdk_build_gb_hugepages_netvsc

Impacted LISA Features:
Sriov, NetworkInterface, Infiniband

Tested Azure Marketplace Images:

• canonical 0001-com-ubuntu-server-jammy 22_04-lts latest
• microsoftcblmariner azure-linux-3 azure-linux-3 latest
• redhat rhel 9_5 latest

[Copilot]

Pull request overview

This PR improves resiliency and reusability of the DPDK/rdma-core source installation flow used by the DPDK SRIOV hot-plug tests, aiming to prevent nodes from being left in a broken “half-installed” state and to reduce redundant downloads/extractions.

Changes:

• Bump rdma-core default source tarball to v59.0 and centralize asset-delete safety checks into the base Installer.
• Add rollback logic in the base Installer to attempt cleanup after installation failures and mark nodes dirty when cleanup fails.
• Reuse already-present downloaded/extracted assets and switch dpdk-stable fetch to the GitHub mirror for reliability.

Reviewed changes

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

File	Description
lisa/microsoft/testsuites/dpdk/rdmacore.py	Updates rdma-core source URL and removes per-installer asset cleanup in favor of base installer handling.
lisa/microsoft/testsuites/dpdk/common.py	Adds shared asset deletion guards + rollback flow, skips redundant download/extract work, and updates the dpdk-stable repo URL.

Suppressed comments (1)

lisa/microsoft/testsuites/dpdk/common.py:283

• Major: do_installation() uses raise e, which resets the original traceback to this handler. Use a bare raise after rollback so failures point to the actual install step that failed.

            except Exception as e:
                self._rollback_installation()
                raise e

---

💡 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 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/common.py:248

• _rollback_installation() re-raises the cleanup exception, which can mask the original installation failure. This contradicts the PR description (mark node dirty if cleanup fails, but re-raise the original error). Rollback should best-effort cleanup, mark the node dirty on rollback failures, and not raise.

    def _rollback_installation(self) -> None:
        try:
            if self._check_if_installed():
                self._uninstall()
            self._delete_assets()
        except Exception as err:
            self._node.log.debug(
                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            raise err

lisa/microsoft/testsuites/dpdk/common.py:192

• _setup_node() only checks hasattr(self, "asset_path"), which can be true even when the directory was deleted on the node (or never created successfully). That can cause later install/uninstall steps to run with a missing asset_path. Prefer checking both attribute presence and remote path existence via _asset_path_exists().

    def _setup_node(self) -> None:
        if not hasattr(self, "asset_path"):
            self._download_assets()

lisa/microsoft/testsuites/dpdk/common.py:283

• In do_installation(), raise e resets the traceback, which makes the original failure harder to debug. Use a bare raise to preserve the original traceback (especially important here since you’re deliberately catching only to rollback).

            try:
                self._download_assets()
                self._uninstall()
                self._install_dependencies()
                self._install()
            except Exception as e:
                self._rollback_installation()
                raise e

[Copilot]

Pull request overview

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

Suppressed comments (4)

lisa/microsoft/testsuites/dpdk/common.py:247

• _rollback_installation currently raise err on cleanup failures, which can override the original install exception from do_installation. Since this method is used as a best-effort rollback, it should mark the node dirty and return without raising (or at least preserve traceback with bare raise).

            self._node.log.debug(
                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            raise err

lisa/microsoft/testsuites/dpdk/common.py:236

• _delete_assets executes rm -rf {asset_path} via the shell without quoting. Even with the guard assertions, using node.shell.remove() avoids shell-escaping issues and reduces the chance of path-injection bugs.

            f"Test bug: Installer source path {asset_path} was set to working path "
            f"'{working_path}' during attempted cleanup!"
        ).is_not_equal_to(working_path)
        self._node.execute(f"rm -rf {str(asset_path)}", shell=True)

lisa/microsoft/testsuites/dpdk/common.py:283

• do_installation re-raises with raise e, which drops the original traceback, and a failure inside _rollback_installation() can mask the original install failure (contradicting the PR description that the original error is re-raised). Wrapping _setup_node()/install steps in a single try/except and using bare raise preserves the original exception while still doing best-effort rollback.

            except Exception as e:
                self._rollback_installation()
                raise e

lisa/microsoft/testsuites/dpdk/common.py:177

• Skipping extraction based only on asset_path existence can leave a partially-extracted source tree (e.g., if a prior run failed mid-extract), and subsequent runs will silently reuse the incomplete directory. Consider always running Tar.extract(..., skip_existing_files=True) to ensure missing files are populated.

        if not node.shell.exists(self.asset_path):
            node.tools[Tar].extract(
                file=str(remote_path),
                dest_dir=str(work_path),
                gzip=True,

[Copilot]

Pull request overview

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

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/common.py:283

• If an install step fails, _rollback_installation() can raise and mask the original failure, and raise e drops the original traceback. This contradicts the PR description (re-raise original error) and makes debugging harder. Wrap rollback in its own try/except and use bare raise to preserve the original exception/traceback.

            try:
                self._download_assets()
                self._uninstall()
                self._install_dependencies()
                self._install()
            except Exception as e:
                self._rollback_installation()
                raise e

lisa/microsoft/testsuites/dpdk/common.py:247

• _rollback_installation() currently raises its own cleanup exception (raise err), which can override the original installation failure (and also resets the cleanup traceback). Since callers already handle the original error, rollback should mark the node dirty + log and then return without raising.

    def _rollback_installation(self) -> None:
        try:
            if self._check_if_installed():
                self._uninstall()
            self._delete_assets()
        except Exception as err:
            self._node.log.debug(
                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            raise err

lisa/microsoft/testsuites/dpdk/common.py:235

• rm -rf is executed via shell=True with an unquoted path. Even with the / and working-path guards, this is still brittle (spaces/shell metacharacters) and is a command-injection footgun if asset_path ever comes from variables. Prefer the built-in shell file API to remove the directory safely.

            f"Test bug: Installer source path {asset_path} was set to working path "
            f"'{working_path}' during attempted cleanup!"
        ).is_not_equal_to(working_path)
        self._node.execute(f"rm -rf {str(asset_path)}", shell=True)

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/microsoft/testsuites/dpdk/common.py:242

• _rollback_installation() currently (1) skips _uninstall() if _check_if_installed() is false (which can miss partially-applied installs), and (2) uses raise err, which loses traceback context. Rollback should be best-effort (attempt uninstall even if detection fails) and should re-raise with bare raise so debugging context is preserved. (The caller can decide whether rollback failures should mask the original install error.)

        try:
            if self._check_if_installed():
                self._uninstall()
            self._delete_assets()
        except Exception as err:

lisa/microsoft/testsuites/dpdk/common.py:283

• do_installation re-raises with raise e, which drops the original traceback, and it also lets _rollback_installation() failures replace the original install error. This contradicts the PR description (“re-raises the original error”) and makes failures harder to diagnose. Preserve the original exception with bare raise, and ensure rollback errors don’t mask it (rollback can still mark the node dirty).

            except Exception as e:
                self._rollback_installation()
                raise e

[Copilot]

Pull request overview

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

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/common.py:223

• _delete_assets() deletes the asset_path attribute before validating/removing the path. If the safety assertions or shell.remove() fail, the attribute is already gone, which makes retries/debugging harder and can leave the instance in an inconsistent state. Delete the attribute only after a successful remove.

        asset_path = self.asset_path
        delattr(self, "asset_path")
        working_path = str(self._node.get_working_path())

lisa/microsoft/testsuites/dpdk/common.py:284

• In do_installation(), raise e inside the except block drops the original traceback context. Use a bare raise after rollback so failures remain diagnosable.

            except Exception as e:
                self._rollback_installation()
                raise e

lisa/microsoft/testsuites/dpdk/common.py:248

• _rollback_installation() raises AssertionError(err, "..."), which produces a tuple-style AssertionError and loses proper exception chaining. It also logs an empty debug line. Raise a single-message AssertionError and chain the original exception with from err.

            self._node.log.debug("")
            raise AssertionError(err, "Test bug: rollback of installation failed")

[Copilot]

Pull request overview

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

Suppressed comments (1)

lisa/microsoft/testsuites/dpdk/common.py:248

• _rollback_installation raises a new AssertionError if cleanup fails, which can mask the original installation error. The PR description says cleanup failure should mark the node dirty and then re-raise the original error, so rollback should be best-effort and not throw.

        except Exception as err:
            self._node.log.debug(
                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            self._node.log.debug("")
            raise AssertionError(err, "Test bug: rollback of installation failed")

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/microsoft/testsuites/dpdk/common.py:247

• _rollback_installation() currently re-raises cleanup failures (raise err). In the install failure path this can mask the original install error, contradicting the intent to roll back and then re-raise the original failure. Also, raise err discards the original traceback for the cleanup error itself.

        except Exception as err:
            self._node.log.debug(
                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            raise err

lisa/microsoft/testsuites/dpdk/common.py:283

• In do_installation(), raise e resets the traceback and can also get replaced by any exception thrown from _rollback_installation(). After a rollback attempt, re-raise the original exception with a bare raise to preserve the original stack trace.

            except Exception as e:
                self._rollback_installation()
                raise e

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/microsoft/testsuites/dpdk/common.py:223

• _delete_assets deletes asset_path via delattr before the safety assertions and the actual removal. If an assertion/removal fails, the object loses the only reference to the path, making subsequent cleanup/diagnostics harder (and _rollback_installation won’t be able to retry deletion in the same process). Move delattr to after a successful remove() (or keep the attribute and rely on _asset_path_exists()).

        asset_path = self.asset_path
        delattr(self, "asset_path")
        working_path = str(self._node.get_working_path())

lisa/microsoft/testsuites/dpdk/common.py:284

• In do_installation, raise e re-raises the exception with a new traceback, making the original failure location harder to debug. After calling _rollback_installation(), use a bare raise to preserve the original traceback.

            except Exception as e:
                self._rollback_installation()
                raise e

[Copilot]

Pull request overview

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

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/common.py:281

• In do_installation(), _download_assets() is called again inside the install try even though _setup_node() already downloads assets; this defeats the "reuse downloaded assets" behavior and can re-run git clone/download attempts unnecessarily. Also, raise e loses the original traceback; use a bare raise after rollback to preserve debugging context.

            try:
                self._download_assets()
                self._uninstall()
                self._install_dependencies()
                self._install()

lisa/microsoft/testsuites/dpdk/common.py:192

• _setup_node() only checks hasattr(self, "asset_path"), which can be true even when the directory was removed on the node (or never successfully created). This can cause later steps to run with a stale/missing asset directory. Use _asset_path_exists() so the decision is based on both the attribute and filesystem state.

    def _setup_node(self) -> None:
        if not hasattr(self, "asset_path"):
            self._download_assets()

lisa/microsoft/testsuites/dpdk/common.py:248

• _rollback_installation() raises AssertionError(err, ...), which produces a tuple-like assertion message and drops exception chaining. Prefer raise AssertionError("...") from err so the rollback failure message is clean and the original cleanup exception is preserved as the cause. The extra debug("") line also adds noise.

                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            self._node.log.debug("")
            raise AssertionError(err, "Test bug: rollback of installation failed")

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/microsoft/testsuites/dpdk/common.py:284

• raise e after rollback drops the original traceback, which makes install failures much harder to diagnose. Use a bare raise to preserve the original exception context/stack trace.

            except Exception as e:
                self._rollback_installation()
                raise e

lisa/microsoft/testsuites/dpdk/common.py:248

• The rollback failure path currently does raise AssertionError(err, ...) (which creates a tuple-style message) and also logs an empty debug line. Prefer a clear AssertionError message and chain the underlying exception via from err so the root cause is retained.

            self._node.log.debug("")
            raise AssertionError(err, "Test bug: rollback of installation failed")

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/microsoft/testsuites/dpdk/common.py:248

• _rollback_installation() currently raises a new AssertionError on cleanup failure, which can mask the original install exception and contradicts the PR description of marking the node dirty and re-raising the original error. It also logs an empty debug line that adds no signal.

                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            self._node.log.debug("")
            raise AssertionError(err, "Test bug: rollback of installation failed")

lisa/microsoft/testsuites/dpdk/common.py:284

• In do_installation(), raise e re-raises the exception with a new traceback, which makes diagnosing transient install failures harder. Use a bare raise after rollback so the original traceback is preserved (and the original error is what propagates).

            except Exception as e:
                self._rollback_installation()
                raise e

[Copilot]

Pull request overview

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

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/common.py:285

• Re-raising with raise e resets the traceback. Use bare raise so failures in install/uninstall keep their original stack, which is important for diagnosing transient install problems.

            try:
                self._download_assets()
                self._uninstall()
                self._install_dependencies()
                self._install()
            except Exception as e:
                self._rollback_installation()
                raise e

lisa/microsoft/testsuites/dpdk/common.py:154

• Wget.get() doesn’t accept a skip_exists parameter (see lisa/base_tools/wget.py). This will raise a TypeError at runtime and prevent tarball downloads.

        if self._is_remote_tarball:
            tarfile = node.tools[Wget].get(
                self._tar_url,
                overwrite=False,
                file_path=str(work_path),
                skip_exists=True,
            )

lisa/microsoft/testsuites/dpdk/common.py:249

• PR description says a failed install should attempt cleanup, mark the node dirty if cleanup fails, and then re-raise the original install error. Currently _rollback_installation() raises an AssertionError on cleanup failures, which will mask the original exception from do_installation(). Consider logging + marking dirty, but not raising from rollback so the original error can be re-raised.

    def _rollback_installation(self) -> None:
        try:
            if self._check_if_installed():
                self._uninstall()
            self._delete_assets()
        except Exception as err:
            self._node.log.debug(
                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            self._node.log.debug("")
            raise AssertionError(err, "Test bug: rollback of installation failed")

[Copilot]

Pull request overview

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

Suppressed comments (3)

lisa/microsoft/testsuites/dpdk/common.py:154

• Major: Wget.get() doesn’t accept a skip_exists keyword argument (see lisa/base_tools/wget.py), so this call will raise TypeError at runtime. overwrite=False already provides the “skip download if present” behavior.

            tarfile = node.tools[Wget].get(
                self._tar_url,
                overwrite=False,
                file_path=str(work_path),
                skip_exists=True,
            )

lisa/microsoft/testsuites/dpdk/common.py:248

• Major: _rollback_installation() raises a new AssertionError (and even passes multiple args) which can mask the original installation failure. The PR description says rollback failures should mark the node dirty and still re-raise the original error; rollback should not throw its own exception here.

                f"Installer cleanup failed; marking node dirty. {str(err)}"
            )
            self._node.mark_dirty()
            self._node.log.debug("")
            raise AssertionError(err, "Test bug: rollback of installation failed")

lisa/microsoft/testsuites/dpdk/common.py:284

• Minor: raise e here will lose the original traceback context. Use a bare raise so the failure points to the real root cause.

            except Exception as e:
                self._rollback_installation()
                raise e

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/microsoft/testsuites/dpdk/common.py:282

• Minor: do_installation() unconditionally calls _download_assets() inside the install try-block (even if assets already exist) and binds the exception to e but never uses it. This undermines the asset-reuse goal and introduces an unused variable.

                self._download_assets()
                self._uninstall()
                self._install_dependencies()
                self._install()
            except Exception as e:

lisa/microsoft/testsuites/dpdk/common.py:248

• Major: _rollback_installation() raises a new AssertionError when cleanup fails, which can mask the original installation failure. The PR description says the installer should attempt rollback, mark the node dirty if cleanup fails, and re-raise the original error.

            raise AssertionError(
                f"Test bug: rollback of installation failed: {str(err)}"
            )

[Copilot]

Pull request overview

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

Suppressed comments (2)

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

lisa/microsoft/testsuites/dpdk/common.py:176

• Major: Skipping extraction when the target directory already exists can leave the node using a partially-extracted source tree (e.g., if a previous run failed mid-extract). Previously, always extracting with skip_existing_files allowed missing files to be restored idempotently. Consider always running Tar.extract (with skip_existing_files) so an incomplete directory is repaired.

        if not node.shell.exists(self.asset_path):
            node.tools[Tar].extract(
                file=str(remote_path),
                dest_dir=str(work_path),
                gzip=True,

lisa/microsoft/testsuites/dpdk/common.py:248

• Major: _rollback_installation() can skip _uninstall() entirely if _check_if_installed() returns False for a half-installed state, leaving partial installs behind. Also, raising a new AssertionError from rollback can mask the original installation failure, which contradicts the PR description’s intent to re-raise the original error while marking the node dirty if cleanup fails. Make rollback best-effort: always attempt _uninstall() and _delete_assets(), mark the node dirty on cleanup errors, but don’t raise a new exception.

    def _rollback_installation(self) -> None:
        try:
            if self._check_if_installed():
                self._uninstall()
            self._delete_assets()