Created attachment 409296 [details] Example Init file WITH fallback to managedsave. The init script is written in such a way only QEMU hosts are shutdown.
Created attachment 409298 [details] conf.d/libvirtd Example
Hi Killian, I'm actually in the process of breaking apart the libvirtd init script from the starting of the network and guests. I'll attach the point I'm at now. But I've added support for specifying any URI.
Created attachment 409362 [details] OpenRC libvirtd init script
Created attachment 409364 [details] OpenRC libvirtd init conf
Created attachment 409366 [details] OpenRC libvirt-guests init script
Created attachment 409368 [details] OpenRC libvirt-guests init conf
Created attachment 409386 [details] OpenRC libvirt-guests init script Uploaded the wrong version.
(In reply to Doug Goldstein from comment #7) Hi Doug, just a quick comment on your current version. * I do not like the fact that the functionality is still implemented in two different places and offer slightly different functionality: systemd: /usr/libexec/libvirt-guests.sh openrc: /etc/init.d/libvirt-guests I would very much like to unify the behavior. What about the following changes: - /etc/init.d/libvirt-guests -> utilizes /usr/libexec/libvirt-guests.sh - replace /usr/libexec/libvirt-guests.sh with our own version? * It is furthermore very unfortunate that the configuration of libvirt-guests is at two separate places: - /etc/conf.d/libvirt-guests - /etc/libvirt/libvirt-guests.conf Given the fact that we already migrate configuration bits from /etc/conf.d/libvirtd to somewhere else, why not unifying those? The only obstacle is a clash of policies: openrc's configuration shall go to /etc/conf.d, systemd configuration shall not be there... This is the reason why I migrated libvirt-guests.conf (away from /etc/sysconfig) to /etc/libvirt/* I would really like to see those two points addressed. Further it makes future maintenance and feature additions (such as the stuff discussed in this bug report) immediately available for both init systems.
(In reply to Matthias Maier from comment #8) > (In reply to Doug Goldstein from comment #7) > > Hi Doug, just a quick comment on your current version. > > * I do not like the fact that the functionality is still implemented in two > different places and offer slightly different functionality: > > systemd: /usr/libexec/libvirt-guests.sh > openrc: /etc/init.d/libvirt-guests > > I would very much like to unify the behavior. What about the following > changes: > > - /etc/init.d/libvirt-guests -> utilizes /usr/libexec/libvirt-guests.sh > > - replace /usr/libexec/libvirt-guests.sh with our own version? Well like I mentioned in my email. I hacked on this stuff over a year ago and its a matter of finding my latest version. I can't remember why I didn't commit it. At the time I didn't use libvirt-guests.sh[k0iyAQ > * It is furthermore very unfortunate that the configuration of > libvirt-guests is > at two separate places: > > - /etc/conf.d/libvirt-guests > - /etc/libvirt/libvirt-guests.conf > > Given the fact that we already migrate configuration bits from > /etc/conf.d/libvirtd to somewhere else, why not unifying those? The only > obstacle is a clash of policies: openrc's configuration shall go to > /etc/conf.d, systemd configuration shall not be there... > > This is the reason why I migrated libvirt-guests.conf (away from > /etc/sysconfig) to /etc/libvirt/* > > I would really like to see those two points addressed. Further it makes > future maintenance and feature additions (such as the stuff discussed in > this bug > report) immediately available for both init systems.
(In reply to Doug Goldstein from comment #9) > (In reply to Matthias Maier from comment #8) > > (In reply to Doug Goldstein from comment #7) > > > > Hi Doug, just a quick comment on your current version. > > > > * I do not like the fact that the functionality is still implemented in two > > different places and offer slightly different functionality: > > > > systemd: /usr/libexec/libvirt-guests.sh > > openrc: /etc/init.d/libvirt-guests > > > > I would very much like to unify the behavior. What about the following > > changes: > > > > - /etc/init.d/libvirt-guests -> utilizes /usr/libexec/libvirt-guests.sh > > > > - replace /usr/libexec/libvirt-guests.sh with our own version? > > Well like I mentioned in my email. I hacked on this stuff over a year ago > and its a matter of finding my latest version. I can't remember why I didn't > commit it. At the time I didn't use libvirt-guests.sh[k0iyAQ > Don't ask. Ok fine, I almost dropped my laptop there. But basically like I said in my email if you want to use libvirt-guests.sh by all means wire that up. My concern is its a sysvinit script that wants /etc/rc.d/init.d/functions and that might not play nice with runscript. > > > * It is furthermore very unfortunate that the configuration of > > libvirt-guests is > > at two separate places: > > > > - /etc/conf.d/libvirt-guests > > - /etc/libvirt/libvirt-guests.conf > > > > Given the fact that we already migrate configuration bits from > > /etc/conf.d/libvirtd to somewhere else, why not unifying those? The only > > obstacle is a clash of policies: openrc's configuration shall go to > > /etc/conf.d, systemd configuration shall not be there... > > > > This is the reason why I migrated libvirt-guests.conf (away from > > /etc/sysconfig) to /etc/libvirt/* > > > > I would really like to see those two points addressed. Further it makes > > future maintenance and feature additions (such as the stuff discussed in > > this bug > > report) immediately available for both init systems. My only concern would be now you've got an init script that's a unique snowflake and is managed by a config file that users of either init system won't expect.
I've pushed the scripts to my fork.
Matthias: I will say you're the maintainer so I'll defer to you.
(In reply to Doug Goldstein from comment #12) > Matthias: I will say you're the maintainer so I'll defer to you. This honors me :-] I will have a look at it later today and show you what changes I had in mind. Thanks a lot for the work on it!
"But I've added support for specifying any URI." I hope this includes multiple URI. I'm using both Qemu and LXC. Also IMO te fallback to managedsave option is very recommended: in case your ACPI shutdown fails (for example someone uninstalled the kvm-tools). You then have the choice between "janking the power-cord" or hibernate it deal with it later. Also maybe your OS doesn't have this option, but others have etc... (Note the example script works for me in production atm.)
(In reply to Killian De Volder from comment #14) > "But I've added support for specifying any URI." I hope this includes > multiple URI. I'm using both Qemu and LXC. This includes multiple URIs. Please have a thorough look at 1.2.18-r2 in the tree. Issues, remarks, suggestions for improvement highly welcomed! commit 75a2af9e00d8701cb3a6f201976ed9ba27bcc7d0 Author: Matthias Maier <tamiko@gentoo.org> Date: Wed Aug 19 20:07:43 2015 -0500 app-emulation/libvirt: Update init scripts for 1.2.18-r2 and 9999 update init scripts for 1.2.18-r2 und 9999. Drop keywords for testing due to major runscript change. Bugs: 551854 Bugs: 555736 Bugs: 558034 Package-Manager: portage-2.2.20.1 commit aed11adf8c5bdff71154e1c5e0c375951d237f36 Author: Doug Goldstein <cardoe@gentoo.org> Date: Tue Aug 18 16:08:56 2015 -0500 app-emulation/libvirt: break apart OpenRC init script This breaks apart the startup/shutdown of libvirtd from the startup/shutdown of the libvirt domains. Additionally this expands out the domain support to work for all the libvirtd backends instead of just QEMU. Signed-off-by: Doug Goldstein <cardoe@gentoo.org> Signed-off-by: Matthias Maier <tamiko@gentoo.org>
Suggestions / Remarks: ==> Remark: persistent/transient ? There is no need to do a separate --transient and --persistent listing in my test case. (I COULD BE WRONG!) But you might wish to double-check its necessity: # virsh list Id Name State ---------------------------------------------------- 2 win_mgmt running 3 win_an running 4 win_test running # virsh list --transient Id Name State ---------------------------------------------------- 4 win_test running # virsh list --persistent Id Name State ---------------------------------------------------- 2 win_mgmt running 3 win_an running ==> Suggestion: shift operators The old script has local uri= $1; shift; in there function definition. This was more flexible, but then again we don't really need it. ==> Comment: State-file The state-file is a nice addition. But can't help to think: if it's flagged as autostart in lxc, do we want really want to restart this domain ? -> Feature request: Allow this option to be turned on/off ? ==> Feature request: option to do a managedsave on failed shutdown Still think it's safer to suspend a domain if the shutdown failed, rather then nuking it. However to be reeeeaaaaaly save the domain should not restart after suspend, some sync apps might freak if they experience time issues ? So that results in another feature request -> disable autostart after "a managedsave on failed shutdown" ==> Looks good though.
(In reply to Killian De Volder from comment #16) > Suggestions / Remarks: > > ==> Remark: persistent/transient ? > There is no need to do a separate --transient and --persistent listing in my > test case. > (I COULD BE WRONG!) But you might wish to double-check its necessity: It's necessary when someone is using "managedsave" to stop the VMs (the default). You cannot call "managedsave" on a transient domain so we must call shutdown on those domains. In the case the user is using "shutdown" then yes there's no need to run them separately. But the complexity increase to handle those two cases does not seem worth it to me and its easier to follow if you treat them the same way. > > ==> Suggestion: shift operators > The old script has local uri= $1; shift; in there function definition. > This was more flexible, but then again we don't really need it. > Yes. Not quite sure why I did it that way. That can be fixed. > > ==> Comment: State-file > The state-file is a nice addition. > But can't help to think: if it's flagged as autostart in lxc, do we want > really want to restart this domain ? > -> Feature request: Allow this option to be turned on/off ? > If the domain is autostarted then the result of calling start on it is a no-op so there's no harm here. Is there then value in adding a flag to turn it on and off? The behavior is that any domain that was running and was not transient will resume running when the script starts back up. > > ==> Feature request: option to do a managedsave on failed shutdown > Still think it's safer to suspend a domain if the shutdown failed, rather > then nuking it. > However to be reeeeaaaaaly save the domain should not restart after suspend, > some sync apps might freak if they experience time issues ? > So that results in another feature request -> disable autostart after "a > managedsave on failed shutdown" > How do you determine that a domain failed to shutdown? There is no communication back and forth. Also how do you know that the domain supports suspend since suspend is also an ACPI operation. The result would be a lot of complexity because you then want another flag to avoid starting up the domain in the case of a shutdown failure which we couldn't prevent if the user has configured the domain to autostart with virsh in the XML.
> > > > ==> Comment: State-file > > The state-file is a nice addition. > > But can't help to think: if it's flagged as autostart in lxc, do we want > > really want to restart this domain ? > > -> Feature request: Allow this option to be turned on/off ? > > > > If the domain is autostarted then the result of calling start on it is a > no-op so there's no harm here. Is there then value in adding a flag to turn > it on and off? The behavior is that any domain that was running and was not > transient will resume running when the script starts back up. > How about a global ON_START=domstart/noop for all URIs?
(In reply to Doug Goldstein from comment #17) > (In reply to Killian De Volder from comment #16) > > > > ==> Comment: State-file > > The state-file is a nice addition. > > But can't help to think: if it's flagged as autostart in lxc, do we want > > really want to restart this domain ? > > -> Feature request: Allow this option to be turned on/off ? > If the domain is autostarted then the result of calling start on it is a > no-op so there's no harm here. Is there then value in adding a flag to turn > it on and off? The behavior is that any domain that was running and was not > transient will resume running when the script starts back up. The "problem" here is not the fact that it is a no-op. The problem is the user already specified if he wants to vm to run during start-up. But this is debatable, hence the feature request. In most situation this won't be a problem, and can easily be changed by editing the init script. > > ==> Feature request: option to do a managedsave on failed shutdown > > Still think it's safer to suspend a domain if the shutdown failed, rather > > then nuking it. > > However to be reeeeaaaaaly save the domain should not restart after suspend, > > some sync apps might freak if they experience time issues ? > > So that results in another feature request -> disable autostart after "a > > managedsave on failed shutdown" > How do you determine that a domain failed to shutdown? There is no > communication back and forth. Also how do you know that the domain supports > suspend since suspend is also an ACPI operation. The result would be a lot > of complexity because you then want another flag to avoid starting up the > domain in the case of a shutdown failure which we couldn't prevent if the > user has configured the domain to autostart with virsh in the XML. The same way you decide to destroy the domain, if the domain didn't disappear from libvirtd after a certain time-out. (See the example init file.) Again this can easily be edit in the init-file (and I will edit this, since I recently had a case where the VM was just fine, but for some reason didn't shut-down as requested).
I've pushed some updates. Please feel free to comment.
These scripts will likely debut with 1.2.19.
This is fixed in 1.2.19.