Merge lp:~edwin-grubbs/launchpad/bug-615654-jobqueue-cron-script into lp:launchpad/db-devel

Hi Edwin,

This branch looks good.

I know you're adding ProcessJobSourceGroups in here as well as the
membership stuff, but it still seems like a lot of work to do
something asynchronously. Moving something out of the app server takes
a lot of plumbing work, and it's not trivial plumbing work either.

I'd really like a way to create, in the app server, something like a
persistent closure that can be run later, there or somewhere
else. When run it would have the same user and database access as in
the app server thread that spawned it. Creating a new job now seems to
involve creating tables, database users, job sources, and so on, and
it's getting in the way of the actual work that needs doing.

I'm rambling on, and it's not a fault in this branch, but after doing
this recently, do you have any thoughts on that?

Gavin.

[1]

+ for job_source in selected_job_sources:
+ child = subprocess.Popen(child_args + [job_source])
+ children.append(child)
+ for child in children:
+ child.wait()

Is selected_job_sources ever likely to be large? If not now, but could
be one day, consider filing a bug to limit the number of concurrency
processes.

[2]

+ for child in children:
+ child.wait()

Is there any value in checking and doing something with the exit code
from the child?

[3]

+ packages=('_pythonpath', 'twisted', 'ampoule'), env=env)

I just looked again at _pythonpath.py and now I can't see.

Gavin, radical changes in the way we create and run tasks are the purview of the NewTaskSystem. Not having to create new database users is already listed in https://dev.launchpad.net/Foundations/NewTaskSystem/Requirements and you may wish to add your other wishes under "nice to have".

> Hi Edwin,
>
> This branch looks good.

Thanks for the review. Unfortunately, abentley had already started reviewing it, but it was all done on IRC up to this point. I should have just listed him as the requested reviewer. Sorry for the duplicate effort. I'll try to answer your questions below.

> I know you're adding ProcessJobSourceGroups in here as well as the
> membership stuff, but it still seems like a lot of work to do
> something asynchronously. Moving something out of the app server takes
> a lot of plumbing work, and it's not trivial plumbing work either.
>
> I'd really like a way to create, in the app server, something like a
> persistent closure that can be run later, there or somewhere
> else. When run it would have the same user and database access as in
> the app server thread that spawned it. Creating a new job now seems to
> involve creating tables, database users, job sources, and so on, and
> it's getting in the way of the actual work that needs doing.
>
> I'm rambling on, and it's not a fault in this branch, but after doing
> this recently, do you have any thoughts on that?
>
> Gavin.

It seems that the reason we don't support a closure method for processing the jobs is that there are a lot of security restrictions placed on us. For example, each job class is required to have its own dbuser, so that the losas can figure out which job class is causing problems on the db. Of course, the job system seems much better suited for really slow processes, and it might be worthwhile to have a secondary solution that just moves the processing after the request response.

> [1]
>
> + for job_source in selected_job_sources:
> + child = subprocess.Popen(child_args + [job_source])
> + children.append(child)
> + for child in children:
> + child.wait()
>
> Is selected_job_sources ever likely to be large? If not now, but could
> be one day, consider filing a bug to limit the number of concurrency
> processes.

Currently, each job source class is given its own process. If we run into a problem with too many concurrent jobs being processed, the job source classes can be reorganized into different crontab_groups, and then different crontab entries can process the jobs at different times. We are not really interested in a long term solution for this, since the job system will most likely be moved to some kind of message queue, so the cron scripts for job processing will disappear.

> [2]
>
> + for child in children:
> + child.wait()
>
> Is there any value in checking and doing something with the exit code
> from the child?

No, waiting for the children to finish is really only helpful for testing, so that all the output from the child processes can be gathered. The cron scripts log to files, and I am actually making the default behavior not to wait on the child processes, so that one really long running child process does not cause the other job sources not to run the next time the crontab entry calls it, because the parent process still has its lock file.

> [3]
>
> + packages=('_pythonpath', 'twisted', 'ampoule'), env=env)
>
> I just looked again at ...

Waiting for children to exit will mean that no new jobs can be processed until all the jobs complete. This may severely impair responsiveness. I suggest starting the subprocesses and then exiting.

The NoErrorOptionParser should not exist. Option handling should be taken care of in the ProcessJobSource class. If the layering is getting in our way, we should fix it, not work around it.

Unless you are specifically testing stringification, you should not stringify objects in tests, so that your tests are not fragile.

In test_setstatus_admin, what's the purpose of transaction.commit?

Since you do person_set.getByEmail('<email address hidden>'), you can use login_person or with person_logged_in instead of login. Also, lifeless requested that you use the constant for <email address hidden>' instead of spelling it out.

Please use lp.testing.run_script instead of rolling your own.

In test_processed, I don't think you really care what the database id of the job is, so use assertTextMatchesExpressionIgnoreWhitespace. (You'll need to move it to TestCaseWithFactory.)

get isn't required to run jobs under the non-twisted job runner, so it seems like an unnecessary requirement on IJobSource.

