Merge all swarm_*.py scripts into swarming.py.

Merge all swarm_*.py scripts into swarming.py.

Use subcommand.py to implement the subcommands.

R=vadimsh@chromium.org
BUG=

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

[Vadim Sh., 2013-08-16 18:58:58]

Mostly looks good except 'trigger_and_return' function.

Also I took a liberty to comment on some existing stuff.

https://codereview.chromium.org/22980008/diff/1/example/run_example_swarm.py
File example/run_example_swarm.py (right):

https://codereview.chromium.org/22980008/diff/1/example/run_example_swarm.py#...
example/run_example_swarm.py:97: 'results',
What do you think about renaming it to some verb? For example: collect, gather,
wait, join...

https://codereview.chromium.org/22980008/diff/1/swarming.py
File swarming.py (right):

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode24
swarming.py:24: from third_party.depot_tools import fix_encoding
nit: I'd rearrange this imports into two groups:

from third_party.depot_tools import fix_encoding
from third_party.depot_tools import subcommand

import trace_inputs
import run_isolated

Though there's probably some specific reason you arranged them that way.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode35
swarming.py:35: ISOLATE_SERVER = 'https://isolateserver-dev.appspot.com/'
At some point we need to get rid of this defaults somehow imho:
1. Humans can setup local environment once. Maybe some config file.
2. Bots can always pass this via cmd line.

What do you think?

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode200
swarming.py:200: if self._test_filter and self._test_filter != '*':
Actually that part looks more chromium specific (or rather gtest specific) than
the one in 'chromium_setup'. Just saying...

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode231
swarming.py:231: # build/scripts/slave/get_swarm_results.py stops passing it.
Is it still passing it? :)

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode331
swarming.py:331: Highly chromium specific.
Actually 'run_isolated.py' is not that chromium specific. It's more isolate
specific. And swarm_cleanup.py also looks generic enough.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode403
swarming.py:403: def trigger_and_return(
Is this function covered by any test?

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode438
swarming.py:438: 'swarm_trigger_step.py',
'swarm_trigger_step.py' is no more.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode455
swarming.py:455: 'swarm_get_results.py',
Same here.

Actually, maybe do it all in the same process?

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode474
swarming.py:474: '-u', '--url', default=SWARM_SERVER,
--url here, --swarm-url below :(

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode564
swarming.py:564: help='Desired working direction on the swarm slave side. '
'direction'?

Also why do we need this argument?

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode571
swarming.py:571: parser.add_option('-d', '--data-server',
At some point we need to converge on single term for
isolate-server/data-server/cas/cac/cad.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode621
swarming.py:621: trace_inputs.OptionParserWithLogging(prog='swarming.py'), args)
I'm really looking forward to a time when we'll have 'utils' package for stuff
like OptionParserWithLogging. Random imports from unrelated packages annoy me.

[M-A Ruel, 2013-08-18 00:18:18]

I think it's preferable to do one big change in the UI and argument names, so
please start the bike-shedding. I'm proposing new command and arguments names
but I'm highly open to suggestions.

The code is not in a fully working state yet but mostly there. I'll have to
commit an updated wrapper script in build/scripts/slave/swarming/ first anyway
and have the masters restarted, so it'll likely take a while before this CL can
be committed.

https://codereview.chromium.org/22980008/diff/1/example/run_example_swarm.py
File example/run_example_swarm.py (right):

https://codereview.chromium.org/22980008/diff/1/example/run_example_swarm.py#...
example/run_example_swarm.py:97: 'results',
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> What do you think about renaming it to some verb? For example: collect,
gather,
> wait, join...

Good idea, chose collect.

https://codereview.chromium.org/22980008/diff/1/swarming.py
File swarming.py (right):

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode24
swarming.py:24: from third_party.depot_tools import fix_encoding
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> nit: I'd rearrange this imports into two groups:
> 
> from third_party.depot_tools import fix_encoding
> from third_party.depot_tools import subcommand
> 
> import trace_inputs
> import run_isolated
> 
> Though there's probably some specific reason you arranged them that way.

Done.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode35
swarming.py:35: ISOLATE_SERVER = 'https://isolateserver-dev.appspot.com/'
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> At some point we need to get rid of this defaults somehow imho:
> 1. Humans can setup local environment once. Maybe some config file.
> 2. Bots can always pass this via cmd line.
> 
> What do you think?

Yes. But it's out of scope for this CL.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode200
swarming.py:200: if self._test_filter and self._test_filter != '*':
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> Actually that part looks more chromium specific (or rather gtest specific)
than
> the one in 'chromium_setup'. Just saying...

That's what the comment one line above at line 199 said. :)

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode231
swarming.py:231: # build/scripts/slave/get_swarm_results.py stops passing it.
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> Is it still passing it? :)

Done.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode331
swarming.py:331: Highly chromium specific.
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> Actually 'run_isolated.py' is not that chromium specific. It's more isolate
> specific. And swarm_cleanup.py also looks generic enough.

But still. We'll have to make this less hardcoded.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode403
swarming.py:403: def trigger_and_return(
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> Is this function covered by any test?

Nope. That's why we have a canary master.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode438
swarming.py:438: 'swarm_trigger_step.py',
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> 'swarm_trigger_step.py' is no more.

This code was not completed yet, as I hadn't sent a review request. Fixed.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode474
swarming.py:474: '-u', '--url', default=SWARM_SERVER,
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> --url here, --swarm-url below :(

Yep, being addressed.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode564
swarming.py:564: help='Desired working direction on the swarm slave side. '
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> 'direction'?

Copy-pasted code from the original script.

> Also why do we need this argument?

No idea, Chris?

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode571
swarming.py:571: parser.add_option('-d', '--data-server',
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> At some point we need to converge on single term for
> isolate-server/data-server/cas/cac/cad.

Doing now.

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode621
swarming.py:621: trace_inputs.OptionParserWithLogging(prog='swarming.py'), args)
On 2013/08/16 18:58:58, Vadim Sh. wrote:
> I'm really looking forward to a time when we'll have 'utils' package for stuff
> like OptionParserWithLogging. Random imports from unrelated packages annoy me.

Yes but that won't happen until run_isolated.py is packaged. So I'm really
looking forward this. :)

https://codereview.chromium.org/22980008/diff/6001/example/run_example_swarm.py
File example/run_example_swarm.py (left):

https://codereview.chromium.org/22980008/diff/6001/example/run_example_swarm....
example/run_example_swarm.py:23: ISOLATE_SERVER =
'https://isolateserver.appspot.com/'
One less hardcoded string. :)

https://codereview.chromium.org/22980008/diff/6001/swarming.py
File swarming.py (right):

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode477
swarming.py:477: '-c', '--cac',
I don't like --data_server. --isolate_server (like I still have left in
run_example_swarm.py) is longish and I'd like to rename the server, since it's
not that isolate-specific.

