[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