Merge branch '10534-go-systemd-sdnotify-v14' of https://github.com/wtsi-hgi/arvados
authorTom Clegg <tom@curoverse.com>
Tue, 15 Nov 2016 16:35:15 +0000 (11:35 -0500)
committerTom Clegg <tom@curoverse.com>
Tue, 15 Nov 2016 16:35:15 +0000 (11:35 -0500)
refs #10534

sdk/cwl/arvados_cwl/arvcontainer.py
sdk/cwl/tests/test_container.py
sdk/go/arvados/container.go
services/api/app/models/container.rb
services/api/app/models/container_request.rb
services/api/db/migrate/20161111143147_add_scheduling_parameters_to_container.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/test/unit/container_request_test.rb
services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go

index c0d82d9585a70488c450f085d9e91704718a5b46..aa088c5e8a06fa00ec086483b9f628c79687965f 100644 (file)
@@ -42,6 +42,7 @@ class ArvadosContainer(object):
                 "kind": "tmp"
             }
         }
+        scheduling_parameters = {}
 
         dirs = set()
         for f in self.pathmapper.files():
@@ -102,11 +103,12 @@ class ArvadosContainer(object):
 
         partition_req, _ = get_feature(self, "http://arvados.org/cwl#PartitionRequirement")
         if partition_req:
-            runtime_constraints["partition"] = aslist(partition_req["partition"])
+            scheduling_parameters["partitions"] = aslist(partition_req["partition"])
 
         container_request["mounts"] = mounts
         container_request["runtime_constraints"] = runtime_constraints
         container_request["use_existing"] = kwargs.get("enable_reuse", True)
+        container_request["scheduling_parameters"] = scheduling_parameters
 
         try:
             response = self.arvrunner.api.container_requests().create(
index 93100ae9f76026c745fd5fdc3b011af550ac0079..bb4bac31dd1767081cdc12a313496a4bb13b4546 100644 (file)
@@ -68,7 +68,8 @@ class TestContainer(unittest.TestCase):
                         'output_path': '/var/spool/cwl',
                         'container_image': '99999999999999999999999999999993+99',
                         'command': ['ls', '/var/spool/cwl'],
-                        'cwd': '/var/spool/cwl'
+                        'cwd': '/var/spool/cwl',
+                        'scheduling_parameters': {}
                     })
 
     # The test passes some fields in builder.resources
@@ -113,8 +114,9 @@ class TestContainer(unittest.TestCase):
                              make_fs_access=make_fs_access, tmpdir="/tmp"):
             j.run()
 
-        runner.api.container_requests().create.assert_called_with(
-            body={
+        call_args, call_kwargs = runner.api.container_requests().create.call_args
+
+        call_body_expected = {
                 'environment': {
                     'HOME': '/var/spool/cwl',
                     'TMPDIR': '/tmp'
@@ -124,8 +126,7 @@ class TestContainer(unittest.TestCase):
                     'vcpus': 3,
                     'ram': 3145728000,
                     'keep_cache_ram': 512,
-                    'API': True,
-                    'partition': ['blurb']
+                    'API': True
                 },
                 'use_existing': True,
                 'priority': 1,
@@ -137,8 +138,16 @@ class TestContainer(unittest.TestCase):
                 'output_path': '/var/spool/cwl',
                 'container_image': '99999999999999999999999999999993+99',
                 'command': ['ls'],
-                'cwd': '/var/spool/cwl'
-            })
+                'cwd': '/var/spool/cwl',
+                'scheduling_parameters': {
+                    'partitions': ['blurb']
+                }
+        }
+
+        call_body = call_kwargs.get('body', None)
+        self.assertNotEqual(None, call_body)
+        for key in call_body:
+            self.assertEqual(call_body_expected.get(key), call_body.get(key))
 
     @mock.patch("arvados.collection.Collection")
     def test_done(self, col):
index 6a76f1f396a32c89544f55030cb586ae413d0c0b..61c14ea0b6c1d445bb2a26fb83a57614e0b240f9 100644 (file)
@@ -2,18 +2,19 @@ package arvados
 
 // Container is an arvados#container resource.
 type Container struct {
-       UUID               string             `json:"uuid"`
-       Command            []string           `json:"command"`
-       ContainerImage     string             `json:"container_image"`
-       Cwd                string             `json:"cwd"`
-       Environment        map[string]string  `json:"environment"`
-       LockedByUUID       string             `json:"locked_by_uuid"`
-       Mounts             map[string]Mount   `json:"mounts"`
-       Output             string             `json:"output"`
-       OutputPath         string             `json:"output_path"`
-       Priority           int                `json:"priority"`
-       RuntimeConstraints RuntimeConstraints `json:"runtime_constraints"`
-       State              ContainerState     `json:"state"`
+       UUID                 string               `json:"uuid"`
+       Command              []string             `json:"command"`
+       ContainerImage       string               `json:"container_image"`
+       Cwd                  string               `json:"cwd"`
+       Environment          map[string]string    `json:"environment"`
+       LockedByUUID         string               `json:"locked_by_uuid"`
+       Mounts               map[string]Mount     `json:"mounts"`
+       Output               string               `json:"output"`
+       OutputPath           string               `json:"output_path"`
+       Priority             int                  `json:"priority"`
+       RuntimeConstraints   RuntimeConstraints   `json:"runtime_constraints"`
+       State                ContainerState       `json:"state"`
+       SchedulingParameters SchedulingParameters `json:"scheduling_parameters"`
 }
 
 // Mount is special behavior to attach to a filesystem path or device.
@@ -31,10 +32,15 @@ type Mount struct {
 // CPU) and network connectivity.
 type RuntimeConstraints struct {
        API          *bool
-       RAM          int      `json:"ram"`
-       VCPUs        int      `json:"vcpus"`
-       KeepCacheRAM int      `json:"keep_cache_ram"`
-       Partition    []string `json:"partition"`
+       RAM          int `json:"ram"`
+       VCPUs        int `json:"vcpus"`
+       KeepCacheRAM int `json:"keep_cache_ram"`
+}
+
+// SchedulingParameters specify a container's scheduling parameters
+// such as Partitions
+type SchedulingParameters struct {
+       Partitions []string `json:"partitions"`
 }
 
 // ContainerList is an arvados#containerList resource.
index b1ea9bd230a47e2382dbb12ac0c0d6bee6929588..52f1cba723ed5744af3bf226265f9bb600d4f61f 100644 (file)
@@ -11,6 +11,7 @@ class Container < ArvadosModel
   serialize :mounts, Hash
   serialize :runtime_constraints, Hash
   serialize :command, Array
+  serialize :scheduling_parameters, Hash
 
   before_validation :fill_field_defaults, :if => :new_record?
   before_validation :set_timestamps
@@ -44,6 +45,7 @@ class Container < ArvadosModel
     t.add :started_at
     t.add :state
     t.add :auth_uuid
+    t.add :scheduling_parameters
   end
 
   # Supported states for a container
@@ -180,6 +182,7 @@ class Container < ArvadosModel
     self.mounts ||= {}
     self.cwd ||= "."
     self.priority ||= 1
+    self.scheduling_parameters ||= {}
   end
 
   def permission_to_create
@@ -222,7 +225,7 @@ class Container < ArvadosModel
     if self.new_record?
       permitted.push(:owner_uuid, :command, :container_image, :cwd,
                      :environment, :mounts, :output_path, :priority,
-                     :runtime_constraints)
+                     :runtime_constraints, :scheduling_parameters)
     end
 
     case self.state
@@ -326,6 +329,9 @@ class Container < ArvadosModel
     if self.runtime_constraints_changed?
       self.runtime_constraints = self.class.deep_sort_hash(self.runtime_constraints)
     end
+    if self.scheduling_parameters_changed?
+      self.scheduling_parameters = self.class.deep_sort_hash(self.scheduling_parameters)
+    end
   end
 
   def handle_completed
@@ -348,7 +354,8 @@ class Container < ArvadosModel
             output_path: self.output_path,
             container_image: self.container_image,
             mounts: self.mounts,
-            runtime_constraints: self.runtime_constraints
+            runtime_constraints: self.runtime_constraints,
+            scheduling_parameters: self.scheduling_parameters
           }
           c = Container.create! c_attrs
           retryable_requests.each do |cr|
