8128: Add Locked state to Container model.

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 787047df68e877bc8944b3c23186cc400c3ae227..5856eddaf6715e881e6818fd72edb1bcdae02e7f 100644 (file)
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -42,6 +42,7 @@ class Container < ArvadosModel
   States =
     [
      (Queued = 'Queued'),
+     (Locked = 'Locked'),
      (Running = 'Running'),
      (Complete = 'Complete'),
      (Cancelled = 'Cancelled')
@@ -49,7 +50,8 @@ class Container < ArvadosModel
 
   State_transitions = {
     nil => [Queued],
-    Queued => [Running, Cancelled],
+    Queued => [Locked, Cancelled],
+    Locked => [Queued, Running, Cancelled],
     Running => [Complete, Cancelled]
   }
 
@@ -102,47 +104,40 @@ class Container < ArvadosModel
   end
 
   def validate_change
-    permitted = []
+    permitted = [:state]
 
     if self.new_record?
-      permitted.push :owner_uuid, :command, :container_image, :cwd, :environment,
-                     :mounts, :output_path, :priority, :runtime_constraints, :state
+      permitted.push(:owner_uuid, :command, :container_image, :cwd,
+                     :environment, :mounts, :output_path, :priority,
+                     :runtime_constraints)
     end
 
     case self.state
-    when Queued
-      # permit priority change only.
+    when Queued, Locked
       permitted.push :priority
 
     when Running
+      permitted.push :priority, :progress
       if self.state_changed?
-        # At point of state change, can set state and started_at
-        permitted.push :state, :started_at
-      else
-        # While running, can update priority and progress.
-        permitted.push :priority, :progress
+        permitted.push :started_at
       end
 
     when Complete
-      if self.state_changed?
-        permitted.push :state, :finished_at, :output, :log, :exit_code
-      else
-        errors.add :state, "cannot update record"
+      if self.state_was == Running
+        permitted.push :finished_at, :output, :log, :exit_code
       end
 
     when Cancelled
-      if self.state_changed?
-        if self.state_was == Running
-          permitted.push :state, :finished_at, :output, :log
-        elsif self.state_was == Queued
-          permitted.push :state, :finished_at
-        end
-      else
-        errors.add :state, "cannot update record"
+      case self.state_was
+      when Running
+        permitted.push :finished_at, :output, :log
+      when Queued, Locked
+        permitted.push :finished_at
       end
 
     else
-      errors.add :state, "invalid state"
+      # The state_transitions check will add an error message for this
+      return false
     end
 
     check_update_whitelist permitted

diff --git a/services/api/lib/whitelist_update.rb b/services/api/lib/whitelist_update.rb
index a81f9924f01aa182bf35efc2e48dd326b3b20942..8fccd0f45c36416e72034a06e8e3ce3880f7aa04 100644 (file)
--- a/services/api/lib/whitelist_update.rb
+++ b/services/api/lib/whitelist_update.rb
@@ -2,7 +2,7 @@ module WhitelistUpdate
   def check_update_whitelist permitted_fields
     attribute_names.each do |field|
       if not permitted_fields.include? field.to_sym and self.send((field.to_s + "_changed?").to_sym)
-        errors.add field, "illegal update of field"
+        errors.add field, "cannot be modified in this state"
       end
     end
   end
@@ -10,7 +10,7 @@ module WhitelistUpdate
   def validate_state_change
     if self.state_changed?
       unless state_transitions[self.state_was].andand.include? self.state
-        errors.add :state, "invalid state change from #{self.state_was} to #{self.state}"
+        errors.add :state, "cannot change from #{self.state_was} to #{self.state}"
         return false
       end
     end

diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index d0def576c3b6450b633cf08003ba5bf4ea296e2f..701147cf6576997c04bf633650b0770c193136ee 100644 (file)
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -306,18 +306,18 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal "Committed", cr.state
 
     c = Container.find_by_uuid cr.container_uuid
-    assert_equal "Queued", c.state
+    assert_equal Container::Queued, c.state
 
     act_as_system_user do
-      c.state = "Running"
-      c.save!
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
     end
 
     cr.reload
     assert_equal "Committed", cr.state
 
     act_as_system_user do
-      c.state = "Complete"
+      c.update_attributes! state: Container::Complete
       c.save!
     end
 

diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index a25f2af46781668786cae56f666147165b4ddb6c..84713c26839b38afc122df7ee6b7d1cc8bd88aaf 100644 (file)
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -9,91 +9,42 @@ class ContainerTest < ActiveSupport::TestCase
     c
   end
 
-  def check_illegal_modify c
-    c.reload
-    c.command = ["echo", "bar"]
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.container_image = "img2"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.cwd = "/tmp2"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.environment = {"FOO" => "BAR"}
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.mounts = {"FOO" => "BAR"}
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+  def show_errors c
+    return lambda { c.errors.full_messages.inspect }
+  end
 
-    c.reload
-    c.output_path = "/tmp3"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
+  def check_illegal_updates c, bad_updates
+    bad_updates.each do |u|
+      refute c.update_attributes(u), u.inspect
+      refute c.valid?
+      c.reload
     end
+  end
 
-    c.reload
-    c.runtime_constraints = {"FOO" => "BAR"}
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+  def check_illegal_modify c
+    check_illegal_updates c, [{command: ["echo", "bar"]},
+                              {container_image: "img2"},
+                              {cwd: "/tmp2"},
+                              {environment: {"FOO" => "BAR"}},
+                              {mounts: {"FOO" => "BAR"}},
+                              {output_path: "/tmp3"},
+                              {runtime_constraints: {"FOO" => "BAR"}}]
   end
 
   def check_bogus_states c
-    c.reload
-    c.state = nil
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Flubber"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+    check_illegal_updates c, [{state: nil},
+                              {state: "Flubber"}]
   end
 
