[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