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

Re: [virt-tools-list] [virt-manager][PATCH v2 2/2] Add delete VM option in console viewer.



Thanks for the review! :)

Comments below.

On 06/08/2013 08:30 PM, Cole Robinson wrote:
Thanks for the patches! General idea is fine, some comments below.

On 06/05/2013 03:40 PM, lagarcia linux vnet ibm com wrote:
From: Leonardo Garcia <lagarcia br ibm com>

---
  ui/vmm-details.ui        |   15 +++++++++++++++
  virtManager/baseclass.py |    5 +++--
  virtManager/details.py   |   11 +++++++++--
  virtManager/engine.py    |   46 +++++++++++++++++++++++++++++++++++++++++++++-
  4 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/ui/vmm-details.ui b/ui/vmm-details.ui
index ddb2a71..ea4b53e 100644
--- a/ui/vmm-details.ui
+++ b/ui/vmm-details.ui
@@ -314,6 +314,21 @@
                        </object>
                      </child>
                      <child>
+                      <object class="GtkMenuItem" id="details-menu-delete">
+                        <property name="visible">True</property>
+                        <property name="can_focus">False</property>
+                        <property name="label" translatable="yes">_Delete...</property>
+                        <property name="use_underline">True</property>
+                        <signal name="activate" handler="on_details_menu_delete_activate" swapped="no"/>
+                      </object>
+                    </child>
+                    <child>
+                      <object class="GtkSeparatorMenuItem" id="separator2">
+                        <property name="visible">True</property>
+                        <property name="can_focus">False</property>
+                      </object>
+                    </child>
+                    <child>
                        <object class="GtkMenuItem" id="details-menu-vm-screenshot">
                          <property name="visible">True</property>
                          <property name="can_focus">False</property>
diff --git a/virtManager/baseclass.py b/virtManager/baseclass.py
index 7bc7812..c3a093e 100644
--- a/virtManager/baseclass.py
+++ b/virtManager/baseclass.py
@@ -194,8 +194,9 @@ class vmmGObjectUI(vmmGObject):
          self.close()
          vmmGObject.cleanup(self)
          self.builder = None
-        self.topwin.destroy()
-        self.topwin = None
+        if self.topwin:
+            self.topwin.destroy()
+            self.topwin = None
          self.uifile = None
          self.err = None
Hmm, I can see from the vmmGObjectUI code why this is needed but it shouldn't
be required in practice. I've pushed a cleanup now that should make this
unnecessary, so please drop this bit.
I thought these pieces you commented about would require some more explanation.

With the changes made in the --show* command line option, I am usually running virt-manager with only one window opened: the details window. If I close this window, the following sequence of events will happen:

user action: close window -> call: vmmDetails.close -> emit: details-closed -> call: vmmEngine.decrement_window_counter --- as it is the last window ---> call: vmmEngine.exit_app -> call: vmmEngine._cleanup -> call: vmmEngine.cleanup_conn -> call: vmmDetails.cleanup (definition in base class) -> call: vmmDetails.close

I think it is easier to see this through the stacktrace:

  File "./virt-manager", line 306, in <module>
    main()
  File "./virt-manager", line 301, in main
    engine.application.run(None)
File "/usr/lib64/python2.7/site-packages/gi/types.py", line 47, in function
    return info.invoke(*args, **kwargs)
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 590, in close
    return self._close()
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 615, in _close
    self.emit("details-closed")
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 157, in emit
    return GObject.GObject.emit(self, signal_name, *args)
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 336, in decrement_window_counter
    self.exit_app(src)
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 393, in exit_app
    self.cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 66, in cleanup
    self._cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 385, in _cleanup
    self.cleanup_conn(uri)
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 474, in cleanup_conn
    win.cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 194, in cleanup
    self.close()
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 590, in close
    return self._close()

This sequence works fine as, eventhough vmmDetails.close is reentrant, vmmDetails.cleanup is called only once.

However, when we have only the details window opened and we choose to delete the VM (the purpose of this patch proposal), something different happens:

