Change in vdsm[master]: Add vdsm upstart jobs

Zhou Zheng Sheng has uploaded a new change for review.

Change subject: Add vdsm upstart jobs
......................................................................

Add vdsm upstart jobs

Add Upstart jobs for vdsmd, supervdsmd, vdsm-restore-net-config,
vdsm-tmpfiles.

vdsmd, supervdsmd, vdsm-restore-net-config jobs are Upstart counterpart
of vdsmd.service, supervdsmd.service and vdsm-restore-net-config.service
in systemd.

vdsm-tmpfiles is to create directories under /var/run/ for vdsm and
supervdsm. In Upstart there is no systemd-tmpfiles.sercice equivalent,
so we implement simple script in vdsm-tmpfiles to do the necessary job.

Change-Id: Id68ec0197bd1e09100f5da96ac4db24f2b90753a
Signed-off-by: Zhou Zheng Sheng 
---
M .gitignore
M vdsm/Makefile.am
A vdsm/supervdsmd.upstart.in
A vdsm/vdsm-restore-net-config.upstart.in
A vdsm/vdsm-tmpfiles.upstart.in
A vdsm/vdsmd.upstart.in
6 files changed, 96 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/12/17812/1

diff --git a/.gitignore b/.gitignore
index 7802279..232a7bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -45,18 +45,22 @@
 vdsm/storage/12-vdsm-lvm.rules
 vdsm/storage/protect/safelease
 vdsm/sudoers.vdsm
+vdsm/supervdsmd.upstart
 vdsm/svdsm.logger.conf
 vdsm/vdscli.py
 vdsm/vdsm-gencerts.sh
 vdsm/vdsm-logrotate.conf
 vdsm/vdsm-restore-net-config.init
+vdsm/vdsm-restore-net-config.upstart
 vdsm/vdsm-sosplugin.py
 vdsm/vdsm-store-net-config
 vdsm/vdsm-tmpfiles.d.conf
+vdsm/vdsm-tmpfiles.upstart
 vdsm/vdsm.rwtab
 vdsm/vdsmd.8
 vdsm/vdsmd.init
 vdsm/vdsmd.service
+vdsm/vdsmd.upstart
 vdsm/vdsmd_init_common.sh
 vdsm/supervdsmd.init
 vdsm/supervdsmd.service
diff --git a/vdsm/Makefile.am b/vdsm/Makefile.am
index ba403da..feb6113 100644
--- a/vdsm/Makefile.am
+++ b/vdsm/Makefile.am
@@ -78,13 +78,17 @@
    sudoers.vdsm \
    supervdsmd.init \
    supervdsmd.service \
+	supervdsmd.upstart \
    svdsm.logger.conf \
    vdsm-logrotate.conf \
    vdsm-restore-net-config.init \
+	vdsm-restore-net-config.upstart \
    vdsm-tmpfiles.d.conf \
+	vdsm-tmpfiles.upstart \
    vdsm.rwtab \
    vdsmd.init \
    vdsmd.service \
+	vdsmd.upstart \
    $(NULL)

dist_vdsm_SCRIPTS = \
@@ -127,6 +131,7 @@
    sudoers.vdsm.in \
    supervdsmd.init.in \
    supervdsmd.service.in \
+	supervdsmd.upstart.in \
    svdsm.logger.conf.in \
    vdsm-gencerts.sh.in \
    vdsm-libvirt-access.pkla \
@@ -137,13 +142,16 @@
    vdsm-modules-load.d.conf \
    vdsm-restore-net-config.init.in \
    vdsm-restore-net-config.service \
+	vdsm-restore-net-config.upstart.in \
    vdsm-store-net-config.in \
    vdsm-sysctl.conf \
    vdsm-tmpfiles.d.conf.in \
+	vdsm-tmpfiles.upstart.in \
    vdsm.rwtab.in \
    vdsmd.8.in \
    vdsmd.init.in \
    vdsmd.service.in \
+	vdsmd.upstart.in \
    vdsmd_init_common.sh.in \
    $(NULL)