isolate.py's --outdir is just wrong but that's for another time.
run_isolated.py's --remote is weird.

Any suggestion? They should all be changed to the same name in any case. I like
--cac but it's not clear what it means and it's not obvious it's acronym.

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode485
swarming.py:485: '-o', '--os_image',
--os makes sense, but --os_image is weird.

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode581
swarming.py:581: '--task', nargs=4, action='append', default=[], dest='tasks',
I like --task contrary to --run_from_hash, especially that I changed it to
support specifying a .isolated file directly.

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode615
swarming.py:615: '-s', '--server', default=SWARM_SERVER,
--server is too generic.
--swarm_url, for an unknown reason, I dislike.
--url is bad.
--swarming is unclear.

Any suggestion?

https://codereview.chromium.org/22980008/diff/6001/tests/swarming_test.py
File tests/swarming_test.py (right):

https://codereview.chromium.org/22980008/diff/6001/tests/swarming_test.py#new...
tests/swarming_test.py:2: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
will revert this change on next patchset.

https://codereview.chromium.org/22980008/diff/6001/tests/swarming_test.py#new...
tests/swarming_test.py:25: FILE_HASH = u'1' * 40
The code now enforces this looks like a sha1.

[csharp, 2013-08-19 13:46:03]

lgtm

https://codereview.chromium.org/22980008/diff/1/swarming.py
File swarming.py (right):