Perhaps ProcessJobSource should go in lp.services.job.runner ?

test_empty_queue looks fragile because it includes the directory of the lock file. Perhaps another case for assertTextMatchesExpressionIgnoreWhitespace?

Perhaps PYTHONPATH and LPCONFIG should be handled by common code, eg:
for name in ['PYTHONPATH', 'LPCONFIG']:
    if name in os.environ:
        env[name] = os.environ[name]

> Thanks for the review. Unfortunately, abentley had already started reviewing
> it, but it was all done on IRC up to this point. I should have just listed him
> as the requested reviewer. Sorry for the duplicate effort. I'll try to answer
> your questions below.

No worries. It's good that Aaron also reviewed this. He spotted a lot
of stuff that I skimmed over; I didn't take as long on this review as
I ought to have done.

>
> > I know you're adding ProcessJobSourceGroups in here as well as the
> > membership stuff, but it still seems like a lot of work to do
> > something asynchronously. Moving something out of the app server takes
> > a lot of plumbing work, and it's not trivial plumbing work either.
> >
> > I'd really like a way to create, in the app server, something like a
> > persistent closure that can be run later, there or somewhere
> > else. When run it would have the same user and database access as in
> > the app server thread that spawned it. Creating a new job now seems to
> > involve creating tables, database users, job sources, and so on, and
> > it's getting in the way of the actual work that needs doing.
> >
> > I'm rambling on, and it's not a fault in this branch, but after doing
> > this recently, do you have any thoughts on that?
> >
> > Gavin.
>
>
> It seems that the reason we don't support a closure method for processing the
> jobs is that there are a lot of security restrictions placed on us. For
> example, each job class is required to have its own dbuser, so that the losas
> can figure out which job class is causing problems on the db. Of course, the
> job system seems much better suited for really slow processes, and it might be
> worthwhile to have a secondary solution that just moves the processing after
> the request response.

Yes, that sounds good. As suggested by Aaron, I'll add it to the
wishlist.

>
> > [1]
> >
> > + for job_source in selected_job_sources:
> > + child = subprocess.Popen(child_args + [job_source])
> > + children.append(child)
> > + for child in children:
> > + child.wait()
> >
> > Is selected_job_sources ever likely to be large? If not now, but could
> > be one day, consider filing a bug to limit the number of concurrency
> > processes.
>
>
> Currently, each job source class is given its own process. If we run into a
> problem with too many concurrent jobs being processed, the job source classes
> can be reorganized into different crontab_groups, and then different crontab
> entries can process the jobs at different times. We are not really interested
> in a long term solution for this, since the job system will most likely be
> moved to some kind of message queue, so the cron scripts for job processing
> will disappear.

Okay, fair enough.

>
> > [2]
> >
> > + for child in children:
> > + child.wait()
> >
> > Is there any value in checking and doing something with the exit code
> > from the child?
>
>
> No, waiting for the children to finish is really only helpful for testing, so
> that all the output from the child processes can be gathered. The cron scripts
> log to files, and I am actually making the default behavi...

On Thu, Sep 30, 2010 at 11:02 AM, Aaron Bentley <email address hidden> wrote:
> Review: Needs Fixing
> Waiting for children to exit will mean that no new jobs can be processed until all the jobs complete.  This may severely impair responsiveness.  I suggest starting the subprocesses and then exiting.

It no longer waits for child processes by default. I added an option
to wait, which is convenient when running it from the command line
when experimenting.

> The NoErrorOptionParser should not exist.  Option handling should be taken care of in the ProcessJobSource class.  If the layering is getting in our way, we should fix it, not work around it.

I moved most of the setup code to the run() methods in LaunchpadScript
and LaunchpadCronScript so __init__ does very little now besides
setting up the parser attribute.

> Unless you are specifically testing stringification, you should not stringify objects in tests, so that your tests are not fragile.

Fixed.

> In test_setstatus_admin, what's the purpose of transaction.commit?

I'm not sure why I was thinking I would need that. Removed.

> Since you do person_set.getByEmail('<email address hidden>'), you can use login_person or with person_logged_in instead of login.  Also, lifeless requested that you use the constant for <email address hidden>' instead of spelling it out.

Fixed.

> Please use lp.testing.run_script instead of rolling your own.

Fixed.

> In test_processed, I don't think you really care what the database id of the job is, so use  assertTextMatchesExpressionIgnoreWhitespace.  (You'll need to move it to TestCaseWithFactory.)

Fixed.

> get isn't required to run jobs under the non-twisted job runner, so it seems like an unnecessary requirement on IJobSource.

ok, I've moved get() to a new ITwistedJobSource interface.

> Perhaps ProcessJobSource should go in lp.services.job.runner ?

I'd rather wait until we have a reason for subclassing from it, which
I hope won't happen before we stop using cron for job processing.

> test_empty_queue looks fragile because it includes the directory of the lock file.  Perhaps another case for assertTextMatchesExpressionIgnoreWhitespace?

Fixed.