index 05738de81e50627654e62e3f3a34d6cc46754460..7dcfbe378b6700a88b41ae11b6a15e8a28a6fe20 100644 (file)
@@ -11,9 +11,11 @@ class ContainerRequest < ArvadosModel
   serialize :mounts, Hash
   serialize :runtime_constraints, Hash
   serialize :command, Array
+  serialize :scheduling_parameters, Hash
 
   before_validation :fill_field_defaults, :if => :new_record?
   before_validation :validate_runtime_constraints
+  before_validation :validate_scheduling_parameters
   before_validation :set_container
   validates :command, :container_image, :output_path, :cwd, :presence => true
   validate :validate_state_change
@@ -42,6 +44,7 @@ class ContainerRequest < ArvadosModel
     t.add :runtime_constraints
     t.add :state
     t.add :use_existing
+    t.add :scheduling_parameters
   end
 
   # Supported states for a container request
@@ -105,6 +108,7 @@ class ContainerRequest < ArvadosModel
     self.mounts ||= {}
     self.cwd ||= "."
     self.container_count_max ||= Rails.configuration.container_count_max
+    self.scheduling_parameters ||= {}
   end
 
   # Create a new container (or find an existing one) to satisfy this
@@ -126,6 +130,7 @@ class ContainerRequest < ArvadosModel
       if not reusable.nil?
         reusable
       else
+        c_attrs[:scheduling_parameters] = self.scheduling_parameters
         Container.create!(c_attrs)
       end
     end
@@ -234,6 +239,17 @@ class ContainerRequest < ArvadosModel
     end
   end
 
+  def validate_scheduling_parameters
+    if self.state == Committed
+      if scheduling_parameters.include? 'partitions' and
+         (!scheduling_parameters['partitions'].is_a?(Array) ||
+          scheduling_parameters['partitions'].reject{|x| !x.is_a?(String)}.size !=
+            scheduling_parameters['partitions'].size)
+            errors.add :scheduling_parameters, "partitions must be an array of strings"
+      end
+    end
+  end
+
   def validate_change
     permitted = [:owner_uuid]
 
@@ -244,7 +260,7 @@ class ContainerRequest < ArvadosModel
                      :container_image, :cwd, :description, :environment,
                      :filters, :mounts, :name, :output_path, :priority,
                      :properties, :requesting_container_uuid, :runtime_constraints,
-                     :state, :container_uuid, :use_existing
+                     :state, :container_uuid, :use_existing, :scheduling_parameters
 
     when Committed
       if container_uuid.nil?
@@ -263,7 +279,7 @@ class ContainerRequest < ArvadosModel
         permitted.push :command, :container_image, :cwd, :description, :environment,
                        :filters, :mounts, :name, :output_path, :properties,
                        :requesting_container_uuid, :runtime_constraints,
-                       :state, :container_uuid
+                       :state, :container_uuid, :scheduling_parameters
       end
 
     when Final
diff --git a/services/api/db/migrate/20161111143147_add_scheduling_parameters_to_container.rb b/services/api/db/migrate/20161111143147_add_scheduling_parameters_to_container.rb
new file mode 100644 (file)
index 0000000..1b317cf
--- /dev/null
@@ -0,0 +1,6 @@
+class AddSchedulingParametersToContainer < ActiveRecord::Migration
+  def change
+    add_column :containers, :scheduling_parameters, :text
+    add_column :container_requests, :scheduling_parameters, :text
+  end
+end
index 0db782af69484e6a8e0c476620891702055f36c7..1d3d238c837611e2858dbfd0959cfc7373a41917 100644 (file)
@@ -291,7 +291,8 @@ CREATE TABLE container_requests (
     filters text,
     updated_at timestamp without time zone NOT NULL,
     container_count integer DEFAULT 0,
-    use_existing boolean DEFAULT true
+    use_existing boolean DEFAULT true,
+    scheduling_parameters text
 );
 
 
@@ -343,7 +344,8 @@ CREATE TABLE containers (
     updated_at timestamp without time zone NOT NULL,
     exit_code integer,
     auth_uuid character varying(255),
-    locked_by_uuid character varying(255)
+    locked_by_uuid character varying(255),
+    scheduling_parameters text
 );
 
 
