Change in vdsm[master]: vdsmd.init: Add init adapter

Alon Bar-Lev alonbl at redhat.com
Wed Jun 12 07:21:48 UTC 2013


Alon Bar-Lev has posted comments on this change.

Change subject: vdsmd.init: Add init adapter
......................................................................


Patch Set 1: (17 inline comments)

Thanks!

Some more questions...

....................................................
File vdsm/init/daemonAdapter
Line 1: #!/usr/bin/python
So Dan was right, the whole thing should be written in python.

Doing this hybrid solution is not good... either you use shell or python.
Line 2: # Copyright 2013 IBM, Inc.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by


Line 38:                         help='Ajust nice according to configuration file')
Line 39:     parser.add_argument('-r', '--respawn-pid-file', dest='respawnPidFile',
Line 40:                         default=None, metavar='respawn_pid_file_path',
Line 41:                         help='Run with respawn watchdog protection and '
Line 42:                              'specify the respawn pid file path')
If we use respawn of systemd/upstart we should be able to use vdsm directly without the respawn, and in this case it should not fork and not create pid.

If we use sysvinit we use the respawn and keep the pid of the respawn process, and monitor this one, this is actually the pid of the service and for making it clear it is --pidfile.

If the respown->vdsm communication needs vdsm pid, ok... but this is not the service  pid.

Each service should have single pid, of top level process that if stopped stop the complete process tree.
Line 43:     args = parser.parse_args(sys.argv[1:])
Line 44:     return args
Line 45: 
Line 46: if __name__ == '__main__':


Line 51:             os.nice(config.getint('vars', 'vdsm_nice'))
Line 52:         except OSError as e:
Line 53:             sys.stderr.write("daemon adapter: failed to renice, %s" % e)
Line 54: 
Line 55:     if args.user is not None:
I would like to avoid as much possible to change identity, all downstream services supports that... unless we force to...
Line 56:         userPw = pwd.getpwnam(args.user)
Line 57:         os.environ.update({
Line 58:             'LOGNAME': userPw.pw_name,
Line 59:             'USER': userPw.pw_name,


Line 53:             sys.stderr.write("daemon adapter: failed to renice, %s" % e)
Line 54: 
Line 55:     if args.user is not None:
Line 56:         userPw = pwd.getpwnam(args.user)
Line 57:         os.environ.update({
regardless, this should be done on the copy of the environment.
Line 58:             'LOGNAME': userPw.pw_name,
Line 59:             'USER': userPw.pw_name,
Line 60:             'LNAME': userPw.pw_name,
Line 61:             'USERNAME': userPw.pw_name})


Line 62:         os.setgid(userPw.pw_gid)
Line 63:         os.setuid(userPw.pw_uid)
Line 64: 
Line 65:     cmd = []
Line 66:     if args.respawnPidFile:
Do not use implicits in code.

If you intend to run the respwan, state so explicitly.

 --use-respawn

Or similar.
Line 67:         cmd += [os.path.join(P_VDSM, 'respawn'), '--minlifetime', '10',
Line 68:                 '--daemon', '--masterpid', args.respawnPidFile]
Line 69: 
Line 70:     cmd.append(os.path.join(P_VDSM, 'vdsm'))


Line 64: 
Line 65:     cmd = []
Line 66:     if args.respawnPidFile:
Line 67:         cmd += [os.path.join(P_VDSM, 'respawn'), '--minlifetime', '10',
Line 68:                 '--daemon', '--masterpid', args.respawnPidFile]
The --daemon here is awkward... I expect it to be an argument of the wrapper.
Line 69: 
Line 70:     cmd.append(os.path.join(P_VDSM, 'vdsm'))
Line 71: 
Line 72:     env = os.environ


Line 82:     os.open("/dev/null", os.O_RDONLY)
Line 83:     os.close(1)
Line 84:     os.open("/dev/null", os.O_WRONLY)
Line 85:     os.close(2)
Line 86:     os.open("/dev/null", os.O_WRONLY)
Please add --console-redirect=file or similar to redirect output to a file, this should be optional and used in sysvinit, as in systemd you will get the messages into the journal which is ok.
Line 87: 


Line 84:     os.open("/dev/null", os.O_WRONLY)
Line 85:     os.close(2)
Line 86:     os.open("/dev/null", os.O_WRONLY)
Line 87: 
Line 88:     os.execve(cmd[0], cmd, env)
the root argument is the valid one, so we need to move the nice level to /etc/sysconfig/vdsmd to avoid parsing vdsm configuration. Also add Nice= to systemd, as far as I know it won't accept ${xxx} environment so user should copy the unit to /etc and modify it. This will allow using downstream services and conventions and avoid running the process as root.


....................................................
File vdsm.spec.in
Line 119: Requires: sanlock, sanlock-python
Line 120: Requires: selinux-policy-targeted
Line 121: %else
Line 122: Requires: python
Line 123: Requires: python-argparse
how vdsm parsed options so far? vdsm-tool and such? is it wise to add more dependencies?

Anyway, there is something[1] for python<2.7

[1] http://docs.python.org/2/library/optparse.html
Line 124: # Update the qemu-kvm requires when block_stream will be included
Line 125: Requires: qemu-kvm >= 2:0.12.1.2-2.295.el6_3.4
Line 126: Requires: qemu-img >= 2:0.12.1.2-2.295.el6_3.4
Line 127: Requires: libvirt >= 0.10.2-18.el6_4.4


....................................................
File vdsm/sudoers.vdsm.in
Line 35:     @REBOOT_PATH@ -f
Line 36: 
Line 37: vdsm  ALL=(ALL) NOPASSWD: VDSM_LIFECYCLE, VDSM_STORAGE
Line 38: Defaults:vdsm !requiretty
Line 39: Defaults:root !requiretty
this is dangerous.... you are changing something of the root of the system, why?


....................................................
File vdsm/vdsm
Line 145: 
Line 146: if __name__ == '__main__':
Line 147:     __assertVdsmUser()
Line 148:     __assertLogPermission()
Line 149:     try:
No better way to do so? example: getting the current group and compare? anything but try and fail.

BTW: shouldn't this be moved to the respawn?
Line 150:         os.setpgrp()
Line 151:     except OSError as e:
Line 152:         if e.errno == errno.EPERM:
Line 153:             # Powerful init systems such as systemd can provide us a clean


....................................................
File vdsm/vdsmd.init.in
Line 25
Line 26
Line 27
Line 28
Line 29
this is general note... I checked and if respawn is stopped then the vdsm is also stopped, why do we need this complexity of handing two pids within this init script?

the init script should be simple and only manage the respawn process, while this process should handle the vdsm process.


Line 68: 
Line 69:     echo $"Starting up vdsm daemon: "
Line 70:     local vdsm_nice=`$GETCONFITEM $CONF_FILE vars vdsm_nice -5`
Line 71: 
Line 72:     NICELEVEL=$vdsm_nice daemon --user=vdsm @VDSMDIR@/init/daemonAdapter -r $RESPAWNPIDFILE
the valid argument was the root argument.... :)
Line 73:     RETVAL=$?
Line 74:     [ "$RETVAL" -eq 0 ] && log_success_msg $"$prog start" || log_failure_msg $"$prog start"
Line 75:     [ "$RETVAL" -eq 0 ] && touch /var/lock/subsys/vdsmd
Line 76: }


