[ARVADOS] created: 1.3.0-2163-gcf0171de6
Git user
git at public.arvados.org
Tue Feb 11 23:16:12 UTC 2020
at cf0171de6e0f875748cc80026c9ea8a11147c750 (commit)
commit cf0171de6e0f875748cc80026c9ea8a11147c750
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Tue Feb 11 20:14:42 2020 -0300
16144: Adds more checks and simplifies nested conditionals.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index e7d0aba59..b30b8cc1d 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -159,26 +159,27 @@ class ContainerRequest < ArvadosModel
# finished/cancelled.
def finalize!
container = Container.find_by_uuid(container_uuid)
- update_collections(container: container)
-
- if container.state == Container::Complete
- log_col = Collection.where(portable_data_hash: container.log).first
- if log_col
- # Need to save collection
- completed_coll = Collection.new(
- owner_uuid: self.owner_uuid,
- name: "Container log for container #{container_uuid}",
- properties: {
- 'type' => 'log',
- 'container_request' => self.uuid,
- 'container_uuid' => container_uuid,
- },
- portable_data_hash: log_col.portable_data_hash,
- manifest_text: log_col.manifest_text)
- completed_coll.save_with_unique_name!
+ if !container.nil?
+ update_collections(container: container)
+
+ if container.state == Container::Complete
+ log_col = Collection.where(portable_data_hash: container.log).first
+ if log_col
+ # Need to save collection
+ completed_coll = Collection.new(
+ owner_uuid: self.owner_uuid,
+ name: "Container log for container #{container_uuid}",
+ properties: {
+ 'type' => 'log',
+ 'container_request' => self.uuid,
+ 'container_uuid' => container_uuid,
+ },
+ portable_data_hash: log_col.portable_data_hash,
+ manifest_text: log_col.manifest_text)
+ completed_coll.save_with_unique_name!
+ end
end
end
-
update_attributes!(state: Final)
end
@@ -277,34 +278,36 @@ class ContainerRequest < ArvadosModel
if self.container_count_changed?
errors.add :container_count, "cannot be updated directly."
return false
- else
- self.container_count += 1
- if self.container_uuid_was
- old_container = Container.find_by_uuid(self.container_uuid_was)
- old_logs = Collection.where(portable_data_hash: old_container.log).first
- if old_logs
- log_coll = self.log_uuid.nil? ? nil : Collection.where(uuid: self.log_uuid).first
- if self.log_uuid.nil?
- log_coll = Collection.new(
- owner_uuid: self.owner_uuid,
- name: coll_name = "Container log for request #{uuid}",
- manifest_text: "")
- end
+ end
- # copy logs from old container into CR's log collection
- src = Arv::Collection.new(old_logs.manifest_text)
- dst = Arv::Collection.new(log_coll.manifest_text)
- dst.cp_r("./", "log for container #{old_container.uuid}", src)
- manifest = dst.manifest_text
-
- log_coll.assign_attributes(
- portable_data_hash: Digest::MD5.hexdigest(manifest) + '+' + manifest.bytesize.to_s,
- manifest_text: manifest)
- log_coll.save_with_unique_name!
- self.log_uuid = log_coll.uuid
- end
- end
+ self.container_count += 1
+ return if self.container_uuid_was.nil?
+
+ old_container = Container.find_by_uuid(self.container_uuid_was)
+ return if old_container.nil?
+
+ old_logs = Collection.where(portable_data_hash: old_container.log).first
+ return if old_logs.nil?
+
+ log_coll = self.log_uuid.nil? ? nil : Collection.where(uuid: self.log_uuid).first
+ if self.log_uuid.nil?
+ log_coll = Collection.new(
+ owner_uuid: self.owner_uuid,
+ name: coll_name = "Container log for request #{uuid}",
+ manifest_text: "")
end
+
+ # copy logs from old container into CR's log collection
+ src = Arv::Collection.new(old_logs.manifest_text)
+ dst = Arv::Collection.new(log_coll.manifest_text)
+ dst.cp_r("./", "log for container #{old_container.uuid}", src)
+ manifest = dst.manifest_text
+
+ log_coll.assign_attributes(
+ portable_data_hash: Digest::MD5.hexdigest(manifest) + '+' + manifest.bytesize.to_s,
+ manifest_text: manifest)
+ log_coll.save_with_unique_name!
+ self.log_uuid = log_coll.uuid
end
end
commit 48b50ca46cbb153aa186105b4b1d85ab47f1740b
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Tue Feb 11 19:22:24 2020 -0300
16144: Fixes the bug.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 5a7818147..e7d0aba59 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -125,10 +125,10 @@ class ContainerRequest < ArvadosModel
# (same order as Container#handle_completed). Locking always
# reloads the Container and ContainerRequest records.
c = Container.find_by_uuid(container_uuid)
- c.lock!
+ c.lock! if !c.nil?
self.lock!
- if container_uuid != c.uuid
+ if !c.nil? && container_uuid != c.uuid
# After locking, we've noticed a race, the container_uuid is
# different than the container record we just loaded. This
# can happen if Container#handle_completed scheduled a new
@@ -138,13 +138,18 @@ class ContainerRequest < ArvadosModel
redo
end
- if state == Committed && c.final?
- # The current container is
- act_as_system_user do
- leave_modified_by_user_alone do
- finalize!
+ if !c.nil?
+ if state == Committed && c.final?
+ # The current container is
+ act_as_system_user do
+ leave_modified_by_user_alone do
+ finalize!
+ end
end
end
+ elsif state == Committed
+ # Behave as if the container is cancelled
+ update_attributes!(state: Final)
end
return true
end
@@ -181,6 +186,10 @@ class ContainerRequest < ArvadosModel
collections.each do |out_type|
pdh = container.send(out_type)
next if pdh.nil?
+ c = Collection.where(portable_data_hash: pdh).first
+ next if c.nil?
+ manifest = c.manifest_text
+
coll_name = "Container #{out_type} for request #{uuid}"
trash_at = nil
if out_type == 'output'
@@ -191,7 +200,6 @@ class ContainerRequest < ArvadosModel
trash_at = db_current_time + self.output_ttl
end
end
- manifest = Collection.where(portable_data_hash: pdh).first.manifest_text
coll_uuid = self.send(out_type + '_uuid')
coll = coll_uuid.nil? ? nil : Collection.where(uuid: coll_uuid).first
commit 22e5442051df04f9e0d74b21bd36e44a6464e945
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Tue Feb 11 19:21:09 2020 -0300
16144: Adds tests exposing the bug.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index b04bae864..b91910d2d 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -261,6 +261,67 @@ class ContainerRequestTest < ActiveSupport::TestCase
assert_equal log.owner_uuid, project.uuid, "Container log should be copied to #{project.uuid}"
end
+ # This tests bug report #16144
+ test "Request is finalized when its container is completed even when log & output don't exist" do
+ set_user_from_auth :active
+ project = groups(:private)
+ cr = create_minimal_req!(owner_uuid: project.uuid,
+ priority: 1,
+ state: "Committed")
+ assert_equal users(:active).uuid, cr.modified_by_user_uuid
+
+ output_pdh = '1f4b0bc7583c2a7f9102c395f4ffc5e3+45'
+ log_pdh = 'fa7aeb5140e2848d39b416daeef4ffc5+45'
+
+ c = 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,
+ output: output_pdh,
+ log: log_pdh)
+ c
+ end
+
+ cr.reload
+ assert_equal "Committed", cr.state
+
+ act_as_system_user do
+ Collection.where(portable_data_hash: output_pdh).delete_all
+ Collection.where(portable_data_hash: log_pdh).delete_all
+ c.update_attributes!(state: Container::Complete)
+ end
+
+ cr.reload
+ assert_equal "Final", cr.state
+ end
+
+ # This tests bug report #16144
+ test "Can destroy CR even if its container doesn't exist" do
+ set_user_from_auth :active
+ project = groups(:private)
+ cr = create_minimal_req!(owner_uuid: project.uuid,
+ priority: 1,
+ state: "Committed")
+ assert_equal users(:active).uuid, cr.modified_by_user_uuid
+
+ c = 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
+ end
+
+ cr.reload
+ assert_equal "Committed", cr.state
+
+ cr_uuid = cr.uuid
+ act_as_system_user do
+ Container.find_by_uuid(cr.container_uuid).destroy
+ cr.destroy
+ end
+ assert_nil ContainerRequest.find_by_uuid(cr_uuid)
+ end
+
test "Container makes container request, then is cancelled" do
set_user_from_auth :active
cr = create_minimal_req!(priority: 5, state: "Committed", container_count_max: 1)
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list