swarming: change meaning of inputs_ref

swarming: change meaning of inputs_ref

Currently inputs_ref implicitly specifies isolated server and namespace
for isolated outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Change the meaning of inputs_ref from "a reference to input file tree"
to "isolate server/namespace and hash of input file tree".
Now (inputs_ref is not None) does not mean there is isolated input,
but (inputs_ref and inputs_ref.isolated) does.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

Add a TODO to refactor the code and update datastore schema.

Update tests.

R=maruel@chromium.org
BUG=601022
Committed: https://github.com/luci/luci-py/commit/abdaf29b948fa7467b5f1fdaed3bfbc41a692145

[nodir, 2016-05-02 23:07:55]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[nodir, 2016-05-02 23:08:00]

PTAL

[commit-bot: I haz the power, 2016-05-02 23:08:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939343002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939343002/20001

[commit-bot: I haz the power, 2016-05-02 23:11:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-02 23:11:07]

Dry run: This issue passed the CQ dry run.

[nodir, 2016-05-03 04:23:36]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 04:23:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939343002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939343002/80001

[commit-bot: I haz the power, 2016-05-03 04:26:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-03 04:26:33]

Dry run: This issue passed the CQ dry run.

[M-A Ruel, 2016-05-03 13:55:42]

Description was changed from

==========
swarming: add outputs_target to task properties

Currently inptus_ref implicitly specifies isolated server and namespace
for outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Add (isolatedserver, namespace) tuple as outputs_target property to
TaskProperties in ndb and rpc. It is mutually exclusive with inputs_ref.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

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

to

==========
swarming: add outputs_target to task properties

Currently inputs_ref implicitly specifies isolated server and namespace
for outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Add (isolatedserver, namespace) tuple as outputs_target property to
TaskProperties in ndb and rpc. It is mutually exclusive with inputs_ref.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

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

[M-A Ruel, 2016-05-03 14:01:25]

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:304: outputs_target =
ndb.LocalStructuredProperty(IsolatedOutputsTarget)
Argh, in retrospect maybe I should have used instead:

input_isolated = String
isolated_server = <server, metadata e.g. namespacE>
output_isolated = String

Do you think it's worth migrating over time the entities to this model?

[nodir, 2016-05-03 16:29:31]

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:304: outputs_target =
ndb.LocalStructuredProperty(IsolatedOutputsTarget)
On 2016/05/03 14:01:23, M-A Ruel wrote:
> Argh, in retrospect maybe I should have used instead:
> 
> input_isolated = String
> isolated_server = <server, metadata e.g. namespacE>
> output_isolated = String
> 
> Do you think it's worth migrating over time the entities to this model?

No

[nodir, 2016-05-03 21:43:49]

ping

[M-A Ruel, 2016-05-05 11:52:10]

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:304: outputs_target =
ndb.LocalStructuredProperty(IsolatedOutputsTarget)
On 2016/05/03 16:29:30, nodir wrote:
> On 2016/05/03 14:01:23, M-A Ruel wrote:
> > Argh, in retrospect maybe I should have used instead:
> > 
> > input_isolated = String
> > isolated_server = <server, metadata e.g. namespacE>
> > output_isolated = String
> > 
> > Do you think it's worth migrating over time the entities to this model?
> 
> No

Ok I thought about this a lot. I think it's a bad idea to add this. I think we
should converge to what I proposed above but this can be done in two steps so
you don't have to get bogged down with this.

So I'd recommend to unconditionally set inputs_ref.isolatedserver and namespace.
This requires updating the client code a bit but that's much simpler overall and
more future proof, as we can then refactor as I listed above without having to
have 3 different possible configurations.

At the top of this class I'd add a comment:

TODO(maruel):
- Convert inputs_ref and _TaskResultCommon.outputs_ref as:
  - input = String which is the isolated input, if any
  - isolated_server = <server, metadata e.g. namespace> which is a simplified
version of FilesRef
  - _TaskResultCommon.output = String which is isolated output, if any.

Don't worry I'm not asking you to do that, just add the TODO.

[nodir, 2016-05-10 18:03:05]

Description was changed from

==========
swarming: add outputs_target to task properties

Currently inputs_ref implicitly specifies isolated server and namespace
for outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Add (isolatedserver, namespace) tuple as outputs_target property to
TaskProperties in ndb and rpc. It is mutually exclusive with inputs_ref.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

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

to

==========
swarming: change meaning of inputs_ref

Currently inputs_ref implicitly specifies isolated server and namespace
for outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Change the meaning of inputs_ref from "a reference to input file tree" to
"isolate server/namespace and hash of input file tree". Now if (inputs_ref is
not None) does not mean there is isolated input, but (inputs_ref and
inputs_ref.isolated) does.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

Add a TODO to refactor the code and update schema.

Update tests.

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

[nodir, 2016-05-10 18:04:15]

Description was changed from

==========
swarming: change meaning of inputs_ref

Currently inputs_ref implicitly specifies isolated server and namespace
for outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Change the meaning of inputs_ref from "a reference to input file tree" to
"isolate server/namespace and hash of input file tree". Now if (inputs_ref is
not None) does not mean there is isolated input, but (inputs_ref and
inputs_ref.isolated) does.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

Add a TODO to refactor the code and update schema.

Update tests.

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

to

==========
swarming: change meaning of inputs_ref

Currently inputs_ref implicitly specifies isolated server and namespace
for isolated outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Change the meaning of inputs_ref from "a reference to input file tree"
to "isolate server/namespace and hash of input file tree".
Now (inputs_ref is not None) does not mean there is isolated input,
but (inputs_ref and inputs_ref.isolated) does.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

Add a TODO to refactor the code and update datastore schema.

Update tests.

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

[nodir, 2016-05-10 18:17:09]

Patchset #9 (id:160001) has been deleted

[nodir, 2016-05-10 18:17:14]

Patchset #8 (id:140001) has been deleted

[nodir, 2016-05-10 18:17:40]

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/1939343002/diff/100001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:304: outputs_target =
ndb.LocalStructuredProperty(IsolatedOutputsTarget)
On 2016/05/05 11:52:10, M-A Ruel wrote:
> On 2016/05/03 16:29:30, nodir wrote:
> > On 2016/05/03 14:01:23, M-A Ruel wrote:
> > > Argh, in retrospect maybe I should have used instead:
> > > 
> > > input_isolated = String
> > > isolated_server = <server, metadata e.g. namespacE>
> > > output_isolated = String
> > > 
> > > Do you think it's worth migrating over time the entities to this model?
> > 
> > No
> 
> Ok I thought about this a lot. I think it's a bad idea to add this. I think we
> should converge to what I proposed above but this can be done in two steps so
> you don't have to get bogged down with this.
> 
> So I'd recommend to unconditionally set inputs_ref.isolatedserver and
namespace.
> This requires updating the client code a bit but that's much simpler overall
and
> more future proof, as we can then refactor as I listed above without having to
> have 3 different possible configurations.
> 
> At the top of this class I'd add a comment:
> 
> TODO(maruel):
> - Convert inputs_ref and _TaskResultCommon.outputs_ref as:
>   - input = String which is the isolated input, if any
>   - isolated_server = <server, metadata e.g. namespace> which is a simplified
> version of FilesRef
>   - _TaskResultCommon.output = String which is isolated output, if any.
> 
> Don't worry I'm not asking you to do that, just add the TODO. 

Done, updated CL description.

[M-A Ruel, 2016-05-11 14:41:41]

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:227: # Specifies FilesRef mode.
This is not needed?

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:244: if self.is_ref:
Remove this part, self.isolated can be None.

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:282: behavior is desired, the member
.inputs_ref.isolated must be supplied.
Strictly speaking, it could still be missing when there's no default config.

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:310: # Only inputs_ref.isolated or
command&data can be specified.
data doesn't exist anymore, it's a left over so remove the reference

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/task_runner.py:90: if
task_details.isolated:  # Almost certainly True.
I don't think the command is useful.

[nodir, 2016-05-11 16:04:15]

Patchset #11 (id:240001) has been deleted

[nodir, 2016-05-11 16:04:22]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-11 16:04:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939343002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939343002/260001

[nodir, 2016-05-11 16:04:29]

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:227: # Specifies FilesRef mode.
On 2016/05/11 14:41:41, M-A Ruel wrote:
> This is not needed?

Done.

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:244: if self.is_ref:
On 2016/05/11 14:41:41, M-A Ruel wrote:
> Remove this part, self.isolated can be None.

Done. Now is_ref is not used, so I've removed it.

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:282: behavior is desired, the member
.inputs_ref.isolated must be supplied.
On 2016/05/11 14:41:41, M-A Ruel wrote:
> Strictly speaking, it could still be missing when there's no default config.

Done.

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/ser...
appengine/swarming/server/task_request.py:310: # Only inputs_ref.isolated or
command&data can be specified.
On 2016/05/11 14:41:41, M-A Ruel wrote:
> data doesn't exist anymore, it's a left over so remove the reference

Done.

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/swa...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/1939343002/diff/200001/appengine/swarming/swa...
appengine/swarming/swarming_bot/bot_code/task_runner.py:90: if
task_details.isolated:  # Almost certainly True.
On 2016/05/11 14:41:41, M-A Ruel wrote:
> I don't think the command is useful.

Done.

[commit-bot: I haz the power, 2016-05-11 16:08:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-11 16:08:20]

Dry run: This issue passed the CQ dry run.

[M-A Ruel, 2016-05-12 13:47:03]

2 small fixes needed before commit:

- templates/user_task.html: lines 222 and 495
and the comment below.

then lgtm, thanks for bearing with me.

https://codereview.chromium.org/1939343002/diff/260001/client/swarming.py
File client/swarming.py (right):

https://codereview.chromium.org/1939343002/diff/260001/client/swarming.py#new...
client/swarming.py:1382: if properties.get('inputs_ref'):
This assumption needs to fixed too.

[nodir, 2016-05-12 21:53:35]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-05-12 21:53:35]

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/1939343002/#ps280001
(title: "update inputs_ref checks")

