14807: Merge branch 'master'

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

diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go
index 79b534dc86c3e68066bcc3e52cdea69acb880c62..62916acd2ac10be14d90d4e02e2703e77949e32b 100644 (file)
--- a/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@ -554,16 +554,20 @@ func (s *FederationSuite) TestGetRemoteContainerRequest(c *check.C) {
 
 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 --git a/lib/controller/handler.go b/lib/controller/handler.go
index 295dde7ca42821b1c8f904eec42ac7e7764812fa..53125ae5543b51287e5de80a8b442f2002972a86 100644 (file)
--- a/lib/controller/handler.go
+++ b/lib/controller/handler.go
@@ -80,12 +80,10 @@ func (h *Handler) setup() {
        h.handlerStack = mux
 
        sc := *arvados.DefaultSecureClient
-       sc.Timeout = time.Duration(h.Cluster.HTTPRequestTimeout)
        sc.CheckRedirect = neverRedirect
        h.secureClient = &sc
 
        ic := *arvados.InsecureHTTPClient
-       ic.Timeout = time.Duration(h.Cluster.HTTPRequestTimeout)
        ic.CheckRedirect = neverRedirect
        h.insecureClient = &ic
 

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 33cc686d4f5a14f3a432bc3df077ab258797a4a8..6147b79f9f5aa16c6b9e24ef5164bab43139373a 100644 (file)
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -262,6 +262,7 @@ class Collection < ArvadosModel
       sync_past_versions if syncable_updates.any?
       if snapshot
         snapshot.attributes = self.syncable_updates
+        snapshot.manifest_text = snapshot.signed_manifest_text
         snapshot.save
       end
     end

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 0682676c5ccc2d7f7f738c2f99971447f1456be9..abcfdbd296b3ab71cf7e8466e7c9279076f2c93f 100644 (file)
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -346,7 +346,7 @@ class Container < ArvadosModel
     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 @@ class Container < ArvadosModel
     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
 
@@ -457,7 +464,7 @@ class Container < ArvadosModel
 
     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 +485,7 @@ class Container < ArvadosModel
       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 --git a/services/api/config/application.default.yml b/services/api/config/application.default.yml
index dcf270e3fb5d1a59a25e9858fc65e2eb2b901c42..d0f3a4caeb11d9f931772a8b1036fb257c2632fe 100644 (file)
--- a/services/api/config/application.default.yml
+++ b/services/api/config/application.default.yml
@@ -529,6 +529,10 @@ common:
   # > 0 = auto-create a new version when older than the specified number of seconds.
   preserve_version_if_idle: -1
 
+  # Number of times a container can be unlocked before being
+  # automatically cancelled.
+  max_container_dispatch_attempts: 5
+
 development:
   force_ssl: false
   cache_classes: false

diff --git a/services/api/db/migrate/20190214214814_add_container_lock_count.rb b/services/api/db/migrate/20190214214814_add_container_lock_count.rb
new file mode 100644 (file)
index 0000000..a496eb0
--- /dev/null
+++ b/services/api/db/migrate/20190214214814_add_container_lock_count.rb
@@ -0,0 +1,5 @@
+class AddContainerLockCount < ActiveRecord::Migration
+  def change
+    add_column :containers, :lock_count, :int, :null => false, :default => 0
+  end
+end

diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 211fa5043fda2aedc33646f8c98dff863bec8d7a..f766f33e1b35e1f85a64aa2c5d87bf85e2bb6d0f 100644 (file)
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -362,7 +362,8 @@ CREATE TABLE public.containers (
     runtime_status jsonb DEFAULT '{}'::jsonb,
     runtime_user_uuid text,
     runtime_auth_scopes jsonb,
-    runtime_token text
+    runtime_token text,
+    lock_count integer DEFAULT 0 NOT NULL
 );
 
 
@@ -3217,3 +3218,5 @@ INSERT INTO schema_migrations (version) VALUES ('20181011184200');
 
 INSERT INTO schema_migrations (version) VALUES ('20181213183234');
 
+INSERT INTO schema_migrations (version) VALUES ('20190214214814');
+

diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 26b8290e6961452e97f505ad3b239f6ef5a28596..997d89d5cd13d72c97dd3edcaa177bca36e1efed 100644 (file)
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -1279,7 +1279,6 @@ EOS
         version: 42,
         current_version_uuid: collections(:collection_owned_by_active).uuid,
         manifest_text: manifest_text,
-        # portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47"
       }
     }
     assert_response :success
@@ -1287,4 +1286,28 @@ EOS
     assert_equal 1, resp['version']
     assert_equal resp['uuid'], resp['current_version_uuid']
   end
+
+  test "update collection with versioning enabled" do
+    Rails.configuration.collection_versioning = true
+    Rails.configuration.preserve_version_if_idle = 1 # 1 second
+
+    col = collections(:collection_owned_by_active)
+    assert_equal 2, col.version
+    assert col.modified_at < Time.now - 1.second
+
+    token = api_client_authorizations(:active).v2token
+    signed = Blob.sign_locator(
+      'acbd18db4cc2f85cedef654fccc4a4d8+3',
+      key: Rails.configuration.blob_signing_key,
+      api_token: token)
+    authorize_with_token token
+    put :update, {
+          id: col.uuid,
+          collection: {
+            manifest_text: ". #{signed} 0:3:foo.txt\n",
+          },
+        }
+    assert_response :success
+    assert_equal 3, json_response['version']
+  end
 end

diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index dac08d4b69bdbf32b58e9a416cf4a83aa3a4fe7a..178135ead87098b23874b3eeb607437458ee2eb0 100644 (file)
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -663,6 +663,52 @@ class ContainerTest < ActiveSupport::TestCase
     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})