Skip to content

Conversation

@meinaLi
Copy link
Contributor

@meinaLi meinaLi commented Sep 4, 2025

Dracut command is used in guest to add the scsi driver. But in image mode it can't write to /boot/efi. Because actually this is not a necessary checkpoint in test scenario, so I think we can directly skip this step in case.

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability when running in image mode by skipping unnecessary system regeneration steps, preventing unintended side effects during multi-disk operations.
  • Tests

    • Enhanced stability of multi-disk testing by conditionally performing system updates only when appropriate.
    • Reduced noise in logs and minimized environment-specific flakiness by respecting image mode during test execution.

Dracut command is used in guest to add the scsi driver. But in image
mode it can't write to /boot/efi. Because actually this is not a
necessary checkpoint in test scenario, so I think we can directly skip
this step in case.

Signed-off-by: meinaLi <[email protected]>
@meinaLi meinaLi marked this pull request as draft September 4, 2025 11:56
@meinaLi
Copy link
Contributor Author

meinaLi commented Sep 4, 2025

This PR depends on avocado-framework/avocado-vt#4215.

@meinaLi
Copy link
Contributor Author

meinaLi commented Sep 4, 2025

# avocado run --vt-type libvirt --vt-omit-data-loss --vt-machine-type q35 virtual_disks.multidisks.coldplug.multi_disks_test.disk_virtio_scsi_multi_queue
JOB ID     : 6b360208c202172b16ad29da40f5835018d1b6cf
JOB LOG    : /var/log/avocado/job-results/job-2025-09-04T08.02-6b36020/job.log
 (1/1) type_specific.io-github-autotest-libvirt.virtual_disks.multidisks.coldplug.multi_disks_test.disk_virtio_scsi_multi_queue: STARTED
 (1/1) type_specific.io-github-autotest-libvirt.virtual_disks.multidisks.coldplug.multi_disks_test.disk_virtio_scsi_multi_queue: PASS (177.24 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML   : /var/log/avocado/job-results/job-2025-09-04T08.02-6b36020/results.html
JOB TIME   : 180.67 s

@meinaLi meinaLi marked this pull request as ready for review September 10, 2025 07:51
@coderabbitai
Copy link

coderabbitai bot commented Sep 10, 2025

Walkthrough

Adds a guard around invoking dracut in virtual_disks_multidisks.py by checking image mode via utils_sys.is_image_mode(session=...). dracut runs only when not in image mode. Introduces new import for utils_sys. No other logic or interfaces changed.

Changes

Cohort / File(s) Summary
Libvirt virtual disks test adjustments
libvirt/tests/src/virtual_disks/virtual_disks_multidisks.py
Import utils_sys; compute image_mode = utils_sys.is_image_mode(session=session); conditionally run dracut only if not image_mode; retain existing error handling and flow otherwise.

Sequence Diagram(s)

sequenceDiagram
    participant Test as Test Case
    participant Session as Guest Session
    participant Utils as utils_sys
    participant System as System (dracut)

    Test->>Utils: is_image_mode(session)
    Utils-->>Test: image_mode (True/False)

    alt add_disk_driver and not image_mode
        Test->>System: run dracut to rebuild initramfs
        System-->>Test: dracut result
    else add_disk_driver and image_mode
        Note over Test: Skip dracut in image mode
    end

    Note over Test: Continue with existing verification logic
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Virtual_disk: fix virtio_scsi driver issue in image mode” succinctly captures the core change of gating the dracut invocation to address the virtio SCSI driver failure in image mode, clearly reflecting the main intent without unnecessary detail.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Poem

I twitched my ears at a guarded call,
“Dracut, wait—are we image at all?”
If yes, I hop and let it be,
If no, rebuild initrd with glee.
One careful check, a lighter leap—
Logs stay calm, and kernels sleep. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly identifies the main change by specifying a fix for the virtio_scsi driver issue in image mode, which aligns with the PR’s objective of conditionally skipping the dracut step when running in image mode. It is concise, descriptive, and directly related to the core modification in the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
libvirt/tests/src/virtual_disks/virtual_disks_multidisks.py (2)

24-24: New dependency on utils_sys.is_image_mode: add a graceful fallback or enforce via explicit dependency check

If CI/dev envs pick an older avocado-vt without utils_sys.is_image_mode, this import/call will break the test at runtime. Either add a defensive fallback (recommended for robustness) or fail fast with a clear message when add_disk_driver is used.

Proposed defensive import:

-from virttest import utils_sys
+try:
+    from virttest import utils_sys  # requires avocado-vt PR 4215+
+except Exception:
+    utils_sys = None  # fallback for older environments

And guard the usage (paired with the block below):

-            image_mode = utils_sys.is_image_mode(session=session)
+            image_mode = False
+            if utils_sys and hasattr(utils_sys, "is_image_mode"):
+                image_mode = utils_sys.is_image_mode(session=session)
+            else:
+                logging.debug("utils_sys.is_image_mode unavailable; assuming non-image mode.")

1098-1107: Gate zipl together with dracut in image mode, and truly ignore dracut failures

  • If image mode skips dracut, zipl on s390x likely isn’t needed and may also fail.
  • The comment says “Ignore errors here”, but session.cmd will raise on non-zero exit. Catch and log instead.
         if add_disk_driver:
             # Ignore errors here
-            image_mode = utils_sys.is_image_mode(session=session)
-            if not image_mode:
-                session.cmd("dracut --force --add-drivers '%s'"
-                            % add_disk_driver, timeout=360)
-            # In terms of s390x, additional step is needed for normal guest
-            # boot, see https://bugzilla.redhat.com/show_bug.cgi?id=2214147
-            if arch == 's390x':
-                session.cmd("zipl")
+            # image mode cannot write to /boot/efi; skip initramfs/bootloader work there
+            # (fallback guard if utils_sys/is_image_mode is unavailable is added above)
+            if image_mode:
+                logging.debug("Image mode detected; skipping dracut and zipl.")
+            else:
+                try:
+                    session.cmd("dracut --force --add-drivers '%s'" % add_disk_driver, timeout=360)
+                except aexpect.ShellCmdError as e:
+                    logging.debug("dracut failed (ignored): %s", e)
+                # On s390x, zipl is required only when we actually rebuilt initramfs.
+                # https://bugzilla.redhat.com/show_bug.cgi?id=2214147
+                if arch == 's390x':
+                    session.cmd("zipl")
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23e29d6 and 08887c0.

📒 Files selected for processing (1)
  • libvirt/tests/src/virtual_disks/virtual_disks_multidisks.py (2 hunks)

Copy link
Contributor

@chunfuwen chunfuwen left a comment

Choose a reason for hiding this comment

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

lgtm

@chunfuwen chunfuwen added minorfix depend on The PR has dependency on other PRs labels Oct 9, 2025
@chunfuwen chunfuwen merged commit fb4decf into autotest:master Oct 9, 2025
6 checks passed
@meinaLi meinaLi deleted the multidisks_scsi branch October 9, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

depend on The PR has dependency on other PRs minorfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants