Zhou Zheng Sheng has posted comments on this change.

Change subject: vdsmd and supervdsmd: extract common init tasks and add init 
adapter
......................................................................


Patch Set 13:

(10 comments)

....................................................
File vdsm.spec.in
Line 563: 
Line 564: %if 0%{?with_systemd}
Line 565: # Install the systemd scripts
Line 566: # vdsm systemd unit does not rely on systemd-vdsmd any more, but we 
keep it to
Line 567: # retain backward compatibility with old ovirt-host-deploy.
New version of ovirt-host-deploy can call vdsm-tool for libvirt configuring, 
but the old versions need this file.
Line 568: install -Dm 0755 vdsm/vdsmd.init 
%{buildroot}/lib/systemd/systemd-vdsmd
Line 569: install -Dm 0644 vdsm/vdsmd.service 
%{buildroot}%{_unitdir}/vdsmd.service
Line 570: install -Dm 0644 vdsm/supervdsmd.service 
%{buildroot}%{_unitdir}/supervdsmd.service
Line 571: 


....................................................
File vdsm/daemonAdapter
Line 19: #
Line 20: 
Line 21: import argparse
Line 22: import os
Line 23: import os.path
Done
Line 24: import sys
Line 25: 
Line 26: from vdsm.config import config
Line 27: 


Line 58:     })
Line 59: 
Line 60:     cmd = [args.target[0]] + args.targetOptions
Line 61: 
Line 62:     if args.redirect:
Done
Line 63:         # Redirction should be in last minute, otherwise it would 
swallow
Line 64:         # error messages.
Line 65:         os.close(0)
Line 66:         os.open(args.redirect, os.O_RDONLY)


Line 59: 
Line 60:     cmd = [args.target[0]] + args.targetOptions
Line 61: 
Line 62:     if args.redirect:
Line 63:         # Redirction should be in last minute, otherwise it would 
swallow
Done
Line 64:         # error messages.
Line 65:         os.close(0)
Line 66:         os.open(args.redirect, os.O_RDONLY)
Line 67:         os.close(1)


Line 62:     if args.redirect:
Line 63:         # Redirction should be in last minute, otherwise it would 
swallow
Line 64:         # error messages.
Line 65:         os.close(0)
Line 66:         os.open(args.redirect, os.O_RDONLY)
Done
Line 67:         os.close(1)
Line 68:         os.open(args.redirect, os.O_WRONLY)
Line 69:         os.close(2)
Line 70:         os.open(args.redirect, os.O_WRONLY)


....................................................
File vdsm/supervdsmServer.py
Line 24: import errno
Line 25: from functools import wraps
Line 26: import threading
Line 27: import re
Line 28: import getopt
Because in vdsm main script, it uses getopt, so I follow the similar pattern...
Line 29: import signal
Line 30: import logging
Line 31: import logging.config
Line 32: 


Line 413:         sys.exit(1)
Line 414: 
Line 415: 
Line 416: def _usage():
Line 417:     print "Usage:  supervdsmServer --sockfile=fullPath 
[--pidfile=fullPath]"
Thanks Yaniv, I've submitted a similar patch based on this.

http://gerrit.ovirt.org/#/c/17100/
"packaging: rename supervdsmServer.py to supervdsmServer and add environment 
file"

Could you have a look as well?
Line 418: 
Line 419: 
Line 420: def _parse_args():
Line 421:     argDict = {}


Line 427:         if o == "--pidfile":
Line 428:             argDict['pidfile'] = v
Line 429:         if o == "-h":
Line 430:             _usage()
Line 431:             sys.exit(0)
Done
Line 432:     if 'sockfile' not in argDict:
Line 433:         _usage()
Line 434:         sys.exit(1)
Line 435: 


....................................................
File vdsm/vdsmd.init.in
Line 192:     ;;
Line 193:     reconfigure)
Line 194:         # Jump over 'reconfigure'
Line 195:         shift 1
Line 196:         reconfigure_sanlock
Thanks for reminding me this. I didn't notice the problem.
Line 197:         reconfigure_libvirt "$@"
Line 198:     RETVAL=$?
Line 199:     ;;
Line 200:      *)


....................................................
File vdsm/vdsmd.service.in
Line 7: [Service]
Line 8: Type=simple
Line 9: EnvironmentFile=-/etc/sysconfig/vdsm
Line 10: ExecStartPre=@LIBEXECDIR@/vdsmd_init_common.sh --pre-start
Line 11: ExecStart=@VDSMDIR@/daemonAdapter -c /dev/null "@VDSMDIR@/vdsm"
Thanks Antoni. Do you mean the output of VDSM stdout and stderr will go to 
system journal and by redirect VDSM output to /dev/null, we will miss some 
output if VDSM crashes?

After using systemd to start VDSM directly, I find the system journal contains 
some verbose output from VDSM even it runs OK. I think those output should be 
in vdsm log, otherwise it fill system journal with verbose information.

In the original way of starting VDSM, the main process is started by respawn 
with "--daemon" option. This option means respawn redirects VDSM's stdin, out 
and err to /dev/null.  Then in this patch I think it's fine to do the same. It 
avoids verbose information in system journal, and it does not give less 
information than before. The console redirection is put as the last step in 
daemonAdapter to give as much as possible information before the shot.
Line 12: ExecStopPost=@LIBEXECDIR@/vdsmd_init_common.sh --post-stop
Line 13: Restart=always
Line 14: Nice=-20
Line 15: User=@VDSMUSER@


-- 
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: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Mei Liu <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to