Minor refactoring in task_runner.py in preparation for adding more code.

Minor refactoring in task_runner.py in preparation for adding more code.

In particular:
  * Move all auth related stuff into bot_auth.py. It will grow later.
    task_runner.py is already huge.
  * Move auth init/shutdown to load_and_run, run_command is too big.
  * Use InternalError exception instead of ValueError to signal errors.
  * Rename MustExit to ExitSignal to indicate that it is raised by signals.
  * Various nits in comments.

Also it makes more kinds of errors recognized as internal errors. For example,
IOError when reading from a task process pipe.

R=maruel@chromium.org
BUG=

Committed: https://github.com/luci/luci-py/commit/43a6d619519fc21eb427cdd25b6d912c4616ff6f

[Vadim Sh., 2016-08-31 03:53:33]

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/bot_auth.py (right):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_auth.py:70: class
AuthSystem(object):
I couldn't come up with a better name...

I want all of this to be encapsulated in a single object that can be started and
stopped.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (left):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:516: proc,
task_details.grace_period, 'exception %s' % e)
'e' here was undefined :)

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:221: # The work
directory is guaranteed to exist since it was created by
moved comment to closer where it is relevant

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:253: except (ExitSignal,
InternalError) as e:
InternalError here is new. I believe it's more clean to catch it here and
transform it into task_result rather than let task_runner.py crash dumping the
stack trace. At least we'll see exception message in Swarming event log.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:514: except (ExitSignal,
InternalError, IOError, OSError) as e:
same here... 

Was there a reason IOError and OSError are just killing the task, but not record
internal_error?

IIUC they never happen if task dies on its own (even if with error).

Also tentatively added InternalError here. It can happen if connectivity with
the server is lost. I think before we just crashed with ValueError without doing
a proper cleanup.

[M-A Ruel, 2016-08-31 15:03:47]

lgtm

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/bot_auth.py (right):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_auth.py:70: class
AuthSystem(object):
On 2016/08/31 03:53:32, Vadim Sh. wrote:
> I couldn't come up with a better name...

BigBrother. :D

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_auth.py:90:
self._auth_params_reader = None
you should guard it with a threading.Lock()

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_auth.py:107: reader =
file_reader.FileReaderThread(auth_params_file, interval_sec=23)
Why not 53?

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (left):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:516: proc,
task_details.grace_period, 'exception %s' % e)
On 2016/08/31 03:53:32, Vadim Sh. wrote:
> 'e' here was undefined :)

I tried to be defensive but sucked at it.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:262:
u'must_signal_internal_failure': e.message or 'unknown error',
str(e.message) if e.message else 'unknown error'

so that if message is not a string, it won't crash json encoder.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:271: if auth_system:
One thing that could help is to use contextlib. contextmanager more. No need to
change this CL but as a potential example:

@contextlib.contextmanager
def auth_system(auth_params_file):
  <lines 231-248>
  yield headers_cb
  <lines 271-272>

then use:
with auth_system(auth_params_files) as headers_cb:

wdyt?

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:477: # Server is telling
us to stop. Normally task cancellation.
"Rarely used outside the U.S. According to graphs that show the popularity of
cancelation vs. cancellation in the U.S., cancelation is almost three times as
common in the U.S. after the 80s."  :D

But as a non-American, that's fine, you can keep it.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:514: except (ExitSignal,
InternalError, IOError, OSError) as e:
On 2016/08/31 03:53:33, Vadim Sh. wrote:
> same here... 
> 
> Was there a reason IOError and OSError are just killing the task, but not
record
> internal_error?
> 
> IIUC they never happen if task dies on its own (even if with error).
> 
> Also tentatively added InternalError here. It can happen if connectivity with
> the server is lost. I think before we just crashed with ValueError without
doing
> a proper cleanup.

- If the file is missing, subprocess.Popen(['missing_file'], ...) will raise
OSError. This is not an internal error, it's a user error, the person forgot to
map the file.
- For IOError, I don't recall exactly. It _could_ be file mode on POSIX or
something else but I forgot to note why it could happen.
- Both are now moot because now task_runner always starts [sys.executable,
'swarming_bot.N.zip', 'run_isolated', ...], which wasn't the case before when
this code was first written. So now OSError *is* an internal error, as this now
means that swarming_bot.N.zip somehow disappeared.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:516:
must_signal_internal_failure = e.message or 'unknown error'
str(e.message) if e.message else 'unknown error'

I prefer to be defensive here

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:606:
must_signal_internal_failure = e.message or 'unknown error'
same

[Vadim Sh., 2016-09-01 00:15:56]

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/bot_auth.py (right):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_auth.py:90:
self._auth_params_reader = None
On 2016/08/31 15:03:46, M-A Ruel wrote:
> you should guard it with a threading.Lock()

Done. Though I think its overkill in Python. GIL effectively enforces atomicity
of attribute writes and reads.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/bot_auth.py:107: reader =
file_reader.FileReaderThread(auth_params_file, interval_sec=23)
On 2016/08/31 15:03:46, M-A Ruel wrote:
> Why not 53?

