13649: Fixes default preemptible parameter when submitting committed child CRs

Two problems fixed:
1) When submitting a committed child CR, its preemptible parameter is set.
2) Resolve a container for said child CR after the preemptible parameter setting.

Added tests for committed child CR submissions.

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 799aa430fbe46906b7e0768700e8fc3f0d8fe3f7..a0ebdbab06aa44298d7e4a8ff17e728413b62f3c 100644 (file)
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -28,8 +28,8 @@ class ContainerRequest < ArvadosModel
 
   before_validation :fill_field_defaults, :if => :new_record?
   before_validation :validate_runtime_constraints
-  before_validation :set_container
   before_validation :set_default_preemptible_scheduling_parameter
+  before_validation :set_container
   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 }
@@ -199,11 +199,11 @@ class ContainerRequest < ArvadosModel
   end
 
   def set_default_preemptible_scheduling_parameter
+    c = get_requesting_container()
     if self.state == Committed
       # If preemptible instances (eg: AWS Spot Instances) are allowed,
       # ask them on child containers by default.
-      if Rails.configuration.preemptible_instances and
-        !self.requesting_container_uuid.nil? and
+      if Rails.configuration.preemptible_instances and !c.nil? and
         self.scheduling_parameters['preemptible'].nil?
           self.scheduling_parameters['preemptible'] = true
       end
@@ -313,10 +313,18 @@ class ContainerRequest < ArvadosModel
   end
 
   def set_requesting_container_uuid
-    return if !current_api_client_authorization
-    if (c = Container.where('auth_uuid=?', current_api_client_authorization.uuid).select([:uuid, :priority]).first)
+    c = get_requesting_container()
+    if !c.nil?
       self.requesting_container_uuid = c.uuid
       self.priority = c.priority>0 ? 1 : 0
     end
   end
+
+  def get_requesting_container
+    return self.requesting_container_uuid if !self.requesting_container_uuid.nil?
+    return if !current_api_client_authorization
+    if (c = Container.where('auth_uuid=?', current_api_client_authorization.uuid).select([:uuid, :priority]).first)
+      return c
+    end
+  end
 end

diff --git a/services/api/lib/whitelist_update.rb b/services/api/lib/whitelist_update.rb
index f25d4238106697002a692c552e0b300b4d90067a..17aed4b48dba66b079431007408dae49ee6442cf 100644 (file)
--- a/services/api/lib/whitelist_update.rb
+++ b/services/api/lib/whitelist_update.rb
@@ -6,7 +6,7 @@ module WhitelistUpdate
   def check_update_whitelist permitted_fields
     attribute_names.each do |field|
       if !permitted_fields.include?(field.to_sym) && really_changed(field)
-        errors.add field, "cannot be modified in this state (#{send(field+"_was").inspect}, #{send(field).inspect})"
+        errors.add field, "cannot be modified in state '#{self.state}' (#{send(field+"_was").inspect}, #{send(field).inspect})"
       end
     end
   end

diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 8071e05cebe92669e3c240d5697f80ab55998633..fc872e1d0309c0513e77656008a85b73e4e916de 100644 (file)
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -849,6 +849,25 @@ class ContainerRequestTest < ActiveSupport::TestCase
     end
   end
 
+  test "submit a Committed child CR with preemptible_instances active" do
+    attrs = {cwd: "test",
+             priority: 1,
+             state: ContainerRequest::Committed,
+             command: ["echo", "hello"],
+             output_path: "test",
+             mounts: {"test" => {"kind" => "json"}}}
+
+    Rails.configuration.preemptible_instances = true
+    set_user_from_auth :active
+
+    cr = with_container_auth(Container.find_by_uuid 'zzzzz-dz642-runningcontainr') do
+      create_minimal_req!(attrs)
+    end
+
+    assert_not_nil cr.requesting_container_uuid
+    assert_equal true, cr.scheduling_parameters['preemptible']
+  end
+
   [
     [{"partitions" => ["fastcpu","vfastcpu", 100]}, ContainerRequest::Committed, ActiveRecord::RecordInvalid],
     [{"partitions" => ["fastcpu","vfastcpu", 100]}, ContainerRequest::Uncommitted],
@@ -881,6 +900,25 @@ class ContainerRequestTest < ActiveSupport::TestCase
     end
   end
 
+  test "Having preemptible_instances=true create a committed child container request and verify the scheduling parameter of its container" do
+    common_attrs = {cwd: "test",
+                    priority: 1,
+                    command: ["echo", "hello"],
+                    output_path: "test",
+                    state: ContainerRequest::Committed,
+                    mounts: {"test" => {"kind" => "json"}}}
+    set_user_from_auth :active
+    Rails.configuration.preemptible_instances = true
+
+    cr = with_container_auth(Container.find_by_uuid 'zzzzz-dz642-runningcontainr') do
+      create_minimal_req!(common_attrs)
+    end
+    assert_equal true, cr.scheduling_parameters["preemptible"]
+
+    c = Container.find_by_uuid(cr.container_uuid)
+    assert_equal true, c.scheduling_parameters["preemptible"]
+  end
+
   [['Committed', true, {name: "foobar", priority: 123}],
    ['Committed', false, {container_count: 2}],
    ['Committed', false, {container_count: 0}],