Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It contains confirmed logic and robustness issues (e.g., an unreachable reboot path in ntttcp and serial-console login/IP-getter edge cases) that can break intended functionality.
Key Test Cases:
verify_serial_console|verify_reboot_in_platform|verify_stop_start_in_platform
Impacted LISA Features:
SerialConsole, StartStop
Tested Azure Marketplace Images:
- canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the baremetal rackmanager orchestrator to better support multi-node environments and serial-console-based workflows (including post-reboot IP discovery), enabling more reliable serial testing and provisioning across multiple nodes.
Changes:
- Add serial-console-based IP discovery for baremetal nodes (both as an IP getter and as a post-reboot IP refresh path).
- Update baremetal cluster/platform handling to support multiple client capabilities/nodes.
- Introduce rackmanager serial console integration and optional ISO-based deploy flow per client.
File summaries
| File | Description |
|---|---|
| lisa/tools/reboot.py | Refresh SSH address from serial console for baremetal reboot flows before waiting for SSH stability. |
| lisa/tools/ntttcp.py | Add TCP perf result logging and adjust TasksMax reboot behavior logic. |
| lisa/sut_orchestrator/baremetal/schema.py | Add iso_name to rackmanager client schema to support ISO-based operations. |
| lisa/sut_orchestrator/baremetal/platform_.py | Support multi-node baremetal deployments by mapping requirements to multiple client capabilities and setting per-node connection info. |
| lisa/sut_orchestrator/baremetal/ip_getter.py | Add a serial-console-based IP getter implementation. |
| lisa/sut_orchestrator/baremetal/cluster/rackmanager.py | Implement rackmanager-backed SerialConsole and ISO mount/reset orchestration across multiple clients. |
| lisa/sut_orchestrator/baremetal/cluster/cluster.py | Populate a list of client capabilities (clients) to support multi-node capability checks. |
| lisa/sut_orchestrator/baremetal/build.py | Lazy-import SMB dependency and raise a clearer error when pysmb is missing. |
| lisa/mixin_modules.py | Make idrac module import optional/isolated so missing dependencies don’t block other baremetal modules. |
| lisa/microsoft/testsuites/ltp/ltp.py | Add gawk dependency for LTP on affected distros. |
| lisa/features/serial_console.py | Add a wait_for_ready() hook for serial console implementations. |
Review details
Suppressed comments (4)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:128
wait_output("Password:")is case-sensitive; some distros/targets emit a lowercasepassword:prompt, which would make serial login fail even though_get_prompt_statewould recognize it.
password_found = self._process.wait_output(
"Password:",
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:147
- After sending credentials,
_loginonly checks that the session didn't immediately return to a login prompt; it doesn't verify that a shell prompt was reached. That can causewrite()to start sending commands while still at a login/password prompt.
deadline = time.time() + SERIAL_LOGIN_RETRY_TIMEOUT
while time.time() < deadline:
output = self._get_console_log(saved_path=None).decode(
"utf-8", errors="ignore"
)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:232
management_portis treated as a truthy value, but the schema default is-1(truthy). This assert will pass for-1and then send invalid commands (and it will incorrectly fail for0). Validate it is set and non-negative instead.
management_port = client.management_port
assert (
management_port
), "management_port is required when rackmanager iso_name is set"
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:214
iso_nameis interpolated directly into a command string sent to the rack manager node. Ifiso_namecontains spaces or shell metacharacters, the command can break or be abused. Quote/escape it before embedding (e.g., viashlex.quote).
def _mount_remote_drive(self, management_port: int, iso_name: str) -> None:
self.rm_node.execute(
"set system remotedrive mount -b 0 -m 2 "
f"-i {management_port} -n {iso_name}"
).assert_exit_code()
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| while time.time() < deadline: | ||
| state = self._get_prompt_state(output_offset) | ||
| if state in ("shell", "login", "password"): | ||
| return state | ||
| return self._get_prompt_state(output_offset) |
| def get_ip_from_node(self, node: Node) -> str: | ||
| serial_console = node.features[SerialConsole] | ||
| deadline = time.time() + self.serial_runbook.timeout | ||
| serial_console.wait_for_ready(self.serial_runbook.timeout) | ||
| previous_output = serial_console.get_console_log(force_run=True) |
| "no config file exist for systemd, either there is no systemd" | ||
| " service or the config file location is incorrect." | ||
| ) | ||
| need_reboot = False |
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT license. | ||
|
|
||
| import re |
ddc8f80 to
fc23a6b
Compare
AI Test Case SelectionSelected 2 test case(s): verify_ltp_lite,verify_ltp_full Marketplace image: Result: Canceled |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness/robustness issues in the new rackmanager and serial IP getter flows that can cause valid configurations to fail or timeouts to be misapplied.
Key Test Cases:
verify_reboot_in_platform|verify_stop_start_in_platform|smoke_test
Impacted LISA Features:
SerialConsole, StartStop
Tested Azure Marketplace Images:
- canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
- redhat rhel 9_5 latest
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lisa/sut_orchestrator/baremetal/ip_getter.py:165
deadlineis computed beforewait_for_ready(). Ifwait_for_ready()takes significant time, the remaining time budget for IP detection can be much shorter thantimeout(or even already expired). Also, accessingnode.features[SerialConsole]without checking support will raise a KeyError; it’s better to raise a clear LisaException instead.
def get_ip_from_node(self, node: Node) -> str:
serial_console = node.features[SerialConsole]
deadline = time.time() + self.serial_runbook.timeout
serial_console.wait_for_ready(self.serial_runbook.timeout)
previous_output = serial_console.get_console_log(force_run=True)
while time.time() < deadline:
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
| deadline = time.time() + SERIAL_LOGIN_RETRY_TIMEOUT | ||
| while time.time() < deadline: | ||
| output = self._get_console_log(saved_path=None).decode( | ||
| "utf-8", errors="ignore" | ||
| ) | ||
| if re.search(r"(?im)^.*login:\s*$", output[output_offset:]): | ||
| raise LisaException( | ||
| "serial console login failed and returned to login prompt" | ||
| ) | ||
| time.sleep(0.5) | ||
|
|
| management_port = client.management_port | ||
| assert ( | ||
| management_port | ||
| ), "management_port is required when rackmanager iso_name is set" | ||
| self._set_boot_from_remote_drive(management_port) |
AI Test Case SelectionSelected 2 test case(s): verify_ltp_lite,verify_ltp_full Marketplace image: Result: Canceled |
There was a problem hiding this comment.
🔵 Needs a closer look
There is at least one correctness issue in the rackmanager ISO flow (management_port validation) and several newly introduced bare-sleep / broad-exception patterns that should be tightened for reliability and diagnosability.
Key Test Cases:
verify_serial_console|verify_reboot_in_platform|verify_nvme_basic
Impacted LISA Features:
SerialConsole, StartStop, Nvme
Tested Azure Marketplace Images:
- canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
- redhat rhel 9_5 latest
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
lisa/tools/reboot.py:199
- Catching
Exceptionhere can hide unexpected failures (e.g. programming errors) and make serial/IP refresh silently stop working. It would be safer to catch the specific exceptions expected from serial console I/O (and re-raise others), or at least log the traceback (exc_info) so failures are diagnosable.
This issue also appears on line 199 of the same file.
lisa/sut_orchestrator/baremetal/ip_getter.py:185
- This polling loop uses
time.sleep(1)while waiting for serial console output. Repository guidance prefers bounded wait helpers over manual sleeps; consider rewriting this as a bounded wait using shared helpers (e.g. check_till_timeout) so timeouts and intervals are handled consistently.
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:233
- The management_port check uses truthiness (
assert management_port), which will incorrectly allow invalid values like -1 (the schema default) and may send rackmanager commands against an invalid port when iso_name is set. Validate it explicitly (not None and >= 0) and make the assertion message reflect the invariant.
management_port = client.management_port
assert (
management_port
), "management_port is required when rackmanager iso_name is set"
self._set_boot_from_remote_drive(management_port)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:152
- This loop uses
time.sleep(0.5)to poll for a failed login prompt. The repository guidance prefers bounded waits over bare sleeps; here you can use the existingwait_outputAPI to block until "login:" appears (or timeout) without manual sleeping.
if re.search(r"(?im)^.*login:\s*$", output[output_offset:]):
raise LisaException(
"serial console login failed and returned to login prompt"
)
time.sleep(0.5)
lisa/tools/reboot.py:201
- This new wait loop uses a bare
sleep(2)between serial-console polls. The repo guidance discourages sleeps in favor of bounded wait helpers (e.g. check_till_timeout / retry) so polling behavior is consistent and less flaky; consider refactoring this loop to use the shared wait utilities.
self._log.debug(f"ignorable serial IP refresh exception: {e}")
sleep(2)
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
AI Test Case SelectionSelected 2 test case(s): verify_ltp_lite,verify_ltp_full Marketplace image: Result: Canceled |
There was a problem hiding this comment.
🔵 Needs a closer look
There are correctness and reliability issues in the new RackManager serial console/login and deploy paths that can break existing baremetal flows or cause flaky serial interactions.
Key Test Cases:
verify_serial_console|verify_reboot_in_platform|verify_stop_start_in_platform
Impacted LISA Features:
SerialConsole, StartStop, Nvme
Tested Azure Marketplace Images:
- canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
- redhat rhel 9_5 latest
Review details
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:133
- The serial login waits specifically for the literal string "Password:", but prompt capitalization varies (e.g. "password:"). This can cause false negatives and fail login even when the password prompt is present.
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:226 - RackManager.deploy() previously power-cycled clients during environment deployment. With the new early return when no iso_name is set, deploy becomes a no-op, which can break existing rackmanager flows that rely on deploy to reset/boot nodes.
This issue also appears on line 237 of the same file.
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:98
- _wait_for_prompt_state() loops without any throttling when the prompt state is unknown, which can cause a hot spin (high CPU) because _get_prompt_state() doesn't block. Add a small sleep in the polling loop to avoid busy-waiting while waiting for serial output.
deadline = time.time() + timeout
while time.time() < deadline:
state = self._get_prompt_state(output_offset)
if state in ("shell", "login", "password"):
return state
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:147
- After sending the password, _login() only checks for a return to the login prompt and then returns without confirming a shell prompt was reached. This can allow callers to write commands before login completes (or after a failed password), making serial interactions flaky.
deadline = time.time() + SERIAL_LOGIN_RETRY_TIMEOUT
while time.time() < deadline:
output = self._get_console_log(saved_path=None).decode(
"utf-8", errors="ignore"
)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:238
- reset() currently asserts management_port as a truthy value, so a valid port/index of 0 would fail, and it's inconsistent with the >= 0 checks used elsewhere in this file. Use an explicit None/negative check for correctness and consistency.
def reset(self, operation: str) -> None:
self.connect_to_rack_manager()
lisa/sut_orchestrator/baremetal/ip_getter.py:164
- SerialChecker.get_ip_from_node() assumes SerialConsole is supported and will fail with a less actionable error if it's not configured. Add an explicit support check and raise a clear LisaException so misconfigured runbooks are easier to diagnose.
def get_ip_from_node(self, node: Node) -> str:
serial_console = node.features[SerialConsole]
deadline = time.time() + self.serial_runbook.timeout
serial_console.wait_for_ready(self.serial_runbook.timeout)
previous_output = serial_console.get_console_log(force_run=True)
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
AI Test Case SelectionSelected 2 test case(s): verify_ltp_lite,verify_ltp_full Marketplace image: Result: Canceled |
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed logic issues in the new RackManager/serial IP paths (timeout accounting and management_port validation) that can cause incorrect behavior or unexpected failures.
Key Test Cases:
verify_serial_console|verify_reboot_in_platform
Impacted LISA Features:
SerialConsole, StartStop, Disk, Nvme
Tested Azure Marketplace Images:
- canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
- redhat rhel 9_5 latest
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
lisa/sut_orchestrator/baremetal/ip_getter.py:165
- The overall timeout window is started before wait_for_ready(), so any time spent waiting for serial readiness reduces (or eliminates) the time available to actually fetch and parse the IP, causing premature timeouts. Start the deadline after wait_for_ready() completes.
lisa/tools/reboot.py:199 - The serial IP refresh loop catches all exceptions but logs only the message, losing the traceback that’s often needed to diagnose why serial parsing/commands failed. Log with exc_info (or narrow the exception types) so failures are debuggable without changing control flow.
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:99
- The prompt-wait loop is a tight busy-spin with no sleep, which can consume a full CPU core for up to the timeout duration. Add a small sleep/backoff inside the loop to reduce CPU usage while waiting for serial output.
while time.time() < deadline:
state = self._get_prompt_state(output_offset)
if state in ("shell", "login", "password"):
return state
return self._get_prompt_state(output_offset)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:233
- The management_port validation uses truthiness, so a valid port 0 will fail, while the default -1 will incorrectly pass. Validate explicitly for non-None and >= 0 before using it.
management_port = client.management_port
assert (
management_port
), "management_port is required when rackmanager iso_name is set"
self._set_boot_from_remote_drive(management_port)
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
AI Test Case SelectionSelected 2 test case(s): verify_ltp_lite,verify_ltp_full Marketplace image: Result: Canceled |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in Disk capability exposure, Rack Manager session/configuration handling, and reboot/IP refresh behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (16)
Previously missed (4) — in code that hasn't changed since the last review.
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:52
- Major:
close()is not enough to release this resource._initialize()starts a long-livedstart serial sessionprocess on the rack manager, but environment/node cleanup does not invoke featureclose(), andRackManager.cleanup()remains inherited as a no-op. Each environment can therefore leave a serial session behind and eventually exhaust rack-manager sessions; track the consoles and close them during cluster cleanup and deployment failure.
lisa/tools/reboot.py:277 - Major:
_is_baremetal_node()also matches the iDRAC cluster, whoseIdracSerialConsoleis read-only and inheritsSerialConsole.write()(which raisesNotImplementedError)._refresh_address_from_serial()catches that exception and retries until its 180-second timeout before waiting for SSH. Gate this refresh on a command-capable serial console or on the rackmanager implementation.
lisa/tools/reboot.py:280 - The bare-metal branch returns after two successful SSH probes without verifying that the boot time advanced. Since the reboot command exceptions are intentionally swallowed above, a failed or no-op reboot with an already reachable SSH session is reported as successful; retain the existing boot-time validation before returning.
lisa/tools/reboot.py:201 - Minor: Catching every
Exceptionhere masks programming/configuration failures, including errors from address updates, and converts them into a long IP-refresh timeout while retaining a stale address. Catch only expected transient serial/connection errors and propagate invalid output or configuration errors.
lisa/sut_orchestrator/baremetal/cluster/cluster.py:99
- Major: Every rackmanager client is now given an
NvmeSettings()capability without detecting whether the machine has an NVMe device. Tests such asverify_nvme_basicrequire only[Nvme]and then assert that devices exist, so non-NVMe rackmanager hosts will be selected and fail instead of being skipped. Populatedisk_countfrom capability detection or advertiseNvmeonly when the hardware is known to support it.
features.NvmeSettings(),
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:251
- Major: The schema default is
management_port=-1, but this truthiness check accepts-1wheniso_nameis set. Deployment will then send-i -1to the Rack Manager commands instead of rejecting the invalid configuration; serial-console initialization already requires a non-negative value.
assert (
management_port
), "management_port is required when rackmanager iso_name is set"
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:166
- Major: After sending the password, this loop waits a fixed three seconds but never verifies that a shell prompt appeared. On a slow login it returns while the session is still at a password/unknown prompt, and the caller then sends the IP command to the wrong state. Wait for a shell prompt and raise if it is not reached before the timeout.
deadline = time.time() + SERIAL_LOGIN_RETRY_TIMEOUT
while time.time() < deadline:
output = self._get_console_log(saved_path=None).decode(
"utf-8", errors="ignore"
)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:244
- When no client has an
iso_name, this new early return removes the previousdeploy()behavior of powering every configured client off and on. Existing rackmanager runbooks without ISO deployment will now skip that reset and can leave clients in their pre-deploy state; preserve the reset path for this case.
if not iso_clients:
return
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:99
- Because
_get_console_logcallswait_outputwith an empty keyword, it returns immediately, so this loop polls as fast as possible while waiting for a prompt. With the 300-second default, every serial session can consume a CPU core during prompt discovery, and multi-node runs multiply that load. Poll with a bounded interval or the repository's wait helper.
def _wait_for_prompt_state(self, timeout: int, output_offset: int = 0) -> str:
deadline = time.time() + timeout
while time.time() < deadline:
state = self._get_prompt_state(output_offset)
if state in ("shell", "login", "password"):
return state
return self._get_prompt_state(output_offset)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:171
- This new login polling path uses a raw
time.sleep(0.5). Use the repository's bounded wait/retry mechanism instead so the timeout is managed consistently and the serial login remains responsive under multi-node execution.
time.sleep(0.5)
lisa/sut_orchestrator/baremetal/cluster/rackmanager.py:154
- This exception does not explain whether the target was still booting, whether credentials were rejected, or where to investigate. Include the login-stage context and a concrete next step such as inspecting the serial log and verifying rack-manager credentials.
raise LisaException("serial console password prompt was not found")
lisa/sut_orchestrator/baremetal/ip_getter.py:164
- The timeout deadline is started before
wait_for_ready(). A rack-manager login can consume the full configured timeout, leaving no polling window and causing this method to fail immediately even though the serial console became ready. Start the IP-query deadline after readiness (or reserve a separate budget for the query).
deadline = time.time() + self.serial_runbook.timeout
serial_console.wait_for_ready(self.serial_runbook.timeout)
previous_output = serial_console.get_console_log(force_run=True)
lisa/sut_orchestrator/baremetal/ip_getter.py:185
- This new polling path uses a raw
time.sleep(1). The repository's bounded wait/retry helpers should be used instead so timeout handling and interruption remain consistent with test execution, especially when several bare-metal nodes are being prepared.
time.sleep(1)
lisa/sut_orchestrator/baremetal/platform_.py:110
- The new multi-node capability matching, per-client deployment, and serial IP-refresh paths are not covered by the added tests; the file only exercises serial-process exit handling. Add focused coverage for mapping two clients to two nodes and for per-client ISO/serial-console behavior so the feature's central path is not untested.
if len(environment.runbook.nodes_requirement) > len(self.cluster.clients):
return False
# Convert test requirements to platform-specific feature types
if environment.runbook.nodes_requirement:
for node_requirement in environment.runbook.nodes_requirement:
convert_to_baremetal_node_space(node_requirement)
return self._check_capability(environment, log, self.cluster.clients)
lisa/sut_orchestrator/baremetal/platform_.py:160
- These new runtime checks use a bare
assert, which is removed when Python runs with optimizations; an invalid node can then reachset_connection_infoand fail with a less useful attribute error. Use an explicit exception (or the repository's assertion helper) for validation that must always execute.
assert isinstance(
remote_node, RemoteNode
), f"expected RemoteNode, got {type(remote_node).__name__}"
selftests/test_rackmanager.py:1
- Please add a description explaining what this PR does and why.
# Copyright (c) Microsoft Corporation.
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
| class Disk(features.Disk): | ||
| """Disk discovery for pre-provisioned baremetal storage. | ||
|
|
||
| Baremetal disks are not dynamically managed by LISA. The base disk | ||
| implementation is sufficient for discovering the boot disk and allowing | ||
| NVMe to exclude it from performance targets. | ||
| """ | ||
|
|
||
| pass |
| @classmethod | ||
| def supported_features(cls) -> List[Type[feature.Feature]]: | ||
| return [StartStop, SerialConsole, SecurityProfile] | ||
| return [StartStop, SerialConsole, SecurityProfile, Disk, Nvme] |
Description
Related Issue
Type of Change
Checklist
Test Validation
Key Test Cases:
Impacted LISA Features:
Tested Azure Marketplace Images:
Test Results