From 378eeb13b1c4d188d07bde0abfe3955ad53d4beb Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Thu, 29 Mar 2018 16:25:12 -0300 Subject: [PATCH] 13168: Use helper to avoid updating 'modified_by_user_uuid' field on CRs. When a container's state changes, the related container requests are updated using the system user privileges, now these updates don't assign the system user's uuid to the container requests 'modified_by_user_uuid' field. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- services/api/app/models/arvados_model.rb | 6 +++- services/api/app/models/container.rb | 34 +++++++++++-------- services/api/app/models/container_request.rb | 5 ++- services/api/lib/arvados_model_updates.rb | 21 ++++++++++++ .../api/test/unit/container_request_test.rb | 6 ++++ 5 files changed, 56 insertions(+), 16 deletions(-) create mode 100644 services/api/lib/arvados_model_updates.rb diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 6a59d3bbaa..b9edeae06e 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -2,6 +2,7 @@ # # SPDX-License-Identifier: AGPL-3.0 +require 'arvados_model_updates' require 'has_uuid' require 'record_filters' require 'serializers' @@ -10,6 +11,7 @@ require 'request_error' class ArvadosModel < ActiveRecord::Base self.abstract_class = true + include ArvadosModelUpdates include CurrentApiClient # current_user, current_api_client, etc. include DbCurrentTime extend RecordFilters @@ -539,7 +541,9 @@ class ArvadosModel < ActiveRecord::Base self.updated_at = current_time self.owner_uuid ||= current_default_owner if self.respond_to? :owner_uuid= self.modified_at = current_time - self.modified_by_user_uuid = current_user ? current_user.uuid : nil + if !anonymous_updater + self.modified_by_user_uuid = current_user ? current_user.uuid : nil + end self.modified_by_client_uuid = current_api_client ? current_api_client.uuid : nil true end diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index 3765266405..8882b2c763 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -7,6 +7,7 @@ require 'whitelist_update' require 'safe_json' class Container < ArvadosModel + include ArvadosModelUpdates include HasUuid include KindAndEtag include CommonApiTemplate @@ -560,9 +561,11 @@ class Container < ArvadosModel c = Container.create! c_attrs retryable_requests.each do |cr| cr.with_lock do - # Use row locking because this increments container_count - cr.container_uuid = c.uuid - cr.save! + leave_modified_by_user_alone do + # Use row locking because this increments container_count + cr.container_uuid = c.uuid + cr.save! + end end end end @@ -570,7 +573,9 @@ class Container < ArvadosModel # Notify container requests associated with this container ContainerRequest.where(container_uuid: uuid, state: ContainerRequest::Committed).each do |cr| - cr.finalize! + leave_modified_by_user_alone do + cr.finalize! + end end # Cancel outstanding container requests made by this container. @@ -578,19 +583,20 @@ class Container < ArvadosModel includes(:container). where(requesting_container_uuid: uuid, state: ContainerRequest::Committed).each do |cr| - cr.update_attributes!(priority: 0) - cr.container.reload - if cr.container.state == Container::Queued || cr.container.state == Container::Locked - # If the child container hasn't started yet, finalize the - # child CR now instead of leaving it "on hold", i.e., - # Queued with priority 0. (OTOH, if the child is already - # running, leave it alone so it can get cancelled the - # usual way, get a copy of the log collection, etc.) - cr.update_attributes!(state: ContainerRequest::Final) + leave_modified_by_user_alone do + cr.update_attributes!(priority: 0) + cr.container.reload + if cr.container.state == Container::Queued || cr.container.state == Container::Locked + # If the child container hasn't started yet, finalize the + # child CR now instead of leaving it "on hold", i.e., + # Queued with priority 0. (OTOH, if the child is already + # running, leave it alone so it can get cancelled the + # usual way, get a copy of the log collection, etc.) + cr.update_attributes!(state: ContainerRequest::Final) + end 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 3587cf046a..bc01b33652 100644 --- a/services/api/app/models/container_request.rb +++ b/services/api/app/models/container_request.rb @@ -5,6 +5,7 @@ require 'whitelist_update' class ContainerRequest < ArvadosModel + include ArvadosModelUpdates include HasUuid include KindAndEtag include CommonApiTemplate @@ -110,7 +111,9 @@ class ContainerRequest < ArvadosModel if state == Committed && Container.find_by_uuid(container_uuid).final? reload act_as_system_user do - finalize! + leave_modified_by_user_alone do + finalize! + end end end end diff --git a/services/api/lib/arvados_model_updates.rb b/services/api/lib/arvados_model_updates.rb new file mode 100644 index 0000000000..b456bd3956 --- /dev/null +++ b/services/api/lib/arvados_model_updates.rb @@ -0,0 +1,21 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +module ArvadosModelUpdates + # ArvadosModel checks this to decide whether it should update the + # 'modified_by_user_uuid' field. + def anonymous_updater + Thread.current[:anonymous_updater] || false + end + + def leave_modified_by_user_alone + anonymous_updater_was = anonymous_updater + begin + Thread.current[:anonymous_updater] = true + yield + ensure + Thread.current[:anonymous_updater] = anonymous_updater_was + end + end +end diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index 70ad11e0f4..cc257ccf48 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -200,6 +200,7 @@ class ContainerRequestTest < ActiveSupport::TestCase test "Request is finalized when its container is cancelled" do set_user_from_auth :active cr = create_minimal_req!(priority: 1, state: "Committed", container_count_max: 1) + assert_equal users(:active).uuid, cr.modified_by_user_uuid act_as_system_user do Container.find_by_uuid(cr.container_uuid). @@ -208,6 +209,7 @@ class ContainerRequestTest < ActiveSupport::TestCase cr.reload assert_equal "Final", cr.state + assert_equal users(:active).uuid, cr.modified_by_user_uuid end test "Request is finalized when its container is completed" do @@ -216,6 +218,7 @@ class ContainerRequestTest < ActiveSupport::TestCase 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) @@ -237,6 +240,7 @@ class ContainerRequestTest < ActiveSupport::TestCase cr.reload assert_equal "Final", cr.state + assert_equal users(:active).uuid, cr.modified_by_user_uuid ['output', 'log'].each do |out_type| pdh = Container.find_by_uuid(cr.container_uuid).send(out_type) assert_equal(1, Collection.where(portable_data_hash: pdh, @@ -261,6 +265,7 @@ class ContainerRequestTest < ActiveSupport::TestCase cr2 = create_minimal_req! cr2.update_attributes!(priority: 10, state: "Committed", requesting_container_uuid: c.uuid, command: ["echo", "foo2"], container_count_max: 1) cr2.reload + assert_equal users(:active).uuid, cr2.modified_by_user_uuid c2 = Container.find_by_uuid cr2.container_uuid assert_operator 0, :<, c2.priority @@ -275,6 +280,7 @@ class ContainerRequestTest < ActiveSupport::TestCase cr2.reload assert_equal 0, cr2.priority + assert_equal users(:active).uuid, cr2.modified_by_user_uuid c2.reload assert_equal 0, c2.priority -- 2.30.2