> Perhaps PYTHONPATH and LPCONFIG should be handled by common code, eg:
> for name in ['PYTHONPATH', 'LPCONFIG']:
>    if name in os.environ:
>        env[name] = os.environ[name]

Fixed.

Incremental diff:

=== modified file 'cronscripts/process-job-source-groups.py'
--- cronscripts/process-job-source-groups.py 2010-09-28 15:50:05 +0000
+++ cronscripts/process-job-source-groups.py 2010-09-30 20:33:49 +0000
@@ -47,6 +47,13 @@
             '-e', '--exclude', dest='excluded_job_sources',
             metavar="JOB_SOURCE", default=[], action='append',
             help="Exclude specific job sources.")
+ self.parser.add_option(
+ '--wait', dest='do_wait', default=False, action='store_true',
+ help="Wait for the child processes to finish. This is useful "
+ "for testing, but it shouldn't be used in a cronjob, since "
+ "it would prevent the cronjob from processing new jobs "
+ "if just one of the child processes is still processing, "
...

I realized that I could make process-job-source.py simpler. Here is the diff.

=== modified file 'cronscripts/process-job-source.py'
--- cronscripts/process-job-source.py 2010-09-30 20:39:02 +0000
+++ cronscripts/process-job-source.py 2010-10-01 13:39:54 +0000
@@ -24,12 +24,8 @@
         "For more help, run:\n"
         " cronscripts/process-job-source-groups.py --help")

- def configure(self):
- """Override configs main() is called by run().
-
- It is done here instead of __init__(), so that we get to use its
- option parser.
- """
+ def __init__(self):
+ super(ProcessJobSource, self).__init__()
         if len(self.args) != 1:
             self.parser.print_help()
             sys.exit(1)
@@ -54,5 +50,4 @@

 if __name__ == '__main__':
     script = ProcessJobSource()
- script.configure()
     script.lock_and_run()

I ran into some problems merging in db-devel due to some changes in LaunchpadCronScript. I switched to using properties to make it easy for subclasses to override the values without interfering with __init__() logic.

Instead of an incremental diff, here is a full diff of the affected files.

=== added file 'cronscripts/process-job-source.py'
--- cronscripts/process-job-source.py 1970-01-01 00:00:00 +0000
+++ cronscripts/process-job-source.py 2010-10-04 04:01:36 +0000
@@ -0,0 +1,69 @@
+#!/usr/bin/python -S
+#
+# Copyright 2009, 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Handle jobs for a specified job source class."""
+
+__metaclass__ = type
+
+import sys
+
+import _pythonpath
+from twisted.python import log
+
+from canonical.config import config
+from lp.services.job import runner
+from lp.services.job.runner import JobCronScript
+
+
+class ProcessJobSource(JobCronScript):
+ """Run jobs for a specified job source class."""
+ usage = (
+ "Usage: %prog [options] JOB_SOURCE\n\n"
+ "For more help, run:\n"
+ " cronscripts/process-job-source-groups.py --help")
+
+ def __init__(self):
+ super(ProcessJobSource, self).__init__()
+ # The fromlist argument is necessary so that __import__()
+ # returns the bottom submodule instead of the top one.
+ module = __import__(self.config_section.module,
+ fromlist=[self.job_source_name])
+ self.source_interface = getattr(module, self.job_source_name)
+
+ @property
+ def config_name(self):
+ return self.job_source_name
+
+ @property
+ def name(self):
+ return 'process-job-source-%s' % self.job_source_name
+
+ @property
+ def dbuser(self):
+ return self.config_section.dbuser
+
+ @property
+ def runner_class(self):
+ runner_class_name = getattr(
+ self.config_section, 'runner_class', 'JobRunner')
+ # Override attributes that are normally set in __init__().
+ return getattr(runner, runner_class_name)
+
+ def handle_options(self):
+ if len(self.args) != 1:
+ self.parser.print_help()
+ sys.exit(1)
+ self.job_source_name = self.args[0]
+ super(ProcessJobSource, self).handle_options()
+
+ def main(self):
+ if self.options.verbose:
+ log.startLogging(sys.stdout)
+ super(ProcessJobSource, self).main()
+
+
+if __name__ == '__main__':
+ script = ProcessJobSource()
+ script.lock_and_run()

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py 2010-09-20 09:48:58 +0000
+++ lib/lp/services/job/runner.py 2010-10-04 04:01:36 +0000
@@ -77,7 +77,6 @@

     # We redefine __eq__ and __ne__ here to prevent the security proxy
     # from mucking up our comparisons in tests and elsewhere.
-
     def __eq__(self, job):
         return (
             self.__class__ is removeSecurityProxy(job.__class__)
@@ -337,13 +336,12 @@
     """Run Jobs via twisted."""

     def __init__(self, job_source, dbuser, logger=None, error_utility=None):
- env = {'PYTHONPATH': os.e...

This looks good, except that there are now two definitions assertTextMatchesExpressionIgnoreWhitespace; one in TestCaseWithFactory and one in BrowserTestCase. Actually, it's not factory-specific, so it should probably go in TestCase.