Socket activation and systemd supervision #171

dkg · 2019-10-04T22:23:36Z

Please consider these changes to enable eaiser integration of aiosmtpd implementations with systemd and other comparable systems.

dkg · 2019-10-05T15:12:34Z

fwiw, i think the travis CI test is failing because the test wants to listen on port 8025 when something else (maybe another test) is already listening on it. i don't think that failure is due to any of the changes proposed here.

waynew · 2019-10-14T16:14:52Z

Hi @dkg, thanks for the contribution! Have you run these tests locally?

My experience is that there's no such thing as unrelated test failures. Either the tests are poorly written or the code has side effects that weren't accounted for. I'm thinking that might be the case here, as I haven't seen this error show up before... but I could be wrong!

dkg · 2019-10-23T01:01:58Z

@waynew -- you were right, somehow i'd mixed up my local tests. I've just pushed a fix (my changes to Controller.__init__ were accidentally ignoring the port argument) and now pytest-3 succeeds for me locally.

Looks like pytest-3 succeeds on Travis now too, but the coverage tests are failing, i think because of the changes i've made. I'm not sure how to increase the coverage effectively for these changes though -- can you help me figure that out?

waynew · 2019-10-23T16:24:02Z

aiosmtpd/main.py

-        loop.create_server(factory, host=args.host, port=args.port))
+    if args.supervised:
+        log.info('Server listening on socket-activated file descriptor')
+        stage = loop.create_server(factory, sock=socket.fromfd(3, socket.AF_INET, socket.SOCK_STREAM))


It looks like these are the lines that aren't actually getting tested, according to what I can glean from Travis.

https://travis-ci.com/aio-libs/aiosmtpd/jobs/248519180#L566

dkg · 2019-10-24T01:23:03Z

I've just tried to add tests to exercise that code, but i'm not sure whether they are what you want. please let me know if you'd rather they were done differently!

waynew · 2019-10-24T02:47:56Z

@dkg I think that fits with the existing pattern, so it seems reasonable to me.

Looks like there are a couple of more problems though:

https://travis-ci.com/aio-libs/aiosmtpd/jobs/248965906#L487
and
https://travis-ci.com/aio-libs/aiosmtpd/jobs/248965906#L466

dkg · 2019-10-24T14:07:11Z

@waynew -- i've fixed the flake complaints, but i don't understand the coverage complaint:

Name                        Stmts   Miss Branch BrPart  Cover   Missing
464-----------------------------------------------------------------------
465aiosmtpd/__init__.py            0      0      0      0   100%
466aiosmtpd/controller.py         56      0     12      2    97%   39->44, 42->44
467aiosmtpd/docs/__init__.py       0      0      0      0   100%
468aiosmtpd/handlers.py          143      0     50      0   100%
469aiosmtpd/lmtp.py               11      0      2      0   100%
470aiosmtpd/main.py               91      0     20      0   100%
471aiosmtpd/smtp.py              506      0    188      0   100%
472-----------------------------------------------------------------------
473TOTAL                         807      0    272      2    99%
474ERROR: InvocationError for command /home/travis/build/aio-libs/aiosmtpd/.tox/py37-cov/bin/python -m coverage report -m --rcfile=/home/travis/build/aio-libs/aiosmtpd/.coverage.ini --fail-under=100 (exited with code 2)

