[ARVADOS] updated: d1baf718d0866c64252006bf61a6f0c5da353f7b

git at public.curoverse.com git at public.curoverse.com
Thu Dec 3 17:22:59 EST 2015


Summary of changes:
 services/api/app/models/container.rb         |  91 ++++++++++++++++++++++--
 services/api/app/models/container_request.rb |  73 +++++++++++++++++++
 services/api/test/unit/container_test.rb     | 101 ++++++++++++++++++++++++++-
 3 files changed, 256 insertions(+), 9 deletions(-)

       via  d1baf718d0866c64252006bf61a6f0c5da353f7b (commit)
      from  d9f8f46ccd5a418dcf7b5f43aeb59cd2d9d424ba (commit)

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 d1baf718d0866c64252006bf61a6f0c5da353f7b
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Dec 3 17:22:54 2015 -0500

    Working on state change assertions and testing for container record.

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 66b33cd..a0ac077 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -3,15 +3,17 @@ class Container < ArvadosModel
   include KindAndEtag
   include CommonApiTemplate
 
-  serialize :properties, Hash
   serialize :environment, Hash
   serialize :mounts, Hash
   serialize :runtime_constraints, Hash
   serialize :command, Array
 
-  has_many :container_requests, :foreign_key => :container_uuid, :class_name => 'ContainerRequest', :primary_key => :uuid
+  before_validation :fill_field_defaults
+  before_validation :set_timestamps
+  validates :command, :container_image, :output_path, :cwd, :presence => true
+  validate :validate_change
 
-  before_create :set_state_before_save
+  has_many :container_requests, :foreign_key => :container_uuid, :class_name => 'ContainerRequest', :primary_key => :uuid
 
   api_accessible :user, extend: :common do |t|
     t.add :command
@@ -28,7 +30,6 @@ class Container < ArvadosModel
     t.add :runtime_constraints
     t.add :started_at
     t.add :state
-    t.add :uuid
   end
 
   # Supported states for a container
@@ -40,9 +41,87 @@ class Container < ArvadosModel
      (Cancelled = 'Cancelled')
     ]
 
-  def set_state_before_save
-    self.state ||= Queued
+  def fill_field_defaults
+    if self.new_record?
+      self.state ||= Queued
+      self.environment ||= {}
+      self.runtime_constraints ||= {}
+      self.cwd ||= "."
+    end
+  end
+
+  protected
+
+  def permission_to_create
+    current_user.andand.is_admin
+  end
+
+  def permission_to_update
+    current_user.andand.is_admin
+  end
+
+  def check_permitted_updates 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 #{field}"
+      end
+    end
   end
 
+  def set_timestamps
+    if self.state_changed? and self.state == Running
+      self.started_at ||= db_current_time
+    end
+
+    if self.state_changed? and [Complete, Cancelled].include? self.state
+      self.finished_at ||= db_current_time
+    end
+  end
+
+  def validate_change
+    permitted = [:modified_at, :modified_by_user_uuid, :modified_by_client_uuid]
+
+    if self.new_record?
+      permitted.push :owner_uuid, :command, :container_image, :cwd, :environment,
+                     :mounts, :output_path, :priority, :runtime_constraints, :state
+    end
+
+    case self.state
+    when Queued
+      # permit priority change only.
+      if self.state_changed? and not self.state_was.nil?
+        errors.add :state, "Can only go to Queued from nil"
+      else
+        permitted.push :priority
+      end
+    when Running
+      if self.state_changed?
+        if self.state_was == Queued
+          permitted.push :state, :started_at
+        else
+          errors.add :state, "Can only go to Runinng from Queued"
+        end
+      else
+        permitted.push :progress
+      end
+    when Complete, 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
+        else
+          errors.add :state, "Can only go to #{self.state} from Running"
+        end
+      end
+    else
+      errors.add :state, "Invalid state #{self.state}"
+    end
+
+    check_permitted_updates permitted
+  end
+
+  def validate_fields
+  end
 
 end
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 9356a70..22b6bde 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -10,6 +10,9 @@ class ContainerRequest < ArvadosModel
   serialize :command, Array
 
   before_create :set_state_before_save
