Skip to content

[AI Generated] BugFix: Add retry loop for LUN detection in hot_add_disk_serial test #4436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paxue wants to merge 1 commit into
base: main
Choose a base branch
from
+30 −4
Open

[AI Generated] BugFix: Add retry loop for LUN detection in hot_add_disk_serial test #4436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions lisa/microsoft/testsuites/core/storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
BadEnvironmentStateException,
LisaException,
SkippedException,
check_till_timeout,
constants,
generate_random_chars,
get_matched_str,
Expand Down Expand Up @@ -846,11 +847,36 @@ def _hot_add_disk_serial(
).is_equal_to(size)

# verify the lun number from linux VM
# Retry to allow udev to create the LUN symlink after disk attach
linux_device_luns_after = disk.get_luns()
linux_device_lun_diff = [
linux_device_luns_after[k]
for k in set(linux_device_luns_after) - set(linux_device_luns)
][0]

def _new_lun_detected(
_baseline: dict[str, int] = linux_device_luns,
) -> bool:
Comment on lines 849 to +855
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider linking the related issue/bug for traceability, since this PR is a bug fix (helps future debugging and release notes).

Copilot uses AI. Check for mistakes.
nonlocal linux_device_luns_after
linux_device_luns_after = disk.get_luns()
new_keys = set(linux_device_luns_after) - set(_baseline)
return len(new_keys) > 0

# 30s matches the disk-detection timeout used in
# _hot_add_disk_parallel for partition appearance after attach.
check_till_timeout(
_new_lun_detected,
timeout_message=(
f"new LUN not detected after disk attach at lun {lun}, "
f"luns_before: {linux_device_luns}, "
f"luns_after: {linux_device_luns_after}"
),
timeout=30,
interval=1,
)
Comment on lines +863 to +872
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout_message is built from an f-string at call time, so luns_after: {linux_device_luns_after} will always reflect the initial value (before retries), not the final state when the timeout occurs. Consider catching LisaTimeoutException from check_till_timeout and re-raising with a message constructed after the loop (using the last linux_device_luns_after) so the failure output is accurate for debugging.

Copilot uses AI. Check for mistakes.
Comment on lines +869 to +872
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interval=1 is a test-behavior magic number. Please add a short inline comment explaining why 1s is appropriate here (or use an existing constant) to make the retry/polling behavior easier to maintain.

Copilot generated this review using guidance from repository custom instructions.
new_lun_keys: list[str] = list(
set(linux_device_luns_after) - set(linux_device_luns)
)
Comment on lines +853 to +875
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new type hints use PEP 585 built-in generics (dict[str, int], list[str]), which are not supported on Python 3.8 unless the module has from __future__ import annotations. Please either add the future import at the top of this file or switch these annotations to typing.Dict/typing.List to keep the test suite compatible with the repo’s py38 target.

Copilot uses AI. Check for mistakes.
assert_that(new_lun_keys, "Expected exactly one new LUN device").is_length(
1
)
linux_device_lun_diff = linux_device_luns_after[new_lun_keys[0]]
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_lun_keys is derived from a set difference, so its ordering is arbitrary. Indexing with [0] makes the selection non-deterministic if more than one key ever appears in the diff. Consider selecting the new entry by the expected lun value (and/or asserting exactly one new device was detected) to avoid potential flakiness.

Copilot uses AI. Check for mistakes.
log.debug(f"linux_device_luns: {linux_device_luns}")
log.debug(f"linux_device_luns_after: {linux_device_luns_after}")
log.debug(f"linux_device_lun_diff: {linux_device_lun_diff}")
Expand Down
Loading