14807: Merge branch 'master'

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --combined lib/controller/federation_test.go
index 79b534dc86c3e68066bcc3e52cdea69acb880c62,d49d16a35eee1be2fe3cbe0f765921316fdafa64..62916acd2ac10be14d90d4e02e2703e77949e32b
--- 1/lib/controller/federation_test.go
--- 2/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@@ -19,7 -19,6 +19,7 @@@ import 
  
        "git.curoverse.com/arvados.git/sdk/go/arvados"
        "git.curoverse.com/arvados.git/sdk/go/arvadostest"
 +      "git.curoverse.com/arvados.git/sdk/go/ctxlog"
        "git.curoverse.com/arvados.git/sdk/go/httpserver"
        "git.curoverse.com/arvados.git/sdk/go/keepclient"
        "github.com/sirupsen/logrus"
@@@ -30,7 -29,7 +30,7 @@@
  var _ = check.Suite(&FederationSuite{})
  
  type FederationSuite struct {
 -      log *logrus.Logger
 +      log logrus.FieldLogger
        // testServer and testHandler are the controller being tested,
        // "zhome".
        testServer  *httpserver.Server
@@@ -45,7 -44,9 +45,7 @@@
  }
  
  func (s *FederationSuite) SetUpTest(c *check.C) {
 -      s.log = logrus.New()
 -      s.log.Formatter = &logrus.JSONFormatter{}
 -      s.log.Out = &logWriter{c.Log}
 +      s.log = ctxlog.TestLogger(c)
  
        s.remoteServer = newServerFromIntegrationTestEnv(c)
        c.Assert(s.remoteServer.Start(), check.IsNil)
@@@ -554,16 -555,20 +554,20 @@@ func (s *FederationSuite) TestGetRemote
  
  func (s *FederationSuite) TestUpdateRemoteContainerRequest(c *check.C) {
        defer s.localServiceReturns404(c).Close()
-       req := httptest.NewRequest("PATCH", "/arvados/v1/container_requests/"+arvadostest.QueuedContainerRequestUUID,
-               strings.NewReader(`{"container_request": {"priority": 696}}`))
-       req.Header.Set("Authorization", "Bearer "+arvadostest.ActiveToken)
-       req.Header.Set("Content-type", "application/json")
-       resp := s.testRequest(req)
-       c.Check(resp.StatusCode, check.Equals, http.StatusOK)
-       var cr arvados.ContainerRequest
-       c.Check(json.NewDecoder(resp.Body).Decode(&cr), check.IsNil)
-       c.Check(cr.UUID, check.Equals, arvadostest.QueuedContainerRequestUUID)
-       c.Check(cr.Priority, check.Equals, 696)
+       setPri := func(pri int) {
+               req := httptest.NewRequest("PATCH", "/arvados/v1/container_requests/"+arvadostest.QueuedContainerRequestUUID,
+                       strings.NewReader(fmt.Sprintf(`{"container_request": {"priority": %d}}`, pri)))
+               req.Header.Set("Authorization", "Bearer "+arvadostest.ActiveToken)
+               req.Header.Set("Content-type", "application/json")
+               resp := s.testRequest(req)
+               c.Check(resp.StatusCode, check.Equals, http.StatusOK)
+               var cr arvados.ContainerRequest
+               c.Check(json.NewDecoder(resp.Body).Decode(&cr), check.IsNil)
+               c.Check(cr.UUID, check.Equals, arvadostest.QueuedContainerRequestUUID)
+               c.Check(cr.Priority, check.Equals, pri)
+       }
+       setPri(696)
+       setPri(1) // Reset fixture so side effect doesn't break other tests.
  }
  
  func (s *FederationSuite) TestCreateRemoteContainerRequest(c *check.C) {

diff --combined services/api/app/models/container.rb
index 0682676c5ccc2d7f7f738c2f99971447f1456be9,3d1e6491150624bcbcbad9aa95a5e4dc202dd390..abcfdbd296b3ab71cf7e8466e7c9279076f2c93f
--- 1/services/api/app/models/container.rb
--- 2/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@@ -346,7 -346,7 +346,7 @@@ class Container < ArvadosMode
      transaction do
        reload
        check_lock_fail
-       update_attributes!(state: Locked)
+       update_attributes!(state: Locked, lock_count: self.lock_count+1)
      end
    end
  
@@@ -364,7 -364,14 +364,14 @@@
      transaction do
        reload(lock: 'FOR UPDATE')
        check_unlock_fail
-       update_attributes!(state: Queued)
+       if self.lock_count < Rails.configuration.max_container_dispatch_attempts
+         update_attributes!(state: Queued)
+       else
+         update_attributes!(state: Cancelled,
+                            runtime_status: {
+                              error: "Container exceeded 'max_container_dispatch_attempts' (lock_count=#{self.lock_count}."
+                            })
+       end
      end
    end
  
@@@ -375,9 -382,6 +382,9 @@@
      else
        kwargs = {}
      end
 +    if users_list.select { |u| u.is_admin }.any?
 +      return super
 +    end
      Container.where(ContainerRequest.readable_by(*users_list).where("containers.uuid = container_requests.container_uuid").exists)
    end
  
@@@ -457,7 -461,7 +464,7 @@@
  
      case self.state
      when Locked
-       permitted.push :priority, :runtime_status, :log
+       permitted.push :priority, :runtime_status, :log, :lock_count
  
      when Queued
        permitted.push :priority
@@@ -478,7 -482,7 +485,7 @@@
        when Running
          permitted.push :finished_at, *progress_attrs
        when Queued, Locked
-         permitted.push :finished_at, :log
+         permitted.push :finished_at, :log, :runtime_status
        end
  
      else

diff --combined services/api/test/unit/container_test.rb
index dac08d4b69bdbf32b58e9a416cf4a83aa3a4fe7a,5750d5ebbd0394d99bdce8ae2a038bb1d799cd77..178135ead87098b23874b3eeb607437458ee2eb0
--- 1/services/api/test/unit/container_test.rb
--- 2/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@@ -663,6 -663,52 +663,52 @@@ class ContainerTest < ActiveSupport::Te
      assert_operator auth_exp, :<, db_current_time
    end
  
+   test "Exceed maximum lock-unlock cycles" do
+     Rails.configuration.max_container_dispatch_attempts = 3
+ 
+     set_user_from_auth :active
+     c, cr = minimal_new
+ 
+     set_user_from_auth :dispatch1
+     assert_equal Container::Queued, c.state
+     assert_equal 0, c.lock_count
+ 
+     c.lock
+     c.reload
+     assert_equal 1, c.lock_count
+     assert_equal Container::Locked, c.state
+ 
+     c.unlock
+     c.reload
+     assert_equal 1, c.lock_count
+     assert_equal Container::Queued, c.state
+ 
+     c.lock
+     c.reload
+     assert_equal 2, c.lock_count
+     assert_equal Container::Locked, c.state
+ 
+     c.unlock
+     c.reload
+     assert_equal 2, c.lock_count
+     assert_equal Container::Queued, c.state
+ 
+     c.lock
+     c.reload
+     assert_equal 3, c.lock_count
+     assert_equal Container::Locked, c.state
+ 
+     c.unlock
+     c.reload
+     assert_equal 3, c.lock_count
+     assert_equal Container::Cancelled, c.state
+ 
+     assert_raise(ArvadosModel::LockFailedError) do
+       # Cancelled to Locked is not allowed
+       c.lock
+     end
+   end
+ 
    test "Container queued cancel" do
      set_user_from_auth :active
      c, cr = minimal_new({container_count_max: 1})
@@@ -677,14 -723,6 +723,14 @@@
      assert_equal 1, Container.readable_by(users(:active)).where(state: "Queued").count
    end
  
 +  test "Containers with no matching request are readable by admin" do
 +    uuids = Container.includes('container_requests').where(container_requests: {uuid: nil}).collect(&:uuid)
 +    assert_not_empty uuids
 +    assert_empty Container.readable_by(users(:active)).where(uuid: uuids)
 +    assert_not_empty Container.readable_by(users(:admin)).where(uuid: uuids)
 +    assert_equal uuids.count, Container.readable_by(users(:admin)).where(uuid: uuids).count
 +  end
 +
    test "Container locked cancel" do
      set_user_from_auth :active
      c, _ = minimal_new