Yaniv Bronhaim has posted comments on this change.
Change subject: [WIP]start vdsm as subprocess in supervdsm server ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(8 inline comments)
did you try to run it?
.................................................... File Makefile.am Line 64: PEP8_BLACKLIST = config.py,constants.py Line 65: Line 66: check-local: Line 67: find . -path './.git' -prune -type f -o \ Line 68: -name '*.py' ! 'vdsmServer.py' -o \ why?? Line 69: -name '*.py.in' | xargs $(PYFLAKES) Line 70: $(PEP8) --exclude="$(PEP8_BLACKLIST)" --filename '*.py,*.py.in' \ Line 71: $(PEP8_WHITELIST) Line 72: @if test -f .gitignore; then \
.................................................... File tests/Makefile.am Line 49 Line 50 Line 51 Line 52 Line 53 you can write in the comment that you remove the tests until we'll fix them, but i prefer that this series of patches will also include superVdsmTests .
.................................................... File vdsm.spec.in Line 635: %{_datadir}/%{vdsm_name}/supervdsm.py* Line 636: %{_datadir}/%{vdsm_name}/supervdsmServer.py* Line 637: %{_datadir}/%{vdsm_name}/vmChannels.py* Line 638: %{_datadir}/%{vdsm_name}/tc.py* Line 639: %{_datadir}/%{vdsm_name}/vdsmServer.py* don't you need to make supervdsmServer.py python file executable? Line 640: %{_datadir}/%{vdsm_name}/vdsm-restore-net-config Line 641: %{_datadir}/%{vdsm_name}/vdsm-store-net-config Line 642: %{_datadir}/%{vdsm_name}/vm.py* Line 643: %{_datadir}/%{vdsm_name}/write-net-config
.................................................... File vdsm/supervdsmServer.py Line 423: sys.exit(1) Line 424: Line 425: if __name__ == '__main__': Line 426: vdsmProc = Process(target=startVdsm) Line 427: vdsmProc.start() import startVdsm is missing..
what if startVdsm gets to log creation after supervdsmServer creates the file as root?
maybe log initialization should be here with root permissions if not exist with chown to vdsm user, if exist we can move forward Line 428: sleep(2)
.................................................... File vdsm/vdsmd.init.in Line 22: ### END INIT INFO Line 23: Line 24: . @LIBEXECDIR@/ovirt_functions.sh Line 25: Line 26: SVDSM_BIN=@VDSMDIR@/supervdsmServer you didn't remove the py suffix... Line 27: CONF_FILE=@CONFDIR@/vdsm.conf Line 28: GETCONFITEM=@VDSMDIR@/get-conf-item Line 29: prog=vdsm Line 30: PIDFILE=@VDSMRUNDIR@/vdsmd.pid
.................................................... File vdsm/vdsmServer.py Line 1: #!/usr/bin/python #!/usr/bin/python is redundant if you want to omit the option to run this file and only use it as package. Line 2: # Line 3: # Copyright 2009 Red Hat, Inc. and/or its affiliates. Line 4: # Line 5: # Licensed to you under the GNU General Public License as published by
Line 27: pthreading.monkey_patch() Line 28: Line 29: loggerConfFile = constants.P_VDSM_CONF + 'logger.conf' Line 30: Line 31: I guess you wont use this function as well if you remove parse_args Line 32: def usage(): Line 33: print "Usage: vdsm [OPTIONS]" Line 34: print " -h - Display this help message" Line 35:
Line 120: os.setgroups([]) Line 121: os.setgid(vdsm_gid) Line 122: os.setuid(vdsm_uid) Line 123: dropPrivileges() Line 124: __assertLogPermission() why did you omit os.setpgrp(). don't we need it?
-- To view, visit http://gerrit.ovirt.org/11191 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I540b1d3f3c823433f100f4803f31322fc7ee2153 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server