Crucial fixes for xfstests and kernel_source_packager code #4526
Crucial fixes for xfstests and kernel_source_packager code #4526psachdeva-ms wants to merge 7 commits into
Conversation
The data plane SDK now natively supports provisioned_iops and provisioned_bandwidth_mibps parameters in create_share() API. Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com> Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR improves LISA’s Azure File Share xfstests execution by parallelizing xfstests runs (with a safer “parallel execute → sequential collect” flow to avoid SSH/paramiko contention), and includes related robustness updates in kernel source worktree handling and Azure file share provisioning.
Changes:
- Add a two-phase parallel execution model for xfstests workers, collecting results sequentially to avoid SSH channel deadlocks.
- Update kernel repo worktree checkout logic (tag-aware checkout + detached worktree cleanup).
- Add Azure Files share creation fallback when PV2 headers aren’t supported, and bump
azure-storage-file-sharedependency.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Bumps azure-storage-file-share dependency version. |
| lisa/transformers/kernel_source_packager.py | Adds worktree cleanup and tag-aware ref checkout logic for kernel source packaging. |
| lisa/transformers/kernel_source_installer.py | Adjusts ccache directory handling for kernel builds. |
| lisa/tools/git.py | Updates worktree_remove parameter type to align with worktree listing output usage. |
| lisa/sut_orchestrator/azure/common.py | Adds PV2-parameter fallback logic when creating Azure file shares. |
| lisa/microsoft/testsuites/xfstests/xfstests.py | Implements parallel xfstests execution with sequential result collection and related refactors. |
| lisa/microsoft/testsuites/xfstests/xfstesting.py | Removes now-unneeded deferred notification calls after parallel execution changes. |
| result = self._node.execute( | ||
| f"git tag -l {ref}", | ||
| shell=True, | ||
| cwd=target_path, | ||
| ) | ||
| return bool(result.stdout.strip()) |
|
@psachdeva-ms please fix the comments, thanks, |
- Add error handling for PV2 unsupported storage accounts - Automatically retry without provisioned_iops/provisioned_bandwidth_mibps when UnsupportedHeader error occurs - Increase quota to 100 GiB minimum when falling back to non-PV2 (PV1 minimum requirement) Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com> Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
lisa/microsoft/testsuites/xfstests/xfstests.py:836
- Major: run_test() waits via pgrep.wait_processes("check"), which can match unrelated processes and can also raise on timeout before the xfstests failure summary is raised. Since you already have the async Process handle, wait on that specific process (with raise_on_timeout=False) and then collect results.
pgrep = self.node.tools[Pgrep]
# this is the actual process name, when xfstests runs.
# monitor till process completes or timesout
try:
pgrep.wait_processes("check", timeout=timeout)
lisa/transformers/kernel_source_packager.py:434
- Major: _is_tag() uses
git tag -l {ref}which performs pattern matching (so a ref like "v1" can match multiple tags) and is executed with shell=True without quoting. Prefer a verification-style command and avoid shell parsing so tag detection is exact and robust.
result = self._node.execute(
f"git tag -l {ref}",
shell=True,
cwd=target_path,
)
return bool(result.stdout.strip())
| """ | ||
| Result object returned by run_test() to support parallel execution. | ||
| Instead of raising immediately on failure, this allows callers to | ||
| aggregate results from multiple parallel runs before deciding how to fail. | ||
|
|
||
| For parallel execution, raw_message and data_disk are stored to enable | ||
| deferred notification sending after all workers complete. This prevents | ||
| deadlock when multiple workers try to send SubTestResult notifications | ||
| concurrently to the single-threaded notifier message manager. | ||
| """ |
| if runbook.worktree_repo: | ||
| assert runbook.worktree_repo == git.remote_get_url( | ||
| code_path, remote | ||
| ), f"Existing remote url doesn't match with {runbook.worktree_repo}" |
| else: | ||
| assert runbook.worktree_repo, "Remote can not be added without a URL" | ||
| remote = runbook.worktree_name | ||
| self._log.info(f"adding remote {remote} for {runbook.worktree_repo}") | ||
| git.remote_add(cwd=code_path, name=remote, url=runbook.worktree_repo) |
| if not git.worktree_exists(cwd=code_path, path=str(worktree_path)): | ||
| assert runbook.worktree_ref, "Worktree ref needs to be set by user" | ||
| self._log.info( |
| except HttpResponseError as e: | ||
| # Handle PV2 parameters unsupported by the storage account | ||
| if "UnsupportedHeader" in str(e) and ( | ||
| "x-ms-share-provisioned-iops" in str(e) | ||
| or "x-ms-share-provisioned-bandwidth" in str(e) | ||
| ): |
| self._node.execute( | ||
| f"git checkout -f {expected_local_branch_name}", | ||
| shell=True, | ||
| cwd=target_path, | ||
| expected_exit_code=0, | ||
| ) |
| self._node.execute( | ||
| f"git branch --list '{worktree_name}-*' | xargs git branch -D", | ||
| shell=True, | ||
| no_error_log=True, | ||
| cwd=code_path, | ||
| ) |
Parallel xfstests workers deadlocked during result collection when multiple threads simultaneously made rapid SSH calls (shell.exists, Cat.run, copy_back) competing for paramiko's transport lock. Workers would hang for hours until timeout. Fix by splitting parallel execution into two phases: - Phase 1: Execute xfstests across all workers in parallel via _execute_test(), which issues a single long-running check command per worker (no rapid SSH-call churn, no deadlock). - Phase 2: Collect results sequentially via check_test_results(), avoiding the rapid SSH channel churn that triggers the deadlock. Because Phase 2 runs on the main thread one worker at a time, subtest notifications are now sent inline during collection. This removes the need for the deferred-notification path, so the following are deleted: - XfstestsParallelRunner.send_deferred_notifications() - XfstestsRunResult.raw_message / data_disk fields - the send_notifications parameter on check_test_results() Subtests are now tagged with the filesystem section (cifs/nfs) instead of the per-worker id (cifs_worker_N), matching non-parallel runs. Also simplify run_test() back to the original run_async + pgrep pattern for single-worker filesystem tests (btrfs, ext4), keeping the parallel-specific _execute_test() as a private method used only by XfstestsParallelRunner.run_parallel(). Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com> Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com> Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
- Add function to support deleting detached worktrees and linked branches. - Modularize the code by moving checkout of target ref in a method. - Don't let the user set a local tracking branch for the worktree, rather use a pattern: "<remote_name>-<target_ref>" Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com> Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com> Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
Signed-off-by: Piyush Sachdeva <psachdeva@microsoft.com> Signed-off-by: Piyush Sachdeva <s.piyush1024@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (5)
lisa/transformers/kernel_source_packager.py:563
- Major: Avoid using
assertfor runbook/remote validation. Assertions are skipped when Python runs with optimizations (-O), which would silently bypass these safety checks and can lead to using the wrong remote URL or trying to add a remote without a URL. Raise LisaException (or ValueError) instead so validation is enforced in all runs.
if runbook.worktree_name:
if runbook.worktree_name in remotes:
self._log.debug("Setting the remote based on worktree name/path.")
remote = runbook.worktree_name
if runbook.worktree_repo:
assert runbook.worktree_repo == git.remote_get_url(
code_path, remote
), f"Existing remote url doesn't match with {runbook.worktree_repo}"
elif runbook.worktree_name == repo_name:
self._log.debug("Using the upstream repo pointed by 'origin' remote")
else:
assert runbook.worktree_repo, "Remote can not be added without a URL"
remote = runbook.worktree_name
self._log.info(f"adding remote {remote} for {runbook.worktree_repo}")
git.remote_add(cwd=code_path, name=remote, url=runbook.worktree_repo)
lisa/transformers/kernel_source_packager.py:434
- Major:
_is_tag()builds a shell command with an unquoted, user-controlledrefand usesgit tag -l <pattern>, which treats the input as a glob pattern. This can mis-detect tags (e.g.,v*) and is vulnerable to shell metacharacters. Prefer an exact ref verification viagit show-ref --tags --verifyand avoidshell=True.
def _is_tag(
self,
target_path: PurePath,
ref: str,
) -> bool:
result = self._node.execute(
f"git tag -l {ref}",
shell=True,
cwd=target_path,
)
return bool(result.stdout.strip())
lisa/transformers/kernel_source_packager.py:415
- Minor:
xargs git branch -Dwill rungit branch -Deven when no branches match, causing a non-zero exit and forcing the code into the exception path. Usingxargs -ravoids the spurious failure and keeps cleanup best-effort without unnecessary exceptions.
self._node.execute(
f"git branch --list '{worktree_name}-*' | xargs git branch -D",
shell=True,
no_error_log=True,
cwd=code_path,
)
lisa/microsoft/testsuites/xfstests/xfstests.py:846
- Major:
run_test()waits for completion viapgrep.wait_processes("check"), which can match unrelated processes named "check" and either return early or hang until timeout. Sincerun_async()already returns a Process handle, prefer waiting on that specific process (process.wait_result(...)) to avoid false matches and reduce extra SSH calls.
# run ./check command
process = self.run_async(
cmd,
sudo=True,
shell=True,
force_run=True,
cwd=self.get_xfstests_path(),
)
pgrep = self.node.tools[Pgrep]
# this is the actual process name, when xfstests runs.
# monitor till process completes or timesout
try:
pgrep.wait_processes("check", timeout=timeout)
finally:
# On timeout wait_processes() only raises; the check process keeps
# running and holds the test/scratch mounts, wedging after_case
# cleanup. Kill the async handle so the mounts are free before we
# collect results.
if process.is_running():
process.kill()
run_result = self.check_test_results(
lisa/sut_orchestrator/azure/common.py:2399
- Minor: The PV2 fallback detection relies on repeated
str(e)substring checks, which is brittle (message formats can change across azure-core/azure-storage versions). Prefer checking the service error code from the response (e.g.,x-ms-error-code) and computemessage = str(e)once for the header-name checks.
except HttpResponseError as e:
# Handle PV2 parameters unsupported by the storage account
if "UnsupportedHeader" in str(e) and (
"x-ms-share-provisioned-iops" in str(e)
or "x-ms-share-provisioned-bandwidth" in str(e)
):
| make_args = "" | ||
| if use_ccache: | ||
| make_args = "CC='ccache gcc'" | ||
| node.execute( | ||
| cmd=f"export CCACHE_DIR={str(code_path.parent)}/.ccache", | ||
| shell=True, | ||
| no_error_log=True, | ||
| ) | ||
| ccache_dir = code_path.parent / ".ccache" | ||
| if not node.shell.exists(ccache_dir): | ||
| node.execute(f"mkdir -p {ccache_dir}", sudo=True) | ||
| node.execute(f"chmod 0777 {ccache_dir}", sudo=True) | ||
|
|
||
| make_args = f"CC='ccache gcc' CCACHE_DIR={str(ccache_dir)}" | ||
|
|
||
| # set timeout to 2 hours | ||
| make.make(arguments=make_args, cwd=code_path, timeout=60 * 60 * 2) |
| combined_cmd = ( | ||
| f"rm -rf {worker_path} && " | ||
| f"mkdir -p {base_dir} && " | ||
| f"cp -a {source_path} {worker_path} && " | ||
| f"chmod -R a+rwx {worker_path}" | ||
| ) |
| self._node.execute( | ||
| f"git fetch -p {remote} --force --tags", | ||
| shell=True, | ||
| no_info_log=False, | ||
| cwd=code_path, | ||
| remote=remote, | ||
| expected_exit_code=0, | ||
| ) |
Changes are put in the order of importance
Type of Change
Checklist
Test Validation
Key Test Cases:
verify_azure_file_share|verify_azure_file_share_nfsv4
Impacted LISA Features:
AzureFileShare
Tested Azure Marketplace Images:
Test Results