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
This PR adds a new LISA test suite for the AZIHSM device and driver (lisa/microsoft/testsuites/azihsm/azihsm.py). All changes in this PR are scoped to that file; a separate pre-existing issue in add_repository() (operating_system.py), discovered while writing these tests, is tracked in #4730 rather than fixed here.
Related Issue
Type of Change
Bug fix
[ X] New feature
Breaking change
Refactoring
Documentation update
Checklist
[ X] Description is filled in above
[ X] No credentials, secrets, or internal details are included
The suite is restricted to CBLMariner and Ubuntu, but this case-level requirement replaces the inherited requirement with unsupported_os=[]. The same setting is used on every case, so the tests become eligible on Windows and other distros where the OS-specific package name/list is never initialized. Remove the case-level requirement or set supported_os=[CBLMariner, Ubuntu] consistently.
This assertion uses assertpy's local filesystem matcher against a path that exists on the remote VM. It will check the controller's /lib/modules/... tree and can fail even after a successful remote install. Check the path through node.shell/a remote tool instead.
assert_that(AziHsm.kmodpath).is_file()
lisa/microsoft/testsuites/azihsm/azihsm.py:281
contents_of() reads a local file, but modules.dep is on the target node. This check therefore cannot validate depmod on the VM and will normally raise a local FileNotFoundError; read the file with a node tool such as remote Cat before asserting its contents.
This is another local filesystem assertion against a remote module path. does_not_exist() can pass simply because the controller lacks that path, even if the .ko file remains installed on the VM; verify the remote path instead.
assert_that(AziHsm.kmodpath).does_not_exist()
lisa/microsoft/testsuites/azihsm/azihsm.py:417
Fixed sleeps make module-cycle validation flaky: a slow VM may still be settling after 0.5 seconds, while a fast VM waits unnecessarily. Replace these sleeps with a bounded remote state wait/retry so each cycle observes the module transition.
This fixed one-second delay does not guarantee that the remote module has finished unloading before the package is removed, so cleanup can race the package manager. Replace it with a bounded module-state wait.
time.sleep(1)
lisa/microsoft/testsuites/azihsm/azihsm.py:154
packages_installed is set before the loop has installed anything. If any package is unavailable or installation fails, later cases return immediately and invoke tests with missing user-space binaries. Mark this flag only after the loop completes successfully and make it node-scoped.
if packages_installed is True:
return
# Indicate we have done this step already
packages_installed = True
lisa/microsoft/testsuites/azihsm/azihsm.py:550
Catching Exception here also swallows programmer and infrastructure errors, converts them into a generic SDK failure, and lets the loop continue. Catch only the expected command-execution exception so unexpected failures remain visible.
This assertion uses a positional message instead of the repository's contextual described_as form, so failures do not follow the test assertion convention. Attach the explanation with described_as(...) before is_true().
assert_that(all_tests_passed, "Not all SDK tests passed").is_true()
lisa/microsoft/testsuites/azihsm/azihsm.py:407
cycles = 3 controls the coverage of this test but has no inline explanation. Add a short comment documenting why three load/unload repetitions are the intended threshold, as required for test-behavior magic numbers.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (48)
Previously missed (15) — in code that hasn't changed since the last review.
lisa/microsoft/testsuites/azihsm/azihsm.py:237
Major: Because each TestCaseMetadata supplies its own requirement, it overrides the suite requirement (TestCaseMetadata.__getattr__ only falls back when the attribute is absent). unsupported_os=[] therefore schedules these cases on every OS, including systems where the package names are never initialized. Use supported_os=[CBLMariner, Ubuntu] for every case (or remove the case-level requirement).
# Make sure we've added the AZIHSM repo
lisa/microsoft/testsuites/azihsm/azihsm.py:123
Minor: This skip message identifies the missing package but does not tell the operator how to investigate or recover. Include the target release and tell the user to verify that the AZIHSM testing/preview repository is enabled and publishes a package for this kernel.
This issue also appears on line 186 of the same file.
# Package is available, install it
log.info(f"Installing package {AziHsm.AziHsmDrvPkgName}")
lisa/microsoft/testsuites/azihsm/azihsm.py:214
Major: assertpy.is_file() checks the controller's filesystem, not the remote node. This assertion therefore does not verify that the driver module exists at AziHsm.kmodpath on the VM. Perform the existence check through node.execute or a remote LISA file tool.
This issue also appears in the following locations of the same file:
line 273
line 467
#
lisa/microsoft/testsuites/azihsm/azihsm.py:285
Major: contents_of() reads the test runner's local filesystem, so this validates a local /lib/modules/.../modules.dep rather than the file on the target VM. Read modules.dep through the node and keep the assertion on the returned remote content.
# Note: thiscould be under either 'extra' or 'updates'
# We do not provide the subdir, so this driver located anywhere
# in the tree would match and cause a false postive
assert_that(contents).contains("azihsm.ko")
lisa/microsoft/testsuites/azihsm/azihsm.py:416
Major: Modprobe.remove accepts a list and iterates over its argument, but this call passes the string AZIHSM_NAME. It therefore attempts to remove modules named a, z, i, etc. and leaves azihsm loaded, so the reload-cycle test is not testing the requested operation.
This issue also appears on line 472 of the same file.
time.sleep(0.5)
lisa/microsoft/testsuites/azihsm/azihsm.py:455
Major: Swallowing the uninstall exception means phase 14 does not enforce that removal succeeded, and the finally block logs success even when it failed. Let uninstall_packages propagate its failure, or catch only a specific exception that is intentionally handled.
Major: node.execute does not require a zero exit code by default. A failing SDK binary that emits an error other than the literal FAILED therefore reaches the pass path. Require expected_exit_code=0 (with a useful failure message) or assert on result.exit_code.
Minor: This logging call passes the exception as an extra argument without a formatting placeholder, so the failure detail is not rendered correctly by Python logging. Log the test name and exception in the message (and at error level for a failed test).
This issue also appears on line 559 of the same file.
Minor: Test methods in this suite should use the verify_ or test_ prefix. Rename this method so its test case is consistently discoverable and named with the suite's test convention.
This issue also appears in the following locations of the same file:
line 491
line 519 lisa/microsoft/testsuites/azihsm/azihsm.py:550
Major: Catching Exception around each SDK invocation also absorbs programming and infrastructure errors, reducing them to a test-failure boolean and allowing the suite to continue with an invalid state. Catch the specific command-failure exception that expected_exit_code=0 produces, and let unexpected exceptions propagate.
[Major] The suite is eligible for every Ubuntu and CBLMariner node, but AZIHSM_DEV is never checked and no AZIHSM capability is in the requirement. Nodes without /dev/azihsm0 will attempt package installation and driver tests and report failures instead of being skipped. Add a device/capability requirement or a before_case check that raises SkippedException.
[Minor] cycles = 3 controls the test scope but has no inline explanation. Add a comment or named constant stating why three cycles is the required minimum. lisa/microsoft/testsuites/azihsm/azihsm.py:285
This depmod assertion is not self-explanatory and lacks the required described_as(...) business context. Add a description explaining that the AZIHSM module must be registered by depmod. lisa/operating_system.py:959
Major: The newly added key URL spells rolling as roling; the Microsoft key endpoint is microsoft-rolling.asc, so Debian Azure Core repository setup will fail when it tries to download this key.
[Major] This replacement no longer handles PPA shorthand. Existing caller lisa/microsoft/testsuites/xdp/xdptools.py:101 passes ppa:ubuntu-toolchain-r/test; writing that literal into a .list file creates an invalid APT source instead of expanding the PPA as apt-add-repository did. Retain a PPA-specific branch or convert it to a valid deb entry.
with open("/etc/apt/sources.list.d/" + repo_file, "w") as f:
lisa/microsoft/testsuites/azihsm/azihsm.py:26
Major: These module-level flags are shared by all AziHsm suite instances and nodes. Setup performed on one environment (or one distro) causes later environments to skip their own setup, so they can use stale state or an unconfigured repository. Keep completion state per node or suite instance.
Major: These mutable class attributes are shared across every test instance, despite the comment calling them instance variables. If cases run against multiple nodes or kernels, one setup overwrites the package name and paths used by another case, causing installs and assertions to target the wrong kernel. Store these values on the instance or derive them from the current node.
Major: This flag is set before add_repository and kernel/package discovery complete. If repository setup fails once, every later call returns at line 57 and never retries, leaving the node without the AZIHSM repository. Set it only after the whole setup succeeds, and keep it per node.
testing_repo_added = True
lisa/microsoft/testsuites/azihsm/azihsm.py:111
Major: The package is installed before package_exists/is_package_in_repo is checked, so an unavailable package raises from install_packages before the intended SkippedException, and an available package is installed twice. Remove this early install and let the checked branch perform the install or update.
Major: packages_installed is marked true before the first package has been installed. Any failure leaves later test cases or nodes returning at line 151 with an incomplete package set, producing misleading follow-on failures. Set the flag only after the loop completes successfully and scope it per node.
# Indicate we have done this step already
packages_installed = True
# Make sure we've added the AZIHSM repo
self.setup_package_repository(node=node, log=log)
lisa/microsoft/testsuites/azihsm/azihsm.py:187
Minor: This skip message has the same problem as the driver-package message: it does not say which release was checked or how to recover. Include the release and instruct the operator to verify the AZIHSM testing/preview repository and package publication.
# Package is available, install it
log.info(f"Installing package {pkg}")
lisa/microsoft/testsuites/azihsm/azihsm.py:273
Major: assertpy.is_file() checks the controller's filesystem, not the remote node. The package-installation test can therefore pass or fail based on a local /lib/modules tree instead of validating the module installed on the VM.
# Test 4 - rpm -V reports no discrepancies
lisa/microsoft/testsuites/azihsm/azihsm.py:343
Minor: A fixed time.sleep() after a remote module operation can either waste time or race when the operation takes longer than one second. Replace it with a bounded poll for the module to become unloaded (for example, check_till_timeout).
finally:
lisa/microsoft/testsuites/azihsm/azihsm.py:349
Minor: This test loads and unloads a kernel driver but never marks the node dirty. Mark the node dirty before mutating driver state so LISA does not reuse an environment with untracked kernel-module changes.
log.info("modprobe load succeeded")
lisa/microsoft/testsuites/azihsm/azihsm.py:417
Minor: The two fixed time.sleep(0.5) calls assume that load and unload complete within a hard-coded interval. Use bounded polling of the remote module state instead, otherwise slower nodes can race the next cycle while faster nodes wait unnecessarily.
modprobe.remove([AZIHSM_NAME])
time.sleep(0.5)
lisa/microsoft/testsuites/azihsm/azihsm.py:414
Minor: This test repeatedly loads and unloads a kernel driver but never marks the node dirty. Mark the node dirty before the first cycle so the environment is not reused with untracked driver-state changes.
time.sleep(0.5)
lisa/microsoft/testsuites/azihsm/azihsm.py:446
Minor: A fixed sleep after a remote module removal is race-prone and adds an unnecessary delay on fast nodes. Replace it with a bounded poll that confirms the module is unloaded before uninstalling the package. lisa/microsoft/testsuites/azihsm/azihsm.py:467
Major: assertpy.does_not_exist() checks the controller's filesystem, not the VM. Since the local path will normally be absent, this can report success even when the module file remains installed on the target node. Check the path remotely with the node shell or a LISA file tool.
log.info("Module file removed from disk")
lisa/microsoft/testsuites/azihsm/azihsm.py:475
Major: This block catches the expected module-load failure and then does nothing if modprobe.load succeeds. Consequently, test 17 passes when the module still loads after uninstall, which is the opposite of the required check. Track the failure and assert it, with a specific exception rather than except Exception.
Major: The pass condition only searches for FAILED; empty output, SKIPPED, or any other non-success output is logged as Passed (including the empty output produced after an exception). Require a positive success marker as well as the absence of failure output before recording a pass.
Minor: Use the assertpy description API instead of passing the failure text as a second positional argument. This assertion should describe the business condition being checked, consistently with the assertions earlier in the suite. lisa/microsoft/testsuites/azihsm/azihsm.py:491
Minor: Test methods in this suite should use the verify_ or test_ prefix. Rename this method so its test case is consistently discoverable and named with the suite's test convention.
# Make sure the driver package is installed
lisa/microsoft/testsuites/azihsm/azihsm.py:519
Minor: Test methods in this suite should use the verify_ or test_ prefix. Rename this method so its test case is consistently discoverable and named with the suite's test convention.
# Make sure the driver package is installed
lisa/microsoft/testsuites/azihsm/azihsm.py:71
[Major] This source line requires /usr/share/keyrings/microsoft-prod.gpg, but this call neither supplies keys_location nor creates that file; Ubuntu.add_repository only imports keys when they are passed, and those imports do not create this named keyring. On a generic supported Ubuntu image, apt-get update can therefore fail because the signed-by keyring is missing. Provision this exact keyring or remove the reference.
[Major] The new Mariner repo uses plain HTTP and the default no_gpgcheck=True in CBLMariner.add_repository, so driver and test packages are downloaded without transport or package signature verification. Use HTTPS and verify the repository's signing key, or explicitly justify an isolated trusted test mirror.
[Minor] The suite metadata contains spelling errors (suck, avaiable, and microsfot) in user-facing test documentation. Correct them to such, available, and microsoft.
verification of user space components suck as the SDK API and the OpenSSL
engine that uses axihsm.
These tests assume that the package to be tested is avaiable via testing
or preview releases in packages.microsfot.com. The needed URLs will be
lisa/microsoft/testsuites/azihsm/azihsm.py:264
The metadata promises that the package version matches the expected version, but this code only logs package_info; it never defines or compares an expected version. A wrong driver package can therefore pass Phase 1. Add a concrete version assertion or remove that step from the case description.
# assert_that(package_installed).described_as(
# f"{pkg} package should be installed"
lisa/microsoft/testsuites/azihsm/azihsm.py:36
The docstring contains spelling errors: suck should be such, and axihsm should be azihsm. Correct these names so the suite documentation describes the actual product and engine.
verification of user space components suck as the SDK API and the OpenSSL
engine that uses axihsm.
lisa/microsoft/testsuites/azihsm/azihsm.py:40
This docstring has several typos: avaiable → available, microsfot → microsoft, and ad → and. Correct the repository URL and grammar in the description.
These tests assume that the package to be tested is avaiable via testing
or preview releases in packages.microsfot.com. The needed URLs will be
added to the system ad the package installed via apt or tdnf.
lisa/microsoft/testsuites/azihsm/azihsm.py:244
The comment misspells explicitely; use explicitly so the installation test description is clear. lisa/microsoft/testsuites/azihsm/azihsm.py:271
The comment says the module exists od disk; correct od to on. lisa/microsoft/testsuites/azihsm/azihsm.py:284
The comment contains thiscould and postive typos and is missing is before located. Correct it so the depmod check rationale is understandable.
# We do not provide the subdir, so this driver located anywhere
# in the tree would match and cause a false postive
assert_that(contents).contains("azihsm.ko")
lisa/microsoft/testsuites/azihsm/azihsm.py:119
The comment misspells avaialble and has incorrect grammar. Use Check if package is available in repositories so the precondition is clear.
f"{AziHsm.AziHsmDrvPkgName} package not found in repositories"
lisa/microsoft/testsuites/azihsm/azihsm.py:185
This duplicated package check comment repeats the avaialble spelling error and incorrect grammar. Use Check if package is available in repositories. lisa/microsoft/testsuites/azihsm/azihsm.py:445
The uninstallation case unloads the kernel driver before changing the package, but it never marks the node dirty. Call node.mark_dirty() for this case as required whenever driver state is changed, so later cases do not reuse altered module state.
time.sleep(1)
lisa/operating_system.py:389
Critical: Making repo_file mandatory breaks existing add_repository callers that were not updated (for example in transformers/upgrade_packages.py, transformers/kernel_installer.py, microsoft/testsuites/xdp/xdptools.py, cvm/cvm_boot.py, and acc/bvt.py); those paths now raise TypeError before adding a repository. Update every caller in the same change or keep a compatible API/default.
repo_file: str,
lisa/operating_system.py:1180
Critical: open() runs on the LISA controller, not on self._node, so this writes (or tries to write) /etc/apt/... on the host and never creates the repository file on the VM. Write the file through the node shell or another remote-file API, with safe quoting, before running apt-get update.
with open("/etc/apt/sources.list.d/" + repo_file, "w") as f:
f.write(f"{repo}\n")
lisa/operating_system.py:1178
The explanatory comment contains multiple spelling/grammar errors (it's should be its, inconsistant → inconsistent, accross → across, useing → using, and to to is duplicated). Correct it so the rationale for bypassing apt-add-repository is clear.
# apt-add-repository seems like the right tool to use here, but it's interface
# is too inconsistant accross the distro/release combinations, and it is not
# able to handle the signed-by option which we need to be useing. Instead,
# It is trivial to to just create the file directly
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.
Suppressed comments (23)
lisa/microsoft/testsuites/azihsm/azihsm.py:61
Major: These module-level flags are shared by every node and testing_repo_added is set before add_repository() succeeds. A failure on the first node can make later nodes skip repository setup, while the class-level kernel/package values remain from the first node. Keep setup state per node/suite and set it only after successful completion.
global testing_repo_added
if testing_repo_added is True:
return
# Indicate we have done this step already
testing_repo_added = True
lisa/microsoft/testsuites/azihsm/azihsm.py:151
Major:packages_installed is set before the repository/package loop completes. If any package check or installation fails, subsequent test cases return immediately and run with an incomplete installation. Set the flag only after the entire loop succeeds, or avoid caching this state globally.
packages_installed = True
lisa/microsoft/testsuites/azihsm/azihsm.py:270
Critical:is_file() checks the LISA runner's filesystem, but AziHsm.kmodpath is a path on the test VM. This assertion can fail or pass based on an unrelated local file and does not verify the installed driver. Use node.shell.exists(node.get_pure_path(AziHsm.kmodpath)) (or a remote test -f) instead.
assert_that(AziHsm.kmodpath).is_file()
lisa/microsoft/testsuites/azihsm/azihsm.py:342
Major: Fixed time.sleep(1) delays are forbidden for integration tests and do not guarantee that the remote module state has converged. Poll the module state with a bounded check_till_timeout()/retry helper instead of relying on a fixed delay.
time.sleep(1)
lisa/microsoft/testsuites/azihsm/azihsm.py:416
Major: The fixed sleeps between reload cycles make this integration test slower and still flaky when module removal takes longer than 0.5 seconds. Replace them with bounded polling of is_module_loaded()/lsmod state, or otherwise wait on the command's observable completion.
Major: This fixed delay has the same flakiness problem: it assumes one second is enough for the remote module removal before package uninstall. Use a bounded state check rather than time.sleep().
time.sleep(1)
lisa/microsoft/testsuites/azihsm/azihsm.py:388
Major: These cases install/remove packages and load/unload a kernel driver, but no node.mark_dirty() is called. LISA can then treat the environment as reusable even though package metadata, repository configuration, and module state were changed. Mark the node dirty from guaranteed cleanup/after-case handling.
finally:
# Clean up
modprobe.remove([AZIHSM_NAME], ignore_error=True)
node.os.uninstall_packages(AziHsm.AziHsmDrvPkgName)
lisa/microsoft/testsuites/azihsm/azihsm.py:548
Major:except Exception hides all errors from the SDK command, and log.info("Failed:", e) passes an argument without a % placeholder to Python logging, which produces a logging formatting error instead of the exception message. Catch only expected command failures and log the test name/exception with an f-string or log.exception().
except Exception as e:
cmd_output = ""
log.info("Failed:", e)
lisa/microsoft/testsuites/azihsm/azihsm.py:546
[Minor] Catching Exception here also converts programming errors and framework failures into a normal SDK-test failure, making diagnosis harder. Catch only the expected command execution exception(s), or re-raise unexpected exceptions after recording the individual test result.
except Exception as e:
lisa/microsoft/testsuites/azihsm/azihsm.py:184
[Minor] This skip message lacks the target OS and recovery guidance, so a missing package cannot be distinguished from an incorrectly configured AZIHSM feed. Include the OS and tell the operator to verify the testing/preview repository and package publication.
if not node.os.is_package_in_repo(pkg):
raise SkippedException(f"{pkg} package not found in repositories")
lisa/microsoft/testsuites/azihsm/azihsm.py:261
[Minor] The installation-case description promises a package-version check, but package_info is only logged and no expected version is compared. Either obtain the expected version from the test variables/repository contract and assert it, or remove that step from the metadata so the case documents behavior it actually validates.
[Nit] The new suite description contains multiple spelling/name errors (suck, axihsm, avaiable, microsfot, and ad). These appear in generated test metadata, so correct them before publishing the suite documentation.
verification of user space components suck as the SDK API and the OpenSSL
engine that uses axihsm.
These tests assume that the package to be tested is avaiable via testing
or preview releases in packages.microsfot.com. The needed URLs will be
lisa/microsoft/testsuites/azihsm/azihsm.py:341
The precondition removes azihsm when it was already loaded, but the cleanup path always leaves it unloaded. That changes the node's original kernel state for later tests; capture the initial state and restore it in cleanup, as the existing XFRM module test does.
if modprobe.is_module_loaded(
AZIHSM_NAME, force_run=True, no_error_log=True
):
modprobe.remove([AZIHSM_NAME])
lisa/microsoft/testsuites/azihsm/azihsm.py:413
The reload loop also always finishes with azihsm unloaded, even if it was loaded before the test. Preserve and restore the initial module state, or explicitly mark the node dirty so later tests do not inherit this mutation.
modprobe.load(AZIHSM_NAME)
lisa/microsoft/testsuites/azihsm/azihsm.py:406
The loop count controls test behavior but the literal has no inline explanation. Add an inline comment (or a named constant) describing why three load/unload cycles are required.
cycles = 3
lisa/microsoft/testsuites/azihsm/azihsm.py:544
The SDK command does not pass expected_exit_code=0, and Node.execute defaults to no exit-code validation. A binary can return nonzero without printing FAILED, after which this loop reports it as passed; require a zero exit code (and keep the output check if needed).
This test method does not follow the repository's test naming convention, which prefixes test cases with verify_ or test_. Rename it to a prefixed name and update any external test selection that uses the current name.
This test method does not follow the repository's test naming convention, which prefixes test cases with verify_ or test_. Rename it to a prefixed name and update any external test selection that uses the current name.
This test method does not follow the repository's test naming convention, which prefixes test cases with verify_ or test_. Rename it to a prefixed name and update any external test selection that uses the current name.
Critical: Making repo_file required breaks existing callers that still invoke add_repository(repo), including the package-upgrade transformer, kernel installer, ACC/XDP setup, and Docker's Debian path. Those paths now raise TypeError before adding a repository. Update every caller in this change, or provide a backward-compatible default and handle it in each implementation.
repo_file: str,
lisa/operating_system.py:1180
Critical:open() executes on the LISA runner, not on self._node, so this tries to modify the runner's /etc/apt/sources.list.d (and without sudo) rather than the VM. apt-get update on the node therefore cannot see the repository. Write the file through a node-side file tool/command with sudo and a node path.
with open("/etc/apt/sources.list.d/" + repo_file, "w") as f:
f.write(f"{repo}\n")
lisa/operating_system.py:2332
Major:CBLMariner.add_repository() accepts repo_file but discards it for local tarballs. _create_local_repo() ultimately always writes /etc/yum.repos.d/builtpackages.repo, so the updated MSHV calls requesting mshv.repo and base.repo do not create those files and cannot keep the two repositories distinct. Thread the filename through the local-repository creation path.
repo_file: str,
lisa/operating_system.py:1178
[Nit] The new explanatory comment has several spelling/grammar errors (it's, inconsistant, accross, useing, and to to). Correct it so the repository behavior and rationale are clear to future maintainers.
# apt-add-repository seems like the right tool to use here, but it's interface
# is too inconsistant accross the distro/release combinations, and it is not
# able to handle the signed-by option which we need to be useing. Instead,
# It is trivial to to just create the file directly
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Suppressed comments (29)
Previously missed (10) — in code that hasn't changed since the last review.
lisa/microsoft/testsuites/azihsm/azihsm.py:211
MajorAziHsm.kmodpath is evaluated by assertpy on the LISA runner, not on node, so this does not verify the remote driver file and can fail even when installation is correct. Check the path through node.shell.exists and assert that remote result.
assert_that(AziHsm.kmodpath).is_file()
lisa/microsoft/testsuites/azihsm/azihsm.py:464
Major Catching the uninstall exception and unconditionally logging success suppresses the actual failure and can make the test report misleading progress. Let the package API exception fail the case, or catch a specific expected exception, log actionable context, and re-raise it.
Major This path check is also evaluated on the LISA runner, so it can pass because the runner lacks the file even when the remote .ko remains installed. Assert the remote node.shell.exists(...) result instead.
assert_that(AziHsm.kmodpath).does_not_exist()
lisa/microsoft/testsuites/azihsm/azihsm.py:484
Major This try block does not assert that modprobe.load failed; if loading succeeds, execution simply continues and the test still passes step 17. Record the expected failure and assert it explicitly.
Critical If node.execute raises, result is never assigned, but the finally block unconditionally reads result.stdout, replacing the actual driver-test failure with UnboundLocalError. Do not swallow the execution error; assert the output only after a successful command.
except Exception:
log.info("test failed")
finally:
cmd_output = result.stdout.strip()
assert_that("PASSED" in cmd_output, "Test did not contain PASSED").is_true()
lisa/microsoft/testsuites/azihsm/azihsm.py:587
Minor Use described_as for this non-self-explanatory assertion so a failed SDK run identifies the business condition being checked.
assert_that(all_tests_passed, "Not all SDK tests passed").is_true()
lisa/microsoft/testsuites/azihsm/azihsm.py:116
Correct the new comment spelling and grammar: Check is package is avaialble is repositories should read Check if the package is available in the repositories.
# Check is package is avaialble is repositories
lisa/microsoft/testsuites/azihsm/azihsm.py:236
Minor: Newly added test methods must use the repository's verify_ or test_ prefix. package_installation_tests does not follow that convention, which makes test discovery and naming inconsistent.
The skip message says only that the driver package is missing; it does not tell the operator how to investigate or resolve the skip. Include that the configured AZIHSM repository should be checked for a package matching the running kernel.
This issue also appears on line 180 of the same file.
raise SkippedException(
f"{AziHsm.AziHsmDrvPkgName} package not found in repositories"
)
lisa/operating_system.py:1178
The explanatory comment added here contains several spelling and grammar errors (inconsistant accross, useing, and to to). Please correct the wording so the repository implementation is documented accurately.
# apt-add-repository seems like the right tool to use here, but it's interface
# is too inconsistant accross the distro/release combinations, and it is not
# able to handle the signed-by option which we need to be useing. Instead,
# It is trivial to to just create the file directly
lisa/microsoft/testsuites/azihsm/azihsm.py:26
Major These module-level flags, together with the mutable class attributes below, cache setup across test cases and nodes in the same Python process. A later node can therefore skip repository/package setup and reuse the first node's kernel package/path. Keep this state per node or per suite instance instead of using process-wide shared state.
Major An empty unsupported_os set imposes no OS restriction and overrides the suite's supported_os=[CBLMariner, Ubuntu]. This case can be scheduled on an unsupported OS, where the package/path variables are never initialized. Use the suite's supported OS list here (and in the other case decorators) or omit the case-level requirement.
Majorcontents_of opens /lib/modules/.../modules.dep on the test runner. This depmod check is reading the wrong machine; fetch the file through node and assert the returned remote content.
contents = contents_of(
lisa/microsoft/testsuites/azihsm/azihsm.py:228
Major The test description promises a package-version check (step 2b), but the body only checks that some version is installed and never obtains or compares an expected version. Either implement that assertion or remove the step so the test does not claim coverage it does not provide.
2a. Package registered in RPM database.
2b. Package version matches expected version.
lisa/microsoft/testsuites/azihsm/azihsm.py:320
Major This case loads and unloads a kernel driver but never marks the node dirty. Cleanup does not remove the requirement to mark an environment whose driver/package state was modified; call node.mark_dirty() before the installation/mutation so LISA does not reuse it as clean.
Minortime.sleep is not an acceptable bounded wait in LISA tests and makes module-state polling timing-dependent. Use check_till_timeout, retry_without_exceptions, or the retry decorator with an explicit timeout, and assert the final state.
time.sleep(0.5)
lisa/microsoft/testsuites/azihsm/azihsm.py:426
Minortime.sleep is not an acceptable bounded wait in LISA tests and makes module-state polling timing-dependent. Use check_till_timeout, retry_without_exceptions, or the retry decorator with an explicit timeout, and assert the final state.
time.sleep(0.5)
lisa/microsoft/testsuites/azihsm/azihsm.py:455
Minortime.sleep is not an acceptable bounded wait in LISA tests and makes module-state polling timing-dependent. Use check_till_timeout, retry_without_exceptions, or the retry decorator with an explicit timeout, and assert the final state.
time.sleep(1)
lisa/microsoft/testsuites/azihsm/azihsm.py:578
Minor Passing an exception as a second logging argument without a format placeholder does not include the exception reliably and can trigger a logging formatting error. Use an f-string (or exc_info) so the failing SDK test and its cause are recorded.
except Exception as e:
cmd_output = ""
log.info("Failed:", e)
lisa/microsoft/testsuites/azihsm/azihsm.py:576
Minor Catching Exception here also hides unexpected programming or transport errors and then continues through the remaining SDK checks. Catch the specific command-failure exception that node.execute can raise, and let unrelated failures surface.
except Exception as e:
lisa/microsoft/testsuites/azihsm/azihsm.py:22
Major: AZIHSM_DEV is defined but never used as a precondition. On an Ubuntu/CBLMariner image without /dev/azihsm0, the cases proceed into module and SDK operations and report failures instead of being skipped for a missing device. Check the target-node device and raise SkippedException before running device-dependent cases.
AZIHSM_DEV = "/dev/azihsm0"
lisa/microsoft/testsuites/azihsm/azihsm.py:79
Critical: This repository is fetched over HTTP, and the default no_gpgcheck=True causes the RPM implementation to invoke yum-config-manager with --nogpgcheck. The test then installs packages as root from an unauthenticated transport. Use the HTTPS endpoint and explicitly enable GPG verification (no_gpgcheck=False) for this signed Microsoft repository.
The skip message says only that the package is missing; it does not tell the operator how to investigate or resolve the skip. Include that the configured AZIHSM repository should be checked for the package on the current OS.
if not node.os.is_package_in_repo(pkg):
raise SkippedException(f"{pkg} package not found in repositories")
lisa/microsoft/testsuites/azihsm/azihsm.py:517
The assertion checks a derived boolean rather than the command output and uses a positional message instead of the repository's described_as form. Assert the output directly (for example, assert_that(cmd_output).contains("PASSED")) so the failure includes useful context.
assert_that("PASSED" in cmd_output, "Test did not contain PASSED").is_true()
lisa/operating_system.py:393
Critical Making repo_file mandatory here breaks existing callers that still call add_repository without it, including lisa/transformers/upgrade_packages.py, lisa/transformers/kernel_installer.py, the ACC/XDP suites, and lisa/tools/docker.py. Those paths now raise TypeError before adding a repository. Update every caller or preserve a backward-compatible default/name-generation path.
Critical This open() runs in the LISA process, not on self._node, and it is not privileged. The repository file is therefore written to the test runner (or fails with permission denied), while the node's subsequent apt-get update never sees the new source. Write the file through a node tool such as Tee with sudo=True.
with open("/etc/apt/sources.list.d/" + repo_file, "w") as f:
f.write(f"{repo}\n")
lisa/operating_system.py:1179
Major Replacing apt-add-repository with a raw .list file write also breaks PPA inputs. The existing caller at lisa/microsoft/testsuites/xdp/xdptools.py:101 passes ppa:ubuntu-toolchain-r/test, which is not a valid line in sources.list.d; the following apt-get update will fail. Preserve a PPA branch using AptAddRepository or convert that caller to a complete deb entry.
# apt-add-repository seems like the right tool to use here, but it's interface
# is too inconsistant accross the distro/release combinations, and it is not
# able to handle the signed-by option which we need to be useing. Instead,
# It is trivial to to just create the file directly
with open("/etc/apt/sources.list.d/" + repo_file, "w") as f:
lisa/operating_system.py:1764
Major: The new repo_file argument is discarded by the Redhat implementation; it is neither passed to YumConfigManager nor used to write the repository file. In docker.py, the two vault URLs both end in x86_64, so yum-config-manager can derive the same output filename and overwrite or reject one instead of creating centos-extras.repo and centos-os.repo. Implement filename handling in the RPM repository path.
repo_file: str,
lisa/operating_system.py:2332
Major: CBLMariner also accepts repo_file but ignores it, so the new AZIHSM setup's preview-ms-oss.repo name is never honored. The repository may be written under a derived name or collide with another generated repo file; use repo_file consistently when adding RPM repositories.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Suppressed comments (34)
Previously missed (1) — in code that hasn't changed since the last review.
lisa/microsoft/testsuites/azihsm/azihsm.py:397
[Minor] The cycle count controls test coverage but is an unexplained magic number. Add an inline rationale or a named constant so changes to the number of reload cycles are intentional and discoverable.
cycles = 3
lisa/microsoft/testsuites/azihsm/azihsm.py:67
[Major] The source entry uses signed-by=/usr/share/keyrings/microsoft-prod.gpg, but this setup neither passes keys_location nor creates that keyring; the existing add_key path installs downloaded keys under /etc/apt/trusted.gpg.d. On a supported Ubuntu image without a preexisting Microsoft keyring, apt-get update will reject this testing repo. Provision the exact keyring or use a matching key-install strategy.
[Major] The result of node.shell.exists(...) is discarded, so this step never fails when the .ko file is missing despite the test description claiming it verifies the file.
[Major] These flags and paths are class attributes shared by every AziHsm instance. Once one environment sets testing_repo_added, a later environment can skip setup and reuse the first node's kernel package/path, causing tests to target the wrong driver or repository. Keep this state per suite instance or key it by node and kernel.
[Major] If modprobe.remove leaves the module loaded after all five attempts, this loop simply falls through and the test proceeds; the subsequent modprobe.load is then a no-op, so the test does not validate a load from an unloaded state. Use a bounded wait that raises or asserts when the module is still loaded before starting Test 7.
for attempt in range(5):
if not modprobe.is_module_loaded(
AZIHSM_NAME, force_run=True, no_error_log=True
):
break
lisa/microsoft/testsuites/azihsm/azihsm.py:456
[Minor] This fixed sleep(1) does not verify that the module has actually unloaded before package removal, and direct sleeps are disallowed for test synchronization. Poll is_module_loaded with a bounded timeout instead.
time.sleep(1)
lisa/microsoft/testsuites/azihsm/azihsm.py:568
[Minor] The log interpolates the previous loop value before assigning azihsm_api_cpp_tests, so it labels the C++ test with the wrong name. Assign test first, then log it.
log.info(f"Running {test}")
test = "azihsm_api_cpp_tests"
lisa/microsoft/testsuites/azihsm/azihsm.py:588
[Minor] Use described_as for this assertion rather than passing a positional message, consistent with the project's assertpy convention and the other assertions in this file.
assert_that(all_tests_passed, "Not all SDK tests passed").is_true()
lisa/microsoft/testsuites/azihsm/azihsm.py:180
[Minor] This skip message only states that the package is missing and does not explain how to investigate or resolve it. Include the configured repository and OS/release in the guidance so an unavailable AZIHSM package is distinguishable from a repository setup failure.
raise SkippedException(f"{pkg} package not found in repositories")
lisa/microsoft/testsuites/azihsm/azihsm.py:43
[Major] The suite requirement filters only by distro, although these tests target an Azure-only /dev/azihsm0 device and no case checks for its presence. Selecting this suite on any Ubuntu/CBLMariner VM without the device will run the tests and report failures instead of skipping. Constrain the requirement to the supported platform/device or add a precondition.
[Major] Catching the uninstall exception and continuing means Test 14 can be reported as successful whenever the package happens to be absent afterward, even though uninstall_packages failed. Re-raise after logging (and catch the specific package-manager exception) so the removal operation itself is validated.
except Exception as e:
log.error(f"Uninstall failed {e}")
lisa/microsoft/testsuites/azihsm/azihsm.py:428
[Minor] This is the same broad catch-and-re-raise pattern as the load wait above. Catching Exception here adds no handling and can hide which failure types the test expects; let the polling exception propagate or catch only the concrete exception.
except Exception as e:
log.error(f"Exception {e}")
raise e
lisa/microsoft/testsuites/azihsm/azihsm.py:517
[Minor] The driver command catches every exception, discards it, and converts it into a generic output assertion. This hides transport/execution failures and makes diagnosis harder. Let unexpected exceptions propagate or catch only the expected process-failure type while preserving its context.
[Minor] This broad catch turns any SDK execution error into an empty-output result and only marks the aggregate false. Catch the specific expected command failure, and allow transport or programming errors to fail with their original context.
[Minor] The C++ SDK test has the same broad exception catch, which hides unexpected failures behind the final aggregate assertion. Catch only the expected command-failure type or let unexpected exceptions propagate with their diagnostic output.
[Minor] The retry count is a test-behavior magic number without an inline explanation. Name the bound or annotate why five polls are sufficient, so changing the unload timing does not silently weaken this precondition.
for attempt in range(5):
lisa/microsoft/testsuites/azihsm/azihsm.py:508
[Minor] The literal thread count controls test execution behavior without an inline explanation. Document why the vendor tests must run with one thread or define the setting as a named constant.
params = "--test-threads 1"
lisa/microsoft/testsuites/azihsm/azihsm.py:544
[Minor] The literal thread count controls test execution behavior without an inline explanation. Document why the vendor tests must run with one thread or define the setting as a named constant.
params = "--test-threads 1"
lisa/microsoft/testsuites/azihsm/azihsm.py:23
[Major] Although this constant declares the expected device node, none of the test cases uses it, so installation and module tests can pass without verifying that the AZIHSM device is exposed. Add a device-node assertion after loading the module, or remove the constant if device presence is intentionally covered only by the vendor binaries.
AZIHSM_DEV = "/dev/azihsm0"
lisa/microsoft/testsuites/azihsm/azihsm.py:36
[Nit] Correct the grammar in this suite description.
These tests assume that the packages to be tested is available via testing
lisa/microsoft/testsuites/azihsm/azihsm.py:115
[Nit] Fix the misspelling in this comment.
# Check that the package is avaialble in configured repositories
lisa/microsoft/testsuites/azihsm/azihsm.py:178
[Nit] Fix the spelling and grammar in this comment.
# Check is package is avaialble is repositories
lisa/microsoft/testsuites/azihsm/azihsm.py:238
[Nit] Fix the misspelling in this comment.
# Remove the driver package so we can explicitely test its install
lisa/microsoft/testsuites/azihsm/azihsm.py:256
[Nit] Fix the typo in this comment.
# Test 3 - Module .ko file exists od disk
lisa/microsoft/testsuites/azihsm/azihsm.py:271
[Nit] Fix the misspelling in this comment.
# in the tree would match and cause a false postive
lisa/microsoft/testsuites/azihsm/azihsm.py:351
[Nit] Separate the words in this comment.
# Test 9 - No dmesg errors from themodule
lisa/microsoft/testsuites/azihsm/azihsm.py:483
[Nit] Fix the misspelling in this log message.
log.error("modprobe succedded when package should not be installed")
lisa/microsoft/testsuites/azihsm/azihsm.py:538
[Nit] Fix the misspelling in this comment.
# "azihsm_api_cpp_tests", - do this seperately
lisa/microsoft/testsuites/azihsm/azihsm.py:505
The driver test installs the kernel package and all user-space test packages but never removes them or marks the node dirty. On a reusable environment, these packages and the testing repository remain installed and can affect later cases; add guaranteed cleanup in after_case or explicitly mark the environment as dirty.
def test_run_azihsm_driver_tests(self, node: Node, log: Logger) -> None:
# Make sure the driver package is installed
self.install_azihsm_driver_package(node=node, log=log)
# Make sure the azihsm packages are installed
lisa/microsoft/testsuites/azihsm/azihsm.py:48
AziHsmDrvPkgName is a variable, not a class, so its CamelCase spelling conflicts with the surrounding snake_case attributes (kmodpath, kernel_version) and PEP 8. Rename it to a snake_case name consistently at all references.
AziHsmDrvPkgName = ""
lisa/operating_system.py:389
[Critical] Making repo_file mandatory breaks existing callers that were not updated (for example upgrade_packages.py, kernel_installer.py, acc/bvt.py, and xdp/*). Those paths now raise TypeError before adding a repository. Either update every call site with a deterministic filename or keep this parameter optional and preserve the previous behavior.
repo_file: str,
lisa/operating_system.py:1179
[Critical] This built-in open() executes in the LISA runner process, not on self._node, and it also has no remote sudo handling. Ubuntu repository additions therefore write to the controller (or fail with permission denied), while the subsequent apt-get update on the VM never sees this file. Use a node file-writing tool or remote command with the node path and sudo.
with open("/etc/apt/sources.list.d/" + repo_file, "w") as f:
lisa/operating_system.py:1179
[Major] This implementation also removes apt-add-repository support for PPA shorthand. Existing callers such as xdp/xdptools.py pass ppa:ubuntu-toolchain-r/test; writing that literal into a .list file is not a valid deb source, so apt-get update fails even after the remote-write issue is fixed. Preserve a PPA-specific path or expand the PPA before writing the source file.
with open("/etc/apt/sources.list.d/" + repo_file, "w") as f:
lisa/operating_system.py:1178
[Nit] The newly added explanatory comment contains spelling and grammar errors, including it's, inconsistant, accross, useing, and to to. Correct the wording so the rationale for bypassing apt-add-repository remains clear.
# apt-add-repository seems like the right tool to use here, but it's interface
# is too inconsistant accross the distro/release combinations, and it is not
# able to handle the signed-by option which we need to be useing. Instead,
# It is trivial to to just create the file directly
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Suppressed comments (1)
lisa/operating_system.py:391
OperatingSystem.add_repository() now requires repo_file, but there are existing call sites that pass only repo/keys_location (e.g. microsoft/testsuites/xdp/xdptools.py:101-110, transformers/upgrade_packages.py:147). This will raise TypeError at runtime and is effectively a breaking API change. Consider making repo_file optional (default None) to preserve backward compatibility, and only require it for Debian’s manual-file fallback path.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Suppressed comments (8)
Previously missed (5) — in code that hasn't changed since the last review.
lisa/operating_system.py:1163
add_repository() logs at ERROR level and contains a typo ("add_Repositroy"). This will create noisy logs on normal code paths and makes log searching harder.
self._log.error(f"in add_Repositroy({repo})")
lisa/microsoft/testsuites/azihsm/azihsm.py:48
This suite stores per-node state (repo added flag, kernel version, package names, paths) in class variables. In a multi-node run, the first node to execute setup_package_repository() will set these values and subsequent nodes will reuse them, which can make later test cases install the wrong kernel-module package and check the wrong module path.
check_driver_package_contents() verifies the kernel module path, but it only installs the user-space AZIHSM packages. The driver/kernel-module package (AziHsm.azihsmdrvpkgname) may not be installed at this point, which can make the module-path assertion fail for the wrong reason.
# Make sure the azihsm packages are installed
self.install_all_azihsm_packages(node=node, log=log)
lisa/microsoft/testsuites/azihsm/azihsm.py:239
Per LISA test suite conventions, each @TestCaseMetadata should include a requirement=.... This decorator is missing requirement, and the same applies to the other test cases in this file.
priority=0,
)
lisa/microsoft/testsuites/azihsm/azihsm.py:342
This wait is checking for the module to be unloaded, but the timeout_message says "load". Also, the timeout=5 is a test-control magic number and should have an inline comment (seconds) per LISA guidelines.
timeout_message="Wait for module to load",
timeout=5,
lisa/operating_system.py:1181
Catching a broad Exception here can hide real failures from apt-add-repository and then silently fall back to writing a file (which is not valid for all repo formats, e.g. ppa:). This can lead to the repo not being added while the code continues as if it succeeded.
except Exception:
# apt-add-repository seems like the right tool to use here,
# but its interface is too inconsistant across the distro/release
# combinations, and it is not able to handle the signed-by option
# which we need to be using. If it fails, just create the file directly
lisa/microsoft/testsuites/azihsm/azihsm.py:267
purepath(...) isn’t defined/imported in this module, so this assertion will raise at runtime. Use node.get_pure_path() instead.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (14)
Previously missed (6) — in code that hasn't changed since the last review.
lisa/operating_system.py:869
Major: apt_key_available is now hardcoded to False, which makes the apt-key branches dead code and may break key installation on images that still rely on apt-key. Either remove the apt-key paths entirely and ensure gpg is always available, or restore the runtime check so the existing branches behave as intended.
# apt-key add is deprecated starting from Ubuntu 2504.
# Use gpg to import the key instead.
apt_key_available = False
lisa/operating_system.py:1164
Major: This method logs normal control flow at ERROR level and includes a misspelled message (add_Repositroy). ERROR logs should be reserved for actual failures so passing runs don't produce misleading ERROR noise.
Minor: Adding repo keys is logged at ERROR level, which is noisy and makes it harder to spot real failures in logs.
if keys_location:
self._log.error("adding keys")
for key_location in keys_location:
self._log.error(f"Adding key {key_location}")
self.add_key(server_name=key_location)
lisa/microsoft/testsuites/azihsm/azihsm.py:50
Major: These are class-level flags/fields (testing_repo_added, packages_installed, etc.). They will be shared across all nodes and runs in the same Python process, which can cause repository/package setup to be skipped on subsequent nodes (or reused across unrelated test runs). Prefer instance state or compute idempotently per-node (e.g., check whether the repo file/package is present).
Major: @TestCaseMetadata is missing a requirement=... field. LISA test cases conventionally declare requirements at the case level (even if the suite already restricts OS), so scheduling/selection remains explicit and consistent.
This issue also appears in the following locations of the same file:
line 291
line 321
line 399
line 454
line 505
...and 1 more
""",
priority=0,
)
lisa/microsoft/testsuites/azihsm/azihsm.py:526
Major: The try/except/finally here hides the real failure details (exceptions are swallowed) while the assertion in finally still fails. Prefer letting node.execute enforce the expected exit code and assert on stdout so failures keep their original context.
Critical: add_repository swallows all exceptions from both apt_repo.add_repository() and the manual fallback, then continues as if the repo was added. This can leave the system in a partially configured state and make later package operations fail with unclear root cause. If the fallback is needed, write the repo entry robustly (avoid echo "..." quoting issues) and raise a clear exception if both mechanisms fail.
Minor: repo_file is now a required parameter for RPM-based add_repository, but it's currently unused. This is misleading for callers who are now required to provide a filename, and it can also hide naming collisions if yum-config-manager chooses a different filename than expected.
Major: @TestCaseMetadata is missing a requirement=... field. Declare it explicitly for consistency with other test suites and to make scheduling/selection deterministic.
""",
priority=0,
)
lisa/microsoft/testsuites/azihsm/azihsm.py:323
Major: @TestCaseMetadata is missing a requirement=... field. Declare it explicitly for consistency with other test suites and to make scheduling/selection deterministic.
""",
priority=0,
)
lisa/microsoft/testsuites/azihsm/azihsm.py:401
Major: @TestCaseMetadata is missing a requirement=... field. Declare it explicitly for consistency with other test suites and to make scheduling/selection deterministic.
""",
priority=0,
)
lisa/microsoft/testsuites/azihsm/azihsm.py:456
Major: @TestCaseMetadata is missing a requirement=... field. Declare it explicitly for consistency with other test suites and to make scheduling/selection deterministic.
""",
priority=0,
)
lisa/microsoft/testsuites/azihsm/azihsm.py:507
Major: @TestCaseMetadata is missing a requirement=... field. Declare it explicitly for consistency with other test suites and to make scheduling/selection deterministic.
""",
priority=0,
)
lisa/microsoft/testsuites/azihsm/azihsm.py:535
Major: @TestCaseMetadata is missing a requirement=... field. Declare it explicitly for consistency with other test suites and to make scheduling/selection deterministic.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (7)
Previously missed (4) — in code that hasn't changed since the last review.
lisa/operating_system.py:868
apt_key_available is always False (the runtime check was removed), so the apt-key branch is now dead code and add_key() will always try to use gpg. That can regress older Ubuntu/Debian images where gpg may not be installed yet, and it also makes the apt_key_available conditional misleading.
# apt-key add is deprecated starting from Ubuntu 2504.
# Use gpg to import the key instead.
apt_key_available = False
lisa/operating_system.py:1169
add_repository() currently logs normal control flow at ERROR level and includes a misspelled message (add_Repositroy). This will create noisy ERROR logs in healthy runs and makes searching for real errors harder.
self._log.error(f"in add_Repositroy({repo})")
self._initialize_package_installation()
if keys_location:
self._log.error("adding keys")
for key_location in keys_location:
lisa/microsoft/testsuites/azihsm/azihsm.py:526
test_run_azihsm_driver_tests doesn't validate the command's exit code, and the try/except won't catch a non-zero exit in the common case (Node.execute returns a result object). As written, a failing run can be reported only as "PASSED" string absence, with no stderr/context. Capture the result, assert exit_code == 0, and include stdout/stderr in the failure message.
try:
result = node.execute(
f"/usr/bin/azihsm/driver_tests {params}",
)
cmd_output = result.stdout.strip()
lisa/microsoft/testsuites/azihsm/azihsm.py:344
The timeout=5 value is a behavior-controlling magic number with no unit/context. Add an inline comment (or a named constant) so it's clear this is a seconds-based unload wait and easy to tune consistently.
timeout=5,
lisa/operating_system.py:1178
The fallback path in add_repository() catches a broad Exception, suppresses failures (logs "OOPS"/"Maybe done?" and continues), and writes the repo string via a shell-constructed echo | sudo tee command. This is unreliable (breaks on quotes), noisy/unprofessional, and it can silently produce an invalid sources.list entry (e.g. for ppa: repos) while still running apt-get update. If apt-add-repository fails, the code should either re-raise for unsupported repo formats or perform a safe, validated file write for deb ... entries and fail fast if that write fails.
try:
self._log.error("calling apt-add-repo")
apt_repo.add_repository(repo)
except Exception:
# apt-add-repository seems like the right tool to use here,
lisa/microsoft/testsuites/azihsm/azihsm.py:242
The PR description's Test Validation section lists test method names that don't exist in this suite (e.g. package_installation_tests, run_azihsm_driver_tests). The actual methods are named test_package_installation, test_run_azihsm_driver_tests, and test_run_azihsm_sdk_tests. This mismatch makes it hard to reproduce validation and can mislead future triage.
In test_run_azihsm_sdk_tests, the loop relies on exceptions to detect failures, but node.execute() doesn't raise on non-zero exit codes by default. This can misclassify failures and lose stderr context. Check result.exit_code explicitly and log stdout/stderr on failures so the final assertion is actionable.
for test in sdk_test_list:
log.info(f"Running {test}")
try:
result = node.execute(
f"/usr/bin/azihsm/{test} {params}",
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Repository signature verification and dmesg validation issues can cause security exposure and false-positive results.
Key Test Cases:
verify_package_installation|verify_azihsm_modinfo|verify_azihsm_module_load_unload|verify_azihsm_module_reload_cycles|verify_azihsm_package_uninstallation|verify_azihsm_driver_tests|verify_azihsm_sdk_tests
Replace the naive len(pre_lines)-offset slice in
verify_azihsm_module_load_unload with an anchor-based overlap search
(_new_dmesg_lines helper): it locates the last line of the pre-load
dmesg snapshot within the post-load snapshot and returns everything
after it. This stays correct even if the kernel ring buffer wraps and
drops older lines between the two snapshots, which would otherwise
shift offsets and could cause the slice-based approach to miss newly
emitted AZIHSM errors. Falls back to treating the whole post-load
snapshot as new if the anchor can no longer be found, erring on the
side of catching errors rather than missing them.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Dmesg errors can be missed, RPM repository key arguments are ineffective, and test-case requirements are incomplete.
Key Test Cases:
verify_package_installation|verify_azihsm_modinfo|verify_azihsm_module_load_unload|verify_azihsm_module_reload_cycles|verify_azihsm_package_uninstallation|verify_azihsm_driver_tests|verify_azihsm_sdk_tests
Minor: The test metadata says only rpm -V is exercised, but Ubuntu executes dpkg -V. Update the step description so the published case accurately documents both supported OS paths.
Exception omits failing test and diagnostic guidance
Minor: This exception message does not identify which SDK test failed or tell the reader how to investigate it. Include test and direct the reader to the diagnostics collected immediately afterward.
This issue also appears on line 925 of the same file.
A single repeated dmesg line is not a sufficiently unique anchor: if
the same text recurs later in the post-load snapshot (e.g. a generic
info message), matching its most recent occurrence could truncate
past, and hide, a genuinely new error line emitted between the real
boundary and the coincidental repeat (e.g. post = pre + ["azihsm
error", pre[-1]] previously returned no new lines).
_new_dmesg_lines now anchors on the last up-to-5 lines of the pre-load
snapshot and searches left-to-right for the first (earliest, and thus
chronologically correct) contiguous match in the post-load snapshot,
making a coincidental collision far less likely while remaining
correct under ring-buffer wraparound.
Major: On Ubuntu, dpkg -V reports checksum discrepancies as output while still returning exit code 0, so this assertion lets a corrupted package pass Test 4. Keep the command-success check and additionally require empty verification output for Ubuntu.
Minor: This node command embeds a remote path with hardcoded separators. Build the executable path with node.get_pure_path() so path handling follows the repository's cross-OS command convention.
This issue also appears in the following locations of the same file:
- Update verify_package_installation's test-case description to note
that step 4 uses rpm -V on CBLMariner but dpkg -V on Ubuntu, instead
of implying only rpm -V is exercised.
- Include the failing test name and point to the diagnostics logged
immediately afterward in the exception-caught log.error() messages
in verify_azihsm_sdk_tests, for both the main test loop and the
separate azihsm_api_cpp_tests block.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Package verification and dmesg boundary logic contain blocking defects, while validation metadata is incomplete.
Review effort: Balanced Findings: None
Previously missed (3)
In code that hasn't changed since last review
Use suffix-prefix overlap to identify newly added log lines
lisa/microsoft/testsuites/azihsm/azihsm.py:88
Major: Searching for the first five-line anchor can classify old messages as new when that sequence occurred earlier in the pre-load snapshot. A stale AZIHSM error is then included in error_lines, causing a false failure even though the load added no error. Compare the maximal suffix of pre_lines with the prefix of post_lines instead; this also handles ring-buffer truncation.
Validate package verification output, not only exit status
Major: dpkg -V reports checksum discrepancies on stdout without changing its exit status, so the Ubuntu branch currently passes even when package files fail verification. Validate that stdout is empty in addition to checking command execution; the same check is valid for rpm -V.
Align add_repository changes with the PR description
Major: The PR description says this change also updates/fixes add_repository() in operating_system.py, but the provided changes only call that API and do not modify its implementation. Either include the stated API fixes or remove that claim so the PR scope and review evidence agree.
- _new_dmesg_lines: replace the multi-line-anchor search (which could
misclassify old messages as new if that anchor sequence also
occurred earlier in the pre-load snapshot) with a suffix-of-pre /
prefix-of-post overlap match. This correctly models ring buffer
semantics (only ever drops from the front, appends at the end) and
anchors the overlap specifically at the start of "post", removing
the ambiguity of searching for the anchor at an arbitrary position.
- verify_package_installation: rpm -V / dpkg -V can report
discrepancies as lines on stdout without a non-zero exit code
(observed with dpkg -V on Ubuntu). Assert that stdout is empty in
addition to the exit code.
Minor: The diagnostic command embeds the device path directly. Convert AZIHSM_DEV with node.get_pure_path() before interpolating it into the remote command.
This issue also appears in the following locations of the same file:
Minor: 200 is an unexplained magic threshold controlling how much failure diagnostic output is retained. Add an inline explanation so future changes do not silently reduce useful diagnostics.
…mber
- Construct AZIHSM_DEV and the /usr/bin/azihsm/* test binary paths via
node.get_pure_path() before interpolating them into remote shell
commands, instead of embedding raw path strings directly.
- Add a new AZIHSM_BIN_DIR constant for the shared test binary directory.
- Apply the same treatment to /proc/modules in the module-state check.
- Add an inline comment explaining why tail_lines=200 was chosen for the
diagnostic dmesg capture in _log_azihsm_test_failure_diagnostics.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Major: AZIHSM_DISABLE_MULTI_PROCESS_TESTS does not disable the upstream test: the current Azure/azihsm-sdk source only mentions this name in an assertion message and never reads it, so the multi-process case still runs. If real hardware requires skipping it, pass a supported libtest filter or update/package the SDK to implement the flag instead of relying on this no-op environment variable.
Advertised OpenSSL engine coverage is not tested
lisa/microsoft/testsuites/azihsm/azihsm.py:47
Minor: The suite description promises OpenSSL engine verification, but this file only installs the engine package and never exercises it, so the advertised coverage is inaccurate. Remove this claim or add a corresponding engine test.
Adds verify_azihsm_openssl_engine, which installs the azihsm driver and
packages (including libengine-azihsm-openssl), locates OpenSSL's
modules directory dynamically (it differs across distros), confirms
the azihsm_provider.so is present there, and confirms the provider
loads and is reported active via a generated openssl.cnf.
Key generation (genpkey/storeutl) is intentionally not exercised: the
device's BK3 base key is one-shot per physical power cycle and was
confirmed (via live testing, including a full Azure VM deallocate/
start cycle) to persist at the hardware level independent of the VM/OS
disk, making a key-generation assertion flaky across repeated runs
rather than a reliable functional check.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Minor: A Modprobe tool already exists and is imported in this suite, so invoke it rather than issuing a raw modprobe command. This keeps command execution and caching behavior consistent with the other module checks.
- Use the Modprobe tool's module_exists() in the uninstall test (Test
17) instead of a raw modprobe node.execute() call.
- Use the OpenSSL tool for 'version -m' and 'list -providers' instead
of raw node.execute() calls in verify_azihsm_openssl_engine.
- Replace the bare assert narrowing modules_dir_match with a typing
cast, since the preceding assert_that() already validates it.
- Use the Rm/Mkdir tools instead of raw shell rm -rf/mkdir -p for the
scratch directory.
- Compose bmk/muk/openssl.cnf paths with the PurePath / operator
instead of string interpolation with a hardcoded separator.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The SDK test run does not actually disable the unsupported multi-process test, and per-case requirement metadata is missing.
Key Test Cases:
verify_package_installation|verify_azihsm_modinfo|verify_azihsm_module_load_unload|verify_azihsm_module_reload_cycles|verify_azihsm_package_uninstallation|verify_azihsm_driver_tests|verify_azihsm_sdk_tests|verify_azihsm_openssl_engine
Tool.run caches by command/flags but not update_envs, so without
force_run=True a rerun could reuse a stale list -providers result
instead of reflecting the freshly written OPENSSL_CONF.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Stop passing the Rust test-harness '--test-threads 1' option to
azihsm_api_cpp_tests, a separate C++ binary that does not
understand it and would reject its arguments.
- Anchor the modules-directory regex to the MODULESDIR: field in
'openssl version -m' output instead of matching the first quoted
string, which could incorrectly capture OPENSSLDIR and cause the
provider .so lookup to fail even when correctly installed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The AZIHSM device is Azure-specific, but this suite requirement only filters by OS. As a result, LISA can schedule it on non-Azure Ubuntu/CBLMariner environments, where the tests first add Microsoft repositories/install packages and only then skip when /dev/azihsm0 is absent. Add the established supported_platform_type=[AZURE] requirement (and import AZURE) so unsupported environments are filtered before setup.
ignore_error=True on Modprobe.remove() does not suppress rmmod
failures: the normal successful path already unloads the module, so
the finally block's unconditional remove() call would raise on the
already-unloaded module and mask the real test result. Guard the
cleanup call with a fresh is_module_loaded() check so it only runs
rmmod when the module is actually still loaded.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Re: Copilot's suggestion to restrict this suite to supported_platform_type=[AZURE] (review 5493862200, "Restrict AZIHSM tests to Azure environments"):
Declining this one. Adding the AZURE platform-type restriction would prevent the suite from being scheduled under the ready platform (e.g. our local validation runbook against pre-provisioned VMs), since that platform reports type ready, not azure — confirmed locally this drops test selection to 0. A ready-platform node can still have the required azihsm hardware/device, so gating on platform type isn't a valid proxy for hardware availability here. The suite already skips cleanly when /dev/azihsm0 is absent, which is the correct guard.
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
This PR adds a new LISA test suite for the AZIHSM device and driver (
lisa/microsoft/testsuites/azihsm/azihsm.py). All changes in this PR are scoped to that file; a separate pre-existing issue inadd_repository()(operating_system.py), discovered while writing these tests, is tracked in #4730 rather than fixed here.Related Issue
Type of Change
Checklist
Test Validation
Key Test Cases:
AziHsm.verify_package_installation
AziHsm.verify_azihsm_modinfo
AziHsm.verify_azihsm_module_load_unload
AziHsm.verify_azihsm_module_reload_cycles
AziHsm.verify_azihsm_package_uninstallation
AziHsm.verify_azihsm_driver_tests
AziHsm.verify_azihsm_openssl_engine
Impacted LISA Features:
This adds support for the AziHSM device
Tested Azure Marketplace Images:
azure-linux-3-gen2
ubuntu-24_04-lts
Test Results
Note: Failures are cause by the outdated FW which is currently deployed in the fleet. The tests are known to pass on the latest FW.