-  def check_no_change_from_complete c
+  def check_no_change_from_cancelled c
     check_illegal_modify c
     check_bogus_states c
 
-    c.reload
-    c.priority = 3
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Queued"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Running"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Complete"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+    check_illegal_updates c, [{ priority: 3 },
+                              { state: Container::Queued },
+                              { state: Container::Locked },
+                              { state: Container::Running },
+                              { state: Container::Complete }]
   end
 
   test "Container create" do
@@ -120,58 +71,79 @@ class ContainerTest < ActiveSupport::TestCase
       c = minimal_new
       c.save!
 
-      c.reload
-      c.state = "Complete"
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
+      check_illegal_updates c, [{state: Container::Running},
+                                {state: Container::Complete}]
 
-      c.reload
-      c.state = "Running"
-      c.save!
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
 
       check_illegal_modify c
       check_bogus_states c
 
+      check_illegal_updates c, [{state: Container::Queued}]
       c.reload
-      c.state = "Queued"
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
 
-      c.reload
-      c.priority = 3
-      c.save!
+      c.update_attributes! priority: 3
     end
   end
 
-  test "Container queued cancel" do
+  test "Lock and unlock" do
     act_as_system_user do
       c = minimal_new
       c.save!
+      assert_equal Container::Queued, c.state
 
+      refute c.update_attributes(state: Container::Running), "not locked"
+      c.reload
+      refute c.update_attributes(state: Container::Complete), "not locked"
       c.reload
-      c.state = "Cancelled"
-      c.save!
 
-      check_no_change_from_complete c
+      assert c.update_attributes(state: Container::Locked), show_errors(c)
+      assert c.update_attributes(state: Container::Queued), show_errors(c)
+
+      refute c.update_attributes(state: Container::Running), "not locked"
+      c.reload
+
+      assert c.update_attributes(state: Container::Locked), show_errors(c)
+      assert c.update_attributes(state: Container::Running), show_errors(c)
+
+      refute c.update_attributes(state: Container::Locked), "already running"
+      c.reload
+      refute c.update_attributes(state: Container::Queued), "already running"
+      c.reload
+
+      assert c.update_attributes(state: Container::Complete), show_errors(c)
     end
   end
 
-  test "Container running cancel" do
+  test "Container queued cancel" do
     act_as_system_user do
       c = minimal_new
       c.save!
+      assert c.update_attributes(state: Container::Cancelled), show_errors(c)
+      check_no_change_from_cancelled c
+    end
+  end
 
-      c.reload
-      c.state = "Running"
+  test "Container locked cancel" do
+    act_as_system_user do
+      c = minimal_new
       c.save!
+      assert c.update_attributes(state: Container::Locked), show_errors(c)
+      assert c.update_attributes(state: Container::Cancelled), show_errors(c)
+      check_no_change_from_cancelled c
+    end
+  end
 
-      c.reload
-      c.state = "Cancelled"
+  test "Container running cancel" do
+    act_as_system_user do
+      c = minimal_new
       c.save!
-
-      check_no_change_from_complete c
+      c.update_attributes! state: Container::Queued
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
+      c.update_attributes! state: Container::Cancelled
+      check_no_change_from_cancelled c
     end
   end
 
@@ -192,28 +164,13 @@ class ContainerTest < ActiveSupport::TestCase
     act_as_system_user do
       c = minimal_new
       c.save!
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
 
-      c.reload
-      c.state = "Running"
-      c.save!
-
-      c.reload
-      c.exit_code = 1
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
+      check_illegal_updates c, [{exit_code: 1},
+                                {exit_code: 1, state: Container::Cancelled}]
 
-      c.reload
-      c.exit_code = 1
-      c.state = "Cancelled"
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
-
-      c.reload
-      c.exit_code = 1
-      c.state = "Complete"
-      c.save!
+      assert c.update_attributes(exit_code: 1, state: Container::Complete)
     end
   end
 end