Add swarming_run_shim.py to run swarming tasks as annotated tasks.

Add swarming_run_shim.py to run swarming tasks as annotated tasks.

This removes relience on the server to preconfigure the steps to be run. Now the
slave decide what test to run.

get_swarm_results_shim.py and trigger_swarm_shim.py are not removed yet to not
break currently running masters. They will be removed in a later CL once masters
have been restarted.

Improve priority selection by using the master url as a signal. Downgrade
priority of unknown masters severely, so that the downgrade in priority is user
visible, so the maintainer can be a signal about misconfiguration quickly.

Remove tons of now dead code.

Add unit test.

Drop support for pre-2013-11-27 checkouts.

R=vadimsh@chromium.org,stip@chromium.org
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=247232

[M-A Ruel, 2014-01-24 03:19:00]

I took a few screenshots if you want to take a look. It's quite awesome.

I tested locally both the canary master and the try server. I tested the try
server with a test filter to confirm it is working. See
process_build_properties() for the logic.

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
File scripts/master/factory/swarm_commands.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
scripts/master/factory/swarm_commands.py:18: def
TestStepFilterTriggerSwarm(bStep):
This is still needed to know if swarm_triggered_tests should be triggered or
not.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
File scripts/slave/swarming/swarming_run_shim.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:321: options.verbose = True
Actually, this doesn't seem to work in my tests.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/t...
File scripts/slave/swarming/trigger_swarm_shim.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/t...
scripts/slave/swarming/trigger_swarm_shim.py:1: #!/usr/bin/env python
this file will be removed soon, no master after being restarted will use it.

[M-A Ruel, 2014-01-24 19:58:00]

Ping. I would have liked to enable this this weekend.

[Vadim Sh., 2014-01-24 20:22:11]

General idea and implementation looks good. My main concern is that script
always returns 0 exit code.

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
File scripts/master/factory/swarm_factory.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
scripts/master/factory/swarm_factory.py:53: def SwarmTestBuilder(swarm_server,
isolation_outdir, tests):
nit: maybe rename isolation_outdir to isolate_server? (as we do now on isolate
client side)

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
scripts/master/factory/swarm_factory.py:79: class
SwarmFactory(chromium_factory.ChromiumFactory):
nit: maybe rename it to SwarmCanaryFactory to better reflect its purpose?

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
scripts/master/factory/swarm_factory.py:93: target = 'Release'
hm... how are we going to canary-test component builds done in debug mode?

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
File scripts/slave/swarming/swarming_run_shim.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:107: if i == '':
nit: if not i ?

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:116: yield p.returncode
nit: add assert p.returncode is not None just to be sure

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:204: return 0
Hm... This entire script always finishes with zero status, even if some steps
fail. It doesn't look right to me. Can we do something like:

return collect(...) 

and in collect return != 0 if any test failed?

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:210: threads = [t for t in threads
if t.is_alive()]
I don't trust 't.is_alive()'...

What do you think about:

def collect(stream, steps_annotations, out):
  while steps_annotations:
    try:
      package = out.get(timeout=10) # no need to poll each 100 ms...
    except Queue.Empty:
      continue
   ...

