882110 - [a10n] move get-pushes from master-ball to a10n

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect)

Description

[Axel Hecht [:Pike]]

As part of a10n, the first step we could migrate to queues is the getPushes work.

I've got a branch on a10n, https://github.com/Pike/a10n/compare/feature;hg-task, giving us the following:

twisted hg poller on twisted 13.0.0 (free, actually, that just worked)
retries on hg failures (course right now)
max-retries, let's fail and stop the master if we can't get it working
sentry logging of those errors (yay, a web page to point IT to)

Open items:

The whole directory structure is a tad ad-hoc.
Once we run out of retries, I don't ack the message. It just stays in the queue. I'm not sure if this is a bug or a feature.
Docs need to be written.
I wonder how to test this, short of having a test install.
The whole queue setup is mostly copy and paste without understanding what I do. It works, but that's it.
What to do with requirements, pin via requirements, or dump into the a10n repo?

Requirements:
kombu for queue management
rabbitmq as broker. redis doesn't seem to be able to do requeue.

Comment 1

[Axel Hecht [:Pike]]

Attachment: use kombu for getPushes

Jannis, up for a review? This is early code, bike-sheds should be had now, style and setup and other useless stuff will be harder to fix later.

Comment 2

[Jannis Leidel [:jezdez]]

Comment on attachment 761384 [details] [diff] [review]
use kombu for getPushes

Review of attachment 761384 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, I have a few suggestions for the structure.

I think it make sense to think about a10n like a library that is self-contained and has a bunch of executable script to trigger the functionality it contains, e.g. `a10n hg_worker`, `a10n compare_worker` etc.

My suggestions for the package (albeit some can be done later, like packaging):

- introduce a Python package named 'a10n' and move the files in hg_elmo/ to it
- move autosettings into a10n.settings and use it as DJANGO_SETTINGS_MODULE
- use argparse (now in stdlib) to create a central command with subcommands in a new module named `runner.py`
- move the current runner code in worker.py (the optparse bit etc) into `runner.py`
- add a 'hg_worker' subcommand that uses the code mentioned in the previous step
- add a setup.py that creates a a10n command line tool during installation that uses a10n.runner.main
- use install_requires instead of a requirements file to define dependencies (drop vendor-local, too)

I'm marking this as r- obviously, but I'm happy to help implement those suggestions if you want.

::: hg_elmo/worker.py
@@ +7,5 @@
> +import os
> +import time
> +import site
> +site.addsitedir('vendor-local')
> +site.addsitedir('.')

This shouldn't be needed if we think of the a10n library as an installable package (which we should).

@@ +12,5 @@
> +
> +from kombu.mixins import ConsumerMixin
> +from kombu.log import setup_logging
> +
> +from queues import hg_queues

This should use absolute imports, e.g. `from a10n.queues import hg_queues` or relative ones like `from .queues import hg_queues`.

@@ +37,5 @@
> +    def process_pushes(self, body, message):
> +        if body.get('type') != 'hg-push':
> +            return
> +        logger.info('got hg-push message %r', body)
> +        from pushes.utils import handlePushes, PushJS

Is it required to have this import inline?

@@ +53,5 @@
> +            if self.retries > self.max_retries:
> +                # this problem might be real, let's just die
> +                # and have a human figure it out
> +                raise
> +            time.sleep(self.retries)

This still strikes me as "this can't be the best way" maybe we should talk to ask whether there is a well-known way®

@@ +64,5 @@
> +            return
> +        logger.info('got message %r', body)
> +        message.ack()
> +
> +if __name__ == '__main__':

This should move into a 'hg_worker' script that we can call from the command line easily.

