[lxc-devel] [PATCH] Rework init scripts
Dwight Engen
dwight.engen at oracle.com
Fri Sep 26 16:24:30 UTC 2014
On Fri, 26 Sep 2014 11:06:00 -0400
Stéphane Graber <stgraber at ubuntu.com> wrote:
> On Wed, Sep 24, 2014 at 07:16:32PM -0400, Dwight Engen wrote:
> > On Wed, 24 Sep 2014 16:13:48 -0400
> > Stéphane Graber <stgraber at ubuntu.com> wrote:
> >
> > > On Wed, Sep 24, 2014 at 04:05:33PM -0400, Dwight Engen wrote:
> > > > On Wed, 24 Sep 2014 13:17:28 -0400
> > > > Stéphane Graber <stgraber at ubuntu.com> wrote:
> > > >
> > > > > I've confirmed that the testsuite on all arches passes fine
> > > > > with that change and close inspection of the resulting
> > > > > package looked good too.
> > > > >
> > > > > It'd be nice to have someone confirm that make rpm works with
> > > > > that change as I don't have an easy way to try that.
> > > >
> > > > Hi Stéphane, make rpm is borken for the sysvinit case without
> > > > (the scripts get installed in the wrong place as can be seen
> > > > with "make install" since initdir isn't actually set):
> > > >
> > > > diff --git a/config/init/sysvinit/Makefile.am
> > > > b/config/init/sysvinit/Makefile.am index d166400..e8b9f4f 100644
> > > > --- a/config/init/sysvinit/Makefile.am
> > > > +++ b/config/init/sysvinit/Makefile.am
> > > > @@ -5,9 +5,9 @@ if INIT_SCRIPT_SYSV
> > > > # directly to the rc directory under the appropriate name.
> > > >
> > > > if HAVE_DEBIAN
> > > > - initdir = "init.d"
> > > > +initdir = "init.d"
> > > > else
> > > > - initdir = "rc.d/init.d"
> > > > +initdir = "rc.d/init.d"
> > > > endif
> > > >
> > > > install-sysvinit: lxc-containers lxc-net
> > >
> > > Thanks, I'll integrate that in my patch when pushing it upstream.
> > >
> > > > > There's the issue of sysconfig/lxc-net which I dropped and
> > > > > will need some other RPM-specific workaround if the file
> > > > > really needs to exist (as shipping a file upstream for the
> > > > > sole purpose of making the rpm DB happy isn't something I'm
> > > > > happy with).
> > > >
> > > > I have not yet tested that the built rpms actually work, will
> > > > get to that in a bit. I don't really like that
> > > > sysconfig/lxc-net is created in %post, this really should just
> > > > be a file that gets installed by make install. Also, I think
> > > > there needs to be a %postun that does a userdel lxc-dnsmasq.
> > > > I'll send a patch for that too if you agree.
> > >
> > > +1 on the postun
> >
> > Way down below is a patch with that and a few more things I had to
> > do to get this running right on my OL6.5 box.
> >
> > > For lxc-net, at least in Ubuntu we wanted to minimize the
> > > potential for configuration files conflicts on upgrade, which
> > > block the upgrade and require user intervention.
> >
> > Yeah, this is handled in rpm by marking it as %config. So in
> > the .spec we have:
> >
> > %config(noreplace) %{_sysconfdir}/sysconfig/*
> >
> > which makes /etc/sysconfig/lxc owned by the pkg but
> > not /etc/sysconfig/lxc-net since that is generated from %post. I
> > think that's what we want.
> >
> > > The way to do that was simply to have lxc-net not be part of the
> > > package at all but instead be entirely constructed from the
> > > maintainer script and only have it built that way if it doesn't
> > > exist.
> > >
> > > Additions to lxc-net won't appear on a system where LXC is already
> > > present, but that's not an issue since the default values are all
> > > set in the lxc-net script, the config file only serves as an
> > > override.
> > >
> > > Another reason not to have the file be registered as a conffile is
> > > that since the subnet is determined at installation time and so
> > > will likely vary between machines, this will cause a
> > > configuration file prompt with every single package update as
> > > even if the user never edited the file manually, it'll differ
> > > from whatever default one we may include in the package.
> >
> > Yeah, its nice that it picks a subnet for you, but it does worry me
> > a bit. I don't think its exactly foolproof, for example what if
> > something else was using the same algorithm to pick a subnet and it
> > did so in between the time our pkg is installed and the time the
> > service is started? I guess its the best we can do, I had a look at
> > the .spec from libvirt and it has this block:
> >
> > # We want to install the default network for initial RPM installs
> > # or on the first upgrade from a non-network aware libvirt only.
> > # We check this by looking to see if the daemon is already installed
> > /sbin/chkconfig --list libvirtd 1>/dev/null 2>&1
> > if test $? != 0 && test ! -f
> > %{_sysconfdir}/libvirt/qemu/networks/default.xml then
> > UUID=`/usr/bin/uuidgen`
> > sed -e "s,</name>,</name>\n <uuid>$UUID</uuid>," \
> > < %{_datadir}/libvirt/networks/default.xml \
> > > %{_sysconfdir}/libvirt/qemu/networks/default.xml
> > ln -s ../default.xml
> > %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml fi
> >
> > ... so it looks like they install the default bridge with a static
> > IP. Anyway, this is now working for me, thanks!
> >
> > > >
> > > > > Once this lands, I think we'll be good for alpha-2.
> > > > >
> > > > >
> > > > > On Wed, Sep 24, 2014 at 01:05:26PM -0400, Stéphane Graber
> > > > > wrote:
> > > > > > From: "Michael H. Warfield" <mhw at WittsEnd.com>
> > > > > >
> > > > > > This commit is based on the work of:
> > > > > > Signed-off-by: Michael H. Warfield <mhw at WittsEnd.com>
> > > > > >
> > > > > > A generic changelog would be:
> > > > > > - Bring support for lxcbr0 to all distributions
> > > > > > - Share the container startup and network configuration
> > > > > > logic across distributions and init systems.
> > > > > > - Have all the init scripts call the helper script.
> > > > > > - Support for the various different distro-specific
> > > > > > configuration locations to configure lxc-net and container
> > > > > > startup.
> > > > > >
> > > > > > Changes on top of Mike's original version:
> > > > > > - Remove sysconfig/lxc-net as it's apparently only there
> > > > > > as a workaround for an RPM limitation and is breaking Debian
> > > > > > systems by including a useless file which will get
> > > > > > registered as a package provided conffile in the dpkg
> > > > > > database and will therefore cause conffile prompts on
> > > > > > upgrades...
> > > > > > - Go with a consistant coding style in the various init
> > > > > > scripts.
> > > > > > - Split out the common logic from the sysvinit scripts and
> > > > > > ship both in their respective location rather than have
> > > > > > them be copies.
> > > > > > - Fix the upstart jobs so they actually work (there's no
> > > > > > such thing as libexec on Debian systems).
> > > > > >
> > > > > > Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
> > > > > > ---
> > > > [snip so msg is small enough to not await moderator approval]
> > > > _______________________________________________
> > > > lxc-devel mailing list
> > > > lxc-devel at lists.linuxcontainers.org
> > > > http://lists.linuxcontainers.org/listinfo/lxc-devel
> > >
> >
> >
> >
> >
> >
> > From: Dwight Engen <dwight.engen at oracle.com>
> > Date: Wed, 24 Sep 2014 18:24:34 -0400
> > Subject: [PATCH] minor fixups to init script rework
> >
> > - fix bug in action() fallback, need to shift away msg before
> > executing action
> >
> > - make lxc-net 98 so it starts before lxc-container (99), otherwise
> > the lxcbr0 won't be available when containers are autostarted
> >
> > - make the default RUNTIME_PATH be /var/run instead of /run. On
> > older distros (like ol6.5) /run doesn't exist. lxc-net will create
> > this directory and attempt to create the dnsmasq.pid file in it,
> > but this will fail when SELinux is enabled because the directory
> > will have the default_t type. Newer systems have /var/run symlinked
> > to /run so you get to the same place in that case.
> >
> > - add %postun to remove lxc-dnsmasq user when pkgs are removed
> >
> > - fix bug in lxc-oracle template that was
> > creating /var/lock/subsys/lxc as a dir and interfering with the
> > init scripts
> >
> > Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
>
> Thanks, some comments below.
>
> > ---
> > .gitignore | 7 ++++++-
> > config/init/common/lxc-containers.in | 3 ++-
> > config/init/common/lxc-net.in | 19 +++++++++++++++++++
> > config/init/sysvinit/Makefile.am | 4 ++--
> > config/init/sysvinit/lxc-net.in | 2 +-
> > configure.ac | 4 ++--
> > lxc.spec.in | 7 ++++++-
> > templates/lxc-oracle.in | 2 +-
> > 8 files changed, 39 insertions(+), 9 deletions(-)
> >
> > diff --git a/.gitignore b/.gitignore
> > index 0b6ec69..bd96fa4 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -113,10 +113,15 @@ config/missing
> > config/libtool.m4
> > config/lt*.m4
> > config/bash/lxc
> > +config/init/common/lxc-containers
> > +config/init/common/lxc-net
> > config/init/systemd/lxc-autostart-helper
> > -config/init/systemd/lxc.service
> > config/init/systemd/lxc-net.service
> > +config/init/systemd/lxc.service
> > config/init/sysvinit/lxc
> > +config/init/sysvinit/lxc-containers
> > +config/init/sysvinit/lxc-net
> > +config/sysconfig/lxc
> >
> > doc/*.1
> > doc/*.5
> > diff --git a/config/init/common/lxc-containers.in
> > b/config/init/common/lxc-containers.in index 9d1d604..77ee0f9 100644
> > --- a/config/init/common/lxc-containers.in
> > +++ b/config/init/common/lxc-containers.in
> > @@ -41,7 +41,8 @@ if ! type action >/dev/null 2>&1; then
> > # Real basic fallback for sysvinit "action" verbage.
> > action() {
> > echo -n "$1 "
> > - "$@" && echo "OK" || echo "Failed"
> > + shift
> > + "$@" && echo "OK" || echo "Failed"
> > }
> > fi
> >
> > diff --git a/config/init/common/lxc-net.in
> > b/config/init/common/lxc-net.in index c921ab7..4bd69c3 100644
> > --- a/config/init/common/lxc-net.in
> > +++ b/config/init/common/lxc-net.in
> > @@ -1,5 +1,6 @@
> > #!/bin/sh -
> >
> > +sysconfdir="@SYSCONFDIR@"
> > distrosysconfdir="@LXC_DISTRO_SYSCONF@"
> > localstatedir="@LOCALSTATEDIR@"
> > varrun="@RUNTIME_PATH@/lxc"
> > @@ -17,6 +18,20 @@ LXC_DHCP_MAX="253"
> > LXC_DHCP_CONFILE=""
> > LXC_DOMAIN=""
> >
> > +# Source function library.
> > +test ! -r "$sysconfdir"/rc.d/init.d/functions ||
> > + . "$sysconfdir"/rc.d/init.d/functions
> > +
> > +# provide action() fallback
> > +if ! type action >/dev/null 2>&1; then
> > + # Real basic fallback for sysvinit "action" verbage.
> > + action() {
> > + echo -n "$1 "
> > + shift
> > + "$@" && echo "OK" || echo "Failed"
> > + }
> > +fi
> > +
> > [ ! -f $distrosysconfdir/lxc ] || . $distrosysconfdir/lxc
> >
> > if [ -d "$localstatedir"/lock/subsys ]; then
> > @@ -30,6 +45,8 @@ start() {
> >
> > [ "x$USE_LXC_BRIDGE" = "xtrue" ] || { exit 0; }
> >
> > + action $"Starting LXC network bridge: " /bin/true
> > +
> > use_iptables_lock="-w"
> > iptables -w -L -n > /dev/null 2>&1 || use_iptables_lock=""
> > cleanup() {
> > @@ -80,6 +97,8 @@ stop() {
> > # if $LXC_BRIDGE has attached interfaces, don't shut it down
> > ls /sys/class/net/${LXC_BRIDGE}/brif/* > /dev/null 2>&1 &&
> > exit 0;
> > + action $"Stopping LXC network bridge: " /bin/true
> > +
> > if [ -d /sys/class/net/${LXC_BRIDGE} ]; then
> > use_iptables_lock="-w"
> > iptables -w -L -n > /dev/null 2>&1 || use_iptables_lock=""
>
> I'm not fond of adding init script logic into those scripts as this
> means that for example upstart will be logging those messages every
> time the script is called.
>
> I'd prefer we make sure the common scripts exit with the proper exit
> codes and print errors on stderr and then have the usual status
> messages be written by config/init/sysvinit/* rather than
> config/init/common/*
Sure that makes sense to me, I was just making lxc-net consistent with
lxc-container so it looks like we should move action out of both of
them and into the sysvinit wrapper.
> > diff --git a/config/init/sysvinit/Makefile.am
> > b/config/init/sysvinit/Makefile.am index d166400..e8b9f4f 100644
> > --- a/config/init/sysvinit/Makefile.am
> > +++ b/config/init/sysvinit/Makefile.am
> > @@ -5,9 +5,9 @@ if INIT_SCRIPT_SYSV
> > # directly to the rc directory under the appropriate name.
> >
> > if HAVE_DEBIAN
> > - initdir = "init.d"
> > +initdir = "init.d"
> > else
> > - initdir = "rc.d/init.d"
> > +initdir = "rc.d/init.d"
> > endif
> >
> > install-sysvinit: lxc-containers lxc-net
> > diff --git a/config/init/sysvinit/lxc-net.in
> > b/config/init/sysvinit/lxc-net.in index 6e22505..abf941d 100644
> > --- a/config/init/sysvinit/lxc-net.in
> > +++ b/config/init/sysvinit/lxc-net.in
> > @@ -2,7 +2,7 @@
> > #
> > # lxc-net Start/Stop LXC Networking
> > #
> > -# chkconfig: 345 99 01
> > +# chkconfig: 345 98 01
> > # description: Starts/Stops LXC Network Bridge
> > #
> > ### BEGIN INIT INFO
> > diff --git a/configure.ac b/configure.ac
> > index 7b54587..8c5487a 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -435,12 +435,12 @@ AC_ARG_ENABLE([tests],
> > [], [enable_tests=no])
> > AM_CONDITIONAL([ENABLE_TESTS], [test "x$enable_tests" = "xyes"])
> >
> > -# Allow overriding the default runtime dir (/run)
> > +# Allow overriding the default runtime dir (/var/run)
> > AC_ARG_WITH([runtime-path],
> > [AC_HELP_STRING(
> > [--with-runtime-path=dir],
> > [runtime directory (default: /run)]
> > - )], [], [with_runtime_path=['/run']])
> > + )], [], [with_runtime_path=['/var/run']])
>
> /run has been the standard cross-distro location for a while, I don't
> see any reason for changing that.
>
> If that's actually a problem for you and I suspect that may only be on
> pretty old versions of Oracle/RedHat, you can just pass
> --with-runtime-path when making the package (after doing the
> appropriate distro+release check).
Yeah, as I mentioned its a real problem since dnsmasq can't start. I
like your solution, I'll put that into the .spec and resend a v2.
Thanks!
> >
> > # LXC container path, where the containers are actually stored
> > # This is overridden by an entry in the file called LXCCONF
> > diff --git a/lxc.spec.in b/lxc.spec.in
> > index 52b6326..1de8bc4 100644
> > --- a/lxc.spec.in
> > +++ b/lxc.spec.in
> > @@ -63,6 +63,8 @@ Group: Applications/System
> > License: LGPLv2+
> > BuildRoot: %{_tmppath}/%{name}-%{version}-build
> > Requires: openssl rsync dnsmasq
> > +Requires(pre): /usr/sbin/useradd
> > +Requires(postun): /usr/sbin/userdel
> > # Note for Suse. The "docbook2X" BuildRequires does properly
> > # match docbook2x on Suse in a case insensitive manner
> > BuildRequires: libcap libcap-devel docbook2X graphviz libxslt
> > pkgconfig @@ -147,7 +149,7 @@ find %{buildroot} -type f -name
> > '*.la' -exec rm -f {} ';' rm -rf %{buildroot}
> >
> > %pre
> > -# Ensure that lxcdnsmasq uid & gid gets correctly allocated
> > +# Ensure that lxc-dnsmasq uid & gid gets correctly allocated
> > if getent passwd lxc-dnsmasq >/dev/null 2>&1 ; then : ; else \
> > /usr/sbin/useradd -M -r -s /sbin/nologin \
> > -c "LXC Networking Service" -d %_localstatedir/%name lxc-dnsmasq
> > 2> /dev/null \ @@ -200,6 +202,9 @@ LXC_DHCP_MAX="253"
> > EOF
> > fi
> >
> > +%postun
> > +/usr/sbin/userdel lxc-dnsmasq > /dev/null 2>&1 || :
> > +
> > %post libs -p /sbin/ldconfig
> > %postun libs -p /sbin/ldconfig
> >
> > diff --git a/templates/lxc-oracle.in b/templates/lxc-oracle.in
> > index 830ce5c..1f65d4c 100644
> > --- a/templates/lxc-oracle.in
> > +++ b/templates/lxc-oracle.in
> > @@ -598,7 +598,7 @@ container_rootfs_create()
> > fi
> > done
> >
> > - mkdir -p @LOCALSTATEDIR@/lock/subsys/lxc
> > + mkdir -p @LOCALSTATEDIR@/lock/subsys
> > (
> > flock -x 9
> > if [ $? -ne 0 ]; then
> > --
> > 1.7.1
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
>
More information about the lxc-devel
mailing list