diff --git a/vdsm/supervdsmd.upstart.in b/vdsm/supervdsmd.upstart.in
new file mode 100644
index 0000000..a076493
--- /dev/null
+++ b/vdsm/supervdsmd.upstart.in
@@ -0,0 +1,20 @@
+# supervdsmd - Auxiliary vdsm service for running helper functions as root
+#
+
+description "Auxiliary vdsm service for running helper functions as root"
+
+start on runlevel [2345]
+stop on runlevel [!2345] or stopping libvirt-bin
+chdir "@VDSMDIR@"
+console log
+respawn
+
+# All commands called inside this script section except the daemon itself
+# should not fork, otherwise Upstart traces the wrong pid.
+# ".", "[", "&&" are built-in command or key-word, no fork.
+# bash exec does not fork, just execve the target binary.
+# So no "expect" stanza is needed.
+script
+    [ -f "/etc/default/supervdsmd" ] && . "/etc/default/supervdsmd"
+    exec "@VDSMDIR@/daemonAdapter" "@VDSMDIR@/supervdsmServer" --sockfile "@VDSMRUNDIR@/svdsm.sock"
+end script
diff --git a/vdsm/vdsm-restore-net-config.upstart.in b/vdsm/vdsm-restore-net-config.upstart.in
new file mode 100644
index 0000000..a18e67f
--- /dev/null
+++ b/vdsm/vdsm-restore-net-config.upstart.in
@@ -0,0 +1,12 @@
+# vdsm-restore-net-config - Restore failed Vdsm network configuration on boot
+#
+
+description "Restore failed Vdsm network configuration on boot"
+
+start on runlevel [2345] and started libvirt-bin
+chdir "@VDSMDIR@"
+console log
+
+task
+
+exec "@VDSMDIR@/vdsm-restore-net-config"
diff --git a/vdsm/vdsm-tmpfiles.upstart.in b/vdsm/vdsm-tmpfiles.upstart.in
new file mode 100644
index 0000000..426192f
--- /dev/null
+++ b/vdsm/vdsm-tmpfiles.upstart.in
@@ -0,0 +1,24 @@
+# vdsm-tmpfiles - Automatically create tempfiles under /var/run for vdsm
+#
+
+description "Automatically create tempfiles under /var/run for vdsm"
+
+start on startup
+console log
+
+task
+
+script
+while read Type Path Mode User Group ; do
+    if [ ! -e "$Path" ]; then
+        if [ "$Type" = "d" ]; then
+            @MKDIR_P@ "$Path"
+        else
+            echo "Type $Type handling is not implemented"
+            exit 1
+        fi
+    fi
+    "@CHOWN_PATH@" $User:$Group "$Path"
+    "@CHMOD_PATH@" $Mode "$Path"
+done < "@VDSMDIR@/vdsm-tmpfiles.d.conf"
+end script
diff --git a/vdsm/vdsmd.upstart.in b/vdsm/vdsmd.upstart.in
new file mode 100644
index 0000000..ab0da59
--- /dev/null
+++ b/vdsm/vdsmd.upstart.in
@@ -0,0 +1,28 @@
+# vdsmd - Virtual Desktop Server Manager
+#
+
+description "Virtual Desktop Server Manager"
+
+start on runlevel [2345]
+stop on runlevel [!2345] or stopping networking or stopping portmap or stopping libvirt-bin or stopping supervdsmd
+chdir "@VDSMDIR@"
+console log
+nice -20
+respawn
+
+pre-start script
+    "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
+    "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
+end script
+
+# All commands called inside this script section except the daemon itself
+# should not fork, otherwise Upstart traces the wrong pid.
+# ".", "[", "&&" are built-in command or key-word, no fork.
+# bash exec does not fork, just execve the target binary.
+# So no "expect" stanza is needed.
+script
+    [ -f "/etc/default/vdsmd" ] && . "/etc/default/vdsmd"
+    exec start-stop-daemon --chuid @VDSMUSER@:@VDSMGROUP@ --start --exec "@VDSMDIR@/daemonAdapter" -- -c /dev/null "@VDSMDIR@/vdsm"
+end script
+
+post-stop exec "@LIBEXECDIR@/vdsmd_init_common.sh" --post-stop

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3810/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3727/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2919/ : SUCCESS

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 1: Verified+1

Test on Fedora, RHEL and Ubuntu by running xmlrpc functional tests.

To test this patch, we need some extra configuration in Ubuntu to install and setup VDSM properly. The installation script is a draft but just works. You can get all the things from https://github.com/edwardbadboy/vdsm-ubuntu . You also have to install latest libvirt from Debian sid and other dependencies from Ubuntu official repo or pypi. The dependencies are documented in https://github.com/edwardbadboy/vdsm-ubuntu/blob/master/debian/dependencyMap... .

In all
(1) clone https://github.com/edwardbadboy/vdsm-ubuntu.git

(2) add Debian sid repo to /etc/apt/sources.list,

deb http://ftp.us.debian.org/debian unstable main non-free contrib
 deb http://non-us.debian.org/debian-non-US unstable/non-US main contrib non-free

