7478: Scheduling parameters validation fixes.

* Set the preemptable parameter to child CRs by default separately from the
validations.
* Allow non-child CR to have the preemptable parameter set explicitly.
* Disallow the preemptable parameter when configuration is disabled.
* Updated tests.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima@veritasgenetics.com>

diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 5c5a043a0563a6c3013c1697b02afcbfc6ee02f1..8f3f99e7ee84d330e5ce8c855e0b11fd876bdab1 100644 (file)
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -28,11 +28,12 @@ class ContainerRequest < ArvadosModel
 
   before_validation :fill_field_defaults, :if => :new_record?
   before_validation :validate_runtime_constraints
-  before_validation :validate_scheduling_parameters
   before_validation :set_container
+  before_validation :set_default_preemptable_scheduling_parameter
   validates :command, :container_image, :output_path, :cwd, :presence => true
   validates :output_ttl, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
   validates :priority, numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 1000 }
+  validate :validate_scheduling_parameters
   validate :validate_state_change
   validate :check_update_whitelist
   validate :secret_mounts_key_conflict
@@ -197,6 +198,16 @@ class ContainerRequest < ArvadosModel
     end
   end
 
+  def set_default_preemptable_scheduling_parameter
+    if self.state == Committed
+      # If preemptable instances (eg: AWS Spot Instances) are allowed,
+      # automatically ask them on non-child containers by default.
+      if Rails.configuration.preemptable_instances and !self.requesting_container_uuid.nil?
+        self.scheduling_parameters['preemptable'] ||= true
+      end
+    end
+  end
+
   def validate_runtime_constraints
     case self.state
     when Committed
@@ -223,12 +234,8 @@ class ContainerRequest < ArvadosModel
             scheduling_parameters['partitions'].size)
             errors.add :scheduling_parameters, "partitions must be an array of strings"
       end
-      if !self.scheduling_parameters.include?('preemptable')
-        if !self.requesting_container_uuid.nil? and Rails.configuration.preemptable_instances
-          self.scheduling_parameters['preemptable'] = true
-        end
-      elsif scheduling_parameters['preemptable'] and self.requesting_container_uuid.nil?
-        errors.add :scheduling_parameters, "only child containers can be preemptable"
+      if !Rails.configuration.preemptable_instances and scheduling_parameters['preemptable']
+        errors.add :scheduling_parameters, "preemptable instances are not allowed"
       end
     end
   end

diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 5bca02a967691ad328a7dee9d999badc3a8cc6aa..6f76218132dc2f0f5619085ea4cab2c550f7abc4 100644 (file)
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -758,31 +758,23 @@ class ContainerRequestTest < ActiveSupport::TestCase
   end
 
   [
-    [{"preemptable" => true}, nil, ActiveRecord::RecordInvalid],
-    [{"preemptable" => false}, nil, nil],
-    [{"preemptable" => true}, 'zzzzz-dz642-runningcontainr', nil],
-    [{"preemptable" => false}, 'zzzzz-dz642-runningcontainr', nil],
-  ].each do |sp, requesting_c, expected|
-    test "create container request with scheduling_parameters #{sp} with requesting_container_uuid=#{requesting_c} and verify #{expected}" do
+    [false, ActiveRecord::RecordInvalid],
+    [true, nil],
+  ].each do |preemptable_conf, expected|
+    test "having Rails.configuration.preemptable_instances=#{preemptable_conf}, create preemptable container request and verify #{expected}" do
+      sp = {"preemptable" => true}
       common_attrs = {cwd: "test",
                       priority: 1,
                       command: ["echo", "hello"],
                       output_path: "test",
                       scheduling_parameters: sp,
                       mounts: {"test" => {"kind" => "json"}}}
-
+      Rails.configuration.preemptable_instances = preemptable_conf
       set_user_from_auth :active
 
-      if requesting_c
-        cr = with_container_auth(Container.find_by_uuid requesting_c) do
-          create_minimal_req!(common_attrs)
-        end
-        assert_not_nil cr.requesting_container_uuid
-      else
-        cr = create_minimal_req!(common_attrs)
-      end
-
+      cr = create_minimal_req!(common_attrs)
       cr.state = ContainerRequest::Committed
+
       if !expected.nil?
         assert_raises(expected) do
           cr.save!