add case for migration with ethernet interface #6625
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a new libvirt migration test for VMs with Ethernet interfaces in unprivileged contexts. Adds configuration file defining migration variants (precopy, postcopy, cancel) with NFS-backed storage and network settings. Implements test script orchestrating unprivileged VM creation, migration workflow, and verification. Updates network utilities to support remote TAP device management and MTU configuration via optional session parameters. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Script
participant Local as Local Host
participant Remote as Remote Host
participant Guest as Guest VM
Test->>Local: Create unprivileged user & VM
Test->>Local: Setup Linux bridge & TAP devices
Test->>Remote: Create remote bridge via session
Test->>Remote: Create remote TAP with MTU via session
Test->>Local: Initiate migration
rect rgb(200, 220, 255)
Note over Test,Remote: Migration Variants
alt Precopy
Test->>Remote: Migrate with precopy
else Postcopy
Test->>Remote: Migrate with postcopy
else Cancel
Test->>Remote: Cancel migration mid-process
Test->>Local: Migrate back to source
end
end
Test->>Guest: Reinitialize serial console
Test->>Guest: Renew DHCP lease
Test->>Guest: Verify connectivity (ping)
Test->>Local: Cleanup TAP & bridge
Test->>Remote: Delete remote TAP via session
Test->>Local: Undefine unprivileged VM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span three distinct areas: a new configuration file (trivial), a complex multi-step test script orchestrating unprivileged VM lifecycle and migration workflows (dense logic), and network utility enhancements with remote execution support (moderate complexity). The test script contains intricate setup/teardown logic with error handling, requiring careful verification of resource management and control flow. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 5
🧹 Nitpick comments (7)
provider/virtual_network/network_base.py (2)
173-175: Consider converting tap_mtu to int and validating range.Prevent invalid MTU values early (e.g., non-numeric, < 576). Add simple guard before calling set_tap_mtu.
- if tap_mtu: - set_tap_mtu(tap_name, tap_mtu, session) + if tap_mtu: + try: + mtu_val = int(tap_mtu) + if mtu_val < 576: + raise ValueError + except Exception: + raise exceptions.TestError(f"Invalid tap_mtu: {tap_mtu}") + set_tap_mtu(tap_name, mtu_val, session)
220-231: Add timeout to local delete and prefer explicit 'ip link delete'.Consistency and clearer intent; keep ignore_status.
- del_cmd = f'ip l del {tap_name}' + del_cmd = f'ip link delete {tap_name}' @@ - else: - process.run(del_cmd, shell=True, ignore_status=True) + else: + process.run(del_cmd, shell=True, ignore_status=True, timeout=30, verbose=True)Based on static analysis hints.
libvirt/tests/cfg/virtual_network/migrate/migrate_with_ethernet_interface.cfg (2)
43-50: Make action_during_mig string robust to quoting.Current nested quotes are brittle. Prefer JSON with escaped quotes or remove inner quotes around 1 to avoid parser issues.
- action_during_mig = '[{"func": "virsh.domjobabort", "after_event": "iteration: '1'", "func_param": "'%s' % params.get('migrate_main_vm')"}]' + action_during_mig = "[{\"func\": \"virsh.domjobabort\", \"after_event\": \"iteration: 1\", \"func_param\": \"%s\" % params.get('migrate_main_vm')}]"Ensure your param consumer evals/loads this correctly (some paths expect Python-literal, not JSON). If it requires a Python literal, keep single quotes but avoid nested single quotes around 1.
14-14: Outside IP as FQDN can introduce DNS flakiness.Consider using a stable IPv4 (e.g., 8.8.8.8) or make resolution explicit in the test.
libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py (3)
116-126: Close the serial login session after ping to avoid leaks.vm_session_after_mig is never closed.
- vm_session_after_mig = unpr_vm_obj.wait_for_serial_login(timeout=240) + vm_session_after_mig = unpr_vm_obj.wait_for_serial_login(timeout=240) vm_session_after_mig.cmd("dhclient -r; dhclient", timeout=120) @@ - network_base.ping_check(params, ips, vm_session_after_mig) + network_base.ping_check(params, ips, vm_session_after_mig) + vm_session_after_mig.close()
118-118: Remove unused backup_uri assignment.Cleaner and silences F841.
- backup_uri, unpr_vm_obj.connect_uri = unpr_vm_obj.connect_uri, dest_uri + unpr_vm_obj.connect_uri = dest_uri
54-60: Catch specific exceptions and use warning().Avoid blind except; warn() is deprecated in logging.
- try: - process.run(f'useradd -m {user_name}', shell=True, ignore_status=True) - process.run(f'echo "{user_name}:{user_passwd}" | chpasswd', shell=True) - except Exception as e: - test.log.warn(f"User {user_name} may already exist: {e}") + try: + process.run(f'useradd -m {user_name}', shell=True, ignore_status=True) + process.run(f'echo "{user_name}:{user_passwd}" | chpasswd', shell=True) + except process.CmdError as e: + test.log.warning(f"User {user_name} may already exist or creation failed: {e}")If usernames/passwords can contain special chars, add quoting/validation to avoid shell injection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/migrate/migrate_with_ethernet_interface.cfg(1 hunks)libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py(1 hunks)provider/virtual_network/network_base.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py (2)
provider/virtual_network/network_base.py (5)
prepare_vmxml_for_unprivileged_user(234-279)define_vm_for_unprivileged_user(282-300)create_tap(152-174)ping_check(98-149)delete_tap(220-231)provider/migration/base_steps.py (3)
setup_connection(343-364)cleanup_connection(366-378)MigrationBase(27-477)
🪛 Ruff (0.14.1)
provider/virtual_network/network_base.py
171-171: Function call with shell=True parameter identified, security issue
(S604)
231-231: Function call with shell=True parameter identified, security issue
(S604)
libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py
22-22: Unused function argument: env
(ARG001)
56-56: Function call with shell=True parameter identified, security issue
(S604)
57-57: Function call with shell=True parameter identified, security issue
(S604)
58-58: Do not catch blind exception: Exception
(BLE001)
118-118: Local variable backup_uri is assigned to but never used
Remove assignment to unused variable backup_uri
(F841)
165-165: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
🔇 Additional comments (1)
libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py (1)
171-177: Inconsistency confirmed: test uses 'main_vm' but migration config and callbacks expect 'migrate_main_vm'.The review comment is valid. The test file at line 171 uses
params.get('main_vm'), but the config file (migrate_with_ethernet_interface.cfg:49) and standard migration test pattern both expectparams.get("migrate_main_vm"). Since the test passesparamsdirectly toMigrationBase(line 178), any MigrationBase callbacks that try to retrieve the VM name viaparams.get("migrate_main_vm")will fail.While the test creates the VM object explicitly and passes it directly to
MigrationBase, the config file'saction_during_migcallback usesparams.get('migrate_main_vm'), which will returnNone, potentially breaking migration abort operations.Update line 171 to use
params.get('migrate_main_vm')instead, or verify that the config file's callback action does not rely on this parameter key.
| utils_net.create_linux_bridge_tmux(bridge_name, host_iface) | ||
| network_base.create_tap(tap_name, bridge_name, unpr_user, tap_mtu) | ||
|
|
||
| utils_net.create_linux_bridge_tmux(bridge_name, remote_host_iface, session=remote_session) | ||
| network_base.create_tap(tap_name, bridge_name, unpr_user, | ||
| session=remote_session, tap_mtu=tap_mtu) | ||
|
|
There was a problem hiding this comment.
Bug: create_tap passes tap_mtu as the 'flag' positional arg (MTU never applied locally).
This will generate an invalid ip command (e.g., trailing “9000”) and skip MTU set. Use a keyword for tap_mtu.
Apply:
- network_base.create_tap(tap_name, bridge_name, unpr_user, tap_mtu)
+ network_base.create_tap(tap_name, bridge_name, unpr_user, tap_mtu=tap_mtu)📝 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.
| utils_net.create_linux_bridge_tmux(bridge_name, host_iface) | |
| network_base.create_tap(tap_name, bridge_name, unpr_user, tap_mtu) | |
| utils_net.create_linux_bridge_tmux(bridge_name, remote_host_iface, session=remote_session) | |
| network_base.create_tap(tap_name, bridge_name, unpr_user, | |
| session=remote_session, tap_mtu=tap_mtu) | |
| utils_net.create_linux_bridge_tmux(bridge_name, host_iface) | |
| network_base.create_tap(tap_name, bridge_name, unpr_user, tap_mtu=tap_mtu) | |
| utils_net.create_linux_bridge_tmux(bridge_name, remote_host_iface, session=remote_session) | |
| network_base.create_tap(tap_name, bridge_name, unpr_user, | |
| session=remote_session, tap_mtu=tap_mtu) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py
around lines 99 to 105, create_tap is being called with tap_mtu as a positional
argument which is interpreted as the 'flags' param and results in an invalid ip
command and MTU not being applied; fix both calls by passing tap_mtu as a
keyword argument (tap_mtu=tap_mtu) so the function receives the MTU correctly
(also keep session=remote_session on the remote call).
| utils_net.create_linux_bridge_tmux(bridge_name, remote_host_iface, session=remote_session) | ||
| network_base.create_tap(tap_name, bridge_name, unpr_user, | ||
| session=remote_session, tap_mtu=tap_mtu) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Ensure unprivileged user exists on the destination host before remote TAP ops.
create_tap on dest uses “user {unpr_user}”; if the user doesn’t exist remotely, tap creation fails.
If missing, add a guarded creation step before create_tap:
remote_session.cmd(f'getent passwd {unpr_user} || (useradd -m {unpr_user} && echo "{unpr_user}:{unpr_passwd}" | chpasswd)', timeout=60)🏁 Script executed:
#!/bin/bash
# Verify/create remote unprivileged user prior to remote TAP ops
remote_ip="${server_ip}"
user="${unpr_user}"
pass="${unpr_passwd}"
# Check presence
ssh -o StrictHostKeyChecking=no "${server_user}@${remote_ip}" "getent passwd ${user} >/dev/null || echo MISSING"Length of output: 41
I need to examine the codebase to verify whether the concern about the unprivileged user is valid.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full context around lines 102-105 in the test file
cat libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py | head -130 | tail -40
Length of output: 1853
🏁 Script executed:
#!/bin/bash
# Find where unpr_user is defined/initialized
rg -n "unpr_user" libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py -B3 -A1Length of output: 1434
🏁 Script executed:
#!/bin/bash
# Find the create_tap function definition
rg -n "def create_tap" --type=py -B2 -A8Length of output: 849
🏁 Script executed:
#!/bin/bash
# Search for user creation patterns in migration/network test files
rg -n "useradd|adduser|create.*user" --type=py libvirt/tests/src/virtual_network/migrate/ -iLength of output: 1041
I need to examine the create_unpr_user_and_vm function and check whether it creates the user on both source and destination hosts.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full create_unpr_user_and_vm function body
sed -n '43,85p' libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py
Length of output: 1921
🏁 Script executed:
#!/bin/bash
# Check for any remote user creation or setup around the remote operations
sed -n '170,110p' libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.pyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for where remote_session is set up and if user creation happens there
rg -n "remote_session" libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py -B2 -A2Length of output: 1427
🏁 Script executed:
#!/bin/bash
# Look at the full setup_test function to see what happens before remote create_tap
sed -n '80,110p' libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.pyLength of output: 1308
I need to verify what the create_tap function does with the user parameter on remote hosts.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full create_tap implementation
sed -n '152,185p' provider/virtual_network/network_base.py
Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Search for how the user parameter is used in create_tap
rg -n "def create_tap" -A30 provider/virtual_network/network_base.pyLength of output: 1142
Create unprivileged user on destination host before remote TAP creation.
The create_tap function executes ip tuntap add mode tap user {user} which requires the user to exist on the target system. Currently, the unprivileged user is created locally (line 177 in create_unpr_user_and_vm), but the remote create_tap call at lines 103-104 will fail because the user doesn't exist on the destination host.
Add user creation on the remote host before line 103:
remote_session.cmd(f'getent passwd {unpr_user} || (useradd -m {unpr_user} && echo "{unpr_user}:{unpr_passwd}" | chpasswd)', timeout=60)🤖 Prompt for AI Agents
libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py
around lines 102-105: the remote create_tap call expects the unprivileged user
to exist on the destination host but the user is only created locally; before
calling create_tap on the remote session, run a remote_session.cmd that ensures
the unprivileged user exists on the remote host (use getent to check and, if
missing, create the user with a home and set its password), then proceed to call
network_base.create_tap on the remote session.
| dest_uri = params.get("virsh_migrate_desturi") | ||
| iface_attrs = eval(params.get("iface_attrs", "{}")) | ||
| host_iface = params.get('host_iface') |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid eval for iface_attrs; use ast.literal_eval.
Safer and matches Ruff S307.
+ import ast
@@
- iface_attrs = eval(params.get("iface_attrs", "{}"))
+ iface_attrs = ast.literal_eval(params.get("iface_attrs", "{}"))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.1)
165-165: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.py
around lines 164-166, replace the unsafe use of eval for iface_attrs with
ast.literal_eval: import ast at the top of the file, change iface_attrs =
eval(params.get("iface_attrs", "{}")) to use
ast.literal_eval(params.get("iface_attrs", "{}")), and optionally wrap the
literal_eval call in a try/except to raise a clear error if parsing fails.
| def create_tap(tap_name, bridge_name, user, flag='', session=None, tap_mtu=''): | ||
| """ | ||
| Create tap device | ||
| :param tap_name: name of tap device | ||
| :param bridge_name: bridge to connect to | ||
| :param user: user with access | ||
| :param flag: additional flags for tap creation | ||
| :param session: session object for remote execution, None for local | ||
| :param tap_mtu: tap mtu setting value | ||
| """ | ||
| # Create tap device with ip command | ||
| tap_cmd = f'ip tuntap add mode tap user {user} group {user} name ' \ | ||
| f'{tap_name} {flag};ip link set {tap_name} up;' \ | ||
| f'ip link set {tap_name} master {bridge_name}' | ||
| # Execute command as root | ||
| process.run(tap_cmd, shell=True, verbose=True) | ||
| if session: | ||
| session.cmd(tap_cmd, timeout=60) | ||
| else: | ||
| process.run(tap_cmd, shell=True, verbose=True) | ||
|
|
||
| if tap_mtu: | ||
| set_tap_mtu(tap_name, tap_mtu, session) | ||
|
|
There was a problem hiding this comment.
Do not chain with ';' — it masks failures; add timeouts and sanitize inputs.
Semicolon-chained tap_cmd can succeed even if earlier subcommands fail. Also, shell=True and unquoted vars risk injection. Use '&&', quote inputs, and add a local timeout for parity with remote.
Apply:
@@
- tap_cmd = f'ip tuntap add mode tap user {user} group {user} name ' \
- f'{tap_name} {flag};ip link set {tap_name} up;' \
- f'ip link set {tap_name} master {bridge_name}'
+ tap_cmd = ' && '.join([
+ f"ip tuntap add mode tap user {user} group {user} name {tap_name} {flag}".strip(),
+ f"ip link set {tap_name} up",
+ f"ip link set {tap_name} master {bridge_name}",
+ ])
@@
- if session:
- session.cmd(tap_cmd, timeout=60)
- else:
- process.run(tap_cmd, shell=True, verbose=True)
+ if session:
+ session.cmd(tap_cmd, timeout=60)
+ else:
+ process.run(tap_cmd, shell=True, verbose=True, timeout=60)Optionally, restrict allowed characters in tap_name/bridge_name/flag or use shlex.quote for stronger hardening. Based on static analysis hints.
🧰 Tools
🪛 Ruff (0.14.1)
171-171: Function call with shell=True parameter identified, security issue
(S604)
| def set_tap_mtu(tap, mtu_size, session=None): | ||
| """ | ||
| Set mtu for tap/macvtap device | ||
| :param tap: tap/macvtap device name | ||
| :param mtu_size: mtu size to set | ||
| :param session: session object for remote execution, None for local | ||
| """ | ||
| process.run(f'ip link set dev {tap} mtu {mtu_size}') | ||
| mtu_cmd = f'ip link set dev {tap} mtu {mtu_size}' | ||
| if session: | ||
| session.cmd(mtu_cmd, timeout=30) | ||
| else: | ||
| process.run(mtu_cmd) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Local process.run lacks shell=True/timeout; align with remote path.
Without shell=True and a timeout, local MTU set can behave inconsistently and hang.
- mtu_cmd = f'ip link set dev {tap} mtu {mtu_size}'
- if session:
- session.cmd(mtu_cmd, timeout=30)
- else:
- process.run(mtu_cmd)
+ mtu_cmd = f'ip link set dev {tap} mtu {mtu_size}'
+ if session:
+ session.cmd(mtu_cmd, timeout=30)
+ else:
+ process.run(mtu_cmd, shell=True, timeout=30, verbose=True)🤖 Prompt for AI Agents
In provider/virtual_network/network_base.py around lines 205 to 218, the local
branch calling process.run(mtu_cmd) does not pass shell=True and a timeout like
the remote session path, which can cause hangs and inconsistent behavior; update
the local call to use process.run(mtu_cmd, shell=True, timeout=30) (and
propagate or handle exceptions similarly to the session path) so both remote and
local code paths behave consistently.
xxxx-299034 - [ethernet] Migrate vm with ethernet type interface Signed-off-by: nanli <[email protected]>
nanli1
force-pushed
the
add_case_for_migration_with_ethernet
branch
from
14bb388 to
5d52012
Compare
1 participant
xxxx-299034 - [ethernet] Migrate vm with ethernet type interface
Signed-off-by: nanli [email protected]
Summary by CodeRabbit