[virt-tools-list] [virt-manager][PATCH v3] Add delete VM option in console viewer.

[Leonardo Augusto Guimarães Garcia]

On 06/17/2013 02:50 PM, Cole Robinson wrote:
> On 06/17/2013 01:06 PM, Leonardo Garcia wrote:
>> From: Leonardo Garcia <lagarcia at br.ibm.com>
>>
>> ---
>>   ui/vmm-delete.ui       |   46 +++++++++++++++++++++++++++++++++++++++++++++-
>>   ui/vmm-details.ui      |   15 +++++++++++++++
>>   virtManager/console.py |   10 ++++++++--
>>   virtManager/delete.py  |   35 +++++++++++++++++++++++++++++++++--
>>   virtManager/details.py |    6 ++++++
>>   virtManager/engine.py  |   16 ++++++++++++++++
>>   virtManager/manager.py |   20 ++------------------
>>   7 files changed, 125 insertions(+), 23 deletions(-)
>>
> Some comments below.
>
>> diff --git a/virtManager/delete.py b/virtManager/delete.py
>> index b7cb50d..ce55105 100644
>> --- a/virtManager/delete.py
>> +++ b/virtManager/delete.py
>> @@ -99,6 +99,10 @@ class vmmDeleteDialog(vmmGObjectUI):
>>   
>>           self.widget("delete-cancel").grab_focus()
>>   
>> +        # Show warning message if VM is running
>> +        vm_active = self.vm.is_active()
>> +        self.widget("delete-warn-running-vm-box").set_visible(vm_active)
>> +
>>           # Disable storage removal by default
>>           self.widget("delete-remove-storage").set_active(True)
>>           self.widget("delete-remove-storage").toggled()
>> @@ -140,10 +144,38 @@ class vmmDeleteDialog(vmmGObjectUI):
>>               if not ret:
>>                   return
>>   
>> +        conn = self.conn
>> +        vm = self.vm
>>           self.topwin.set_sensitive(False)
>>           self.topwin.get_window().set_cursor(Gdk.Cursor.new(Gdk.CursorType.WATCH))
>>   
>> -        title = _("Deleting virtual machine '%s'") % self.vm.get_name()
>> +        if vm.is_active():
>> +            def tmpcb(job, *args, **kwargs):
>> +                ignore = job
>> +                vm.destroy()
>> +            docb = tmpcb
>> +            title = _("Forcing VM Power off")
>> +            text = _("Powering off the VM in order to proceed with its deletion.")
>> +
>> +            syncjob = vmmAsyncJob(docb, [], title, text, self.topwin,
>> +                                  async=False)
>> +            error, details = syncjob.run()
>> +
>> +            if error is not None:
>> +                self.topwin.set_sensitive(True)
>> +                self.topwin.get_window().set_cursor(Gdk.Cursor.new(Gdk.CursorType.TOP_LEFT_ARROW))
>> +
>> +                self.err.show_err(error, details=details)
>> +
>> +                conn.tick(noStatsUpdate=True)
>> +
>> +                self.close()
>> +                return
>> +
>> +            logging.debug("Forced power off of vm '%s in order to proceed with "
>> +                          "its deletion'", vm.get_name())
>> +
>> +        title = _("Deleting virtual machine '%s'") % vm.get_name()
> Hmm, I don't really like the duplication here, both of the destroy logic and
> the little stuff like restoring the cursor/reporting error.
I didn't like that piece of code as well... but I was unable to figure 
out a better way of doing that at that time.
>
> Can we not just self.emit("action-vm-destroy") ? Then check the VM's state
> afterwards, and if it's still running, just exit without showing an error.
I've been thinking and I'll actually propose something much more 
simpler. Let's see what you think...

Best regards,

Leonardo Garcia
>
> Also, I'd prefer if we split this patch into two parts: first adding the
> delete option to the details dialog and the code movement to engine.py, next
> is this patch adding the new 'delete running' functionality. Should be pretty
> simple since this change is mostly isolated to delete.py, but you'll need to
> temporarily re-add the logic to not allow deleting a running VM. That way I
> can apply the first patch while we work on this bit.
>
> Thanks,
> Cole
>