https://codereview.chromium.org/22980008/diff/1/swarming.py#newcode564
swarming.py:564: help='Desired working direction on the swarm slave side. '
On 2013/08/18 00:18:18, M-A Ruel wrote:
> On 2013/08/16 18:58:58, Vadim Sh. wrote:
> > 'direction'?
> 
> Copy-pasted code from the original script.
> 
> > Also why do we need this argument?
> 
> No idea, Chris?

This lets swarm know what directory to download, unzip and run the code in.
Without this value swarm will fall back on the default of c:\swarm_tests, so we
need to keep this.

(This default assumes windows because the server sets the value right now and
there is no way for it to know the OS of the slave atm).

[csharp, 2013-08-19 13:47:42]

Oops, hadn't meant to include that LGTM, just meant to publish the comment.

[M-A Ruel, 2013-08-19 14:43:13]

On 2013/08/19 13:46:03, csharp wrote:
> On 2013/08/18 00:18:18, M-A Ruel wrote:
> > On 2013/08/16 18:58:58, Vadim Sh. wrote:
> > > Also why do we need this argument?
> > 
> > No idea, Chris?
> 
> This lets swarm know what directory to download, unzip and run the code in.
> Without this value swarm will fall back on the default of c:\swarm_tests, so
we
> need to keep this.
> 
> (This default assumes windows because the server sets the value right now and
> there is no way for it to know the OS of the slave atm).

It'd be nice if there was a sane relative default value. Also I think we
shouldn't allow relative directory.

The thing here is that this value encodes something important and subtle: The
inter-task survival of code and data.
It's important since this is what makes the local run_isolated.py cache work at
all. It is because the data is *not* deleted between the swarming tasks.

I think this fact could be 1) better documented and 2) better designed. In
particular, there should be a case of:
- keep the directory downloaded and use a constant name
- zap the whole thing after running the task.

So the 'constant name' would be what describes the fact that data needs to be
saved. And not using a 'working directory name' would result in the whole thing
being zapped after execution. I'm not 100% sure it's the best though.

At the moment, it's run_isolated.py and swarm_cleanup.py that have the burden of
cleaning up the temporary parts while keeping the cache intact. We need a better
story IMHO.

---

Also Chris, any opinion on the argument names? The more I think about it, the
more I like renaming isolateserver. We can keep isolateserver.appspot.com, I'm
concerned about the project's code name. CAC is an acronym for a type of design,
not an implementation, so I'm not 100% sure it's appropriate. Also it requires
some sort of glossary to have users understand what it is, while isolateserver
self-convey its purpose. But --isolateserver is too long, and --dataserver is
so-so IMHO but I could probably live with it. Also --server is not clear about
which server it is supposed to be given. --swarming ?

[csharp, 2013-08-19 15:07:57]

On 2013/08/19 14:43:13, M-A Ruel wrote:
> On 2013/08/19 13:46:03, csharp wrote:
> > On 2013/08/18 00:18:18, M-A Ruel wrote:
> > > On 2013/08/16 18:58:58, Vadim Sh. wrote:
> > > > Also why do we need this argument?
> > > 
> > > No idea, Chris?
> > 
> > This lets swarm know what directory to download, unzip and run the code in.
> > Without this value swarm will fall back on the default of c:\swarm_tests, so
> we
> > need to keep this.
> > 
> > (This default assumes windows because the server sets the value right now
and
> > there is no way for it to know the OS of the slave atm).
> 
> It'd be nice if there was a sane relative default value. Also I think we
> shouldn't allow relative directory.

The code actually has comments saying relative values shouldn't be allowed, but
that we can't check for it. (I don't know when that comment was added or the
reason we couldn't check).

> The thing here is that this value encodes something important and subtle: The
> inter-task survival of code and data.
> It's important since this is what makes the local run_isolated.py cache work
at
> all. It is because the data is *not* deleted between the swarming tasks.
> 
> I think this fact could be 1) better documented and 2) better designed. In
> particular, there should be a case of:
> - keep the directory downloaded and use a constant name
> - zap the whole thing after running the task.
>
> So the 'constant name' would be what describes the fact that data needs to be
> saved. And not using a 'working directory name' would result in the whole
thing
> being zapped after execution. I'm not 100% sure it's the best though.

I don't think swarm should have to know about constant vs. temporary data. Swarm
already has the ability to delete the working directory after the test runs (I
think I fixed it to work correctly a while ago), and the swarm code doesn't know
anything about the cache. The run_isolated.py script could move the cache to a
new location, and then swarm could delete the working directory after each run
without destroying the cache and without knowing about the cache.

