Ip: allow set_mtu to skip asserting the resulting mtu

[mcgov (mcgov)]

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

Some drivers silently clamp or ignore an mtu change, and callers that only wanted a best effort change had no way to avoid the assertion. set_mtu gains assert_success, defaulting to the existing behavior, and logs the mismatch instead of failing when it is disabled.

Key Test Cases:
verify_dpdk_send_receive_multi_txrx_queue_failsafe|verify_dpdk_send_receive_netvsc

Impacted LISA Features:
Sriov, NetworkInterface

Tested Azure Marketplace Images:

• canonical 0001-com-ubuntu-server-jammy 22_04-lts latest

[LiliDeng]

after this change, it won't fail even assert_success is set as true

[Copilot]

Pull request overview

This PR updates the Ip tool’s set_mtu helper to support “best-effort” MTU updates for drivers that clamp/ignore MTU changes, by adding an assert_success flag (defaulting to the current strict behavior).

Changes:

• Extend Ip.set_mtu() with assert_success: bool = True.
• When assert_success is disabled, avoid failing the caller on MTU readback mismatch (intended behavior per PR description).

---

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

Suppressed comments (2)

lisa/tools/ip.py:275

• When assert_success is False, the code currently logs only that the assertion was skipped, but it doesn’t log the actual MTU mismatch. This contradicts the PR description (“logs the mismatch instead of failing”) and makes troubleshooting harder. Consider logging nic name + requested/actual MTU, and reuse that context in the exception message when assert_success is True.

                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")
            else:
                self.node.log.debug(
                    "set_mtu: skipping result assertion since assert_success was False. "
                )

lisa/tools/ip.py:268

• There’s a whitespace-only line after the ip link set call (line 267). This can trip linters and creates noisy diffs; use a real blank line (no trailing spaces).

        self.run(f"link set dev {nic_name} mtu {mtu}", force_run=True, sudo=True)
        
        new_mtu = self.get_mtu(nic_name=nic_name)

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:274

• When assert_success=False, the debug log doesn’t include the interface name or the requested/actual MTU, which makes it hard to diagnose driver clamping/ignoring MTU changes. Also, there’s a whitespace-only line after the ip link set call that can trip linters.

        self.run(f"link set dev {nic_name} mtu {mtu}", force_run=True, sudo=True)
        
        new_mtu = self.get_mtu(nic_name=nic_name)
        if new_mtu != mtu:
            if assert_success:

lisa/tools/ip.py:262

• Nit: mtu_file assignment is missing spaces around =, and the else: block is unnecessary. Keeping this aligned with the surrounding style improves readability and avoids lint noise.

        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))
        else:
            return int(self.get_detail(nic_name, "mtu"))

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:279

• set_mtu(..., assert_success=False) is described as logging the MTU mismatch instead of failing, but the current debug message doesn't include the wanted/got MTU (or nic name), making the log much less actionable. Also, when assert_success=True, the exception message should include the nic and a hint to use assert_success=False for best-effort updates on drivers that clamp MTU.

                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")
            else:
                self.node.log.debug(
                    "set_mtu: skipping result assertion since assert_success was False. "
                )

lisa/tools/ip.py:262

• PEP 8/style: add spaces around the assignment, and avoid the redundant else after an early return to keep the control flow clearer.

        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))
        else:
            return int(self.get_detail(nic_name, "mtu"))

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:279

• When assert_success=False, the code logs a generic message but does not log the actual MTU mismatch (wanted vs observed), even though the PR description states the mismatch should be logged. Also, the failure exception message could include a hint that some drivers clamp/ignore MTU changes and how to proceed.

        new_mtu = self.get_mtu(nic_name=nic_name)
        if new_mtu != mtu:
            if assert_success:
                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")
            else:
                self.node.log.debug(
                    "set_mtu: skipping result assertion since assert_success was False. "
                )

lisa/tools/ip.py:262

• get_mtu() falls back to get_detail() when the sysfs MTU file is missing, but get_detail() calls self.run(..., force_run=False) which is cached by Tool.run_async (command+flags). That can return stale MTU values after set_mtu() and cause false mismatches/failures. Use a forced ip -d link show call (or otherwise bypass the cache) in this fallback path.

    def get_mtu(self, nic_name: str) -> int:
        cat = self.node.tools[Cat]
        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))
        else:
            return int(self.get_detail(nic_name, "mtu"))

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:291

