Alon Bar-Lev has posted comments on this change.
Change subject: vdsmd.init: Add init adapter
......................................................................
Patch Set 1: (25 inline comments)
....................................................
File vdsm/init/daemonAdapter
Line 1: #!/usr/bin/python
why 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 31: def getArgs():
Line 32: parser = argparse.ArgumentParser(description='Start VDSM daemon
with'
Line 33: 'various options')
Line 34: parser.add_argument('-u', '--user', dest='user', default=None,
Line 35: metavar='user_name', help='Run as user_name')
no need to support that... you always run as vdsm user.
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')
Line 39: parser.add_argument('-r', '--respawn-pid-file',
dest='respawnPidFile',
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')
no need to support that, you can read the nice value out of configuration.
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')
pidfile?
Line 43: args = parser.parse_args(sys.argv[1:])
Line 44: return args
Line 45:
Line 46: if __name__ == '__main__':
Line 68: '--daemon', '--masterpid', args.respawnPidFile]
Line 69:
Line 70: cmd.append(os.path.join(P_VDSM, 'vdsm'))
Line 71:
Line 72: env = os.environ
env = os.environ.copy()
Line 73: env.update({
Line 74: 'LIBVIRT_LOG_FILTERS': config.get('vars',
Line 75:
'libvirt_env_variable_log_filters'),
Line 76: 'LIBVIRT_LOG_OUTPUTS': config.get('vars',
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)
where do we go into background or not?
I expect --background parameter to be set if sysvinit and for the process to
fork and return.
if you write in python, consider using python-daemon, sample[1]
[1]
http://gerrit.ovirt.org/gitweb?p=ovirt-engine.git;a=blob;f=packaging/services/service.py;hb=HEAD
....................................................
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
why don't you use the standard option parser of python?
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/vdsmd.init.in
Line 20
Line 21
Line 22
Line 23
Line 24
quotes
Line 22
Line 23
Line 24
Line 25
Line 26
quotes
Line 32
Line 33
Line 34
Line 35
Line 36
I don't think we need these... use downstream failure and success.
Line 35
Line 36
Line 37
Line 38
Line 39
not sure why this is needed... you can use $0 status
Line 37
Line 38
Line 39
Line 40
Line 41
quotes
Line 50
Line 51
Line 52
Line 53
Line 54
not good to have the list of tasks here, why do we need it?
Line 55
Line 56
Line 57
Line 58
Line 59
why not?
if ! run_task.sh ...; then
failure "...."
return 1
fi
Line 62
Line 63
Line 64
Line 65
Line 66
this should be moved before the pre-start, no?
Line 65
Line 66
Line 67
Line 68
Line 69
this should be within the adapter before running the 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 adapter can set the nice level
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: }
Line 77
Line 78
Line 79
Line 80
Line 81
quotes
Line 89
Line 90
Line 91
Line 92
Line 93
quotes
Line 96
Line 97
Line 98
Line 99
Line 100
decide to tab or not to tab...
Line 128
Line 129
Line 130
Line 131
Line 132
is there no difference between restart and force-reload? why do we need it?
....................................................
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
/etc/sysconfig/vdsmd?
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"
not good to specify here modules... why you cannot just use pre-start?
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
are you sure you need the killmode?
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
can we give up running the @VDSMDIR@/respawn in this case?
Line 15:
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 <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
