migration: Add case to test migrate vm between 2 hosts with special CPU #5856
Conversation
cliping
force-pushed
the
l2vm
branch
2 times, most recently
from
ee49b3f
to
7c4e69a
Compare
|
Depends on: avocado-framework/avocado-vt#3978 Test results: (1/8) type_specific.io-github-autotest-libvirt.migration.migration_misc.migration_with_special_cpu.with_precopy.p2p.cpu_disable_vmx_apicv_register: PASS (425.92 s) |
cliping
changed the title
| l2vm_name = "l2vm" | ||
| variants: | ||
| - cpu_disable_vmx_apicv_register: | ||
| l1vm_feature_list = {"vmx": "require", "vmx-apicv-register": "disable"} |
There was a problem hiding this comment.
Does this test work for multi archs?
There was a problem hiding this comment.
I only tested it on x86_64. Not sure if vmx/vmx-apicv-register is supported on other archs.
There was a problem hiding this comment.
Hi @Yingshun
I understood , that vmx CPU capabilities are specific for x86_64, therefore it will not be available on ARM??
I didn't find any particular info, but here: https://qemu-project.gitlab.io/qemu/system/arm/cpu-features.html there are listed only other.
Also in the test design there are mentioned only the Intel CPUs ... I think this should be specified in the configuration, so the test will not run on other then x86_64 machines. (not clear how to specify that only on machines, that has required vmx-* capabilities.
Another question is, if there are other same/similar types for ARM, that has to be tested same way.
| try: | ||
| vmx_index = cpu_xml.get_feature_index(key) | ||
| except Exception as detail: | ||
| test.log.debug("Got a exception: %s" % detail) |
| else: | ||
| cpu_xml.set_feature(vmx_index, name=key, policy=value) | ||
| vm_xml.cpu = cpu_xml | ||
| test.log.debug("cpu xml: %s" % vm_xml.cpu) |
XXX-301817 - [VM migration] migrate vm between 2 hosts with special CPU Signed-off-by: lcheng <[email protected]>
| """ | ||
| test.log.info("Setup steps for cases.") | ||
|
|
||
| server_ip = params.get("server_ip") |
There was a problem hiding this comment.
variables server_ip, client_ip and mount_src are not used in this method().
There was a problem hiding this comment.
Hi @cliping,
Sorry for so long review, you did good work, only for me as a newcomer are some part not so well readable and I believe just reordering or putting some duplicated code inside a separated method would really help me.
Please let me know in case you want to discuss anything.
BR.H.H.
| mount_src = params.get("nfs_mount_src") | ||
| mount_dir = params.get("nfs_mount_dir") | ||
|
|
||
| prepare_vm(params, vm1, vm2, vm1_xml, vm2_xml, test) |
There was a problem hiding this comment.
the method name is misleading .. as there are two VMs prepared ... probably would be worth to rename to
prepare_both_vms()
| vm2_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm2_name) | ||
| bak_vm2_xml = vm2_xml.copy() | ||
|
|
||
| l2vm_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm1_name) |
There was a problem hiding this comment.
I am not fully aware what all is done in the "new_from_inactive_dumpxml) .. but probably it would be worth to jus do the "copy of vm1_xml" .. than call it again?
i.e. do the
l2vm_xml = vm1_xml.copy() as in case of bak_vm2_xml above ...
| params.update({"server_ip": migrate_dest_host}) | ||
| params.update({"client_ip": migrate_source_host}) | ||
|
|
||
| vms = params.get("vms").split() |
There was a problem hiding this comment.
To be "safe" I would recommend here to add some check that the vms parameter is set properly.
something like: (is it correct to use test.error instead of test.fail, as it is issue of the test script and not issue of tested feature)
vms = params.get("vms", "").split()
if len(vms) >=2:
vm1_name = vms[0]
vm2_name = vms[1]
else:
test.error("Wrong test configuration, there should be defined two VM names.")
| shutil.copyfile(vm1_img_bak, vm1_img) | ||
| os.remove(vm1_img_bak) | ||
|
|
||
| params.update({"server_ip": migrate_dest_host}) |
There was a problem hiding this comment.
for better readability, I would recommend to put the hosts get param and update together .. moving rows 298,2-- .. (as variables are not used), or put it together .. but it will be worse for readability.
# set server/client IP back to configured values
migrate_dest_host = params.get("migrate_dest_host")
params.update({"server_ip": migrate_dest_host})
migrate_source_host = params.get("migrate_source_host")
params.update({"client_ip": migrate_source_host})
or
#set server/client IP back to configured values
params.update({"server_ip": params.get("migrate_dest_host")})
params.update({"client_ip": params.get("migrate_source_host")})
| if vm2.is_alive(): | ||
| vm2.destroy(gracefully=False) | ||
| if vm1.is_alive(): | ||
| vm1.destroy(gracefully=False) | ||
|
|
||
| bak_vm2_xml.sync() | ||
| if os.path.exists(l2vm_img): | ||
| os.remove(l2vm_img) | ||
| shutil.copyfile(vm2_img_bak, vm2_img) | ||
| os.remove(vm2_img_bak) | ||
| shutil.copyfile(vm1_img_bak, vm1_img) | ||
| os.remove(vm1_img_bak) |
There was a problem hiding this comment.
According best practices in development it is recommended to not duplicate the code, and introduce a method instead.
def revert_vm_setup(vm, vm_img, vm_img,bak):
if vm.is_alive():
vm.destroy(gracefully=False)
shutil.copyfile(vm_img_bak, vm_img)
os.remove(vm_img_bak)
# and do the remaining code, that was between, am just not sure,
# if it should be before or after the revert, or it is not important
bak_vm2_xml.sync()
if os.path.exists(l2vm_img):
os.remove(l2vm_img)
revert_vm_setup(vm1, vm1_img, vm1_img,bak):
revert_vm_setup(vm2, vm2_img, vm2_img,bak):
| options = params.get("virsh_migrate_options") | ||
|
|
||
| test.log.debug("Run migration.") | ||
| vm1_ip = vm1.get_address() |
There was a problem hiding this comment.
getting IP addresses doesn't need to be requested again if they will be returned in setup_test()
| utils_net.map_hostname_ipaddress(hosts_dict, vm2_session) | ||
| vm1_session.close() | ||
| vm2_session.close() | ||
|
|
There was a problem hiding this comment.
I would suggest to return IP addresses here from the method, so other methods doesn't need to call get_address again ... it will be available in the "run" method.
return vm1_ip, vm2_ip
| server_ip = params.get("server_ip") | ||
| server_user = params.get("server_user", "root") | ||
| server_pwd = params.get("server_pwd") | ||
| client_ip = params.get("client_ip") |
There was a problem hiding this comment.
variables server_ip and client_ip are not used in this method, so they doesn't need to be retrieved here.
Or did I miss something?
| cpu_mode = params.get("cpu_mode") | ||
| l1vm_feature_list = eval(params.get("l1vm_feature_list")) | ||
|
|
||
| vm1_xml = update_cpu_xml(vm1_xml, cpu_mode, test, l1vm_feature_list) |
There was a problem hiding this comment.
According best coding practices this code should not be duplicated, but moved into some method .. in this case inner, so it will be able to use parameters got above.
e.g :
def prepare_single_vm(vm, vm_xml, vm_hostname, test):
vm_xml = update_cpu_xml(vm_xml, cpu_mode, test, l1vm_feature_list)
prepare_package_in_vm(vm)
prepare_env_in_vm(vm, vm_hostname, mount_src, mount_dir, desturi_port)
return vm.get_address()
# ... params.get part should be here ??
vm1_ip = prepare_single_vm(vm1, vm1_xml, vm1_hostname, test)
vm2_ip = prepare_single_vm(vm2, vm2_xml, vm2_hostname, test)
| test.log.debug("Prepare nfs server in %s.", vm1_ip) | ||
| prepare_nfs_server_in_vm(vm1_ip, params) | ||
|
|
||
| mount_in_vm(vm1, (vm1_ip + ":" + mount_src), mount_dir) |
There was a problem hiding this comment.
the vm.session is called also inside the mount_in_vm .. then closed and opened here again.
I am not sure, if the session should be brought outside .. or the map_hostnam_ipaddress should be put inside the mount_in_vm.
Both has it's pros and cons.
But I guess the variant here will be better readable - even it will remove the mount_in_vm method itself
hosts_dict = {"%s" % vm1_hostname: "%s" % vm1_ip, "%s" % vm2_hostname: "%s" % vm2_ip}
mount_src = vm1_ip + ":" + mount_src
for vm in (vm1,vm2):
vm_session = vm.wait_for_login()
utils_disk.mount(mount_src, mount_dir, fstype="nfs", session=vm_session)
utils_net.map_hostname_ipaddress(hosts_dict, vm_session)
vm_session.close()
| shutil.copyfile(vm2_img, vm2_img_bak) | ||
|
|
||
| try: | ||
| setup_test() |
There was a problem hiding this comment.
As the test is focused on special CPU capabilities, there are requirements for the hardware.
I believe here should be also some prerequisites check, and the test should fail with error
test.error(f"tested CPU capability {l1vm_feature_list} not available on host")
XXX-301817 - [VM migration] migrate vm between 2 hosts with special CPU