[ARVADOS] updated: ce6f582e7e1d5b927aeee0aab3def7ab8a5cae4f
git at public.curoverse.com
git at public.curoverse.com
Wed Dec 9 15:06:51 EST 2015
Summary of changes:
.../app/models/{containers.rb => container.rb} | 0
...{container_requests.rb => container_request.rb} | 0
services/api/app/models/container.rb | 70 +++++++++-------------
services/api/app/models/container_request.rb | 52 ++++++++++------
...0151202151426_create_containers_and_requests.rb | 18 +++---
services/api/db/structure.sql | 18 +++---
services/api/lib/whitelist_update.rb | 11 +++-
services/api/test/factories/container_requests.rb | 29 ---------
services/api/test/factories/containers.rb | 26 --------
services/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
via ce6f582e7e1d5b927aeee0aab3def7ab8a5cae4f (commit)
from 6fc8952ed133607f5ce317d929d731657e405edf (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 ce6f582e7e1d5b927aeee0aab3def7ab8a5cae4f
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Dec 9 15:06:46 2015 -0500
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.
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 f452b10..15bbbe0 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 9dc5c7b..1011f6f 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 99611b4..4741515 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 7ff4af2..1ce44cd 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 7413edf..a81f992 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 c710b1a..0000000
--- 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 5c2cd3d..0000000
--- 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 32a60f8..6d1a576 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
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list