[ARVADOS] created: 4437774e863465c0daa41dfd9716174e18d93122
Git user
git at public.curoverse.com
Thu Mar 30 17:11:21 EDT 2017
at 4437774e863465c0daa41dfd9716174e18d93122 (commit)
commit 4437774e863465c0daa41dfd9716174e18d93122
Author: Tom Clegg <tom at curoverse.com>
Date: Thu Mar 30 17:11:05 2017 -0400
11100: Add comment about delete_at validation race.
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index f7829a5..edd69d0 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -109,6 +109,8 @@ class ContainerRequest < ArvadosModel
end
if self.output_ttl > 0
trash_at = db_current_time + self.output_ttl
+ # delete_at cannot be sooner than blob_signature_ttl, even
+ # after the delay between now and the collection validation.
delete_at = db_current_time +
[self.output_ttl,
Rails.configuration.blob_signature_ttl + 60].max
commit a12ed889f9d0106ea26d0e2d6ff1f74e9ab14aac
Author: Tom Clegg <tom at curoverse.com>
Date: Thu Mar 30 16:56:59 2017 -0400
11100: Clean up permission checks.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index b77ba1c..96dc85e 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -386,36 +386,31 @@ class ArvadosModel < ActiveRecord::Base
raise PermissionDeniedError
end
- # Verify "write" permission on old owner
- # default fail unless one of:
- # owner_uuid did not change
- # previous owner_uuid is nil
- # current user is the old owner
- # current user is this object
- # current user can_write old owner
- unless !owner_uuid_changed? or
- owner_uuid_was.nil? or
- current_user.uuid == self.owner_uuid_was or
- current_user.uuid == self.uuid or
- current_user.can? write: self.owner_uuid_was
- logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{uuid} but does not have permission to write old owner_uuid #{owner_uuid_was}"
- errors.add :owner_uuid, "cannot be changed without write permission on old owner"
- raise PermissionDeniedError
- end
-
- # Verify "write" permission on new owner
- # default fail unless one of:
- # current_user is this object
- # current user can_write new owner, or this object if owner unchanged
- if new_record? or owner_uuid_changed? or is_a?(ApiClientAuthorization)
- write_target = owner_uuid
+ if new_record? || owner_uuid_changed?
+ # Permission on owner_uuid_was is needed to move an existing
+ # object away from its previous owner (which implies permission
+ # to modify this object itself, so we don't need to check that
+ # separately). Permission on the new owner_uuid is also needed.
+ [['old', owner_uuid_was],
+ ['new', owner_uuid]
+ ].each do |which, check_uuid|
+ if check_uuid.nil?
+ # old_owner_uuid is nil? New record, no need to check.
+ elsif !current_user.can?(write: check_uuid)
+ logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
+ errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
+ raise PermissionDeniedError
+ end
+ end
else
- write_target = uuid
- end
- unless current_user == self or current_user.can? write: write_target
- logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{uuid} but does not have permission to write new owner_uuid #{owner_uuid}"
- errors.add :owner_uuid, "cannot be changed without write permission on new owner"
- raise PermissionDeniedError
+ # If the object already existed and we're not changing
+ # owner_uuid, we only need write permission on the object
+ # itself.
+ if !current_user.can?(write: self.uuid)
+ logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} without write permission"
+ errors.add :uuid, "is not writable"
+ raise PermissionDeniedError
+ end
end
true
commit c507b0b072ad62c0087d059aedeaae8bae9b715f
Author: Tom Clegg <tom at curoverse.com>
Date: Thu Mar 30 12:34:12 2017 -0400
11100: Add container_requests.output_ttl field. Fix validation of output/log changes on finalized CRs.
diff --git a/doc/api/methods/container_requests.html.textile.liquid b/doc/api/methods/container_requests.html.textile.liquid
index 3809b2f..446ba15 100644
--- a/doc/api/methods/container_requests.html.textile.liquid
+++ b/doc/api/methods/container_requests.html.textile.liquid
@@ -43,6 +43,8 @@ table(table table-bordered table-condensed).
|cwd|string|Initial working directory, given as an absolute path (in the container) or a path relative to the WORKDIR given in the image's Dockerfile.|Required.|
|command|array of strings|Command to execute in the container.|Required. e.g., @["echo","hello"]@|
|output_path|string|Path to a directory or file inside the container that should be preserved as container's output when it finishes. This path must be, or be inside, one of the mount targets. For best performance, point output_path to a writable collection mount. Also, see "Pre-populate output using Mount points":#pre-populate-output for details regarding optional output pre-population using mount points.|Required.|
+|output_name|string|Desired name for the output collection. If null, a name will be assigned automatically.||
+|output_ttl|integer|Desired lifetime for the output collection. If zero, the output collection will not be deleted automatically.||
|priority|integer|Higher value means spend more resources on this container_request, i.e., go ahead of other queued containers, bring up more nodes etc.|Priority 0 means a container should not be run on behalf of this request. Clients are expected to submit container requests with zero priority in order to preview the container that will be used to satisfy it. Priority can be null if and only if state!="Committed".|
|expires_at|datetime|After this time, priority is considered to be zero.|Not yet implemented.|
|use_existing|boolean|If possible, use an existing (non-failed) container to satisfy the request instead of creating a new one.|Default is true|
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 694c174..f7829a5 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -18,8 +18,9 @@ class ContainerRequest < ArvadosModel
before_validation :validate_scheduling_parameters
before_validation :set_container
validates :command, :container_image, :output_path, :cwd, :presence => true
+ validates :output_ttl, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
validate :validate_state_change
- validate :validate_change
+ validate :check_update_whitelist
after_save :update_priority
after_save :finalize_if_needed
before_create :set_requesting_container_uuid
@@ -41,6 +42,7 @@ class ContainerRequest < ArvadosModel
t.add :output_name
t.add :output_path
t.add :output_uuid
+ t.add :output_ttl
t.add :priority
t.add :properties
t.add :requesting_container_uuid
@@ -64,6 +66,13 @@ class ContainerRequest < ArvadosModel
Committed => [Final]
}
+ AttrsPermittedAlways = [:owner_uuid, :state, :name, :description]
+ AttrsPermittedBeforeCommit = [:command, :container_count_max,
+ :container_image, :cwd, :environment, :filters, :mounts,
+ :output_path, :priority, :properties, :requesting_container_uuid,
+ :runtime_constraints, :state, :container_uuid, :use_existing,
+ :scheduling_parameters, :output_name, :output_ttl]
+
def state_transitions
State_transitions
end
@@ -91,10 +100,19 @@ class ContainerRequest < ArvadosModel
['output', 'log'].each do |out_type|
pdh = c.send(out_type)
next if pdh.nil?
- if self.output_name and out_type == 'output'
- coll_name = self.output_name
- else
- coll_name = "Container #{out_type} for request #{uuid}"
+ coll_name = "Container #{out_type} for request #{uuid}"
+ trash_at = nil
+ delete_at = nil
+ if out_type == 'output'
+ if self.output_name
+ coll_name = self.output_name
+ end
+ if self.output_ttl > 0
+ trash_at = db_current_time + self.output_ttl
+ delete_at = db_current_time +
+ [self.output_ttl,
+ Rails.configuration.blob_signature_ttl + 60].max
+ end
end
manifest = Collection.unscoped do
Collection.where(portable_data_hash: pdh).first.manifest_text
@@ -104,6 +122,8 @@ class ContainerRequest < ArvadosModel
manifest_text: manifest,
portable_data_hash: pdh,
name: coll_name,
+ trash_at: trash_at,
+ delete_at: delete_at,
properties: {
'type' => out_type,
'container_request' => uuid,
@@ -132,6 +152,7 @@ class ContainerRequest < ArvadosModel
self.cwd ||= "."
self.container_count_max ||= Rails.configuration.container_count_max
self.scheduling_parameters ||= {}
+ self.output_ttl ||= 0
end
def set_container
@@ -183,57 +204,45 @@ class ContainerRequest < ArvadosModel
end
end
- def validate_change
- permitted = [:owner_uuid]
+ def check_update_whitelist
+ permitted = AttrsPermittedAlways.dup
- 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, :container_uuid, :use_existing, :scheduling_parameters,
- :output_name
+ if self.new_record? || self.state_was == Uncommitted
+ # Allow create-and-commit in a single operation.
+ permitted.push *AttrsPermittedBeforeCommit
+ end
+ case self.state
when Committed
- if container_uuid.nil?
- errors.add :container_uuid, "has not been resolved to a container."
- end
+ permitted.push :priority, :container_count_max, :container_uuid
- if priority.nil?
- errors.add :priority, "cannot be nil"
+ if self.container_uuid.nil?
+ self.errors.add :container_uuid, "has not been resolved to a container."
end
- # Can update priority, container count, name and description
- permitted.push :priority, :container_count, :container_count_max, :container_uuid,
- :name, :description
+ if self.priority.nil?
+ self.errors.add :priority, "cannot be nil"
+ end
- if self.state_changed?
- # 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, :use_existing, :scheduling_parameters,
- :output_name
+ # Allow container count to increment by 1
+ if (self.container_uuid &&
+ self.container_uuid != self.container_uuid_was &&
+ self.container_count == 1 + (self.container_count_was || 0))
+ permitted.push :container_count
end
when Final
- if not current_user.andand.is_admin and not (self.name_changed? || self.description_changed?)
- errors.add :state, "of container request can only be set to Final by system."
+ if self.state_changed? and not current_user.andand.is_admin
+ self.errors.add :state, "of container request can only be set to Final by system."
end
- if self.state_changed? || self.name_changed? || self.description_changed? || self.output_uuid_changed? || self.log_uuid_changed?
- permitted.push :state, :name, :description, :output_uuid, :log_uuid
- else
- errors.add :state, "does not allow updates"
+ if self.state_was == Committed
+ permitted.push :output_uuid, :log_uuid
end
- else
- errors.add :state, "invalid value"
end
- check_update_whitelist permitted
+ super(permitted)
end
def update_priority
diff --git a/services/api/db/migrate/20170330012505_add_output_ttl_to_container_requests.rb b/services/api/db/migrate/20170330012505_add_output_ttl_to_container_requests.rb
new file mode 100644
index 0000000..ee6fa37
--- /dev/null
+++ b/services/api/db/migrate/20170330012505_add_output_ttl_to_container_requests.rb
@@ -0,0 +1,5 @@
+class AddOutputTtlToContainerRequests < ActiveRecord::Migration
+ def change
+ add_column :container_requests, :output_ttl, :integer, default: 0, null: false
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index d877452..e25a2a9 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -297,7 +297,8 @@ CREATE TABLE container_requests (
scheduling_parameters text,
output_uuid character varying(255),
log_uuid character varying(255),
- output_name character varying(255) DEFAULT NULL::character varying
+ output_name character varying(255) DEFAULT NULL::character varying,
+ output_ttl integer DEFAULT 0 NOT NULL
);
@@ -2753,4 +2754,6 @@ INSERT INTO schema_migrations (version) VALUES ('20170216170823');
INSERT INTO schema_migrations (version) VALUES ('20170301225558');
-INSERT INTO schema_migrations (version) VALUES ('20170328215436');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20170328215436');
+
+INSERT INTO schema_migrations (version) VALUES ('20170330012505');
\ No newline at end of file
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index b268ce4..f9ce41c 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -3,6 +3,7 @@ require 'helpers/docker_migration_helper'
class ContainerRequestTest < ActiveSupport::TestCase
include DockerMigrationHelper
+ include DbCurrentTime
def create_minimal_req! attrs={}
defaults = {
@@ -579,38 +580,65 @@ class ContainerRequestTest < ActiveSupport::TestCase
test "Output collection name setting using output_name with name collision resolution" do
set_user_from_auth :active
- output_name = collections(:foo_file).name
+ output_name = 'unimaginative name'
+ Collection.create!(name: output_name)
cr = create_minimal_req!(priority: 1,
state: ContainerRequest::Committed,
output_name: output_name)
- act_as_system_user do
- c = Container.find_by_uuid(cr.container_uuid)
- c.update_attributes!(state: Container::Locked)
- c.update_attributes!(state: Container::Running)
- c.update_attributes!(state: Container::Complete,
- exit_code: 0,
- output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
- log: 'fa7aeb5140e2848d39b416daeef4ffc5+45')
- end
- cr.save
+ run_container(cr)
+ cr.reload
assert_equal ContainerRequest::Final, cr.state
output_coll = Collection.find_by_uuid(cr.output_uuid)
# Make sure the resulting output collection name include the original name
# plus the date
assert_not_equal output_name, output_coll.name,
- "It shouldn't exist more than one collection with the same owner and name '${output_name}'"
+ "more than one collection with the same owner and name"
assert output_coll.name.include?(output_name),
"New name should include original name"
assert_match /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z/, output_coll.name,
"New name should include ISO8601 date"
end
- test "Finalize committed request when reusing a finished container" do
- set_user_from_auth :active
- cr = create_minimal_req!(priority: 1, state: ContainerRequest::Committed)
- cr.reload
- assert_equal ContainerRequest::Committed, cr.state
+ [[0, :check_output_ttl_0],
+ [1, :check_output_ttl_1s],
+ [365*86400, :check_output_ttl_1y],
+ ].each do |ttl, checker|
+ test "output_ttl=#{ttl}" do
+ act_as_user users(:active) do
+ cr = create_minimal_req!(priority: 1,
+ state: ContainerRequest::Committed,
+ output_name: 'foo',
+ output_ttl: ttl)
+ run_container(cr)
+ cr.reload
+ output = Collection.find_by_uuid(cr.output_uuid)
+ send(checker, db_current_time, output.trash_at, output.delete_at)
+ end
+ end
+ end
+
+ def check_output_ttl_0(now, trash, delete)
+ assert_nil(trash)
+ assert_nil(delete)
+ end
+
+ def check_output_ttl_1s(now, trash, delete)
+ assert_not_nil(trash)
+ assert_not_nil(delete)
+ assert_in_delta(trash, now + 1.second, 10)
+ assert_in_delta(delete, now + Rails.configuration.blob_signature_ttl.second, 120)
+ end
+
+ def check_output_ttl_1y(now, trash, delete)
+ year = (86400*365).second
+ assert_not_nil(trash)
+ assert_not_nil(delete)
+ assert_in_delta(trash, now + year, 20)
+ assert_in_delta(delete, now + year, 20)
+ end
+
+ def run_container(cr)
act_as_system_user do
c = Container.find_by_uuid(cr.container_uuid)
c.update_attributes!(state: Container::Locked)
@@ -619,7 +647,16 @@ class ContainerRequestTest < ActiveSupport::TestCase
exit_code: 0,
output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
log: 'fa7aeb5140e2848d39b416daeef4ffc5+45')
+ c
end
+ end
+
+ test "Finalize committed request when reusing a finished container" do
+ set_user_from_auth :active
+ cr = create_minimal_req!(priority: 1, state: ContainerRequest::Committed)
+ cr.reload
+ assert_equal ContainerRequest::Committed, cr.state
+ run_container(cr)
cr.reload
assert_equal ContainerRequest::Final, cr.state
@@ -665,4 +702,48 @@ class ContainerRequestTest < ActiveSupport::TestCase
end
end
end
+
+ [['Committed', true, {name: "foobar", priority: 123}],
+ ['Committed', false, {container_count: 2}],
+ ['Committed', false, {container_count: 0}],
+ ['Committed', false, {container_count: nil}],
+ ['Final', false, {state: ContainerRequest::Committed, name: "foobar"}],
+ ['Final', false, {name: "foobar", priority: 123}],
+ ['Final', false, {name: "foobar", output_uuid: "zzzzz-4zz18-znfnqtbbv4spc3w"}],
+ ['Final', false, {name: "foobar", log_uuid: "zzzzz-4zz18-znfnqtbbv4spc3w"}],
+ ['Final', false, {log_uuid: "zzzzz-4zz18-znfnqtbbv4spc3w"}],
+ ['Final', false, {priority: 123}],
+ ['Final', false, {mounts: {}}],
+ ['Final', false, {container_count: 2}],
+ ['Final', true, {name: "foobar"}],
+ ['Final', true, {name: "foobar", description: "baz"}],
+ ].each do |state, permitted, updates|
+ test "state=#{state} can#{'not' if !permitted} update #{updates.inspect}" do
+ act_as_user users(:active) do
+ cr = create_minimal_req!(priority: 1,
+ state: "Committed",
+ container_count_max: 1)
+ case state
+ when 'Committed'
+ # already done
+ when 'Final'
+ act_as_system_user do
+ Container.find_by_uuid(cr.container_uuid).
+ update_attributes!(state: Container::Cancelled)
+ end
+ cr.reload
+ else
+ raise 'broken test case'
+ end
+ assert_equal state, cr.state
+ if permitted
+ assert cr.update_attributes!(updates)
+ else
+ assert_raises(ActiveRecord::RecordInvalid) do
+ cr.update_attributes!(updates)
+ end
+ end
+ end
+ end
+ end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list