Actually, looking at the manifest we send right now, we set cleanup=root, which
should completely clear the working directory, but it looks like it doesn't work
('zip' and 'data seem to work though). If the cache is moved out of the working
directory I can fix the cleanup root command and this should provide a cleaner
picture.

 
> At the moment, it's run_isolated.py and swarm_cleanup.py that have the burden
of
> cleaning up the temporary parts while keeping the cache intact. We need a
better
> story IMHO.
> 
> ---
> 
> Also Chris, any opinion on the argument names? The more I think about it, the
> more I like renaming isolateserver. We can keep
http://isolateserver.appspot.com, I'm
> concerned about the project's code name. CAC is an acronym for a type of
design,
> not an implementation, so I'm not 100% sure it's appropriate. Also it requires
> some sort of glossary to have users understand what it is, while isolateserver
> self-convey its purpose. But --isolateserver is too long, and --dataserver is
> so-so IMHO but I could probably live with it. Also --server is not clear about
> which server it is supposed to be given. --swarming ?

--swarming sounds reasonable to me. I think I'd go with --isolateserver, it is a
bit long, but that makes it very clear what it is, and since we use
isolate/isolated for several different things, I think trying to make it shorter
might just cause confusion.

[M-A Ruel, 2013-08-19 15:30:09]

On 2013/08/19 15:07:57, csharp wrote:
> On 2013/08/19 14:43:13, M-A Ruel wrote:
> > On 2013/08/19 13:46:03, csharp wrote:
> > > On 2013/08/18 00:18:18, M-A Ruel wrote:
> > > > On 2013/08/16 18:58:58, Vadim Sh. wrote:
> > > > > Also why do we need this argument?
> > > > 
> > > > No idea, Chris?
> > > 
> > > This lets swarm know what directory to download, unzip and run the code
in.
> > > Without this value swarm will fall back on the default of c:\swarm_tests,
so
> > we
> > > need to keep this.
> > > 
> > > (This default assumes windows because the server sets the value right now
> and
> > > there is no way for it to know the OS of the slave atm).
> > 
> > It'd be nice if there was a sane relative default value. Also I think we
> > shouldn't allow relative directory.
> 
> The code actually has comments saying relative values shouldn't be allowed,
but
> that we can't check for it. (I don't know when that comment was added or the
> reason we couldn't check).

Argh, I meant exactly the reverse. :( E.g. the task requester shouldn't have a
possibility to know where the swarm_bot code lives, it should run from an
arbitrary directory chosen by the bot.

Potential idea: swarming could have two modes; 1 where it *knows* it runs an
isolated file, the other where it's running an arbitrary zipped script;
basically the current design. The second mode would always zap, the first
wouldn't. Just an idea, not sure it's preferable.


> --swarming sounds reasonable to me. I think I'd go with --isolateserver, it is
a
> bit long, but that makes it very clear what it is, and since we use
> isolate/isolated for several different things, I think trying to make it
shorter
> might just cause confusion.

Fine with me too, cac is so-so after all. I will update the CL soonish with
these new values but I have a few other fixes to do too.

[Vadim Sh., 2013-08-19 18:55:43]

About data directory.
----

I agree with M-A: task requester should not know or care about path where task
is running. Though I can imagine a situation when actual task want's to know
it's absolute path (and getting it from getcwd() is not very explicit). Also I'd
like to ALWAYS delete working directory. But we still want to keep the cache. So
I propose the following design:

In manifest:

...
'tests': {{'action': 
  ['python', 'script.py', 
      '--working_dir', '%(working_dir)s',
      '--cache_dir', '%(cache_dir)s'],
  ...
}}

Then swarm bot substitutes:
* %(working_dir)s - clean directory where zip will be unpacked and test will be
run. Once test is finished, it's always deleted.
* %(cache_dir)s - a directory shared by all swarm tasks, it persists between
runs.


About command line arguments.
----

I agree on --swarming for swarm server. I don't like --isolateserver because
it's too long and two words but I don't see any good alternatives... Also I
really like dashes: isolate-server :)

Also I'd like to see one of the following:
1) add suffix '-url' and use URLs as arguments (as it is now): --swarming-url
https://chromium-swarm.appspot.com/ --isolate-url
https://isolateserver.appspot.com/
2) do not use '-url' and pass only names: --swarming chromium-swarm.appspot.com.
--isolate-server isolateserver.appspot.com. If name is localhost:<port> then use
http:// scheme, else https://.