emit: vm-removed -> call: vmmEngine._do_vm_removed --- tries to cleanup the details window corresponding to the deleted VM ---> call vmmDetails.cleanup (definition in base class) -> call: vmmDetails.close -> emit: details-closed -> call: vmmEngine.decrement_window_counter --- as it is the last window ---> call: vmmEngine.exit_app -> call: vmmEngine._cleanup -> call: vmmEngine.cleanup_conn -> call: vmmDetails.cleanup (definition in base class) -> call: vmmDetails.close

Or, in the stack trace:

  File "./virt-manager", line 306, in <module>
    main()
  File "./virt-manager", line 301, in main
    engine.application.run(None)
File "/usr/lib64/python2.7/site-packages/gi/types.py", line 47, in function
    return info.invoke(*args, **kwargs)
File "/home/laggarcia/src/git/virt-manager/virtManager/connection.py", line 1330, in tick_send_signals
    self.emit("vm-removed", uuid)
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 157, in emit
    return GObject.GObject.emit(self, signal_name, *args)
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 243, in _do_vm_removed
    self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 194, in cleanup
    self.close()
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 590, in close
    return self._close()
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 615, in _close
    self.emit("details-closed")
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 157, in emit
    return GObject.GObject.emit(self, signal_name, *args)
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 336, in decrement_window_counter
    self.exit_app(src)
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 393, in exit_app
    self.cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 66, in cleanup
    self._cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 385, in _cleanup
    self.cleanup_conn(uri)
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 474, in cleanup_conn
    win.cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 194, in cleanup
    self.close()
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 590, in close
    return self._close()

The last call to vmmDetails.cleanup will destroy self.topwin. However, when the processing goes back to the first call to vmmDetails.cleanup, we will get an exception, as self.topwin has already been destroyed:

2013-06-10 16:56:55,719 (delete:74): Showing delete wizard
2013-06-10 16:57:00,644 (asyncjob:193): Creating async job for function cb=<bound method vmmDeleteDialog._async_delete of <vmmDeleteDialog object at 0x2fd1730 (virtManager+delete+vmmDeleteDialog at 0x321cf00)>> 2013-06-10 16:57:00,754 (delete:173): Threading off connection to delete vol. 2013-06-10 16:57:00,756 (delete:179): Deleting path: /var/lib/libvirt/images/Fedora18-test-clone.img
2013-06-10 16:57:00,927 (delete:187): Removing VM 'Fedora18-test-clone'
2013-06-10 16:57:02,326 (delete:83): Closing delete wizard
2013-06-10 16:57:02,600 (details:589): Closing VM details: <vmmDomain object at 0x235c870 (virtManager+domain+vmmDomain at 0x7f5894005240)>
2013-06-10 16:57:02,603 (engine:330): window counter decremented to 0
2013-06-10 16:57:02,618 (manager:211): Closing manager
2013-06-10 16:57:02,627 (delete:83): Closing delete wizard
2013-06-10 16:57:02,633 (details:589): Closing VM details: <vmmDomain object at 0x235c870 (virtManager+domain+vmmDomain at 0x7f5894005240)>
2013-06-10 16:57:02,781 (engine:408): Exiting app normally.
2013-06-10 16:57:02,782 (baseclass:68): Error cleaning up <vmmDetails object at 0x2514f50 (virtManager+details+vmmDetails at 0x2aa2320)>
Traceback (most recent call last):
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 66, in cleanup
    self._cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 564, in _cleanup
    self.console.cleanup()
AttributeError: 'NoneType' object has no attribute 'cleanup'
2013-06-10 16:57:02,785 (cliutils:87): Uncaught exception:
Traceback (most recent call last):
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 243, in _do_vm_removed
    self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 197, in cleanup
    self.topwin.destroy()
AttributeError: 'NoneType' object has no attribute 'destroy'

Traceback (most recent call last):
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 243, in _do_vm_removed
    self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 197, in cleanup
    self.topwin.destroy()
AttributeError: 'NoneType' object has no attribute 'destroy'

The changes you made in vmmGObjectUI fixed this issue, but didn't fix the other two issues below.

diff --git a/virtManager/details.py b/virtManager/details.py
index 8e927b1..1323232 100644
--- a/virtManager/details.py
+++ b/virtManager/details.py
@@ -329,6 +329,7 @@ class vmmDetails(vmmGObjectUI):
          "action-exit-app": (GObject.SignalFlags.RUN_FIRST, None, []),
          "action-view-manager": (GObject.SignalFlags.RUN_FIRST, None, []),
          "action-migrate-domain": (GObject.SignalFlags.RUN_FIRST, None, [str, str]),
