Merge lp:~jameinel/launchpad/lp-service into lp:launchpad

small update, I removed the "connect_to_lpservice.py" script. It was old prototyping code, which isn't relevant anymore.

That's a great improvement in time.

I think (per your decision 1) it would be good to change it to a unix-domain socket, for the sake of a bit more security, and better reusability as a per-user thing.

Per http://www.pathname.com/fhs/pub/fhs-2.3.html#VARRUNRUNTIMEVARIABLEDATA

> System programs that maintain transient UNIX-domain sockets must place them in this directory.

which will require sysadmins making a directory under there that's writable by this process. That can be in the config file. You should just need socket.bind((AF_UNIX, path)). Then just chmod the socket.

Having a config option to turn this on is great. We could make it active only on staging at first and see the effect.

481 + while not self._should_terminate.isSet():
482 + try:
483 + conn, client_addr = self._server_socket.accept()
484 + except self._socket_timeout:
485 + pass # run shutdown and children checks
486 + except self._socket_error, e:
487 + if e.args[0] == errno.EINTR:
488 + pass # run shutdown and children checks
489 + elif e.args[0] != errno.EBADF:
490 + # We can get EBADF here while we are shutting down
491 + # So we just ignore it for now
492 + pass

It seems like this might spin if the server socket's closed and there are no dead children.

+ fifos on the filesystem, and start running the requseted command. The

typo

We should do some kind of plan or review with LOSAs on how to deploy and support this. We should configure it to log to a file, and for that to be synced to devpad. Putting the pid in the log file would be nice.

First off, my apologies for accidentally including some db changes in this patch.

I went for a default socket location of '/var/run/launchpad_forking_service.sock' and an lp configured '/var/tmp/launchpad_forking_service.sock', since that matched a lot of the other stuff in schema-lazr.conf. I assume we can set it wherever we want in the end.

...

> It seems like this might spin if the server socket's closed and there are no dead children.

I'm not sure how you would get the server socket closed inside this process. Regardless, children shouldn't affect whether it spins or not. We just need a way to set '_should_terminate', which can be done via SIGINT or SIGTERM.

So, yes, if self._server_socket.accept() always raises an error but the should terminate event is not set, we will spin.

I suppose I could put a "self._should_terminate.set()" inside the "EBADF" check. Would that be better for you?

I do put PID into the log file, but bzr does not for each mutter() request. So when spawning I indicate that PID started, etc. But each individual message doesn't. mutter() does its own formatting, so it is a matter of changing these lines:
=== modified file 'bzrlib/trace.py'
--- bzrlib/trace.py 2010-06-28 02:41:22 +0000
+++ bzrlib/trace.py 2010-09-22 20:15:56 +0000
@@ -183,7 +183,7 @@
     else:
         out = fmt
     now = time.time()
- timestamp = '%0.3f ' % (now - _bzr_log_start_time,)
+ timestamp = '%0.3f [%d] ' % (now - _bzr_log_start_time, os.getpid())
     out = timestamp + out + '\n'
     _trace_file.write(out)
     # there's no explicit flushing; the file is typically line buffered.

There are many unrelated changes in this diff, which is part of why the line
count is so enormous. I guess the target branch isn't the one you branched
from? It does make the review harder than it should be, and certainly much
more intimidating at a glance than it deserves, so it'd be worth fixing :)

SIGCHLD: is this handler uninstalled in the child? If not, there's a risk of
EINTR issues if the child spawns its own subprocesses. Ah, yes, it is. Good :)

> + # [Decision #4]
> + # Exit information
> + # How do we inform the client process that the child has exited?

This section seems to describe the questions without clearly stating what the
answer was. I have to read through the wall of text fairly carefully to see
what the choice was.

> + # There is some possibility that files won't get flushed, etc. So we
> + # may want to be calling sys.exitfunc() first. Note that bzr itself

Yes, I think it would be good to call sys.exitfunc() for the reasons you give.
I'd be particularly worried about incompletely written log files... it would be
a shame to have the most interesting log content never get flushed! (The end of
the log is often the most interesting bit.)

> + # [Decision #8]
> + # env vars

This section doesn't make clear what the issue is, nor what the conclusion is.
What does “this data” refer to? Are env vars the problem, or solution to some
problem?

> + # [Decision #9]
> + # nicer errors to clients
> + # This service is meant to be run only on the local system. As such,
> + # we don't try to be extra defensive about leaking information to
> + # clients. Instead we try to be helpful, and tell them as much as we
> + # know about what went wrong.

