migration: Add case about migrate-compcache #6547
Conversation
|
Test result: (1/5) type_specific.io-github-autotest-libvirt.migration.migration_cmd.migrate_compcache.vm_running.compcache_512M: STARTED |
| elif unit == "GiB": | ||
| size = int(int(compression_cache) / 1073741824) | ||
| if value != size: | ||
| test.fail("For successful setting, compression cache is not match with set.") |
There was a problem hiding this comment.
This message is confusing, do you mean to say something like f"Failed to set the compcache size: expected {size}, actual {value}"?
| if status_error: | ||
| libvirt.check_result(ret, err_msg) | ||
| else: | ||
| test.fail("Set compression cache fail.") |
There was a problem hiding this comment.
Do you mean to say something like f"Failed to set compression cache: {ret.stdout_text}"?
| if value != size: | ||
| test.fail("For successful setting, compression cache is not match with set.") | ||
| else: | ||
| test.log.info("For successful setting, check compression cache PASS.") |
There was a problem hiding this comment.
Do you mean to say something like "Compression cache size was set successfully"?
WalkthroughAdds a Libvirt migration compression-cache test: a YAML config defining multiple Changes
Sequence DiagramsequenceDiagram
participant Test
participant Virsh
participant VM
Test->>Virsh: migrate_compcache(vm_name) — get current cache
Virsh->>VM: Query compression cache
VM-->>Virsh: Current cache size
Virsh-->>Test: Return cache size
alt status_error = false
Test->>Virsh: migrate_compcache(vm_name, size) — set cache
Virsh->>VM: Set compression cache
VM-->>Virsh: Success
Virsh-->>Test: Result OK
opt vm_running
Test->>Virsh: migrate_compcache(vm_name) — verify cache
Virsh->>VM: Query compression cache
VM-->>Virsh: New cache size
Virsh-->>Test: Return cache size
Test->>Test: Parse & compare (unit conversion)
end
else status_error = true
Test->>Virsh: migrate_compcache(vm_name, size) — set cache
Virsh->>VM: Attempt set -> Error
VM-->>Virsh: Error message
Virsh-->>Test: Error response
Test->>Test: Validate error matches expected `err_msg`
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 4
🧹 Nitpick comments (1)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (1)
32-32: Fix redundant text in log message.The log message contains "migrate-compcache/migrate-compcache" which appears redundant.
Apply this diff:
- test.log.info("Run test for migrate-compcache/migrate-compcache.") + test.log.info("Run test for migrate-compcache.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/migration/migration_cmd/migrate_compcache.cfg(1 hunks)libvirt/tests/src/migration/migration_cmd/migrate_compcache.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py
57-57: f-string: unterminated string
(invalid-syntax)
57-58: Expected FStringEnd, found newline
(invalid-syntax)
58-58: Expected ',', found indent
(invalid-syntax)
58-58: Expected ',', found name
(invalid-syntax)
58-58: Expected ',', found name
(invalid-syntax)
58-58: Expected ',', found 'as'
(invalid-syntax)
58-58: missing closing quote in string literal
(invalid-syntax)
58-59: Expected ',', found newline
(invalid-syntax)
59-59: Expected ')', found dedent
(invalid-syntax)
70-70: f-string: unterminated string
(invalid-syntax)
70-71: Expected FStringEnd, found newline
(invalid-syntax)
71-71: Expected ',', found indent
(invalid-syntax)
71-71: Expected ',', found name
(invalid-syntax)
71-71: Expected ',', found ':'
(invalid-syntax)
71-71: Expected ',', found '{'
(invalid-syntax)
71-71: Expected ',', found '{'
(invalid-syntax)
71-71: missing closing quote in string literal
(invalid-syntax)
71-72: Expected ',', found newline
(invalid-syntax)
72-72: Expected ')', found dedent
(invalid-syntax)
72-72: Expected a statement
(invalid-syntax)
72-73: Expected a statement
(invalid-syntax)
73-73: Unexpected indentation
(invalid-syntax)
73-73: f-string: unterminated string
(invalid-syntax)
73-74: Expected FStringEnd, found newline
(invalid-syntax)
74-74: Expected ',', found indent
(invalid-syntax)
74-74: Expected ',', found name
(invalid-syntax)
74-74: Expected ',', found name
(invalid-syntax)
74-74: Expected ',', found name
(invalid-syntax)
74-74: missing closing quote in string literal
(invalid-syntax)
⏰ 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: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
🔇 Additional comments (6)
libvirt/tests/cfg/migration/migration_cmd/migrate_compcache.cfg (3)
1-16: LGTM!The base configuration is well-structured and includes all necessary settings for migration testing.
18-36: LGTM!The test variants provide excellent coverage including:
- Valid compression cache sizes (512M, 256M)
- Invalid sizes violating power-of-two constraints (1K, 0)
- Out-of-range values testing overflow handling
37-41: LGTM!The vm_shutoff variant correctly validates that migration compression cache operations require a running domain.
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (3)
1-24: LGTM!The imports and function structure are appropriate for a libvirt test module.
25-30: LGTM!Parameter retrieval follows standard libvirt test patterns.
44-49: LGTM!The set operation and error handling logic are correct.
cliping
force-pushed
the
cache
branch
2 times, most recently
from
f2bff01 to
992420f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (3)
30-30: Consider removing unused variable.The
vmobject is retrieved but never used in the test. Since onlyvm_nameis needed for thevirsh.migrate_compcache()calls, this assignment can be removed.Apply this diff:
- vm = env.get_vm(vm_name)
32-32: Clarify redundant text in log message.The log message contains "migrate-compcache/migrate-compcache" which appears redundant. Consider simplifying to clearly indicate what is being tested.
Apply this diff:
- test.log.info("Run test for migrate-compcache/migrate-compcache.") + test.log.info("Run test for migrate-compcache.")
54-54: Remove redundant strip() operations.The
.strip()calls after.split()[index]are redundant becausesplit()already removes leading and trailing whitespace from the resulting elements.Apply this diff to simplify:
- if orig_size.split()[0].strip() != new_size.split()[0].strip(): + if orig_size.split()[0] != new_size.split()[0]:- value = new_size.split()[0].strip() - unit = new_size.split()[-1].strip() + value = new_size.split()[0] + unit = new_size.split()[-1]Also applies to: 60-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/migration/migration_cmd/migrate_compcache.cfg(1 hunks)libvirt/tests/src/migration/migration_cmd/migrate_compcache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/migration/migration_cmd/migrate_compcache.cfg
🧰 Additional context used
🪛 Ruff (0.14.1)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py
30-30: Local variable vm is assigned to but never used
Remove assignment to unused variable vm
(F841)
⏰ 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). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (2)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (2)
44-49: LGTM!The SET operation correctly handles both expected errors (negative tests) and unexpected failures with appropriate error checking.
59-76: LGTM!The positive test verification logic correctly:
- Parses the compression cache size and unit from virsh output
- Converts between units using correct conversion factors (1024 for KiB, 1048576 for MiB, 1073741824 for GiB)
- Handles unexpected units with a clear error message
- Provides detailed failure messages when verification fails
The issues flagged in previous reviews (f-string syntax, undefined size variable) have been properly addressed.
| test.fail(f"Failed to set compression cache size: {ret.stdout_text}") | ||
|
|
||
| if vm_status != "vm_shutoff": | ||
| new_size = virsh.migrate_compcache(vm_name, debug=True).stdout_text.strip().split(":")[-1] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for the verification GET operation.
The third migrate_compcache call (line 52) doesn't check exit_status before accessing stdout_text. If this call fails, it could cause unexpected behavior.
Apply this diff to add error handling:
- if vm_status != "vm_shutoff":
- new_size = virsh.migrate_compcache(vm_name, debug=True).stdout_text.strip().split(":")[-1]
+ if vm_status != "vm_shutoff":
+ ret = virsh.migrate_compcache(vm_name, debug=True)
+ if ret.exit_status:
+ test.fail(f"Failed to get compression cache size for verification: {ret.stdout_text}")
+ new_size = ret.stdout_text.strip().split(":")[-1]🤖 Prompt for AI Agents
In libvirt/tests/src/migration/migration_cmd/migrate_compcache.py around line
52, the third call to virsh.migrate_compcache uses .stdout_text without
verifying exit_status; update this to first check the result (e.g., capture the
call into a variable, test result.exit_status == 0) before accessing
stdout_text, and if the exit_status is non-zero raise an exception or log an
error including result.stderr_text/result.stdout_text for debugging so the test
fails cleanly instead of crashing.
| test.fail(f"For negative testing, compression cache size is modified: {new_size}") | ||
| else: | ||
| test.log.info("For negative testing, compression cache size " | ||
| "remains the same as before.") |
There was a problem hiding this comment.
I think in the above messages for negative test cases, it is important that you use a message that describes your expectation rather than your observation. Something like "Negative test case: the compression cache size shouldn't have been modified but found {new_size}"'.
| test.fail(f"For positive testing, failed to set the compression " | ||
| f"cache size: expected {size}, actual {value}") | ||
| else: | ||
| test.log.info("For positive testing, the compression cache " |
There was a problem hiding this comment.
I'm not a native English speaker but "For positive testing," doesn't sound right. Why is there a need to prefix your expectations with 'negative testing' or 'positive testing'? You either have a certain expectation or not IMO.
To test that memory compression cache size can be set/got by migrate-compcache. VIRT-298168 - [migrate-compcache] set/get cache size for migration memory compression Signed-off-by: lcheng <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (1)
52-52: Add error handling for the verification GET operation.The verification GET at line 52 doesn't check
exit_statusbefore accessingstdout_text. If this call fails, it will cause a test crash or incorrect behavior.Apply this diff to add error handling:
if vm_status != "vm_shutoff": - new_size = virsh.migrate_compcache(vm_name, debug=True).stdout_text.strip().split(":")[-1] + ret = virsh.migrate_compcache(vm_name, debug=True) + if ret.exit_status: + test.fail(f"Failed to get compression cache size for verification: {ret.stdout_text}") + new_size = ret.stdout_text.strip().split(":")[-1]
🧹 Nitpick comments (3)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (3)
30-30: Remove unused variable assignment.The
vmvariable is assigned but never used in the test. Since the test only interacts with the VM throughvirsh.migrate_compcache(vm_name, ...), the VM object itself is not needed.Apply this diff to remove the unused variable:
- vm = env.get_vm(vm_name)
54-55: Consider comparing the full size string for robustness.Comparing only the numeric part (
split()[0]) could cause false negatives if libvirt changes the unit representation (e.g., "1024 KiB" → "1 MiB" represents the same size but would fail the comparison).For a more robust negative test comparison, consider:
- Comparing the entire
orig_size == new_sizestring (if libvirt guarantees consistent formatting), or- Normalizing both sizes to bytes before comparing (similar to lines 62-69)
Example normalization approach:
def normalize_to_bytes(size_str): """Convert size string like '1 MiB' to bytes.""" value, unit = size_str.split() value = int(float(value)) if unit == "KiB": return value * 1024 elif unit == "MiB": return value * 1048576 elif unit == "GiB": return value * 1073741824 else: test.fail(f"Unexpected unit '{unit}'") if normalize_to_bytes(orig_size) != normalize_to_bytes(new_size): test.fail(f"The compression cache size shouldn't have been modified but found {new_size}")
57-57: Consider simplifying log messages.The "For positive testing," and "For negative testing," prefixes were noted in past reviews as potentially unnecessary. The context is clear from the conditional logic itself.
Consider these alternatives:
- Line 57:
"The compression cache size remains unchanged."or"Compression cache size correctly remained unchanged (negative test)."- Line 73:
"The compression cache size was set successfully."Also applies to: 73-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/migration/migration_cmd/migrate_compcache.cfg(1 hunks)libvirt/tests/src/migration/migration_cmd/migrate_compcache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/migration/migration_cmd/migrate_compcache.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
status_error(81-83)
🪛 Ruff (0.14.2)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py
30-30: Local variable vm is assigned to but never used
Remove assignment to unused variable vm
(F841)
⏰ 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). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
🔇 Additional comments (4)
libvirt/tests/src/migration/migration_cmd/migrate_compcache.py (4)
1-13: LGTM!The file header and imports are appropriate for this test module.
34-42: LGTM!The initial GET operation includes proper error handling for both shutoff and running VM states.
44-49: LGTM!The SET operation includes proper error handling for both positive and negative test scenarios.
62-69: LGTM!The unit conversion logic correctly handles KiB, MiB, and GiB, with proper error handling for unexpected units. The conversion factors (1024, 1048576, 1073741824) are correct.
3 participants
To test that memory compression cache size can be set/got by migrate-compcache.
VIRT-298168 - [migrate-compcache] set/get cache size for migration memory compression
Summary by CodeRabbit