Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
- migration.migration_uri.tcp.migration_port_allocation.port_min_and_port_max:
type = tcp_port_min_and_port_max
migration_setup = 'yes'
storage_type = 'nfs'
setup_local_nfs = 'yes'
disk_type = "file"
disk_source_protocol = "netfs"
mnt_path_name = ${nfs_mount_dir}
# Console output can only be monitored via virsh console output
only_pty = True
take_regular_screendumps = no
# Extra options to pass after <domain> <desturi>
virsh_migrate_extra = ''
# SSH connection time out
ssh_timeout = 60
# Local URI
virsh_migrate_connect_uri = 'qemu:///system'
virsh_migrate_dest_state = "running"
virsh_migrate_src_state = "shut off"
image_convert = 'no'
server_ip = "${migrate_dest_host}"
server_user = "root"
server_pwd = "${migrate_dest_pwd}"
status_error = "no"
check_network_accessibility_after_mig = "yes"
migrate_desturi_port = "16509"
migrate_desturi_type = "tcp"
virsh_migrate_desturi = "qemu+tcp://${migrate_dest_host}/system"
vms = "avocado-vt-vm1 vm2 vm3"
qemu_conf_path = '/etc/libvirt/qemu.conf'
variants:
- p2p:
virsh_migrate_options = '--live --p2p --verbose --bandwidth 15'
- non_p2p:
virsh_migrate_options = '--live --verbose --bandwidth 15'
variants test_case:
- default:
qemu_conf_dest = '{r".*migration_port_min\s*=.*": "migration_port_min=49152", r".*migration_port_max\s*=.*": "migration_port_max=49215"}'
port_list = ["49152", "49153", "49154"]
- min_49111_and_max_49222:
qemu_conf_dest = '{r".*migration_port_min\s*=.*": "migration_port_min=49111", r".*migration_port_max\s*=.*": "migration_port_max=49222"}'
port_list = ["49111", "49112", "49113"]
- min_49333_and_max_49334:
qemu_conf_dest = '{r".*migration_port_min\s*=.*": "migration_port_min=49333", r".*migration_port_max\s*=.*": "migration_port_max=49334"}'
port_list = ["49333", "49334"]
145 changes: 145 additions & 0 deletions libvirt/tests/src/migration/migration_uri/tcp_port_min_and_port_max.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#
# Copyright Redhat
#
# SPDX-License-Identifier: GPL-2.0
#
# Author: Liping Cheng <lcheng@redhat.com>
#
# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

import re
import os
import threading
import time

from avocado.utils import process

from virttest import libvirt_remote
from virttest import remote
from virttest import virsh
from virttest.utils_libvirt import libvirt_vmxml
from virttest.libvirt_xml import vm_xml

from provider.migration import base_steps
from provider.migration import migration_base


def check_port(test, port_list):
"""
Check port during migration

"""
time.sleep(5)
cmd = "netstat -tunap|grep qemu-kvm|grep 49"
ret = process.run(cmd, shell=True, verbose=True, ignore_status=True).stdout_text.strip()
for port in port_list:
if not re.findall(port, ret):
test.fail(f"Not found {port} in {ret}.")
else:
test.log.debug(f"Checked {port} successfully.")


def run(test, params, env):
"""
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].

"""
def setup_test():
"""
Setup steps

"""
qemu_conf_path = params.get("qemu_conf_path")
qemu_conf_dest = params.get("qemu_conf_dest", "{}")
nfs_mount_dir = params.get("nfs_mount_dir")
migrate_desturi_port = params.get("migrate_desturi_port")
migrate_desturi_type = params.get("migrate_desturi_type", "tcp")

migration_obj.conn_list.append(migration_base.setup_conn_obj(migrate_desturi_type, params, test))
migration_obj.remote_add_or_remove_port(migrate_desturi_port)

nonlocal remote_obj
remote_obj = libvirt_remote.update_remote_file(params,
qemu_conf_dest,
qemu_conf_path)
for vm in vms:
disk_dict = {'source': {'attrs': {'file': os.path.join(nfs_mount_dir,
os.path.basename(vm.get_first_disk_devices()['source']))}}}
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_inactive_dumpxml(vm.name),
'disk', disk_dict)
vm.start()
vm.wait_for_login()

def run_test():
"""
run test steps

"""
src_uri = params.get("virsh_migrate_connect_uri")
options = params.get("virsh_migrate_options", "")
status_error = "yes" == params.get("status_error", "no")
migrate_timeout = int(params.get("virsh_migrate_thread_timeout", 900))
port_list = eval(params.get("port_list", "[]"))
test_case = params.get("test_case")

test.log.debug("run test.")
p = threading.Thread(target=check_port, args=(test, port_list,))
p.start()

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)
Comment on lines +92 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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 the Exception object, which is not iterable and can raise TypeError. You likely want to inspect the exception message instead.
  • Line 106: vms=vm[2] uses vm (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 the vms list you built.
  • Lines 104 and 114: test.fail("Migrate vms failed: %s", info) assumes a printf-style API; Avocado’s fail generally 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 -40

Length 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 2

Length 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 5

Length 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 3

Length 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 1

Length 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 -60

Length 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 2

Length 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 2

Length 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 2

Length 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 2

Length 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 -150

Length 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; use if err_msg not in str(info):
  • Line 106: vms=vm[2] fails because vm (from the outer loop) is a single VM object, not subscriptable; use vms=[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.


def cleanup_test():
"""
Cleanup steps

"""
test.log.info("Cleanup steps.")
for vm in vms:
virsh.destroy(vm.name, debug=True, uri=dest_uri)
migration_obj.cleanup_connection()
nonlocal remote_obj
if remote_obj:
del remote_obj

dest_uri = params.get("virsh_migrate_desturi")
vm_names = params.get("vms").split()
remote_obj = None
vms = []
for vm_name in vm_names:
vm = env.get_vm(vm_name)
vms.append(vm)
test.log.debug("vm name: %s", vm.name)

migration_obj = base_steps.MigrationBase(test, vms[0], params)

try:
setup_test()
run_test()
migration_obj.verify_default()
finally:
cleanup_test()
Loading