::: twisted/plugins/pushes_plugin.py
@@ +109,5 @@
> +            self.latest_push[repo_id] = submits[-1]['id']
> +            connection = Connection(settings.TRANSPORT)
> +            with producers[connection].acquire(block=True) as producer:
> +                maybe_declare(hg_exchange, producer.channel)
> +                producer.publish(

Maybe have the message to be published in a variable here so we can inspect it in this code frame in case of an exception? Also, do you think we should add Sentry exception handling here?

Comment 3

[Axel Hecht [:Pike]]

Thanks for the feedback. First the high-level stuff:

I'm trying to stay away from requiring a virtualenv and installing packages that are core of what we're hacking on. This is mostly for deployment, which isn't "run setup.py" AFAIK, but "install the rpm we've built for you".

I'm also wary of requiring an installation step for the code we're hacking on.

I'm a tad less concerned about those items for stable upstream packages, it's probably easier to generate packages for those.

Yeah, mozilla style, pypi is considered evil for a production push. At least that's how I got raised here.

Django imports need to come after the settings are done in os.ENVIRON. If we have a central runner script, that might stop being an issue.

I'm still an optparse guy, as argparse is 2.7+. If my suspicion is right, our production hardware will be centos, and 2.6 :-/

Comment 4

[Jannis Leidel [:jezdez]]

Glad you like the general gist of my suggestions.

PyPI got *a lot* more secure, although that's not the place to iterate over that.

The goal of the `setup.py` can also be to create a RPM, and be able to run "pip install -e ." locally. That "installs" it in a virtualenv or globally. virtualenv isn't implied in that step.

argparse is available from PyPI for Python < 2.7 https://pypi.python.org/pypi/argparse

Comment 5

[Axel Hecht [:Pike]]

Attachment: just the refactor and some changes based on the review

Here's an incremental patch, the full patch queue is on https://github.com/Pike/a10n/compare/master...feature;hg-task

Stuff not done:

I'm keeping vendor-local and requirements.txt. vendor-local contains elmo, and we'll not move code there into python modules anytime soon.
I'm open to have the discussion on whether django should be there. There's the pro that we could patch it, there the con that we could patch it.

As I can't really go for setup.py to install the necessary modules anyway, I'm sticking to requirements.txt for now.

I haven't made up my mind yet on how to deal with argsparse and python2.6.

Stuff I did:

Moved the code into an a10n module.

Created a runner script for the hg daemon. That works quite nicely, actually.
It does not include the twisted daemon, because the existing twistd daemon does a bunch of nice things for us, like log rotation, daemonization, etc.

Added sentry to the twistd daemon. Which makes me wonder, I probably should have done the KeyboardInterrupt in the worker, too?

Comment 6

[Jannis Leidel [:jezdez]]

Comment on attachment 764726 [details] [diff] [review]
just the refactor and some changes based on the review

Review of attachment 764726 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good so far, please don't get me wrong, requirements.txt files are fine as long as you want to resolve a dependency chain of an environment (even when not using virtualenv), instead of just one package. Using setup.py's dependency system only makes sense when you're willing to make use of the rest of the distutils/setuptools advantages, e.g. automatic installation of scripts in bin/, repeatable and easy project setup, community conformity. It's in no way the *only* solution, admittedly.

vendor-local is a hack at best (e.g. having to use site.addsitedir is a good indicator) and leads to bad programming patterns like being able to patch the code of 3rd party apps as you mentioned. I can't think of a case in which patching 3rd party code in place (instead of via a monkey patch) leads to easier maintenance. vendor-local also doesn't solve the binary dependency problem.

Regarding argparse, I would argue that if you want to keep using argparse it's easy enough to put it in the requirements.txt. The release on PyPI is an official backport by a Python core team member. You may want to unpack its release file into vendor-lib if you feel that's the way you want to go.

I don't think we should duplicate functionality twistd already implements, agreed.

About catching KeyboardInterrupt in the runner, yes, let's do this.

Other than that, I like this a lot.

::: scripts/a10n
@@ +39,5 @@
> +os.environ["DJANGO_SETTINGS_MODULE"] = args.settings
> +
> +if args.cmd == 'hg':
> +    import a10n.hg_elmo.worker
> +    a10n.hg_elmo.worker.run(args)

Maybe print a help message here in case non of the subcommands have been used? parser.exit(status=0, message='something something') should be enough. Or is this done already?

Comment 7

[Axel Hecht [:Pike]]

Attachment: address review comments, remove django from vendor-local (not in patch)

Janis, trying you for another review. I hope it's quick enough that you're fine doing this once more.

This is three follow-ups to the previous patch:

Remove django from vendor-local, specify 1.4.5 in requirements, https://github.com/Pike/a10n/commit/88dc49884679b1d336f9696ca12634c6b8ec5864
vendor-local is now only elmo. I excluded the django files from the attached diff

Add argparse to requirements, https://github.com/Pike/a10n/commit/e7d75aede7125f8555ecbb79201ef3cd9a909b1a

Don't catch KeyboardError in hg worker, https://github.com/Pike/a10n/commit/ced3266620ac4dd8fede369ea23c3e4e0551d31c

The no-subcommand case is handled by argparse sufficiently well, IMHO, there's nothing to do there:

./scripts/a10n 
usage: a10n [-h] [--settings SETTINGS] {help,hg} ...
a10n: error: too few arguments

Comment 8

[Jannis Leidel [:jezdez]]

Comment on attachment 767137 [details] [diff] [review]
address review comments, remove django from vendor-local (not in patch)

Review of attachment 767137 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 9

[Axel Hecht [:Pike]]

Rebased a bit and landed. pypi acted up, gonna deploy that later today.