You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat: support multi-node rackmanager serial testing - #4706
The reason will be displayed to describe this comment to others. Learn more.
🟡 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
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.
wait_output("Password:") is case-sensitive; some distros/targets emit a lowercase password: prompt, which would make serial login fail even though _get_prompt_state would recognize it.
After sending credentials, _login only checks that the session didn't immediately return to a login prompt; it doesn't verify that a shell prompt was reached. That can cause write() to start sending commands while still at a login/password prompt.
management_port is treated as a truthy value, but the schema default is -1 (truthy). This assert will pass for -1 and then send invalid commands (and it will incorrectly fail for 0). 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"
iso_name is interpolated directly into a command string sent to the rack manager node. If iso_name contains spaces or shell metacharacters, the command can break or be abused. Quote/escape it before embedding (e.g., via shlex.quote).
The reason will be displayed to describe this comment to others. Learn more.
🟡 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
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
deadline is computed before wait_for_ready(). If wait_for_ready() takes significant time, the remaining time budget for IP detection can be much shorter than timeout (or even already expired). Also, accessing node.features[SerialConsole] without checking support will raise a KeyError; it’s better to raise a clear LisaException instead.
The reason will be displayed to describe this comment to others. Learn more.
🔵 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
Previously missed (2) — in code that hasn't changed since the last review.
lisa/tools/reboot.py:199
Catching Exception here 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.
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)
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 existing wait_output API 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)
The reason will be displayed to describe this comment to others. Learn more.
🔵 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
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.
_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
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.
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.
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.
The reason will be displayed to describe this comment to others. Learn more.
🔵 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
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.
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)
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)
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
Major: close() is not enough to release this resource. _initialize() starts a long-lived start serial session process on the rack manager, but environment/node cleanup does not invoke feature close(), and RackManager.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, whose IdracSerialConsole is read-only and inherits SerialConsole.write() (which raises NotImplementedError). _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 Exception here 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.
Major: Every rackmanager client is now given an NvmeSettings() capability without detecting whether the machine has an NVMe device. Tests such as verify_nvme_basic require only [Nvme] and then assert that devices exist, so non-NVMe rackmanager hosts will be selected and fail instead of being skipped. Populate disk_count from capability detection or advertise Nvme only when the hardware is known to support it.
Major: The schema default is management_port=-1, but this truthiness check accepts -1 when iso_name is set. Deployment will then send -i -1 to 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"
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.
When no client has an iso_name, this new early return removes the previous deploy() 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.
Because _get_console_log calls wait_output with 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)
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.
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).
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 reach set_connection_info and fail with a less useful attribute error. Use an explicit exception (or the repository's assertion helper) for validation that must always execute.
[Major] The timeout starts before wait_for_ready, so a serial console that takes most of the configured timeout to log in gets no polling attempt and this method raises even though the console is ready. Start the IP-query deadline after wait_for_ready (or pass the remaining time) so login time does not consume the query timeout.
This polling loop uses raw time.sleep, which makes the new serial IP lookup bypass LISA's bounded-wait/retry helpers and is explicitly disallowed for test/runtime waits in this repository. Use check_till_timeout or an equivalent bounded helper so timeout handling and diagnostics remain consistent.
if not self.node.features.is_supported(SerialConsole):
return False
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issue
Type of Change
Checklist
Test Validation
Key Test Cases:
Impacted LISA Features:
Tested Azure Marketplace Images:
Test Results