[ARVADOS] updated: 2.1.0-236-g69d5199e8

Git user git at public.arvados.org
Tue Jan 5 22:15:05 UTC 2021


Summary of changes:
 services/api/app/models/arvados_model.rb           | 30 ++++++++
 services/api/app/models/container.rb               | 17 ++---
 services/api/app/models/container_request.rb       | 83 ++--------------------
 .../v1/container_requests_controller_test.rb       |  3 +-
 services/api/test/unit/container_request_test.rb   | 33 +--------
 services/api/test/unit/container_test.rb           | 14 ++--
 6 files changed, 55 insertions(+), 125 deletions(-)

       via  69d5199e87e23523c050c692a27f818c5931770e (commit)
      from  e337906b3f5d38c25dd6ae58b7855ae3c2dc4744 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 69d5199e87e23523c050c692a27f818c5931770e
Author: Tom Clegg <tom at curii.com>
Date:   Tue Jan 5 17:14:13 2021 -0500

    17014: Refactor handling of default runtime/scheduling values.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 2a9e4e980..3a6d4489d 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -869,6 +869,36 @@ class ArvadosModel < ApplicationRecord
     nil
   end
 
+  # Fill in implied zero/false values in database records that were
+  # created before #17014 made them explicit, and reset all the Rails
+  # "changed" info.
+  #
+  # Invoked by Container and ContainerRequest models as an after_find
+  # hook.
+  def fill_container_defaults_after_find
+    fill_container_defaults
+    set_attribute_was('runtime_constraints', runtime_constraints)
+    set_attribute_was('scheduling_parameters', scheduling_parameters)
+    clear_changes_information
+  end
+
+  # Fill in implied zero/false values. Invoked by ContainerRequest as
+  # a before_validation hook so an omitted value in an update request
+  # doesn't count as "changed".
+  def fill_container_defaults
+    self.runtime_constraints = {
+      'api' => false,
+      'keep_cache_ram' => 0,
+      'ram' => 0,
+      'vcpus' => 0,
+    }.merge(attributes['runtime_constraints'] || {})
+    self.scheduling_parameters = {
+      'max_run_time' => 0,
+      'partitions' => [],
+      'preemptible' => false,
+    }.merge(attributes['scheduling_parameters'] || {})
+  end
+
   # ArvadosModel.find_by_uuid needs extra magic to allow it to return
   # an object in any class.
   def self.find_by_uuid uuid
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 04ee3ab53..d01787cbc 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -29,6 +29,7 @@ class Container < ArvadosModel
   serialize :command, Array
   serialize :scheduling_parameters, Hash
 
+  after_find :fill_container_defaults_after_find
   before_validation :fill_field_defaults, :if => :new_record?
   before_validation :set_timestamps
   before_validation :check_lock
@@ -206,25 +207,17 @@ class Container < ArvadosModel
   # is not implemented yet, or we have already found that no existing
   # containers are suitable).
   def self.resolve_runtime_constraints(runtime_constraints)
-    # Remove zero-values before merging to avoid stepping over other defaults.
-    cleaned_rc = {}
-    runtime_constraints.each do |k, v|
-      if ContainerRequest.runtime_constraints_defaults[k] != v
-        cleaned_rc[k] = v
-      end
-    end
     rc = {}
-    defaults = {
-      'keep_cache_ram' =>
-      Rails.configuration.Containers.DefaultKeepCacheRAM,
-    }
-    defaults.merge(cleaned_rc).each do |k, v|
+    runtime_constraints.each do |k, v|
       if v.is_a? Array
         rc[k] = v[0]
       else
         rc[k] = v
       end
     end
+    if rc['keep_cache_ram'] == 0
+      rc['keep_cache_ram'] = Rails.configuration.Containers.DefaultKeepCacheRAM
+    end
     rc
   end
 
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 687aa5368..837f2cdc7 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -30,15 +30,16 @@ class ContainerRequest < ArvadosModel
   serialize :command, Array
   serialize :scheduling_parameters, Hash
 
+  after_find :fill_container_defaults_after_find
   before_validation :fill_field_defaults, :if => :new_record?
-  before_validation :forget_innocuous_serialized_fields_updates, on: :update
-  before_validation :validate_runtime_constraints
+  before_validation :fill_container_defaults
   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 }
   validate :validate_datatypes
