From c507b0b072ad62c0087d059aedeaae8bae9b715f Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Thu, 30 Mar 2017 12:34:12 -0400 Subject: [PATCH] 11100: Add container_requests.output_ttl field. Fix validation of output/log changes on finalized CRs. --- .../container_requests.html.textile.liquid | 2 + services/api/app/models/container_request.rb | 89 ++++++++------ ...05_add_output_ttl_to_container_requests.rb | 5 + services/api/db/structure.sql | 7 +- .../api/test/unit/container_request_test.rb | 115 +++++++++++++++--- 5 files changed, 159 insertions(+), 59 deletions(-) create mode 100644 services/api/db/migrate/20170330012505_add_output_ttl_to_container_requests.rb diff --git a/doc/api/methods/container_requests.html.textile.liquid b/doc/api/methods/container_requests.html.textile.liquid index 3809b2f1eb..446ba1518a 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 694c174812..f7829a54c7 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 0000000000..ee6fa37a3c --- /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 d877452f20..e25a2a9605 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 b268ce4220..f9ce41c1e1 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 -- 2.30.2