[virt-tools-list] Add virDomainSetMigrateMaxDowntime support

[Wen Congyang]

At 2010-11-17 01:01, Cole Robinson Write:
> On 11/10/2010 01:17 AM, Wen Congyang wrote:
>> # HG changeset patch
>> # User Wen Congyang <wency at cn.fujitsu.com>
>> # Date 1289353517 -28800
>> # Node ID 6f13917004fe49d30e125030570cf2dae6dd364c
>> # Parent  dc89aa7162ee04bbd94aa754434bdd87286efcc8
>> Add virDomainSetMigrateMaxDowntime support
>>
> 
> Sorry for the delay, and thanks for the patch.
> 
>> diff -r dc89aa7162ee -r 6f13917004fe src/virtManager/domain.py
>> --- a/src/virtManager/domain.py	Wed Oct 20 17:29:17 2010 +0300
>> +++ b/src/virtManager/domain.py	Wed Nov 10 09:45:17 2010 +0800
>> @@ -1090,6 +1090,9 @@
>>          if self.get_autostart() != val:
>>              self._backend.setAutostart(val)
>>  
>> +    def migrate_set_max_downtime(self, max_downtime, flag):
>> +        self._backend.migrateSetMaxDowntime(max_downtime, flag)
>> +
> 
> I'd have flags default to 0 here so the caller doesn't always need to
> specify it.
OK
> 

>> @@ -382,6 +388,10 @@
>>          interface = self.get_config_interface()
>>          rate = self.get_config_rate()
>>          port = self.get_config_port()
>> +        max_downtime = self.get_config_max_downtime()
>> +
>> +        if max_downtime < 1:
>> +            return self.err.val_err(_("max downtime must be greater than 0."))
>>
> 
> You should be able to set limits on the spin box to prevent entering a
> negative value.

> 

>>  
>> +    def _async_set_max_downtime(self, vm, max_downtime, migrate_thread):
>> +        if (migrate_thread.isAlive()):
> 
> Rather than nest this whole block, invert this check and return False
> immediately.
OK
> 
>> +            try:
>> +                vm.migrate_set_max_downtime(max_downtime, 0)
>> +                return False
>> +            except libvirt.libvirtError:
>> +                # domain is not being migrated
>> +                # wait 100 milliseconds
>> +                return True
> 
> The 'except' here isn't fine grained enough: for hypervisors that don't
> support the max downtime API, we will end up looping over and over
> getting a NO_SUPPORT exception.
> 
> You probably want to use:
> 
>    except Exception, e:
>       if (isinstance(e, libvirt.libvirtError) and
> 	  e.get_error_code() == libvirt.VIR_ERR_OPERATION_INVALID):
>           return True
>       logging.debug("Error setting migrate downtime: %s" % e)
>       return False
Sounds good. I will modify it.

> 
>> +            except Exception:
>> +                logging.warning("setting max_downtime to %lu ms failed."
>> +                                % max_downtime)
>> +                return False
>> +        else:
>> +            return False
>> +
>>      def _async_migrate(self, origvm, origdconn, migrate_uri, rate, live,
>> -                       secure, asyncjob):
>> +                       secure, max_downtime, asyncjob):
>>          errinfo = None
>>          try:
>>              try:
>> @@ -454,7 +482,11 @@
>>  
>>                  logging.debug("Migrating vm=%s from %s to %s", vm.get_name(),
>>                                srcconn.get_uri(), dstconn.get_uri())
>> +                current_thread = threading.currentThread()
>> +                timer = util.safe_timeout_add(100, self._async_set_max_downtime,
>> +                                              vm, max_downtime, current_thread)
> 
> I just added a support module check to virtinst to see if the libvirt
> connection actually supports setting migrate downtime. So now with
> latest virtinst you should only kick off the downtime thread if:
> 
> if virtinst.support.check_domain_support(vm,
>                     virtinst.support.SUPPORT_DOMAIN_MIGRATE_DOWNTIME):
> 
> 
>>                  vm.migrate(dstconn, migrate_uri, rate, live, secure)
>> +                gobject.source_remove(timer)
>>              except Exception, e:
>>                  errinfo = (str(e), ("Unable to migrate guest:\n %s" %
>>                                      "".join(traceback.format_exc())))
I think we should check whether the domain supports setting migrate downtime
early. If the domain does not support setting migrate downtime,
we should hide the input box in the UI.

>> diff -r dc89aa7162ee -r 6f13917004fe src/vmm-migrate.glade
>> --- a/src/vmm-migrate.glade	Wed Oct 20 17:29:17 2010 +0300
>> +++ b/src/vmm-migrate.glade	Wed Nov 10 09:45:17 2010 +0800
> 
> A few UI recommendations:
> 
> - Move the max downtime field under the 'advanced' expander, above the
> 'connectivity' section
> 
> - Have the UI disabled by default with a check box, similar to how the
> connectivity options are handled. This way we aren't calling 'max
> downtime' for a default migration and just deferring to the hypervisor
> defaults. That way we can hardcode 30ms (the current QEMU default) into
> the UI, but if the hypervisor ever changes that value we at least aren't
> overriding it on every migration.
> 
> - Add a few pixels spacing between the spin box and the 'ms' label (see
> the 'Bandwidth' field as an example)
> 
> - The spin box shouldn't stretch if the migrate window is resized
> horizontally. You can fix this by putting the spin box in a 2 item hbox
> with an expanding alignment as the other item (see the 'Port' field as
> an example)
> 
> Thanks!
> Cole
> 
Thanks for your comment. I will modify the patch.