+        "action-delete-domain": (GObject.SignalFlags.RUN_FIRST, None, [str, str]),
          "action-clone-domain": (GObject.SignalFlags.RUN_FIRST, None, [str, str]),
          "details-closed": (GObject.SignalFlags.RUN_FIRST, None, []),
          "details-opened": (GObject.SignalFlags.RUN_FIRST, None, []),
@@ -412,6 +413,7 @@ class vmmDetails(vmmGObjectUI):
              "on_details_menu_pause_activate": self.control_vm_pause,
              "on_details_menu_clone_activate": self.control_vm_clone,
              "on_details_menu_migrate_activate": self.control_vm_migrate,
+            "on_details_menu_delete_activate": self.control_vm_delete,
              "on_details_menu_screenshot_activate": self.control_vm_screenshot,
              "on_details_menu_view_toolbar_activate": self.toggle_toolbar,
              "on_details_menu_view_manager_activate": self.view_manager,
@@ -559,8 +561,9 @@ class vmmDetails(vmmGObjectUI):
          for serial in self.serial_tabs:
              self._close_serial_tab(serial)
- self.console.cleanup()
-        self.console = None
+        if self.console:
+            self.console.cleanup()
+            self.console = None

self.console should be unconditionally set, so I'm not sure why this is needed.
If the previous patch (mine or yours) is applied to baseclass.py, we will hit another error:

2013-06-10 17:51:59,007 (engine:408): Exiting app normally.
2013-06-10 17:51:59,008 (baseclass:68): Error cleaning up <vmmDetails object at 0x330fe60 (virtManager+details+vmmDetails at 0x7f2218004620)>
Traceback (most recent call last):
File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line 66, in cleanup
    self._cleanup()
File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line 564, in _cleanup
    self.console.cleanup()
AttributeError: 'NoneType' object has no attribute 'cleanup'

The cause for this error is the same as the previous one: vmmDetails cleanup will call vmmDetails._cleanup at some point. The last call to this function will correctly execute self.console.cleanup(). However, the first call will generate above error. Hence, the need for this check.

          self.vm = None
          self.conn = None
@@ -1580,6 +1583,10 @@ class vmmDetails(vmmGObjectUI):
          self.emit("action-migrate-domain",
                    self.vm.conn.get_uri(), self.vm.get_uuid())
+ def control_vm_delete(self, src_ignore):
+        self.emit("action-delete-domain",
+                  self.vm.conn.get_uri(), self.vm.get_uuid())
+
      def control_vm_screenshot(self, src_ignore):
          image = self.console.viewer.get_pixbuf()
diff --git a/virtManager/engine.py b/virtManager/engine.py
index 16ed552..7ab20e6 100644
--- a/virtManager/engine.py
+++ b/virtManager/engine.py
@@ -48,6 +48,7 @@ from virtManager.create import vmmCreate
  from virtManager.host import vmmHost
  from virtManager.error import vmmErrorDialog
  from virtManager.systray import vmmSystray
+from virtManager.delete import vmmDeleteDialog
# Enable this to get a report of leaked objects on app shutdown
  # gtk3/pygobject has issues here as of Fedora 18
@@ -95,6 +96,7 @@ class vmmEngine(vmmGObject):
          self.last_timeout = 0
self.systray = None
+        self.delete_dialog = None
          self.application = Gtk.Application(
                                   application_id="com.redhat.virt-manager",
                                   flags=0)
@@ -239,7 +241,9 @@ class vmmEngine(vmmGObject):
              return
self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
-        del(self.conns[hvuri]["windowDetails"][vmuuid])
+        if self.conns:
+            # The cleanup call above might end up emptying the conns dictionary
+            del(self.conns[hvuri]["windowDetails"][vmuuid])

How is this called after cleanup ?
The cause here is the same as explained for the two patch hunks commented above. Even with the two previous patches, if I delete the VM while the details window is the only opened window, I got the following error:

