Merge lp://staging/~pwlars/ubuntu-ci-services-itself/full-er-er-imagebuild-logging into lp://staging/ubuntu-ci-services-itself

PASSED: Continuous integration, rev:310
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/291/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/291/rebuild

its hard to argue with "full-er-er"!

This is really orthogonal to this MP, but I just noticed an issue in run_worker when looking at it as a whole:

In your finally block you call:

        logurl = store.put_file('imagebuild-output.log',
                                logstringio.getvalue())

before you call amqp_utils.progress_failed or amqp_utils.progress_completed. This could lead to the lander job never terminating if we fail to upload this logging information.

Another note as per:

  https://code.launchpad.net/~doanac/ubuntu-ci-services-itself/bug1286232-mimetype/+merge/209330

You should probably merge this branch with trunk once my MP has landed. Then you can change this call:

 logurl = store.put_file('imagebuild-output.log',
                         logstringio.getvalue())

to:
  logurl = store.put_file('imagebuild-output.log',
                          logstringio.getvalue(),
                          content_type='text/plain')

This way people can click on the URL inside the webui and be able to view it in browser (for some reason swift doesn't guess the mime-type correctly for our .log files).

Ok, I think this should handle all your comments. One more thing I'd like to do is add some unit tests for run_worker as there are quite a few more paths through it now. It will have to be heavily mocked of course, but I think it would be worthwhile. On the other hand, I'd like to get this merged sooner than later, since it helps quite a bit with debugging when things go wrong in the image build step. So I added https://bugs.launchpad.net/ubuntu-ci-services-itself/+bug/1288543 to remind me.

PASSED: Continuous integration, rev:312
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/302/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/302/rebuild

On 03/05/2014 10:18 PM, Paul Larson wrote:
> One more thing I'd like to do is add some unit tests for run_worker as there are quite a few more paths through it now.

It might be worth considering creating some type of run_worker base
function/class that can be used for all the run workers that handles the
tricky parts of amqp_utils calls and unexcepted exceptions. Then we
could just write a test for that and let the actual run_workers focus on
handling domain-specific problems

looks better.

I think you may have issues merging with the changes vila made when renaming things run_worker.py. I've got a fix for run_worker stuff. so you might want to try and merge with my changes (coming shortly) or just merge this knowing it won't work out-of-the-box

> you may have issues merging with the changes vila made when renaming things run_worker.py

Let's trust bzr and see what happens ;) (Shamelessly tried to merge into trunk and saw no issues ;)

103 + except:
104 + ds_exc_type, ds_val, ds_tb = sys.exc_info()
105 + log.exception('data_store put_file failed:')
106 + if isinstance(e, KeyboardInterrupt):
107 + raise # re-raise so amqp_utils.process_queue can exit

'isinstance(e' there is no 'e' in the except clause (miss Exception as e ?)

I understand what you're after here (getting the exception in the finally clause), but this is hard to read (and error prone as seen above).

We probably want a simpler way to get that result but I won't block on that as long as you mention it in a comment.

FAILED: Continuous integration, rev:313
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/308/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/308/rebuild

This should fix things up. I made some fixes and handled the cut/paste error that vila mentioned, and was able to merge it cleanly a few minutes ago and it merged cleanly.

PASSED: Continuous integration, rev:314
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/309/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/309/rebuild

Merge with trunk

I'm re-acking. I looking at the complexity of these run_workers, i'm starting to worry we should create a safe wrapper for these in phase-0.

PASSED: Continuous integration, rev:315
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/311/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/311/rebuild