From ce6f582e7e1d5b927aeee0aab3def7ab8a5cae4f Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Wed, 9 Dec 2015 15:06:46 -0500 Subject: [PATCH] 6429: Use text instead of string for longer API fields. Move "resolve" from Container to ContainerRequest. Simplify state transition checking. Improve formatting of error messages. Rename workbench models to be singular. Remove bogus factory files. --- .../models/{containers.rb => container.rb} | 0 ...ainer_requests.rb => container_request.rb} | 0 services/api/app/models/container.rb | 70 +++++++------------ services/api/app/models/container_request.rb | 52 +++++++++----- ...02151426_create_containers_and_requests.rb | 18 ++--- services/api/db/structure.sql | 18 ++--- services/api/lib/whitelist_update.rb | 11 ++- .../api/test/factories/container_requests.rb | 29 -------- services/api/test/factories/containers.rb | 26 ------- .../api/test/unit/container_request_test.rb | 58 +++++++-------- 10 files changed, 118 insertions(+), 164 deletions(-) rename apps/workbench/app/models/{containers.rb => container.rb} (100%) rename apps/workbench/app/models/{container_requests.rb => container_request.rb} (100%) delete mode 100644 services/api/test/factories/container_requests.rb delete mode 100644 services/api/test/factories/containers.rb diff --git a/apps/workbench/app/models/containers.rb b/apps/workbench/app/models/container.rb similarity index 100% rename from apps/workbench/app/models/containers.rb rename to apps/workbench/app/models/container.rb diff --git a/apps/workbench/app/models/container_requests.rb b/apps/workbench/app/models/container_request.rb similarity index 100% rename from apps/workbench/app/models/container_requests.rb rename to apps/workbench/app/models/container_request.rb diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index f452b10a2d..15bbbe017d 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -14,6 +14,7 @@ class Container < ArvadosModel before_validation :fill_field_defaults, :if => :new_record? before_validation :set_timestamps validates :command, :container_image, :output_path, :cwd, :presence => true + validate :validate_state_change validate :validate_change after_save :request_finalize after_save :process_tree_priority @@ -46,17 +47,14 @@ class Container < ArvadosModel (Cancelled = 'Cancelled') ] - # Turn a container request into a container. - def self.resolve req - # In the future this will do things like resolve symbolic git and keep - # references to content addresses. - Container.create!({ :command => req.command, - :container_image => req.container_image, - :cwd => req.cwd, - :environment => req.environment, - :mounts => req.mounts, - :output_path => req.output_path, - :runtime_constraints => req.runtime_constraints }) + State_transitions = { + nil => [Queued], + Queued => [Running, Cancelled], + Running => [Complete, Cancelled] + } + + def state_transitions + State_transitions end def update_priority! @@ -64,7 +62,7 @@ class Container < ArvadosModel # its committed container requests and save the record. max = 0 ContainerRequest.where(container_uuid: uuid).each do |cr| - if cr.state == "Committed" and cr.priority > max + if cr.state == ContainerRequest::Committed and cr.priority > max max = cr.priority end end @@ -114,32 +112,20 @@ class Container < ArvadosModel # permit priority change only. permitted.push :priority - if self.state_changed? and not self.state_was.nil? - errors.add :state, "Can only go to from nil to Queued" - end - when Running if self.state_changed? # At point of state change, can set state and started_at - if self.state_was == Queued - permitted.push :state, :started_at - else - errors.add :state, "Can only go from Queued to Running" - end - + permitted.push :state, :started_at + else # While running, can update priority and progress. permitted.push :priority, :progress end when Complete if self.state_changed? - if self.state_was == Running - permitted.push :state, :finished_at, :output, :log - else - errors.add :state, "Cannot go from #{self.state_was} to #{self.state}" - end + permitted.push :state, :finished_at, :output, :log else - errors.add :state, "Cannot update record in Complete state" + errors.add :state, "cannot update record" end when Cancelled @@ -148,41 +134,39 @@ class Container < ArvadosModel permitted.push :state, :finished_at, :output, :log elsif self.state_was == Queued permitted.push :state, :finished_at - else - errors.add :state, "Cannot go from #{self.state_was} to #{self.state}" end else - errors.add :state, "Cannot update record in Cancelled state" + errors.add :state, "cannot update record" end else - errors.add :state, "Invalid state #{self.state}" + errors.add :state, "invalid state" end check_update_whitelist permitted end def request_finalize + # This container is finished so finalize any associated container requests + # that are associated with this container. if self.state_changed? and [Complete, Cancelled].include? self.state act_as_system_user do - ContainerRequest.where(container_uuid: uuid).each do |cr| - cr.state = "Final" - cr.save! - end + # Note using update_all skips model validation and callbacks. + ContainerRequest.update_all({:state => ContainerRequest::Final}, ['container_uuid=?', uuid]) end end end def process_tree_priority - if self.priority_changed? + # This container is cancelled so cancel any container requests made by this + # container. + if self.priority_changed? and self.priority == 0 # This could propagate any parent priority to the children (not just # priority 0) - if self.priority == 0 - act_as_system_user do - ContainerRequest.where(requesting_container_uuid: uuid).each do |cr| - cr.priority = self.priority - cr.save! - end + act_as_system_user do + ContainerRequest.where(requesting_container_uuid: uuid).each do |cr| + cr.priority = self.priority + cr.save! end end end diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb index 9dc5c7b4a6..1011f6fd75 100644 --- a/services/api/app/models/container_request.rb +++ b/services/api/app/models/container_request.rb @@ -15,6 +15,7 @@ class ContainerRequest < ArvadosModel before_validation :fill_field_defaults, :if => :new_record? before_validation :set_container validates :command, :container_image, :output_path, :cwd, :presence => true + validate :validate_state_change validate :validate_change after_save :update_priority @@ -46,6 +47,16 @@ class ContainerRequest < ArvadosModel (Final = 'Final'), ] + State_transitions = { + nil => [Uncommitted, Committed], + Uncommitted => [Committed], + Committed => [Final] + } + + def state_transitions + State_transitions + end + def skip_uuid_read_permission_check # XXX temporary until permissions are sorted out. %w(modified_by_client_uuid container_uuid requesting_container_uuid) @@ -62,16 +73,29 @@ class ContainerRequest < ArvadosModel self.priority ||= 1 end + # Turn a container request into a container. + def resolve + # In the future this will do things like resolve symbolic git and keep + # references to content addresses. + Container.create!({ :command => self.command, + :container_image => self.container_image, + :cwd => self.cwd, + :environment => self.environment, + :mounts => self.mounts, + :output_path => self.output_path, + :runtime_constraints => self.runtime_constraints }) + end + def set_container if self.container_uuid_changed? if not current_user.andand.is_admin and not self.container_uuid.nil? - errors.add :container_uuid, "Cannot only update container_uuid to nil." + errors.add :container_uuid, "can only be updated to nil." end else if self.state_changed? if self.state == Committed and (self.state_was == Uncommitted or self.state_was.nil?) act_as_system_user do - self.container_uuid = Container.resolve(self).andand.uuid + self.container_uuid = self.resolve.andand.uuid end end end @@ -92,37 +116,29 @@ class ContainerRequest < ArvadosModel when Committed if container_uuid.nil? - errors.add :container_uuid, "Has not been resolved to a container." + errors.add :container_uuid, "has not been resolved to a container." end # Can update priority, container count. permitted.push :priority, :container_count_max, :container_uuid if self.state_changed? - if self.state_was == Uncommitted or self.state_was.nil? - # Allow create-and-commit in a single operation. - permitted.push :command, :container_image, :cwd, :description, :environment, - :filters, :mounts, :name, :output_path, :properties, - :requesting_container_uuid, :runtime_constraints, - :state, :container_uuid - else - errors.add :state, "Can only go from Uncommitted to Committed" - end + # Allow create-and-commit in a single operation. + permitted.push :command, :container_image, :cwd, :description, :environment, + :filters, :mounts, :name, :output_path, :properties, + :requesting_container_uuid, :runtime_constraints, + :state, :container_uuid end when Final if self.state_changed? - if self.state_was == Committed permitted.push :state - else - errors.add :state, "Can only go from Committed to Final" - end else - errors.add "Cannot update record in Final state" + errors.add :state, "does not allow updates" end else - errors.add :state, "Invalid state #{self.state}" + errors.add :state, "invalid value" end check_update_whitelist permitted diff --git a/services/api/db/migrate/20151202151426_create_containers_and_requests.rb b/services/api/db/migrate/20151202151426_create_containers_and_requests.rb index 99611b41ee..4741515d46 100644 --- a/services/api/db/migrate/20151202151426_create_containers_and_requests.rb +++ b/services/api/db/migrate/20151202151426_create_containers_and_requests.rb @@ -13,10 +13,10 @@ class CreateContainersAndRequests < ActiveRecord::Migration t.string :log t.text :environment t.string :cwd - t.string :command + t.text :command t.string :output_path - t.string :mounts - t.string :runtime_constraints + t.text :mounts + t.text :runtime_constraints t.string :output t.string :container_image t.float :progress @@ -34,21 +34,21 @@ class CreateContainersAndRequests < ActiveRecord::Migration t.string :modified_by_user_uuid t.string :name t.text :description - t.string :properties + t.text :properties t.string :state t.string :requesting_container_uuid t.string :container_uuid t.integer :container_count_max - t.string :mounts - t.string :runtime_constraints + t.text :mounts + t.text :runtime_constraints t.string :container_image - t.string :environment + t.text :environment t.string :cwd - t.string :command + t.text :command t.string :output_path t.integer :priority t.datetime :expires_at - t.string :filters + t.text :filters t.timestamps end diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql index 7ff4af2ef8..1ce44cde4c 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -273,21 +273,21 @@ CREATE TABLE container_requests ( modified_by_user_uuid character varying(255), name character varying(255), description text, - properties character varying(255), + properties text, state character varying(255), requesting_container_uuid character varying(255), container_uuid character varying(255), container_count_max integer, - mounts character varying(255), - runtime_constraints character varying(255), + mounts text, + runtime_constraints text, container_image character varying(255), - environment character varying(255), + environment text, cwd character varying(255), - command character varying(255), + command text, output_path character varying(255), priority integer, expires_at timestamp without time zone, - filters character varying(255), + filters text, updated_at timestamp without time zone NOT NULL ); @@ -329,10 +329,10 @@ CREATE TABLE containers ( log character varying(255), environment text, cwd character varying(255), - command character varying(255), + command text, output_path character varying(255), - mounts character varying(255), - runtime_constraints character varying(255), + mounts text, + runtime_constraints text, output character varying(255), container_image character varying(255), progress double precision, diff --git a/services/api/lib/whitelist_update.rb b/services/api/lib/whitelist_update.rb index 7413edf4bc..a81f9924f0 100644 --- a/services/api/lib/whitelist_update.rb +++ b/services/api/lib/whitelist_update.rb @@ -2,7 +2,16 @@ 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 #{field}" + errors.add field, "illegal update of field" + end + end + end + + 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}" + return false end end end diff --git a/services/api/test/factories/container_requests.rb b/services/api/test/factories/container_requests.rb deleted file mode 100644 index c710b1a04b..0000000000 --- a/services/api/test/factories/container_requests.rb +++ /dev/null @@ -1,29 +0,0 @@ -# Read about factories at https://github.com/thoughtbot/factory_girl - -FactoryGirl.define do - factory :container_request do - uuid "MyString" - owner_uuid "MyString" - created_at "2015-12-02 10:17:04" - modified_at "2015-12-02 10:17:04" - modified_by_client_uuid "MyString" - modified_by_user_uuid "MyString" - name "MyString" - description "MyText" - properties "MyString" - state "MyString" - requesting_container_uuid "MyString" - container_uuid "MyString" - container_count_max "" - mounts "MyString" - runtime_constraints "MyString" - container_image "MyString" - environment "MyString" - cwd "MyString" - command "MyString" - output_path "MyString" - priority "" - expires_at "2015-12-02 10:17:04" - filters "MyString" - end -end diff --git a/services/api/test/factories/containers.rb b/services/api/test/factories/containers.rb deleted file mode 100644 index 5c2cd3d184..0000000000 --- a/services/api/test/factories/containers.rb +++ /dev/null @@ -1,26 +0,0 @@ -# Read about factories at https://github.com/thoughtbot/factory_girl - -FactoryGirl.define do - factory :container do - uuid "MyString" - owner_uuid "MyString" - created_at "MyString" - modified_at "MyString" - modified_by_client_uuid "MyString" - modified_by_user_uuid "MyString" - state "MyString" - started_at "2015-12-02 10:14:26" - finished_at "2015-12-02 10:14:26" - log "MyString" - environment "MyText" - cwd "MyString" - command "MyString" - output_path "MyString" - mounts "MyString" - runtime_constraints "MyString" - output "MyString" - container_image "MyString" - progress 1.5 - priority "" - end -end diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index 32a60f855c..6d1a57699b 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -132,22 +132,22 @@ class ContainerRequestTest < ActiveSupport::TestCase c.reload t = Container.find_by_uuid c.container_uuid - assert_equal t.command, ["echo", "foo"] - assert_equal t.container_image, "img" - assert_equal t.cwd, "/tmp" - assert_equal t.environment, {} - assert_equal t.mounts, {"BAR" => "FOO"} - assert_equal t.output_path, "/tmpout" - assert_equal t.runtime_constraints, {} - assert_equal t.priority, 1 + assert_equal ["echo", "foo"], t.command + assert_equal "img", t.container_image + assert_equal "/tmp", t.cwd + assert_equal({}, t.environment) + assert_equal({"BAR" => "FOO"}, t.mounts) + assert_equal "/tmpout", t.output_path + assert_equal({}, t.runtime_constraints) + assert_equal 1, t.priority c.priority = 0 c.save! c.reload t.reload - assert_equal c.priority, 0 - assert_equal t.priority, 0 + assert_equal 0, c.priority + assert_equal 0, t.priority end @@ -164,7 +164,7 @@ class ContainerRequestTest < ActiveSupport::TestCase c.save! t = Container.find_by_uuid c.container_uuid - assert_equal t.priority, 5 + assert_equal 5, t.priority c2 = ContainerRequest.new c2.container_image = "img" @@ -181,21 +181,21 @@ class ContainerRequestTest < ActiveSupport::TestCase end t.reload - assert_equal t.priority, 10 + assert_equal 10, t.priority c2.reload c2.priority = 0 c2.save! t.reload - assert_equal t.priority, 5 + assert_equal 5, t.priority c.reload c.priority = 0 c.save! t.reload - assert_equal t.priority, 0 + assert_equal 0, t.priority end @@ -221,19 +221,19 @@ class ContainerRequestTest < ActiveSupport::TestCase c2.save! t = Container.find_by_uuid c.container_uuid - assert_equal t.priority, 5 + assert_equal 5, t.priority t2 = Container.find_by_uuid c2.container_uuid - assert_equal t2.priority, 10 + assert_equal 10, t2.priority c.priority = 0 c.save! t.reload - assert_equal t.priority, 0 + assert_equal 0, t.priority t2.reload - assert_equal t2.priority, 0 + assert_equal 0, t2.priority end @@ -259,19 +259,19 @@ class ContainerRequestTest < ActiveSupport::TestCase c2.save! t = Container.find_by_uuid c.container_uuid - assert_equal t.priority, 5 + assert_equal 5, t.priority t2 = Container.find_by_uuid c2.container_uuid - assert_equal t2.priority, 10 + assert_equal 10, t2.priority c.priority = 0 c.save! t.reload - assert_equal t.priority, 0 + assert_equal 0, t.priority t2.reload - assert_equal t2.priority, 10 + assert_equal 10, t2.priority end test "Container container cancel" do @@ -286,10 +286,10 @@ class ContainerRequestTest < ActiveSupport::TestCase c.save! c.reload - assert_equal c.state, "Committed" + assert_equal "Committed", c.state t = Container.find_by_uuid c.container_uuid - assert_equal t.state, "Queued" + assert_equal "Queued", t.state act_as_system_user do t.state = "Cancelled" @@ -297,7 +297,7 @@ class ContainerRequestTest < ActiveSupport::TestCase end c.reload - assert_equal c.state, "Final" + assert_equal "Final", c.state end @@ -314,10 +314,10 @@ class ContainerRequestTest < ActiveSupport::TestCase c.save! c.reload - assert_equal c.state, "Committed" + assert_equal "Committed", c.state t = Container.find_by_uuid c.container_uuid - assert_equal t.state, "Queued" + assert_equal "Queued", t.state act_as_system_user do t.state = "Running" @@ -325,7 +325,7 @@ class ContainerRequestTest < ActiveSupport::TestCase end c.reload - assert_equal c.state, "Committed" + assert_equal "Committed", c.state act_as_system_user do t.state = "Complete" @@ -333,7 +333,7 @@ class ContainerRequestTest < ActiveSupport::TestCase end c.reload - assert_equal c.state, "Final" + assert_equal "Final", c.state end -- 2.30.2