Skip to content

Conversation

@hholoubk
Copy link
Contributor

@hholoubk hholoubk commented Sep 30, 2025

  • Add shutdown test scenario to iommu_device_lifecycle.py
  • Create reusable VM lifecycle provider module (provider/vm_lifecycle/)
  • Refactor iommu_device_lifecycle.py for better code organization:
    • Extract inline logic into dedicated functions
    • Add check_network_access() for centralized network testing
    • Add test_save_suspend() for save/restore and suspend/resume
    • Add test_reboot_reset_shutdown() for lifecycle operations
    • Add setup_vm_xml() for VM configuration setup
  • Fix typo: sroiv_test_obj -> sriov_test_obj
  • Add support for optional VM restart after shutdown
  • Improve code maintainability and reduce duplication

This enables testing VM shutdown scenarios with vIOMMU devices and provides reusable lifecycle operations for other tests.

Summary by CodeRabbit

  • New Features

    • Added a shared lifecycle module with shutdown/reset/reboot operations and integrated network verification; SR‑IOV aware.
  • Refactor

    • Consolidated VM, IOMMU and SR‑IOV setup into a reusable setup flow and centralized scenario execution and teardown.
  • Tests

    • Expanded lifecycle scenarios (reboot/reset/shutdown, save/restore, suspend/resume), added an extra shutdown step, improved timeouts, connectivity checks and save-file cleanup.

@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025

Walkthrough

Adds VM lifecycle helpers and centralizes VM XML/IOMMU/SR-IOV setup in the vIOMMU test; introduces a new provider module for virsh-based shutdown/reset/reboot; refactors the test run flow to use new helpers and ensures unified teardown and save-file cleanup.

Changes

Cohort / File(s) Summary of Changes
vIOMMU lifecycle helpers & refactor
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py
Added helpers: check_network_access, test_save_suspend, test_reboot_reset_shutdown, setup_vm_xml. Replaced inline VM XML/IOMMU/SR-IOV setup and per-scenario blocks with these helpers, delegated scenario execution to them, imported provider.vm_lifecycle.lifecycle_base, and ensured teardown and save-file cleanup.
Provider lifecycle operations (new module)
provider/vm_lifecycle/lifecycle_base.py
New module adding VIRSH_ARGS and functions: test_shutdown(test, vm, params), test_reset(test, vm, login_timeout), test_reboot(test, vm, params, login_timeout) implementing virsh-based shutdown/reset/reboot with login/wait and basic error handling.
Test configuration update
libvirt/tests/cfg/sriov/vIOMMU/iommu_device_lifecycle.cfg
Added a shutdown step to the test_scenario variant (start_after_shutdown = yes, shutdown_timeout = 60), inserting an additional shutdown phase after reset.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Runner as Test Runner
  participant IOMMU as iommu_device_lifecycle
  participant LBase as provider.lifecycle_base
  participant Virsh as virsh
  participant Guest as Guest Serial

  Runner->>IOMMU: run(params)
  IOMMU->>IOMMU: setup_vm_xml(..., need_sriov)
  IOMMU->>Guest: open serial session / wait for login
  IOMMU->>IOMMU: check_network_access(...)

  alt reboot/reset/shutdown
    IOMMU->>LBase: test_reboot_reset_shutdown(...)
    LBase->>Guest: ensure login (pre)
    LBase->>Virsh: perform virsh.reset / virsh.shutdown or send reboot command
    LBase->>Guest: wait for login (post)
    LBase-->>IOMMU: return
    IOMMU->>IOMMU: check_network_access(...) post-action
  else save/restore or suspend/resume
    IOMMU->>Virsh: save/restore or suspend/resume operations
    IOMMU->>IOMMU: check_network_access(...) post-action
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hop through XML, I tweak a thread,
SR-IOV pins and IOMMU spread.
Reboots, saves, and gentle suspends,
Provider paws guide all their ends.
A rabbit cheers — the VM ascends 🐇✨

Pre-merge checks and finishing touches

