Juan Hernandez has posted comments on this change.
Change subject: packaging: engine-service: downstream infrastructure usage
......................................................................
Patch Set 4: (10 inline comments)
....................................................
File backend/manager/conf/engine.conf.defaults.in
Line 29
Line 30
Line 31
Line 32
Line 33
I miss the engine-upgrade code to move the changes that users potentially made
here to the new place where this setting should go.
....................................................
File packaging/fedora/engine-service.py.in
Line 589: stderr=engineConsoleLog,
Line 590: ):
Line 591: saveEnginePid(os.getpid())
Line 592:
Line 593: p = subprocess.Popen(
You could use a name better than "p".
Line 594: args=engineArgs,
Line 595: executable=javaLauncher,
Line 596: env=engineEnv,
Line 597: close_fds=True,
Line 599:
Line 600: stopTime = engineConfig.getInteger("ENGINE_STOP_TIME")
Line 601: stopInterval = engineConfig.getInteger("ENGINE_STOP_INTERVAL")
Line 602:
Line 603: terminated = [False]
You could explain why you need a list and not a simple variable.
Line 604: def myterm(signum, frame):
Line 605:
Line 606: # avoid recursive signals
Line 607: for sig in (signal.SIGTERM, signal.SIGINT):
Line 600: stopTime = engineConfig.getInteger("ENGINE_STOP_TIME")
Line 601: stopInterval = engineConfig.getInteger("ENGINE_STOP_INTERVAL")
Line 602:
Line 603: terminated = [False]
Line 604: def myterm(signum, frame):
You could use a name better than "myterm".
Line 605:
Line 606: # avoid recursive signals
Line 607: for sig in (signal.SIGTERM, signal.SIGINT):
Line 608: signal.signal(sig, signal.SIG_IGN)
Line 603: terminated = [False]
Line 604: def myterm(signum, frame):
Line 605:
Line 606: # avoid recursive signals
Line 607: for sig in (signal.SIGTERM, signal.SIGINT):
No need for these ^ parenthesis.
Line 608: signal.signal(sig, signal.SIG_IGN)
Line 609:
Line 610: p.terminate()
Line 611: for i in range(stopTime // stopInterval):
Line 607: for sig in (signal.SIGTERM, signal.SIGINT):
Line 608: signal.signal(sig, signal.SIG_IGN)
Line 609:
Line 610: p.terminate()
Line 611: for i in range(stopTime // stopInterval):
I think that this lengthy loop should go outside of the signal handler.
Line 612: if p.poll() is not None:
Line 613: terminated[0] = True
Line 614: break
Line 615: time.sleep(1)
....................................................
File packaging/fedora/engine-service.sysv.in
Line 6: # description: oVirt Engine
Line 7: # pidfile: /var/run/ovirt-engine.pid
Line 8:
Line 9: ### BEGIN INIT INFO
Line 10: # Provides: ovirt-enine
s/enine/engine/
Line 11: # Short-Description: oVirt Engine
Line 12: ### END INIT INFO
Line 13:
Line 14: # Source function library.
Line 29: echo $"Insufficient privilege" 1>&2
Line 30: exit 4
Line 31: fi
Line 32: echo -n $"Starting $PROG: "
Line 33: ulimit -n ${FILENO:-65535}
This ^ should go in a file in /etc/security/limits.d.
Line 34: touch "${PIDFILE}"
Line 35: chown "${USER}" "${PIDFILE}"
Line 36: daemon --user "${USER}" --pidfile="${PIDFILE}"
/usr/bin/engine-service --pidfile="${PIDFILE}" --quiet start
Line 37: RETVAL=$?
Line 64: echo $"Usage: $0 {start|stop|status|restart}"
Line 65: exit 2
Line 66: esac
Line 67:
Line 68: exit $RETVAL
Use spaces instead of tabs.
....................................................
File packaging/fedora/spec/ovirt-engine.spec.in
Line 282:
Line 283: install -dm 755 %{buildroot}/%{engine_state}/deployments
Line 284: install -dm 755 %{buildroot}/%{engine_state}/content
Line 285: install -dm 755
%{buildroot}/%{engine_log}/{host-deploy,notifier,engine-manage-domains}
Line 286: install -dm 755 %{buildroot}/%{engine_cache}
Why do we need to include this directory in the package? And why in this patch?
Line 287: install -dm 755 %{buildroot}/%{engine_run}/notifier
Line 288:
Line 289: # Needed for compatibility if package is different than the directory
structure
Line 290: %if "%{name}" != "%{engine_name}"
--
To view, visit http://gerrit.ovirt.org/13488
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie02f0d44bbba7d1a7af6bcf6b808610e85b907c6
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