You delete from |step_annotations| now anyway whenever task finishes.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:242: out.task_done()
As I proposed above: return non 0 from 'collect' if at least one step fails.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:246: """Returns a dict of test:hash
for the test that should be run thru Swarming.
nit: typo 'thru'

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:296: if
os.path.isfile(os.path.join(client, 'swarm_get_results.py')):
Is this check necessary? Doesn't version check below covers this case as well?

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:307: parser.add_option('--verbose',
action='store_true', default=True)
err... default=True and action='store_true'. IIUC there's no way to make it
non-verbose.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:319: if
(options.build_properties.get('buildbotURL') ==
maybe .endswith('/chromium.swarm/') (the same way priority checking function
does). Or even move this into swarming_utils: 

if swarming_utils.is_on_canary_master(options.build_properties):
 ...

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:334: return 0
why not fail?

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
File scripts/slave/swarming/swarming_utils.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_utils.py:102: return 10
nit: add comment somewhere that says that tasks with lower priority number are
more likely to run. It's more like niceness in linux, rather than priority.

[M-A Ruel, 2014-01-24 20:41:41]

I took a few screenshots, I'll mail it to you.

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
File scripts/master/factory/swarm_factory.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
scripts/master/factory/swarm_factory.py:53: def SwarmTestBuilder(swarm_server,
isolation_outdir, tests):
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> nit: maybe rename isolation_outdir to isolate_server? (as we do now on isolate
> client side)

Done.

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
scripts/master/factory/swarm_factory.py:79: class
SwarmFactory(chromium_factory.ChromiumFactory):
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> nit: maybe rename it to SwarmCanaryFactory to better reflect its purpose?

I'm still not sure if I'm not going to revert back to BuilderTester on the TS so
I prefer to not do it now.

https://codereview.chromium.org/139343011/diff/50001/scripts/master/factory/s...
scripts/master/factory/swarm_factory.py:93: target = 'Release'
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> hm... how are we going to canary-test component builds done in debug mode?

via a new function argument, in time.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
File scripts/slave/swarming/swarming_run_shim.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:107: if i == '':
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> nit: if not i ?

Done.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:116: yield p.returncode
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> nit: add assert p.returncode is not None just to be sure

It can't be. But I refactored to make this clearer.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:204: return 0
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> Hm... This entire script always finishes with zero status, even if some steps
> fail. It doesn't look right to me. Can we do something like:
> 
> return collect(...) 
> 
> and in collect return != 0 if any test failed?

Each step will have its own result code.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:210: threads = [t for t in threads
if t.is_alive()]
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> I don't trust 't.is_alive()'...
> 
> What do you think about:
> 
> def collect(stream, steps_annotations, out):
>   while steps_annotations:
>     try:
>       package = out.get(timeout=10) # no need to poll each 100 ms...
>     except Queue.Empty:
>       continue
>    ...
> 
> You delete from |step_annotations| now anyway whenever task finishes.

Done.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:242: out.task_done()
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> As I proposed above: return non 0 from 'collect' if at least one step fails.

That's not how annotated steps work. And this would be very confusing for the
users.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:246: """Returns a dict of test:hash
for the test that should be run thru Swarming.
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> nit: typo 'thru'

Done.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:296: if
os.path.isfile(os.path.join(client, 'swarm_get_results.py')):
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> Is this check necessary? Doesn't version check below covers this case as well?

Confirmed that (None < (0, 1)) == True so removed.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:307: parser.add_option('--verbose',
action='store_true', default=True)
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> err... default=True and action='store_true'. IIUC there's no way to make it
> non-verbose.

Oops, removed. I had added it during testing.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:321: options.verbose = True
On 2014/01/24 03:19:00, M-A Ruel wrote:
> Actually, this doesn't seem to work in my tests.

No it doesn't, I removed the code.

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_run_shim.py:334: return 0
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> why not fail?

I wanted to put a warning. In practice it could be fine to fail. But for some
reason 88 is not recognized as a warning.

Mike, do you know why?

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
File scripts/slave/swarming/swarming_utils.py (right):

https://codereview.chromium.org/139343011/diff/50001/scripts/slave/swarming/s...
scripts/slave/swarming/swarming_utils.py:102: return 10
On 2014/01/24 20:22:11, Vadim Sh. wrote:
> nit: add comment somewhere that says that tasks with lower priority number are
> more likely to run. It's more like niceness in linux, rather than priority.

See line 80. :)

[ghost stip (do not use), 2014-01-24 22:47:25]

annotator comments:

there are two things: one is to emit step_warnings() within the main step. note
that you come back to the main step every time you emit a step_closed before you
seed_step. so your final emission after step_closed can be step_warnings to make
the main step turn yellow

two is how you want buildbot to handle things. if you want *any* failure to mean
buildbot takes the step as a failure, do nothing. otherwise take a look at
honor_return_code.
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/maste...
has info, look at line 1023 and self.annotate_status.

