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

zhshzhou at linux.vnet.ibm.com zhshzhou at linux.vnet.ibm.com
Thu Jun 13 06:19:05 UTC 2013


Zhou Zheng Sheng has posted comments on this change.

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


Patch Set 1: (19 inline comments)

....................................................
File vdsm/init/daemonAdapter
Line 1: #!/usr/bin/python
I'm OK with the hybrid solution, because I like working step by step. For now, the initialization tasks are in shell scripts, so it's good to have run_task.sh in shell as well. For daemon adapter, I think Python is better and cleaner. Python code is the most obvious method to implement daemon adapter at the time I was writhing the patch (specifically on argument parsing). Having shell tasks are because of some historic reason, and in future we can improve the tasks by re-writing them in Python. However, I'm fine to use shell for daemon adapter as well.
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 34:     parser.add_argument('-u', '--user', dest='user', default=None,
Line 35:                         metavar='user_name', help='Run as user_name')
Line 36:     parser.add_argument('-n', '--renice', dest='renice', default=False,
Line 37:                         action='store_true',
Line 38:                         help='Ajust nice according to configuration file')
Yes, but the nice value may be set outside daemon adapter by the init system, so I think it's OK to make this optional.
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')


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')
Yes. In first case, the vdsm is started directly by systemd or upstart, so the pid file is useless, but if I write daemon adapter in shell, it forks the vdsm process, then systemd would need the pid file as a hint to manage vdsm process. There is a pidfile field available in system service spec file to handle this case.

I think though the pid file sometimes is useless, it does not harm to have them.
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:
Yes. Downstream init systems knows how to change identity. I keep this in daemon adapter for some reason. The nice level can be configured in vdsm.conf, so I can not write a fixed value in vdsmd systemd service file or vdsmd upstart job, daemon adapter has to read the nice level from vdsm.conf then renice itself, and the renice needs root privilege, so daemon adapter later has to drop the privilege and change the identity itself.
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({
Yes.
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 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 downstream init systems already daemonize the process that this daemon adapter runs, we do not need to daemonize ourself again. So the --daemon here does not means we daemonize the vdsm, it's just an option for "respawn" to redirect std out and err to /dev/null when it runs vdsm. The name is not good though.
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)
Yes, totally agree. However I get a lot of vdsm debug messages in /var/log/messages if I does not disable std out and err, and I think these messages should be put in vdsm.log. Before this patch, no one notices these garbage messages because respawn closes std out and err for vdsm. I think a future patch is needed to guid those messages to vdsm.log. I'll take your advice and add a --console-redirect=file option.
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)
I agree that the best way to set nice level in systemd is "Nice=", but I want to here a third person's opinion on this. Because having the user modify nice level in service file manually is not very "nice"... If it works this way, we should retire the "vdsm_nice" setting in vdsm.conf, otherwise it's a bug to edit "vdsm_nice" in vdsm.conf but does not effective. In some cases, if we have many vdsm hosts, some are sysv, some are systemd and some are upstart, when we want to update the nice level for all of them, the vdsm.conf is a better place. This problem is a bit troublesome to me, so I just implement the --renice and --user and see if someone have a better idea.


....................................................
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
In Python 2.7 official documentation it says optparse is marked deprecated since version 2.7, future development is in argparse module. Is there any reason we are trying to avoid dependencies? For now I see in other files we use getopt to parse arguments. I think it's OK to add this dependency, because argparse is in Python 2.7 standard library. argparse does better than getopt in terms of "write less code and get better help and error messages". I'm OK to use getopt if we don't like this dependency.

[1] http://docs.python.org/2.7/library/getopt.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
My mistake, this file should not be modified. I was testing alternative ways to implement daemon adapter, this line was for an abandoned solution. I forgot to remove it, my bad.


....................................................
File vdsm/vdsm
Line 145: 
Line 146: if __name__ == '__main__':
Line 147:     __assertVdsmUser()
Line 148:     __assertLogPermission()
Line 149:     try:
If setpgrp is moved to respawn, respawn should call setpgid(vdsmpid, vdsmpid), I don't see obvious way to implement this in shell...

As you said, it's better to check instead of fail and ignore, I'll update it.
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
Agree. Should be improved in future patches.


Line 55
Line 56
Line 57
Line 58
Line 59
Yes, thanks.


Line 62
Line 63
Line 64
Line 65
Line 66
Yes.


Line 96
Line 97
Line 98
Line 99
Line 100
Agree! A separate patch should fix this.


....................................................
File vdsm/vdsmd.service.in
Line 5: Conflicts=libvirt-guests.service ksmtuned.service
Line 6: 
Line 7: [Service]
Line 8: Type=simple
Line 9: EnvironmentFile=-/etc/sysconfig/vdsm
Yes.
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 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"
Thanks. I think it can be run_task.sh pre-start --exclude "abc def". SystemD does not need to run start_need_srv and shutdown_conflicting_srv.
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
OK.
Line 14: Restart=always
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: 
SystemD is nice because there is a field available to run pre-start and post-stop tasks as root and run ExecStart as vdsm, but if the daemon adapter want's to renice itself according to the vdsm_nice in vdsm.conf, it needs root..
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