+  validate :validate_change_permitted
+  validate :validate_status
+  validate :validate_state_change
 
   api_accessible :user, extend: :common do |t|
     t.add :command
@@ -43,6 +46,76 @@ class ContainerRequest < ArvadosModel
     self.state ||= Uncommitted
   end
 
+  def validate_change_permitted
+    if self.changed?
+      ok = case self.state
+           when nil
+             true
+           when Uncommitted
+             true
+           when Committed
+             # only allow state and priority to change.
+             not (self.command_changed? or
+                  self.container_count_max_changed? or
+                  self.container_image_changed? or
+                  self.container_uuid_changed? or
+                  self.cwd_changed? or
+                  self.description_changed? or
+                  self.environment_changed? or
+                  self.expires_at_changed? or
+                  self.filters_changed? or
+                  self.mounts_changed? or
+                  self.name_changed? or
+                  self.output_path_changed? or
+                  self.properties_changed? or
+                  self.requesting_container_uuid_changed? or
+                  self.runtime_constraints_changed?)
+           when Final
+             false
+           else
+             false
+           end
+      if not ok
+        errors.add :state, "Invalid update of container request in #{self.state} state"
+      end
+    end
+  end
+
+  def validate_status
+    if self.state.in?(States)
+      true
+    else
+      errors.add :state, "#{state.inspect} must be one of: #{States.inspect}"
+      false
+    end
+  end
+
+  def validate_state_change
+    ok = true
+    if self.state_changed?
+      ok = case self.state_was
+           when nil
+             # Must go to Uncommitted
+             self.state == Uncommitted
+           when Uncommitted
+             # Must go to Committed
+             self.state == Committed
+           when Committed
+             # Must to go Final
+             self.state == Final
+           when Final
+             # Once in a final state, don't permit any more state changes
+             false
+           else
+             # Any other state transition is also invalid
+             false
+           end
+      if not ok
+        errors.add :state, "invalid change from #{self.state_was} to #{self.state}"
+      end
+    end
+    ok
+  end
 
 
 end
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 3ba35c4..c635af3 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -1,7 +1,102 @@
 require 'test_helper'
 
 class ContainerTest < ActiveSupport::TestCase
-  # test "the truth" do
-  #   assert true
-  # end
+  def check_illegal_modify c
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.command = ["echo", "bar"]
+        c.save!
+      end
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.container_image = "img2"
+        c.save!
+      end
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.cwd = "/tmp2"
+        c.save!
+      end
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.environment = {"FOO" => "BAR"}
+        c.save!
+      end
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.mounts = {"FOO" => "BAR"}
+        c.save!
+      end
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.output_path = "/tmp3"
+        c.save!
+      end
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.runtime_constraints = {"FOO" => "BAR"}
+        c.save!
+      end
+
+  end
+
+  def check_bogus_states c
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.state = nil
+        c.save!
+      end
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.state = "Flubber"
+        c.save!
+      end
+  end
+
+  test "Container create" do
+    act_as_system_user do
+      c = Container.new
+      c.command = ["echo", "foo"]
+      c.container_image = "img"
+      c.cwd = "/tmp"
+      c.environment = {}
+      c.mounts = {}
+      c.output_path = "/tmp"
+      c.priority = 1
+      c.runtime_constraints = {}
+      c.save!
+
+      check_illegal_modify c
+      check_bogus_states c
+
+      c.reload
+      c.priority = 2
+      c.save!
+
+      c.reload
+      c.state = "Running"
+      c.save!
+
+      check_illegal_modify c
+      check_bogus_states c
+
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.reload
+        c.state = "Queued"
+        c.save!
+      end
+
+      c.reload
+      c.priority = 3
+      c.save!
+
+    end
+  end
 end

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list