GNOME Bugzilla – Bug 673930
Reboot doesn't work when guest in grub/syslinux
Last modified: 2016-03-31 13:56:38 UTC
I recently tried to do an OS install that requires 768mb or ram, so the defaul 512 wasn't going to work. I went into the System settings thing, slide the slider to 1gb and clicked the Restart button in the popup dialog thing. At this point it didn't reboot, but just sat there. I was able to get the os installed by using virsh, editing the xml manually and running virsh create.
The ability to (cleanly) restart requires cooperation from the guest OS. Wonder if we should just restart forcibly (pull the plug out and back in) if clean restart fails. One thing to keep in mind is that there is no error when guest doesn't cooperate but I guess we can detect the failure by waiting for 'restarted' signal from the domain for a specific amount of seconds.
(In reply to comment #0) > > I was able to get the os installed by using virsh, editing the xml manually and > running virsh create. Bug#672554
in this case i was sitting at the grub screen, so i think forceful restart is the only way that could work?
s/grub/syslinux/
tangentially related libvirt bug I filed many moons ago: https://bugzilla.redhat.com/show_bug.cgi?id=560678
Fwiw, memory setting can now be edited before VM creation, and it's also possible to fore VM restarts, so this bug is much easier to workaround now.
Created attachment 226587 [details] [review] libvirt-machine: Stop if guest ignores shutdown request It is not guaranteed that guest will honor the ACPI shutdown (e.g GRUB). Force shutdown the guest if VM fails to shutdown in 5 seconds.
so in theory shutdown could be slower than 5 seconds. Maybe it should slide down a little infobar thing asking the user what to do after 5 seconds have elapsed? Still, attachment 226587 [details] [review] should handle the lion's share of cases alright i think.
So whats the verdict on the attached patch?
Forcing shutdown this way can cause data loss so we need to warn the user about what's going on and let him cancel the force shutdown
(In reply to comment #10) > Forcing shutdown this way can cause data loss so we need to warn the user about > what's going on and let him cancel the force shutdown I'm not so sure about that. Here we are only doing it because guest is not responding to ACPI event. AFAIK that means that its hung, has crashed or its in a state where force shutdown can't cause any harm (e.g at boot loader). In all these cases, user doesn't really have any choice but to force shutdown so cancelling would be just postponing the inevitable. Perhaps we should instead figure how long should we wait to be sure ACPI shutdown failed?
I think teuf's point was some machines may have perfectly ordered shut down that takes 10 seconds to complete. If we pull the cord for these guests at the 5 second mark, there may be data loss. Are you saying there's some sort of feedback provided by the guest os that it got the shutdown request, that the host is privvy to? And the patch, just waits 5 seconds for the feedback, not 5 seconds for the machine to finish the shutdown?
(In reply to comment #12) > I think teuf's point was some machines may have perfectly ordered shut down > that takes 10 seconds to complete. If we pull the cord for these guests at the > 5 second mark, there may be data loss. I understood his point and don't exactly disagree with it. I was just wonder it might be better (in terms of user experience) to wait longer before doing force shutdown rather than giving users a choice that is most probably going to be a fake one. > Are you saying there's some sort of feedback provided by the guest os that it > got the shutdown request, that the host is privvy to? No, if only that was true. :)
Actually I was thinking of OSes not supporting ACPI, or ignoring ACPI shutdown requests (I think that's doable?), but it's true that shutdown taking too long will also trigger this. With win7 applying updates on shutdown, the time it takes to actually shutdown can be arbitrarily long.
Created attachment 227763 [details] [review] libvirt-machine: Set status after stopping, not before Otherwise _FORCE_STOPPED state gets overridden by our domain state event handlers.
Created attachment 227764 [details] [review] libvirt-machine: Stop if guest ignores shutdown request v2: Ask user first.
Review of attachment 227763 [details] [review]: I don't understand this. The domain state handlers do: domain.stopped.connect (() => { if (state == MachineState.FORCE_STOPPED) return; // State already set by us when machine is forced to shutdown So, it should not be overridden, should it? Also, this will make us set the state to STOPPED first, and then FORCE_STOPPED. Maybe you can set some local state instead that we handle inside the domain.stopped handler to pick FORCE_STOPPED rather than STOPPED state.
Review of attachment 227764 [details] [review]: ::: src/libvirt-machine.vala @@ +629,3 @@ + Gtk.ButtonsType.YES_NO, + msg); + var response = dialog.run (); I don't think we want to run the dialog synchronously here. mainloop recursion like that is a risky business.
Review of attachment 227763 [details] [review]: Yeah, it shouldn't. I misread the code..
Review of attachment 227764 [details] [review]: ::: src/libvirt-machine.vala @@ +629,3 @@ + Gtk.ButtonsType.YES_NO, + msg); + var response = dialog.run (); How should it be done then? run_in_thread?
Review of attachment 227764 [details] [review]: ::: src/libvirt-machine.vala @@ +629,3 @@ + Gtk.ButtonsType.YES_NO, + msg); + var response = dialog.run (); Just show the dialog and connect to the response signal.
So right now when I move the slider, it puts this little floating info bar that says: Changes require restart of 'blah'. Attempt restart? [Yes] [X] (at least for this case) maybe instead of a dialog, the same little floating info bar should come down and say: "Restart operation is taking longer than expected. Force immediate restart? [Yes] [X]" ?
(In reply to comment #22) > So right now when I move the slider, it puts this little floating info bar that > says: > > Changes require restart of 'blah'. Attempt restart? [Yes] [X] > > (at least for this case) maybe instead of a dialog, the same little floating > info bar should come down and say: > > "Restart operation is taking longer than expected. Force immediate restart? > [Yes] [X]" > > ? Good thought, I think that would be more consistent. I'll think a bit more about it and provide another patch in either case.
Created attachment 227842 [details] [review] libvirt-machine: Stop if guest ignores shutdown request This version uses notificationbar rather than dialog.
Review of attachment 227842 [details] [review]: This makes more sense to me than the dialog. ::: src/libvirt-machine.vala @@ +630,3 @@ + shutdown_timeout = 0; + + return false; Does this timeout get a ref to "this". Its not clear from the code. If it doesn't we need to ensure that we also remove the timeout on finalize.
Created attachment 227892 [details] [review] libvirt-machine: Stop if guest ignores shutdown request This version keeps shutdown timeout id in the object and removes timeout on deletion of machine.
Comment on attachment 227763 [details] [review] libvirt-machine: Set status after stopping, not before Actually, this one is needed. Not because of the reason I mentioned but because the state change handler in the other patch gets called when you set the state. The handler try to start the domain and that fails because the domain state is in reality still running.. I'll update the commit log.
Created attachment 227893 [details] [review] libvirt-machine: Set status after stopping, not before Setting the state property will synchronously trigger all notifies on it and they'll have a wrong idea about the actual state of the machine if state has not actually changed.
Review of attachment 227892 [details] [review]: ::: src/libvirt-machine.vala @@ +637,3 @@ + shutdown_timeout = 0; + + return false; One issue with using a notification here is that by default that will time out after 6 seconds, but you might want to wait longer to see if it will reboot correctly, or you might miss the notification and not want it to reboot. So, i think we need to add some flag to the notification code to make it not have a timeout, requiring the user to nack or ack. We also want to get some kind of reference to the notification so we can remove it if the VM actually shuts down.
Review of attachment 227893 [details] [review]: ::: src/libvirt-machine.vala @@ +473,2 @@ domain.stop (0); + state = MachineState.FORCE_STOPPED; I don't like how this seems potentially racy wrt the event handler setting state to STOPPED first. I Realize this is a sync call, so that won't happen, but this still seems kinda risky when things change later on. It seems more reasonable to *only* change the state when we get an event from libvirt saying its state is changed, rather than having two divergent states. So, can you just set some local boolean that we requested a force shutdown, and then when we get the STOPPED event we set it to FORCE_STOPPED if that boolean was set.
Created attachment 228220 [details] [review] libvirt-machine: Set forced state from domain state change handler We should set the state only from the handler of underlying domain's state change to avoid any race-conditions.
Created attachment 228221 [details] [review] notificationbar: Allow tweaking of timeout Currently we just assume that 6 seconds timeout is enough for everyone. This patch makes it possible to set or even unset the timeout on the notifications.
Created attachment 228222 [details] [review] notificationbar: Provide users with Gd.Notification instance The notification displaying methods now returns the underlying Gd.Notification instance so that users of this API can do things like dismissing the displayed notification.
Created attachment 228224 [details] [review] libvirt-machine: Stop if guest ignores shutdown request New in this version: * "Force shutdown" notification does not timeout. * "Force shutdown" notification is dismissed automatically if shutdown happens before user hits a button.
Looks all good to me!
Attachment 228220 [details] pushed as b43ad37 - libvirt-machine: Set forced state from domain state change handler Attachment 228221 [details] pushed as 496f477 - notificationbar: Allow tweaking of timeout Attachment 228222 [details] pushed as c7f9207 - notificationbar: Provide users with Gd.Notification instance Attachment 228224 [details] pushed as 581b43c - libvirt-machine: Stop if guest ignores shutdown request
thanks for all the work on this, you really went the extra mile.