(3) Install all the dependencies documented in "debian/dependencyMap.txt", by either apt-get or pip.

(4) run ./ubuntuInstall.sh

I will package vdsm to .deb file in future patches, then you can install and test it easily.

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2: Verified+1

Rebase the patch because parent is changed and solve the conflicts. Test on Ubuntu.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3892/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3809/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3003/ : SUCCESS

Antoni Segura Puimedon has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2: Code-Review-1

I feel like maybe we should have repo directories for this:

/init/systemd
/init/upstart
/init/sysVinit
/init/openrc

etc.

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2:

(3 comments)

....................................................
File vdsm/supervdsmd.upstart.in
Line 2: #
Line 3: 
Line 4: description "Auxiliary vdsm service for running helper functions as root"
Line 5: 
Line 6: start on runlevel [2345]
shouldn't libvirtd be a dependency?
Line 7: stop on runlevel [!2345] or stopping libvirt-bin
Line 8: chdir "@VDSMDIR@"
Line 9: console log
Line 10: respawn

....................................................
File vdsm/vdsm-tmpfiles.upstart.in
Line 8: 
Line 9: task
Line 10: 
Line 11: script
Line 12: while read Type Path Mode User Group ; do
Please do not use 'Path' as is confusing with PATH

also I recommend using lower case variables in shell script... :)
Line 13:     if [ ! -e "$Path" ]; then
Line 14:         if [ "$Type" = "d" ]; then
Line 15:             @MKDIR_P@ "$Path"
Line 16:         else

....................................................
File vdsm/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
why can't you add this as dependencies?
Line 15:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 16: end script
Line 17: 
Line 18: # All commands called inside this script section except the daemon itself

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2:

(2 comments)

Thanks Antoni and Alon. I'm working on a new patch to move systemd, sysvinit and upstart files to respective sub-directories, then I'll rebase the current patch on that one. I'll also update this patch when we agree on the dependency solution.

....................................................
File vdsm/vdsm-tmpfiles.upstart.in
Line 8: 
Line 9: task
Line 10: 
Line 11: script
Line 12: while read Type Path Mode User Group ; do
Done
Line 13:     if [ ! -e "$Path" ]; then
Line 14:         if [ "$Type" = "d" ]; then
Line 15:             @MKDIR_P@ "$Path"
Line 16:         else

....................................................
File vdsm/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
1. Some of our dependencies are SysV services, but Upstart does not support depending on SysV services, so we need to start them if they are not up.

2. There is no SystemD-like dependency equivalent in Upstart job, because Upstart works in the reverse way to SystemD. If I use

start on started libvirt-bin and started portmap ...

, vdsmd will be started after libvirt-bin and portmap is started, but when libvirt-bin is stopped and I use initctl start vdsmd, Upstart does not automatically start libvirt-bin for me. To have Upstart automatically start dependencies as SystemD, I have to add another Upstart jobs as follow.

vdsmd-dependency.upstart.in
  start on starting vdsmd
  stop on stopped vdsmd
  task
  script
      "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
  end script

and in vdsmd.upstart.in

start on started libvirt-bin and started portmap ...
  stop on stopped vdsmd-dependency RESULT=failed or stopping libvirt-bin or stopping portmap ...

I think this translation is complicated, so I decide to call vdsmd_init_common.sh directly in pre-start script, simpler.

3. In fact there is a further problem.

I found a problem in Upstart, it does not fully restart libvirt-bin and re-read environment file.  So when I was implementing vdsm-tool service managment, I let service-restart firstly stop libvirt-bin, then start libvirt-bin again, to make it re-read the environment file.

When vdsmd starts and re-configures libvirt, the libvirt-bin service will be fully restarted (stop -> start) by vdsm-tool. If we have a dependency to libvirt-bin, then vdsmd will be stopped. This creates a circular wait situation. vdsmd is waiting for libvirt-bin to stop so as to start it again, but Upstart is waiting for vdsmd to stop before stopping libvirt-bin, at last Upstart itself is locked. So in the next patch I want to remove "stop on stopped libvirt-bin".

So the final solution looks like following.

start on runlevel [2345] and started networking and started supervdsmd and started libvirt-bin and started portmap
  stop on runlevel [!2345] or stopping supervdsmd

pre-start script
      "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "networking ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
  end script
Line 15:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 16: end script
Line 17: 
Line 18: # All commands called inside this script section except the daemon itself

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3922/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3839/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3033/ : SUCCESS

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 3: Verified+1

I write a patch to move init related files to /init. This patch rebases to the that one and moves Upstart related files to /init/upstart/, this makes /vdsm cleaner.

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
Few thoughts...

