[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] Add virDomainSetMigrateMaxDowntime support



On 11/10/2010 01:17 AM, Wen Congyang wrote:
> # HG changeset patch
> # User Wen Congyang <wency 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.

>      def migrate(self, destconn, interface=None, rate=0,
>                  live=False, secure=False):
>          newname = None
> diff -r dc89aa7162ee -r 6f13917004fe src/virtManager/migrate.py
> --- a/src/virtManager/migrate.py	Wed Oct 20 17:29:17 2010 +0300
> +++ b/src/virtManager/migrate.py	Wed Nov 10 09:45:17 2010 +0800
> @@ -23,6 +23,7 @@
>  
>  import traceback
>  import logging
> +import threading
>  
>  import virtinst
>  import libvirt
> @@ -129,6 +130,7 @@
>  
>          self.window.get_widget("migrate-label-name").set_text(name)
>          self.window.get_widget("migrate-label-src").set_text(srchost)
> +        self.window.get_widget("migrate-max-downtime").set_value(30)
>  
>          self.window.get_widget("migrate-advanced-expander").set_expanded(False)
>          self.window.get_widget("migrate-set-interface").set_active(False)
> @@ -207,6 +209,10 @@
>  
>      def get_config_offline(self):
>          return self.window.get_widget("migrate-offline").get_active()
> +
> +    def get_config_max_downtime(self):
> +        return int(self.window.get_widget("migrate-max-downtime").get_value())
> +
>      def get_config_secure(self):
>          return self.window.get_widget("migrate-secure").get_active()
>  
> @@ -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.

>          if self.get_config_interface_enabled() and interface == None:
>              return self.err.val_err(_("An interface must be specified."))
> @@ -402,6 +412,7 @@
>              destconn = self.get_config_destconn()
>              srchost = self.vm.get_connection().get_hostname()
>              dsthost = destconn.get_qualified_hostname()
> +            max_downtime = self.get_config_max_downtime()
>              live = not self.get_config_offline()
>              secure = self.get_config_secure()
>              uri = self.build_migrate_uri(destconn)
> @@ -418,7 +429,8 @@
>          self.topwin.window.set_cursor(gtk.gdk.Cursor(gtk.gdk.WATCH))
>  
>          progWin = vmmAsyncJob(self.config, self._async_migrate,
> -                              [self.vm, destconn, uri, rate, live, secure],
> +                              [self.vm, destconn, uri, rate, live, secure,
> +                               max_downtime],
>                                title=_("Migrating VM '%s'" % self.vm.get_name()),
>                                text=(_("Migrating VM '%s' from %s to %s. "
>                                        "This may take awhile.") %
> @@ -437,8 +449,24 @@
>              destconn.tick(noStatsUpdate=True)
>              self.close()
>  
> +    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.

> +            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

> +            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())))
> 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

> @@ -68,7 +68,7 @@
>                  <child>
>                    <widget class="GtkTable" id="table2">
>                      <property name="visible">True</property>
> -                    <property name="n_rows">4</property>
> +                    <property name="n_rows">5</property>
>                      <property name="n_columns">2</property>
>                      <property name="column_spacing">6</property>
>                      <property name="row_spacing">6</property>
> @@ -194,6 +194,55 @@
>                          <property name="bottom_attach">3</property>
>                        </packing>
>                      </child>
> +                    <child>
> +                      <widget class="GtkLabel" id="label1">
> +                        <property name="visible">True</property>
> +                        <property name="xalign">1</property>
> +                        <property name="label" translatable="yes">&lt;span color='#484848'&gt;Max downtime:&lt;/span&gt;</property>
> +                        <property name="use_markup">True</property>
> +                        <property name="use_underline">True</property>
> +                      </widget>
> +                      <packing>
> +                        <property name="top_attach">4</property>
> +                        <property name="bottom_attach">5</property>
> +                        <property name="x_options">GTK_FILL</property>
> +                      </packing>
> +                    </child>
> +                    <child>
> +                      <widget class="GtkHBox" id="hbox5">
> +                        <property name="visible">True</property>
> +                        <child>
> +                          <widget class="GtkSpinButton" id="migrate-max-downtime">
> +                            <property name="visible">True</property>
> +                            <property name="can_focus">True</property>
> +                            <property name="invisible_char">&#x25CF;</property>
> +                            <property name="activates_default">True</property>
> +                            <property name="adjustment">30 0 1000000 1 1000 0</property>
> +                            <property name="snap_to_ticks">True</property>
> +                            <property name="numeric">True</property>
> +                          </widget>
> +                          <packing>
> +                            <property name="position">0</property>
> +                          </packing>
> +                        </child>
> +                        <child>
> +                          <widget class="GtkLabel" id="label14">
> +                            <property name="visible">True</property>
> +                            <property name="xalign">0</property>
> +                            <property name="label" translatable="yes">ms</property>
> +                          </widget>
> +                          <packing>
> +                            <property name="position">1</property>
> +                          </packing>
> +                        </child>
> +                      </widget>
> +                      <packing>
> +                        <property name="left_attach">1</property>
> +                        <property name="right_attach">2</property>
> +                        <property name="top_attach">4</property>
> +                        <property name="bottom_attach">5</property>
> +                      </packing>
> +                    </child>
>                    </widget>
>                    <packing>
>                      <property name="expand">False</property>
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list redhat com
> https://www.redhat.com/mailman/listinfo/virt-tools-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]