GNOME Bugzilla – Bug 677690
Need a way to forcefully stop/reset a box
Last modified: 2016-03-31 13:53:40 UTC
Currently, if anything goes wrong in a box (kernel panic, ...), there is no way to force the box to shutdown or to restart it. This would be an equivalent to the power/reset button on physical hardware. This has been reported by a few people so far. Next question is how do we do it ;) context menu on the various boxes? something else?
Sound reasonable to me to have a menu and context menu item for that. Something that would show "Force shutdown" should be enough. It would go back to main view, and then clicking on the VM again would start it.
(In reply to comment #1) > Sound reasonable to me to have a menu and context menu item for that. Something > that would show "Force shutdown" should be enough. It would go back to main > view, and then clicking on the VM again would start it. Agreed but while we do that, it wouldn't hurt to provide a 'Force reboot' option as well since these both translate to direct libvirt functions. It goes without saying that this will apply only to libvirt machines..
Created attachment 216068 [details] [review] libvirt: provide a menu entry to force shutdown I would prefer to only show the force shutdown entry when the Box can be shutdown, via libvirt. But it doesn't seem to be easy to remove or hide an item from the global menu. Also, I wonder if from a UI POV this is very desirable, since it may or may not be in the menu.
No clue about what we want from an UI point of view, would be good to get some design team blessing with that.
Review of attachment 216068 [details] [review]: I think your doubt on whether it should be a global menu option is very much justified. I would suggest an option in the selection mode. We can also provide an option in the context menu if/when we have that (bug#674674). ::: src/libvirt-machine.vala @@ -402,0 +397,18 @@ + public void force_shutdown (bool confirm = true) { + Notificationbar.OKFunc do_force_shutdown = () => { + debug ("Force shutdown '%s'..", name); ... 15 more ... Why create a lambda if you are going to launch it sync straight afterwards rather than pass anywhere? Since you used Notificationbar.OKFunc, I suspect you originally intended to use Notificationbar for this? I would recommend using that actually rather than a MessageDialog. Something like what we do with deletion. "X being forcefully shutdown (may cause data loss)" with a Cancel button.
Review of attachment 216068 [details] [review]: ::: src/app.vala @@ +119,3 @@ display_section.append (_("Properties"), "app.display.properties"); display_section.append (_("Fullscreen"), "app.display.fullscreen"); + display_section.append (_("Force shutdown"), "app.display.shutdown"); we can remove that line for now to remove it from menu, but have the rest there and accessible via dbus. ::: src/libvirt-machine.vala @@ -402,0 +397,18 @@ + public void force_shutdown (bool confirm = true) { + Notificationbar.OKFunc do_force_shutdown = () => { + debug ("Force shutdown '%s'..", name); ... 15 more ... left-over, but it also looks nicer: var do_force_shutdown = () => { bar } ... if (foo) do_force_shutdown () vs var do_force_shutdown = false; if (foo) do_force_shutdown = true; if (force_shutdown) { bar } somehow I prefer the lambda version.
(In reply to comment #6) > Review of attachment 216068 [details] [review]: > > ::: src/app.vala > @@ +119,3 @@ > display_section.append (_("Properties"), > "app.display.properties"); > display_section.append (_("Fullscreen"), > "app.display.fullscreen"); > + display_section.append (_("Force shutdown"), > "app.display.shutdown"); > > we can remove that line for now to remove it from menu, but have the rest there > and accessible via dbus. > > ::: src/libvirt-machine.vala > @@ -402,0 +397,18 @@ > + public void force_shutdown (bool confirm = true) { > + Notificationbar.OKFunc do_force_shutdown = () => { > + debug ("Force shutdown '%s'..", name); > ... 15 more ... > > left-over, but it also looks nicer: > > var do_force_shutdown = () => { bar } ... > if (foo) > do_force_shutdown () > > vs > > var do_force_shutdown = false; > > if (foo) > do_force_shutdown = true; > > if (force_shutdown) { > bar > } > > somehow I prefer the lambda version. I was thinking more like this: if (confirm) { // run dialog if (response != OK) return; } // Force shutdown code here
(In reply to comment #7) > I was thinking more like this: > > if (confirm) { > // run dialog > if (response != OK) > return; > } > > // Force shutdown code here Right, that is fine in this case.
(In reply to comment #8) > (In reply to comment #7) > > I was thinking more like this: > > > > if (confirm) { > > // run dialog > > if (response != OK) > > return; > > } > > > > // Force shutdown code here > > Right, that is fine in this case. But what about my suggestion about use of Notificationbar and selection mode for this? Your currently lambda approach would fit there.
What about trying to get a designer to tell us how this should be done ? ;)
(In reply to comment #9) > But what about my suggestion about use of Notificationbar and selection mode > for this? I am not sure we should consider this as a notification/transient thing but rather like a real dialog that "freezes" the UI until confirmation/cancellation. It's also a current limitation, since notification-bar is clutter-ish thing, but the display is all in gtk. Anyway, that is easy to change later on if we can do it and decide to do it.
(In reply to comment #10) > What about trying to get a designer to tell us how this should be done ? ;) Imho, there is no need to ask and to wait for them for each and every change. They can play with the result and suggest to improve it. Let's go by iteration, but not just seat and wait.
(In reply to comment #11) > (In reply to comment #9) > > But what about my suggestion about use of Notificationbar and selection mode > > for this? > > I am not sure we should consider this as a notification/transient thing but > rather like a real dialog that "freezes" the UI until > confirmation/cancellation. One can say the exact same thing about box deletion. In that case, its even more important since reboot *may* cause data loss but deletion is guaranteed to cause data loss. Since designers chose not to go with this approach for deletion, they'll most probably not like to do it for this one either IMO. > It's also a current limitation, since notification-bar is clutter-ish thing, > but the display is all in gtk. Sorry, I didn't get it. How is that relevant/limiting? (In reply to comment #12) > (In reply to comment #10) > > What about trying to get a designer to tell us how this should be done ? ;) > > Imho, there is no need to ask and to wait for them for each and every change. > They can play with the result and suggest to improve it. Let's go by iteration, > but not just seat and wait. I agree and based on existing designs, we can attempt to predict what designers will say. However if we have disagreements about how UI should be and can't easily convince each other, we probably should try to get the designers' attention.
(In reply to comment #13) > One can say the exact same thing about box deletion. In that case, its even > more important since reboot *may* cause data loss but deletion is guaranteed to > cause data loss. Since designers chose not to go with this approach for > deletion, they'll most probably not like to do it for this one either IMO. Deletion can be un-done during notification. Reboot is not reversible, so confirmation should be a dialog.
(In reply to comment #14) > (In reply to comment #13) > > > One can say the exact same thing about box deletion. In that case, its even > > more important since reboot *may* cause data loss but deletion is guaranteed to > > cause data loss. Since designers chose not to go with this approach for > > deletion, they'll most probably not like to do it for this one either IMO. > > Deletion can be un-done during notification. Thats because we put it that way. We can handle deletion exactly like your patch is handling force shutdown. > Reboot is not reversible, so > confirmation should be a dialog. It doesn't need to be immediately actionable: "Forcefully shutting down in 5 seconds [Cancel]", where we update the counter each sec..
(In reply to comment #15) > Thats because we put it that way. We can handle deletion exactly like your > patch is handling force shutdown. In my mind, we want immediate action: - delete: immediate, but fake and can be reversed. - reboot: can't be faked > > Reboot is not reversible, so > > confirmation should be a dialog. > > It doesn't need to be immediately actionable: "Forcefully shutting down in 5 > seconds [Cancel]", where we update the counter each sec.. That's not immediate and is really poor way to ask for confirmation.
(In reply to comment #16) > (In reply to comment #15) > > Thats because we put it that way. We can handle deletion exactly like your > > patch is handling force shutdown. > > In my mind, we want immediate action: > - delete: immediate, but fake and can be reversed. > - reboot: can't be faked > > > > Reboot is not reversible, so > > > confirmation should be a dialog. > > > > It doesn't need to be immediately actionable: "Forcefully shutting down in 5 > > seconds [Cancel]", where we update the counter each sec.. > > That's not immediate and is really poor way to ask for confirmation. I do see your point but I still find it inconsistent UI. However since we don't seem to be getting designers' input, lets just go with your approach for starters. As you said, we can easily change this later. Do remember to modify your patch as agreed in comment#8 though.
Attachment 216068 [details] pushed as d7c57d8 - libvirt: provide a menu entry to force shutdown
just copying my answer from irc (In reply to comment #5) > Review of attachment 216068 [details] [review]: > > I think your doubt on whether it should be a global menu option is very much > justified. I would suggest an option in the selection mode. We can also provide > an option in the context menu if/when we have that (bug#674674). 12:04 < elmarco> zeenix: it's hardly something you want to do for several VM, and if the VM is running, you should first try to shut it down properly 12:05 < elmarco> so user will first connect to vm, try to shut it down normally, and only then force shutdown 12:05 * zeenix nods
(In reply to comment #19) > 12:05 < elmarco> so user will first connect to vm, try to shut it down > normally, and only then force shutdown I don't know how this is exposed in the UI, but one use case I can see for force-shutdown is when a box cannot be interacted with because of a spice/qemu/libvirt/... bug. In such cases, being able to force shutdown (ie kill qemu) from the overview would be useful.