First, I do not like the use of the common code to start services. I think that each distribution has its own method of doing that and as this file is distribution specific best to use distribution tools so that maintainer will be able to maintain without knowledge of package internals.

Second... I read the upstart manual... and there is an option just to start services... what is the problem in that? I asking as I never actually used upstart...

# statd - NSM status monitor
 description   "NSM status monitor"
 author                "Steve Langasek "
 start on (started portmap or mounting TYPE=nfs)
 stop on stopping portmap
 expect fork
 respawn
 env DEFAULTFILE=/etc/default/nfs-common
 pre-start script
     if [ -f "$DEFAULTFILE" ]; then
         . "$DEFAULTFILE"
     fi
     [ "x$NEED_STATD" != xno ] || { stop; exit 0; }
     start portmap || true
     status portmap | grep -q start/running
     exec sm-notify
 end script
Line 15:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 16: end script
Line 17: 
Line 18: # All commands called inside this script section except the daemon itself

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
Yes, it supports starting other Upstart services in the job configuration file, and there is a small problem with that.

"start" is "/sbin/start -> initctl", and initctl can only control Upstart native services, to start sysv dependencies, it should call "service ntp start".

So if we use Upstart native service management, the pre-start section is like following.

pre-start script
    service ntp status >/dev/null 2>&1 || service ntp start
    service open-iscsi status >/dev/null 2>&1 || service open-iscsi start
    service multipath-tools status >/dev/null 2>&1 || service multipath-tools start
    service wdmd status >/dev/null 2>&1 || service wdmd start
    service sanlock status >/dev/null 2>&1 || service sanlock start
    start portmap || true
    status portmap | grep -q start/running
    start libvirt-bin || true
    status libvirt-bin | grep -q start/running
    start supervdsmd || true
    status supervdsmd | grep -q start/running
    "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
end

In Upstart if the service is already running, "start XXX" fails, and whatever the service is running or not, "status XXX" always succeeds. So it needs "start XXX || true" and "status XXX | grep -q start/running".

It looks like a half re-implemented (start_needed_srv + vdsm-tool service management). I think it a bit verbose, but not a big problem for me. I'm fine with both my solution and the native solution.
Line 15:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 16: end script
Line 17: 
Line 18: # All commands called inside this script section except the daemon itself

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
I think it looks much better from downstream maintainer point of view. Over abstraction is never good.

But I do recommend this:

for service in ntp open-iscsi multipath-tools wdmsd sanlock; do
     service "${service}" status > /dev/null 2>&1 || service "${service}" start
 done
 for service in portmap libvirt-bin supervdsmd; do
     status "${service}" | grep -q start/running || start "${service}"
 done

or:

for ...; do
     if [ -x "/etc/init.d/${service}" ]; then
         # do sysv
     else
         # do upstart
 done

or:

for ...; do
     if status "${service}" > /dev/null 2>&1; then
         # do upstart
     else
         # do sysv
 done
Line 15:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 16: end script
Line 17: 
Line 18: # All commands called inside this script section except the daemon itself

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     "@LIBEXECDIR@/vdsmd_init_common.sh" --start-needed-srv "ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd"
Wise suggestion, thanks!
Line 15:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 16: end script
Line 17: 
Line 18: # All commands called inside this script section except the daemon itself

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3968/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3885/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3079/ : SUCCESS

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 4: Verified+1

The new patch set start dependency services using Ubuntu native commands instead of vdsmd_init_common.sh service helper.

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 4: Code-Review+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4110/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3215/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4031/ : SUCCESS

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 5: Verified+1

The parent is updated so rebase this patch, no conflicts. In the new patch set, the tmpfile list is read from "@CONFDIR@/vdsm-tmpfiles.conf", so it need correct packaging to work. In http://gerrit.ovirt.org/#/c/18443/ , it packages vdsm-tmpfiles.d.conf to /etc/vdsm/vdsm-tmpfiles.conf .

Test on Ubuntu.

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

....................................................
File init/upstart/vdsmd.upstart.in
Line 18:             status "${srv}" | grep -q start/running
Line 19:         else
Line 20:             service "${srv}" status >/dev/null 2>&1 || service "${srv}" start
Line 21:         fi
Line 22:     done
question: have you checked if one of the service is failing (not the last)?

is this run using sh -xe ?
Line 23:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 24: end script
Line 25: 
Line 26: # All commands called inside this script section except the daemon itself

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 5:

(1 comment)

