20606: Don't reuse unstarted preemptible ctr for non-preemptible cr.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 8a51d749f7137ec5ace615c365a234c23505180d..5e2d449b276e6b88f0dda7316de37427f3e5fc3a 100644 (file)
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -343,7 +343,7 @@ class Container < ArvadosModel
     # Check for non-failing Running candidates and return the most likely to finish sooner.
     log_reuse_info { "checking for state=Running..." }
     running = candidates.where(state: Running).
-              where("(runtime_status->'error') is null").
+              where("(runtime_status->'error') is null and priority > 0").
               order('progress desc, started_at asc').
               limit(1).first
     if running
@@ -357,10 +357,15 @@ class Container < ArvadosModel
     locked_or_queued = candidates.
                        where("state IN (?)", [Locked, Queued]).
                        order('state asc, priority desc, created_at asc').
-                       limit(1).first
-    if locked_or_queued
-      log_reuse_info { "done, reusing container #{locked_or_queued.uuid} with state=#{locked_or_queued.state}" }
-      return locked_or_queued
+                       limit(1)
+    if !attrs[:scheduling_parameters]['preemptible']
+      locked_or_queued = locked_or_queued.
+                           where("not ((scheduling_parameters::jsonb)->>'preemptible')::boolean")
+    end
+    chosen = locked_or_queued.first
+    if chosen
+      log_reuse_info { "done, reusing container #{chosen.uuid} with state=#{chosen.state}" }
+      return chosen
     else
       log_reuse_info { "have no containers in Locked or Queued state" }
     end

diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 286aa32ae209a98f3930076cb76d5b9ec0988700..300c73ed59708c59da7284ee183b04ab7fe5f3d1 100644 (file)
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -37,7 +37,8 @@ class ContainerTest < ActiveSupport::TestCase
     },
     secret_mounts: {},
     runtime_user_uuid: "zzzzz-tpzed-xurymjxw79nv3jz",
-    runtime_auth_scopes: ["all"]
+    runtime_auth_scopes: ["all"],
+    scheduling_parameters: {},
   }
 
   REUSABLE_ATTRS_SLIM = {
@@ -57,6 +58,7 @@ class ContainerTest < ActiveSupport::TestCase
     },
     runtime_user_uuid: "zzzzz-tpzed-xurymjxw79nv3jz",
     secret_mounts: {},
+    scheduling_parameters: {},
   }
 
   def request_only attrs
@@ -526,6 +528,34 @@ class ContainerTest < ActiveSupport::TestCase
     assert_nil reused
   end
 
+  [[false, false, true],
+   [false, true, true],
+   [true, false, false],
+   [true, true, true]
+  ].each do |c1_preemptible, c2_preemptible, should_reuse|
+    [[Container::Queued, 1],
+     [Container::Locked, 1],
+     [Container::Running, 0],   # not cancelled yet, but obviously will be soon
+    ].each do |c1_state, c1_priority|
+      test "find_reusable for #{c2_preemptible ? '' : 'non-'}preemptible req should #{should_reuse ? '' : 'not'} reuse a #{c1_state} #{c1_preemptible ? '' : 'non-'}preemptible container with priority #{c1_priority}" do
+        configure_preemptible_instance_type
+        set_user_from_auth :active
+        c1_attrs = REUSABLE_COMMON_ATTRS.merge({environment: {"test" => name, "state" => c1_state}, scheduling_parameters: {"preemptible" => c1_preemptible}})
+        c1, _ = minimal_new(c1_attrs)
+        set_user_from_auth :dispatch1
+        c1.update_attributes!({state: Container::Locked}) if c1_state != Container::Queued
+        c1.update_attributes!({state: Container::Running, priority: c1_priority}) if c1_state == Container::Running
+        c2_attrs = c1_attrs.merge({scheduling_parameters: {"preemptible" => c2_preemptible}})
+        reused = Container.find_reusable(c2_attrs)
+        if should_reuse && c1_priority > 0
+          assert_not_nil reused
+        else
+          assert_nil reused
+        end
+      end
+    end
+  end
+
   test "find_reusable with logging disabled" do
     set_user_from_auth :active
     Rails.logger.expects(:info).never