From fbc576b76977938cf7b742f9770ab90559136dc8 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Wed, 17 May 2017 13:51:57 -0400 Subject: [PATCH] 11546: Avoid loading/saving non-essential fields in /arvados/v1/containers/lock. --- .../arvados/v1/containers_controller.rb | 9 ++++ services/api/app/models/arvados_model.rb | 12 +++++ services/api/app/models/container.rb | 45 +++++++++++++------ .../fixtures/api_client_authorizations.yml | 7 +++ services/api/test/fixtures/containers.yml | 1 + .../arvados/v1/containers_controller_test.rb | 23 ++++++++++ services/api/test/unit/container_test.rb | 17 +++++-- 7 files changed, 97 insertions(+), 17 deletions(-) diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb index 51f15ad84f..3f11b4f5dd 100644 --- a/services/api/app/controllers/arvados/v1/containers_controller.rb +++ b/services/api/app/controllers/arvados/v1/containers_controller.rb @@ -24,6 +24,15 @@ class Arvados::V1::ContainersController < ApplicationController end end + def find_objects_for_index + super + if action_name == 'lock' || action_name == 'unlock' + # Avoid loading more fields than we need + @objects = @objects.select(:id, :uuid, :state, :priority, :auth_uuid, :locked_by_uuid) + @select = %w(uuid state priority auth_uuid locked_by_uuid) + end + end + def lock @object.lock show diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 89f9a8886e..bb33c5595a 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -46,6 +46,12 @@ class ArvadosModel < ActiveRecord::Base end end + class LockFailedError < StandardError + def http_status + 422 + end + end + class InvalidStateTransitionError < StandardError def http_status 422 @@ -98,6 +104,12 @@ class ArvadosModel < ActiveRecord::Base super(self.class.permit_attribute_params(raw_params), *args) end + # Reload "old attributes" for logging, too. + def reload(*args) + super + log_start_state + end + def self.create raw_params={}, *args super(permit_attribute_params(raw_params), *args) end diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index 15a9c501f7..92ccb7c397 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -7,6 +7,7 @@ class Container < ArvadosModel include CommonApiTemplate include WhitelistUpdate extend CurrentApiClient + extend DbCurrentTime serialize :environment, Hash serialize :mounts, Hash @@ -212,26 +213,44 @@ class Container < ArvadosModel nil end + def check_lock_fail + if self.state != Queued + raise LockFailedError.new("cannot lock when #{self.state}") + elsif self.priority <= 0 + raise LockFailedError.new("cannot lock when priority<=0") + end + end + def lock - with_lock do - if self.state == Locked - raise AlreadyLockedError - end - self.state = Locked - self.save! + # Check invalid state transitions once before getting the lock + # (because it's cheaper that way) and once after getting the lock + # (because state might have changed while acquiring the lock). + check_lock_fail + begin + reload(lock: 'FOR UPDATE NOWAIT') + rescue + raise LockFailedError.new("cannot lock: other transaction in progress") end + check_lock_fail + update_attributes!(state: Locked) end - def unlock - with_lock do - if self.state == Queued - raise InvalidStateTransitionError - end - self.state = Queued - self.save! + def check_unlock_fail + if self.state != Locked + raise InvalidStateTransitionError.new("cannot unlock when #{self.state}") + elsif self.locked_by_uuid != current_api_client_authorization.uuid + raise InvalidStateTransitionError.new("locked by a different token") end end + def unlock + # Check invalid state transitions twice (see lock) + check_unlock_fail + reload(lock: 'FOR UPDATE') + check_unlock_fail + update_attributes!(state: Queued) + end + def self.readable_by(*users_list) if users_list.select { |u| u.is_admin }.any? return self diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml index 0b5baf3b9c..40baf46987 100644 --- a/services/api/test/fixtures/api_client_authorizations.yml +++ b/services/api/test/fixtures/api_client_authorizations.yml @@ -285,6 +285,13 @@ dispatch1: api_token: kwi8oowusvbutahacwk2geulqewy5oaqmpalczfna4b6bb0hfw expires_at: 2038-01-01 00:00:00 +dispatch2: + uuid: zzzzz-gj3su-jrriu629zljsnuf + api_client: untrusted + user: system_user + api_token: pbe3v4v5oag83tjwxjh0a551j44xdu8t7ol5ljw3ixsq8oh50q + expires_at: 2038-01-01 00:00:00 + running_container_auth: uuid: zzzzz-gj3su-077z32aux8dg2s2 api_client: untrusted diff --git a/services/api/test/fixtures/containers.yml b/services/api/test/fixtures/containers.yml index 07ccb13428..2a201faefa 100644 --- a/services/api/test/fixtures/containers.yml +++ b/services/api/test/fixtures/containers.yml @@ -57,6 +57,7 @@ locked: uuid: zzzzz-dz642-lockedcontainer owner_uuid: zzzzz-tpzed-000000000000000 state: Locked + locked_by_uuid: zzzzz-gj3su-k9dvestay1plssr priority: 2 created_at: <%= 2.minute.ago.to_s(:db) %> updated_at: <%= 2.minute.ago.to_s(:db) %> diff --git a/services/api/test/functional/arvados/v1/containers_controller_test.rb b/services/api/test/functional/arvados/v1/containers_controller_test.rb index 65a1a915da..1f8a7c4315 100644 --- a/services/api/test/functional/arvados/v1/containers_controller_test.rb +++ b/services/api/test/functional/arvados/v1/containers_controller_test.rb @@ -55,6 +55,14 @@ class Arvados::V1::ContainersControllerTest < ActionController::TestCase uuid = containers(:queued).uuid post :lock, {id: uuid} assert_response :success + assert_nil json_response['mounts'] + assert_nil json_response['command'] + assert_not_nil json_response['auth_uuid'] + assert_not_nil json_response['locked_by_uuid'] + assert_equal containers(:queued).uuid, json_response['uuid'] + assert_equal 'Locked', json_response['state'] + assert_equal containers(:queued).priority, json_response['priority'] + container = Container.where(uuid: uuid).first assert_equal 'Locked', container.state assert_not_nil container.locked_by_uuid @@ -66,12 +74,27 @@ class Arvados::V1::ContainersControllerTest < ActionController::TestCase uuid = containers(:locked).uuid post :unlock, {id: uuid} assert_response :success + assert_nil json_response['mounts'] + assert_nil json_response['command'] + assert_nil json_response['auth_uuid'] + assert_nil json_response['locked_by_uuid'] + assert_equal containers(:locked).uuid, json_response['uuid'] + assert_equal 'Queued', json_response['state'] + assert_equal containers(:locked).priority, json_response['priority'] + container = Container.where(uuid: uuid).first assert_equal 'Queued', container.state assert_nil container.locked_by_uuid assert_nil container.auth_uuid end + test "unlock container locked by different dispatcher" do + authorize_with :dispatch2 + uuid = containers(:locked).uuid + post :unlock, {id: uuid} + assert_response 422 + end + [ [:queued, :lock, :success, 'Locked'], [:queued, :unlock, 422, 'Queued'], diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb index 52d2aa6741..9a859c6229 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -365,7 +365,10 @@ class ContainerTest < ActiveSupport::TestCase set_user_from_auth :dispatch1 assert_equal Container::Queued, c.state - assert_raise(ActiveRecord::RecordInvalid) {c.lock} # "no priority" + assert_raise(ArvadosModel::LockFailedError) do + # "no priority" + c.lock + end c.reload assert cr.update_attributes priority: 1 @@ -378,7 +381,7 @@ class ContainerTest < ActiveSupport::TestCase assert c.locked_by_uuid assert c.auth_uuid - assert_raise(ArvadosModel::AlreadyLockedError) {c.lock} + assert_raise(ArvadosModel::LockFailedError) {c.lock} c.reload assert c.unlock, show_errors(c) @@ -397,9 +400,15 @@ class ContainerTest < ActiveSupport::TestCase auth_uuid_was = c.auth_uuid - assert_raise(ActiveRecord::RecordInvalid) {c.lock} # Running to Locked is not allowed + assert_raise(ArvadosModel::LockFailedError) do + # Running to Locked is not allowed + c.lock + end c.reload - assert_raise(ActiveRecord::RecordInvalid) {c.unlock} # Running to Queued is not allowed + assert_raise(ArvadosModel::InvalidStateTransitionError) do + # Running to Queued is not allowed + c.unlock + end c.reload assert c.update_attributes(state: Container::Complete), show_errors(c) -- 2.39.5