....................................................
File init/upstart/vdsmd.upstart.in
Line 18:             status "${srv}" | grep -q start/running
Line 19:         else
Line 20:             service "${srv}" status >/dev/null 2>&1 || service "${srv}" start
Line 21:         fi
Line 22:     done
Yes. According to the Upstart Cookbook, it is executed with -e (but no -x). So if a service fails to start, vdsmd is not started. From my debugging experience, when I make a mistake in the script, it does not continue, and vdsmd fails to start.

http://upstart.ubuntu.com/cookbook/#develop-scripts-using-bin-sh
Line 23:     "@LIBEXECDIR@/vdsmd_init_common.sh" --pre-start
Line 24: end script
Line 25: 
Line 26: # All commands called inside this script section except the daemon itself

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 6:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4230/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3335/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4151/ : SUCCESS

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 6: Code-Review+1

Yaniv Bronhaim has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

....................................................
File init/upstart/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     for srv in networking ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd; do
Why not sharing the required services list between services' scripts, instead of declaring it in each service script file? we can make conf file that translates between the actual service's name to the required service in the system. This can assist not to miss updating all the service scripts in each new dependency.

For example, the list will contain: network, iscsi, multipatch, sanlock, libvirt ...

and the conf file will be:

system="upstat"

if ${system} = 'upstat'; then

libvirt=libvirt-bin

sanlock=sanlock
..

elif ${system} = 'systemd'; then

libvirt=libvirtd.service

..

elif ${system} = 'initctl'; then

libvirt=libvirtd

..

required_services=${libvirt} ${sanlock} ..

and in vdsmd.upstat.in, vdsmd.init.in and vdsmd.service  we'll declare only

system='upstat'
...
  for srv in ${required_services}; do ..
...
Line 15:         if status "${srv}" >/dev/null 2>&1; then
Line 16:             # When srv is Upstart service, status srv always returns 0
Line 17:             start "${srv}" || true
Line 18:             status "${srv}" | grep -q start/running

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 6:

(1 comment)

....................................................
File init/upstart/vdsmd.upstart.in
Line 10: nice -20
Line 11: respawn
Line 12: 
Line 13: pre-start script
Line 14:     for srv in networking ntp open-iscsi multipath-tools wdmd sanlock portmap libvirt-bin supervdsmd; do
> Why not sharing the required services list between services' scripts, instead of declaring it in each service script file? we can make conf file that translates between the actual service's name to the required service in the system. This can assist not to miss updating all the service scripts in each new dependency.

Again... this only makes downstream packager's life harder.

If downstream maintains specific naming for service and we have downstream init.d script, we can use explicit dependencies and not abstraction nor conditionals.

This is downstream specific script, it uses downstream specific layout, commands, names and methods.
Line 15:         if status "${srv}" >/dev/null 2>&1; then
Line 16:             # When srv is Upstart service, status srv always returns 0
Line 17:             start "${srv}" || true
Line 18:             status "${srv}" | grep -q start/running

Zhou Zheng Sheng has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 6: Verified+1

Thanks Alon and Yaniv. Test on Ubuntu 1304.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Add vdsm upstart jobs
......................................................................

Patch Set 6: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Add vdsm upstart jobs
......................................................................

Add vdsm upstart jobs

Add Upstart jobs for vdsmd, supervdsmd, vdsm-restore-net-config,
vdsm-tmpfiles.

vdsmd, supervdsmd, vdsm-restore-net-config jobs are Upstart counterpart
of vdsmd.service, supervdsmd.service and vdsm-restore-net-config.service
in systemd.

vdsm-tmpfiles is to create directories under /var/run/ for vdsm and
supervdsm. In Upstart there is no systemd-tmpfiles.sercice equivalent,
so we implement simple script in vdsm-tmpfiles to do the necessary job.

Change-Id: Id68ec0197bd1e09100f5da96ac4db24f2b90753a
Signed-off-by: Zhou Zheng Sheng 
Reviewed-on: http://gerrit.ovirt.org/17812
Reviewed-by: Alon Bar-Lev 
Reviewed-by: Yaniv Bronhaim 
Reviewed-by: Dan Kenigsberg 
---
M .gitignore
M configure.ac
M init/Makefile.am
A init/upstart/Makefile.am
A init/upstart/supervdsmd.upstart.in
A init/upstart/vdsm-restore-net-config.upstart.in
A init/upstart/vdsm-tmpfiles.upstart.in
A init/upstart/vdsmd.upstart.in
8 files changed, 142 insertions(+), 1 deletion(-)

Approvals:
  Alon Bar-Lev: Looks good to me, but someone else must approve
  Yaniv Bronhaim: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Zhou Zheng Sheng: Verified