Create proper wrapper scripts to decouple from swarm_client.

Create proper wrapper scripts to decouple from swarm_client.

The wrapper script determines the swarm_client code and pass the supported flags.

R=ilevy@chromium.org
BUG=

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

[M-A Ruel, 2013-08-14 20:06:49]

The problem with this is that it'll break for older revisions that do not have
support for --priority.

An option is to only use it for try jobs that are using swarming_client (vs
swarm_client)

[Isaac (away), 2013-08-14 20:23:18]

https://codereview.chromium.org/23176003/diff/1/scripts/master/factory/swarm_...
File scripts/master/factory/swarm_commands.py (right):

https://codereview.chromium.org/23176003/diff/1/scripts/master/factory/swarm_...
scripts/master/factory/swarm_commands.py:107: command.extend(('--priority',
str(priority)))
Is there a way to move this logic to a slaves script?  It looks OK but we might
want to tweak it without needed a ts restart.

I'm going to pull in robbie to get a second opinion.

[M-A Ruel, 2013-08-14 20:24:59]

On 2013/08/14 20:23:18, Isaac wrote:
> Is there a way to move this logic to a slaves script?  It looks OK but we
might
> want to tweak it without needed a ts restart.

Actually, I could create a wrapper script in build/scripts/slave. Not a bad
idea. Will look at it.

[M-A Ruel, 2013-08-15 17:47:55]

Still untested but I'd like your opinion on this layout.

[Isaac (away), 2013-08-15 23:27:01]

Yeah this looks better.  Thanks.

https://chromiumcodereview.appspot.com/23176003/diff/10001/scripts/master/fac...
File scripts/master/factory/swarm_commands.py (right):

https://chromiumcodereview.appspot.com/23176003/diff/10001/scripts/master/fac...
scripts/master/factory/swarm_commands.py:93: tasktype = 'ci'
what's ci?  Maybe use more descriptive names?

https://chromiumcodereview.appspot.com/23176003/diff/10001/scripts/slave/trig...
File scripts/slave/trigger_swarm.py (right):

https://chromiumcodereview.appspot.com/23176003/diff/10001/scripts/slave/trig...
scripts/slave/trigger_swarm.py:29: src_swarm_client = os.path.join(os.getcwd(),
'src', 'tools', 'swarm_client')
Ideally we wouldn't be dependent on cwd (i.e. instead passing via a
build_properties or factory_properties param).  If this is too difficult, let's
document this in top level comment.

https://chromiumcodereview.appspot.com/23176003/diff/10001/scripts/slave/trig...
scripts/slave/trigger_swarm.py:58: '-o', os,
let's use long option names here if they are available.

[M-A Ruel, 2013-08-20 16:23:06]

Changed CL description since I changed the purpose of the CL.
Addressed comments.
If you prefer, I can split it up in smaller parts, I don't mind.

[Isaac (away), 2013-08-20 22:22:32]

https://codereview.chromium.org/23176003/diff/24001/scripts/master/factory/ch...
File scripts/master/factory/chromium_commands.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/master/factory/ch...
scripts/master/factory/chromium_commands.py:1687: 'requester',
I just added this property in crrev.com/218418 -- do you still need this?

