Hi Chris:
I got all the comments, thanks. :)
As you said, it is a really complicate patch.
Fortunately, most of the points are not big ones because it won't raise problem
for now.
So if you don't mind, would you please apply it first ? So that I don't need to
do the whole patch again.:)
After that, I will send a much more simple patch to fix all that you said.
Thanks. :)
On 07/03/2012 10:04 PM, Chris Evich wrote:
>
> Cool, okay, taking a fresh look by trying to pretend I've never seen this
> code before...
>
> On 06/29/2012 03:56 AM, tangchen wrote:
>> Signed-off-by: Tang Chen<tanc...@cn.fujitsu.com>
>> ---
>> client/tests/libvirt/tests/virsh_migrate.py | 267
>> ++++++++++++++++++++++-----
>> 1 files changed, 223 insertions(+), 44 deletions(-)
>>
>> diff --git a/client/tests/libvirt/tests/virsh_migrate.py
>> b/client/tests/libvirt/tests/virsh_migrate.py
>> index b342987..16fd298 100644
>> --- a/client/tests/libvirt/tests/virsh_migrate.py
>> +++ b/client/tests/libvirt/tests/virsh_migrate.py
>> @@ -1,5 +1,6 @@
>> -import logging, time
>> -from autotest.client.shared import error
>> +import logging, os, re, time, shutil, codecs
>> +from autotest.client.shared import utils, error
>> +from autotest.client.virt import libvirt_vm, virt_utils, virt_test_utils
>>
>> def run_virsh_migrate(test, params, env):
>> """
>> @@ -16,51 +17,69 @@ def run_virsh_migrate(test, params, env):
>> else:
>> return False
>>
>> - def cleanup_dest(vm, src_uri = ""):
>> + def cleanup_dest(vm, src_uri=""):
>
> Good, better style.
>
>> """
>> Clean up the destination host environment
>> when doing the uni-direction migration.
>> """
>> - vm_state = vm.state()
>> - if vm_state == "running":
>> - vm.destroy()
>> - elif vm_state == "paused":
>> - vm.resume()
>> - vm.destroy()
>> + logging.info("Cleaning up VMs on %s" % vm.connect_uri)
>> + try:
>> + if libvirt_vm.virsh_domain_exists(vm.name, vm.connect_uri):
>> + vm_state = vm.state()
>> + if vm_state == "paused":
>> + vm.resume()
>> + elif vm_state == "shut off":
>> + vm.start()
>> + vm.destroy()
>> +
>> + if vm.is_persistent():
>> + vm.undefine()
>>
>> - if vm.is_persistent():
>> - vm.undefine()
>> + except Exception, detail:
>> + logging.error("Cleaning up destination failed.\n%s" % detail)
>>
>> - vm.connect_uri = src_uri
>> + if src_uri:
>> + vm.connect_uri = src_uri
>
>
> Okay, looking at this fresh, I like the new way. I was concerned for a
> moment throwing exception but not raising another would cause problem.
> However, assuming cleanup_dest() is only even done for uni-directional
> testing, I think it's okay. If there's a problem, the next attempt to
> migrate should fail, and we'll have logs to see what happened. Good.
>
>>
>> - def do_migration(dly, vm, dest_uri, options, extra):
>> - logging.info("Sleeping %d seconds before migration" % dly)
>> - time.sleep(dly)
>> + def do_migration(delay, vm, dest_uri, options, extra):
>> + logging.info("Sleeping %d seconds before migration" % delay)
>> + time.sleep(delay)
>> # Migrate the guest.
>> - successful = vm.migrate(dest_uri, options, extra)
>> - if not successful:
>> - raise error.TestFail("Migration failed for %s." % vm_name)
>> + successful = vm.migrate(dest_uri, options, extra, True,
>> True).exit_status
>> + logging.info("successful: %d", successful)
>> + if int(successful) != 0:
>> + logging.error("Migration failed for %s." % vm_name)
>> + return False
>> +
>> + if options.count("dname") or extra.count("dname"):
>> + vm.name = extra.split()[1].strip()
>>
>
> I'm sorry if I recommended this, I think I suggested a mistake :S This will
> only work if --dname is always the first option/value. However, I'm inclined
> to accept it since all the test configurations are setup this way. Something
> we can examine further later, let's not worry about it for now.
>
>> if vm.is_alive(): # vm.connect_uri was updated
>> logging.info("Alive guest found on destination %s." % dest_uri)
>> else:
>> - raise error.TestFail("VM not running on destination %s" %
>> dest_uri)
>> -
>> - # Migration may fail, but VM is alive on destination.
>> - dest_state = params.get("virsh_migrate_dest_state")
>> - ret = check_vm_state(vm, dest_state)
>> - logging.info("Supposed state: %s" % dest_state)
>> - logging.info("Actual state: %s" % vm.state())
>> - if not ret:
>> - raise error.TestFail("VM is not in the supposed state.")
>> + logging.error("VM not alive on destination %s" % dest_uri)
>> + return False
>>
>> # FIXME: This needs to be tested, but won't work currently
>> # vm.verify_kernel_crash()
>> + logging.debug("vm.verify_kernel_crash() needs to be tested, "
>> + "but won't work currently.")
>
> Bah, right, this is still broken for libvirt. Thanks for the reminder :D
>
>> + return True
>> +
>>
>> vm_name = params.get("main_vm")
>> vm = env.get_vm(params["main_vm"])
>> vm.verify_alive()
>>
>> + # For safety reasons, we'd better back up xmlfile.
>> + vm_xmlfile_bak = vm.backup_xml()
>> + if not vm_xmlfile_bak:
>> + logging.error("Backing up xmlfile failed.")
>> +
>> + vm.connect_uri = params.get("connect_uri", "default")
>> + if vm.connect_uri == 'default':
>> + vm.connect_uri = libvirt_vm.virsh_uri()
>> +
>
> Looks good.
>
>> src_uri = vm.connect_uri
>> dest_uri = params.get("virsh_migrate_desturi")
>> # Identify easy config. mistakes early
>> @@ -75,24 +94,184 @@ def run_virsh_migrate(test, params, env):
>> if dest_uri.count('///') or dest_uri.count('EXAMPLE'):
>> logging.warning(warning_text % ('destination', dest_uri))
>>
>> + vm_ref = params.get("vm_ref", vm.name)
>> options = params.get("virsh_migrate_options")
>> extra = params.get("virsh_migrate_extra")
>> - dly = int(params.get("virsh_migrate_delay", 10))
>> -
>> -
>> - do_migration(dly, vm, dest_uri, options, extra)
>> - # Repeat the migration with a recursive call and guaranteed exit
>> - if params.get("virsh_migrate_back", "no") == 'yes':
>> - back_dest_uri = params.get("virsh_migrate_back_desturi", 'default')
>> - back_options = params.get("virsh_migrate_back_options", 'default')
>> - back_extra = params.get("virsh_migrate_back_extra", 'default')
>> - if back_dest_uri == 'default':
>> - back_dest_uri = src_uri
>> - if back_options == 'default':
>> - back_options = options
>> - if back_extra == 'default':
>> - back_extra = extra
>> - do_migration(dly, vm, back_dest_uri, back_options, back_extra)
>> - # Do the uni-direction migration here.
>> + delay = int(params.get("virsh_migrate_delay", 10))
>> + status_error = params.get("status_error", 'no')
>> + libvirtd_state = params.get("virsh_migrate_libvirtd_state", 'on')
>> + src_state = params.get("virsh_migrate_src_state", "running")
>> + new_nic_mac = "ff:ff:ff:ff:ff:ff"
>> + dest_xmlfile = ""
>> +
>> + exception = False
>> + try:
>> + # Confirm VM can be accessed through network.
>> + time.sleep(delay)
>> + vm_ip = vm.get_address()
>> + s_ping, o_ping = virt_test_utils.ping(vm_ip, count=2, timeout=delay)
>> + logging.info(o_ping)
>> + if s_ping != 0:
>> + raise error.TestError("%s did not respond after %d sec." %
>> (vm.name, delay))
>> +
>> + # Prepare for --xml.
>> + logging.debug("Preparing new xml file for --xml option.")
>> + if options.count("xml") or extra.count("xml"):
>> + dest_xmlfile = params.get("virsh_migrate_xml", "")
>> + if dest_xmlfile:
>> + ret_attach = vm.attach_interface("--type bridge --source
>> virbr0 --mac %s" % new_nic_mac, True, True)
>> + if not ret_attach:
>> + exception = True
>> + raise error.TestError("Attaching nic to %s failed." %
>> vm.name)
>> + vm_xml_new = vm.get_xml()
>> + logging.debug("Xml file on source: %s" % vm_xml_new)
>> + f = codecs.open(dest_xmlfile, 'wb', encoding='utf-8')
>> + f.write(vm_xml_new)
>> + f.close()
>> + if not os.path.exists(dest_xmlfile):
>> + exception = True
>> + raise error.TestError("Creating %s failed." %
>> dest_xmlfile)
>> +
>> + # Turn VM into certain state.
>> + logging.debug("Turning %s into certain state." % vm.name)
>> + if src_state == "paused":
>> + if vm.is_alive():
>> + vm.pause()
>> + elif src_state == "shut off":
>> + if vm.is_alive():
>> + if not vm.shutdown():
>> + vm.destroy()
>
> For future reference, I think you can do vm.destroy(gracefully=True), but
> this is fine for now.
>
>> +
>> + # Turn libvirtd into certain state.
>> + logging.debug("Turning libvirtd into certain status.")
>> + if libvirtd_state == "off":
>> + libvirt_vm.libvirtd_stop()
>> +
>> + # Test uni-direction migration.
>> + logging.debug("Doing migration test.")
>> + if vm_ref != vm_name:
>> + vm.name = vm_ref # For vm name error testing.
>
> Ahh, I was just wondering what vm_ref was for. Good putting comment here.
>
>> + ret_migrate = do_migration(delay, vm, dest_uri, options, extra)
>> + if vm_ref != vm_name:
>> + vm.name = vm_name
>> +
>> + # Recover libvirtd state.
>> + logging.debug("Recovering libvirtd status.")
>> + if libvirtd_state == "off":
>> + libvirt_vm.libvirtd_start()
>> +
>> + # Check vm state on destination.
>> + logging.debug("Checking %s state on %s." % (vm.name,
>> vm.connect_uri))
>> + if options.count("dname") or extra.count("dname"):
>> + vm.name = extra.split()[1].strip()
>
> For future reference, if you have to code the same thing twice, it's a "clue"
> that you maybe need to just define a function/method. However, as noted
> above, it would be "nice" to have a more robust option-checker here, but this
> is okay for now.
>
>> + check_dest_state = True
>> + dest_state = params.get("virsh_migrate_dest_state", "running")
>> + check_dest_state = check_vm_state(vm, dest_state)
>> + logging.info("Supposed state: %s" % dest_state)
>> + logging.info("Actual state: %s" % vm.state())
>> +
>> + # Recover VM state.
>> + logging.debug("Recovering %s state." % vm.name)
>> + if src_state == "paused":
>> + vm.resume()
>> + elif src_state == "shut off":
>> + vm.start()
>> +
>> + # Checking for --persistent.
>> + logging.debug("Checking for --persistent option.")
>> + check_dest_persistent = True
>> + if options.count("persistent") or extra.count("persistent"):
>> + if not vm.is_persistent():
>> + check_dest_persistent = False
>> +
>> + # Checking for --undefinesource.
>> + logging.debug("Checking for --undefinesource option.")
>> + check_src_undefine = True
>> + if options.count("undefinesource") or extra.count("undefinesource"):
>> + logging.info("Verifying<virsh domstate> DOES return an error."
>> + "%s should not exist on %s." % (vm_name, src_uri))
>> + if libvirt_vm.virsh_domain_exists(vm_name, src_uri):
>> + check_src_undefine = False
>> +
>> + # Checking for --dname.
>> + logging.debug("Checking for --dname option.")
>> + check_dest_dname = True
>> + if options.count("dname") or extra.count("dname"):
>> + dname = extra.split()[1].strip()
>> + if not libvirt_vm.virsh_domain_exists(dname, dest_uri):
>> + check_dest_dname = False
>> +
>> + # Checking for --xml.
>> + logging.debug("Checking for --xml option.")
>> + check_dest_xml = True
>> + if options.count("xml") or extra.count("xml"):
>> + if dest_xmlfile:
>> + vm_dest_xml = vm.get_xml()
>> + logging.info("Xml file on destination: %s" % vm_dest_xml)
>> + if not re.search(new_nic_mac, vm_dest_xml):
>> + check_dest_xml = False
>> +
>> + # Repeat the migration from destination to source.
>> + if params.get("virsh_migrate_back", "no") == 'yes':
>> + back_dest_uri = params.get("virsh_migrate_back_desturi",
>> 'default')
>> + back_options = params.get("virsh_migrate_back_options",
>> 'default')
>> + back_extra = params.get("virsh_migrate_back_extra", 'default')
>> + if back_dest_uri == 'default':
>> + back_dest_uri = src_uri
>> + if back_options == 'default':
>> + back_options = options
>> + if back_extra == 'default':
>> + back_extra = extra
>> + ret_migrate = do_migration(delay, vm, back_dest_uri,
>> back_options, back_extra)
>> +
>> + except Exception, detail:
>> + exception = True
>> + logging.error("%s: %s" % (detail.__class__, detail))
>> +
>> +
>> + # Whatever error occurs, we have to clean up all environment.
>> + # Make sure vm.connect_uri is the destination uri.
>> + vm.connect_uri = dest_uri
>> + if options.count("dname") or extra.count("dname"):
>> + # Use the VM object to remove
>> + vm.name = extra.split()[1].strip()
>
> Ahh, three times, see it would help to have a function to do this :) It's
> okay for now though since it's such a simplistic check.
>
>> + cleanup_dest(vm, src_uri)
>> + vm.name = vm_name
>> else:
>> cleanup_dest(vm, src_uri)
>> +
>> + # Recover source (just in case).
>> + # vm.connect_uri has been set back to src_uri in cleanup_dest().
>
> Good to put this comment here, guarantee no confusion.
>
>> + if not libvirt_vm.virsh_domain_exists(vm_name, src_uri):
>> + vm.define(vm_xmlfile_bak)
>
> This we can improve with my new xml_utils class, it will automatically take
> backup. What you have here is fine for now since my code is still
> experimental.
>
>> + else:
>> + #if not vm.shutdown():
>> + vm.destroy()
>> +
>> + # Cleanup source.
>> + if os.path.exists(vm_xmlfile_bak):
>> + os.remove(vm_xmlfile_bak)
>> + logging.info("%s removed." % vm_xmlfile_bak)
>> + if os.path.exists(dest_xmlfile):
>> + os.remove(dest_xmlfile)
>> +
>> + if exception:
>> + raise error.TestError("Error occurred. \n%s: %s" %
>> (detail.__class__, detail))
>> +
>> + # Check test result.
>> + if status_error == 'yes':
>> + if ret_migrate:
>> + raise error.TestFail("Migration finished with unexpected
>> status.")
>> + else:
>> + if not ret_migrate:
>> + raise error.TestFail("Migration finished with unexpected
>> status.")
>> + if not check_dest_state:
>> + raise error.TestFail("Wrong VM state on destination.")
>> + if not check_dest_persistent:
>> + raise error.TestFail("VM is not persistent on destination.")
>> + if not check_src_undefine:
>> + raise error.TestFail("VM is not undefined on source.")
>> + if not check_dest_dname:
>> + raise error.TestFail("Wrong VM name %s on destination." % dname)
>> + if not check_dest_xml:
>> + raise error.TestFail("Wrong xml configuration on destination.")
>
> This is becomming very complicated test module, but the code reads very
> clean, style looks good, and it's well commented. There are a few areas for
> improvement (nothing is ever perfect :) but overall it's very good. I'll
> tripple check it through pylint and then see about getting it committed to
> "next" branch for integration testing. Nice job, thanks for following
> through.
>
--
Best Regards,
Tang chen
_______________________________________________
Autotest mailing list
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
