From 6828728001af20d8b75d841ead727c47d6ee2c96 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Tue, 21 Mar 2017 11:59:40 -0400 Subject: [PATCH] 7709: De-duplicate "ensure unique name" implementations. --- .../app/controllers/application_controller.rb | 46 +------ services/api/app/models/arvados_model.rb | 51 +++++++ services/api/app/models/container.rb | 93 ++++++++++++- services/api/app/models/container_request.rb | 129 ++---------------- .../api/test/unit/container_request_test.rb | 43 +++--- services/api/test/unit/container_test.rb | 21 ++- 6 files changed, 183 insertions(+), 200 deletions(-) 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/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 6cb9fd8e0e..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,96 +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 = { - 'keep_cache_ram' => - Rails.configuration.container_default_keep_cache_ram, - }.merge(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 @@ -249,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? diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index df3a2c3d9c..b268ce4220 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -312,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 @@ -345,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 @@ -361,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 @@ -377,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 @@ -387,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 @@ -412,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 @@ -428,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 @@ -447,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 @@ -465,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 @@ -477,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 @@ -609,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 diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb index 50f2ec5944..52d2aa6741 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -157,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 @@ -184,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 -- 2.30.2