+  validate :validate_runtime_constraints
   validate :validate_scheduling_parameters
   validate :validate_state_change
   validate :check_update_whitelist
@@ -98,19 +99,6 @@ class ContainerRequest < ArvadosModel
   :runtime_constraints, :state, :container_uuid, :use_existing,
   :scheduling_parameters, :secret_mounts, :output_name, :output_ttl]
 
-  AttrsRuntimeConstraintsDefaults = {
-    "vcpus"=>0,
-    "ram"=>0,
-    "api"=>false,
-    "keep_cache_ram"=>0
-  }
-
-  AttrsSchedulingParametersDefaults = {
-    "max_run_time"=>0,
-    "partitions"=>[],
-    ## "preemptible"=> Rails.configuration.Containers.UsePreemptibleInstances  ##  we have to read this on the fly to pass the tests
-  }
-
   def self.limit_index_columns_read
     ["mounts"]
   end
@@ -123,14 +111,6 @@ class ContainerRequest < ArvadosModel
     State_transitions
   end
 
-  def self.runtime_constraints_defaults
-    AttrsRuntimeConstraintsDefaults
-  end
-
-  def self.scheduling_parameters_defaults
-    AttrsSchedulingParametersDefaults
-  end
-
   def skip_uuid_read_permission_check
     # The uuid_read_permission_check prevents users from making
     # references to objects they can't view.  However, in this case we
@@ -334,41 +314,17 @@ 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.Containers.UsePreemptibleInstances and !c.nil? and
-        self.scheduling_parameters['preemptible'].nil?
-          self.scheduling_parameters['preemptible'] = true
-      end
+    if Rails.configuration.Containers.UsePreemptibleInstances && state == Committed && get_requesting_container()
+      self.scheduling_parameters['preemptible'] = true
     end
   end
 
-  # Updates to serialized fields are all-or-nothing. Here we avoid making
-  # unnecessary updates.
-  def forget_innocuous_serialized_fields_updates
-    forgettable_attrs = []
-    if (runtime_constraints.to_a - runtime_constraints_was.to_a).empty?
-      forgettable_attrs.append('runtime_constraints')
-    end
-    if (scheduling_parameters.to_a - scheduling_parameters_was.to_a).empty?
-      forgettable_attrs.append('scheduling_parameters')
-    end
-    self.clear_attribute_changes(forgettable_attrs) if !forgettable_attrs.empty?
-  end
-
   def validate_runtime_constraints
     case self.state
     when Committed
-      [['vcpus', true],
-       ['ram', true],
-       ['keep_cache_ram', false]].each do |k, required|
-        if !required && (!runtime_constraints.include?(k) || runtime_constraints[k] == 0)
-          next
-        end
+      ['vcpus', 'ram'].each do |k|
         v = runtime_constraints[k]
-        unless (v.is_a?(Integer) && v > 0)
+        if !v.is_a?(Integer) || v <= 0
           errors.add(:runtime_constraints,
                      "[#{k}]=#{v.inspect} must be a positive integer")
         end
@@ -478,31 +434,6 @@ class ContainerRequest < ArvadosModel
     super(permitted)
   end
 
-  def set_zero_values
-    # this will fill out zero values that are not in the database,
-    # see https://dev.arvados.org/issues/17014#note-28 for details
-
-    AttrsRuntimeConstraintsDefaults.each do |key, value|
-      if attributes["runtime_constraints"] && !attributes["runtime_constraints"].key?(key)
-        attributes["runtime_constraints"][key] = value
-      end
-    end
-    AttrsSchedulingParametersDefaults.each do |key, value|
-      if attributes["scheduling_parameters"] && !attributes["scheduling_parameters"].key?(key)
-        attributes["scheduling_parameters"][key] = value
-      end
-    end
-
-    ## We treat scheduling parameters["preemtible"] differently
-    if attributes["scheduling_parameters"] && !attributes["scheduling_parameters"].key?("preemptible")
-      attributes["scheduling_parameters"]["preemptible"] = Rails.configuration.Containers.UsePreemptibleInstances
-    end
-
-    self.clear_attribute_changes(["runtime_constraints", "scheduling_parameters"])
-
-    super
-  end
-
   def secret_mounts_key_conflict
     secret_mounts.each do |k, v|
       if mounts.has_key?(k)