https://codereview.chromium.org/23176003/diff/24001/scripts/master/factory/sw...
File scripts/master/factory/swarm_commands.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/master/factory/sw...
scripts/master/factory/swarm_commands.py:134: swarm_request_name_prefix =
WithProperties(
While this is OK, the preferred style is probably to pass the entire
build_properties map and let the slave script parse and decide what it needs.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
File scripts/slave/swarming/get_swarm_results.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:83: def v0(client, options,
test_name):
v0?  Can you add a comment for what this does?

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:84: sys.path.insert(0, client)
This will repeatedly extend sys.path with each call.

Guard with "if sys.path[0] != client" ?

Also: can you verify get_swarm_results is reexeced with each build (i.e. that
this import won't get cached)?

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:94: options.shards =
int(options.shards)
not needed if you add type='int' to parser below.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:152: parser.add_option('-s',
'--shards', help='Number of shards')
type='int'?

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/tr...
File scripts/slave/swarming/trigger_swarm.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/tr...
scripts/slave/swarming/trigger_swarm.py:1: #!/usr/bin/env python
I'm confused what the purpose of this file is -- can you explain?

[M-A Ruel, 2013-08-23 02:18:32]

I did a complete rewrite. It was a good idea to pass build properties, thanks.
It'll make future changes much simpler.

Then I tested it on a local try server with a local linux_rel and a local
swarm_triggered.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
File scripts/slave/swarming/get_swarm_results.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:83: def v0(client, options,
test_name):
On 2013/08/20 22:22:33, Isaac wrote:
> v0?  Can you add a comment for what this does?

There will be multiple version handler, this is what handles all the older
versions of the client code.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:84: sys.path.insert(0, client)
On 2013/08/20 22:22:33, Isaac wrote:
> This will repeatedly extend sys.path with each call.
> 
> Guard with "if sys.path[0] != client" ?

This function is only called once indirectly by main(). This file is not a
library.

> Also: can you verify get_swarm_results is reexeced with each build (i.e. that
> this import won't get cached)?

I don't understand what you mean.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:94: options.shards =
int(options.shards)
On 2013/08/20 22:22:33, Isaac wrote:
> not needed if you add type='int' to parser below.

Done.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:152: parser.add_option('-s',
'--shards', help='Number of shards')
On 2013/08/20 22:22:33, Isaac wrote:
> type='int'?

Done.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/tr...
File scripts/slave/swarming/trigger_swarm.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/tr...
scripts/slave/swarming/trigger_swarm.py:1: #!/usr/bin/env python
On 2013/08/20 22:22:33, Isaac wrote:
> I'm confused what the purpose of this file is -- can you explain?

It triggers a swarm job. Then get_swarm_results.py gets the results.

The masters currently directly calls into swarm_client, which is a pain when we
want to update the code. I've explained it in the README file.

[M-A Ruel, 2013-08-25 12:29:34]

Mike, want to take a look? Isaac is overloaded these days.

[Isaac (away), 2013-08-26 02:35:46]

lgtm w/ nits.

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
File scripts/slave/swarming/get_swarm_results.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:84: sys.path.insert(0, client)
On 2013/08/23 02:18:33, M-A Ruel wrote:
> On 2013/08/20 22:22:33, Isaac wrote:
> > This will repeatedly extend sys.path with each call.
> > 
> > Guard with "if sys.path[0] != client" ?
> 
> This function is only called once indirectly by main(). This file is not a
> library.
> 
> > Also: can you verify get_swarm_results is reexeced with each build (i.e.
that
> > this import won't get cached)?
> 
> I don't understand what you mean.

I mean, can you verify that if you change swarm_get_results, the new version
will be used for subsequent builds, even though we're importing here.

https://codereview.chromium.org/23176003/diff/37001/scripts/master/factory/sw...
File scripts/master/factory/swarm_commands.py (right):

https://codereview.chromium.org/23176003/diff/37001/scripts/master/factory/sw...
scripts/master/factory/swarm_commands.py:129: command =
self.AddBuildProperties(command)
Maybe add factory_properties too, in case they are later needed.

https://codereview.chromium.org/23176003/diff/37001/scripts/slave/swarming/ge...
File scripts/slave/swarming/get_swarm_results.py (right):

https://codereview.chromium.org/23176003/diff/37001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:90: import swarm_get_results  #
pylint: disable=F0401
File names are a little confusing.  Would these be appropriate?:
get_swarm_results -> swarm_results_shim
trigger_swarm - trigger_swarm_shim

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

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
File scripts/slave/swarming/get_swarm_results.py (right):

https://codereview.chromium.org/23176003/diff/24001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:84: sys.path.insert(0, client)
On 2013/08/26 02:35:46, Isaac wrote:
> I mean, can you verify that if you change swarm_get_results, the new version
> will be used for subsequent builds, even though we're importing here.

My next CL is to delete swarm_get_results;
https://codereview.chromium.org/22980008 so this ensures it'll be a non-issue.
We don't want to do importing anymore, from now on it will be a
subprocess.call().

https://codereview.chromium.org/23176003/diff/37001/scripts/master/factory/sw...
File scripts/master/factory/swarm_commands.py (right):

https://codereview.chromium.org/23176003/diff/37001/scripts/master/factory/sw...
scripts/master/factory/swarm_commands.py:129: command =
self.AddBuildProperties(command)
On 2013/08/26 02:35:46, Isaac wrote:
> Maybe add factory_properties too, in case they are later needed.

factory_properties is not yet passed to these functions. I'll add it in a follow
up CL when it's needed since this CL is already complex.

https://codereview.chromium.org/23176003/diff/37001/scripts/slave/swarming/ge...
File scripts/slave/swarming/get_swarm_results.py (right):

https://codereview.chromium.org/23176003/diff/37001/scripts/slave/swarming/ge...
scripts/slave/swarming/get_swarm_results.py:90: import swarm_get_results  #
pylint: disable=F0401
On 2013/08/26 02:35:46, Isaac wrote:
> File names are a little confusing.  Would these be appropriate?:
> get_swarm_results -> swarm_results_shim
> trigger_swarm - trigger_swarm_shim

Good idea, done.

[commit-bot: I haz the power, 2013-08-26 14:21:35]

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

[commit-bot: I haz the power, 2013-08-26 14:22:40]

Change committed as 219544