[PKG-Openstack-devel] Bug#770706: Bug#770706: Bug#770706: Bug#770706: keystone.service does not start, /var/run/keystone not created

[Gaudenz Steinlin]

Hi

Thanks for working on this. This looks like a step in the right
direction. I however still have some problems with the proposed
approach.

Mikaël Cluseau <mcluseau at isi.nc> writes:

> Hi all,
>
> I tried to minimize the changes to get close to a unit file in the 
> systemd spirit, while still using on the init-script to keep same 
> features as before. The patch below reflect the following actions:
>

I still don't like the idea of using the sysv init script here. I'd
rather simplify the init script to the point where this is no longer
needed and we can just have the following in our unit file:
EnvironmentFile=/etc/default/openstack /etc/default/${PROJECT_NAME}

And then use the proper variables in the ExecStart setting.


> 1. don't auto-create the /var/*/${PROJECT_NAME} folder if not root, as 
> it will fail anyway.

Most of these should not be created by the init script anyway. Creating
/var/lib/X and /var/log/X there seems like a bug to me. These should be
part of the package. That's how it's done in all other packages I know.

> 2. add a do_systemd_start function to implement the systemd-start 
> argument that will exec' the daemon with the previously computed args.

That's an improvement over the current state. But how do you ensure that
the daemon is started in the foreground?

> 3. since then systemd will manage the daemon, the systemd-stop argument 
> can be removed.
> 4. since the script doesn't create the necessary folders anymore, create 
> what's required using the systemd unit file.
>
> The point 4 is where I did the most invasive choices; I assumed that
> 1. /var/run/${PROJECT_NAME} is not needed since PID file is not created 
> anymore (process managed by systemd);
> 2. /var/lib/${PROJECT_NAME} doesn't have to be created as it is created 
> when the package is installed;

The please remove it from the init script alltogether. It's also not
needed when under sysv then.

> 3. /var/lock/${PROJECT_NAME}, AFAIK, is not needed when using systemd;

What was it used for? Seems strange that there is a difference between
sysv and systemd here.

> 4. /var/log/${PROJECT_NAME} is the only one that is both required and 
> can be considered volatile.

No this should definitely not be considered volatile. It's ok to delete
the logfiles inside, but no one can expect the daemon to still work if
he just removes this directory completely. For all other packages I know
if this is part of the package and not created on the fly.

I would really prefer to have a full systemd unit file that does not
depend on the sysv script at all. It's fine and probably a good idea to
use the files in /etc/default/.

Gaudenz

>
> If any of these choices is wrong, it's easy to add the folder in the 
> ExecStartPre lines of the systemd unit file.
>
> If the systemd-stop argument should still be supported, I think it 
> should be noop or print a depreciation warning.
>
> As a consequence of these changes, the unit file doesn't need its 
> RuntimeDirectory and PIDFile directives. Since the daemon is exec'd, the 
> Type falls back to the default (simple) so it is removed too.
>
> I don't how to do less than that, so here is my minimal patch proposal:
>
> diff --git a/init-template/init-script-template 
> b/init-template/init-script-template
> index 0326b5d..fd20957 100644
> --- a/init-template/init-script-template
> +++ b/init-template/init-script-template
> @@ -36,11 +36,13 @@ fi
>   # Exit if the package is not installed
>   [ -x $DAEMON ] || exit 0
>
> -# Create /var/lock/X, /var/run/X, /var/lib/X and /var/log/X
> -for i in lock run log lib ; do
> -    mkdir -p /var/$i/${PROJECT_NAME}
> -    chown ${SYSTEM_USER} /var/$i/${PROJECT_NAME}
> -done
> +# If ran as root, create /var/lock/X, /var/run/X, /var/lib/X and 
> /var/log/X as needed
> +if [ "x$USER" = "xroot" ] ; then
> +    for i in lock run log lib ; do
> +        mkdir -p /var/$i/${PROJECT_NAME}
> +        chown ${SYSTEM_USER} /var/$i/${PROJECT_NAME}
> +    done
> +fi
>
>   # This defines init_is_upstart which we use later on (+ more...)
>   . /lib/lsb/init-functions
> @@ -65,6 +67,10 @@ do_stop() {
>       return "$RETVAL"
>   }
>
> +do_systemd_start() {
> +    exec $DAEMON $DAEMON_ARGS
> +}
> +
>   case "$1" in
>   start)
>       init_is_upstart > /dev/null 2>&1 && exit 1
> @@ -88,11 +94,8 @@ status)
>       status_of_proc "$DAEMON" "$NAME" && exit 0 || exit $?
>   ;;
>   systemd-start)
> -    do_start
> +    do_systemd_start
>   ;;
> -systemd-stop)
> -    do_stop
> -;;
>   restart|force-reload)
>       init_is_upstart > /dev/null 2>&1 && exit 1
>       log_daemon_msg "Restarting $DESC" "$NAME"
> @@ -110,7 +113,7 @@ restart|force-reload)
>       esac
>   ;;
>   *)
> -    echo "Usage: $SCRIPTNAME 
> {start|stop|status|restart|force-reload|systemd-start|systemd-stop}" >&2
> +    echo "Usage: $SCRIPTNAME 
> {start|stop|status|restart|force-reload|systemd-start}" >&2
>       exit 3
>   ;;
>   esac
> diff --git a/init-template/pkgos-gen-systemd-unit 
> b/init-template/pkgos-gen-systemd-unit
> index b97e2a9..09cf3e5 100755
> --- a/init-template/pkgos-gen-systemd-unit
> +++ b/init-template/pkgos-gen-systemd-unit
> @@ -33,12 +33,11 @@ $AFTER
>   [Service]
>   User=${SYSTEM_USER}
>   Group=${SYSTEM_GROUP}
> +PermissionsStartOnly=true
> +ExecStartPre=/bin/mkdir -p /var/log/${PROJECT_NAME}
> +ExecStartPre=/bin/chown ${SYSTEM_USER}:${SYSTEM_GROUP} 
> /var/log/${PROJECT_NAME}
>   ExecStart=${SCRIPTNAME} systemd-start
> -ExecStop=${SCRIPTNAME} systemd-stop
> -RuntimeDirectory=${PROJECT_NAME}
> -PIDFile=/var/run/${PROJECT_NAME}/${NAME}.pid
>   Restart=on-failure
> -Type=forking
>
>   [Install]
>   WantedBy=multi-user.target
>
> _______________________________________________
> Openstack-devel mailing list
> Openstack-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/openstack-devel