Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 558034 - app-emulation/libvirt: libvirtd doesn't shutdown LXC
Summary: app-emulation/libvirt: libvirtd doesn't shutdown LXC
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: [OLD] Server (show other bugs)
Hardware: All All
: Normal normal (vote)
Assignee: Matthias Maier
URL: https://github.com/cardoe/gentoo/comm...
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-17 18:50 UTC by Killian De Volder
Modified: 2015-09-17 15:08 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
Example Init file WITH fallback to managedsave. (libvirtd,5.41 KB, text/plain)
2015-08-17 18:50 UTC, Killian De Volder
Details
conf.d/libvirtd Example (conf.d.libvird,2.24 KB, text/plain)
2015-08-17 18:50 UTC, Killian De Volder
Details
OpenRC libvirtd init script (libvirtd.init-r15,1.12 KB, text/plain)
2015-08-18 14:45 UTC, Doug Goldstein (RETIRED)
Details
OpenRC libvirtd init conf (libvirtd.confd-r5,504 bytes, text/plain)
2015-08-18 14:46 UTC, Doug Goldstein (RETIRED)
Details
OpenRC libvirt-guests init script (libvirt-guests.init,3.22 KB, text/plain)
2015-08-18 14:46 UTC, Doug Goldstein (RETIRED)
Details
OpenRC libvirt-guests init conf (libvirt-guests.confd,1.52 KB, text/plain)
2015-08-18 14:47 UTC, Doug Goldstein (RETIRED)
Details
OpenRC libvirt-guests init script (libvirt-guests.init,4.64 KB, text/plain)
2015-08-18 18:14 UTC, Doug Goldstein (RETIRED)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Killian De Volder 2015-08-17 18:50:12 UTC
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.
Comment 1 Killian De Volder 2015-08-17 18:50:56 UTC
Created attachment 409298 [details]
conf.d/libvirtd Example
Comment 2 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 14:38:46 UTC
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.
Comment 3 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 14:45:43 UTC
Created attachment 409362 [details]
OpenRC libvirtd init script
Comment 4 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 14:46:21 UTC
Created attachment 409364 [details]
OpenRC libvirtd init conf
Comment 5 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 14:46:43 UTC
Created attachment 409366 [details]
OpenRC libvirt-guests init script
Comment 6 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 14:47:07 UTC
Created attachment 409368 [details]
OpenRC libvirt-guests init conf
Comment 7 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 18:14:48 UTC
Created attachment 409386 [details]
OpenRC libvirt-guests init script

Uploaded the wrong version.
Comment 8 Matthias Maier gentoo-dev 2015-08-18 18:54:57 UTC
(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.
Comment 9 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 20:38:38 UTC
(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.
Comment 10 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 20:41:00 UTC
(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.
Comment 11 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 21:27:44 UTC
I've pushed the scripts to my fork.
Comment 12 Doug Goldstein (RETIRED) gentoo-dev 2015-08-18 21:58:41 UTC
Matthias: I will say you're the maintainer so I'll defer to you.
Comment 13 Matthias Maier gentoo-dev 2015-08-18 22:12:57 UTC
(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!
Comment 14 Killian De Volder 2015-08-19 06:25:07 UTC
"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.)
Comment 15 Matthias Maier gentoo-dev 2015-08-20 01:15:24 UTC
(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>
Comment 16 Killian De Volder 2015-08-20 07:52:47 UTC
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.
Comment 17 Doug Goldstein (RETIRED) gentoo-dev 2015-08-24 21:11:12 UTC
(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 18 Doug Goldstein (RETIRED) gentoo-dev 2015-08-24 21:18:50 UTC
> > 
> > ==> 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?
Comment 19 Killian De Volder 2015-08-24 21:41:26 UTC
(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).
Comment 20 Doug Goldstein (RETIRED) gentoo-dev 2015-08-28 13:48:54 UTC
I've pushed some updates. Please feel free to comment.
Comment 21 Doug Goldstein (RETIRED) gentoo-dev 2015-08-28 13:49:44 UTC
These scripts will likely debut with 1.2.19.
Comment 22 Doug Goldstein (RETIRED) gentoo-dev 2015-09-17 15:08:03 UTC
This is fixed in 1.2.19.