basically: normally buildbot would interpret any section failing as the entire
step failing; using honor_return_code means that a 0 would mean success. you'd
still need to signal a warning through annotations as above.

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/master/f...
File scripts/master/factory/swarm_commands.py (left):

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/master/f...
scripts/master/factory/swarm_commands.py:29: def
TestStepFilterRetrieveSwarmResult(bStep):
I like this CL already

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/master/f...
File scripts/master/factory/swarm_commands.py (right):

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/master/f...
scripts/master/factory/swarm_commands.py:93: chromium_step.AnnotatedCommand,
yes yes yes yes

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
File scripts/slave/swarming/swarming_run_shim.py (right):

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
scripts/slave/swarming/swarming_run_shim.py:167: stream =
annotator.AdvancedAnnotationStream(sys.stdout, False)
stream.honor_zero_return_code() if you want the return code of the process to
matter

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
scripts/slave/swarming/swarming_run_shim.py:197: stream.step_cursor(step_name)
steps_annotations[task_name].step_started()

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
scripts/slave/swarming/swarming_run_shim.py:215: if last_cursor != task_name:
might want to take lines 215-217, make it into a small function and reuse
throughout this

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
scripts/slave/swarming/swarming_run_shim.py:315: # normal. Not sure how to do
this.
annotator.AdvancedAnnotationStep(sys.stdout, False).step_warnings()

[M-A Ruel, 2014-01-25 02:12:19]

On 2014/01/24 22:47:25, stip wrote:
> there are two things: one is to emit step_warnings() within the main step.
note
> that you come back to the main step every time you emit a step_closed before
you
> seed_step. so your final emission after step_closed can be step_warnings to
make
> the main step turn yellow

Ok, done, but I'll have to test it to ensure it still works.

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
File scripts/slave/swarming/swarming_run_shim.py (right):

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
scripts/slave/swarming/swarming_run_shim.py:197: stream.step_cursor(step_name)
On 2014/01/24 22:47:26, stip wrote:
> steps_annotations[task_name].step_started()

It's not really necessary.

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
scripts/slave/swarming/swarming_run_shim.py:215: if last_cursor != task_name:
On 2014/01/24 22:47:26, stip wrote:
> might want to take lines 215-217, make it into a small function and reuse
> throughout this

Cleaned up.

https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
scripts/slave/swarming/swarming_run_shim.py:315: # normal. Not sure how to do
this.
On 2014/01/24 22:47:26, stip wrote:
> annotator.AdvancedAnnotationStep(sys.stdout, False).step_warnings()

From reading annotator.py, I don't think this works.

[ghost stip (do not use), 2014-01-25 02:24:22]

On 2014/01/25 02:12:19, M-A Ruel wrote:
> On 2014/01/24 22:47:25, stip wrote:
> > there are two things: one is to emit step_warnings() within the main step.
> note
> > that you come back to the main step every time you emit a step_closed before
> you
> > seed_step. so your final emission after step_closed can be step_warnings to
> make
> > the main step turn yellow
> 
> Ok, done, but I'll have to test it to ensure it still works.
> 
>
https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
> File scripts/slave/swarming/swarming_run_shim.py (right):
> 
>
https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
> scripts/slave/swarming/swarming_run_shim.py:197: stream.step_cursor(step_name)
> On 2014/01/24 22:47:26, stip wrote:
> > steps_annotations[task_name].step_started()
> 
> It's not really necessary.

you're emitting text directly in, which starts the step automatically. bad form,
but it'll work

> 
>
https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
> scripts/slave/swarming/swarming_run_shim.py:215: if last_cursor != task_name:
> On 2014/01/24 22:47:26, stip wrote:
> > might want to take lines 215-217, make it into a small function and reuse
> > throughout this
> 
> Cleaned up.
> 
>
https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
> scripts/slave/swarming/swarming_run_shim.py:315: # normal. Not sure how to do
> this.
> On 2014/01/24 22:47:26, stip wrote:
> > annotator.AdvancedAnnotationStep(sys.stdout, False).step_warnings()
> 
> From reading annotator.py, I don't think this works.

totally does:

build(master)> scripts/tools/runit.py python
Python 2.7.3 (default, Sep 26 2013, 20:03:06) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> from common import annotator
>>> annotator.AdvancedAnnotationStep(sys.stdout, False).step_warnings()

@@@STEP_WARNINGS@@@

[M-A Ruel, 2014-01-25 02:43:50]

On 2014/01/25 02:24:22, stip wrote:
> > scripts/slave/swarming/swarming_run_shim.py:197:
stream.step_cursor(step_name)
> > On 2014/01/24 22:47:26, stip wrote:
> > > steps_annotations[task_name].step_started()
> > 
> > It's not really necessary.
> 
> you're emitting text directly in, which starts the step automatically. bad
form,
> but it'll work

OK. It works. :)

I just don't understand the reason for step_started, since it's effectively a
noop.


