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

Alon Bar-Lev alonbl at redhat.com
Thu Jun 13 08:30:41 UTC 2013


Alon Bar-Lev has posted comments on this change.

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


Patch Set 1: (6 inline comments)

Thanks!!!

Few more questions... sorry for me bothering you.

....................................................
File vdsm/init/daemonAdapter
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')
> I write daemon adapter in shell, it forks the vdsm process

I do not understand... we use the spawn with for sysvinit and this always forks, all the other cases has no fork, right?

It does not harm to have but it should be taken care of explicitly like preparing empty file at init script, chown to the daemon user to allow the daemon to write into the file.

It is complex and needed only for the spawn at sysvinit, which is actually the pid we are looking for.

Maybe I miss something...
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:
Can't we move this configuration? Dan?

If not, can't we set -20 level at init script and then reset here to lower value? as far as I know this should work without root.

If not, I would have preferred to do this via svdsm after startup... but ignore that one.
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 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]
as far as I understand sysv is the only that uses respawn, and in this case we must have respawn to fork, this is what --daemon does.

what I suggest that you add this option to daemonize the respawn or not to the cmdline of the script so we have control over it, and it will be clear that the sysv init script expect the script to daemonize, as it currently does implicitly based on the existence of pid parameter.
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)
And you can call --console-redirect=/dev/null for now, until the other issue is resolved.

Thanks!
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)
Apart of my reply above to try to set nice=-20 at init then reset it here...

The core parameters of a service are expected to be at standard location... /etc/sysconfig/<service> at rpm based.

The problem with systemd is that it is quite primitive (in contrast of what people may think), it has the nature of environment, but these are not support this generically... the truly expect people to copy the unit to /etc and modify it. But this is how systemd is working...

I have another stupid idea... we can use the ExecStartPost to execute renice of the process provided we have pid... just an idea...


....................................................
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"
So the other way around... make systemd and upstart 'pure', and include these ones in the sysv... :)
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


-- 
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