[commit-bot: I haz the power, 2016-05-12 21:53:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939343002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939343002/280001

[nodir, 2016-05-12 21:54:09]

The CQ bit was unchecked by nodir@chromium.org

[nodir, 2016-05-12 21:54:56]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-12 21:55:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939343002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939343002/300001

[commit-bot: I haz the power, 2016-05-12 22:01:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-12 22:01:09]

Dry run: Try jobs failed on following builders:
  Luci-py Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-py%20Presubmit/bui...)

[nodir, 2016-05-12 23:05:59]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-05-12 23:06:00]

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/1939343002/#ps320001
(title: "fix templates")

[commit-bot: I haz the power, 2016-05-12 23:06:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939343002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939343002/320001

[commit-bot: I haz the power, 2016-05-12 23:08:58]

Description was changed from

==========
swarming: change meaning of inputs_ref

Currently inputs_ref implicitly specifies isolated server and namespace
for isolated outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Change the meaning of inputs_ref from "a reference to input file tree"
to "isolate server/namespace and hash of input file tree".
Now (inputs_ref is not None) does not mean there is isolated input,
but (inputs_ref and inputs_ref.isolated) does.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

Add a TODO to refactor the code and update datastore schema.

Update tests.

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

to

==========
swarming: change meaning of inputs_ref

Currently inputs_ref implicitly specifies isolated server and namespace
for isolated outputs. The problem is that a user may need to be able to specify
them for non-isolated command.

Change the meaning of inputs_ref from "a reference to input file tree"
to "isolate server/namespace and hash of input file tree".
Now (inputs_ref is not None) does not mean there is isolated input,
but (inputs_ref and inputs_ref.isolated) does.

In bot manifest, change "inputs_ref" to "isolated" that contains
input/output settings.

Add a TODO to refactor the code and update datastore schema.

Update tests.

R=maruel@chromium.org
BUG=601022

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

[commit-bot: I haz the power, 2016-05-12 23:09:00]

Committed patchset #14 (id:320001) as
https://github.com/luci/luci-py/commit/abdaf29b948fa7467b5f1fdaed3bfbc41a692145