Automate nbdkit data integrity test for NBD request size constraints and NFS truncation risk - #6847
Conversation
WalkthroughShared nbdcopy settings (version_required, disk_path, checkpoint) were moved into the nbdcopy variant hierarchy in the nbdkit test configuration, and a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
80d29e0 to
47f3b5b
Compare
|
@xiaodwan Please review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
v2v/tests/src/nbdkit/nbdkit.py (2)
949-956: Consider cleaning up the temporary image file.The
finallyblock removes the socket file but notIMG_PATH. Since this is a 1GB sparse image, explicitly removing it would prevent disk space accumulation during repeated test runs.♻️ Proposed fix: Add image cleanup
finally: # Cleanup using the class methods if p and p.poll() is None: LOG.info("Cleanup: Stopping nbdkit process") # stop() is better than terminate() here as it handles the wait() logic p.stop(timeout=5) if os.path.exists(SOCKET): os.remove(SOCKET) + if os.path.exists(IMG_PATH): + os.remove(IMG_PATH)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 949 - 956, The finally block currently cleans up the socket and stops the nbdkit process but does not remove the temporary image file (IMG_PATH), leading to disk accumulation; update the same finally block in the nbdkit teardown to check if IMG_PATH exists and is a file and then remove it (os.remove(IMG_PATH)), ensuring this runs after stopping the process (p.stop) and wrap the remove in a try/except to log any failures (use LOG) to avoid masking exceptions.
906-906: Minor: Extra space in command string.There's a double space between
-Uand{SOCKET}. This won't affect functionality but is inconsistent.- cmd = f"nbdkit -D file.zero=1 -U {SOCKET} file {IMG_PATH}" + cmd = f"nbdkit -D file.zero=1 -U {SOCKET} file {IMG_PATH}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` at line 906, The command string assigned to cmd contains an extra space between "-U" and the {SOCKET} token; update the f-string in the cmd assignment (the variable named cmd in the nbdkit invocation) to use a single space like "-U {SOCKET}" so the command becomes f"nbdkit -D file.zero=1 -U {SOCKET} file {IMG_PATH}" (preserving SOCKET and IMG_PATH references).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 897-911: The variable p used in the try/finally block may be
undefined if an exception occurs before p is assigned (e.g., during truncate or
os.remove); initialize p = None before the try and in the finally only reference
p if it is not None and has been started (e.g., check p is not None and
p.get_pid() or hasattr(p, "stop") before calling p.stop()/cleanup). Update the
block around process.SubProcess, p.start(), and the finally cleanup to use this
pre-initialized p and guard cleanup calls to avoid NameError.
---
Nitpick comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 949-956: The finally block currently cleans up the socket and
stops the nbdkit process but does not remove the temporary image file
(IMG_PATH), leading to disk accumulation; update the same finally block in the
nbdkit teardown to check if IMG_PATH exists and is a file and then remove it
(os.remove(IMG_PATH)), ensuring this runs after stopping the process (p.stop)
and wrap the remove in a try/except to log any failures (use LOG) to avoid
masking exceptions.
- Line 906: The command string assigned to cmd contains an extra space between
"-U" and the {SOCKET} token; update the f-string in the cmd assignment (the
variable named cmd in the nbdkit invocation) to use a single space like "-U
{SOCKET}" so the command becomes f"nbdkit -D file.zero=1 -U {SOCKET} file
{IMG_PATH}" (preserving SOCKET and IMG_PATH references).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60d79f61-cce4-437c-ab8b-0552b1ee9777
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/src/nbdkit/nbdkit.py
…and NFS truncation risk Signed-off-by: Ganesh Hubale <ghubale@redhat.com>
47f3b5b to
01badf6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 894-897: Replace the hard-coded SOCKET and image paths by creating
a unique per-run work_dir under data_dir.get_tmp_dir(), set SOCKET and IMG_PATH
to files inside that work_dir (use a uuid or tempfile.mkdtemp to create it), and
ensure the work_dir is removed in the function's finally block (import shutil
and call shutil.rmtree(work_dir, ignore_errors=True)) so parallel avocado jobs
don't clash or leave stale sockets/images; update any unlink calls (and
references to SOCKET, IMG_PATH, p) to operate on paths inside this work_dir.
- Around line 905-915: The nbdkit launcher currently daemonizes (cmd variable)
and uses a fixed time.sleep(1) which hides startup failures and prevents p.stop
from stopping the daemon; update the cmd string to include the -f flag (so
nbdkit runs in foreground) and replace the fixed sleep with a short polling loop
that checks for the SOCKET file's existence (using os.path.exists or similar)
with a reasonable timeout, calling test.fail if the socket never appears; keep
the existing p.start(), use p.get_pid() to verify the starter started, and
ensure p.stop() will then reliably stop the foreground process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cf3a16b-5506-4417-b8ba-25f3e07ac42f
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/src/nbdkit/nbdkit.py
| SOCKET = "/tmp/test.sock" | ||
| IMG_PATH = os.path.join(data_dir.get_tmp_dir(), "test.img") | ||
| IMAGE_SIZE = "1G" | ||
| p = None |
There was a problem hiding this comment.
Use a per-run work directory instead of a global /tmp/test.sock.
Line 894 hard-codes a shared UNIX socket path, and Lines 955-956 unlink it unconditionally. Parallel avocado jobs can remove each other’s live socket or reuse stale artifacts. Put both the socket and backing image under a unique directory beneath data_dir.get_tmp_dir() and tear that directory down in finally.
🐛 Proposed fix
+ work_dir = tempfile.mkdtemp(prefix="nbdkit_data_integrity_", dir=data_dir.get_tmp_dir())
- SOCKET = "/tmp/test.sock"
- IMG_PATH = os.path.join(data_dir.get_tmp_dir(), "test.img")
+ SOCKET = os.path.join(work_dir, "test.sock")
+ IMG_PATH = os.path.join(work_dir, "test.img")Add cleanup in finally (and import shutil at file scope if you use this form):
if os.path.isdir(work_dir):
shutil.rmtree(work_dir, ignore_errors=True)Also applies to: 949-956
🧰 Tools
🪛 Ruff (0.15.7)
[error] 894-894: Probable insecure usage of temporary file or directory: "/tmp/test.sock"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 894 - 897, Replace the
hard-coded SOCKET and image paths by creating a unique per-run work_dir under
data_dir.get_tmp_dir(), set SOCKET and IMG_PATH to files inside that work_dir
(use a uuid or tempfile.mkdtemp to create it), and ensure the work_dir is
removed in the function's finally block (import shutil and call
shutil.rmtree(work_dir, ignore_errors=True)) so parallel avocado jobs don't
clash or leave stale sockets/images; update any unlink calls (and references to
SOCKET, IMG_PATH, p) to operate on paths inside this work_dir.
| LOG.info("Starting nbdkit server in background") | ||
| cmd = f"nbdkit -D file.zero=1 -U {SOCKET} file {IMG_PATH}" | ||
| p = process.SubProcess(cmd, shell=True, logger=LOG) | ||
| p.start() | ||
| # Check if the process actually started (has a PID) | ||
| if not p.get_pid(): | ||
| test.fail("nbdkit failed to start!") | ||
|
|
||
| # Wait for the socket file to appear | ||
| import time | ||
| time.sleep(1) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does nbdkitdaemonize by default unless-f/--foregroundis passed, and should-fbe used whennbdkit is managed by a parent subprocess in tests?
💡 Result:
Yes, nbdkit daemonizes (forks into the background) by default unless -f/--foreground or --no-fork is passed, or unless certain options that imply foreground are used (such as --run, --exit-with-parent, -s/--single, or socket activation). Yes, -f should be used when nbdkit is managed by a parent subprocess in tests to prevent daemonizing, ensure the parent can track and clean up the process, and capture output reliably. Options like --exit-with-parent also imply -f and are suitable for such scenarios.
Citations:
- 1: https://www.libguestfs.org/nbdkit-captive.1.html
- 2: https://manpages.ubuntu.com/manpages/resolute/man1/nbdkit-service.1.html
- 3: https://libguestfs.org/nbdkit.1.html
- 4: https://manpages.ubuntu.com/manpages/jammy/man1/nbdkit.1.html
- 5: https://manpages.org/nbdkit/1
🏁 Script executed:
# First, locate and examine the nbdkit.py file
find . -name nbdkit.py -type f | head -20Repository: autotest/tp-libvirt
Length of output: 95
🏁 Script executed:
# Read the file to see the context around lines 905-915 and 950-954
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | sed -n '900,920p'Repository: autotest/tp-libvirt
Length of output: 1113
🏁 Script executed:
# Check the second location mentioned at lines 950-954
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | sed -n '945,960p'Repository: autotest/tp-libvirt
Length of output: 985
🏁 Script executed:
# Find and examine the SubProcess class to understand its behavior
find . -name "*.py" -type f -exec grep -l "class SubProcess" {} \; | head -5Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Search for where SubProcess is imported from
grep -n "from.*SubProcess\|import.*SubProcess" ./v2v/tests/src/nbdkit/nbdkit.pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Find the import statements at the top of nbdkit.py
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | head -50Repository: autotest/tp-libvirt
Length of output: 1824
🏁 Script executed:
# Search for SubProcess class in the repository
rg -n "class SubProcess" --type pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# SubProcess is from avocado framework. Let's search for how p.poll() is used and check the actual cleanup
grep -n "p\.poll\|p\.stop\|p\.get_pid" ./v2v/tests/src/nbdkit/nbdkit.py | head -20Repository: autotest/tp-libvirt
Length of output: 382
🏁 Script executed:
# Let's examine the context around other uses of p.poll() to understand the pattern
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | sed -n '770,810p'Repository: autotest/tp-libvirt
Length of output: 2172
🏁 Script executed:
# Let's check if there are other similar patterns with nbdkit that might show the intended usage
grep -B5 -A5 "nbdkit.*-f" ./v2v/tests/src/nbdkit/nbdkit.py | head -40Repository: autotest/tp-libvirt
Length of output: 1957
🏁 Script executed:
# Let's look at how nbdkit is used elsewhere - specifically check if there's any version using -f
grep -n "\-f " ./v2v/tests/src/nbdkit/nbdkit.py | head -20Repository: autotest/tp-libvirt
Length of output: 1808
🏁 Script executed:
# Let's examine the nbdkit usage with -f flag
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | sed -n '596,610p'Repository: autotest/tp-libvirt
Length of output: 1124
🏁 Script executed:
# Let's look at the other usage around line 818
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | sed -n '815,840p'Repository: autotest/tp-libvirt
Length of output: 1318
🏁 Script executed:
# Let's verify that the second location (around line 950) is part of the same function
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | sed -n '880,920p'Repository: autotest/tp-libvirt
Length of output: 2127
🏁 Script executed:
# Let's verify the complete function to confirm both locations are in the same function
cat -n ./v2v/tests/src/nbdkit/nbdkit.py | sed -n '945,965p'Repository: autotest/tp-libvirt
Length of output: 1285
Add -f flag and replace fixed sleep with socket existence polling.
Line 906 starts nbdkit without -f, causing it to daemonize. This means SubProcess only tracks the launcher process while the actual server runs detached. The p.stop() call in the finally block (line 954) then fails to stop the real daemon, and the fixed 1-second sleep masks startup failures instead of detecting them reliably.
Add -f to force foreground mode and replace the sleep with a polling loop that verifies the socket was created before proceeding.
Proposed fix
- cmd = f"nbdkit -D file.zero=1 -U {SOCKET} file {IMG_PATH}"
+ cmd = f"nbdkit -f -D file.zero=1 -U {SOCKET} file {IMG_PATH}"
p = process.SubProcess(cmd, shell=True, logger=LOG)
p.start()
@@
- import time
- time.sleep(1)
+ import time
+ deadline = time.time() + 10
+ while time.time() < deadline:
+ if p.poll() is not None:
+ test.fail("nbdkit exited before creating the UNIX socket")
+ if os.path.exists(SOCKET):
+ break
+ time.sleep(0.1)
+ else:
+ test.fail(f"nbdkit did not create socket {SOCKET}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOG.info("Starting nbdkit server in background") | |
| cmd = f"nbdkit -D file.zero=1 -U {SOCKET} file {IMG_PATH}" | |
| p = process.SubProcess(cmd, shell=True, logger=LOG) | |
| p.start() | |
| # Check if the process actually started (has a PID) | |
| if not p.get_pid(): | |
| test.fail("nbdkit failed to start!") | |
| # Wait for the socket file to appear | |
| import time | |
| time.sleep(1) | |
| LOG.info("Starting nbdkit server in background") | |
| cmd = f"nbdkit -f -D file.zero=1 -U {SOCKET} file {IMG_PATH}" | |
| p = process.SubProcess(cmd, shell=True, logger=LOG) | |
| p.start() | |
| # Check if the process actually started (has a PID) | |
| if not p.get_pid(): | |
| test.fail("nbdkit failed to start!") | |
| # Wait for the socket file to appear | |
| import time | |
| deadline = time.time() + 10 | |
| while time.time() < deadline: | |
| if p.poll() is not None: | |
| test.fail("nbdkit exited before creating the UNIX socket") | |
| if os.path.exists(SOCKET): | |
| break | |
| time.sleep(0.1) | |
| else: | |
| test.fail(f"nbdkit did not create socket {SOCKET}") |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 907-907: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 905 - 915, The nbdkit launcher
currently daemonizes (cmd variable) and uses a fixed time.sleep(1) which hides
startup failures and prevents p.stop from stopping the daemon; update the cmd
string to include the -f flag (so nbdkit runs in foreground) and replace the
fixed sleep with a short polling loop that checks for the SOCKET file's
existence (using os.path.exists or similar) with a reasonable timeout, calling
test.fail if the socket never appears; keep the existing p.start(), use
p.get_pid() to verify the starter started, and ensure p.stop() will then
reliably stop the foreground process.
2 participants
Test result:
blkhash test case code modification and test rerun result:
Summary by CodeRabbit