[ARVADOS] updated: b1b93d2d90e58fbdfe4a4f8e363a4705dbd39bd5
git at public.curoverse.com
git at public.curoverse.com
Mon Dec 7 16:28:19 EST 2015
Summary of changes:
services/api/app/models/container.rb | 97 +++++++++++----
services/api/app/models/container_request.rb | 143 ++++++++++++-----------
services/api/lib/whitelist_update.rb | 9 ++
services/api/test/unit/container_request_test.rb | 140 +++++++++++++++++++++-
services/api/test/unit/container_test.rb | 16 +++
5 files changed, 311 insertions(+), 94 deletions(-)
create mode 100644 services/api/lib/whitelist_update.rb
via b1b93d2d90e58fbdfe4a4f8e363a4705dbd39bd5 (commit)
via 5044c03a66e63b3e1fe4e0fdeec4a3f77fed0310 (commit)
from df439a7eb705da7aae720a43c85c462cc6235e5a (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 b1b93d2d90e58fbdfe4a4f8e363a4705dbd39bd5
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Mon Dec 7 16:27:42 2015 -0500
6429: Committing container request creates container
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 4d1908f..f87004a 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -15,6 +15,8 @@ class Container < ArvadosModel
before_validation :set_timestamps
validates :command, :container_image, :output_path, :cwd, :presence => true
validate :validate_change
+ after_save :request_finalize
+ after_save :process_tree_priority
has_many :container_requests, :foreign_key => :container_uuid, :class_name => 'ContainerRequest', :primary_key => :uuid
@@ -44,6 +46,27 @@ class Container < ArvadosModel
(Cancelled = 'Cancelled')
]
+ def self.resolve req
+ 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 })
+ end
+
+ def update_priority!
+ max = 0
+ ContainerRequest.where(container_uuid: uuid).each do |cr|
+ if cr.priority > max
+ max = cr.priority
+ end
+ end
+ self.priority = max
+ self.save!
+ end
+
protected
def fill_field_defaults
@@ -134,4 +157,28 @@ class Container < ArvadosModel
check_update_whitelist permitted
end
+ def request_finalize
+ 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 = ContainerRequest.Final
+ cr.save!
+ end
+ end
+ end
+ end
+
+ def process_tree_priority
+ if self.priority_changed?
+ 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
+ end
+ end
+ end
+ end
+
end
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index aaebcd3..f936468 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -13,8 +13,10 @@ class ContainerRequest < ArvadosModel
serialize :command, Array
before_validation :fill_field_defaults, :if => :new_record?
+ before_validation :set_container
validates :command, :container_image, :output_path, :cwd, :presence => true
validate :validate_change
+ after_save :update_priority
api_accessible :user, extend: :common do |t|
t.add :command
@@ -55,6 +57,20 @@ class ContainerRequest < ArvadosModel
self.priority ||= 1
end
+ def set_container
+ 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
+ Link.create!(head_uuid: self.container_uuid,
+ tail_uuid: self.owner_uuid,
+ link_class: "permission",
+ name: "can_read")
+ end
+ end
+ end
+ end
+
def validate_change
permitted = [:owner_uuid]
@@ -65,28 +81,29 @@ class ContainerRequest < ArvadosModel
:container_image, :cwd, :description, :environment,
:filters, :mounts, :name, :output_path, :priority,
:properties, :requesting_container_uuid, :runtime_constraints,
- :state
+ :state, :container_uuid
- if self.container_uuid_changed? and (current_user.andand.is_admin or self.container_uuid.nil?)
- permitted.push :container_uuid
+ when Committed
+ if container_uuid.nil?
+ errors.add :container_uuid, "Has not been resolved to a container."
end
- when Committed
- # Can only update priority, container count.
+ # Can update priority, container count.
permitted.push :priority, :container_count_max
if self.state_changed?
- if self.state_was == Uncommitted
- permitted.push :state
+ if self.state_was == Uncommitted or self.state_was.nil?
+ # Allow create-and-commit in a single operation.
+ permitted.push :command, :container_count_max,
+ :container_image, :cwd, :description, :environment,
+ :filters, :mounts, :name, :output_path, :priority,
+ :properties, :requesting_container_uuid, :runtime_constraints,
+ :state, :container_uuid
else
errors.add :state, "Can only go from Uncommitted to Committed"
end
end
- if self.container_uuid_changed? and current_user.andand.is_admin
- permitted.push :container_uuid
- end
-
when Final
if self.state_changed?
if self.state_was == Committed
@@ -105,4 +122,11 @@ class ContainerRequest < ArvadosModel
check_update_whitelist permitted
end
+ def update_priority
+ if self.state == Committed and self.priority_changed?
+ c = Container.find_by_uuid self.container_uuid
+ c.update_priority!
+ end
+ end
+
end
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index d962f30..1248475 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -1,7 +1,141 @@
require 'test_helper'
class ContainerRequestTest < 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
+
+ assert_raises(ActiveRecord::RecordInvalid) do
+ c.reload
+ c.name = "baz"
+ c.save!
+ end
+
+ assert_raises(ActiveRecord::RecordInvalid) do
+ c.reload
+ c.description = "baz"
+ 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 request create" do
+ set_user_from_auth :active_trustedclient
+ c = ContainerRequest.new
+ c.command = ["echo", "foo"]
+ c.container_image = "img"
+ c.cwd = "/tmp"
+ c.environment = {}
+ c.mounts = {"BAR" => "FOO"}
+ c.output_path = "/tmp"
+ c.priority = 1
+ c.runtime_constraints = {}
+ c.name = "foo"
+ c.description = "bar"
+ c.save!
+
+ assert_nil c.container_uuid
+
+ check_bogus_states c
+
+ c.reload
+ c.command = ["echo", "foo3"]
+ c.container_image = "img3"
+ c.cwd = "/tmp3"
+ c.environment = {"BUP" => "BOP"}
+ c.mounts = {"BAR" => "BAZ"}
+ c.output_path = "/tmp4"
+ c.priority = 2
+ c.runtime_constraints = {"X" => "Y"}
+ c.name = "foo3"
+ c.description = "bar3"
+ c.save!
+
+ assert_nil c.container_uuid
+ end
+
+ test "Container request commit" do
+ set_user_from_auth :active_trustedclient
+ c = ContainerRequest.new
+ c.command = ["echo", "foo"]
+ c.container_image = "img"
+ c.cwd = "/tmp"
+ c.environment = {}
+ c.mounts = {"BAR" => "FOO"}
+ c.output_path = "/tmp"
+ c.priority = 1
+ c.runtime_constraints = {}
+ c.name = "foo"
+ c.description = "bar"
+ c.save!
+
+ c.reload
+ assert_nil c.container_uuid
+
+ c.reload
+ c.state = "Committed"
+ c.save!
+
+ c.reload
+
+ t = Container.find_by_uuid c.container_uuid
+ assert_equal c.command, t.command
+ assert_equal c.container_image, t.container_image
+ assert_equal c.cwd, t.cwd
+ end
+
+
end
commit 5044c03a66e63b3e1fe4e0fdeec4a3f77fed0310
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Mon Dec 7 12:57:57 2015 -0500
6429: Put whitelist_update in module.
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 9499d89..4d1908f 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -1,14 +1,17 @@
+require 'whitelist_update'
+
class Container < ArvadosModel
include HasUuid
include KindAndEtag
include CommonApiTemplate
+ include WhitelistUpdate
serialize :environment, Hash
serialize :mounts, Hash
serialize :runtime_constraints, Hash
serialize :command, Array
- before_validation :fill_field_defaults
+ before_validation :fill_field_defaults, :if => :new_record?
before_validation :set_timestamps
validates :command, :container_image, :output_path, :cwd, :presence => true
validate :validate_change
@@ -41,19 +44,17 @@ class Container < ArvadosModel
(Cancelled = 'Cancelled')
]
+ protected
+
def fill_field_defaults
- if self.new_record?
- self.state ||= Queued
- self.environment ||= {}
- self.runtime_constraints ||= {}
- self.mounts ||= {}
- self.cwd ||= "."
- self.priority ||= 1
- end
+ self.state ||= Queued
+ self.environment ||= {}
+ self.runtime_constraints ||= {}
+ self.mounts ||= {}
+ self.cwd ||= "."
+ self.priority ||= 1
end
- protected
-
def permission_to_create
current_user.andand.is_admin
end
@@ -62,14 +63,6 @@ class Container < ArvadosModel
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
@@ -81,7 +74,7 @@ class Container < ArvadosModel
end
def validate_change
- permitted = [:modified_at, :modified_by_user_uuid, :modified_by_client_uuid]
+ permitted = []
if self.new_record?
permitted.push :owner_uuid, :command, :container_image, :cwd, :environment,
@@ -91,29 +84,36 @@ class Container < ArvadosModel
case self.state
when Queued
# permit priority change only.
+ permitted.push :priority
+
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?
+ # At point of state change, can only set state and started_at
if self.state_was == Queued
permitted.push :state, :started_at
else
- errors.add :state, "Can only go to Runinng from Queued"
+ errors.add :state, "Can only go from Queued to Running"
end
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} from #{self.state}"
+ errors.add :state, "Cannot go from #{self.state_was} to #{self.state}"
end
+ else
+ errors.add :state, "Cannot update record in Complete state"
end
+
when Cancelled
if self.state_changed?
if self.state_was == Running
@@ -121,17 +121,17 @@ class Container < ArvadosModel
elsif self.state_was == Queued
permitted.push :state, :finished_at
else
- errors.add :state, "Cannot go from #{self.state_was} from #{self.state}"
+ errors.add :state, "Cannot go from #{self.state_was} to #{self.state}"
end
+ else
+ errors.add :state, "Cannot update record in Cancelled state"
end
+
else
errors.add :state, "Invalid state #{self.state}"
end
- check_permitted_updates permitted
- end
-
- def validate_fields
+ check_update_whitelist permitted
end
end
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 22b6bde..aaebcd3 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -1,7 +1,10 @@
+require 'whitelist_update'
+
class ContainerRequest < ArvadosModel
include HasUuid
include KindAndEtag
include CommonApiTemplate
+ include WhitelistUpdate
serialize :properties, Hash
serialize :environment, Hash
@@ -9,10 +12,9 @@ class ContainerRequest < ArvadosModel
serialize :runtime_constraints, Hash
serialize :command, Array
- before_create :set_state_before_save
- validate :validate_change_permitted
- validate :validate_status
- validate :validate_state_change
+ before_validation :fill_field_defaults, :if => :new_record?
+ validates :command, :container_image, :output_path, :cwd, :presence => true
+ validate :validate_change
api_accessible :user, extend: :common do |t|
t.add :command
@@ -42,80 +44,65 @@ class ContainerRequest < ArvadosModel
(Final = 'Final'),
]
- def set_state_before_save
+ protected
+
+ def fill_field_defaults
self.state ||= Uncommitted
+ self.environment ||= {}
+ self.runtime_constraints ||= {}
+ self.mounts ||= {}
+ self.cwd ||= "."
+ self.priority ||= 1
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"
+ def validate_change
+ permitted = [:owner_uuid]
+
+ case self.state
+ when Uncommitted
+ # Permit updating most fields
+ permitted.push :command, :container_count_max,
+ :container_image, :cwd, :description, :environment,
+ :filters, :mounts, :name, :output_path, :priority,
+ :properties, :requesting_container_uuid, :runtime_constraints,
+ :state
+
+ if self.container_uuid_changed? and (current_user.andand.is_admin or self.container_uuid.nil?)
+ permitted.push :container_uuid
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
+ when Committed
+ # Can only update priority, container count.
+ permitted.push :priority, :container_count_max
- 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}"
+ if self.state_changed?
+ if self.state_was == Uncommitted
+ permitted.push :state
+ else
+ errors.add :state, "Can only go from Uncommitted to Committed"
+ end
end
+
+ if self.container_uuid_changed? and current_user.andand.is_admin
+ permitted.push :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"
+ end
+
+ else
+ errors.add :state, "Invalid state #{self.state}"
end
- ok
- end
+ check_update_whitelist permitted
+ end
end
diff --git a/services/api/lib/whitelist_update.rb b/services/api/lib/whitelist_update.rb
new file mode 100644
index 0000000..7413edf
--- /dev/null
+++ b/services/api/lib/whitelist_update.rb
@@ -0,0 +1,9 @@
+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}"
+ end
+ end
+ end
+end
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 7d8d8f0..d3216fc 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -181,4 +181,20 @@ class ContainerTest < ActiveSupport::TestCase
end
end
+ test "Container create forbidden for non-admin" do
+ set_user_from_auth :active_trustedclient
+ c = Container.new
+ c.command = ["echo", "foo"]
+ c.container_image = "img"
+ c.cwd = "/tmp"
+ c.environment = {}
+ c.mounts = {"BAR" => "FOO"}
+ c.output_path = "/tmp"
+ c.priority = 1
+ c.runtime_constraints = {}
+ assert_raises(ArvadosModel::PermissionDeniedError) do
+ c.save!
+ end
+ end
+
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list