Allow for configurable extra vm dump checks for present/absent messages #4265
Conversation
WalkthroughThe patch modifies virttest/env_process.py. It adds an alias import Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
virttest/env_process.py (1)
685-690: Replace bareexcept:with specific exception handling.The bare
except:clause catches all exceptions includingSystemExitandKeyboardInterrupt, which can mask critical errors and make debugging difficult.Apply this diff:
- except: + except Exception as e: LOG.error( - "Could not copy the extra dump '%s' from the vm '%s'", + "Could not copy the extra dump '%s' from the vm '%s': %s", dump_path, vm.name, + e, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (3)
virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/guest_agent.py (1)
cmd(412-444)virttest/qemu_monitor.py (2)
cmd(931-981)cmd(2033-2093)
🪛 GitHub Actions: CI
virttest/env_process.py
[error] 1-1: Black formatting would reformat 1 file. Run 'black' to format and re-run 'make check'.
🪛 Ruff (0.14.4)
virttest/env_process.py
694-696: Avoid specifying long messages outside the exception class
(TRY003)
699-701: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
virttest/env_process.py (1)
682-682: Good change to useget_listfor parsing the dump paths.The switch from string splitting to
params.get_list()with a delimiter is a cleaner approach and consistent with the codebase patterns.
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
a1c8ff6 to
6184f30
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
virttest/env_process.py (1)
672-706: Fix shell injection risk and incorrect grep/exit-status handling in dump verificationThe current verification logic has multiple critical issues:
messageanddump_pathare interpolated directly into shell commands (Lines 694 and 702), so any spaces, metacharacters, or user-controlled content can break grep or allow shell injection.- For unexpected messages (Lines 699-703),
vm.session.cmd()is used but then treated as if it returned an exit status; instead it returns command output and may raise on non‑zero status.- The
grep -q -v {message}usage andstatus != 0check implement the wrong semantics: it will generally succeed as long as there is at least one line without the pattern, instead of failing when the unexpected pattern is present.You can address all of these in one go by (1) using
utils_misc.shell_escape()for both arguments, and (2) usingcmd_status()with plaingrep -qand checking the exit code appropriately:- for message in params.get_list( - f"expected_vm_extra_dump_{i}", delimiter=";" - ): - status = vm.session.cmd_status(f"grep -q {message} {dump_path}") - if status != 0: - raise exceptions.TestFail( - f"Missing expected message {message} in {vm.name} extra dump {dump_path}" - ) - for message in params.get_list( - f"unexpected_vm_extra_dump_{i}", delimiter=";" - ): - status = vm.session.cmd(f"grep -q -v {message} {dump_path}") - if status != 0: - raise exceptions.TestFail( - f"Redundant unexpected message {message} in {vm.name} extra dump {dump_path}" - ) + for message in params.get_list( + f"expected_vm_extra_dump_{i}", delimiter=";" + ): + cmd = "grep -q %s %s" % ( + utils_misc.shell_escape(message), + utils_misc.shell_escape(dump_path), + ) + status = vm.session.cmd_status(cmd) + if status != 0: + raise exceptions.TestFail( + f"Missing expected message '{message}' in {vm.name} extra dump {dump_path}" + ) + + for message in params.get_list( + f"unexpected_vm_extra_dump_{i}", delimiter=";" + ): + cmd = "grep -q %s %s" % ( + utils_misc.shell_escape(message), + utils_misc.shell_escape(dump_path), + ) + status = vm.session.cmd_status(cmd) + if status == 0: + raise exceptions.TestFail( + f"Found unexpected message '{message}' in {vm.name} extra dump {dump_path}" + )This keeps the behavior clear and robust: exit status 0 = match; non‑zero = no match/error, and all arguments are properly escaped.
🧹 Nitpick comments (1)
virttest/env_process.py (1)
708-716: Avoid swallowing all exceptions when closing remote sessionsThe
try/except Exception: passaroundvm.remote_sessionscleanup (Lines 709-714) hides any real problems and triggers the BLE001/S110 lints. It’s cheap to make this more robust while still being non‑critical for the test flow.Consider logging at least at debug level and ensuring the session is removed from the list even on failure:
- # Close all SSH sessions that might be active to this VM - for s in vm.remote_sessions[:]: - try: - s.close() - vm.remote_sessions.remove(s) - except Exception: - pass - - utils_logfile.close_log_file() + # Close all SSH sessions that might be active to this VM + for s in vm.remote_sessions[:]: + try: + s.close() + except Exception as exc: + LOG.debug( + "Failed to close remote session %r for VM %s: %s", + s, + vm.name, + exc, + ) + finally: + if s in vm.remote_sessions: + vm.remote_sessions.remove(s) + + utils_logfile.close_log_file()This keeps failures non‑fatal but makes them visible and avoids silently swallowing unexpected exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (3)
virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/remote.py (3)
cmd_status(708-716)close(30-31)remove(424-438)virttest/utils_logfile.py (1)
close_log_file(150-175)
🪛 Ruff (0.14.4)
virttest/env_process.py
696-698: Avoid specifying long messages outside the exception class
(TRY003)
704-706: Avoid specifying long messages outside the exception class
(TRY003)
713-714: try-except-pass detected, consider logging the exception
(S110)
713-713: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Static checks
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
6184f30 to
b27d0e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
virttest/env_process.py (1)
707-715: Consider logging exceptions during session cleanup.The bare exception handler on lines 712-713 silently swallows all errors, which could hide issues during cleanup. While this is existing code that was moved (not newly introduced), consider logging exceptions for better debuggability.
Apply this diff to log exceptions:
# Close all SSH sessions that might be active to this VM for s in vm.remote_sessions[:]: try: s.close() vm.remote_sessions.remove(s) - except Exception: - pass + except Exception as e: + LOG.debug("Failed to close session: %s", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (4)
virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/qemu_monitor.py (2)
is_responsive(511-522)close(731-748)virttest/remote.py (2)
close(30-31)remove(424-438)virttest/utils_logfile.py (1)
close_log_file(150-175)
🪛 GitHub Actions: CI
virttest/env_process.py
[error] 1-1: isort: Imports are incorrectly sorted and/or formatted.
🪛 Ruff (0.14.5)
virttest/env_process.py
696-698: Avoid specifying long messages outside the exception class
(TRY003)
703-705: Avoid specifying long messages outside the exception class
(TRY003)
712-713: try-except-pass detected, consider logging the exception
(S110)
712-712: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
virttest/env_process.py (2)
673-675: LGTM: Clean refactor to support indexed dump configuration.The change from splitting a string to using
params.get_list()with enumeration enables per-dump configuration, which aligns with the PR objectives.
684-705: Verify theops.grep()check parameter behavior before merging.The code at lines 695 and 702 calls
ops.grep(..., check=True)and assumes it returns a boolean value. However, sinceopsis imported from the externalaexpectlibrary (line 16), I cannot definitively verify the behavior of thecheckparameter from the codebase alone.Key concerns:
Return type verification: Confirm that
ops.grep(..., check=True)returnsTruewhen a pattern is found andFalsewhen not found (or raises an exception, which the code doesn't handle).Empty pattern handling: If
params.get_list()returns strings with only whitespace, those will be passed to grep and may cause unexpected behavior. Consider filtering empty or whitespace-only strings before calling grep.The suggested diff in the original review for handling empty patterns is still valid:
for message in params.get_list( f"expected_vm_extra_dump_{i}", delimiter=";" ): + if not message.strip(): + continue if not ops.grep(vm.session, message, dump_path, check=True):
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
b27d0e5 to
ebc52e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
virttest/env_process.py (1)
15-15: Import ordering violation persists - run isort to fix.The import order still violates isort rules. Third-party imports should be alphabetically sorted:
aexpectshould come beforeavocadoandsix.Run this command to fix:
isort virttest/env_process.py
🧹 Nitpick comments (2)
virttest/env_process.py (2)
696-698: Consider moving exception messages into TestFail constructor as a cleanup.The static analysis tool suggests avoiding long messages outside the exception class, though this is a minor style concern.
If you want to address the Ruff TRY003 warnings, you could create custom exception classes or keep the messages concise, but this is a low-priority cleanup since the current code is functional and readable.
Also applies to: 703-705
707-715: Consider logging exceptions during session cleanup.The exception handling in the session cleanup loop suppresses all exceptions without logging. While this pattern is consistent with existing code in this file (e.g., lines 1522-1525), logging exceptions would help with debugging cleanup issues.
If you want to address the static analysis warnings (S110, BLE001), consider logging exceptions:
for s in vm.remote_sessions[:]: try: s.close() vm.remote_sessions.remove(s) - except Exception: - pass + except Exception as e: + LOG.debug("Failed to close remote session: %s", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (3)
virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/qemu_monitor.py (2)
is_responsive(511-522)close(731-748)virttest/utils_logfile.py (1)
close_log_file(150-175)
🪛 Ruff (0.14.5)
virttest/env_process.py
696-698: Avoid specifying long messages outside the exception class
(TRY003)
703-705: Avoid specifying long messages outside the exception class
(TRY003)
712-713: try-except-pass detected, consider logging the exception
(S110)
712-712: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (3)
- GitHub Check: Static checks
- GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install . VT_TYPE:libvirt
- GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install PYPI_UPLOAD/*.whl VT_TYPE:libvirt
🔇 Additional comments (1)
virttest/env_process.py (1)
684-705: No issues found. The code correctly handles missing verification parameters.The
get_listmethod returns an empty list if the key is not in the parameters, which is exactly what occurs whenexpected_vm_extra_dump_{i}orunexpected_vm_extra_dump_{i}parameters are missing. The for loops at lines 692-698 and 699-705 will simply not execute, and no exception is raised.
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
ebc52e6 to
0bb9915
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
virttest/env_process.py (2)
684-707: Logic looks correct, and session validation is properly implemented.The verification logic correctly handles both expected and unexpected messages:
- For expected messages: raises
TestFailwhenops.grepreturnsFalse(not found)- For unexpected messages: raises
TestFailwhenops.grepreturnsTrue(found)The session availability check prevents errors when the VM session is unavailable.
Optional: Consider extracting exception messages to constants to address Ruff TRY003.
The static analysis tool flags long exception messages on lines 697-699 and 705-707. While not critical, you could extract these to module-level constants or a helper function:
def _format_dump_verification_error(vm_name, message, dump_path, found): state = "Redundant unexpected" if found else "Missing expected" return f"{state} message '{message}' in {vm_name} extra dump {dump_path}"Then use it in the exception raises. However, this is purely stylistic and the current implementation is clear and functional.
709-717: Consider logging exceptions during session cleanup for better debugging.The bare
except Exception: passon lines 714-715 silently ignores any errors during session cleanup. While this is acceptable for best-effort cleanup, logging the exceptions would help with debugging issues.Apply this diff to add logging:
# Close all SSH sessions that might be active to this VM for s in vm.remote_sessions[:]: try: s.close() vm.remote_sessions.remove(s) - except Exception: - pass + except Exception as e: + LOG.debug("Failed to close session: %s", e)Based on coding guidelines or learnings if similar patterns exist in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (3)
virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/qemu_monitor.py (2)
is_responsive(511-522)close(731-748)virttest/utils_logfile.py (1)
close_log_file(150-175)
🪛 Ruff (0.14.5)
virttest/env_process.py
697-699: Avoid specifying long messages outside the exception class
(TRY003)
705-707: Avoid specifying long messages outside the exception class
(TRY003)
714-715: try-except-pass detected, consider logging the exception
(S110)
714-714: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Static checks
|
@luckyh @YongxueHong Finally, I hope you consider a change like this one, it is meant to enhance the original dmesg validation during postprocessing with arbitrary validation for presence or absence of log messages or other strings from arbitrary configurable dumped files from any vm. |
|
Just to clarify, the failed test here is not related to the changes but reads: " |
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
0bb9915 to
7dba010
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
virttest/env_process.py (1)
687-703: Hardenops.grepusage and clarify success/failure conditionsThere are two concerns in this verification block:
Shell‑injection surface via
ops.grep
Earlier analysis showedops.grepbuilds a shell command where the pattern argument is not safely quoted, so a craftedmessage(fromexpected_vm_extra_dump_{i}/unexpected_vm_extra_dump_{i}) can inject shell when it contains problematic characters like single quotes. Even if you plan to fix this upstream inaexpect, you should add a local guard here to reject or sanitize unsafe patterns before callingops.grep(e.g., at minimum reject messages containing'and clearly fail the test with a explanatory error).Ambiguous behavior with
check=Trueand theifconditions
Withcheck=True, a non‑zero grep exit status (no match or other error) is likely to raise an exception internally. That makes conditions like:
if not ops.grep(..., check=True, ...)for expected messagesif ops.grep(..., check=True, ...)for unexpected messagessomewhat brittle and hard to reason about – it’s unclear whether “no match” will be surfaced via a boolean result or an exception, and for unexpected messages you probably want “no match” to be treated as success rather than an exception.
Consider restructuring to make the contract explicit, for example:
- Use
check=False(or equivalent) and inspect the return value / status for “found vs not found”.- Or catch the specific exception raised on non‑zero exit and translate it into a
TestFailonly when it truly indicates a logic failure, not just “pattern not found”.Formatting / CI
CI reports that Black would reformat this file. Please run:black virttest/env_process.pyto clear the formatting failure.
🧹 Nitpick comments (2)
virttest/env_process.py (2)
669-678: Double‑check control flow and indexing for extra dump handlingThe overall flow (only acting when
vm_extra_dump_pathsis set, creating a single session, and iterating withenumerate()overparams.get_list("vm_extra_dump_paths", delimiter=";")) is reasonable. Two things to verify:
enumerate()starts at 0, so the configuration must useverify_vm_extra_dump_0,expected_vm_extra_dump_0, etc. If your configs were written with 1‑based indices, they won’t be picked up.vm.wait_for_login()is called even if none of theverify_vm_extra_dump_{i}flags are enabled; if that’s a common case or if VMs are often unreachable in postprocess, you may want to defer the login until you know at least one dump needs verification.If you confirm both are intentional, the structure is fine.
705-713: Avoid silentexcept Exception: passwhen closing remote sessionsThe cleanup loop correctly iterates over a copy of
vm.remote_sessionsand removes entries as they are closed, but the bare:except Exception: passfully swallows any errors, which can hide real issues (e.g., leaking sessions or file descriptors) and is flagged by static analysis.
Consider at least logging at debug/warning level, and narrowing the exception type if possible, e.g.:
- for s in vm.remote_sessions[:]: - try: - s.close() - vm.remote_sessions.remove(s) - except Exception: - pass + for s in vm.remote_sessions[:]: + try: + s.close() + vm.remote_sessions.remove(s) + except Exception as exc: + LOG.debug("Failed to close remote session %r for VM %s: %s", s, vm.name, exc)This keeps cleanup best‑effort while retaining observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (4)
virttest/libvirt_vm.py (2)
wait_for_login(1979-2020)remove(2410-2427)virttest/virt_vm.py (1)
wait_for_login(1216-1307)virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/utils_logfile.py (1)
close_log_file(150-175)
🪛 GitHub Actions: CI
virttest/env_process.py
[error] 687-689: Black would reformat 1 file; 406 files would be left unchanged. Run 'black' to format the code before committing.
🪛 Ruff (0.14.5)
virttest/env_process.py
693-695: Avoid specifying long messages outside the exception class
(TRY003)
701-703: Avoid specifying long messages outside the exception class
(TRY003)
710-711: try-except-pass detected, consider logging the exception
(S110)
710-710: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
virttest/env_process.py (1)
15-16: Import ofops_linuxalias looks fineUsing
from aexpect import ops_linux as opsis consistent with existingaexpectusage in this module; no issues from a dependency or style perspective.
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
7dba010 to
d561027
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
virttest/env_process.py (1)
669-705: Tighten vm-extra-dump verification: session lifecycle, behavior change, andops.grepsemanticsThis block introduces useful functionality but has a few non-trivial concerns:
Unconditional login in postprocess may change behavior
vm.wait_for_login()is now always called whenevervm_extra_dump_pathsis non-None, even if noverify_vm_extra_dump_{i}flag is set. If the VM has already shut down or crashed by the time postprocess runs, this will raise (e.g. login timeout) and fail postprocessing where previouslycopy_files_from()failures were only logged and the test could still pass. Consider:
- Only attempting login if at least one
verify_vm_extra_dump_{i}is enabled, or- Catching login errors here and turning them into a clearer
TestFail/skip with context (“cannot verify extra dumps because login failed”).Session created by
wait_for_login()is never explicitly closed
Thesessionobtained at Line 672 is not closed in this function. Elsewhere (e.g.,kill_unresponsive_vms), the pattern issession = vm.wait_for_login(...); session.close(), so this is likely a new leak unlesswait_for_logininternally registers the session invm.remote_sessionsand you rely solely on that list. To be safe and explicit, wrap this block and closesessioninfinally:- if params.get("vm_extra_dump_paths") is not None: - # even if there is previously existing session we cannot guarantee its state - # is viable, e.g. it may be in python .env or SMTP subsession, etc. - session = vm.wait_for_login(timeout=vm.LOGIN_WAIT_TIMEOUT) + if params.get("vm_extra_dump_paths") is not None: + # even if there is previously existing session we cannot guarantee its state + # is viable, e.g. it may be in python .env or SMTP subsession, etc. + session = None + try: + session = vm.wait_for_login(timeout=vm.LOGIN_WAIT_TIMEOUT) @@ - if params.get_boolean(f"verify_vm_extra_dump_{i}"): + if params.get_boolean(f"verify_vm_extra_dump_{i}"): for message in params.get_list( @@ LOG.info(f"Checking for unexpected message in {dump_path}") if ops.grep(session, message, dump_path, check=True, flags="-aP"): raise exceptions.TestFail( f"Redundant unexpected message '{message}' in {vm.name} extra dump {dump_path}" ) - - # Close all SSH sessions that might be active to this VM + finally: + if session is not None: + session.close() + + # Close all SSH sessions that might be active to this VM
Verify
ops.grepbehavior withcheck=Trueand conditional use
The conditions
if not ops.grep(..., check=True, flags="-aP"):for expected messages, andif ops.grep(..., check=True, flags="-aP"):for unexpected messagesassume that
ops.grepreturns a truthy/falsy value without throwing whencheck=True. In many APIs, acheck=Trueflag instead raises on non-zero exit status (i.e. when the pattern is not found), in which case:
- The
ifconditions may never see aFalseresult for “not found”, and- You’ll get raw command errors rather than the tailored
TestFailmessages in Lines 695–697 and 703–705.Please double-check the exact semantics of
aexpect.ops_linux.grepin the version you’re using and adjust accordingly (e.g., usecheck=Falseand inspect the return/exit status, or wrap the call intry/exceptto translate grep failures intoTestFailwith your custom message).What is the behavior of `ops_linux.grep` in the `aexpect` library when `check=True`? Does it raise on non-zero grep exit codes, or does it return a boolean/tuple that can be safely used in conditionals?Shell-injection risk via
ops.grepexprparameter (already noted earlier)
As identified in the earlier review on this PR,ops.grepcurrently builds a shell command where the search expression (expr) is only wrapped in single quotes and not fully shell-quoted. Becausemessageanddump_pathhere come from configuration (expected_vm_extra_dump_{i},unexpected_vm_extra_dump_{i},vm_extra_dump_paths), a crafted value containing'and shell metacharacters could inject commands into the guest shell. Until an upstream fix foraexpectis confirmed and consumed here, consider:
- Restricting allowed characters in
expected/unexpectedmessages (e.g., reject entries with shell metacharacters), or- Avoiding
ops.grephere and instead running grep viasession.cmd_status()withutils_misc.shell_escape()applied to bothmessageanddump_path.Since this matches the previously raised concern, I’m marking it as a duplicate for tracking.
Overall, I’d treat (2) and the confirmed
ops.grepinjection behavior as important enough to address before merging, and (1)/(3) as behavior/robustness points that at least deserve an explicit decision.
🧹 Nitpick comments (1)
virttest/env_process.py (1)
707-715: Avoid silent bareexceptwhen closing remote sessionsThis teardown block is best-effort, but a bare
except Exception: passwill silently swallow any issue closing sessions or mutatingvm.remote_sessions, which can hide real cleanup problems and triggered Ruff warnings (S110/BLE001).A lightweight improvement that keeps behavior non-fatal but surfaces issues for debugging would be:
- # Close all SSH sessions that might be active to this VM - for s in vm.remote_sessions[:]: - try: - s.close() - vm.remote_sessions.remove(s) - except Exception: - pass - - utils_logfile.close_log_file() + # Close all SSH sessions that might be active to this VM + for s in vm.remote_sessions[:]: + try: + s.close() + vm.remote_sessions.remove(s) + except Exception as exc: + LOG.debug( + "Ignoring error while closing remote session %r for VM %s: %s", + s, + vm.name, + exc, + ) + + utils_logfile.close_log_file()This keeps failures non-fatal but provides some observability when cleanup goes wrong.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (5)
virttest/libvirt_vm.py (2)
wait_for_login(1979-2020)remove(2410-2427)virttest/virt_vm.py (1)
wait_for_login(1216-1307)virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/remote.py (2)
close(30-31)remove(424-438)virttest/utils_logfile.py (1)
close_log_file(150-175)
🪛 Ruff (0.14.5)
virttest/env_process.py
695-697: Avoid specifying long messages outside the exception class
(TRY003)
703-705: Avoid specifying long messages outside the exception class
(TRY003)
712-713: try-except-pass detected, consider logging the exception
(S110)
712-712: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Static checks
🔇 Additional comments (1)
virttest/env_process.py (1)
15-15: Import alias forops_linuxlooks fineUsing
from aexpect import ops_linux as opsis appropriate for the newops.grepusage and keeps call sites readable. No issues from a functionality standpoint here.
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
d561027 to
bdbf86f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
virttest/env_process.py (1)
679-686: Replace bareexcept:with specific exception types.The bare
except:clause will catch all exceptions includingSystemExitandKeyboardInterrupt, which can prevent proper test cleanup and interruption.Apply this diff:
try: vm.copy_files_from(dump_path, vm_extra_dumps) - except: + except Exception as e: LOG.error( - "Could not copy the extra dump '%s' from the vm '%s'", + "Could not copy the extra dump '%s' from the vm '%s': %s", dump_path, vm.name, + e, )
♻️ Duplicate comments (1)
virttest/env_process.py (1)
15-15: Import ordering violation (duplicate of past review).This import should be sorted according to the project's isort rules. Run
isort virttest/env_process.pyto fix.
🧹 Nitpick comments (1)
virttest/env_process.py (1)
707-715: Improve exception handling in SSH session cleanup.The bare
except Exception:withpasssilently swallows all errors, making debugging difficult. Consider logging exceptions while still ensuring cleanup continues.Apply this diff:
# Close all SSH sessions that might be active to this VM for s in vm.remote_sessions[:]: try: s.close() vm.remote_sessions.remove(s) - except Exception: - pass + except Exception as e: + LOG.warning("Failed to close SSH session: %s", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (4)
virttest/libvirt_vm.py (2)
wait_for_login(1979-2020)remove(2410-2427)virttest/virt_vm.py (1)
wait_for_login(1216-1307)virttest/utils_config.py (2)
get_list(200-202)get_boolean(188-198)virttest/utils_logfile.py (1)
close_log_file(150-175)
🪛 Ruff (0.14.5)
virttest/env_process.py
695-697: Avoid specifying long messages outside the exception class
(TRY003)
703-705: Avoid specifying long messages outside the exception class
(TRY003)
712-713: try-except-pass detected, consider logging the exception
(S110)
712-712: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: Static checks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
virttest/env_process.py (1)
669-711: Tighten error handling around extra dump copying and verification.The overall flow (single login, per‑path copy, then per‑path expected/unexpected checks) looks good, but two points are worth adjusting:
- Bare
exceptaroundvm.copy_files_from(Line 684)
Catching all exceptions (includingBaseExceptionsubclasses likeKeyboardInterrupt) is unnecessary here and can hide serious problems. Prefer narrowing this toExceptionand logging the exception details:
try:vm.copy_files_from(dump_path, vm_extra_dumps)except:LOG.error("Could not copy the extra dump '%s' from the vm '%s'",dump_path,vm.name,)
try:vm.copy_files_from(dump_path, vm_extra_dumps)except Exception:LOG.exception("Could not copy the extra dump '%s' from the vm '%s'",dump_path,vm.name,)
- Behavior when copy fails but verification is requested
After a failed copy, the code continues and still runsops.grepon the guest path. If the intention is that inability to retrieve a requested dump is a test failure, you might want to either:
- Raise
TestFailimmediately on copy failure whenverify_vm_extra_dump_{i}is true, or- Track a failure flag and raise once the loop completes.
If continuing despite copy failure is intentional (e.g., dumps are “best‑effort” artifacts while content verification against the guest path is the only requirement), consider adding a short comment to document that choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (2)
virttest/qemu_vm.py (1)
is_alive(308-314)virttest/virt_vm.py (3)
is_alive(1635-1639)wait_for_login(1216-1307)copy_files_from(1370-1427)
🪛 Ruff (0.14.5)
virttest/env_process.py
684-684: Do not use bare except
(E722)
685-689: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
698-700: Avoid specifying long messages outside the exception class
(TRY003)
708-710: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Static checks
🔇 Additional comments (2)
virttest/env_process.py (2)
15-15: Import alias usage looks correct.
opsis only used forops.grepin this file, and the alias import alongsideremotekeeps the aexpect imports grouped logically. No issues here.
657-661: Condition simplification for kernel param revert is safe.Switching to
if not vm.is_alive():is correct here becausevmis already validated earlier inpostprocess_vm(env.get_vm(name)+ early return onNone). Behavior is unchanged and clearer.
The typical use for this is when dumping extra logs and wanting to make sure certain information is always present (e.g. startup, etc.) and certain information is always absent (e.g. segfaults, etc.). It makes use of aexpect's linux ops as well as pattern matrching for grepping the expected or unexpected messages. Signed-off-by: Plamen Dimitrov <[email protected]>
The vm being defined is something done at the very beginning of postprocessing. Only is_alive() checks make sense at each separate stage, including the handling of extra file dumps from the vms. Signed-off-by: Plamen Dimitrov <[email protected]>
pevogam
force-pushed
the
extra-vm-dump-validation
branch
from
14d1f49 to
e9fc541
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/env_process.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/env_process.py (2)
virttest/virt_vm.py (3)
is_alive(1635-1639)wait_for_login(1216-1307)copy_files_from(1370-1427)virttest/qemu_vm.py (1)
is_alive(308-314)
🪛 Ruff (0.14.5)
virttest/env_process.py
684-684: Do not use bare except
(E722)
685-689: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
698-700: Avoid specifying long messages outside the exception class
(TRY003)
708-710: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Static checks
🔇 Additional comments (2)
virttest/env_process.py (2)
15-15: Alias import ofaexpect.ops_linuxlooks correct and matches usage.The new
from aexpect import ops_linux as opsimport is consistent with howops.grepis used later inpostprocess_vm; no issues here.
655-667: Kernel param revert guard is safe after priorvmexistence check.Changing the guard to
if not vm.is_alive():is correct becausevmwas already validated earlier (if not vm: return), so the extravm andcheck was redundant. The revert logic for kernel params remains unchanged.
| if kernel_extra_params_add or kernel_extra_params_remove: | ||
| # VM might be brought down after test | ||
| if vm and not vm.is_alive(): | ||
| if not vm.is_alive(): |
There was a problem hiding this comment.
From my understanding, we will return if the vm is None at the beginning of postprocess_vm:
avocado-vt/virttest/env_process.py
Lines 638 to 640 in 3fc0482
It means that the
vm is a VM instance here.
Could you illustrate more on which cases will result in the vm being None? Thanks.
| if not vm.is_alive(): | ||
| LOG.warning("VM is not alive so we cannot download extra dump paths") | ||
| else: | ||
| session = vm.wait_for_login(timeout=vm.LOGIN_WAIT_TIMEOUT) |
There was a problem hiding this comment.
Your intention is to check the VM's dump content at the post_process stage, right?
We will download the dump files from the VM to the host first and then check them, so I think we can check the content on the host session instead of the guest session.
Please correct me if I was understood. Thanks.
| dump_path, | ||
| vm.name, | ||
| ) | ||
| if params.get_boolean(f"verify_vm_extra_dump_{i}"): |
There was a problem hiding this comment.
Defining parameters for vm_extra_dump using an index feels a bit inelegant and unfriendly to users. It risks causing confusion when configuring multiple vm_extra_dump instances.
Since the framework already has a mechanism for this (similar to the image object parameter), I believe we should align with that existing design principle.
For example:
vm_extra_dumps = foo boo
vm_extra_dump_file_foo = /path/foo_dump_file
vm_extra_dump_file_boo = /path/foo_dump_boo
verify_vm_extra_dump_foo = yes
verify_vm_extra_dump_boo = yes
expected_vm_extra_dump_foo = boot success
expected_vm_extra_dump_boo = shut down success
unexpected_vm_extra_dump_foo = boot failed
unexpected_vm_extra_dump_boo = shut down failed
However, I realize this approach would impact existing test case configurations, so I’d like to hear other maintainers' opinions. That being said, your current workaround solution is acceptable to me. Thanks.
2 participants
The typical use for this is when dumping extra logs and wanting to make sure certain information is always present (e.g. startup, etc.) and certain information is always absent (e.g. segfaults, etc.).
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.