....................................................
File vdsm/vdsmd.service.in
Line 6: 
Line 7: [Service]
Line 8: Type=simple
Line 9: EnvironmentFile=-/etc/sysconfig/vdsm
Line 10: ExecStartPre=@VDSMDIR@/init/run_task.sh pre-start "run_init_hooks gencerts libvirt_reconfigure syslog_available nwfilter dummybr load_needed_modules tune_system mkdirs test_space test_lo test_conflicting_conf"
Maybe you need to put one statement for common and another for specific, but still I would like to know why there is a difference, there should be none.
Line 11: ExecStart=@PYTHON@ @VDSMDIR@/init/daemonAdapter -u vdsm -n
Line 12: ExecStopPost=@VDSMDIR@/init/run_task.sh post-stop "run_final_hooks"
Line 13: KillMode=process
Line 14: Restart=always


Line 9: EnvironmentFile=-/etc/sysconfig/vdsm
Line 10: ExecStartPre=@VDSMDIR@/init/run_task.sh pre-start "run_init_hooks gencerts libvirt_reconfigure syslog_available nwfilter dummybr load_needed_modules tune_system mkdirs test_space test_lo test_conflicting_conf"
Line 11: ExecStart=@PYTHON@ @VDSMDIR@/init/daemonAdapter -u vdsm -n
Line 12: ExecStopPost=@VDSMDIR@/init/run_task.sh post-stop "run_final_hooks"
Line 13: KillMode=process
Is there real reason to leave running processes within the process tree on termination?

If no, please use systemd/upstart service to terminate the complete process group to avoid leftovers.

The fact that it was not implemented in sysvinit was probably because it was complex...
Line 14: Restart=always
Line 15: 
Line 16: [Install]


Line 10: ExecStartPre=@VDSMDIR@/init/run_task.sh pre-start "run_init_hooks gencerts libvirt_reconfigure syslog_available nwfilter dummybr load_needed_modules tune_system mkdirs test_space test_lo test_conflicting_conf"
Line 11: ExecStart=@PYTHON@ @VDSMDIR@/init/daemonAdapter -u vdsm -n
Line 12: ExecStopPost=@VDSMDIR@/init/run_task.sh post-stop "run_final_hooks"
Line 13: KillMode=process
Line 14: Restart=always
This is good, I did not notice that because you used implicit parameters.
Line 15: 
Line 16: [Install]


Line 11: ExecStart=@PYTHON@ @VDSMDIR@/init/daemonAdapter -u vdsm -n
Line 12: ExecStopPost=@VDSMDIR@/init/run_task.sh post-stop "run_final_hooks"
Line 13: KillMode=process
Line 14: Restart=always
Line 15: 
set nice?

set user?
Line 16: [Install]


-- 
To view, visit http://gerrit.ovirt.org/15578
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id8e514df1ca88500f746242134ddb24c31588046
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list