@@ -2694,4 +2696,6 @@ INSERT INTO schema_migrations (version) VALUES ('20160909181442');
 
 INSERT INTO schema_migrations (version) VALUES ('20160926194129');
 
-INSERT INTO schema_migrations (version) VALUES ('20161019171346');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20161019171346');
+
+INSERT INTO schema_migrations (version) VALUES ('20161111143147');
\ No newline at end of file
index 34aa442c0938381e5fb56ebb2c6379616ad93976..1465c7180ad7b59cb947b4cfb825d2957ea17844 100644 (file)
@@ -552,4 +552,36 @@ class ContainerRequestTest < ActiveSupport::TestCase
       end
     end
   end
+
+  [
+    [{"partitions" => ["fastcpu","vfastcpu", 100]}, ContainerRequest::Committed, ActiveRecord::RecordInvalid],
+    [{"partitions" => ["fastcpu","vfastcpu", 100]}, ContainerRequest::Uncommitted],
+    [{"partitions" => "fastcpu"}, ContainerRequest::Committed, ActiveRecord::RecordInvalid],
+    [{"partitions" => "fastcpu"}, ContainerRequest::Uncommitted],
+    [{"partitions" => ["fastcpu","vfastcpu"]}, ContainerRequest::Committed],
+  ].each do |sp, state, expected|
+    test "create container request with scheduling_parameters #{sp} in state #{state} and verify #{expected}" do
+      common_attrs = {cwd: "test",
+                      priority: 1,
+                      command: ["echo", "hello"],
+                      output_path: "test",
+                      scheduling_parameters: sp,
+                      mounts: {"test" => {"kind" => "json"}}}
+      set_user_from_auth :active
+
+      if expected == ActiveRecord::RecordInvalid
+        assert_raises(ActiveRecord::RecordInvalid) do
+          create_minimal_req!(common_attrs.merge({state: state}))
+        end
+      else
+        cr = create_minimal_req!(common_attrs.merge({state: state}))
+        assert_equal sp, cr.scheduling_parameters
+
+        if state == ContainerRequest::Committed
+          c = Container.find_by_uuid(cr.container_uuid)
+          assert_equal sp, c.scheduling_parameters
+        end
+      end
+    end
+  end
 end
index 7e690eba80ae035bfe5ebbe9a086597680abd56d..3c4f281912842a0ceedb6df409aa61e80fa38fa2 100644 (file)
@@ -127,8 +127,8 @@ func sbatchFunc(container arvados.Container) *exec.Cmd {
        sbatchArgs = append(sbatchArgs, fmt.Sprintf("--job-name=%s", container.UUID))
        sbatchArgs = append(sbatchArgs, fmt.Sprintf("--mem-per-cpu=%d", int(memPerCPU)))
        sbatchArgs = append(sbatchArgs, fmt.Sprintf("--cpus-per-task=%d", container.RuntimeConstraints.VCPUs))
-       if container.RuntimeConstraints.Partition != nil {
-               sbatchArgs = append(sbatchArgs, fmt.Sprintf("--partition=%s", strings.Join(container.RuntimeConstraints.Partition, ",")))
+       if container.SchedulingParameters.Partitions != nil {
+               sbatchArgs = append(sbatchArgs, fmt.Sprintf("--partition=%s", strings.Join(container.SchedulingParameters.Partitions, ",")))
        }
 
        return exec.Command("sbatch", sbatchArgs...)
index c9208a6943924a1604c7b15735536229cde68104..fbea48e548a59f78718cb0afa419b5a84a1cd89b 100644 (file)
@@ -318,7 +318,7 @@ func testSbatchFuncWithArgs(c *C, args []string) {
 
 func (s *MockArvadosServerSuite) TestSbatchPartition(c *C) {
        theConfig.SbatchArguments = nil
-       container := arvados.Container{UUID: "123", RuntimeConstraints: arvados.RuntimeConstraints{RAM: 250000000, VCPUs: 1, Partition: []string{"blurb", "b2"}}}
+       container := arvados.Container{UUID: "123", RuntimeConstraints: arvados.RuntimeConstraints{RAM: 250000000, VCPUs: 1}, SchedulingParameters: arvados.SchedulingParameters{Partitions: []string{"blurb", "b2"}}}
        sbatchCmd := sbatchFunc(container)
 
        var expected []string