✅ 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 accurately and concisely summarizes the primary changes—adding VM shutdown support and refactoring lifecycle operations—without unnecessary detail or jargon, making it clear what the pull request achieves.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 48561e9 and d270d00.

📒 Files selected for processing (3)
  • libvirt/tests/cfg/sriov/vIOMMU/iommu_device_lifecycle.cfg (1 hunks)
  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (2 hunks)
  • provider/vm_lifecycle/lifecycle_base.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libvirt/tests/cfg/sriov/vIOMMU/iommu_device_lifecycle.cfg
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.

Applied to files:

  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py
🧬 Code graph analysis (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (4)
provider/save/save_base.py (2)
  • pre_save_setup (11-36)
  • post_save_check (39-70)
provider/vm_lifecycle/lifecycle_base.py (3)
  • test_shutdown (19-33)
  • test_reset (36-49)
  • test_reboot (52-66)
provider/viommu/viommu_base.py (4)
  • setup_iommu_test (219-234)
  • prepare_controller (134-190)
  • update_disk_addr (192-217)
  • parse_iface_dict (112-132)
provider/sriov/sriov_base.py (2)
  • setup_iommu_test (396-420)
  • parse_iface_dict (173-195)
🪛 Ruff (0.13.3)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py

108-108: Use of possibly insecure function; consider using ast.literal_eval

(S307)


113-113: Use of possibly insecure function; consider using ast.literal_eval

(S307)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.8
🔇 Additional comments (8)
provider/vm_lifecycle/lifecycle_base.py (3)

15-33: LGTM! Clean shutdown implementation with proper timeout handling.

The shutdown function correctly uses utils_misc.wait_for to poll VM state and fails appropriately if the timeout is exceeded. The configurable shutdown_timeout parameter provides flexibility for different test scenarios.


36-49: LGTM! Reset operation correctly validates VM responsiveness.

The function properly establishes a serial session before issuing the reset command and verifies that the VM comes back up by waiting for the login prompt. The use of read_until_last_line_matches with the regex pattern ensures reliable detection of the login prompt.


52-66: LGTM! Reboot implementation uses guest-initiated approach.

The function correctly sends the reboot command from within the guest session, providing a realistic test scenario. The parameterized reboot_command allows flexibility for different guest operating systems.

libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (5)

19-35: LGTM! Centralized network access verification.

The function provides a clean abstraction for network connectivity checks, properly handling both SRIOV and non-SRIOV test scenarios. The error reporting includes both status code and output for debugging.


37-59: LGTM! Save/suspend operations with proper state verification.

The function correctly leverages the existing save_base helpers to validate VM state consistency across save/restore and suspend/resume operations. The use of serial console ensures reliable communication during state transitions.


61-94: LGTM! Comprehensive lifecycle operation handler with proper scenario routing.

The function effectively coordinates shutdown, reset, and reboot operations with appropriate network verification. The optional start_after_shutdown parameter provides flexibility for different test workflows.

Note: The previous issue where reboot_many_times incorrectly called test_reset has been properly fixed—it now calls test_reboot as intended (line 89).


96-130: LGTM! Well-structured VM configuration setup.

The function effectively centralizes VM XML configuration with clear separation between IOMMU setup, controller preparation, device modification, and interface configuration. The conditional SRIOV handling is clean and integrates well with the existing test framework.

Note: Address the eval() security issue flagged separately.


132-174: LGTM! Clean main test flow with proper resource management.

The refactored run() function demonstrates improved organization:

  • Centralized XML setup via setup_vm_xml()
  • Clear scenario routing to dedicated helper functions
  • Appropriate handling of unknown scenarios with a warning
  • Reliable cleanup in the finally block ensures proper teardown and save file removal

The refactoring successfully reduces code duplication and improves maintainability.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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 (4)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (4)

19-29: Consider removing unused 'vm' parameter.

The vm parameter is not used in the function body. If it's not needed for API consistency with other helpers, consider removing it to simplify the signature.

Apply this diff if the parameter is unnecessary:

-def check_network_access(test, vm, session, need_sriov, ping_dest):
+def check_network_access(test, session, need_sriov, ping_dest):
     """
     Check network access in VM
     """

Then update the call sites at lines 75 and 138 accordingly.


31-47: Consider removing unused 'params' parameter.

The params parameter is not used in the function body. Unless kept for API consistency, consider removing it to clean up the signature.

Apply this diff if the parameter is unnecessary:

-def test_save_suspend(test, vm, params, save_path, test_scenario):
+def test_save_suspend(test, vm, save_path, test_scenario):
     """
     Test save/restore or suspend/resume scenario
     """

Then update the call site at line 140 accordingly.


85-86: Consider ast.literal_eval for safer parameter parsing.

While eval() is common in test frameworks with trusted configuration, ast.literal_eval provides safer evaluation for dictionary literals and prevents arbitrary code execution.

If the parameter values are simple dictionary literals, apply this diff:

+from ast import literal_eval
+
 # ... later in the function
-    test_obj.setup_iommu_test(iommu_dict=eval(test_obj.params.get('iommu_dict', '{}')),
+    test_obj.setup_iommu_test(iommu_dict=literal_eval(test_obj.params.get('iommu_dict', '{}')),
                               cleanup_ifaces=cleanup_ifaces)

However, if the configuration uses complex Python expressions, eval() may be necessary. Based on learnings.


91-91: Consider ast.literal_eval for device dictionary evaluation.

Similar to the iommu_dict evaluation, consider using ast.literal_eval instead of eval() for safer parsing of device configuration dictionaries.

-        dev_dict = eval(test_obj.params.get('%s_dict' % dev, '{}'))
+        dev_dict = literal_eval(test_obj.params.get('%s_dict' % dev, '{}'))
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8266542 and f4c2b80.

📒 Files selected for processing (2)
  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (2 hunks)
  • provider/vm_lifecycle/lifecycle_base.py (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.

Applied to files:

  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py
🧬 Code graph analysis (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (4)
provider/save/save_base.py (2)
  • pre_save_setup (11-36)
  • post_save_check (39-70)
provider/vm_lifecycle/lifecycle_base.py (2)
  • test_shutdown (19-33)
  • test_reset (36-49)
provider/viommu/viommu_base.py (4)
  • setup_iommu_test (219-234)
  • prepare_controller (134-190)
  • update_disk_addr (192-217)
  • parse_iface_dict (112-132)
provider/sriov/sriov_base.py (3)
  • setup_iommu_test (396-420)
  • SRIOVTest (103-436)
  • parse_iface_dict (173-195)
🪛 Ruff (0.13.1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py

19-19: Unused function argument: vm

(ARG001)


31-31: Unused function argument: params

(ARG001)


85-85: Use of possibly insecure function; consider using ast.literal_eval

(S307)


91-91: Use of possibly insecure function; consider using ast.literal_eval

(S307)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.8
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.9
🔇 Additional comments (8)
provider/vm_lifecycle/lifecycle_base.py (4)

1-16: LGTM! Clean module setup.

The module structure, imports, and shared VIRSH_ARGS constant are well-organized for reusable lifecycle operations.


19-33: LGTM! Proper shutdown handling.

The shutdown flow correctly issues the virsh command, waits for VM death with a configurable timeout, and explicitly fails the test if shutdown doesn't complete.


36-49: LGTM! Correct reset flow.

The reset operation properly establishes a serial session, issues the reset command, and waits for the login prompt to confirm the VM has restarted.


52-66: LGTM! Proper reboot implementation.

The reboot function correctly sends a guest-initiated reboot command and waits for the login prompt, providing a complementary approach to the virsh-based reset.

libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (4)

14-14: LGTM! Appropriate import.

The lifecycle_base import is correctly added to support the refactored lifecycle operations.


69-71: Verify: Should reboot_many_times use test_reboot instead of test_reset?

The "reboot_many_times" scenario currently calls lifecycle_base.test_reset (virsh reset), which performs a hard reset. Based on the scenario name, this might be intentional for a more robust test. However, if the intent is to test guest-initiated reboots, consider using lifecycle_base.test_reboot instead.

If guest-initiated reboot is intended, apply this diff:

-    else:  # reboot_many_times
+    else:  # reboot_many_times
         for _ in range(int(params.get('loop_time', '5'))):
-            lifecycle_base.test_reset(test, vm, login_timeout)
+            lifecycle_base.test_reboot(test, vm, params, login_timeout)

98-103: LGTM! SR-IOV interface configuration is correct.

The SR-IOV test object setup and interface dictionary parsing properly handle the SR-IOV-specific network configuration requirements.


127-142: LGTM! Excellent refactoring for maintainability.

The refactored run function successfully delegates scenario-specific logic to dedicated helper functions, improving code organization and reusability. The defensive logging for unknown scenarios is a nice touch.

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4c2b80 and c95f7d9.

📒 Files selected for processing (1)
  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.

Applied to files:

  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py
🧬 Code graph analysis (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (4)
provider/save/save_base.py (2)
  • pre_save_setup (11-36)
  • post_save_check (39-70)
provider/vm_lifecycle/lifecycle_base.py (2)
  • test_shutdown (19-33)
  • test_reset (36-49)
provider/viommu/viommu_base.py (4)
  • setup_iommu_test (219-234)
  • prepare_controller (134-190)
  • update_disk_addr (192-217)
  • parse_iface_dict (112-132)
provider/sriov/sriov_base.py (2)
  • setup_iommu_test (396-420)
  • parse_iface_dict (173-195)
🪛 Ruff (0.13.1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py

108-108: Use of possibly insecure function; consider using ast.literal_eval

(S307)


113-113: Use of possibly insecure function; consider using ast.literal_eval

(S307)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.8
🔇 Additional comments (5)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (5)

19-34: LGTM! Clean abstraction for network verification.

The function properly delegates to SRIOV-specific checks when needed and falls back to a standard ping test otherwise. Error handling is appropriate.


37-58: LGTM! Clean encapsulation of save/suspend workflows.

The function properly delegates to existing save_base helpers and handles both save/restore and suspend/resume scenarios correctly. The use of serial connection for both pre and post checks is consistent.


96-130: LGTM! Well-structured VM XML setup consolidation.

The function successfully consolidates VM XML configuration logic, properly handling both SRIOV and non-SRIOV scenarios. The conditional interface cleanup and device modification follow established patterns in the codebase.

Note: The eval() usage on lines 108 and 113 is flagged by static analysis, but this pattern is consistent with the existing codebase (as seen in viommu_base.py and sriov_base.py). While ast.literal_eval() would be more secure, changing this would require a broader refactoring effort across the codebase.


132-174: Excellent refactoring! Much cleaner and more maintainable.

The run function is now significantly more readable with clear delegation to helper functions. Key improvements:

  • VM XML setup consolidated into setup_vm_xml()
  • Scenario routing is clean and explicit
  • Unknown scenarios are properly logged rather than silently ignored
  • Teardown remains in the finally block as per framework conventions

Based on learnings.


14-14: LGTM! Necessary import for lifecycle operations.

The new import provides the lifecycle helper functions used in test_reboot_reset_shutdown().

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

♻️ Duplicate comments (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (1)

87-89: Fix missing params argument in test_reboot call.

The loop calls lifecycle_base.test_reboot(test, vm, login_timeout) but the function signature requires test_reboot(test, vm, params, login_timeout) with params as the third argument. This will cause a runtime error.

Apply this diff to fix the function call:

     else:  # reboot_many_times
         for _ in range(int(params.get('loop_time', '5'))):
-            lifecycle_base.test_reboot(test, vm, login_timeout)
+            lifecycle_base.test_reboot(test, vm, params, login_timeout)
🧹 Nitpick comments (2)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (2)

108-109: Consider using ast.literal_eval for safer config parsing.

While eval() is consistent with existing patterns in this codebase, ast.literal_eval would be safer and prevent potential code injection if test parameters are ever sourced from untrusted inputs.

As per coding guidelines (Ruff S307 hint).

Apply this diff to use ast.literal_eval:

+import ast
+
 def setup_vm_xml(test, vm, test_obj, need_sriov=False, cleanup_ifaces=True):
     """
     Configure VM XML with IOMMU, device, and network interface settings.
 
     :param test: Test object for logging and assertions
     :param vm: VM object to configure
     :param test_obj: VIOMMU test object containing configuration parameters
     :param need_sriov: Boolean indicating if SRIOV network interfaces are required
     :param cleanup_ifaces: Boolean indicating if existing interfaces should be cleaned up
     :raises: TestFail if VM XML configuration fails
     """
     test.log.info("TEST_SETUP: Update VM XML.")
-    test_obj.setup_iommu_test(iommu_dict=eval(test_obj.params.get('iommu_dict', '{}')),
+    test_obj.setup_iommu_test(iommu_dict=ast.literal_eval(test_obj.params.get('iommu_dict', '{}')),
                               cleanup_ifaces=cleanup_ifaces)

113-113: Consider using ast.literal_eval for safer config parsing.

Similar to line 108, prefer ast.literal_eval over eval() for parsing device dictionary configurations.

As per coding guidelines (Ruff S307 hint).

Apply this diff:

     for dev in ["disk", "video"]:
-        dev_dict = eval(test_obj.params.get('%s_dict' % dev, '{}'))
+        dev_dict = ast.literal_eval(test_obj.params.get('%s_dict' % dev, '{}'))
         if dev == "disk":
             dev_dict = test_obj.update_disk_addr(dev_dict)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c95f7d9 and f62862a.

📒 Files selected for processing (1)
  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.

Applied to files:

  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py
🧬 Code graph analysis (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (4)
provider/save/save_base.py (2)
  • pre_save_setup (11-36)
  • post_save_check (39-70)
provider/vm_lifecycle/lifecycle_base.py (3)
  • test_shutdown (19-33)
  • test_reset (36-49)
  • test_reboot (52-66)
provider/viommu/viommu_base.py (4)
  • setup_iommu_test (219-234)
  • prepare_controller (134-190)
  • update_disk_addr (192-217)
  • parse_iface_dict (112-132)
provider/sriov/sriov_base.py (3)
  • setup_iommu_test (396-420)
  • SRIOVTest (103-436)
  • parse_iface_dict (173-195)
🪛 Ruff (0.13.1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py

108-108: Use of possibly insecure function; consider using ast.literal_eval

(S307)


113-113: Use of possibly insecure function; consider using ast.literal_eval

(S307)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.8
  • GitHub Check: Python 3.12
🔇 Additional comments (5)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (5)

14-14: LGTM!

The lifecycle_base import is properly added and used consistently throughout the refactored code.


19-34: LGTM!

The network access check is properly abstracted with clear separation between SRIOV and non-SRIOV scenarios. Error handling is appropriate.


37-58: LGTM!

The save/suspend helper correctly leverages existing save_base utilities and properly handles both save/restore and suspend/resume scenarios with appropriate pre/post checks.


96-130: LGTM with recommended improvements above.

The VM XML setup helper effectively consolidates previously inline configuration logic. The refactoring improves maintainability and reusability across different test scenarios.


157-170: LGTM!

The refactored run function effectively delegates to the new helper functions, improving code organization and maintainability. The warning for unknown scenarios is a good defensive practice.

@hholoubk hholoubk force-pushed the LIBVIRTAT-22036-add-VM-shutdown-with-lifecycle-provider branch 2 times, most recently from df39711 to 48561e9 Compare October 7, 2025 10:30
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: 1

♻️ Duplicate comments (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (1)

87-89: Fix missing params argument in test_reboot call.

The test_reboot function requires 4 arguments: test, vm, params, and login_timeout, but line 89 only passes 3 arguments. This will cause a TypeError at runtime.

Apply this diff:

     else:  # reboot_many_times
         for _ in range(int(params.get('loop_time', '5'))):
-            lifecycle_base.test_reboot(test, vm, login_timeout)
+            lifecycle_base.test_reboot(test, vm, params, login_timeout)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df39711 and 48561e9.

📒 Files selected for processing (3)
  • libvirt/tests/cfg/sriov/vIOMMU/iommu_device_lifecycle.cfg (1 hunks)
  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (2 hunks)
  • provider/vm_lifecycle/lifecycle_base.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libvirt/tests/cfg/sriov/vIOMMU/iommu_device_lifecycle.cfg
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.

Applied to files:

  • libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py
🧬 Code graph analysis (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (4)
provider/save/save_base.py (2)
  • pre_save_setup (11-36)
  • post_save_check (39-70)
provider/vm_lifecycle/lifecycle_base.py (3)
  • test_shutdown (19-33)
  • test_reset (36-49)
  • test_reboot (52-66)
provider/viommu/viommu_base.py (4)
  • setup_iommu_test (219-234)
  • prepare_controller (134-190)
  • update_disk_addr (192-217)
  • parse_iface_dict (112-132)
provider/sriov/sriov_base.py (3)
  • setup_iommu_test (396-420)
  • SRIOVTest (103-436)
  • parse_iface_dict (173-195)
🪛 Ruff (0.13.3)
libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py

108-108: Use of possibly insecure function; consider using ast.literal_eval

(S307)


113-113: Use of possibly insecure function; consider using ast.literal_eval

(S307)

🔇 Additional comments (6)
provider/vm_lifecycle/lifecycle_base.py (1)

1-66: LGTM: Well-structured lifecycle operations module.

The module provides clean, reusable VM lifecycle operations with proper logging, timeout handling, and session management. The functions follow consistent patterns and integrate well with the virttest framework.

libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py (5)

19-35: LGTM: Clean network verification abstraction.

The function properly delegates to SRIOV-specific checks when needed and falls back to a simple ping otherwise, with appropriate error handling.


37-59: LGTM: Proper save/suspend orchestration.

The function correctly uses save_base helpers for pre-save setup and post-restore validation, and handles both save/restore and suspend/resume scenarios appropriately with serial console operations.


61-86: LGTM: Shutdown and reset scenarios handled correctly.

The shutdown scenario properly handles the optional restart via start_after_shutdown parameter, and the reset scenario calls test_reset with the correct arguments. Network verification after lifecycle operations is appropriate.

Also applies to: 91-94


96-107: LGTM: VM XML configuration setup is well-structured.

The function properly orchestrates IOMMU setup, controller preparation, device modifications, and optional SRIOV interface configuration. The logic flow and conditional handling are appropriate.

Also applies to: 110-130


132-174: LGTM: Clean test orchestration with proper cleanup.

The main test function properly delegates to scenario-specific handlers, includes a fallback for unknown scenarios, and ensures cleanup in the finally block. The refactoring improves maintainability and testability.

- Add shutdown test scenario to iommu_device_lifecycle.py
- Create reusable VM lifecycle provider module (provider/vm_lifecycle/)
- Refactor iommu_device_lifecycle.py for better code organization:
  * Extract inline logic into dedicated functions
  * Add check_network_access() for centralized network testing
  * Add test_save_suspend() for save/restore and suspend/resume
  * Add test_reboot_reset_shutdown() for lifecycle operations
  * Add setup_vm_xml() for VM configuration setup
- Fix typo: sroiv_test_obj -> sriov_test_obj
- Add support for optional VM restart after shutdown
- Improve code maintainability and reduce duplication

This enables testing VM shutdown scenarios with vIOMMU devices
and provides reusable lifecycle operations for other tests.

- Add shutdown test scenario with start_after_shutdown and shutdown_timeout options
- Configure shutdown timeout to 60 seconds
- Enable VM restart after shutdown for comprehensive lifecycle testing

code assisted by Cursor IDE
msg generated by Cursor IDE
@hholoubk hholoubk force-pushed the LIBVIRTAT-22036-add-VM-shutdown-with-lifecycle-provider branch from 48561e9 to d270d00 Compare October 7, 2025 12:44
@hholoubk
Copy link
Contributor Author

hholoubk commented Oct 8, 2025

Hi @dzhengfy and @yalzhang could you please review this changes? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant