Launchpad itself

Merge lp:~stub/launchpad/trivial into lp:launchpad

  1. trivial
  2. Merge into devel
Proposed by Stuart Bishop
Status: Merged
Merge reported by: Stuart Bishop
Merged at revision: not available
Proposed branch: lp:~stub/launchpad/trivial
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~stub/launchpad/trivial
Reviewer Review Type Date Requested Status
Christian Reis release-critical Pending
Canonical Launchpad Engineering Pending
Review via email: mp+9179@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

A startup script for the Librarian.

bin/run isn't suitable for use on production as it launches an appserver.

This whole area needs to be scorched earthed and redone. Bug #396209 is the first step of this.

Revision history for this message
Christian Reis (kiko) wrote :

On Thu, Jul 23, 2009 at 08:38:44AM -0000, Stuart Bishop wrote:
> A startup script for the Librarian.
> bin/run isn't suitable for use on production as it launches an appserver.

Could it not be made to run the librarian as well? I guess at this point
whatever's easiest to do and causes less problems is sanest.

> === modified file 'Makefile'
> --- Makefile 2009-07-19 02:34:24 +0000
> +++ Makefile 2009-07-23 06:57:17 +0000
> @@ -176,6 +176,12 @@
> stop_codebrowse:
> $(PY) sourcecode/launchpad-loggerhead/stop-loggerhead.py
>
> +start_librarian: build
> + bin/start_librarian
> +
> +stop_librarian:
> + bin/killservice librarian

Are these created by buildout automatically? I haven't seen any of this
code before, sorry.

> === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'

What else uses this script? /bin/run?

> @@ -84,7 +85,6 @@
> tacfile = make_abspath(self.tac_filename)
>
> args = [
> - sys.executable,
> TWISTD_SCRIPT,

Was this broken or just unnecessary? I can't see what this code does
from the context.

> - # We want to make sure that the Launchpad process will have the
> - # benefit # of all of the dependency paths inserted by the buildout
> - # bin/run script. We pass them via PYTHONPATH.
> - env = dict(os.environ)
> - env['PYTHONPATH'] = os.path.pathsep.join(sys.path)

Will this not cause problems anywhere else?
--
Christian Robottom Reis | [+55 16] 3376 0125 | http://launchpad.net/~kiko
                        | [+55 16] 9112 6430 | http://async.com.br/~kiko

Revision history for this message
Stuart Bishop (stub) wrote :

On Fri, Jul 24, 2009 at 3:00 AM, Christian Reis<email address hidden> wrote:
> On Thu, Jul 23, 2009 at 08:38:44AM -0000, Stuart Bishop wrote:
>> A startup script for the Librarian.
>> bin/run isn't suitable for use on production as it launches an appserver.
>
> Could it not be made to run the librarian as well? I guess at this point
> whatever's easiest to do and causes less problems is sanest.

It was easier to add the new script. The existing code runs daemons in
foreground mode but puts them in the background using
subprocess.Popen - it is designed for devs and around having the
services shutdown when the Launchpad instance does. Its also
undocumented with no command line help, and that doesn't look fixable
without maybe breaking other call sites that depend on it such as
ec2test.

>> === modified file 'Makefile'
>>  stop_codebrowse:
>>       $(PY) sourcecode/launchpad-loggerhead/stop-loggerhead.py
>>
>> +start_librarian: build
>> +     bin/start_librarian
>> +
>> +stop_librarian:
>> +     bin/killservice librarian
>
> Are these created by buildout automatically? I haven't seen any of this
> code before, sorry.

Yes.

In buildout.cfg, there are stanzas that tell it to generate these
scripts - they are wrappers with the correct !# set, some boilerplate
code to set the pythopath correctly, and then invokes the entry point
method (canonical.launchpad.scripts.runlaunchpad.start_librarian).

>> === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
>
> What else uses this script? /bin/run?

Yes.

>> @@ -84,7 +85,6 @@
>>          tacfile = make_abspath(self.tac_filename)
>>
>>          args = [
>> -            sys.executable,
>>              TWISTD_SCRIPT,
>
> Was this broken or just unnecessary? I can't see what this code does
> from the context.

It was unnecessary - it needlessly tied us to using the same version
of Python that was used to invoke this script. Buildout handles this
for us, adding the correct interpreter and python paths per buildout
configuration.

>> -        # We want to make sure that the Launchpad process will have the
>> -        # benefit # of all of the dependency paths inserted by the buildout
>> -        # bin/run script. We pass them via PYTHONPATH.
>> -        env = dict(os.environ)
>> -        env['PYTHONPATH'] = os.path.pathsep.join(sys.path)
>
> Will this not cause problems anywhere else?

No - buildout handles this for us now by building _pythonpath.py will
all the libraries we are supposed to need. If you don't like it, we
can land this bit another time - it is harmless and just a drive by
cleanup to keep our variables down when trying to debug things.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

Revision history for this message
Christian Reis (kiko) wrote :

On Fri, Jul 24, 2009 at 02:18:08AM -0000, Stuart Bishop wrote:
> >> bin/run isn't suitable for use on production as it launches an appserver.
> >
> > Could it not be made to run the librarian as well? I guess at this point
> > whatever's easiest to do and causes less problems is sanest.
>
> It was easier to add the new script. The existing code runs daemons in
> foreground mode but puts them in the background using
> subprocess.Popen - it is designed for devs and around having the
> services shutdown when the Launchpad instance does. Its also

And for production deployments, what should be used to launch the
launchpad instances, and stuff like scripts and daemons? Do/should they
all have their own scripts in the same line as start_librarian?

> undocumented with no command line help, and that doesn't look fixable
> without maybe breaking other call sites that depend on it such as
> ec2test.

Can you file a bug on it being undocumented, maybe assign to Gary?

> >> === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
> >> -        # We want to make sure that the Launchpad process will have the
> >> -        # benefit # of all of the dependency paths inserted by the buildout
> >> -        # bin/run script. We pass them via PYTHONPATH.
> >> -        env = dict(os.environ)
> >> -        env['PYTHONPATH'] = os.path.pathsep.join(sys.path)
> >
> > Will this not cause problems anywhere else?
>
> No - buildout handles this for us now by building _pythonpath.py will
> all the libraries we are supposed to need. If you don't like it, we
> can land this bit another time - it is harmless and just a drive by
> cleanup to keep our variables down when trying to debug things.

Well, we're likely to test this works on a production-like box before
actually updating the librarian, so I think this is fine. rc=kiko or can
I do something like

    approve
    merge-approved

to do this via email? :-)
--
Christian Robottom Reis | [+55 16] 3376 0125 | http://launchpad.net/~kiko
                        | [+55 16] 9112 6430 | http://async.com.br/~kiko

Revision history for this message
Stuart Bishop (stub) wrote :

On Fri, Jul 24, 2009 at 5:48 PM, Christian Reis<email address hidden> wrote:
> On Fri, Jul 24, 2009 at 02:18:08AM -0000, Stuart Bishop wrote:
>> >> bin/run isn't suitable for use on production as it launches an appserver.
>> >
>> > Could it not be made to run the librarian as well? I guess at this point
>> > whatever's easiest to do and causes less problems is sanest.
>>
>> It was easier to add the new script. The existing code runs daemons in
>> foreground mode but puts them in the background  using
>> subprocess.Popen - it is designed for devs and around having the
>> services shutdown when the Launchpad instance does. Its also
>
> And for production deployments, what should be used to launch the
> launchpad instances, and stuff like scripts and daemons? Do/should they
> all have their own scripts in the same line as start_librarian?

I don't know what things should look like ideally. Perhaps use
supervisord to fireup instances, automatically restart on failure
conditions, report back to a central monitoring server. For now, we
need something that starts the Librarian.

>> undocumented with no command line help, and that doesn't look fixable
>> without maybe breaking other call sites that depend on it such as
>> ec2test.
>
> Can you file a bug on it being undocumented, maybe assign to Gary?

Bug 404008.

>> >> === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
>> >> -        # We want to make sure that the Launchpad process will have the
>> >> -        # benefit # of all of the dependency paths inserted by the buildout
>> >> -        # bin/run script. We pass them via PYTHONPATH.
>> >> -        env = dict(os.environ)
>> >> -        env['PYTHONPATH'] = os.path.pathsep.join(sys.path)
>> >
>> > Will this not cause problems anywhere else?
>>
>> No - buildout handles this for us now by building _pythonpath.py will
>> all the libraries we are supposed to need. If you don't like it, we
>> can land this bit another time - it is harmless and just a drive by
>> cleanup to keep our variables down when trying to debug things.
>
> Well, we're likely to test this works on a production-like box before
> actually updating the librarian, so I think this is fine. rc=kiko or can

Yup. Now with the startup script in Launchpad, we should update
staging so it tests it to catch any future breakage.

> I do something like
>
>    approve
>    merge-approved
>
> to do this via email? :-)