With '-url' we are open for more alternatives for isolate server :) Like
'isolate-url'.

https://codereview.chromium.org/22980008/diff/6001/swarming.py
File swarming.py (right):

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode426
swarming.py:426: os_image,
That's one hell of an argument list.

Maybe put some of this stuff into utility object and pass it around?

Stuff than can go there: os_image, test_name_prefix, working_dir, cac,
swarm_url, verbose, profile, priority (so it can be passed as is to
process_manifest).

Alternatively 'trigger' and 'process_manifest' can be converted to a methods of
some 'SwarmTrigger' class that can hold all this details.

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode485
swarming.py:485: '-o', '--os_image',
On 2013/08/18 00:18:19, M-A Ruel wrote:
> --os makes sense, but --os_image is weird.
If it's 'one of valid sys.platform' maybe call it 'platform'? :)

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode489
swarming.py:489: '-T', '--test-name-prefix', default='',
I personally prefer dashes in argument names (like here). So I propose to change
everything else to use dashes as well. 

If it's not possible, then change --test-name-prefix to --test_name_prefix for
consistency.

[M-A Ruel, 2013-08-20 17:09:30]

I also changed --run_from_hash to --task.
I used --swarming and --isolate-server.

https://codereview.chromium.org/22980008/diff/6001/swarming.py
File swarming.py (right):

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode426
swarming.py:426: os_image,
On 2013/08/19 18:55:44, Vadim Sh. wrote:
> That's one hell of an argument list.
> 
> Maybe put some of this stuff into utility object and pass it around?
> 
> Stuff than can go there: os_image, test_name_prefix, working_dir, cac,
> swarm_url, verbose, profile, priority (so it can be passed as is to
> process_manifest).
> 
> Alternatively 'trigger' and 'process_manifest' can be converted to a methods
of
> some 'SwarmTrigger' class that can hold all this details.

The Manifest object will have to be redesigned, but I prefer to punt that on a
follow up CL with no user impacting change.

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode485
swarming.py:485: '-o', '--os_image',
On 2013/08/19 18:55:44, Vadim Sh. wrote:
> On 2013/08/18 00:18:19, M-A Ruel wrote:
> > --os makes sense, but --os_image is weird.
> If it's 'one of valid sys.platform' maybe call it 'platform'? :)

platform is more ambiguous than 'os'

https://codereview.chromium.org/22980008/diff/6001/swarming.py#newcode489
swarming.py:489: '-T', '--test-name-prefix', default='',
On 2013/08/19 18:55:44, Vadim Sh. wrote:
> I personally prefer dashes in argument names (like here). So I propose to
change
> everything else to use dashes as well. 
> 
> If it's not possible, then change --test-name-prefix to --test_name_prefix for
> consistency.

--task-prefix.

[Vadim Sh., 2013-08-20 20:47:31]

lgtm

[M-A Ruel, 2013-08-26 18:14:08]

Notes:
- I rebased on r219402 so I had to do minor fixups.
- I wrote https://codereview.chromium.org/23380003 because otherwise the
presubmit check wouldn't pass locally anymore.
- This CL should be committed right before
https://codereview.chromium.org/22909021 so the proper revision number can be
put in this CL. The delta between these 2 commits will break the swarm canary
master.
- I tested it locally (well before the rebase but I'm confident)

[Vadim Sh., 2013-08-26 21:33:06]

Any estimates when this lands?

I'm assuming you are waiting for lgtm on
https://codereview.chromium.org/22909021 ?

[M-A Ruel, 2013-08-27 01:35:22]

On 2013/08/26 21:33:06, Vadim Sh. wrote:
> Any estimates when this lands?
> 
> I'm assuming you are waiting for lgtm on
> https://codereview.chromium.org/22909021 ?

Yes, I asked Isaac to take a look tonight so I can commit this tomorrow morning.

[commit-bot: I haz the power, 2013-08-27 16:04:17]

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

[commit-bot: I haz the power, 2013-08-27 16:05:54]

Change committed as 219798