looking at the relevant lines in aiosmtpd/controller.py, i see:

    36	    def _run(self, ready_event):
    37	        asyncio.set_event_loop(self.loop)
    38	        try:
    39	            if self.sock is None:
    40	                if self.port is None:
    41	                    self.port = 8025
    42	                if self.hostname is None:
    43	                    self.hostname = '::1'
    44	            self.server = self.loop.run_until_complete(
    45	                self.loop.create_server(

i don't understand this for two reasons:

why is there an overlap in the indicated ranges?
why aren't these cases already covered by the tests in tests/test_server.py ?

dkg · 2019-10-24T19:35:36Z

OK, flake complaints are all actually fixed now, but i'm stumped on how to get this additional coverage, because it looks like it should already be covered to me. If you can help me understand the coverage test suite better, i'd be happy to take pointers.

dkg · 2019-12-09T19:33:55Z

@waynew, any hints ?

waynew · 2019-12-10T14:08:12Z

The overlap is because there are two branches there - neither is getting exercised.

Which tests should be exercising that code? I'm not seeing anything...

systemd-style socket-activation is a nice safe way to launch a process that is already unprivileged. It works by the parent process opening a socket, and then doing its own privilege dropping. This means the child process starts with an already-listening file descriptor, but no elevated privileges. To make use of this, the user needs to be able to pass an existing socket to the aiosmtpd controller. Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>

We only try to test --supervised on non-Windows platforms. This uses the strategy outlined in sd_listen_fds(), where file descriptor 3 is the first socket passed in. I chose an arbitrary different port (19666) to make sure that the right socket is getting through.

dkg · 2019-12-10T17:44:02Z

if the branch from 39-44 is not getting exercised at all, then surely none of the sub-branches within it are being exercised. so either i would expect no sub-branches to be listed, or both sub-branches to be listed. but instead, only the 42-44 sub-branch is listed, and the 40-41 sub-branch is not listed. This makes me doubt my understanding of the report :/

Anyway, the two cases identified are in Controller.run where:

39-44: self.sock is None
42-44: self.hostname is None

But aiosmtpd/tests/test_server.py contains test_socket_error, test_server_attribute, and test_smtp_utf8, each of which instantiates a Controller object with both hostname and sock left unset (thereby defaulting to None), which are then started or otherwise indirectly run, right?

Sorry if i'm asking obvious questions -- feel free to point me to answers even if they're things that you think i should already know.

waynew · 2019-12-10T19:47:07Z

Sorry if i'm asking obvious questions -- feel free to point me to answers even if they're things that you think i should already know.

You're doing fine 🙂

I'm not super familiar with those tests myself. What I would do is run the tests I'm expecting to hit that code, and perhaps throw assert self.sock is None. You should see that fail, given the coverage report.

CLAassistant · 2020-11-16T10:21:19Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

pepoluan · 2020-11-21T15:54:24Z

This is an old PR, more than 6 months old, only brought up recently due to CLAassistant.

Is there desire to continue with this PR? If not, I will close this.

pepoluan · 2020-12-01T04:54:57Z

Note to anyone wanting to implement similar functionality, either by picking up this PR or submitting a new one:

I'm -1 on changing Controller class. Especially considering the complexity added by #213 when it gets merged.

If systemd/supervised support is needed, IMHO we should create a new subclass of Controller, say, SupervisedController. In that class, _testconn() can (and should) be overridden to cater for the different method of activation when using supervised.

Adding a new class will also make merging much easier :-)

pepoluan · 2020-12-22T07:24:29Z

Okay, no activities for more than 3 weeks. I'm closing this PR for now.

If anyone wants to pick this up, feel free to reopen.

dkg force-pushed the socket-activation branch from d293dee to cae5c06 Compare October 23, 2019 00:57

waynew requested changes Oct 23, 2019

View reviewed changes

dkg force-pushed the socket-activation branch from 95c4744 to f2bc73e Compare October 24, 2019 14:01

dkg force-pushed the socket-activation branch 3 times, most recently from 9e4dedb to dfe3636 Compare October 24, 2019 16:08

dkg added 3 commits December 10, 2019 12:33

Prepare cli for systemd-style supervision

3d689b2

dkg force-pushed the socket-activation branch from dfe3636 to 7320bc4 Compare December 10, 2019 17:34

pepoluan mentioned this pull request Dec 2, 2020

Support for UNIX sockets in the Controller #114

Closed

pepoluan closed this Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socket activation and systemd supervision #171

Socket activation and systemd supervision #171

dkg commented Oct 4, 2019

dkg commented Oct 5, 2019

waynew commented Oct 14, 2019

dkg commented Oct 23, 2019 •

edited

Loading

waynew Oct 23, 2019

waynew Oct 23, 2019

dkg commented Oct 24, 2019

waynew commented Oct 24, 2019

dkg commented Oct 24, 2019 •

edited

Loading

dkg commented Oct 24, 2019

dkg commented Dec 9, 2019

waynew commented Dec 10, 2019

dkg commented Dec 10, 2019 •

edited

Loading

waynew commented Dec 10, 2019

CLAassistant commented Nov 16, 2020

pepoluan commented Nov 21, 2020

pepoluan commented Dec 1, 2020

pepoluan commented Dec 22, 2020

Socket activation and systemd supervision #171

Socket activation and systemd supervision #171

Conversation

dkg commented Oct 4, 2019

dkg commented Oct 5, 2019

waynew commented Oct 14, 2019

dkg commented Oct 23, 2019 • edited Loading

waynew Oct 23, 2019

Choose a reason for hiding this comment

waynew Oct 23, 2019

Choose a reason for hiding this comment

dkg commented Oct 24, 2019

waynew commented Oct 24, 2019

dkg commented Oct 24, 2019 • edited Loading

dkg commented Oct 24, 2019

dkg commented Dec 9, 2019

waynew commented Dec 10, 2019

dkg commented Dec 10, 2019 • edited Loading

waynew commented Dec 10, 2019

CLAassistant commented Nov 16, 2020

pepoluan commented Nov 21, 2020

pepoluan commented Dec 1, 2020

pepoluan commented Dec 22, 2020

dkg commented Oct 23, 2019 •

edited

Loading

dkg commented Oct 24, 2019 •

edited

Loading

dkg commented Dec 10, 2019 •

edited

Loading