Ta.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Makefile'
2--- Makefile 2009-07-19 02:34:24 +0000
3+++ Makefile 2009-07-23 06:57:17 +0000
4@@ -176,6 +176,12 @@
5 stop_codebrowse:
6 $(PY) sourcecode/launchpad-loggerhead/stop-loggerhead.py
7
8+start_librarian: build
9+ bin/start_librarian
10+
11+stop_librarian:
12+ bin/killservice librarian
13+
14 pull_branches: support_files
15 # Mirror the hosted branches in the development upload area to the
16 # mirrored area.
17
18=== modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
19--- lib/canonical/launchpad/scripts/runlaunchpad.py 2009-07-17 00:26:05 +0000
20+++ lib/canonical/launchpad/scripts/runlaunchpad.py 2009-07-23 06:57:17 +0000
21@@ -19,13 +19,14 @@
22 from canonical.launchpad.mailman import runmailman
23 from canonical.launchpad.testing import googletestservice
24
25-TWISTD_SCRIPT = None
26-
27
28 def make_abspath(path):
29 return os.path.abspath(os.path.join(config.root, *path.split('/')))
30
31
32+TWISTD_SCRIPT = make_abspath('bin/twistd')
33+
34+
35 class Service(object):
36 @property
37 def should_launch(self):
38@@ -84,7 +85,6 @@
39 tacfile = make_abspath(self.tac_filename)
40
41 args = [
42- sys.executable,
43 TWISTD_SCRIPT,
44 "--no_save",
45 "--nodaemon",
46@@ -94,12 +94,6 @@
47 "--logfile", logfile,
48 ]
49
50- # We want to make sure that the Launchpad process will have the
51- # benefit # of all of the dependency paths inserted by the buildout
52- # bin/run script. We pass them via PYTHONPATH.
53- env = dict(os.environ)
54- env['PYTHONPATH'] = os.path.pathsep.join(sys.path)
55-
56 if config[self.section_name].spew:
57 args.append("--spew")
58
59@@ -108,7 +102,7 @@
60 # log to stdout and redirect it ourselves because we then lose the
61 # ability to cycle the log files by sending a signal to the twisted
62 # process.
63- process = subprocess.Popen(args, stdin=subprocess.PIPE, env=env)
64+ process = subprocess.Popen(args, stdin=subprocess.PIPE)
65 process.stdin.close()
66 # I've left this off - we still check at termination and we can
67 # avoid the startup delay. -- StuartBishop 20050525
68@@ -254,9 +248,6 @@
69
70
71 def start_launchpad(argv=list(sys.argv)):
72- global TWISTD_SCRIPT
73- TWISTD_SCRIPT = make_abspath('bin/twistd')
74-
75 # We really want to replace this with a generic startup harness.
76 # However, this should last us until this is developed
77 services, argv = split_out_runlaunchpad_arguments(argv[1:])
78@@ -275,3 +266,19 @@
79 config.generate_overrides()
80
81 main(argv)
82+
83+def start_librarian():
84+ """Start the Librarian in the background."""
85+ # Create the ZCML override file based on the instance.
86+ config.generate_overrides()
87+ # Create the Librarian storage directory if it doesn't already exist.
88+ prepare_for_librarian()
89+ pidfile = pidfile_path('librarian')
90+ cmd = [
91+ TWISTD_SCRIPT,
92+ "--python", 'daemons/librarian.tac',
93+ "--pidfile", pidfile,
94+ "--prefix", 'Librarian',
95+ "--logfile", config.librarian_server.logfile,
96+ ]
97+ return subprocess.call(cmd)
98
99=== modified file 'setup.py'
100--- setup.py 2009-07-18 21:19:37 +0000
101+++ setup.py 2009-07-23 06:57:17 +0000
102@@ -65,6 +65,8 @@
103 'run = canonical.launchpad.scripts.runlaunchpad:start_launchpad',
104 'harness = canonical.database.harness:python',
105 'twistd = twisted.scripts.twistd:run',
106+ 'start_librarian '
107+ '= canonical.launchpad.scripts.runlaunchpad:start_librarian',
108 ]
109 ),
110 )