• When assert_success=False and the MTU doesn't match, the log message doesn't include the interface name or the wanted/actual MTU values, which makes troubleshooting harder (especially since the whole point of the flag is to allow silent clamping).

        if new_mtu != mtu:
            if assert_success:
                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")
            else:
                self.node.log.debug(
                    "set_mtu: skipping result assertion since assert_success was False. "
                )

lisa/tools/ip.py:262

• PEP8/style: missing spaces around = in the mtu_file assignment, and the else: is unnecessary after a return. This file generally uses standard spacing and early returns.

    def get_mtu(self, nic_name: str) -> int:
        cat = self.node.tools[Cat]
        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))
        else:
            return int(self.get_detail(nic_name, "mtu"))

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:262

• get_mtu() now uses node.shell.exists() (SFTP-backed) before reading /sys, which can deadlock under concurrent access and adds an extra remote round-trip. Also, returning 0 when MTU can’t be determined can silently break callers (0 isn’t a valid MTU). Prefer trying the sysfs read and falling back to ip -d link show, and raise a LisaException if neither source yields an MTU.

        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))
        else:
            mtu = self.get_detail(nic_name, "mtu")

lisa/tools/ip.py:279

• set_mtu() doesn’t check the exit code from ip link set ... mtu ..., and uses node.log + inconsistent indentation in the non-assert path. This can hide real failures (e.g., permission/invalid MTU) and makes logs harder to correlate. Capture and handle command results: fail fast with a helpful message when assert_success=True, and log/return when assert_success=False; also use the tool logger (self._log) for consistency.

        # check if the device exists
        exists = self.run(f"link show {nic_name}", force_run=True).exit_code == 0
        if not exists:
            if assert_success:
                raise LisaException(f"MTU set failed, could not find interface {nic_name}.")

[Copilot]

Pull request overview

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

Suppressed comments (3)

lisa/tools/ip.py:267

• get_mtu now falls back to get_detail() and returns 0 when /sys/class/net/<nic>/mtu is missing. This changes behavior (silently hides failures) and the fallback is Linux ip-output specific, which won’t work for IpFreebsd (it uses ifconfig). It’s safer to keep get_mtu as a straightforward sysfs read and let failures propagate to callers (while set_mtu(assert_success=False) can handle best-effort semantics).

    def get_mtu(self, nic_name: str) -> int:
        cat = self.node.tools[Cat]
        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))
        else:
            mtu = self.get_detail(nic_name, "mtu")
            if mtu:
                return int(mtu)
            else:
                self.node.log.debug(f"Could not find mtu information for interface {nic_name}")
                return 0

lisa/tools/ip.py:286

• set_mtu introduces a new interface-existence check that duplicates the existing nic_exists() helper and relies on exit_code == 0, which can misclassify other ip failures as “interface not found”. Reuse nic_exists() so the behavior stays consistent (including sudo=True) and avoid duplicated logic.

        # check if the device exists
        exists = self.run(f"link show {nic_name}", force_run=True).exit_code == 0
        if not exists:
            if assert_success:
                raise LisaException(f"MTU set failed, could not find interface {nic_name}.")

lisa/tools/ip.py:288

• The ip link set ... mtu ... command result isn’t checked. With assert_success=False, a command failure (non-zero exit code) will currently be reported as an MTU mismatch warning (or may be missed entirely), and with assert_success=True the raised exception won’t include the underlying ip error output. Capture the command result and enforce expected_exit_code=0 when assertions are enabled; for best-effort mode, log-and-return on a command failure.

        self.run(f"link set dev {nic_name} mtu {mtu}", force_run=True, sudo=True)
        
        new_mtu = self.get_mtu(nic_name=nic_name)
        if new_mtu != mtu:
            if assert_success:
                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:260

• get_mtu now calls node.shell.exists(...), which relies on SFTP and can fail/hang in environments where SFTP is unavailable or problematic. It also changes behavior to return 0 when MTU can't be read, which can silently propagate an invalid MTU to callers (e.g., tests that record/restore MTU). A safer approach is to read /sys/class/net/<nic>/mtu directly (as before), and only fall back to ip -d link show if the sysfs read fails; if both fail, raise a clear exception instead of returning 0.

    def get_mtu(self, nic_name: str) -> int:
        cat = self.node.tools[Cat]
        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))

lisa/tools/ip.py:298

