Automated nbdcopy obeys allocated and destination_is_zero options when running in synchronous mode - #6858
Open
ganeshhubale wants to merge 1 commit into
Open
Conversation
WalkthroughAdds an 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: 3
🧹 Nitpick comments (2)
v2v/tests/cfg/libnbd/libnbd.cfg (1)
13-19: Inconsistent indentation with other variants.The
nbdcopyvariant uses 2-space indentation for its content (lines 14-19), while other variants likeget_size(lines 9-10) andunsanitized_hostname(lines 21-23) use 4-space indentation. Consider aligning for consistency.📐 Suggested indentation fix
- nbdcopy: - version_required = "[nbdkit-server-1.46.1-3,);[libnbd-1.24.1-1,)" - variants: - - destination_is_zero: - checkpoint = 'check_option_destination_is_zero' - - allocated: - checkpoint = 'check_option_allocated' + version_required = "[nbdkit-server-1.46.1-3,);[libnbd-1.24.1-1,)" + variants: + - destination_is_zero: + checkpoint = 'check_option_destination_is_zero' + - allocated: + checkpoint = 'check_option_allocated'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/cfg/libnbd/libnbd.cfg` around lines 13 - 19, The nbdcopy variant block uses 2-space indentation inconsistent with other variants; update the indentation inside the nbdcopy block so its keys (version_required, variants, the nested - destination_is_zero and - allocated entries, and their checkpoint lines) use the same 4-space indentation style as other variant blocks (e.g., get_size and unsanitized_hostname) to maintain consistent formatting for the nbdcopy configuration.v2v/tests/src/libnbd/libnbd.py (1)
103-109: Minor docstring clarification.The docstring says "human readable format" but the function returns a numeric float (MB). Consider updating to clarify it returns usage in megabytes as a number.
📝 Suggested docstring fix
def get_disk_usage(path): - """Returns the actual disk usage (blocks) in human readable format.""" + """Returns the actual disk usage in megabytes (MB).""" # du -h equivalent stat = os.stat(path) # st_blocks is usually in 512-byte units usage_bytes = stat.st_blocks * 512 return usage_bytes / (1024 * 1024)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/libnbd/libnbd.py` around lines 103 - 109, The get_disk_usage function's docstring incorrectly claims it returns a "human readable format" but actually returns a numeric float representing megabytes; update the get_disk_usage docstring to state it returns disk usage as a float in megabytes (MB) and indicate the units and type (e.g., "Returns disk usage in megabytes as a float"). Also ensure the brief description and return documentation mention the calculation from st_blocks * 512 and the returned unit (MB).
🤖 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/libnbd/libnbd.py`:
- Around line 144-147: The cmp call uses process.run without ignore_status so
failures raise instead of returning exit_status, and the boolean logic uses and
instead of or; update the call to process.run(f"cmp {SRC_IMG} {DEST_ZERO}",
ignore_status=True) and change the check to if result.exit_status != 0 or
dest_usage > 1: test.fail("FAILURE: Content mismatch or destination not
sparsified.") so the test fails when either the content mismatches or the
destination isn't sparsified (use the existing symbols process.run, SRC_IMG,
DEST_ZERO, dest_usage, test.fail).
- Around line 128-138: The setup commands in the test use process.run(...) with
ignore_status=True which hides failures; update the critical setup steps (the
truncate/write to SRC_IMG, the dd creating DEST_ZERO, and the nbdcopy call that
uses SRC_IMG and DEST_ZERO) to either remove ignore_status=True so exceptions
are raised or capture the return value and assert the command succeeded by
checking result.exit_status (or raising if non‑zero) before continuing; ensure
you reference the exact calls to process.run that mention SRC_IMG, DEST_ZERO and
the nbdcopy invocation so the test aborts immediately on setup failure.
- Around line 149-186: The test_nbdcopy_option_allocated function is missing the
documented logical-size verification and still uses ignore_status=True for
critical commands; add a check after running nbdcopy that compares the source
and destination logical sizes (e.g., use os.path.getsize or an existing helper
like get_logical_size if present) for SRC_IMG and DEST_ALLOC and fail the test
if they differ, and remove or set ignore_status=False for the nbdcopy invocation
(and other critical shell calls) so errors are surfaced via process.run
(reference SRC_IMG, DEST_ALLOC, get_disk_usage, process.run, and
test_nbdcopy_option_allocated).
---
Nitpick comments:
In `@v2v/tests/cfg/libnbd/libnbd.cfg`:
- Around line 13-19: The nbdcopy variant block uses 2-space indentation
inconsistent with other variants; update the indentation inside the nbdcopy
block so its keys (version_required, variants, the nested - destination_is_zero
and - allocated entries, and their checkpoint lines) use the same 4-space
indentation style as other variant blocks (e.g., get_size and
unsanitized_hostname) to maintain consistent formatting for the nbdcopy
configuration.
In `@v2v/tests/src/libnbd/libnbd.py`:
- Around line 103-109: The get_disk_usage function's docstring incorrectly
claims it returns a "human readable format" but actually returns a numeric float
representing megabytes; update the get_disk_usage docstring to state it returns
disk usage as a float in megabytes (MB) and indicate the units and type (e.g.,
"Returns disk usage in megabytes as a float"). Also ensure the brief description
and return documentation mention the calculation from st_blocks * 512 and the
returned unit (MB).
🪄 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: e24210a6-d4db-44a2-83d8-b86715ad891a
📒 Files selected for processing (2)
v2v/tests/cfg/libnbd/libnbd.cfgv2v/tests/src/libnbd/libnbd.py
ganeshhubale
force-pushed
the
auto-nbdcopy-allocate-dest-zero
branch
from
April 8, 2026 10:58
787dea4 to
c96b7e0
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/libnbd/libnbd.py`:
- Around line 184-194: Add a byte-level payload integrity check to the
"--allocated" flow by comparing SRC_IMG and DEST_ALLOC contents (e.g., compute
and compare a checksum or perform a chunked byte-by-byte comparison) after the
existing allocation and logical size checks; use the same pattern/utility used
elsewhere in this file (or read both files in chunks and compare) and call
test.fail with a descriptive message if the contents differ, referencing
DEST_ALLOC, SRC_IMG, get_disk_usage, and the existing logical-size check
(os.path.getsize) to locate where to insert the check.
- Around line 136-139: The nbdcopy invocation for the destination-is-zero
checkpoint misses the --synchronous flag; update the process.run call that
currently executes "nbdcopy --destination-is-zero {SRC_IMG} {DEST_ZERO} -v" so
it includes "--synchronous" (i.e., call nbdcopy with --destination-is-zero
--synchronous {SRC_IMG} {DEST_ZERO} -v) to match the allocated checkpoint
behavior; edit the process.run line referencing SRC_IMG and DEST_ZERO in
libnbd.py accordingly.
🪄 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: 25a76395-9eb6-497f-9acd-483e71462cae
📒 Files selected for processing (2)
v2v/tests/cfg/libnbd/libnbd.cfgv2v/tests/src/libnbd/libnbd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- v2v/tests/cfg/libnbd/libnbd.cfg
Collaborator
Author
|
@xiaodwan Please review |
…n running in synchronous mode Signed-off-by: Ganesh Hubale <ghubale@redhat.com>
ganeshhubale
force-pushed
the
auto-nbdcopy-allocate-dest-zero
branch
from
April 8, 2026 12:40
c96b7e0 to
c5f7685
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2v/tests/src/libnbd/libnbd.py (1)
134-135: Promote logged preconditions to assertions for stronger test guarantees.Both flows log source/destination usage expectations but do not assert them. Consider failing fast when preconditions are not met so false positives are less likely.
Also applies to: 177-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/libnbd/libnbd.py` around lines 134 - 135, The test currently only logs expected disk-usage preconditions (e.g., the LOG.info call around get_disk_usage(DEST_ZERO)) but does not assert them; add explicit assertions immediately after those logs to fail fast if preconditions are not met (for example assert get_disk_usage(DEST_ZERO) == 100 with a clear message), and do the same for the other logged checks referenced around lines 177-179 (source/destination usage expectations) so the test enforces the preconditions rather than only logging them.
🤖 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/libnbd/libnbd.py`:
- Around line 125-127: Tests currently reuse fixed names SRC_IMG and DEST_ZERO
in a shared temp dir which can cause cross-test contamination; change the code
that sets SRC_IMG, DEST_ZERO (and the other fixed filenames around the same
block) to create unique temp files per test using tempfile.mkstemp or
tempfile.NamedTemporaryFile(delete=False), open/truncate them explicitly (e.g.,
open(file, "wb") and f.truncate(100*1024*1024) or os.ftruncate) to ensure
deterministic size/content, and register cleanup to unlink/delete those files at
teardown (or use try/finally) so artifacts are removed after each test run.
---
Nitpick comments:
In `@v2v/tests/src/libnbd/libnbd.py`:
- Around line 134-135: The test currently only logs expected disk-usage
preconditions (e.g., the LOG.info call around get_disk_usage(DEST_ZERO)) but
does not assert them; add explicit assertions immediately after those logs to
fail fast if preconditions are not met (for example assert
get_disk_usage(DEST_ZERO) == 100 with a clear message), and do the same for the
other logged checks referenced around lines 177-179 (source/destination usage
expectations) so the test enforces the preconditions rather than only logging
them.
🪄 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: 7056c02d-2286-4add-a928-dccd3b9f60ce
📒 Files selected for processing (2)
v2v/tests/cfg/libnbd/libnbd.cfgv2v/tests/src/libnbd/libnbd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- v2v/tests/cfg/libnbd/libnbd.cfg
Comment on lines
+125
to
+127
| SRC_IMG = os.path.join(data_dir.get_tmp_dir(), "src.img") | ||
| DEST_ZERO = os.path.join(data_dir.get_tmp_dir(), "dest_zero.img") | ||
|
|
There was a problem hiding this comment.
Use unique temp filenames and cleanup to avoid cross-test contamination.
Both tests reuse fixed names in a shared temp dir, and truncate -s 100M does not clear existing content if the file already exists at 100M. This can make source layout non-deterministic between runs and leak artifacts.
🔧 Proposed fix (isolate fixtures + cleanup)
@@
-import os
+import os
+import tempfile
@@
- SRC_IMG = os.path.join(data_dir.get_tmp_dir(), "src.img")
- DEST_ZERO = os.path.join(data_dir.get_tmp_dir(), "dest_zero.img")
+ tmp_dir = data_dir.get_tmp_dir()
+ src_fd, SRC_IMG = tempfile.mkstemp(prefix="nbdcopy-src-", suffix=".img", dir=tmp_dir)
+ os.close(src_fd)
+ dest_fd, DEST_ZERO = tempfile.mkstemp(prefix="nbdcopy-dest-zero-", suffix=".img", dir=tmp_dir)
+ os.close(dest_fd)
@@
- process.run(f"truncate -s 100M {SRC_IMG}", shell=True)
- process.run(f"echo 'HELLO' | dd of={SRC_IMG} bs=1 seek=1M conv=notrunc", shell=True)
+ try:
+ process.run(f"truncate -s 100M {SRC_IMG}", shell=True)
+ process.run(f"echo 'HELLO' | dd of={SRC_IMG} bs=1 seek=1M conv=notrunc", shell=True)
@@
- result = process.run(f"cmp {SRC_IMG} {DEST_ZERO}", shell=True, ignore_status=True)
- if result.exit_status != 0:
- test.fail("FAILURE: Content mismatch between source and destination.")
+ result = process.run(f"cmp {SRC_IMG} {DEST_ZERO}", shell=True, ignore_status=True)
+ if result.exit_status != 0:
+ test.fail("FAILURE: Content mismatch between source and destination.")
+ finally:
+ for p in (SRC_IMG, DEST_ZERO):
+ if os.path.exists(p):
+ os.unlink(p)
@@
- SRC_IMG = os.path.join(data_dir.get_tmp_dir(), "src.img")
- DEST_ALLOC = os.path.join(data_dir.get_tmp_dir(), "dest_alloc.img")
+ tmp_dir = data_dir.get_tmp_dir()
+ src_fd, SRC_IMG = tempfile.mkstemp(prefix="nbdcopy-src-", suffix=".img", dir=tmp_dir)
+ os.close(src_fd)
+ dest_fd, DEST_ALLOC = tempfile.mkstemp(prefix="nbdcopy-dest-alloc-", suffix=".img", dir=tmp_dir)
+ os.close(dest_fd)
@@
- process.run(f"truncate -s 100M {SRC_IMG}", shell=True)
+ try:
+ process.run(f"truncate -s 100M {SRC_IMG}", shell=True)
@@
- result = process.run(f"cmp {SRC_IMG} {DEST_ALLOC}", shell=True, ignore_status=True)
- if result.exit_status != 0:
- test.fail("FAILURE: Content mismatch between source and destination.")
+ result = process.run(f"cmp {SRC_IMG} {DEST_ALLOC}", shell=True, ignore_status=True)
+ if result.exit_status != 0:
+ test.fail("FAILURE: Content mismatch between source and destination.")
+ finally:
+ for p in (SRC_IMG, DEST_ALLOC):
+ if os.path.exists(p):
+ os.unlink(p)Also applies to: 167-168, 171-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2v/tests/src/libnbd/libnbd.py` around lines 125 - 127, Tests currently reuse
fixed names SRC_IMG and DEST_ZERO in a shared temp dir which can cause
cross-test contamination; change the code that sets SRC_IMG, DEST_ZERO (and the
other fixed filenames around the same block) to create unique temp files per
test using tempfile.mkstemp or tempfile.NamedTemporaryFile(delete=False),
open/truncate them explicitly (e.g., open(file, "wb") and
f.truncate(100*1024*1024) or os.ftruncate) to ensure deterministic size/content,
and register cleanup to unlink/delete those files at teardown (or use
try/finally) so artifacts are removed after each test run.
|
Closing this PR due to current team constraints. This is part of a broader effort to triage all in-flight work across our upstream repos. If this work is still needed, please feel free to reopen and it will be picked up. Apologies for any inconvenience. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test result:
Summary by CodeRabbit