[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.



On 06/10/2013 05:29 PM, Leonardo Augusto GuimarĂ£es Garcia wrote:
> 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.

Thanks for the thorough explanations! Indeed all these issues can be hit with
current code: have the last open window be a details window, and 'virsh
undefine' the VM behind virt-manager's back.

I pushed a patch upstream that defers vmmEngine.exit_app to an idle callback,
to let the vmmDetails cleanup bits finish and remove themselves from the
window tracking before we try and do a final cleanup pass. That should fix
these issues.

Thanks,
Cole


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