migration: Add case about migration_port_min/max #6657
Conversation
This case is to verify that when migration port is not specified, libvirt will allocate a port in range [migration_port_min, migration_port_max]. ID: VIRT-293975 Signed-off-by: lcheng <lcheng@redhat.com>
WalkthroughThis pull request introduces a new test case for libvirt TCP migration port range allocation. It adds a configuration file specifying multiple test variants for NFS-backed VM migrations with explicit port range configurations (default range 49152-49215, and custom ranges 49111-49222 and 49333-49334). Alongside this, a Python test script implements the test logic, including port allocation verification through network scanning, migration execution, and conditional error handling based on port range scenarios. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
🧹 Nitpick comments (3)
libvirt/tests/cfg/migration/migration_uri/tcp_port_min_and_port_max.cfg (1)
1-45: Config structure and variants look consistent with existing migration testsThe test definition, p2p/non_p2p variants, and
test_caseport ranges all line up with the new Python test expectations (including the failing49333-49334case). I don’t see functional issues in this cfg.If you expect to add more ranges later, it might be slightly more maintainable to express
port_listas integers and let the test convert to strings for regex matching, but this is purely optional.libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py (2)
28-40:check_port: constantshell=Trueis acceptable here; consider future-proofing the port filterGiven
cmdis a fixed string and there’s no user input, usingshell=Trueis low risk in this test context despite the static warning.Two optional tweaks you might consider:
- The hardcoded
grep 49assumes all tested ports are in the49xxxrange; if future configs use other ranges, this filter will silently drop relevant lines.re.findall(port, ret)works, but a slightly stricter match (e.g. matching":{port}") would reduce the chance of accidental substring hits.Not blockers, just cleanups you could do if you revisit this helper.
81-86: Useast.literal_evalinstead ofevalto parseport_list
port_list = eval(params.get("port_list", "[]"))works with your current cfg, butevalis unnecessarily powerful and flagged by linters. You can safely parse the list literal from the cfg usingast.literal_eval:+import ast @@ - port_list = eval(params.get("port_list", "[]")) + port_list = ast.literal_eval(params.get("port_list", "[]"))This preserves behavior while avoiding the usual
evalpitfalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/migration/migration_uri/tcp_port_min_and_port_max.cfg(1 hunks)libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py (2)
provider/migration/migration_base.py (1)
setup_conn_obj(135-161)provider/migration/base_steps.py (2)
remote_add_or_remove_port(457-477)cleanup_connection(366-378)
🪛 Ruff (0.14.5)
libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py
35-35: Function call with shell=True parameter identified, security issue
(S604)
85-85: Use of possibly insecure function; consider using ast.literal_eval
(S307)
100-100: Do not catch blind exception: Exception
(BLE001)
| try: | ||
| migration_obj.migration_test.do_migration(vms=vms, srcuri=src_uri, | ||
| desturi=dest_uri, | ||
| migration_type="simultaneous", | ||
| options=options, | ||
| thread_timeout=migrate_timeout, | ||
| ignore_status=False, | ||
| status_error=status_error) | ||
| except Exception as info: | ||
| if test_case == "min_49333_and_max_49334": | ||
| err_msg = "Unable to find an unused port in range 'migration' (49333-49334)" | ||
| if err_msg not in info: | ||
| test.fail("Migrate vms failed: %s", info) | ||
| else: | ||
| migration_obj.migration_test.do_migration(vms=vm[2], srcuri=src_uri, | ||
| desturi=dest_uri, | ||
| migration_type="orderly", | ||
| options=options, | ||
| thread_timeout=migrate_timeout, | ||
| ignore_status=False, | ||
| status_error=status_error) | ||
| else: | ||
| test.fail("Migrate vms failed: %s", info) |
There was a problem hiding this comment.
🧩 Analysis chain
Fix exception-message check, VM selection, and test.fail formatting in the error path
There are a few correctness issues in this block:
- Line 101–104:
if err_msg not in info:compares a string to theExceptionobject, which is not iterable and can raiseTypeError. You likely want to inspect the exception message instead. - Line 106:
vms=vm[2]usesvm(the last VM object from the outer loop) as if it were a list; this will fail at runtime ('LibvirtVM' object is not subscriptable) and also ignores thevmslist you built. - Lines 104 and 114:
test.fail("Migrate vms failed: %s", info)assumes a printf-style API; Avocado’sfailgenerally takes a single message string, so this may not format as intended.
A concrete fix could look like:
- except Exception as info:
+ except Exception as info:
if test_case == "min_49333_and_max_49334":
err_msg = "Unable to find an unused port in range 'migration' (49333-49334)"
- if err_msg not in info:
- test.fail("Migrate vms failed: %s", info)
+ if err_msg not in str(info):
+ test.fail(f"Migrate vms failed: {info}")
else:
- migration_obj.migration_test.do_migration(vms=vm[2], srcuri=src_uri,
+ # Retry with a single VM; adjust index as desired if not the third VM.
+ migration_obj.migration_test.do_migration(vms=[vms[2]], srcuri=src_uri,
desturi=dest_uri,
migration_type="orderly",
options=options,
thread_timeout=migrate_timeout,
ignore_status=False,
status_error=status_error)
else:
- test.fail("Migrate vms failed: %s", info)
+ test.fail(f"Migrate vms failed: {info}")Additionally, if do_migration raises a specific exception type for migration failures, narrowing the except Exception to that type would address the BLE001 warning and avoid masking unrelated errors.
I'll verify the review comment by examining the actual code and understanding the context.
🏁 Script executed:
#!/bin/bash
# View the file around the problem lines
cat "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" | head -120 | tail -40Length of output: 2106
🏁 Script executed:
#!/bin/bash
# Find where vm and vms are defined
rg -n "^\s*(vm|vms)\s*=" "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" -A 2 -B 2Length of output: 235
🏁 Script executed:
#!/bin/bash
# Look for the full function/loop context
rg -n "for.*vm" "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" -C 5Length of output: 1319
🏁 Script executed:
#!/bin/bash
# Check imports and test framework used
head -50 "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py"Length of output: 1279
🏁 Script executed:
#!/bin/bash
# Find do_migration signature to understand vms parameter
rg -n "def do_migration" "libvirt/tests/src/migration/migration_uri/" -A 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for test.fail usage patterns in the codebase
rg -n 'test\.fail\s*\(' "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" -B 1 -A 1Length of output: 344
🏁 Script executed:
#!/bin/bash
# Check Exception handling patterns in similar files
rg -n "except Exception" "libvirt/tests/src/migration/migration_uri/" -A 5 | head -60Length of output: 755
🏁 Script executed:
#!/bin/bash
# Look for how vms is used/passed to do_migration
rg -n "do_migration.*vms" "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" -B 2 -A 2Length of output: 628
🏁 Script executed:
#!/bin/bash
# Check loop structure - what vm iterates over
rg -n "for vm in" "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" -A 10 -B 2Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Full context of the except block and surrounding code
sed -n '85,120p' "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py"Length of output: 1846
🏁 Script executed:
#!/bin/bash
# Check where vms is initialized
sed -n '1,100p' "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" | rg -n "vms\s*=" -B 2 -A 2Length of output: 264
🏁 Script executed:
#!/bin/bash
# Look at full file structure
wc -l "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py"Length of output: 74
🏁 Script executed:
#!/bin/bash
# Get complete context: show lines 75-125
sed -n '75,125p' "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py"Length of output: 2370
🏁 Script executed:
#!/bin/bash
# Find vms list construction
rg -n "vms\s*=\s*\[" "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" -A 5 -B 2Length of output: 240
🏁 Script executed:
#!/bin/bash
# Check what vm is in context of the loop
sed -n '1,200p' "libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py" | tail -150Length of output: 5299
🌐 Web query:
Avocado test framework fail method signature
💡 Result:
In Avocado tests call:
self.fail(message)
where message is an optional string describing the failure. Calling it raises the test failure (TestFail) and marks the test as FAIL. [1][2]
Sources:
- Avocado "Writing Avocado Tests" documentation (usage of self.fail). [1]
- Avocado VT docs (shows test.fail and mentions TestFail exception). [2]
🌐 Web query:
Avocado test.fail() python documentation
💡 Result:
In Avocado Python tests use the Test.fail method (called as self.fail(msg)) to mark a test as FAIL with an explanatory message — typically used to convert a caught exception or an unexpected condition into a failure (instead of an ERROR). Example:
try:
something()
except SomeError as e:
self.fail(f"operation failed: {e}")
See the Avocado "Writing Avocado Tests" guide for details. [1][2]
Sources
[1] Avocado documentation — Writing Avocado Tests (v105.0).
[2] Avocado documentation — Writing Avocado Tests (v76.0).
Fix exception-message check, VM selection, and test.fail formatting in the error path
All three issues identified are confirmed:
- Line 103:
if err_msg not in info:compares against an Exception object; useif err_msg not in str(info): - Line 106:
vms=vm[2]fails becausevm(from the outer loop) is a single VM object, not subscriptable; usevms=[vms[2]] - Lines 104 and 114:
test.fail("Migrate vms failed: %s", info)uses printf-style formatting not supported by Avocado; use f-string:test.fail(f"Migrate vms failed: {info}")
Suggested fix:
except Exception as info:
if test_case == "min_49333_and_max_49334":
err_msg = "Unable to find an unused port in range 'migration' (49333-49334)"
if err_msg not in str(info):
test.fail(f"Migrate vms failed: {info}")
else:
migration_obj.migration_test.do_migration(vms=[vms[2]], srcuri=src_uri,
desturi=dest_uri,
migration_type="orderly",
options=options,
thread_timeout=migrate_timeout,
ignore_status=False,
status_error=status_error)
else:
test.fail(f"Migrate vms failed: {info}")🧰 Tools
🪛 Ruff (0.14.5)
100-100: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py around
lines 92-114, the exception handling block has three issues: it compares the
expected error string to an Exception object, selects VMs incorrectly, and uses
incorrect test.fail formatting. Fix by converting the exception to string when
checking the message (use if err_msg not in str(info)), pass the correct VM list
to do_migration (e.g., vms=[vms[2]] if vms is a list) and call test.fail with a
formatted string (e.g., test.fail(f"Migrate vms failed: {info}")) so the checks,
VM selection, and error reporting work properly.
1 participant
This case is to verify that when migration port is not specified, libvirt will allocate a port in range
[migration_port_min, migration_port_max].
ID: VIRT-293975
Summary by CodeRabbit