2013-06-10 18:01:10,171 (engine:408): Exiting app normally.
2013-06-10 18:01:10,172 (cliutils:87): Uncaught exception:
Traceback (most recent call last):
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 244, in _do_vm_removed
    del(self.conns[hvuri]["windowDetails"][vmuuid])
KeyError: 'qemu+ssh://root 192 168 122 1/system'

Traceback (most recent call last):
File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 244, in _do_vm_removed
    del(self.conns[hvuri]["windowDetails"][vmuuid])
KeyError: 'qemu+ssh://root 192 168 122 1/system'

This happens because when vmmEngine._do_vm_removed tries to cleanup the details window corresponding to the deleted VM, it will end up calling vmmDetails.close, which will, in turn, call vmmEngine.exit_app, as the details window is the last one opened, (see my initial comment here). However, vmmEngine.exit_app calls vmmEngine._cleanup, which, in turns, has the following statement in its last line:

        self.conns = {}

Hence, in the sequence of events shot by deleting a VM while the only window opened in virt-manager is the details window, vmmEngine.conns will be emptied before vmmEngine._do_vm_removed is able to delete the corresponding details window from the self.conns dictionary. That's why we need this additional check.

The best way I found to deal with this sequence of events was to implement the simple checks I suggested above. However, I am open to other suggestions here as well.

      def _do_conn_changed(self, conn):
          if (conn.get_state() == conn.STATE_ACTIVE or
@@ -373,6 +377,10 @@ class vmmEngine(vmmGObject):
              self.windowMigrate.cleanup()
              self.windowMigrate = None
+ if self.delete_dialog:
+            self.delete_dialog.cleanup()
+            self.delete_dialog = None
+
          # Do this last, so any manually 'disconnected' signals
          # take precedence over cleanup signal removal
          for uri in self.conns:
@@ -594,6 +602,7 @@ class vmmEngine(vmmGObject):
          obj.connect("action-exit-app", self.exit_app)
          obj.connect("action-view-manager", self._do_show_manager)
          obj.connect("action-migrate-domain", self._do_show_migrate)
+        obj.connect("action-delete-domain", self._do_delete_domain)
          obj.connect("action-clone-domain", self._do_show_clone)
          obj.connect("details-opened", self.increment_window_counter)
          obj.connect("details-closed", self.decrement_window_counter)
@@ -984,3 +993,38 @@ class vmmEngine(vmmGObject):
          logging.debug("Resetting vm '%s'", vm.get_name())
          vmmAsyncJob.simple_async_noshow(vm.reset, [], src,
                                          _("Error resetting domain"))
+
+    def _do_delete_domain(self, src, uri, uuid):
+        conn = self._lookup_conn(uri)
+        vm = conn.get_vm(uuid)
+        details_dialog = self._get_details_dialog(uri, uuid)
+
+        if vm.is_active():
+            if not util.chkbox_helper(src, self.config.get_confirm_delrunningvm,
+                self.config.set_confirm_delrunningvm,
+                text1=_("Are you sure you want to force poweroff '%s'?" %
+                        vm.get_name()),
+                text2=_("In order to delete a running VM, you first need to power "
+                        "it off. This will immediately power off the VM without "
+                        "shutting down the OS and may cause data loss.")):
+                return
+
+            logging.debug("Forced power off of vm '%s in order to proceed with "
+                          "its deletion'", vm.get_name())
+            def tmpcb(job, *args, **kwargs):
+                ignore = job
+                vm.destroy()
+            docb = tmpcb
+
+            asyncjob = vmmAsyncJob(docb, [], _("Forcing VM Power off"),
+                _("Powering off the VM in order to proceed with its deletion."),
+                details_dialog.topwin, async=False, show_progress=True)
+            error, details = asyncjob.run()
+            if error is not None:
+                error = _("Error shutting down domain") + ": " + error
+                src.err.show_err(error, details=details)
+                return
+
+        if not self.delete_dialog:
+            self.delete_dialog = vmmDeleteDialog()
+        self.delete_dialog.show(vm, details_dialog.topwin)
This bit looks fine, but please also change manager.py to use this as well,
rather than instantiate its own delete dialog.
Agreed. I'll work on that.

Best regards,

Leonardo Garcia

Thanks,
Cole



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