From 0f14b3456d2d3bdf95b78b65a1a41280a7416928 Mon Sep 17 00:00:00 2001
From: Peter Amstutz <pamstutz@veritasgenetics.com>
Date: Thu, 14 Feb 2019 18:28:00 -0500
Subject: [PATCH] 11561: Add lock_count to containers, cancel container on
 unlock exceeded

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
---
 services/api/app/models/container.rb          | 15 ++++--
 services/api/config/application.default.yml   |  4 ++
 ...20190214214814_add_container_lock_count.rb |  5 ++
 services/api/db/structure.sql                 |  5 +-
 services/api/test/unit/container_test.rb      | 46 +++++++++++++++++++
 5 files changed, 70 insertions(+), 5 deletions(-)
 create mode 100644 services/api/db/migrate/20190214214814_add_container_lock_count.rb

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index bd586907ee..3d1e649115 100644
--- 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
 
@@ -454,7 +461,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
@@ -475,7 +482,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 dcf270e3fb..d0f3a4caeb 100644
--- 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
index 0000000000..a496eb0d1c
--- /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 211fa5043f..f766f33e1b 100644
--- 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/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 2a9ff5bf4c..5750d5ebbd 100644
--- 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})
-- 
2.30.2