Merge lp://qastaging/~doanac/lava-celery/scheduler-monitor-support into lp://qastaging/lava-celery
Proposed by
Andy Doan
| Status: | Merged |
|---|---|
| Merged at revision: | 27 |
| Proposed branch: | lp://qastaging/~doanac/lava-celery/scheduler-monitor-support |
| Merge into: | lp://qastaging/lava-celery |
| Diff against target: |
281 lines (+150/-50) 3 files modified
lava/celery/commands.py (+70/-3) lava/celery/tasks.py (+79/-47) setup.py (+1/-0) |
| To merge this branch: | bzr merge lp://qastaging/~doanac/lava-celery/scheduler-monitor-support |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Michael Hudson-Doyle (community) | Approve | ||
|
Review via email:
|
|||
Description of the change
This updates the celery module a few ways:
1) improve run_command_task to take files as arguments
2) use fork/exec for run_command task
3) add a new celery-
To post a comment you must log in.
Revision history for this message
| Andy Doan (doanac) wrote | # |
Revision history for this message
| Michael Hudson-Doyle (mwhudson) wrote | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Yay for all that Unix fd yoga.
I think you should be a bunch more anal about errors in the child process: don't call sys.exit() but rather os._exit() and probably do it in a finally: block whose try: you enter first thing in the pid == 0 branch (going through the finally: clause in the run_command twice would be pretty confusing I think!).
You add celery-
There's something about the way files are handled that makes me uneasy -- the code on the worker side doesn't do anything about directories or permissions, so I could imagine that if we started to write the json into /tmp/blah/foo.json rather than directly in /tmp/ things might break in unexpected ways. But well, maybe that just needs an XXX.
Otherwise, all looks ok!