The client is connected to a remote user though. So the information returned
here won't be repeated back to that remote client?

I'm worried about this becase we already echo more information back to the
remote client than really makes sense, e.g. random warnings on stderr are sent
to the remote client, and tend to confuse users.

> + _fork_function = os.fork

I assume this is to make unit testing easier, which is fine. Peeking ahead in
the diff I see that you test this at least in part by subclassing, which isn't
so great: I feel it violates the “Use the front door first” guideline for tests,
because this isn't a class intended for subclassing in production code, so the
tests are assuming a more intimate (and not sharply defined) interface with this
code than is ideal. I'm not sure it's worth spending effort trying to find a
better way, but hopefully you finding this interesting food for thought.

> + # XXX: Cheap hack. by this point bzrlib has opened stderr for logging
> + # (as part of starting the service process in the first place). As
> + # such, it has a stream handler that writes to stderr. logging
> + # tries to flush and close that, but the file is already closed.
> + # This just supresses that exception
> + logging.raiseExceptions = False

IIRC the Launchpad coding standard requires XXX comments to have a name and date
a...

>> === added file 'lp_service_interface.txt'
> I don't think this belongs in the root of the tree.

fwiw, my inclination is to add docstrings, not comments or text files, because they more easily show up in <http://people.canonical.com/~mwh/canonicalapi/>. I don't know what other lp devs would say.

<mwhudson> poolie: the branch has conflicts with db-devel it seems
<mwhudson> jam: can you merge db-devel, fix the conflicts and repropose it against db-devel?

I've done the changes, but I'm rejecting this in favor of an updated merge w/ db-devel and proposing against db-devel directly.

(note that I'm not authorized to reject my own submission, I can only delete it, but I don't want to lose the conversation)

John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Unfortunately this won't get put on the MP, because Launchpad now
> rejects all my emails. But I really wanted to reply to your comments inline.

Ok. I won't trim the quoted text for other people's benefit. Warm up those
scrolling fingers!

>
>
>
> ...
>
> >> + # [Decision #4]
> >> + # Exit information
> >> + # How do we inform the client process that the child has exited?
> >
> > This section seems to describe the questions without clearly stating what the
> > answer was. I have to read through the wall of text fairly carefully to see
> > what the choice was.
>
> New summary.
>
> >
> >> + # There is some possibility that files won't get flushed, etc. So we
> >> + # may want to be calling sys.exitfunc() first. Note that bzr itself
> >
> > Yes, I think it would be good to call sys.exitfunc() for the reasons you give.
> > I'd be particularly worried about incompletely written log files... it would be
> > a shame to have the most interesting log content never get flushed! (The end of
> > the log is often the most interesting bit.)
>
> I switched to this, though I'm not really sure it is the right thing.
> (When *do* buffered files get synced to disk? It seems that it might be
> during stack unwinding, which we explicitly avoid via os._exitfunc(),
> which bzr has been doing for a long time now...)

We call os._exitfunc from the outermost frame, so we don't avoid any stack
unwinding.

We do it to avoid wasting time that would be spent garbage collecting perhaps
100000s of objects when the OS can free that memory for us more cheaply.

>
>
> >
> >> + # [Decision #8]
> >> + # env vars
> >
> > This section doesn't make clear what the issue is, nor what the conclusion is.
> > What does “this data” refer to? Are env vars the problem, or solution to some
> > problem?
>
> Removed. Passing env vars from the Twisted process to the spawned child
> is a bit tricky. I did end up going with a syntax that can be parsed
> without ambiguity, though it is limited to 8-bit without encoding
> information. (AFAIK, so are real env vars)
>
> >
> >> + # [Decision #9]
> >> + # nicer errors to clients
> >> + # This service is meant to be run only on the local system. As such,
> >> + # we don't try to be extra defensive about leaking information to
> >> + # clients. Instead we try to be helpful, and tell them as much as we
> >> + # know about what went wrong.
> >
> > The client is connected to a remote user though. So the information returned
> > here won't be repeated back to that remote client?
>
> Wrong client. The 'client' here is twisted. I clarified it with:
> nicer errors on the request socket

Ok.

>
> >
> > I'm worried about this becase we already echo more information back to the
> > remote client than really makes sense, e.g. random warnings on stderr are sent
> > to the remote client, and tend to confuse users.
>
> That is true, though often the alternative would be to tell them
> nothing, which would have been worse. (Getting Permission Error can be
> confusing when i...

This is replaced by https://code.edge.launchpad.net/~jameinel/launchpad/lp-service/+merge/37531 so I'll close this mp.