From: Peter Amstutz Date: Thu, 30 Mar 2017 18:32:06 +0000 (-0400) Subject: Merge branch '11332-fix-crunchscript' refs #11332 X-Git-Tag: 1.1.0~340 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/f34f20a9ac4b7ade7eeb05157a3b190d8a98c37e?hp=c6a8839b1888d2eeb302ddfc675772428b8895a9 Merge branch '11332-fix-crunchscript' refs #11332 --- diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 2072520bb3..71fb365fc6 100644 --- a/services/api/app/controllers/application_controller.rb +++ b/services/api/app/controllers/application_controller.rb @@ -51,8 +51,6 @@ class ApplicationController < ActionController::Base attr_writer :resource_attrs - MAX_UNIQUE_NAME_ATTEMPTS = 10 - begin rescue_from(Exception, ArvadosModel::PermissionDeniedError, @@ -99,50 +97,12 @@ class ApplicationController < ActionController::Base def create @object = model_class.new resource_attrs - if @object.respond_to? :name and params[:ensure_unique_name] - # Record the original name. See below. - name_stem = @object.name - retries = MAX_UNIQUE_NAME_ATTEMPTS + if @object.respond_to?(:name) && params[:ensure_unique_name] + @object.save_with_unique_name! else - retries = 0 - end - - begin @object.save! - rescue ActiveRecord::RecordNotUnique => rn - raise unless retries > 0 - retries -= 1 - - # Dig into the error to determine if it is specifically calling out a - # (owner_uuid, name) uniqueness violation. In this specific case, and - # the client requested a unique name with ensure_unique_name==true, - # update the name field and try to save again. Loop as necessary to - # discover a unique name. It is necessary to handle name choosing at - # this level (as opposed to the client) to ensure that record creation - # never fails due to a race condition. - raise unless rn.original_exception.is_a? PG::UniqueViolation - - # Unfortunately ActiveRecord doesn't abstract out any of the - # necessary information to figure out if this the error is actually - # the specific case where we want to apply the ensure_unique_name - # behavior, so the following code is specialized to Postgres. - err = rn.original_exception - detail = err.result.error_field(PG::Result::PG_DIAG_MESSAGE_DETAIL) - raise unless /^Key \(owner_uuid, name\)=\([a-z0-9]{5}-[a-z0-9]{5}-[a-z0-9]{15}, .*?\) already exists\./.match detail - - @object.uuid = nil - - new_name = "#{name_stem} (#{db_current_time.utc.iso8601(3)})" - if new_name == @object.name - # If the database is fast enough to do two attempts in the - # same millisecond, we need to wait to ensure we try a - # different timestamp on each attempt. - sleep 0.002 - new_name = "#{name_stem} (#{db_current_time.utc.iso8601(3)})" - end - @object.name = new_name - retry end + show end diff --git a/services/api/app/controllers/arvados/v1/nodes_controller.rb b/services/api/app/controllers/arvados/v1/nodes_controller.rb index 5e2404e62c..023d2ff888 100644 --- a/services/api/app/controllers/arvados/v1/nodes_controller.rb +++ b/services/api/app/controllers/arvados/v1/nodes_controller.rb @@ -46,10 +46,12 @@ class Arvados::V1::NodesController < ApplicationController @objects = model_class.where('last_ping_at >= ?', db_current_time - 1.hours) end super - job_uuids = @objects.map { |n| n[:job_uuid] }.compact - assoc_jobs = readable_job_uuids(job_uuids) - @objects.each do |node| - node.job_readable = assoc_jobs.include?(node[:job_uuid]) + if @select.nil? or @select.include? 'job_uuid' + job_uuids = @objects.map { |n| n[:job_uuid] }.compact + assoc_jobs = readable_job_uuids(job_uuids) + @objects = @objects.each do |node| + node.job_readable = assoc_jobs.include?(node[:job_uuid]) + end end end diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 0419dadafb..b77ba1cdf8 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -243,6 +243,57 @@ class ArvadosModel < ActiveRecord::Base permission_link_classes: ['permission', 'resources']) end + def save_with_unique_name! + uuid_was = uuid + name_was = name + max_retries = 2 + transaction do + conn = ActiveRecord::Base.connection + conn.exec_query 'SAVEPOINT save_with_unique_name' + begin + save! + rescue ActiveRecord::RecordNotUnique => rn + raise if max_retries == 0 + max_retries -= 1 + + conn.exec_query 'ROLLBACK TO SAVEPOINT save_with_unique_name' + + # Dig into the error to determine if it is specifically calling out a + # (owner_uuid, name) uniqueness violation. In this specific case, and + # the client requested a unique name with ensure_unique_name==true, + # update the name field and try to save again. Loop as necessary to + # discover a unique name. It is necessary to handle name choosing at + # this level (as opposed to the client) to ensure that record creation + # never fails due to a race condition. + err = rn.original_exception + raise unless err.is_a?(PG::UniqueViolation) + + # Unfortunately ActiveRecord doesn't abstract out any of the + # necessary information to figure out if this the error is actually + # the specific case where we want to apply the ensure_unique_name + # behavior, so the following code is specialized to Postgres. + detail = err.result.error_field(PG::Result::PG_DIAG_MESSAGE_DETAIL) + raise unless /^Key \(owner_uuid, name\)=\([a-z0-9]{5}-[a-z0-9]{5}-[a-z0-9]{15}, .*?\) already exists\./.match detail + + new_name = "#{name_was} (#{db_current_time.utc.iso8601(3)})" + if new_name == name + # If the database is fast enough to do two attempts in the + # same millisecond, we need to wait to ensure we try a + # different timestamp on each attempt. + sleep 0.002 + new_name = "#{name_was} (#{db_current_time.utc.iso8601(3)})" + end + + self[:name] = new_name + self[:uuid] = nil if uuid_was.nil? && !uuid.nil? + conn.exec_query 'SAVEPOINT save_with_unique_name' + retry + ensure + conn.exec_query 'RELEASE SAVEPOINT save_with_unique_name' + end + end + end + def logged_attributes attributes.except(*Rails.configuration.unlogged_attributes) end diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index a3cc9c1071..9420ef3cb8 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -82,15 +82,102 @@ class Container < ArvadosModel end end + # Create a new container (or find an existing one) to satisfy the + # given container request. + def self.resolve(req) + c_attrs = { + command: req.command, + cwd: req.cwd, + environment: req.environment, + output_path: req.output_path, + container_image: resolve_container_image(req.container_image), + mounts: resolve_mounts(req.mounts), + runtime_constraints: resolve_runtime_constraints(req.runtime_constraints), + scheduling_parameters: req.scheduling_parameters, + } + act_as_system_user do + if req.use_existing && (reusable = find_reusable(c_attrs)) + reusable + else + Container.create!(c_attrs) + end + end + end + + # Return a runtime_constraints hash that complies with requested but + # is suitable for saving in a container record, i.e., has specific + # values instead of ranges. + # + # Doing this as a step separate from other resolutions, like "git + # revision range to commit hash", makes sense only when there is no + # opportunity to reuse an existing container (e.g., container reuse + # is not implemented yet, or we have already found that no existing + # containers are suitable). + def self.resolve_runtime_constraints(runtime_constraints) + rc = {} + defaults = { + 'keep_cache_ram' => + Rails.configuration.container_default_keep_cache_ram, + } + defaults.merge(runtime_constraints).each do |k, v| + if v.is_a? Array + rc[k] = v[0] + else + rc[k] = v + end + end + rc + end + + # Return a mounts hash suitable for a Container, i.e., with every + # readonly collection UUID resolved to a PDH. + def self.resolve_mounts(mounts) + c_mounts = {} + mounts.each do |k, mount| + mount = mount.dup + c_mounts[k] = mount + if mount['kind'] != 'collection' + next + end + if (uuid = mount.delete 'uuid') + c = Collection. + readable_by(current_user). + where(uuid: uuid). + select(:portable_data_hash). + first + if !c + raise ArvadosModel::UnresolvableContainerError.new "cannot mount collection #{uuid.inspect}: not found" + end + if mount['portable_data_hash'].nil? + # PDH not supplied by client + mount['portable_data_hash'] = c.portable_data_hash + elsif mount['portable_data_hash'] != c.portable_data_hash + # UUID and PDH supplied by client, but they don't agree + raise ArgumentError.new "cannot mount collection #{uuid.inspect}: current portable_data_hash #{c.portable_data_hash.inspect} does not match #{c['portable_data_hash'].inspect} in request" + end + end + end + return c_mounts + end + + # Return a container_image PDH suitable for a Container. + def self.resolve_container_image(container_image) + coll = Collection.for_latest_docker_image(container_image) + if !coll + raise ArvadosModel::UnresolvableContainerError.new "docker image #{container_image.inspect} not found" + end + coll.portable_data_hash + end + def self.find_reusable(attrs) candidates = Container. where_serialized(:command, attrs[:command]). where('cwd = ?', attrs[:cwd]). where_serialized(:environment, attrs[:environment]). where('output_path = ?', attrs[:output_path]). - where('container_image = ?', attrs[:container_image]). - where_serialized(:mounts, attrs[:mounts]). - where_serialized(:runtime_constraints, attrs[:runtime_constraints]) + where('container_image = ?', resolve_container_image(attrs[:container_image])). + where_serialized(:mounts, resolve_mounts(attrs[:mounts])). + where_serialized(:runtime_constraints, resolve_runtime_constraints(attrs[:runtime_constraints])) # Check for Completed candidates whose output and log are both readable. select_readable_pdh = Collection. diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb index 87c3ebed30..694c174812 100644 --- a/services/api/app/models/container_request.rb +++ b/services/api/app/models/container_request.rb @@ -99,33 +99,16 @@ class ContainerRequest < ArvadosModel manifest = Collection.unscoped do Collection.where(portable_data_hash: pdh).first.manifest_text end - begin - coll = Collection.create!(owner_uuid: owner_uuid, - manifest_text: manifest, - portable_data_hash: pdh, - name: coll_name, - properties: { - 'type' => out_type, - 'container_request' => uuid, - }) - rescue ActiveRecord::RecordNotUnique => rn - # In case this is executed as part of a transaction: When a Postgres exception happens, - # the following statements on the same transaction become invalid, so a rollback is - # needed. One example are Unit Tests, every test is enclosed inside a transaction so - # that the database can be reverted before every new test starts. - # See: http://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html#module-ActiveRecord::Transactions::ClassMethods-label-Exception+handling+and+rolling+back - ActiveRecord::Base.connection.execute 'ROLLBACK' - raise unless out_type == 'output' and self.output_name - # Postgres specific unique name check. See ApplicationController#create for - # a detailed explanation. - raise unless rn.original_exception.is_a? PG::UniqueViolation - err = rn.original_exception - detail = err.result.error_field(PG::Result::PG_DIAG_MESSAGE_DETAIL) - raise unless /^Key \(owner_uuid, name\)=\([a-z0-9]{5}-[a-z0-9]{5}-[a-z0-9]{15}, .*?\) already exists\./.match detail - # Output collection name collision detected: append a timestamp. - coll_name = "#{self.output_name} #{Time.now.getgm.strftime('%FT%TZ')}" - retry - end + + coll = Collection.new(owner_uuid: owner_uuid, + manifest_text: manifest, + portable_data_hash: pdh, + name: coll_name, + properties: { + 'type' => out_type, + 'container_request' => uuid, + }) + coll.save_with_unique_name! if out_type == 'output' out_coll = coll.uuid else @@ -151,93 +134,6 @@ class ContainerRequest < ArvadosModel self.scheduling_parameters ||= {} end - # Create a new container (or find an existing one) to satisfy this - # request. - def resolve - c_mounts = mounts_for_container - c_runtime_constraints = runtime_constraints_for_container - c_container_image = container_image_for_container - c = act_as_system_user do - c_attrs = {command: self.command, - cwd: self.cwd, - environment: self.environment, - output_path: self.output_path, - container_image: c_container_image, - mounts: c_mounts, - runtime_constraints: c_runtime_constraints} - - reusable = self.use_existing ? Container.find_reusable(c_attrs) : nil - if not reusable.nil? - reusable - else - c_attrs[:scheduling_parameters] = self.scheduling_parameters - Container.create!(c_attrs) - end - end - self.container_uuid = c.uuid - end - - # Return a runtime_constraints hash that complies with - # self.runtime_constraints but is suitable for saving in a container - # record, i.e., has specific values instead of ranges. - # - # Doing this as a step separate from other resolutions, like "git - # revision range to commit hash", makes sense only when there is no - # opportunity to reuse an existing container (e.g., container reuse - # is not implemented yet, or we have already found that no existing - # containers are suitable). - def runtime_constraints_for_container - rc = {} - runtime_constraints.each do |k, v| - if v.is_a? Array - rc[k] = v[0] - else - rc[k] = v - end - end - rc - end - - # Return a mounts hash suitable for a Container, i.e., with every - # readonly collection UUID resolved to a PDH. - def mounts_for_container - c_mounts = {} - mounts.each do |k, mount| - mount = mount.dup - c_mounts[k] = mount - if mount['kind'] != 'collection' - next - end - if (uuid = mount.delete 'uuid') - c = Collection. - readable_by(current_user). - where(uuid: uuid). - select(:portable_data_hash). - first - if !c - raise ArvadosModel::UnresolvableContainerError.new "cannot mount collection #{uuid.inspect}: not found" - end - if mount['portable_data_hash'].nil? - # PDH not supplied by client - mount['portable_data_hash'] = c.portable_data_hash - elsif mount['portable_data_hash'] != c.portable_data_hash - # UUID and PDH supplied by client, but they don't agree - raise ArgumentError.new "cannot mount collection #{uuid.inspect}: current portable_data_hash #{c.portable_data_hash.inspect} does not match #{c['portable_data_hash'].inspect} in request" - end - end - end - return c_mounts - end - - # Return a container_image PDH suitable for a Container. - def container_image_for_container - coll = Collection.for_latest_docker_image(container_image) - if !coll - raise ArvadosModel::UnresolvableContainerError.new "docker image #{container_image.inspect} not found" - end - coll.portable_data_hash - end - def set_container if (container_uuid_changed? and not current_user.andand.is_admin and @@ -246,7 +142,7 @@ class ContainerRequest < ArvadosModel return false end if state_changed? and state == Committed and container_uuid.nil? - resolve + self.container_uuid = Container.resolve(self).uuid end if self.container_uuid != self.container_uuid_was if self.container_count_changed? @@ -261,20 +157,17 @@ class ContainerRequest < ArvadosModel def validate_runtime_constraints case self.state when Committed - ['vcpus', 'ram'].each do |k| - if not (runtime_constraints.include? k and - runtime_constraints[k].is_a? Integer and - runtime_constraints[k] > 0) - errors.add :runtime_constraints, "#{k} must be a positive integer" + [['vcpus', true], + ['ram', true], + ['keep_cache_ram', false]].each do |k, required| + if !required && !runtime_constraints.include?(k) + next + end + v = runtime_constraints[k] + unless (v.is_a?(Integer) && v > 0) + errors.add(:runtime_constraints, + "[#{k}]=#{v.inspect} must be a positive integer") end - end - - if runtime_constraints.include? 'keep_cache_ram' and - (!runtime_constraints['keep_cache_ram'].is_a?(Integer) or - runtime_constraints['keep_cache_ram'] <= 0) - errors.add :runtime_constraints, "keep_cache_ram must be a positive integer" - elsif !runtime_constraints.include? 'keep_cache_ram' - runtime_constraints['keep_cache_ram'] = Rails.configuration.container_default_keep_cache_ram end end end diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb index 579b8cc6d0..98643a9e74 100644 --- a/services/api/test/functional/arvados/v1/users_controller_test.rb +++ b/services/api/test/functional/arvados/v1/users_controller_test.rb @@ -6,7 +6,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase include UsersTestHelper setup do - @all_links_at_start = Link.all + @initial_link_count = Link.count @vm_uuid = virtual_machines(:testvm).uuid end @@ -107,7 +107,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase assert_nil created['identity_url'], 'expected no identity_url' # arvados#user, repo link and link add user to 'All users' group - verify_num_links @all_links_at_start, 4 + verify_links_added 4 verify_link response_items, 'arvados#user', true, 'permission', 'can_login', created['uuid'], created['email'], 'arvados#user', false, 'User' @@ -269,7 +269,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase assert_equal response_object['email'], 'foo@example.com', 'expected given email' # four extra links; system_group, login, group and repo perms - verify_num_links @all_links_at_start, 4 + verify_links_added 4 end test "setup user with fake vm and expect error" do @@ -306,7 +306,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase assert_equal response_object['email'], 'foo@example.com', 'expected given email' # five extra links; system_group, login, group, vm, repo - verify_num_links @all_links_at_start, 5 + verify_links_added 5 end test "setup user with valid email, no vm and no repo as input" do @@ -324,7 +324,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase assert_equal response_object['email'], 'foo@example.com', 'expected given email' # three extra links; system_group, login, and group - verify_num_links @all_links_at_start, 3 + verify_links_added 3 verify_link response_items, 'arvados#user', true, 'permission', 'can_login', response_object['uuid'], response_object['email'], 'arvados#user', false, 'User' @@ -361,7 +361,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase 'expecting first name' # five extra links; system_group, login, group, repo and vm - verify_num_links @all_links_at_start, 5 + verify_links_added 5 end test "setup user with an existing user email and check different object is created" do @@ -384,7 +384,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase 'expected different uuid after create operation' assert_equal inactive_user['email'], response_object['email'], 'expected given email' # system_group, openid, group, and repo. No vm link. - verify_num_links @all_links_at_start, 4 + verify_links_added 4 end test "setup user with openid prefix" do @@ -412,7 +412,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase # verify links # four new links: system_group, arvados#user, repo, and 'All users' group. - verify_num_links @all_links_at_start, 4 + verify_links_added 4 verify_link response_items, 'arvados#user', true, 'permission', 'can_login', created['uuid'], created['email'], 'arvados#user', false, 'User' @@ -472,7 +472,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase # five new links: system_group, arvados#user, repo, vm and 'All # users' group link - verify_num_links @all_links_at_start, 5 + verify_links_added 5 verify_link response_items, 'arvados#user', true, 'permission', 'can_login', created['uuid'], created['email'], 'arvados#user', false, 'User' @@ -841,9 +841,9 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase "admin's filtered index did not return inactive user") end - def verify_num_links (original_links, expected_additional_links) - assert_equal expected_additional_links, Link.all.size-original_links.size, - "Expected #{expected_additional_links.inspect} more links" + def verify_links_added more + assert_equal @initial_link_count+more, Link.count, + "Started with #{@initial_link_count} links, expected #{more} more" end def find_obj_in_resp (response_items, object_type, head_kind=nil) diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index af1d4b25fd..b268ce4220 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -123,6 +123,8 @@ class ContainerRequestTest < ActiveSupport::TestCase cr.reload + assert_equal({"vcpus" => 2, "ram" => 30}, cr.runtime_constraints) + assert_not_nil cr.container_uuid c = Container.find_by_uuid cr.container_uuid assert_not_nil c @@ -310,8 +312,7 @@ class ContainerRequestTest < ActiveSupport::TestCase lambda { |resolved| resolved["ram"] == 1234234234 }], ].each do |rc, okfunc| test "resolve runtime constraint range #{rc} to values" do - cr = ContainerRequest.new(runtime_constraints: rc) - resolved = cr.send :runtime_constraints_for_container + resolved = Container.resolve_runtime_constraints(rc) assert(okfunc.call(resolved), "container runtime_constraints was #{resolved.inspect}") end @@ -343,10 +344,9 @@ class ContainerRequestTest < ActiveSupport::TestCase ].each do |mounts, okfunc| test "resolve mounts #{mounts.inspect} to values" do set_user_from_auth :active - cr = ContainerRequest.new(mounts: mounts) - resolved = cr.send :mounts_for_container + resolved = Container.resolve_mounts(mounts) assert(okfunc.call(resolved), - "mounts_for_container returned #{resolved.inspect}") + "Container.resolve_mounts returned #{resolved.inspect}") end end @@ -359,9 +359,8 @@ class ContainerRequestTest < ActiveSupport::TestCase "path" => "/foo", }, } - cr = ContainerRequest.new(mounts: m) assert_raises(ArvadosModel::UnresolvableContainerError) do - cr.send :mounts_for_container + Container.resolve_mounts(m) end end @@ -375,9 +374,8 @@ class ContainerRequestTest < ActiveSupport::TestCase "path" => "/foo", }, } - cr = ContainerRequest.new(mounts: m) assert_raises(ArgumentError) do - cr.send :mounts_for_container + Container.resolve_mounts(m) end end @@ -385,21 +383,19 @@ class ContainerRequestTest < ActiveSupport::TestCase 'arvados/apitestfixture', 'd8309758b8fe2c81034ffc8a10c36460b77db7bc5e7b448c4e5b684f9d95a678', ].each do |tag| - test "container_image_for_container(#{tag.inspect})" do + test "Container.resolve_container_image(#{tag.inspect})" do set_user_from_auth :active - cr = ContainerRequest.new(container_image: tag) - resolved = cr.send :container_image_for_container + resolved = Container.resolve_container_image(tag) assert_equal resolved, collections(:docker_image).portable_data_hash end end - test "container_image_for_container(pdh)" do + test "Container.resolve_container_image(pdh)" do set_user_from_auth :active [[:docker_image, 'v1'], [:docker_image_1_12, 'v2']].each do |coll, ver| Rails.configuration.docker_image_formats = [ver] pdh = collections(coll).portable_data_hash - cr = ContainerRequest.new(container_image: pdh) - resolved = cr.send :container_image_for_container + resolved = Container.resolve_container_image(pdh) assert_equal resolved, pdh end end @@ -410,9 +406,8 @@ class ContainerRequestTest < ActiveSupport::TestCase ].each do |img| test "container_image_for_container(#{img.inspect}) => 422" do set_user_from_auth :active - cr = ContainerRequest.new(container_image: img) assert_raises(ArvadosModel::UnresolvableContainerError) do - cr.send :container_image_for_container + Container.resolve_container_image(img) end end end @@ -426,12 +421,12 @@ class ContainerRequestTest < ActiveSupport::TestCase set_user_from_auth :active cr = create_minimal_req!(command: ["true", "1"], container_image: collections(:docker_image).portable_data_hash) - assert_equal(cr.send(:container_image_for_container), + assert_equal(Container.resolve_container_image(cr.container_image), collections(:docker_image_1_12).portable_data_hash) cr = create_minimal_req!(command: ["true", "2"], container_image: links(:docker_image_collection_tag).name) - assert_equal(cr.send(:container_image_for_container), + assert_equal(Container.resolve_container_image(cr.container_image), collections(:docker_image_1_12).portable_data_hash) end @@ -445,12 +440,12 @@ class ContainerRequestTest < ActiveSupport::TestCase set_user_from_auth :active cr = create_minimal_req!(command: ["true", "1"], container_image: collections(:docker_image).portable_data_hash) - assert_equal(cr.send(:container_image_for_container), + assert_equal(Container.resolve_container_image(cr.container_image), collections(:docker_image).portable_data_hash) cr = create_minimal_req!(command: ["true", "2"], container_image: links(:docker_image_collection_tag).name) - assert_equal(cr.send(:container_image_for_container), + assert_equal(Container.resolve_container_image(cr.container_image), collections(:docker_image).portable_data_hash) end @@ -463,7 +458,7 @@ class ContainerRequestTest < ActiveSupport::TestCase cr = create_minimal_req!(command: ["true", "1"], container_image: collections(:docker_image_1_12).portable_data_hash) assert_raises(ArvadosModel::UnresolvableContainerError) do - cr.send(:container_image_for_container) + Container.resolve_container_image(cr.container_image) end end @@ -475,12 +470,12 @@ class ContainerRequestTest < ActiveSupport::TestCase cr = create_minimal_req!(command: ["true", "1"], container_image: collections(:docker_image).portable_data_hash) assert_raises(ArvadosModel::UnresolvableContainerError) do - cr.send(:container_image_for_container) + Container.resolve_container_image(cr.container_image) end cr = create_minimal_req!(command: ["true", "2"], container_image: links(:docker_image_collection_tag).name) assert_raises(ArvadosModel::UnresolvableContainerError) do - cr.send(:container_image_for_container) + Container.resolve_container_image(cr.container_image) end end @@ -502,8 +497,7 @@ class ContainerRequestTest < ActiveSupport::TestCase command: ["echo", "hello"], output_path: "test", runtime_constraints: {"vcpus" => 4, - "ram" => 12000000000, - "keep_cache_ram" => 268435456}, + "ram" => 12000000000}, mounts: {"test" => {"kind" => "json"}}} set_user_from_auth :active cr1 = create_minimal_req!(common_attrs.merge({state: ContainerRequest::Committed, @@ -608,7 +602,7 @@ class ContainerRequestTest < ActiveSupport::TestCase "It shouldn't exist more than one collection with the same owner and name '${output_name}'" assert output_coll.name.include?(output_name), "New name should include original name" - assert_match /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z/, output_coll.name, + assert_match /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z/, output_coll.name, "New name should include ISO8601 date" end @@ -640,34 +634,6 @@ class ContainerRequestTest < ActiveSupport::TestCase assert_equal ContainerRequest::Final, cr3.state end - [ - [{"vcpus" => 1, "ram" => 123, "keep_cache_ram" => 100}, ContainerRequest::Committed, 100], - [{"vcpus" => 1, "ram" => 123}, ContainerRequest::Uncommitted], - [{"vcpus" => 1, "ram" => 123}, ContainerRequest::Committed], - [{"vcpus" => 1, "ram" => 123, "keep_cache_ram" => -1}, ContainerRequest::Committed, ActiveRecord::RecordInvalid], - [{"vcpus" => 1, "ram" => 123, "keep_cache_ram" => '123'}, ContainerRequest::Committed, ActiveRecord::RecordInvalid], - ].each do |rc, state, expected| - test "create container request with #{rc} in state #{state} and verify keep_cache_ram #{expected}" do - common_attrs = {cwd: "test", - priority: 1, - command: ["echo", "hello"], - output_path: "test", - runtime_constraints: rc, - mounts: {"test" => {"kind" => "json"}}} - set_user_from_auth :active - - if expected == ActiveRecord::RecordInvalid - assert_raises(ActiveRecord::RecordInvalid) do - create_minimal_req!(common_attrs.merge({state: state})) - end - else - cr = create_minimal_req!(common_attrs.merge({state: state})) - expected = Rails.configuration.container_default_keep_cache_ram if state == ContainerRequest::Committed and expected.nil? - assert_equal expected, cr.runtime_constraints['keep_cache_ram'] - end - end - end - [ [{"partitions" => ["fastcpu","vfastcpu", 100]}, ContainerRequest::Committed, ActiveRecord::RecordInvalid], [{"partitions" => ["fastcpu","vfastcpu", 100]}, ContainerRequest::Uncommitted], diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb index 5a19f05ee4..52d2aa6741 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -11,14 +11,22 @@ class ContainerTest < ActiveSupport::TestCase runtime_constraints: {"vcpus" => 1, "ram" => 1}, } - REUSABLE_COMMON_ATTRS = {container_image: "9ae44d5792468c58bcf85ce7353c7027+124", - cwd: "test", - command: ["echo", "hello"], - output_path: "test", - runtime_constraints: {"vcpus" => 4, - "ram" => 12000000000}, - mounts: {"test" => {"kind" => "json"}}, - environment: {"var" => 'val'}} + REUSABLE_COMMON_ATTRS = { + container_image: "9ae44d5792468c58bcf85ce7353c7027+124", + cwd: "test", + command: ["echo", "hello"], + output_path: "test", + runtime_constraints: { + "ram" => 12000000000, + "vcpus" => 4, + }, + mounts: { + "test" => {"kind" => "json"}, + }, + environment: { + "var" => "val", + }, + } def minimal_new attrs={} cr = ContainerRequest.new DEFAULT_ATTRS.merge(attrs) @@ -86,7 +94,7 @@ class ContainerTest < ActiveSupport::TestCase test "Container serialized hash attributes sorted before save" do env = {"C" => 3, "B" => 2, "A" => 1} m = {"F" => {"kind" => 3}, "E" => {"kind" => 2}, "D" => {"kind" => 1}} - rc = {"vcpus" => 1, "ram" => 1} + rc = {"vcpus" => 1, "ram" => 1, "keep_cache_ram" => 1} 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 @@ -149,21 +157,21 @@ class ContainerTest < ActiveSupport::TestCase log: 'ea10d51bcf88862dbcc36eb292017dfd+45', } - set_user_from_auth :dispatch1 - - c_output1 = Container.create common_attrs - c_output2 = Container.create common_attrs - assert_not_equal c_output1.uuid, c_output2.uuid - cr = ContainerRequest.new common_attrs + cr.use_existing = false cr.state = ContainerRequest::Committed - cr.container_uuid = c_output1.uuid cr.save! + c_output1 = Container.where(uuid: cr.container_uuid).first cr = ContainerRequest.new common_attrs + cr.use_existing = false cr.state = ContainerRequest::Committed - cr.container_uuid = c_output2.uuid cr.save! + c_output2 = Container.where(uuid: cr.container_uuid).first + + assert_not_equal c_output1.uuid, c_output2.uuid + + set_user_from_auth :dispatch1 out1 = '1f4b0bc7583c2a7f9102c395f4ffc5e3+45' log1 = collections(:real_log_collection).portable_data_hash @@ -176,9 +184,8 @@ class ContainerTest < ActiveSupport::TestCase c_output2.update_attributes!({state: Container::Running}) c_output2.update_attributes!(completed_attrs.merge({log: log1, output: out2})) - reused = Container.find_reusable(common_attrs) - assert_not_nil reused - assert_equal reused.uuid, c_output1.uuid + reused = Container.resolve(ContainerRequest.new(common_attrs)) + assert_equal c_output1.uuid, reused.uuid end test "find_reusable method should select running container by start date" do diff --git a/services/fuse/arvados_fuse/command.py b/services/fuse/arvados_fuse/command.py index 45e902f87c..fea6048798 100644 --- a/services/fuse/arvados_fuse/command.py +++ b/services/fuse/arvados_fuse/command.py @@ -11,6 +11,7 @@ import sys import time import arvados.commands._util as arv_cmd +from arvados_fuse import crunchstat from arvados_fuse import * from arvados_fuse.unmount import unmount from arvados_fuse._version import __version__ diff --git a/services/fuse/tests/test_crunchstat.py b/services/fuse/tests/test_crunchstat.py new file mode 100644 index 0000000000..1fa28fb2d4 --- /dev/null +++ b/services/fuse/tests/test_crunchstat.py @@ -0,0 +1,13 @@ +import subprocess + +from integration_test import IntegrationTest + + +class CrunchstatTest(IntegrationTest): + def test_crunchstat(self): + output = subprocess.check_output( + ['./bin/arv-mount', + '--crunchstat-interval', '1', + self.mnt, + '--exec', 'echo', 'ok']) + self.assertEqual("ok\n", output)