Review comments

Author: adityagesh

lisa/features/non_ssh_executor.py

@@ -1,3 +1,5 @@
+from typing import List
+
 from lisa.feature import Feature
 from lisa.features.serial_console import SerialConsole
 
@@ -9,32 +11,29 @@ class NonSshExecutor(Feature):
     execution method for scenarios where SSH connectivity is not possible or desired.
     """
 
-    COMMANDS_TO_EXECUTE = [
-        "ip addr show",
-        "ip link show",
-        "systemctl status NetworkManager --no-pager --plain",
-        "systemctl status network --no-pager --plain",
-        "systemctl status systemd-networkd --no-pager --plain",
-        "ping -c 3 -n 8.8.8.8",
-    ]
-
     @classmethod
     def name(cls) -> str:
         return "NonSshExecutor"
 
     def enabled(self) -> bool:
         return True
 
-    def execute(self, commands: list[str] = COMMANDS_TO_EXECUTE) -> list[str]:
+    def execute(self, commands: List[str]) -> List[str]:
         """
         Executes a list of commands on the node and returns their outputs.
 
         :param commands: A list of shell commands to execute.
         :return: A string containing the output of the executed commands.
         """
         out = []
+
         serial_console = self._node.features[SerialConsole]
-        serial_console.login()
+        if not serial_console.enabled():
+            raise NotImplementedError(
+                "NonSshExecutor requires SerialConsole feature to be enabled."
+            )
+
+        serial_console.ensure_login()
         # clear the console before executing commands
         serial_console.write("\n")
         _ = serial_console.read()

lisa/features/serial_console.py

@@ -191,7 +191,7 @@ def check_initramfs(
                 f"{initramfs_logs} {filesystem_exception_logs}"
             )
 
-    def login(self) -> None:
+    def ensure_login(self) -> None:
         # Clear the serial console and try to get the login prompt
         self.read()
         self.write("\n")

lisa/node.py

@@ -3,7 +3,6 @@
 
 from __future__ import annotations
 
-import time
 from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
 from random import randint
 from typing import (
@@ -693,18 +692,14 @@ def _initialize(self, *args: Any, **kwargs: Any) -> None:
         assert self._connection_info, "call setConnectionInfo before use remote node"
         try:
             super()._initialize(*args, **kwargs)
-        except TcpConnectionException as e:
+        except TcpConnectionException:
             try:
-                vm_logs = self._collect_logs_using_non_ssh_executor()
-                if vm_logs:
-                    self.log.info(
-                        f"Collected information using non-ssh executor:\n{vm_logs}"
-                    )
+                self._collect_logs_using_non_ssh_executor()
             except Exception as log_error:
                 self.log.debug(
                     f"Failed to collect logs using non-ssh executor: {log_error}"
                 )
-            raise e
+            raise
 
     def get_working_path(self) -> PurePath:
         return self._get_remote_working_path()
@@ -748,17 +743,27 @@ def check_sudo_password_required(self) -> None:
                     raise RequireUserPasswordException("Reset password failed")
             self._check_password_and_store_prompt()
 
-    def _collect_logs_using_non_ssh_executor(self) -> Optional[str]:
+    def _collect_logs_using_non_ssh_executor(self) -> None:
         """
         Collects information using the NonSshExecutor feature.
         This is used when the connection to the node is not stable.
         """
         from lisa.features import NonSshExecutor
 
+        commands = [
+            "ip addr show",
+            "ip link show",
+            "systemctl status NetworkManager --no-pager --plain",
+            "systemctl status network --no-pager --plain",
+            "systemctl status systemd-networkd --no-pager --plain",
+            "ping -c 3 -n 8.8.8.8",
+        ]
+
         if self.features.is_supported(NonSshExecutor):
             non_ssh_executor = self.features[NonSshExecutor]
-            out = non_ssh_executor.execute()
-            return "\n".join(out)
+            out = non_ssh_executor.execute(commands=commands)
+            out = "\n\n".join(out)
+            self.log.info(f"Collected information using NonSshExecutor:\n{out}")
         else:
             self.log.debug(
                 f"NonSshExecutor is not supported on {self.name}, "
@@ -821,15 +826,15 @@ def get_password(self, generate: bool = True) -> str:
         if not self._connection_info.password:
             if not generate:
                 raise RequireUserPasswordException(
-                    "The password is not set, and generate is False."
+                    "The password is not set and generation is disabled."
                 )
             self.log.debug("password is not set, generating a strong password.")
             if not self._reset_password():
                 raise RequireUserPasswordException("Reset password failed")
         password = self._connection_info.password
         if not password:
             raise RequireUserPasswordException(
-                "The password is not set, and generate is False."
+                "The password has neither been set nor generated."
             )
         return password
 

lisa/sut_orchestrator/azure/features.py

@@ -6,7 +6,6 @@
 import re
 import string
 import time
-import uuid
 from dataclasses import dataclass, field
 from functools import partial
 from pathlib import Path
@@ -28,7 +27,6 @@
     HardwareProfile,
     NetworkInterfaceReference,
     RunCommandInput,
-    RunCommandInputParameter,
     VirtualMachineExtension,
     VirtualMachineUpdate,
 )
@@ -3735,7 +3733,6 @@ def _prepare_azure_file_share(
 
 
 class RunCommand(AzureFeatureMixin, Feature):
-
     @classmethod
     def create_setting(
         cls, *args: Any, **kwargs: Any
@@ -3777,7 +3774,7 @@ def execute(self, commands: List[str]) -> str:
 
         return result["value"][0]["message"]
 
-    def _add_echo_before_command(self, commands: List[str]):
+    def _add_echo_before_command(self, commands: List[str]) -> List[str]:
         """
         Adds an echo command before each command in the list to ensure
         that the output of each command is captured in the logs.
@@ -3786,9 +3783,7 @@ def _add_echo_before_command(self, commands: List[str]):
 
 
 class NonSshExecutor(AzureFeatureMixin, features.NonSshExecutor):
-    def execute(
-        self, commands: list[str] = features.NonSshExecutor.COMMANDS_TO_EXECUTE
-    ) -> list[str]:
+    def execute(self, commands: List[str]) -> List[str]:
         # RunCommand is faster than SerialConsole. Hence attempt to use it first.
         try:
             output = self._node.features[RunCommand].execute(commands)