Dunno. Changed to 53. It's not really important.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:262:
u'must_signal_internal_failure': e.message or 'unknown error',
On 2016/08/31 15:03:47, M-A Ruel wrote:
> str(e.message) if e.message else 'unknown error'
> 
> so that if message is not a string, it won't crash json encoder.

Done.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:271: if auth_system:
On 2016/08/31 15:03:47, M-A Ruel wrote:
> One thing that could help is to use contextlib. contextmanager more. No need
to
> change this CL but as a potential example:
> 
> @contextlib.contextmanager
> def auth_system(auth_params_file):
>   <lines 231-248>
>   yield headers_cb
>   <lines 271-272>
> 
> then use:
> with auth_system(auth_params_files) as headers_cb:
> 
> wdyt?

I've considered this. AuthSystem isn't supposed to be used in many places. And
the place where it is used already has 'finally' block. So I decided just to
reuse this 'finally' block instead of adding additional layer of indentation.
Deep levels of try: and with: are hard to read.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:477: # Server is telling
us to stop. Normally task cancellation.
On 2016/08/31 15:03:47, M-A Ruel wrote:
> "Rarely used outside the U.S. According to graphs that show the popularity of
> cancelation vs. cancellation in the U.S., cancelation is almost three times as
> common in the U.S. after the 80s."  :D
> 
> But as a non-American, that's fine, you can keep it.

Oh. My spellchecker is British then or something :) I read the code, see a word
highlighted by spellchecker, and change it to suggested replacement. Good way to
find typos in comments.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:514: except (ExitSignal,
InternalError, IOError, OSError) as e:
On 2016/08/31 15:03:47, M-A Ruel wrote:
> On 2016/08/31 03:53:33, Vadim Sh. wrote:
> > same here... 
> > 
> > Was there a reason IOError and OSError are just killing the task, but not
> record
> > internal_error?
> > 
> > IIUC they never happen if task dies on its own (even if with error).
> > 
> > Also tentatively added InternalError here. It can happen if connectivity
with
> > the server is lost. I think before we just crashed with ValueError without
> doing
> > a proper cleanup.
> 
> - If the file is missing, subprocess.Popen(['missing_file'], ...) will raise
> OSError. This is not an internal error, it's a user error, the person forgot
to
> map the file.
I believe this is caught sooner (on line 434) when subprocess is starting (and
it is real error, not internal one).

> - For IOError, I don't recall exactly. It _could_ be file mode on POSIX or
> something else but I forgot to note why it could happen.
> - Both are now moot because now task_runner always starts [sys.executable,
> 'swarming_bot.N.zip', 'run_isolated', ...], which wasn't the case before when
> this code was first written. So now OSError *is* an internal error, as this
now
> means that swarming_bot.N.zip somehow disappeared.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:516:
must_signal_internal_failure = e.message or 'unknown error'
On 2016/08/31 15:03:47, M-A Ruel wrote:
> str(e.message) if e.message else 'unknown error'
> 
> I prefer to be defensive here

Done.

https://codereview.chromium.org/2300543002/diff/1/appengine/swarming/swarming...
appengine/swarming/swarming_bot/bot_code/task_runner.py:606:
must_signal_internal_failure = e.message or 'unknown error'
On 2016/08/31 15:03:47, M-A Ruel wrote:
> same

Done.

[Vadim Sh., 2016-09-06 21:36:15]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2016-09-06 21:36:16]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2300543002/#ps20001
(title: "Minor refactoring in task_runner.py in preparation for adding more
code.")

[commit-bot: I haz the power, 2016-09-06 21:36:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 21:39:14]

Description was changed from

==========
Minor refactoring in task_runner.py in preparation for adding more code.

In particular:
  * Move all auth related stuff into bot_auth.py. It will grow later.
    task_runner.py is already huge.
  * Move auth init/shutdown to load_and_run, run_command is too big.
  * Use InternalError exception instead of ValueError to signal errors.
  * Rename MustExit to ExitSignal to indicate that it is raised by signals.
  * Various nits in comments.

Also it makes more kinds of errors recognized as internal errors. For example,
IOError when reading from a task process pipe.

R=maruel@chromium.org
BUG=
==========

to

==========
Minor refactoring in task_runner.py in preparation for adding more code.

In particular:
  * Move all auth related stuff into bot_auth.py. It will grow later.
    task_runner.py is already huge.
  * Move auth init/shutdown to load_and_run, run_command is too big.
  * Use InternalError exception instead of ValueError to signal errors.
  * Rename MustExit to ExitSignal to indicate that it is raised by signals.
  * Various nits in comments.

Also it makes more kinds of errors recognized as internal errors. For example,
IOError when reading from a task process pipe.

R=maruel@chromium.org
BUG=

Committed:
https://github.com/luci/luci-py/commit/43a6d619519fc21eb427cdd25b6d912c4616ff6f
==========

[commit-bot: I haz the power, 2016-09-06 21:39:15]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-py/commit/43a6d619519fc21eb427cdd25b6d912c4616ff6f