swarming: change meaning of inputs_ref

appengine/swarming/server/task_request.py

 # coding: utf-8
 # Copyright 2014 The LUCI Authors. All rights reserved.
 # Use of this source code is governed by the Apache v2.0 license that can be
 # found in the LICENSE file.
 
 """Tasks definition.
 
 Each user request creates a new TaskRequest. The TaskRequest instance saves the
 metadata of the request, e.g. who requested it, when why, etc. It links to the
 actual data of the request in a TaskProperties. The TaskProperties represents
@@ skipping 220 matching lines @@
   # Namespace on the isolate server.
   namespace = ndb.StringProperty(validator=_validate_namespace, indexed=False)
 
   def _pre_put_hook(self):
     super(FilesRef, self)._pre_put_hook()
     if not self.isolated or not self.isolatedserver or not self.namespace:
       raise datastore_errors.BadValueError(
           'isolated requires server and namespace')
 
 
+class IsolatedOutputsTarget(ndb.Model):
+  """Defines a target of isolated output."""
+  # The hostname of the isolated server to use.
+  isolatedserver = ndb.StringProperty(
+      validator=_validate_hostname, indexed=False)
+  # Namespace on the isolate server.
+  namespace = ndb.StringProperty(validator=_validate_namespace, indexed=False)
+
+  def _pre_put_hook(self):
+    super(IsolatedOutputsTarget, self)._pre_put_hook()
+    if not self.isolatedserver or not self.namespace:
+      raise datastore_errors.BadValueError(
+        'isolated outputs target requires server and namespace')
+
+
 class CipdPackage(ndb.Model):
   """A CIPD package to install in $CIPD_PATH and $PATH before task execution.
 
   A part of TaskProperties.
   """
   package_name = ndb.StringProperty(
       indexed=False, validator=_validate_package_name)
   version = ndb.StringProperty(
       indexed=False, validator=_validate_package_version)
 
@@ skipping 26 matching lines @@
   commands = datastore_utils.DeterministicJsonProperty(
       json_type=list, indexed=False)
   # Command to run. This is only relevant when self._inputs_ref is None. This is
   # what is called 'raw commands', in the sense that no inputs files are
   # declared.
   command = ndb.StringProperty(repeated=True, indexed=False)
 
   # File inputs of the task. Only inputs_ref or command&data can be specified.
   inputs_ref = ndb.LocalStructuredProperty(FilesRef)
 
+  # Isolate output settings. Mutually exclusive with inputs_ref.
+  # If inputs_ref is not None, its server and namespace must be used for output.
+  outputs_target = ndb.LocalStructuredProperty(IsolatedOutputsTarget)

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

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:30]

No

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

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:17:39]

Done, updated CL description.

+
   # A list of CIPD packages to install $CIPD_PATH and $PATH before task
   # execution.
   packages = ndb.LocalStructuredProperty(CipdPackage, repeated=True)
 
   # Filter to use to determine the required properties on the bot to run on. For
   # example, Windows or hostname. Encoded as json. Optional but highly
   # recommended.
   dimensions = datastore_utils.DeterministicJsonProperty(
       validator=_validate_dimensions, json_type=dict, indexed=False)
 
@@ skipping 65 matching lines @@
     if self.commands:
         raise datastore_errors.BadValueError(
             'commands is not supported anymore')
     if not self.is_terminate:
       if bool(self.command) == bool(self.inputs_ref):
         raise datastore_errors.BadValueError('use one of command or inputs_ref')
       if self.extra_args and not self.inputs_ref:
         raise datastore_errors.BadValueError('extra_args require inputs_ref')
       if self.inputs_ref:
         self.inputs_ref._pre_put_hook()
+        if self.outputs_target:
+          raise datastore_errors.BadValueError(
+              'do not specify outputs_target with inputs_ref')
+      elif self.outputs_target:
+        self.outputs_target._pre_put_hook()
 
       package_names = set()
       for p in self.packages:
         p._pre_put_hook()
         if p.package_name in package_names:
           raise datastore_errors.BadValueError(
               'package %s is specified more than once' % p.package_name)
         package_names.add(p.package_name)
       self.packages.sort(key=lambda p: p.package_name)
 
       if self.idempotent:
         pinned = lambda p: cipd.is_pinned_version(p.version)
         if self.packages and any(not pinned(p) for p in self.packages):
           raise datastore_errors.BadValueError(
             'an idempotent task cannot have unpinned packages; '
             'use instance IDs or tags as package versions')
 
 
-
 class TaskRequest(ndb.Model):
   """Contains a user request.
 
   Key id is a decreasing integer based on time since utils.EPOCH plus some
   randomness on lower order bits. See _new_request_key() for the complete gory
   details.
 
   There is also "old style keys" which inherit from a fake root entity
   TaskRequestShard.
   TODO(maruel): Remove support 2015-10-01 once entities are deleted.
@@ skipping 301 matching lines @@
   _put_request(request)
   return request
 
 
 def validate_priority(priority):
   """Throws ValueError if priority is not a valid value."""
   if 0 > priority or MAXIMUM_PRIORITY < priority:
     raise datastore_errors.BadValueError(
         'priority (%d) must be between 0 and %d (inclusive)' %
         (priority, MAXIMUM_PRIORITY))