>
https://chromiumcodereview.appspot.com/139343011/diff/140001/scripts/slave/sw...
> > scripts/slave/swarming/swarming_run_shim.py:315: # normal. Not sure how to
do
> > this.
> > On 2014/01/24 22:47:26, stip wrote:
> > > annotator.AdvancedAnnotationStep(sys.stdout, False).step_warnings()
> > 
> > From reading annotator.py, I don't think this works.
> 
> totally does:
> 
> build(master)> scripts/tools/runit.py python
> Python 2.7.3 (default, Sep 26 2013, 20:03:06) 
> [GCC 4.6.3] on linux2
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import sys
> >>> from common import annotator
> >>> annotator.AdvancedAnnotationStep(sys.stdout, False).step_warnings()
> 
> @@@STEP_WARNINGS@@@

Oh it was a stale comment.

Anything else?

[Vadim Sh., 2014-01-25 02:59:33]

lgtm

[commit-bot: I haz the power, 2014-01-27 13:42:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/139343011/220001

[commit-bot: I haz the power, 2014-01-27 13:43:29]

Change committed as 247232

[M-A Ruel, 2014-01-27 14:20:16]

On 2014/01/27 13:43:29, I haz the power (commit-bot) wrote:
> Change committed as 247232

Humm, it generated an exception on Windows but not on the other platforms;
http://build.chromium.org/p/chromium.swarm/builders/Windows%20Swarm%20Tests/b...
http://build.chromium.org/p/chromium.swarm/builders/Windows%20Swarm%20Tests/b...
413  def addEntry(self, channel, text):
414    assert not self.finished

so it looks like it thinks the log is closed and then a line is added. I fear it
could be as silly as CRLF issue. Mike, any idea?

Note that on the TS, all the swarm_triggered slaves are running linux, so they
won't get this issue if it is Windows specific.

[Nico, 2014-01-29 17:04:17]

This test fails for me:

======================================================================
FAIL: test_default (__main__.SwarmingRunTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/slave/unittests/swarming_run_shim_test.py", line 167, in
test_default
    self.assertEqual(expected, sys.stdout.getvalue())
AssertionError: 'Selected tests:\n base_test\nSelected OS: Linux\n\n@@@SEED_STEP
base_test@@@\n\n@@@STEP_CURSOR
base_test@@@\n\n@@@STEP_STARTED@@@\n\n@@@STEP_TEXT@Linux@@@\n\n@@@STEP_TEXT@1234@@@\na\n\n@@@STEP_CLOSED@@@\n\n'
!= u'Selected tests:\n base_test\nSelected OS: Mac\n\n@@@SEED_STEP
base_test@@@\n\n@@@STEP_CURSOR
base_test@@@\n\n@@@STEP_STARTED@@@\n\n@@@STEP_TEXT@Mac@@@\n\n@@@STEP_TEXT@1234@@@\na\n\n@@@STEP_CLOSED@@@\n\n'


(Note expected "Linux" got "Mac".)

[Nico, 2014-01-29 17:30:59]

…whoa, there's even a comment that says

        # This test only passes on linux.

Please don't check in tests that don't pass :-( Can you fix this soon
please?


On Wed, Jan 29, 2014 at 9:04 AM, <thakis@chromium.org> wrote:

> This test fails for me:
>
> ======================================================================
> FAIL: test_default (__main__.SwarmingRunTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "scripts/slave/unittests/swarming_run_shim_test.py", line 167, in
> test_default
>     self.assertEqual(expected, sys.stdout.getvalue())
> AssertionError: 'Selected tests:\n base_test\nSelected OS: Linux\n\n@
> @@SEED_STEP
> base_test@@@\n\n@@@STEP_CURSOR
> base_test@@@\n\n@@@STEP_STARTED@@@\n\n@@@STEP_TEXT@Linux@@@\n\n@
> @@STEP_TEXT@1234@@@\na\n\n@@@STEP_CLOSED@@@\n\n'
> != u'Selected tests:\n base_test\nSelected OS: Mac\n\n@@@SEED_STEP
> base_test@@@\n\n@@@STEP_CURSOR
> base_test@@@\n\n@@@STEP_STARTED@@@\n\n@@@STEP_TEXT@Mac@@@\n\n@
> @@STEP_TEXT@1234@@@\na\n\n@@@STEP_CLOSED@@@\n\n'
>
>
> (Note expected "Linux" got "Mac".)
>
> https://codereview.chromium.org/139343011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.