[ARVADOS] updated: fbc576b76977938cf7b742f9770ab90559136dc8
Git user
git at public.curoverse.com
Wed May 17 13:56:29 EDT 2017
Summary of changes:
.../arvados/v1/containers_controller.rb | 11 ++-
services/api/app/models/container.rb | 96 ++++++----------------
.../test/fixtures/api_client_authorizations.yml | 7 ++
.../arvados/v1/containers_controller_test.rb | 29 ++++++-
services/api/test/unit/container_test.rb | 34 ++++----
5 files changed, 81 insertions(+), 96 deletions(-)
discards f3d300ce629f1c17ccc83bf1978ff5c7dd41e33d (commit)
via fbc576b76977938cf7b742f9770ab90559136dc8 (commit)
This update added new revisions after undoing existing revisions. That is
to say, the old revision is not a strict subset of the new revision. This
situation occurs when you --force push a change and generate a repository
containing something like this:
* -- * -- B -- O -- O -- O (f3d300ce629f1c17ccc83bf1978ff5c7dd41e33d)
\
N -- N -- N (fbc576b76977938cf7b742f9770ab90559136dc8)
When this happens we assume that you've already had alert emails for all
of the O revisions, and so we here report only the revisions in the N
branch from the common base, B.
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit fbc576b76977938cf7b742f9770ab90559136dc8
Author: Tom Clegg <tom at curoverse.com>
Date: Wed May 17 13:51:57 2017 -0400
11546: Avoid loading/saving non-essential fields in /arvados/v1/containers/lock.
diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index 51f15ad..3f11b4f 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 89f9a88..bb33c55 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 15a9c50..92ccb7c 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 0b5baf3..40baf46 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 07ccb13..2a201fa 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 65a1a91..1f8a7c4 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 52d2aa6..9a859c6 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)
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list