[ARVADOS] updated: f3d300ce629f1c17ccc83bf1978ff5c7dd41e33d
Git user
git at public.curoverse.com
Wed May 17 10:52:03 EDT 2017
Summary of changes:
services/api/app/controllers/arvados/v1/containers_controller.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
discards 7a02915c8a8454fc286309b71dfbe349c64ba1b0 (commit)
via f3d300ce629f1c17ccc83bf1978ff5c7dd41e33d (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 (7a02915c8a8454fc286309b71dfbe349c64ba1b0)
\
N -- N -- N (f3d300ce629f1c17ccc83bf1978ff5c7dd41e33d)
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 f3d300ce629f1c17ccc83bf1978ff5c7dd41e33d
Author: Tom Clegg <tom at curoverse.com>
Date: Wed May 17 10:49:48 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..a05ea50 100644
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -24,8 +24,18 @@ class Arvados::V1::ContainersController < ApplicationController
end
end
+ def find_objects_for_index
+ if action_name == 'lock'
+ # Avoid loading more fields than we need
+ @select = %w(uuid state auth_uuid locked_by_uuid)
+ end
+ super
+ end
+
def lock
- @object.lock
+ Container.lock_container(uuid: @object.uuid)
+ # Reload (but still only the essential fields)
+ find_object_by_uuid
show
end
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..5dd1e4e 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,23 +213,85 @@ class Container < ArvadosModel
nil
end
- def lock
- with_lock do
- if self.state == Locked
- raise AlreadyLockedError
+ def lock_and_reload
+ self.class.lock_container(uuid: self.uuid)
+ self.id = self.class.where(uuid: self.uuid).select(:id).first.id
+ self.reload
+ end
+
+ # This class method should be used instead of the instance method
+ # when possible. It avoids a "reload entire AR object" operation,
+ # which can be expensive -- especially when it occurs inside a
+ # transaction with uncommitted writes, in which case it can block
+ # other readers.
+ def self.lock_container(uuid:)
+ c = select(:id, :uuid, :state, :priority,
+ :updated_at, :modified_at, :modified_by_user_uuid,
+ :owner_uuid).
+ where(uuid: uuid).
+ first
+ if !c
+ raise ActiveRecord::RecordNotFound
+ elsif c.priority <= 0
+ raise InvalidStateTransitionError
+ end
+
+ if c.state == Locked
+ raise LockFailedError.new("cannot lock: already locked")
+ elsif c.state != Queued
+ raise InvalidStateTransitionError
+ end
+ cr = ContainerRequest.
+ where('container_uuid=? and priority>0', uuid).
+ order('priority desc').
+ first
+ if !cr
+ raise LockFailedError.new("cannot lock: priority <= 0")
+ end
+
+ locked_by_uuid = current_api_client_authorization.uuid
+
+ transaction do
+ # Create a new auth token for the container. This will be rolled
+ # back if we end up raising LockFailedError.
+ auth_uuid = ApiClientAuthorization.
+ create!(user_id: User.find_by_uuid(cr.modified_by_user_uuid).id,
+ api_client_id: 0).
+ uuid
+
+ now = db_current_time.utc.iso8601(9)
+ r = ActiveRecord::Base.connection.
+ execute("UPDATE containers "+
+ "SET state='#{Locked}', auth_uuid='#{auth_uuid}', "+
+ "locked_by_uuid='#{locked_by_uuid}', modified_at='#{now}' "+
+ "WHERE uuid='#{uuid}' AND state='#{Queued}' and priority>0")
+ if r.cmd_status != 'UPDATE 1'
+ if r.cmd_status != 'UPDATE 0'
+ Rails.logger.error("unexpected db result: #{r.cmd_status.inspect}")
+ end
+ raise LockFailedError.new("cannot lock: lost race with another caller")
end
- self.state = Locked
- self.save!
+ c["updated_at"] = now
+ c["modified_at"] = now
+ c["modified_by_user_uuid"] = current_user.uuid
+ c["auth_uuid"] = auth_uuid
+ c["locked_by_uuid"] = locked_by_uuid
+ c["state"] = Locked
+ c.send(:log_update)
end
end
def unlock
+ 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
with_lock do
- if self.state == Queued
- raise InvalidStateTransitionError
+ if self.state != Locked
+ raise InvalidStateTransitionError.new("cannot unlock when #{self.state}")
end
- self.state = Queued
- self.save!
+ self.update_attributes!(state: Queued)
end
end
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..5aa7ea7 100644
--- a/services/api/test/functional/arvados/v1/containers_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/containers_controller_test.rb
@@ -24,7 +24,7 @@ class Arvados::V1::ContainersControllerTest < ActionController::TestCase
test 'cannot get auth with wrong token' do
authorize_with :dispatch1
c = containers(:queued)
- assert c.lock, show_errors(c)
+ assert c.lock_and_reload, show_errors(c)
authorize_with :system_user
get :auth, id: c.uuid
@@ -34,7 +34,7 @@ class Arvados::V1::ContainersControllerTest < ActionController::TestCase
test 'get auth' do
authorize_with :dispatch1
c = containers(:queued)
- assert c.lock, show_errors(c)
+ assert c.lock_and_reload, show_errors(c)
get :auth, id: c.uuid
assert_response :success
assert_operator 32, :<, json_response['api_token'].length
@@ -44,7 +44,7 @@ class Arvados::V1::ContainersControllerTest < ActionController::TestCase
test 'no auth in container response' do
authorize_with :dispatch1
c = containers(:queued)
- assert c.lock, show_errors(c)
+ assert c.lock_and_reload, show_errors(c)
get :show, id: c.uuid
assert_response :success
assert_nil json_response['auth']
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 52d2aa6..7126249 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -347,7 +347,7 @@ class ContainerTest < ActiveSupport::TestCase
check_illegal_updates c, [{state: Container::Running},
{state: Container::Complete}]
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
check_illegal_modify c
@@ -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::InvalidStateTransitionError) do
+ # "no priority"
+ c.lock_and_reload
+ end
c.reload
assert cr.update_attributes priority: 1
@@ -374,11 +377,11 @@ class ContainerTest < ActiveSupport::TestCase
refute c.update_attributes(state: Container::Complete), "not locked"
c.reload
- assert c.lock, show_errors(c)
+ assert c.lock_and_reload, show_errors(c)
assert c.locked_by_uuid
assert c.auth_uuid
- assert_raise(ArvadosModel::AlreadyLockedError) {c.lock}
+ assert_raise(ArvadosModel::LockFailedError) {c.lock_and_reload}
c.reload
assert c.unlock, show_errors(c)
@@ -390,16 +393,22 @@ class ContainerTest < ActiveSupport::TestCase
refute c.locked_by_uuid
refute c.auth_uuid
- assert c.lock, show_errors(c)
+ assert c.lock_and_reload, show_errors(c)
assert c.update_attributes(state: Container::Running), show_errors(c)
assert c.locked_by_uuid
assert c.auth_uuid
auth_uuid_was = c.auth_uuid
- assert_raise(ActiveRecord::RecordInvalid) {c.lock} # Running to Locked is not allowed
+ assert_raise(ArvadosModel::InvalidStateTransitionError) do
+ # Running to Locked is not allowed
+ c.lock_and_reload
+ 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)
@@ -420,7 +429,7 @@ class ContainerTest < ActiveSupport::TestCase
test "Container locked cancel" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- assert c.lock, show_errors(c)
+ assert c.lock_and_reload, show_errors(c)
assert c.update_attributes(state: Container::Cancelled), show_errors(c)
check_no_change_from_cancelled c
end
@@ -428,7 +437,7 @@ class ContainerTest < ActiveSupport::TestCase
test "Container running cancel" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
c.update_attributes! state: Container::Cancelled
check_no_change_from_cancelled c
@@ -450,7 +459,7 @@ class ContainerTest < ActiveSupport::TestCase
test "Container only set exit code on complete" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
check_illegal_updates c, [{exit_code: 1},
@@ -462,7 +471,7 @@ class ContainerTest < ActiveSupport::TestCase
test "locked_by_uuid can set output on running container" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
assert_equal c.locked_by_uuid, Thread.current[:api_client_authorization].uuid
@@ -474,7 +483,7 @@ class ContainerTest < ActiveSupport::TestCase
test "auth_uuid can set output on running container, but not change container state" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
Thread.current[:api_client_authorization] = ApiClientAuthorization.find_by_uuid(c.auth_uuid)
@@ -490,7 +499,7 @@ class ContainerTest < ActiveSupport::TestCase
test "not allowed to set output that is not readable by current user" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
Thread.current[:api_client_authorization] = ApiClientAuthorization.find_by_uuid(c.auth_uuid)
@@ -504,7 +513,7 @@ class ContainerTest < ActiveSupport::TestCase
test "other token cannot set output on running container" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
set_user_from_auth :not_running_container_auth
@@ -516,7 +525,7 @@ class ContainerTest < ActiveSupport::TestCase
test "can set trashed output on running container" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
output = Collection.unscoped.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3jk')
@@ -529,7 +538,7 @@ class ContainerTest < ActiveSupport::TestCase
test "not allowed to set trashed output that is not readable by current user" do
c, _ = minimal_new
set_user_from_auth :dispatch1
- c.lock
+ c.lock_and_reload
c.update_attributes! state: Container::Running
output = Collection.unscoped.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3jr')
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list