Change the specified fsfreeze mountpoint to /boot#6824
Change the specified fsfreeze mountpoint to /boot#6824iccaszhulili wants to merge 1 commit intoautotest:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughConfiguration for domfsfreeze tests updated to use "/boot" instead of "/" for mountpoint values. The inner function check_freeze(session) was changed to check_freeze(session, options) and now derives a target path from the options string to construct a dynamic touch command. Tests in virsh_domfsfreeze_domfsthaw.py replace test.log.info with logging.info, use session.cmd_status for a file-existence check during thaw, add debug=True to domfsfreeze calls, and change the default mountpoint from "/" to "/boot". No other control flow or public API signatures were introduced. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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)
libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py (1)
57-70:⚠️ Potential issue | 🟡 MinorPotential
UnboundLocalErrorforoutputand typo fix needed.Two issues in this function:
- Line 62: Typo - "exsiting" should be "existing"
- Lines 66-70: If
statusis 0 (file exists), the code skips theifblock whereoutputis assigned, but theexcepthandler at line 69 still referencesoutput. Ifcmd_statusraisesShellTimeoutError,outputwill be undefined.🐛 Proposed fix
def check_thaw(session, mountpoints, file_name): ... logging.info("TEST_STEP: Check file system thaw.") for mntpoint in mountpoints: + output = "" try: status = session.cmd_status("ls %s/%s" % (mntpoint, file_name)) if status: - logging.info("file %s/%s is not exsiting after thaw" + logging.info("file %s/%s is not existing after thaw" % (mntpoint, file_name)) output = session.cmd_output("touch %s/%s" % (mntpoint, file_name), timeout=10) except aexpect.ShellTimeoutError: test.fail( "Failed to thaw file system %s. Touch file timeout:\n%s" % (mntpoint, output) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py` around lines 57 - 70, Initialize a local variable output before the try block and correct the typo "exsiting"→"existing"; specifically, in the loop over mountpoints where session.cmd_status and session.cmd_output are used (referencing session.cmd_status, session.cmd_output, aexpect.ShellTimeoutError, mountpoints, file_name, and test.fail), set output = "" (or None) just before try so the except handler can safely reference it, and change the logging string from "not exsiting" to "not existing".
🧹 Nitpick comments (1)
libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze.py (1)
20-26: Update docstring to document the newoptionsparameter.The docstring should document the
optionsparameter that was added to the function signature.📝 Proposed docstring update
def check_freeze(session, options): """ Check whether file system has been frozen by touch a test file and see if command will hang. :param session: Guest session to be tested. + :param options: The domfsfreeze options string to extract the mountpoint. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze.py` around lines 20 - 26, The docstring for the function check_freeze is missing documentation for the newly added options parameter; update the check_freeze docstring to include a short description of options (its type and purpose) alongside the existing :param session: entry—e.g., add a line like ":param options: <type> and description of what options controls (e.g., timeout, file path, flags)"; keep wording concise and consistent with other docstrings in the file and reference the function name check_freeze and the parameter options when adding the new docstring line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py`:
- Line 2: The file currently imports the logging module and calls
logging.info(); change these to use the project convention by removing the
import of logging and replacing all calls to logging.info(...) with
test.log.info(...). Ensure any other logging.* calls in this file (e.g.,
logging.debug/error) are likewise replaced with the appropriate test.log methods
and that the function parameter named test is used for logging (e.g.,
test.log.info(...)) so the file matches the other tests.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze.py`:
- Around line 28-30: The code assumes options.split() has elements and does path
= options.split()[-1], which raises IndexError when domfsfreeze_options is
empty; change it to first split into parts = options.split() and then set path =
parts[-1] if parts else '' (or another safe default), and only build/run cmd =
'touch %s/freeze_test' % path and call session.cmd_output when path is non-empty
(skip or handle the empty-case appropriately) so that options, path, cmd, and
session.cmd_output are guarded against empty domfsfreeze_options.
---
Outside diff comments:
In `@libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py`:
- Around line 57-70: Initialize a local variable output before the try block and
correct the typo "exsiting"→"existing"; specifically, in the loop over
mountpoints where session.cmd_status and session.cmd_output are used
(referencing session.cmd_status, session.cmd_output, aexpect.ShellTimeoutError,
mountpoints, file_name, and test.fail), set output = "" (or None) just before
try so the except handler can safely reference it, and change the logging string
from "not exsiting" to "not existing".
---
Nitpick comments:
In `@libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze.py`:
- Around line 20-26: The docstring for the function check_freeze is missing
documentation for the newly added options parameter; update the check_freeze
docstring to include a short description of options (its type and purpose)
alongside the existing :param session: entry—e.g., add a line like ":param
options: <type> and description of what options controls (e.g., timeout, file
path, flags)"; keep wording concise and consistent with other docstrings in the
file and reference the function name check_freeze and the parameter options when
adding the new docstring line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eed8348-6e26-4848-9d31-403e703ff591
📒 Files selected for processing (4)
libvirt/tests/cfg/virsh_cmd/domain/virsh_domfsfreeze.cfglibvirt/tests/cfg/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.cfglibvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze.pylibvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py
| @@ -1,4 +1,5 @@ | |||
| import aexpect | |||
| import logging | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which logging pattern is more common in the codebase
echo "=== Files using test.log.info ==="
rg -l "test\.log\.info" libvirt/tests/src/ | wc -l
echo "=== Files using logging.info directly ==="
rg -l "^import logging" libvirt/tests/src/ | head -20Repository: autotest/tp-libvirt
Length of output: 1573
🏁 Script executed:
#!/bin/bash
# Examine the reviewed file to see the logging pattern
cat -n libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py | head -50Repository: autotest/tp-libvirt
Length of output: 2362
🏁 Script executed:
#!/bin/bash
# Search for logging usage in the reviewed file
echo "=== logging.info calls ==="
rg -n "logging\.info" libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py
echo "=== test.log.info calls ==="
rg -n "test\.log\.info" libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py
echo "=== Check if test.log is defined ==="
rg -n "test\.log\s*=" libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.pyRepository: autotest/tp-libvirt
Length of output: 761
🏁 Script executed:
#!/bin/bash
# Check if there's a test fixture/setup that initializes test.log
rg -n "class.*Test\|def.*setUp\|test\.log" libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py | head -30Repository: autotest/tp-libvirt
Length of output: 45
Consider using test.log.info() for consistency with project conventions.
The file imports logging and uses logging.info() throughout, but the project standard is to use test.log.info() (used in 362 other test files). Since test is available as a function parameter, align this file with the codebase convention by switching to test.log.info().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py` at line 2,
The file currently imports the logging module and calls logging.info(); change
these to use the project convention by removing the import of logging and
replacing all calls to logging.info(...) with test.log.info(...). Ensure any
other logging.* calls in this file (e.g., logging.debug/error) are likewise
replaced with the appropriate test.log methods and that the function parameter
named test is used for logging (e.g., test.log.info(...)) so the file matches
the other tests.
Under image mode, there is no mountpoint of "/", so change the specified fsfreeze mountpoint to /boot. Also change the function in test_thaw for listing file, The file can not be created if fsfreeze is sucessfully, and is not created before fsthaw in the code. It does not make sense to fail the case if the file is not exsiting after fsthaw. Signed-off-by: Lili Zhu <lizhu@redhat.com>
|
Image mode: # avocado run --vt-type libvirt domfsfreeze_domfsthaw |
1 participant
Under image mode, there is no mountpoint of "/",
so change the specified fsfreeze mountpoint to /boot.
Also change the function in test_thaw for listing file, The file can not be created if fsfreeze is sucessfully, and is not created before fsthaw in the code. It does not make sense to fail the case if the file is not exsiting after fsthaw.
Summary by CodeRabbit