utils_libvirt/libvirt_service: wait for service to terminate on kill

[smitterl]

The function only issued the kill command but didn't wait for it to become effective. This lead to timing issues with test steps afterwards, possibly requiring invocation of sleep with absolute times.

Instead, now wait for a set timeout for the process to terminate. If it doesn't, log this for better debugging later.

Summary by CodeRabbit

• Bug Fixes
  • Service termination operations now verify the process has fully stopped before continuing, rather than returning immediately after the kill command.
  • Added configurable timeout for termination verification (default 30 seconds).
  • Enhanced reliability for both local and remote service management scenarios with improved logging.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[smitterl]

JOB ID     : 88425ab7893317c839d0029b5e351ea74364ed2b
JOB LOG    : /var/log/avocado/job-results/job-2025-12-22T11.16-88425ab/job.log
 (1/1) type_specific.io-github-autotest-libvirt.migration.pause_postcopy_migration_and_recover.unattended_migration_and_disruptive.pause_by_network.non_p2p: STARTED
 (1/1) type_specific.io-github-autotest-libvirt.migration.pause_postcopy_migration_and_recover.unattended_migration_and_disruptive.pause_by_network.non_p2p: PASS (343.13 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-12-22T11.16-88425ab/results.html
JOB TIME   : 344.60 s

[coderabbitai]

Walkthrough

A new libvirt service termination timeout configuration is introduced in the base configuration file with a default value of 30 seconds. The libvirt service module is updated to add a verification mechanism that confirms a service process has been fully terminated after issuing a kill command, rather than returning immediately. The verification uses process checking (pidof) and supports both local and remote execution contexts, with logging for success or warning cases when the process remains running.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• The _check_service_gone helper method implementation and how it handles both local and remote contexts
• Integration of the timeout configuration into the service killing workflow
• Edge cases around timeout expiration and persistent process scenarios
• Impact on the overall kill_service control flow and service lifecycle management

Pre-merge checks

✅ 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 clearly and specifically describes the main change: adding wait behavior to the service kill function to ensure termination before returning.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

virttest/utils_libvirt/libvirt_service.py (1)

64-68: Verification logic is sound; consider explicit type conversion.

The termination verification correctly uses wait_for to poll for service termination, with appropriate logging for both success and warning cases. The wait_for function internally converts the timeout to float, so string values from config will work.

💡 Optional improvements for clarity and robustness

Consider explicitly converting the timeout parameter to an int or float for better code clarity:

-    timeout = params.get("libvirt_kill_service_timeout", 30)
+    timeout = int(params.get("libvirt_kill_service_timeout", 30))

Additionally, you could return a boolean to indicate success/failure for programmatic checking by callers:

     if not utils_misc.wait_for(_check_service_gone, timeout=timeout, step=0.5):
         LOG.warning("Service %s may still be running after kill", service_name)
+        return False
     else:
         LOG.debug("Service %s confirmed terminated", service_name)
+        return True

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f33c6e1 and 0a1f801.

📒 Files selected for processing (2)

• virttest/shared/cfg/base.cfg
• virttest/utils_libvirt/libvirt_service.py

🧰 Additional context used

🧬 Code graph analysis (1)

virttest/utils_libvirt/libvirt_service.py (2)

virttest/remote.py (1)

• run_remote_cmd (112-151)

virttest/utils_misc.py (3)

• run (118-135)
• wait_for (564-599)
• wait_for (4193-4241)

🪛 Ruff (0.14.10)

virttest/utils_libvirt/libvirt_service.py

61-61: Function call with shell=True parameter identified, security issue

(S604)

🔇 Additional comments (4)

virttest/shared/cfg/base.cfg (1)

867-869: LGTM! Clear configuration addition.

The new timeout parameter is well-documented and has a reasonable default value of 30 seconds for waiting for service termination after a kill operation.

virttest/utils_libvirt/libvirt_service.py (3)

5-5: LGTM! Import addition is appropriate.

The utils_misc import is necessary for the wait_for function used in the verification logic.

---

54-62: Note: Static analysis flags shell=True as potential security risk.

The static analysis tool flagged line 61 for using shell=True with string formatting. While this is typically a security concern for command injection, in this test framework context where service_name comes from configuration files (not runtime user input), the risk is minimal.

However, if there's ever a possibility of external input influencing service_name, consider using shell=False with proper argument list construction.

Current implementation is acceptable for the test framework use case, but document if service_name values are strictly controlled.

---

54-62: pidof is appropriate for this Linux-specific testing framework, though pgrep may be more portable.

The code correctly uses pidof to check process termination, which is standard on supported platforms (RHEL, Fedora). If broader Linux compatibility is desired, pgrep is a robust alternative with wider availability across distributions.

[nanli1]

This is useful.