[ARVADOS] updated: dfc18fd74a0e74ce5228196602ad6ed3bb0c2df0
Git user
git at public.curoverse.com
Wed May 17 10:12:38 EDT 2017
Summary of changes:
services/crunch-run/crunchrun.go | 94 +++++++++++++++++++++++++++++++----
services/crunch-run/crunchrun_test.go | 83 ++++++++++++++++++++++++++++++-
2 files changed, 165 insertions(+), 12 deletions(-)
discards 285323e01f41e684a5ca2c972cc4357e3af3701c (commit)
via dfc18fd74a0e74ce5228196602ad6ed3bb0c2df0 (commit)
via f26f70d0a60798065c5f7a5cb91b95587cc9e9ef (commit)
via aceb2d1ed239fa82fcb8bb352b632a8d92251dac (commit)
via a340487a7d406e73e51479a765a3d08bdb92b8d0 (commit)
via 94b92f075dbfb60a25fbe28e5741a553ac4985fd (commit)
via eee2c470dfa879c769eccb515861419a6b900101 (commit)
via 34f9129a3d7d2a625455fccbd01c94fc18f6685a (commit)
via 1903e0e26b3677d9686e1d19cea897690945e3ed (commit)
via bbe86c4a80d53807b325b46dd51557a7a01670ae (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 (285323e01f41e684a5ca2c972cc4357e3af3701c)
\
N -- N -- N (dfc18fd74a0e74ce5228196602ad6ed3bb0c2df0)
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 dfc18fd74a0e74ce5228196602ad6ed3bb0c2df0
Author: Tom Clegg <tom at curoverse.com>
Date: Wed May 17 10:11:55 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..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..9a19b48 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,11 @@ class ArvadosModel < ActiveRecord::Base
super(self.class.permit_attribute_params(raw_params), *args)
end
+ 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..f027001 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,80 @@ 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, :updated_at,
+ :modified_at, :modified_by_user_uuid,
+ :owner_uuid).
+ where('uuid=? and priority>0', uuid).
+ first
+ if !c
+ raise ActiveRecord::RecordNotFound
+ 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
+ end
with_lock do
- if self.state == Queued
+ if self.state != Locked
raise InvalidStateTransitionError
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