On 07/04/2012 05:27 AM, tangchen wrote:
> 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. :)
>
Oh, sorry I wasn't clear. What you've said above is what I already did
:) Your whole patchset All of this is now in the "next" branch. In
case you missed the announcement- we're using this "next" branch for
continuous-integration-testing.
Basically all new code gets dumped there, and we have automated jobs
running nightly to check for failures. Think of it like an automated
autotest test :) It's to make sure all the changes we're putting in can
play nice with each other. Every week or so, assuming there are no
failures, LMR is using git cherry-pick to move patches into master.
If you have time to fix the small things later, go for it. Otherwise I
don't see them as critical, so don't worry about it :)
> 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.
>>
>
--
Chris Evich, RHCA, RHCE, RHCDS, RHCSS
Quality Assurance Engineer
e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
_______________________________________________
Autotest mailing list
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