• set_mtu doesn't check the exit code from ip link set ... mtu .... If the command fails (permissions/invalid MTU/driver rejection), the code currently only detects it indirectly via the subsequent MTU read, and the exception/warning doesn't include command stderr. Capturing the result and surfacing stderr makes failures actionable (and keeps the non-asserting path informative without failing the test).

        self.run(f"link set dev {nic_name} mtu {mtu}", force_run=True, sudo=True)

        new_mtu = self.get_mtu(nic_name=nic_name)
        if new_mtu != mtu:
            if assert_success:
                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")
            else:
                 # warn if assertion is turned off.
                 # Weird enough situation to justify log.warning
                self.node.log.warning(
                    f"set_mtu: expected new mtu {mtu}, got {new_mtu} instead. "
                )

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:262

• get_mtu now does an exists() check (SFTP/stat) before reading sysfs, adding an extra remote round-trip and potential SFTP-related issues. Also, returning 0 when MTU can't be determined can silently propagate an invalid MTU value to callers.

Consider reading /sys/class/net/<nic>/mtu directly and falling back to ip -d link show only if the sysfs read fails; if neither source yields an MTU, raise an exception instead of returning 0.

        mtu_file=f"/sys/class/net/{nic_name}/mtu"
        if self.node.shell.exists(self.node.get_pure_path(mtu_file)):
            return int(cat.read(mtu_file, force_run=True))
        else:
            mtu = self.get_detail(nic_name, "mtu")

lisa/tools/ip.py:279

• set_mtu doesn't check the exit code of ip link set ... mtu .... If the command fails (permissions, transient netlink error, etc.) but the current MTU already equals the requested value, the method will incorrectly report success.

It also duplicates nic_exists() with slightly different sudo/behavior, and the mismatch warning/exception doesn't include the interface name, which makes debugging harder.

        # check if the device exists
        exists = self.run(f"link show {nic_name}", force_run=True).exit_code == 0
        if not exists:
            if assert_success:
                raise LisaException(f"MTU set failed, could not find interface {nic_name}.")

[Copilot]

Pull request overview

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

Suppressed comments (3)

lisa/tools/ip.py:269

• get_mtu() now returns 0 when MTU cannot be determined. 0 is not a valid MTU and can be misinterpreted by callers (including set_mtu()), hiding the real failure. The previous behavior would fail loudly if /sys/class/net/.../mtu wasn't readable, so consider keeping that contract by raising an exception when MTU cannot be determined via either mechanism.

                self.node.log.debug(
                    f"Could not find mtu information for interface {nic_name}"
                )
                return 0

lisa/tools/ip.py:289

• set_mtu() added a device-existence probe that duplicates nic_exists() and logs via self.node.log (other Ip methods use self._log). Reusing the existing helper keeps behavior consistent (it already handles cases where ip link show prints "not exist" without relying solely on exit code) and standardizes logging.

        # check if the device exists
        exists = self.run(f"link show {nic_name}", force_run=True).exit_code == 0
        if not exists:
            if assert_success:
                raise LisaException(

lisa/tools/ip.py:297

• With assert_success=True (the default), the previous implementation failed via assert_that(...).is_equal_to(...). The new code raises LisaException instead, which changes the exception type and contradicts the PR description that the default preserves existing behavior. Consider keeping the original assertion path when assert_success is enabled, and only downgrade to a warning when it is disabled.

        new_mtu = self.get_mtu(nic_name=nic_name)
        if new_mtu != mtu:
            if assert_success:
                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")
            else:

[Copilot]

Pull request overview

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

Suppressed comments (2)

lisa/tools/ip.py:269

• Major: get_mtu returns 0 when it can’t determine the MTU, which can silently propagate an invalid MTU value (and later cause confusing failures, e.g. restoring MTU back to 0). It’s safer to raise a LisaException with actionable context when neither sysfs nor ip -d link show provide an MTU.

            else:
                self.node.log.debug(
                    f"Could not find mtu information for interface {nic_name}"
                )
                return 0

lisa/tools/ip.py:296

• Major: set_mtu ignores the exit code/output from ip link set ... mtu .... If the command fails (permissions/invalid MTU/device state), the code can misreport it as an MTU clamp/mismatch. Capture the result and fail (or warn+return when assert_success=False). Also consider skipping the read-back check when MTU can’t be queried and assert_success=False.

        self.run(f"link set dev {nic_name} mtu {mtu}", force_run=True, sudo=True)

        new_mtu = self.get_mtu(nic_name=nic_name)
        if new_mtu != mtu:
            if assert_success:
                raise LisaException(f"set mtu failed, wanted {mtu} and got {new_mtu}")