diff --git a/services/api/test/functional/arvados/v1/container_requests_controller_test.rb b/services/api/test/functional/arvados/v1/container_requests_controller_test.rb
index 6f0a41158..e99af39c9 100644
--- a/services/api/test/functional/arvados/v1/container_requests_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/container_requests_controller_test.rb
@@ -24,7 +24,8 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
 
     cr = JSON.parse(@response.body)
     assert_not_nil cr, 'Expected container request'
-    assert_equal sp, cr['scheduling_parameters']
+    assert_equal sp['partitions'], cr['scheduling_parameters']['partitions']
+    assert_equal false, cr['scheduling_parameters']['preemptible']
   end
 
   test "secret_mounts not in #create responses" do
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index aed3d9342..b2dde7995 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -978,40 +978,9 @@ class ContainerRequestTest < ActiveSupport::TestCase
     end
   end
 
-  [
-    'zzzzz-dz642-runningcontainr',
-    nil,
-  ].each do |requesting_c|
-    test "having preemptible instances active on the API server, a committed #{requesting_c.nil? ? 'non-':''}child CR should not ask for preemptible instance if parameter already set to false" do
-      common_attrs = {cwd: "test",
-                      priority: 1,
-                      command: ["echo", "hello"],
-                      output_path: "test",
-                      scheduling_parameters: {"preemptible" => false},
-                      mounts: {"test" => {"kind" => "json"}}}
-
-      Rails.configuration.Containers.UsePreemptibleInstances = true
-      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.state = ContainerRequest::Committed
-      cr.save!
-
-      assert_equal false, cr.scheduling_parameters['preemptible']
-    end
-  end
-
   [
     [true, 'zzzzz-dz642-runningcontainr', true],
-    [true, nil, true],
+    [true, nil, false],
     [false, 'zzzzz-dz642-runningcontainr', false],
     [false, nil, false],
   ].each do |preemptible_conf, requesting_c, schedule_preemptible|
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 98e60e057..4d8538524 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -23,6 +23,8 @@ class ContainerTest < ActiveSupport::TestCase
     command: ["echo", "hello"],
     output_path: "test",
     runtime_constraints: {
+      "api" => false,
+      "keep_cache_ram" => 0,
       "ram" => 12000000000,
       "vcpus" => 4,
     },
@@ -227,11 +229,12 @@ class ContainerTest < ActiveSupport::TestCase
     set_user_from_auth :active
     env = {"C" => "3", "B" => "2", "A" => "1"}
     m = {"F" => {"kind" => "3"}, "E" => {"kind" => "2"}, "D" => {"kind" => "1"}}
-    rc = {"vcpus" => 1, "ram" => 1, "keep_cache_ram" => 1}
+    rc = {"vcpus" => 1, "ram" => 1, "keep_cache_ram" => 1, "api" => true}
     c, _ = minimal_new(environment: env, mounts: m, runtime_constraints: rc)
-    assert_equal c.environment.to_json, Container.deep_sort_hash(env).to_json
-    assert_equal c.mounts.to_json, Container.deep_sort_hash(m).to_json
-    assert_equal c.runtime_constraints.to_json, Container.deep_sort_hash(rc).to_json
+    c.reload
+    assert_equal Container.deep_sort_hash(env).to_json, c.environment.to_json
+    assert_equal Container.deep_sort_hash(m).to_json, c.mounts.to_json
+    assert_equal Container.deep_sort_hash(rc).to_json, c.runtime_constraints.to_json
   end
 
   test 'deep_sort_hash on array of hashes' do
@@ -561,6 +564,7 @@ class ContainerTest < ActiveSupport::TestCase
     assert_equal Container::Queued, c1.state
     reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token)))
     # See #14584
+    assert_not_nil reused
     assert_equal c1.uuid, reused.uuid
   end
 
@@ -571,6 +575,7 @@ class ContainerTest < ActiveSupport::TestCase
     assert_equal Container::Queued, c1.state
     reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token)))
     # See #14584
+    assert_not_nil reused
     assert_equal c1.uuid, reused.uuid
   end
 
@@ -581,6 +586,7 @@ class ContainerTest < ActiveSupport::TestCase
     assert_equal Container::Queued, c1.state
     reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token)))
     # See #14584
+    assert_not_nil reused
     assert_equal c1.uuid, reused.uuid
   end
 

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list