New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Systemd socket activation #1458

Merged
merged 4 commits into from Jul 25, 2016

Conversation

4 participants
@xaiki
Contributor

xaiki commented Jul 21, 2016

This tiny patch implements systemd's socket activation mechanism[0] for
cherrypy servers, it's based on work sponsored by Endless Computers[1] to
package kalite[2] as a flatpak[3].

Socket Activation allows to setup a system so that systemd will sit on a
port and start services 'on demand' (a little bit like inetd and xinetd
used to do). This patch makes that mechanism work for any cherrypy
service without modification.

[0] http://0pointer.de/blog/projects/socket-activation.html
[1] https://endlessm.com
[2] https://learningequality.org/ka-lite/
[3] https://flatpak.org

https://phabricator.endlessm.com/T4489

Signed-off-by: Niv Sardi xaiki@evilgiggle.com

@@ -1939,7 +1939,11 @@ def start(self):
self.software = "%s Server" % self.version
# Select the appropriate socket
if isinstance(self.bind_addr, six.string_types):
self.socket = None
if os.environ.get('LISTEN_PID', None):

This comment has been minimized.

@webknjaz

webknjaz Jul 22, 2016

Member

Why set default value to None? It's not necessary.
Moreover, os.getenv('LISTEN_PID') would be more verbose.

This comment has been minimized.

@xaiki

xaiki Jul 22, 2016

Contributor

fixed

self.socket.close()
self.socket = None
continue
break

This comment has been minimized.

@webknjaz

webknjaz Jul 22, 2016

Member

I'd move break to line 1986, just next to self.bind() and remove continue.

This comment has been minimized.

@xaiki

xaiki Jul 22, 2016

Contributor

fixed

af, socktype, proto, canonname, sa = res
try:
self.bind(af, socktype, proto)
except socket.error, serr:

This comment has been minimized.

@webknjaz

webknjaz Jul 22, 2016

Member

This outdated exception handling syntax breaks TravisCI tests. Plz fix

This comment has been minimized.

@xaiki

xaiki Jul 22, 2016

Contributor

fixed

@xaiki xaiki force-pushed the xaiki:master branch from e5e0a3c to 848cbd7 Jul 22, 2016

@xaiki

This comment has been minimized.

Contributor

xaiki commented Jul 22, 2016

@webknjaz thanks for the review, i fixed your comments and pushed !

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 22, 2016

LGTM, but there's some issue w/ TravisCI. Could you plz try restarting builds?
I think we can now ping @jaraco and see if he would merge this.

@jaraco

This comment has been minimized.

Member

jaraco commented Jul 22, 2016

I tried restarting the builds, but the result is the same. Why would the tests not run on Travis with this patch?

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 22, 2016

@jaraco I think, there's some issue with TravisCI itself. Look at the pip install output. It differs for different jobs. Did you mention this?
I have a strong feeling that it gets cut/halted at some point.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 22, 2016

@jaraco I've found the same issue, which happened in the past: https://travis-ci.org/cherrypy/cherrypy/jobs/139925725

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 22, 2016

Okay.. I've tried running nosetests -s -v myself and I also get return code 70. So the issue is not in Travis

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 22, 2016

$ .tox/py27/bin/nosetests -s -v
nose.config: INFO: Set working dir to ~/src/cherrypy-xaiki/cherrypy
nose.config: INFO: Working directory ~/src/cherrypy-xaiki/cherrypy is a package; adding to sys.path
nose.config: INFO: Ignoring files matching ['^\\.', '^_', '^setup\\.py$']
nose.selector: INFO: ~/src/cherrypy-xaiki/cherrypy/cherryd is executable; skipped
nose.selector: INFO: ~/src/cherrypy-xaiki/cherrypy/daemon.py is executable; skipped
nose.selector: INFO: ~/src/cherrypy-xaiki/cherrypy/test/sessiondemo.py is executable; skipped
nose.selector: INFO: ~/src/cherrypy-xaiki/cherrypy/test/test_session.py is executable; skipped
$ echo $?
70
@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 22, 2016

It looks there's something malformed in this patch. Even if I generate diff and apply it to working master it breaks nose command.

Systemd socket activation
This tiny patch implements systemd's socket activation mechanism[0] for
cherrypy servers, it's based on work sponsored by Endless Computers[1] to
package kalite[2] as a flatpak[3].

Socket Activation allows to setup a system so that systemd will sit on a
port and start services 'on demand' (a little bit like inetd and xinetd
used to do). This patch makes that mechanism work for any cherrypy
service without modification.

[0] http://0pointer.de/blog/projects/socket-activation.html
[1] https://endlessm.com
[2] https://learningequality.org/ka-lite/
[3] https://flatpak.org

https://phabricator.endlessm.com/T4489

Signed-off-by: Niv Sardi <xaiki@evilgiggle.com>

@xaiki xaiki force-pushed the xaiki:master branch from 848cbd7 to 1f5960b Jul 22, 2016

@xaiki

This comment has been minimized.

Contributor

xaiki commented Jul 22, 2016

@webknjaz it was the removal of self.socket = None i added it back and it now unbreaks nose, i have no idea why ....

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 22, 2016

@xaiki okay, seems good

Could you plz include some documentation/examples of running cherrypy app with use of this feature?

Also it would be nice to have some tests covering it.

@xaiki

This comment has been minimized.

Contributor

xaiki commented Jul 22, 2016

a test is a bit hard as you need to have systemd setup,

we (endlessm.com) use that patch to enable kalite systemd activation, here's our system part:
https://github.com/mariospr/eos-kalite-system-helper

basically, you need to setup these files:
https://github.com/mariospr/eos-kalite-system-helper/blob/master/debian/eos-kalite-system-helper.service
https://github.com/mariospr/eos-kalite-system-helper/blob/master/debian/eos-kalite-system-helper.socket

to make it work

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 25, 2016

@xaiki Could you plz describe this in docs/deploy.rst?
Ability of others to use this feature seamlessly will make this PR more valuable.
Also add some information about the feature to the docstring of start() method.

add docs to deploy.rst
Signed-off-by: Niv Sardi <xaiki@evilgiggle.com>
@xaiki

This comment has been minimized.

Contributor

xaiki commented Jul 25, 2016

docs added to deploy.rst
for start you mean start of the wcgiserver ? or the dameon start ? i'm not that familiar with those uses =)

document socket activation in bind_addr's docstring
Signed-off-by: Niv Sardi <xaiki@evilgiggle.com>
@xaiki

This comment has been minimized.

Contributor

xaiki commented Jul 25, 2016

added to the bind_addr docstring as it seemed more relevant.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 25, 2016

LGTM

@jaraco I thinks this PR is ready for merge

Only wait for the port if not running via socket-activation
Otherwise this will always end up killing the server in the socket-activated
scenario, as the port won't ever be occupied by cherrypy, since the socket
is handled by systemd.

Signed-off-by: Niv Sardi <xaiki@evilgiggle.com>
@xaiki

This comment has been minimized.

Contributor

xaiki commented Jul 25, 2016

good, i had missed a patch from @mariospr just got it in the PR. thanks for a great review @webknjaz

@jaraco

This comment has been minimized.

Member

jaraco commented Jul 25, 2016

Excellent. Thanks everyone for putting this together.

@jaraco jaraco merged commit 420a398 into cherrypy:master Jul 25, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jaraco

This comment has been minimized.

Member

jaraco commented Jul 25, 2016

Pending release in 7.1.0.

@mariospr

This comment has been minimized.

Contributor

mariospr commented Jul 25, 2016

@xaiki, @jaraco: that was fast! Thanks for accepting my patch :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment