[ARVADOS] updated: bbb8b8eee1aa7d74a76bc52c6bd1caa48d89cb5b
Git user
git at public.curoverse.com
Wed May 17 10:24:32 EDT 2017
Summary of changes:
services/api/app/controllers/application_controller.rb | 2 +-
services/api/app/models/arvados_model.rb | 1 +
services/api/app/models/container.rb | 12 +++++++-----
3 files changed, 9 insertions(+), 6 deletions(-)
discards dfc18fd74a0e74ce5228196602ad6ed3bb0c2df0 (commit)
via bbb8b8eee1aa7d74a76bc52c6bd1caa48d89cb5b (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 (dfc18fd74a0e74ce5228196602ad6ed3bb0c2df0)
\
N -- N -- N (bbb8b8eee1aa7d74a76bc52c6bd1caa48d89cb5b)
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 bbb8b8eee1aa7d74a76bc52c6bd1caa48d89cb5b
Author: Tom Clegg <tom at curoverse.com>
Date: Wed May 17 10:24:19 2017 -0400
11546: Avoid loading/saving non-essential fields in /arvados/v1/containers/lock.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index b6816d3..09b67c4 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -251,7 +251,7 @@ class ApplicationController < ActionController::Base
# Map attribute names in @select to real column names, resolve
# those to fully-qualified SQL column names, and pass the
# resulting string to the select method.
- columns_list = model_class.columns_for_attributes(@select).
+ columns_list = ["id"] + model_class.columns_for_attributes(@select).
map { |s| "#{ar_table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }
@objects = @objects.select(columns_list.join(", "))
end
diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index 51f15ad..00d161f 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' || action_name == 'unlock'
+ # 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..acc2126 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,82 @@ 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.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 ActiveRecord::RecordInvalid.new("cannot lock when priority<=0")
+ 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}")
+ 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/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..c9fbf11 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,7 @@ 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(ActiveRecord::RecordInvalid) {c.lock_and_reload} # "no priority"
c.reload
assert cr.update_attributes priority: 1
@@ -374,11 +374,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::AlreadyLockedError) {c.lock_and_reload}
c.reload
assert c.unlock, show_errors(c)
@@ -390,14 +390,14 @@ 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(ActiveRecord::RecordInvalid) {c.lock_and_reload} # Running to Locked is not allowed
c.reload
assert_raise(ActiveRecord::RecordInvalid) {c.unlock} # Running to Queued is not allowed
c.reload
@@ -420,7 +420,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 +428,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 +450,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 +462,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 +474,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 +490,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 +504,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 +516,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 +529,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