[ARVADOS] created: 2.1.0-236-ga2ae9be78
Git user
git at public.arvados.org
Wed Jan 6 02:09:45 UTC 2021
at a2ae9be785bdc066c74a6157f921b07638807fbe (commit)
commit a2ae9be785bdc066c74a6157f921b07638807fbe
Author: Tom Clegg <tom at curii.com>
Date: Tue Jan 5 21:00:53 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..7d5bea8fa 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -16,7 +16,6 @@ class ArvadosModel < ApplicationRecord
include DbCurrentTime
extend RecordFilters
- after_find :set_zero_values
after_find :schedule_restoring_changes
after_initialize :log_start_state
before_save :ensure_permission_to_save
@@ -47,19 +46,6 @@ class ArvadosModel < ApplicationRecord
# penalty.
attr_accessor :async_permissions_update
- # Fill out zero values that are not in the database
- # this is meant to be implemented in the subclasses as needed.
- def set_zero_values
- ## to do this correctly this is an example:
- ## attributes["runtime_constraints"]["keep_cache_ram"] = 0
- ## self.clear_attribute_changes(["runtime_constraints"])
- ## super # <- we should call arvados_model's set_zero_values()
-
- # Having read the `attributes`, makes serializable attrs to be seen as changed
- # even if they aren't
- self.clear_attribute_changes(changes.keys)
- end
-
# Ignore listed attributes on mass assignments
def self.protected_attributes
[]
@@ -869,6 +855,40 @@ class ArvadosModel < ApplicationRecord
nil
end
+ # Fill in implied zero/false values in database records that were
+ # created before #17014 made them explicit, and reset the Rails
+ # "changed" state so the record doesn't appear to have been modified
+ # after loading.
+ #
+ # 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 in order to (a) ensure every key has a
+ # value in the updated database record and (b) ensure the attribute
+ # whitelist doesn't reject a change from an explicit zero/false
+ # value in the database to an implicit zero/false value in an update
+ # request.
+ 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
commit e337906b3f5d38c25dd6ae58b7855ae3c2dc4744
Author: Nico Cesar <nico at nicocesar.com>
Date: Tue Jan 5 10:49:16 2021 -0500
using getCRfromMockRequest()
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go
index 5e52019f6..cabd3f99e 100644
--- a/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@ -728,29 +728,8 @@ func (s *FederationSuite) TestCreateRemoteContainerRequestCheckSetRuntimeToken(c
req.Header.Set("Content-type", "application/json")
resp := s.testRequest(req).Result()
c.Check(resp.StatusCode, check.Equals, http.StatusOK)
- var cr arvados.ContainerRequest
- // Body can be a json formated or something like:
- // (forceLegacyAPI14==false) cluster_id=zmock&container_request=%7B%22command%22%3A%5B%22abc%22%5D%2C%22container_image%22%3A%22123%22%2C%22...7D
- // or:
- // (forceLegacyAPI14==true) "{\"container_request\":{\"command\":[\"abc\"],\"container_image\":\"12...Uncommitted\"}}"
- data, err := ioutil.ReadAll(s.remoteMockRequests[0].Body)
- c.Check(err, check.IsNil)
-
- // this exposes the different inputs we get in the mock
- if forceLegacyAPI14 {
- var answerCR struct {
- ContainerRequest arvados.ContainerRequest `json:"container_request"`
- }
- c.Check(json.Unmarshal(data, &answerCR), check.IsNil)
- cr = answerCR.ContainerRequest
- } else {
- var decodedValueCR string
- decodedValue, err := url.ParseQuery(string(data))
- c.Check(err, check.IsNil)
- decodedValueCR = decodedValue.Get("container_request")
- c.Check(json.Unmarshal([]byte(decodedValueCR), &cr), check.IsNil)
- }
+ cr := s.getCRfromMockRequest(c)
// After mocking around now making sure the runtime_token we sent is still there.
c.Check(cr.RuntimeToken, check.Equals, "xyz")
commit 888bafe5cfc950c950e4a22681f08456bee095ee
Author: Nico Cesar <nico at nicocesar.com>
Date: Tue Jan 5 10:31:51 2021 -0500
The problem we had in #17014#Note-25 is not there anymore
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go
index 15b929890..d12e4fa33 100644
--- a/lib/controller/handler_test.go
+++ b/lib/controller/handler_test.go
@@ -333,25 +333,21 @@ func (s *HandlerSuite) TestGetObjects(c *check.C) {
ksUUID := ksList.Items[0].UUID
testCases := map[string]map[string]bool{
- "api_clients/" + arvadostest.TrustedWorkbenchAPIClientUUID: nil,
- "api_client_authorizations/" + arvadostest.AdminTokenUUID: nil,
- "authorized_keys/" + arvadostest.AdminAuthorizedKeysUUID: nil,
- "collections/" + arvadostest.CollectionWithUniqueWordsUUID: {"href": true},
- "containers/" + arvadostest.RunningContainerUUID: nil,
- "container_requests/" + arvadostest.QueuedContainerRequestUUID: {
- "runtime_constraints": true,
- "scheduling_parameters": true,
- "modified_by_client_uuid": true,
- }, // see https://dev.arvados.org/issues/17014#note-25
- "groups/" + arvadostest.AProjectUUID: nil,
- "keep_services/" + ksUUID: nil,
- "links/" + arvadostest.ActiveUserCanReadAllUsersLinkUUID: nil,
- "logs/" + arvadostest.CrunchstatForRunningJobLogUUID: nil,
- "nodes/" + arvadostest.IdleNodeUUID: nil,
- "repositories/" + arvadostest.ArvadosRepoUUID: nil,
- "users/" + arvadostest.ActiveUserUUID: {"href": true},
- "virtual_machines/" + arvadostest.TestVMUUID: nil,
- "workflows/" + arvadostest.WorkflowWithDefinitionYAMLUUID: nil,
+ "api_clients/" + arvadostest.TrustedWorkbenchAPIClientUUID: nil,
+ "api_client_authorizations/" + arvadostest.AdminTokenUUID: nil,
+ "authorized_keys/" + arvadostest.AdminAuthorizedKeysUUID: nil,
+ "collections/" + arvadostest.CollectionWithUniqueWordsUUID: {"href": true},
+ "containers/" + arvadostest.RunningContainerUUID: nil,
+ "container_requests/" + arvadostest.QueuedContainerRequestUUID: nil,
+ "groups/" + arvadostest.AProjectUUID: nil,
+ "keep_services/" + ksUUID: nil,
+ "links/" + arvadostest.ActiveUserCanReadAllUsersLinkUUID: nil,
+ "logs/" + arvadostest.CrunchstatForRunningJobLogUUID: nil,
+ "nodes/" + arvadostest.IdleNodeUUID: nil,
+ "repositories/" + arvadostest.ArvadosRepoUUID: nil,
+ "users/" + arvadostest.ActiveUserUUID: {"href": true},
+ "virtual_machines/" + arvadostest.TestVMUUID: nil,
+ "workflows/" + arvadostest.WorkflowWithDefinitionYAMLUUID: nil,
}
for url, skippedFields := range testCases {
s.CheckObjectType(c, "/arvados/v1/"+url, arvadostest.AdminToken, skippedFields)
commit 2879be603902fca7d5e796b78ab2b2f9f27bd309
Author: Nico Cesar <nico at nicocesar.com>
Date: Mon Jan 4 14:47:35 2021 -0500
Preemptible related tests fixed to accomodate new defaults
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index fc1961127..687aa5368 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -108,7 +108,7 @@ class ContainerRequest < ArvadosModel
AttrsSchedulingParametersDefaults = {
"max_run_time"=>0,
"partitions"=>[],
- "preemptible"=>false
+ ## "preemptible"=> Rails.configuration.Containers.UsePreemptibleInstances ## we have to read this on the fly to pass the tests
}
def self.limit_index_columns_read
@@ -492,6 +492,12 @@ class ContainerRequest < ArvadosModel
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
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 a5e946e19..6f0a41158 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
@@ -77,7 +77,7 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
'keep_cache_ram' => 0,
},
scheduling_parameters: {
- "preemptible"=>nil
+ "preemptible"=>false
}
},
}
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index f841de6c3..aed3d9342 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -1011,9 +1011,9 @@ class ContainerRequestTest < ActiveSupport::TestCase
[
[true, 'zzzzz-dz642-runningcontainr', true],
- [true, nil, nil],
- [false, 'zzzzz-dz642-runningcontainr', nil],
- [false, nil, nil],
+ [true, nil, true],
+ [false, 'zzzzz-dz642-runningcontainr', false],
+ [false, nil, false],
].each do |preemptible_conf, requesting_c, schedule_preemptible|
test "having Rails.configuration.Containers.UsePreemptibleInstances=#{preemptible_conf}, #{requesting_c.nil? ? 'non-':''}child CR should #{schedule_preemptible ? '':'not'} ask for preemptible instance by default" do
common_attrs = {cwd: "test",
commit af2ce3cfafb4d4aa3d215b812f22e8db8ff4a9b4
Author: Nico Cesar <nico at nicocesar.com>
Date: Mon Jan 4 13:07:40 2021 -0500
17014: Fixes functional tests.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 0e650bbeb..fc1961127 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -31,6 +31,7 @@ class ContainerRequest < ArvadosModel
serialize :scheduling_parameters, Hash
before_validation :fill_field_defaults, :if => :new_record?
+ before_validation :forget_innocuous_serialized_fields_updates, on: :update
before_validation :validate_runtime_constraints
before_validation :set_default_preemptible_scheduling_parameter
before_validation :set_container
@@ -344,6 +345,19 @@ class ContainerRequest < ArvadosModel
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
@@ -469,12 +483,12 @@ class ContainerRequest < ArvadosModel
# see https://dev.arvados.org/issues/17014#note-28 for details
AttrsRuntimeConstraintsDefaults.each do |key, value|
- if !attributes["runtime_constraints"].key?(key)
+ 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"].key?(key)
+ if attributes["scheduling_parameters"] && !attributes["scheduling_parameters"].key?(key)
attributes["scheduling_parameters"][key] = value
end
end
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 ceb94fe92..a5e946e19 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
@@ -62,7 +62,7 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
assert_equal 'bar', req.secret_mounts['/foo']['content']
end
- test "runtime constraints with default values" do
+ test "cancel with runtime_constraints and scheduling_params with default values" do
authorize_with :active
req = container_requests(:queued)
@@ -77,7 +77,7 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
'keep_cache_ram' => 0,
},
scheduling_parameters: {
- "preemptible"=>false
+ "preemptible"=>nil
}
},
}
commit edaa163c7c65c1a577a9744d7eee6cc1cd46ca6c
Author: Nico Cesar <nico at nicocesar.com>
Date: Mon Jan 4 12:28:38 2021 -0500
Making sure we have no pointers to Preemptible
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/lib/dispatchcloud/node_size.go b/lib/dispatchcloud/node_size.go
index 7278b78d2..fd0486086 100644
--- a/lib/dispatchcloud/node_size.go
+++ b/lib/dispatchcloud/node_size.go
@@ -105,7 +105,7 @@ func ChooseInstanceType(cc *arvados.Cluster, ctr *arvados.Container) (best arvad
case int64(it.Scratch) < needScratch:
case int64(it.RAM) < needRAM:
case it.VCPUs < needVCPUs:
- case ctr.SchedulingParameters && (it.Preemptible != *ctr.SchedulingParameters.Preemptible):
+ case it.Preemptible != ctr.SchedulingParameters.Preemptible:
case it.Price == best.Price && (it.RAM < best.RAM || it.VCPUs < best.VCPUs):
// Equal price, but worse specs
default:
diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index 194f7e6c6..a8d601d5f 100644
--- a/sdk/go/arvados/config.go
+++ b/sdk/go/arvados/config.go
@@ -395,7 +395,7 @@ type InstanceType struct {
IncludedScratch ByteSize
AddedScratch ByteSize
Price float64
- Preemptible *bool
+ Preemptible bool
}
type ContainersConfig struct {
diff --git a/sdk/go/arvados/container.go b/sdk/go/arvados/container.go
index 66c7a478f..3ff7c5205 100644
--- a/sdk/go/arvados/container.go
+++ b/sdk/go/arvados/container.go
@@ -99,7 +99,7 @@ type RuntimeConstraints struct {
// such as Partitions
type SchedulingParameters struct {
Partitions []string `json:"partitions"`
- Preemptible *bool `json:"preemptible"`
+ Preemptible bool `json:"preemptible"`
MaxRunTime int `json:"max_run_time"`
}
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 7012c1c32..0e650bbeb 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -107,7 +107,7 @@ class ContainerRequest < ArvadosModel
AttrsSchedulingParametersDefaults = {
"max_run_time"=>0,
"partitions"=>[],
- "preemptible"=>nil
+ "preemptible"=>false
}
def self.limit_index_columns_read
commit 30c06f38c84496df0954661e06c8e6d321697ef4
Author: Nico Cesar <nico at nicocesar.com>
Date: Mon Jan 4 12:19:45 2021 -0500
Revert "17014: Fixes functional tests."
This reverts commit 22895df84dadaa0c2101443f765a3436b0282643.
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 51fda5469..7012c1c32 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -31,7 +31,6 @@ class ContainerRequest < ArvadosModel
serialize :scheduling_parameters, Hash
before_validation :fill_field_defaults, :if => :new_record?
- before_validation :forget_innocuous_serialized_fields_updates, on: :update
before_validation :validate_runtime_constraints
before_validation :set_default_preemptible_scheduling_parameter
before_validation :set_container
@@ -345,19 +344,6 @@ class ContainerRequest < ArvadosModel
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
@@ -483,12 +469,12 @@ class ContainerRequest < ArvadosModel
# 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)
+ if !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)
+ if !attributes["scheduling_parameters"].key?(key)
attributes["scheduling_parameters"][key] = value
end
end
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 a5e946e19..ceb94fe92 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
@@ -62,7 +62,7 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
assert_equal 'bar', req.secret_mounts['/foo']['content']
end
- test "cancel with runtime_constraints and scheduling_params with default values" do
+ test "runtime constraints with default values" do
authorize_with :active
req = container_requests(:queued)
@@ -77,7 +77,7 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
'keep_cache_ram' => 0,
},
scheduling_parameters: {
- "preemptible"=>nil
+ "preemptible"=>false
}
},
}
commit 34f9f86a5433f51bc05c70613a6615f6c187756c
Author: Nico Cesar <nico at nicocesar.com>
Date: Wed Dec 30 19:25:31 2020 -0500
preemptible is either explicit true or false, or nil (cluster config)
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/lib/dispatchcloud/node_size.go b/lib/dispatchcloud/node_size.go
index e335e9255..7278b78d2 100644
--- a/lib/dispatchcloud/node_size.go
+++ b/lib/dispatchcloud/node_size.go
@@ -105,7 +105,7 @@ func ChooseInstanceType(cc *arvados.Cluster, ctr *arvados.Container) (best arvad
case int64(it.Scratch) < needScratch:
case int64(it.RAM) < needRAM:
case it.VCPUs < needVCPUs:
- case it.Preemptible != *ctr.SchedulingParameters.Preemptible:
+ case ctr.SchedulingParameters && (it.Preemptible != *ctr.SchedulingParameters.Preemptible):
case it.Price == best.Price && (it.RAM < best.RAM || it.VCPUs < best.VCPUs):
// Equal price, but worse specs
default:
diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index a8d601d5f..194f7e6c6 100644
--- a/sdk/go/arvados/config.go
+++ b/sdk/go/arvados/config.go
@@ -395,7 +395,7 @@ type InstanceType struct {
IncludedScratch ByteSize
AddedScratch ByteSize
Price float64
- Preemptible bool
+ Preemptible *bool
}
type ContainersConfig struct {
commit dcbf53c77d06485cdaa28cf25383cff6d3c7bb39
Author: Nico Cesar <nico at nicocesar.com>
Date: Wed Dec 30 17:45:05 2020 -0500
preemptible is either explicit true or false, or nil (cluster config)
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/lib/dispatchcloud/node_size.go b/lib/dispatchcloud/node_size.go
index fd0486086..e335e9255 100644
--- a/lib/dispatchcloud/node_size.go
+++ b/lib/dispatchcloud/node_size.go
@@ -105,7 +105,7 @@ func ChooseInstanceType(cc *arvados.Cluster, ctr *arvados.Container) (best arvad
case int64(it.Scratch) < needScratch:
case int64(it.RAM) < needRAM:
case it.VCPUs < needVCPUs:
- case it.Preemptible != ctr.SchedulingParameters.Preemptible:
+ case it.Preemptible != *ctr.SchedulingParameters.Preemptible:
case it.Price == best.Price && (it.RAM < best.RAM || it.VCPUs < best.VCPUs):
// Equal price, but worse specs
default:
diff --git a/sdk/go/arvados/container.go b/sdk/go/arvados/container.go
index 3ff7c5205..66c7a478f 100644
--- a/sdk/go/arvados/container.go
+++ b/sdk/go/arvados/container.go
@@ -99,7 +99,7 @@ type RuntimeConstraints struct {
// such as Partitions
type SchedulingParameters struct {
Partitions []string `json:"partitions"`
- Preemptible bool `json:"preemptible"`
+ Preemptible *bool `json:"preemptible"`
MaxRunTime int `json:"max_run_time"`
}
commit 22895df84dadaa0c2101443f765a3436b0282643
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Wed Dec 30 18:02:20 2020 -0300
17014: Fixes functional tests.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 7012c1c32..51fda5469 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -31,6 +31,7 @@ class ContainerRequest < ArvadosModel
serialize :scheduling_parameters, Hash
before_validation :fill_field_defaults, :if => :new_record?
+ before_validation :forget_innocuous_serialized_fields_updates, on: :update
before_validation :validate_runtime_constraints
before_validation :set_default_preemptible_scheduling_parameter
before_validation :set_container
@@ -344,6 +345,19 @@ class ContainerRequest < ArvadosModel
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
@@ -469,12 +483,12 @@ class ContainerRequest < ArvadosModel
# see https://dev.arvados.org/issues/17014#note-28 for details
AttrsRuntimeConstraintsDefaults.each do |key, value|
- if !attributes["runtime_constraints"].key?(key)
+ 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"].key?(key)
+ if attributes["scheduling_parameters"] && !attributes["scheduling_parameters"].key?(key)
attributes["scheduling_parameters"][key] = value
end
end
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 ceb94fe92..a5e946e19 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
@@ -62,7 +62,7 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
assert_equal 'bar', req.secret_mounts['/foo']['content']
end
- test "runtime constraints with default values" do
+ test "cancel with runtime_constraints and scheduling_params with default values" do
authorize_with :active
req = container_requests(:queued)
@@ -77,7 +77,7 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
'keep_cache_ram' => 0,
},
scheduling_parameters: {
- "preemptible"=>false
+ "preemptible"=>nil
}
},
}
commit cd1329fbada00e83578725a77d1b59433061c655
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Wed Dec 30 16:20:27 2020 -0300
17014: Fixes one last unit test.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index e12ed4b89..2a9e4e980 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -16,6 +16,7 @@ class ArvadosModel < ApplicationRecord
include DbCurrentTime
extend RecordFilters
+ after_find :set_zero_values
after_find :schedule_restoring_changes
after_initialize :log_start_state
before_save :ensure_permission_to_save
@@ -31,7 +32,6 @@ class ArvadosModel < ApplicationRecord
before_validation :normalize_collection_uuids
before_validation :set_default_owner
validate :ensure_valid_uuids
- after_find :set_defaults
# Note: This only returns permission links. It does not account for
# permissions obtained via user.is_admin or
@@ -47,13 +47,17 @@ class ArvadosModel < ApplicationRecord
# penalty.
attr_accessor :async_permissions_update
- # set_defaults will fill out default values that are not in the database
- # this is meant to be implemented in the children as needed.
- def set_defaults
+ # Fill out zero values that are not in the database
+ # this is meant to be implemented in the subclasses as needed.
+ def set_zero_values
## to do this correctly this is an example:
## attributes["runtime_constraints"]["keep_cache_ram"] = 0
## self.clear_attribute_changes(["runtime_constraints"])
- ## super # <- we should call arvados_model's set_defaults()
+ ## super # <- we should call arvados_model's set_zero_values()
+
+ # Having read the `attributes`, makes serializable attrs to be seen as changed
+ # even if they aren't
+ self.clear_attribute_changes(changes.keys)
end
# Ignore listed attributes on mass assignments
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 350621544..7012c1c32 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -464,8 +464,8 @@ class ContainerRequest < ArvadosModel
super(permitted)
end
- def set_defaults
- # this will fill out default values that are not in the database,
+ 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|
commit 6e8af911900eaf3e402c45405353cb740c580cf2
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Wed Dec 30 15:43:38 2020 -0300
17014: Fixes almost all railsapi's unit tests.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 5833c2251..04ee3ab53 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -206,12 +206,19 @@ 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(runtime_constraints).each do |k, v|
+ defaults.merge(cleaned_rc).each do |k, v|
if v.is_a? Array
rc[k] = v[0]
else
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 02a54d8a2..350621544 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -106,8 +106,8 @@ class ContainerRequest < ArvadosModel
AttrsSchedulingParametersDefaults = {
"max_run_time"=>0,
- "partitions"=>nil,
- "preemptible"=>false
+ "partitions"=>[],
+ "preemptible"=>nil
}
def self.limit_index_columns_read
@@ -122,6 +122,14 @@ 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
@@ -342,7 +350,7 @@ class ContainerRequest < ArvadosModel
[['vcpus', true],
['ram', true],
['keep_cache_ram', false]].each do |k, required|
- if !required && !runtime_constraints.include?(k)
+ if !required && (!runtime_constraints.include?(k) || runtime_constraints[k] == 0)
next
end
v = runtime_constraints[k]
@@ -460,22 +468,17 @@ class ContainerRequest < ArvadosModel
# this will fill out default values that are not in the database,
# see https://dev.arvados.org/issues/17014#note-28 for details
- if self.runtime_constraints?
- AttrsRuntimeConstraintsDefaults.each do |key, value|
- if !self.runtime_constraints.key?(key)
- attributes["runtime_constraints"][key] = value
- self.clear_attribute_changes(["runtime_constraints"])
- end
+ AttrsRuntimeConstraintsDefaults.each do |key, value|
+ if !attributes["runtime_constraints"].key?(key)
+ attributes["runtime_constraints"][key] = value
end
end
- if self.scheduling_parameters?
- AttrsSchedulingParametersDefaults.each do |key, value|
- if !self.scheduling_parameters.key?(key)
- attributes["scheduling_parameters"][key] = value
- end
- self.clear_attribute_changes(["scheduling_parameters"])
+ AttrsSchedulingParametersDefaults.each do |key, value|
+ if !attributes["scheduling_parameters"].key?(key)
+ attributes["scheduling_parameters"][key] = value
end
end
+ self.clear_attribute_changes(["runtime_constraints", "scheduling_parameters"])
super
end
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 90de800b2..f841de6c3 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -153,7 +153,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
cr.reload
- assert_equal({"vcpus" => 2, "ram" => 30}, cr.runtime_constraints)
+ assert ({"vcpus" => 2, "ram" => 30}.to_a - cr.runtime_constraints.to_a).empty?
assert_not_nil cr.container_uuid
c = Container.find_by_uuid cr.container_uuid
@@ -164,7 +164,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
assert_equal({}, c.environment)
assert_equal({"/out" => {"kind"=>"tmp", "capacity"=>1000000}}, c.mounts)
assert_equal "/out", c.output_path
- assert_equal({"keep_cache_ram"=>268435456, "vcpus" => 2, "ram" => 30}, c.runtime_constraints)
+ assert ({"keep_cache_ram"=>268435456, "vcpus" => 2, "ram" => 30}.to_a - c.runtime_constraints.to_a).empty?
assert_operator 0, :<, c.priority
assert_raises(ActiveRecord::RecordInvalid) do
@@ -973,7 +973,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
end
else
cr.save!
- assert_equal sp, cr.scheduling_parameters
+ assert (sp.to_a - cr.scheduling_parameters.to_a).empty?
end
end
end
@@ -1068,11 +1068,11 @@ class ContainerRequestTest < ActiveSupport::TestCase
end
else
cr = create_minimal_req!(common_attrs.merge({state: state}))
- assert_equal sp, cr.scheduling_parameters
+ assert (sp.to_a - cr.scheduling_parameters.to_a).empty?
if state == ContainerRequest::Committed
c = Container.find_by_uuid(cr.container_uuid)
- assert_equal sp, c.scheduling_parameters
+ assert (sp.to_a - c.scheduling_parameters.to_a).empty?
end
end
end
commit 45acfa403ff0bc635443062e3f1577ba2256876a
Author: Nico Cesar <nico at nicocesar.com>
Date: Wed Dec 30 10:58:24 2020 -0500
typo
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index ede4f47b4..02a54d8a2 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -473,10 +473,10 @@ class ContainerRequest < ArvadosModel
if !self.scheduling_parameters.key?(key)
attributes["scheduling_parameters"][key] = value
end
- self.clear_attribute_changes("scheduling_parameters"])
+ self.clear_attribute_changes(["scheduling_parameters"])
end
end
-
+
super
end
commit 50601aa0129ef04a892256d4ede08ed8eb81db54
Author: Nico Cesar <nico at nicocesar.com>
Date: Wed Dec 30 10:45:00 2020 -0500
Added set_defaults to arvados_model to handle zero value in attrs
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 8c1210ab9..ede4f47b4 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -460,17 +460,23 @@ class ContainerRequest < ArvadosModel
# this will fill out default values that are not in the database,
# see https://dev.arvados.org/issues/17014#note-28 for details
- AttrsRuntimeConstraintsDefaults.each do |key, value|
- if !self.runtime_constraints.key?(key)
- attributes["runtime_constraints"][key] = value
+ if self.runtime_constraints?
+ AttrsRuntimeConstraintsDefaults.each do |key, value|
+ if !self.runtime_constraints.key?(key)
+ attributes["runtime_constraints"][key] = value
+ self.clear_attribute_changes(["runtime_constraints"])
+ end
end
end
- AttrsSchedulingParametersDefaults.each do |key, value|
- if !self.scheduling_parameters.key?(key)
- attributes["scheduling_parameters"][key] = value
+ if self.scheduling_parameters?
+ AttrsSchedulingParametersDefaults.each do |key, value|
+ if !self.scheduling_parameters.key?(key)
+ attributes["scheduling_parameters"][key] = value
+ end
+ self.clear_attribute_changes("scheduling_parameters"])
end
end
- self.clear_attribute_changes(["runtime_constraints","scheduling_parameters"])
+
super
end
commit f693516a9f47903e791b3e1c918fcbb6ebf46110
Author: Nico Cesar <nico at nicocesar.com>
Date: Wed Dec 30 09:55:11 2020 -0500
Added set_defaults to arvados_model to handle zero value in attrs
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 6a0a58f08..e12ed4b89 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -31,6 +31,7 @@ class ArvadosModel < ApplicationRecord
before_validation :normalize_collection_uuids
before_validation :set_default_owner
validate :ensure_valid_uuids
+ after_find :set_defaults
# Note: This only returns permission links. It does not account for
# permissions obtained via user.is_admin or
@@ -46,6 +47,15 @@ class ArvadosModel < ApplicationRecord
# penalty.
attr_accessor :async_permissions_update
+ # set_defaults will fill out default values that are not in the database
+ # this is meant to be implemented in the children as needed.
+ def set_defaults
+ ## to do this correctly this is an example:
+ ## attributes["runtime_constraints"]["keep_cache_ram"] = 0
+ ## self.clear_attribute_changes(["runtime_constraints"])
+ ## super # <- we should call arvados_model's set_defaults()
+ end
+
# Ignore listed attributes on mass assignments
def self.protected_attributes
[]
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 2ba175fd9..8c1210ab9 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -97,6 +97,19 @@ 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"=>nil,
+ "preemptible"=>false
+ }
+
def self.limit_index_columns_read
["mounts"]
end
@@ -443,6 +456,24 @@ class ContainerRequest < ArvadosModel
super(permitted)
end
+ def set_defaults
+ # this will fill out default values that are not in the database,
+ # see https://dev.arvados.org/issues/17014#note-28 for details
+
+ AttrsRuntimeConstraintsDefaults.each do |key, value|
+ if !self.runtime_constraints.key?(key)
+ attributes["runtime_constraints"][key] = value
+ end
+ end
+ AttrsSchedulingParametersDefaults.each do |key, value|
+ if !self.scheduling_parameters.key?(key)
+ attributes["scheduling_parameters"][key] = value
+ end
+ 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 95c477f41..ceb94fe92 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
@@ -62,6 +62,28 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase
assert_equal 'bar', req.secret_mounts['/foo']['content']
end
+ test "runtime constraints with default values" do
+ authorize_with :active
+ req = container_requests(:queued)
+
+ patch :update, params: {
+ id: req.uuid,
+ container_request: {
+ state: 'Final',
+ priority: 0,
+ runtime_constraints: {
+ 'vcpus' => 1,
+ 'ram' => 123,
+ 'keep_cache_ram' => 0,
+ },
+ scheduling_parameters: {
+ "preemptible"=>false
+ }
+ },
+ }
+ assert_response :success
+ end
+
test "update without deleting secret_mounts" do
authorize_with :active
req = container_requests(:uncommitted)
commit d5daf2f717d4021aec77e6cd7d36bcd9b849958e
Author: Tom Clegg <tom at curii.com>
Date: Tue Dec 22 03:25:51 2020 -0500
17014: Un-obfuscate.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/crunchrun/crunchrun.go b/lib/crunchrun/crunchrun.go
index ba6fba3ee..730185c19 100644
--- a/lib/crunchrun/crunchrun.go
+++ b/lib/crunchrun/crunchrun.go
@@ -621,7 +621,7 @@ func (runner *ContainerRunner) SetupMounts() (err error) {
return fmt.Errorf("output path does not correspond to a writable mount point")
}
- if wantAPI := runner.Container.RuntimeConstraints.API; needCertMount && wantAPI {
+ if needCertMount && runner.Container.RuntimeConstraints.API {
for _, certfile := range arvadosclient.CertFiles {
_, err := os.Stat(certfile)
if err == nil {
@@ -1092,7 +1092,7 @@ func (runner *ContainerRunner) CreateContainer() error {
},
}
- if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI {
+ if runner.Container.RuntimeConstraints.API {
tok, err := runner.ContainerToken()
if err != nil {
return err
@@ -1269,7 +1269,7 @@ func (runner *ContainerRunner) updateLogs() {
// CaptureOutput saves data from the container's output directory if
// needed, and updates the container output accordingly.
func (runner *ContainerRunner) CaptureOutput() error {
- if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI {
+ if runner.Container.RuntimeConstraints.API {
// Output may have been set directly by the container, so
// refresh the container record to check.
err := runner.DispatcherArvClient.Get("containers", runner.Container.UUID,
commit e1eba75fd362aadc5119e6bebb5efe05ad46d6db
Author: Tom Clegg <tom at curii.com>
Date: Tue Dec 22 03:21:43 2020 -0500
17014: Fix test.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/crunchrun/crunchrun_test.go b/lib/crunchrun/crunchrun_test.go
index a1b33b33d..dbdaa6293 100644
--- a/lib/crunchrun/crunchrun_test.go
+++ b/lib/crunchrun/crunchrun_test.go
@@ -1291,9 +1291,7 @@ func (s *TestSuite) TestSetupMounts(c *C) {
cr.Container.Mounts = make(map[string]arvados.Mount)
cr.Container.Mounts["/tmp"] = arvados.Mount{Kind: "tmp"}
cr.Container.OutputPath = "/tmp"
-
- apiflag := true
- cr.Container.RuntimeConstraints.API = apiflag
+ cr.Container.RuntimeConstraints.API = true
err := cr.SetupMounts()
c.Check(err, IsNil)
@@ -1305,7 +1303,7 @@ func (s *TestSuite) TestSetupMounts(c *C) {
cr.CleanupDirs()
checkEmpty()
- apiflag = false
+ cr.Container.RuntimeConstraints.API = false
}
{
commit cf209afe7018a987bd67a1b20e90b0f31eceba16
Author: Nico Cesar <nico at nicocesar.com>
Date: Mon Dec 21 12:12:31 2020 -0500
addressing https://dev.arvados.org/issues/17014#note-25
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go
index d12e4fa33..15b929890 100644
--- a/lib/controller/handler_test.go
+++ b/lib/controller/handler_test.go
@@ -333,21 +333,25 @@ func (s *HandlerSuite) TestGetObjects(c *check.C) {
ksUUID := ksList.Items[0].UUID
testCases := map[string]map[string]bool{
- "api_clients/" + arvadostest.TrustedWorkbenchAPIClientUUID: nil,
- "api_client_authorizations/" + arvadostest.AdminTokenUUID: nil,
- "authorized_keys/" + arvadostest.AdminAuthorizedKeysUUID: nil,
- "collections/" + arvadostest.CollectionWithUniqueWordsUUID: {"href": true},
- "containers/" + arvadostest.RunningContainerUUID: nil,
- "container_requests/" + arvadostest.QueuedContainerRequestUUID: nil,
- "groups/" + arvadostest.AProjectUUID: nil,
- "keep_services/" + ksUUID: nil,
- "links/" + arvadostest.ActiveUserCanReadAllUsersLinkUUID: nil,
- "logs/" + arvadostest.CrunchstatForRunningJobLogUUID: nil,
- "nodes/" + arvadostest.IdleNodeUUID: nil,
- "repositories/" + arvadostest.ArvadosRepoUUID: nil,
- "users/" + arvadostest.ActiveUserUUID: {"href": true},
- "virtual_machines/" + arvadostest.TestVMUUID: nil,
- "workflows/" + arvadostest.WorkflowWithDefinitionYAMLUUID: nil,
+ "api_clients/" + arvadostest.TrustedWorkbenchAPIClientUUID: nil,
+ "api_client_authorizations/" + arvadostest.AdminTokenUUID: nil,
+ "authorized_keys/" + arvadostest.AdminAuthorizedKeysUUID: nil,
+ "collections/" + arvadostest.CollectionWithUniqueWordsUUID: {"href": true},
+ "containers/" + arvadostest.RunningContainerUUID: nil,
+ "container_requests/" + arvadostest.QueuedContainerRequestUUID: {
+ "runtime_constraints": true,
+ "scheduling_parameters": true,
+ "modified_by_client_uuid": true,
+ }, // see https://dev.arvados.org/issues/17014#note-25
+ "groups/" + arvadostest.AProjectUUID: nil,
+ "keep_services/" + ksUUID: nil,
+ "links/" + arvadostest.ActiveUserCanReadAllUsersLinkUUID: nil,
+ "logs/" + arvadostest.CrunchstatForRunningJobLogUUID: nil,
+ "nodes/" + arvadostest.IdleNodeUUID: nil,
+ "repositories/" + arvadostest.ArvadosRepoUUID: nil,
+ "users/" + arvadostest.ActiveUserUUID: {"href": true},
+ "virtual_machines/" + arvadostest.TestVMUUID: nil,
+ "workflows/" + arvadostest.WorkflowWithDefinitionYAMLUUID: nil,
}
for url, skippedFields := range testCases {
s.CheckObjectType(c, "/arvados/v1/"+url, arvadostest.AdminToken, skippedFields)
diff --git a/lib/crunchrun/crunchrun.go b/lib/crunchrun/crunchrun.go
index 341938354..ba6fba3ee 100644
--- a/lib/crunchrun/crunchrun.go
+++ b/lib/crunchrun/crunchrun.go
@@ -621,7 +621,7 @@ func (runner *ContainerRunner) SetupMounts() (err error) {
return fmt.Errorf("output path does not correspond to a writable mount point")
}
- if wantAPI := runner.Container.RuntimeConstraints.API; needCertMount && wantAPI != nil && *wantAPI {
+ if wantAPI := runner.Container.RuntimeConstraints.API; needCertMount && wantAPI {
for _, certfile := range arvadosclient.CertFiles {
_, err := os.Stat(certfile)
if err == nil {
@@ -1092,7 +1092,7 @@ func (runner *ContainerRunner) CreateContainer() error {
},
}
- if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI != nil && *wantAPI {
+ if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI {
tok, err := runner.ContainerToken()
if err != nil {
return err
@@ -1269,7 +1269,7 @@ func (runner *ContainerRunner) updateLogs() {
// CaptureOutput saves data from the container's output directory if
// needed, and updates the container output accordingly.
func (runner *ContainerRunner) CaptureOutput() error {
- if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI != nil && *wantAPI {
+ if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI {
// Output may have been set directly by the container, so
// refresh the container record to check.
err := runner.DispatcherArvClient.Get("containers", runner.Container.UUID,
diff --git a/lib/crunchrun/crunchrun_test.go b/lib/crunchrun/crunchrun_test.go
index eb83bbd41..a1b33b33d 100644
--- a/lib/crunchrun/crunchrun_test.go
+++ b/lib/crunchrun/crunchrun_test.go
@@ -1293,7 +1293,7 @@ func (s *TestSuite) TestSetupMounts(c *C) {
cr.Container.OutputPath = "/tmp"
apiflag := true
- cr.Container.RuntimeConstraints.API = &apiflag
+ cr.Container.RuntimeConstraints.API = apiflag
err := cr.SetupMounts()
c.Check(err, IsNil)
diff --git a/sdk/go/arvados/container.go b/sdk/go/arvados/container.go
index d5f0b5bb1..3ff7c5205 100644
--- a/sdk/go/arvados/container.go
+++ b/sdk/go/arvados/container.go
@@ -89,18 +89,18 @@ type Mount struct {
// RuntimeConstraints specify a container's compute resources (RAM,
// CPU) and network connectivity.
type RuntimeConstraints struct {
- API *bool `json:",omitempty"`
- RAM int64 `json:"ram,omitempty"`
- VCPUs int `json:"vcpus,omitempty"`
- KeepCacheRAM int64 `json:"keep_cache_ram,omitempty"`
+ API bool `json:"api"`
+ RAM int64 `json:"ram"`
+ VCPUs int `json:"vcpus"`
+ KeepCacheRAM int64 `json:"keep_cache_ram"`
}
// SchedulingParameters specify a container's scheduling parameters
// such as Partitions
type SchedulingParameters struct {
- Partitions []string `json:"partitions,omitempty"`
- Preemptible bool `json:"preemptible,omitempty"`
- MaxRunTime int `json:"max_run_time,omitempty"`
+ Partitions []string `json:"partitions"`
+ Preemptible bool `json:"preemptible"`
+ MaxRunTime int `json:"max_run_time"`
}
// ContainerList is an arvados#containerList resource.
commit 5a5737ba7565b985a20ce1256d09424860e70337
Author: Nico Cesar <nico at nicocesar.com>
Date: Fri Dec 18 12:56:10 2020 -0500
addressing https://dev.arvados.org/issues/17014#note-23
Arvados-DCO-1.1-Signed-off-by: Nico Cesar <nico at curii.com>
diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go
index 2221ce27e..5e52019f6 100644
--- a/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@ -633,6 +633,39 @@ func (s *FederationSuite) TestCreateRemoteContainerRequest(c *check.C) {
c.Check(strings.HasPrefix(cr.UUID, "zzzzz-"), check.Equals, true)
}
+// getCRfromMockRequest returns a ContainerRequest with the content of
+func (s *FederationSuite) getCRfromMockRequest(c *check.C) arvados.ContainerRequest {
+
+ // Body can be a json formated or something like:
+ // cluster_id=zmock&container_request=%7B%22command%22%3A%5B%22abc%22%5D%2C%22container_image%22%3A%22123%22%2C%22...7D
+ // or:
+ // "{\"container_request\":{\"command\":[\"abc\"],\"container_image\":\"12...Uncommitted\"}}"
+
+ var cr arvados.ContainerRequest
+ data, err := ioutil.ReadAll(s.remoteMockRequests[0].Body)
+ c.Check(err, check.IsNil)
+
+ if s.remoteMockRequests[0].Header.Get("Content-Type") == "application/json" {
+ // legacy code path sends a JSON request body
+ var answerCR struct {
+ ContainerRequest arvados.ContainerRequest `json:"container_request"`
+ }
+ c.Check(json.Unmarshal(data, &answerCR), check.IsNil)
+ cr = answerCR.ContainerRequest
+ } else if s.remoteMockRequests[0].Header.Get("Content-Type") == "application/x-www-form-urlencoded" {
+ // new code path sends a form-encoded request body with a JSON-encoded parameter value
+ decodedValue, err := url.ParseQuery(string(data))
+ c.Check(err, check.IsNil)
+ decodedValueCR := decodedValue.Get("container_request")
+ c.Check(json.Unmarshal([]byte(decodedValueCR), &cr), check.IsNil)
+ } else {
+ // mock needs to have Content-Type that we can parse.
+ c.Fail()
+ }
+
+ return cr
+}
+
func (s *FederationSuite) TestCreateRemoteContainerRequestCheckRuntimeToken(c *check.C) {
// Send request to zmock and check that outgoing request has
// runtime_token set with a new random v2 token.
@@ -663,33 +696,12 @@ func (s *FederationSuite) TestCreateRemoteContainerRequestCheckRuntimeToken(c *c
resp := s.testRequest(req).Result()
c.Check(resp.StatusCode, check.Equals, http.StatusOK)
- var cr arvados.ContainerRequest
- // Body can be a json formated or something like:
- // (forceLegacyAPI14==false) cluster_id=zmock&container_request=%7B%22command%22%3A%5B%22abc%22%5D%2C%22container_image%22%3A%22123%22%2C%22...7D
- // or:
- // (forceLegacyAPI14==true) "{\"container_request\":{\"command\":[\"abc\"],\"container_image\":\"12...Uncommitted\"}}"
- data, err := ioutil.ReadAll(s.remoteMockRequests[0].Body)
- c.Check(err, check.IsNil)
-
- // this exposes the different inputs we get in the mock
- if forceLegacyAPI14 {
- var answerCR struct {
- ContainerRequest arvados.ContainerRequest `json:"container_request"`
- }
- c.Check(json.Unmarshal(data, &answerCR), check.IsNil)
- cr = answerCR.ContainerRequest
- } else {
- var decodedValueCR string
- decodedValue, err := url.ParseQuery(string(data))
- c.Check(err, check.IsNil)
- decodedValueCR = decodedValue.Get("container_request")
- c.Check(json.Unmarshal([]byte(decodedValueCR), &cr), check.IsNil)
- }
+ cr := s.getCRfromMockRequest(c)
- // let's make sure the Runtime token is there
- c.Check(strings.HasPrefix(cr.RuntimeToken, "v2/zzzzz-gj3su-"), check.Equals, true)
- // the Runtimetoken should be a different one than than the Token we originally did the request with.
+ // Runtime token must match zzzzz cluster
+ c.Check(cr.RuntimeToken, check.Matches, "v2/zzzzz-gj3su-.*")
+ // RuntimeToken must be different than the Original Token we originally did the request with.
c.Check(cr.RuntimeToken, check.Not(check.Equals), arvadostest.ActiveTokenV2)
}
diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go
index 0567d43fa..9cfef11cb 100644
--- a/lib/controller/integration_test.go
+++ b/lib/controller/integration_test.go
@@ -495,11 +495,6 @@ func (s *IntegrationSuite) TestCreateContainerRequestWithFedToken(c *check.C) {
}
func (s *IntegrationSuite) TestCreateContainerRequestWithBadToken(c *check.C) {
- var (
- body bytes.Buffer
- resp *http.Response
- )
-
conn1 := s.conn("z1111")
rootctx1, _, _ := s.rootClients("z1111")
_, ac1, _, au := s.userClients(rootctx1, c, conn1, "z1111", true)
@@ -515,7 +510,7 @@ func (s *IntegrationSuite) TestCreateContainerRequestWithBadToken(c *check.C) {
{"v2-looking token", "v2/" + au.UUID + "/badtoken00badtoken00badtoken00badtoken00b", http.StatusUnauthorized},
}
- json.NewEncoder(&body).Encode(map[string]interface{}{
+ body, _ := json.Marshal(map[string]interface{}{
"container_request": map[string]interface{}{
"command": []string{"echo"},
"container_image": "d41d8cd98f00b204e9800998ecf8427e+0",
@@ -527,10 +522,10 @@ func (s *IntegrationSuite) TestCreateContainerRequestWithBadToken(c *check.C) {
for _, tt := range tests {
c.Log(c.TestName() + " " + tt.name)
ac1.AuthToken = tt.token
- req, err := http.NewRequest("POST", "https://"+ac1.APIHost+"/arvados/v1/container_requests", bytes.NewReader(body.Bytes()))
+ req, err := http.NewRequest("POST", "https://"+ac1.APIHost+"/arvados/v1/container_requests", bytes.NewReader(body))
c.Assert(err, check.IsNil)
req.Header.Set("Content-Type", "application/json")
- resp, err = ac1.Do(req)
+ resp, err := ac1.Do(req)
c.Assert(err, check.IsNil)
c.Assert(resp.StatusCode, check.Equals, tt.expectedCode)
}
@@ -556,14 +551,14 @@ func (s *IntegrationSuite) dbConn(c *check.C, clusterID string) (*sql.DB, *sql.C
}
// TestRuntimeTokenInCR will test several different tokens in the runtime attribute
-// and check the expected retualts
+// and check the expected results accessing directly to the database if needed.
func (s *IntegrationSuite) TestRuntimeTokenInCR(c *check.C) {
db, dbconn := s.dbConn(c, "z1111")
defer db.Close()
defer dbconn.Close()
conn1 := s.conn("z1111")
rootctx1, _, _ := s.rootClients("z1111")
- _, ac1, _, au := s.userClients(rootctx1, c, conn1, "z1111", true)
+ userctx1, ac1, _, au := s.userClients(rootctx1, c, conn1, "z1111", true)
tests := []struct {
name string
@@ -587,7 +582,7 @@ func (s *IntegrationSuite) TestRuntimeTokenInCR(c *check.C) {
"output_path": "/",
"runtime_token": tt.token,
}
- cr, err := conn1.ContainerRequestCreate(rootctx1, arvados.CreateOptions{Attrs: rq})
+ cr, err := conn1.ContainerRequestCreate(userctx1, arvados.CreateOptions{Attrs: rq})
if tt.expectAToGetAValidCR {
c.Check(err, check.IsNil)
c.Check(cr, check.NotNil)
diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go
index 8898aad04..4bf515fc3 100644
--- a/lib/controller/localdb/login.go
+++ b/lib/controller/localdb/login.go
@@ -142,9 +142,8 @@ func noopLogout(cluster *arvados.Cluster, opts arvados.LogoutOptions) (arvados.L
}
func (conn *Conn) CreateAPIClientAuthorization(ctx context.Context, rootToken string, authinfo rpc.UserSessionAuthInfo) (resp arvados.APIClientAuthorization, err error) {
- // if rootToken is "" then return an error.
if rootToken == "" {
- return arvados.APIClientAuthorization{}, errors.New("In CreateAPIClientAuthorization() rootToken can't be empty string")
+ return arvados.APIClientAuthorization{}, errors.New("configuration error: empty SystemRootToken")
}
ctxRoot := auth.NewContext(ctx, &auth.Credentials{Tokens: []string{rootToken}})
newsession, err := conn.railsProxy.UserSessionCreate(ctxRoot, rpc.UserSessionCreateOptions{
commit 0aec42cb5f4ad6d9593388f3ea5e887ff8b70006
Author: Tom Clegg <tom at curii.com>
Date: Wed Dec 16 16:56:31 2020 -0500
17014: Remove redundant copy of timestamp/zero-value munging code.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/controller/router/response.go b/lib/controller/router/response.go
index df5a40c9f..d554ab930 100644
--- a/lib/controller/router/response.go
+++ b/lib/controller/router/response.go
@@ -109,45 +109,6 @@ func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp i
rtr.mungeItemFields(tmp)
}
- for k, v := range tmp {
- if strings.HasSuffix(k, "_at") {
- // Format non-nil timestamps as
- // rfc3339NanoFixed (by default they will have
- // been encoded to time.RFC3339Nano, which
- // omits trailing zeroes).
- switch tv := v.(type) {
- case *time.Time:
- if tv == nil {
- break
- }
- tmp[k] = tv.Format(rfc3339NanoFixed)
- case time.Time:
- if tv.IsZero() {
- tmp[k] = nil
- } else {
- tmp[k] = tv.Format(rfc3339NanoFixed)
- }
- case string:
- t, err := time.Parse(time.RFC3339Nano, tv)
- if err != nil {
- break
- }
- tmp[k] = t.Format(rfc3339NanoFixed)
- }
- }
- switch k {
- // in all this cases, RoR returns nil instead the Zero value for the type.
- // Maytbe, this should all go away when RoR is out of the picture.
- case "output_uuid", "output_name", "log_uuid", "modified_by_client_uuid", "description", "requesting_container_uuid", "expires_at":
- if v == "" {
- tmp[k] = nil
- }
- case "container_count_max":
- if v == float64(0) {
- tmp[k] = nil
- }
- }
- }
w.Header().Set("Content-Type", "application/json")
enc := json.NewEncoder(w)
enc.SetEscapeHTML(false)
@@ -184,14 +145,16 @@ func (rtr *router) mungeItemFields(tmp map[string]interface{}) {
for k, v := range tmp {
if strings.HasSuffix(k, "_at") {
// Format non-nil timestamps as
- // rfc3339NanoFixed (otherwise they
- // would use the default time encoding).
+ // rfc3339NanoFixed (otherwise they would use
+ // the default time encoding, which omits
+ // trailing zeroes).
switch tv := v.(type) {
case *time.Time:
- if tv == nil {
- break
+ if tv == nil || tv.IsZero() {
+ tmp[k] = nil
+ } else {
+ tmp[k] = tv.Format(rfc3339NanoFixed)
}
- tmp[k] = tv.Format(rfc3339NanoFixed)
case time.Time:
if tv.IsZero() {
tmp[k] = nil
@@ -199,25 +162,21 @@ func (rtr *router) mungeItemFields(tmp map[string]interface{}) {
tmp[k] = tv.Format(rfc3339NanoFixed)
}
case string:
- t, err := time.Parse(time.RFC3339Nano, tv)
- if err != nil {
- break
- }
- if t.IsZero() {
+ if tv == "" {
+ tmp[k] = nil
+ } else if t, err := time.Parse(time.RFC3339Nano, tv); err != nil {
+ // pass through an invalid time value (?)
+ } else if t.IsZero() {
tmp[k] = nil
} else {
tmp[k] = t.Format(rfc3339NanoFixed)
}
}
}
+ // Arvados API spec says when these fields are empty
+ // they appear in responses as null, rather than a
+ // zero value.
switch k {
- // lib/controller/handler_test.go:TestGetObjects tries to test if we break
- // RoR compatibility. The main reason that we keep this transformations is to comply
- // with that test.
- // In some cases the Arvados specification doesn't mention how to treat "" or nil values,
- // as a first step, we'll just try to return the same that railsapi. In the future,
- // when railsapi is not used anymore, this could all be changed to return whatever we define
- // in the specification.
case "output_uuid", "output_name", "log_uuid", "description", "requesting_container_uuid", "container_uuid":
if v == "" {
tmp[k] = nil
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list