[virt-tools-list] [virt-manager][PATCH v2 2/2] Add delete VM option in console viewer.
Leonardo Augusto Guimarães Garcia
lagarcia at linux.vnet.ibm.com
Mon Jun 10 21:29:20 UTC 2013
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 at linux.vnet.ibm.com wrote:
>> From: Leonardo Garcia <lagarcia at 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
>
More information about the virt-tools-list
mailing list