[ARVADOS] created: 1.1.2-115-g937a84b
Git user
git at public.curoverse.com
Tue Jan 30 22:07:03 EST 2018
at 937a84b103340b0abcb351b800fa53cdf209351f (commit)
commit 937a84b103340b0abcb351b800fa53cdf209351f
Author: Tom Clegg <tclegg at veritasgenetics.com>
Date: Tue Jan 30 17:56:13 2018 -0500
12902: Update state=Final when cancelling a container request.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg at veritasgenetics.com>
diff --git a/apps/workbench/app/controllers/container_requests_controller.rb b/apps/workbench/app/controllers/container_requests_controller.rb
index f61596e..783cafa 100644
--- a/apps/workbench/app/controllers/container_requests_controller.rb
+++ b/apps/workbench/app/controllers/container_requests_controller.rb
@@ -77,6 +77,17 @@ class ContainerRequestsController < ApplicationController
end
def cancel
+ if @object.container_uuid
+ c = Container.select(['state']).where(uuid: @object.container_uuid).first
+ if c && c.state != 'Running'
+ # If the container hasn't started yet, setting priority=0
+ # leaves our request in "Committed" state and doesn't cancel
+ # the container (even if no other requests are giving it
+ # priority). To avoid showing this container request as "on
+ # hold" after hitting the Cancel button, set state=Final too.
+ @object.state = 'Final'
+ end
+ end
@object.update_attributes! priority: 0
if params[:return_to]
redirect_to params[:return_to]
diff --git a/apps/workbench/test/controllers/container_requests_controller_test.rb b/apps/workbench/test/controllers/container_requests_controller_test.rb
index 206352a..261169c 100644
--- a/apps/workbench/test/controllers/container_requests_controller_test.rb
+++ b/apps/workbench/test/controllers/container_requests_controller_test.rb
@@ -42,7 +42,21 @@ class ContainerRequestsControllerTest < ActionController::TestCase
get :show, {id: uuid}, session_for(:active)
assert_response :success
- assert_includes @response.body, "action=\"/container_requests/#{uuid}/copy\""
+ assert_includes @response.body, "action=\"/container_requests/#{uuid}/copy\""
+ end
+
+ test "cancel request for queued container" do
+ cr_fixture = api_fixture('container_requests')['queued']
+ post :cancel, {id: cr_fixture['uuid']}, session_for(:active)
+ assert_response 302
+
+ use_token 'active'
+ cr = ContainerRequest.find(cr_fixture['uuid'])
+ assert_equal 'Final', cr.state
+ assert_equal 0, cr.priority
+ c = Container.find(cr_fixture['container_uuid'])
+ assert_equal 'Queued', c.state
+ assert_equal 0, c.priority
end
[
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 0b18914..bcca407 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -239,12 +239,13 @@ class ContainerRequest < ArvadosModel
end
when Final
- 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_was == Committed
- permitted.push :output_uuid, :log_uuid
+ # "Cancel" means setting priority=0, state=Committed
+ permitted.push :priority
+
+ if current_user.andand.is_admin
+ permitted.push :output_uuid, :log_uuid
+ end
end
end
commit b1708b5b5a6feb8513ec8153d8ef21774d050f51
Author: Tom Clegg <tclegg at veritasgenetics.com>
Date: Tue Jan 23 10:40:21 2018 -0500
12902: Leave child CRs "cancelled" (not "held") when parent exits.
Changing a container's priority to 0 effects "cancel" only after it
has started. Before that, it just gets held in the queue with priority
0. The child CRs of a terminated container are presumed abandoned, so
it makes more sense to finalize them.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg at veritasgenetics.com>
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index edcb850..b013776 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -90,7 +90,7 @@ class Container < ArvadosModel
self.priority = ContainerRequest.
where(container_uuid: uuid,
state: ContainerRequest::Committed).
- maximum('priority')
+ maximum('priority') || 0
self.save!
end
end
@@ -515,7 +515,7 @@ class Container < ArvadosModel
cr.with_lock do
# Use row locking because this increments container_count
cr.container_uuid = c.uuid
- cr.save
+ cr.save!
end
end
end
@@ -526,11 +526,21 @@ class Container < ArvadosModel
cr.finalize!
end
- # Try to cancel any outstanding container requests made by this container.
- ContainerRequest.where(requesting_container_uuid: uuid,
- state: ContainerRequest::Committed).each do |cr|
- cr.priority = 0
- cr.save
+ # Cancel outstanding container requests made by this container.
+ ContainerRequest.
+ 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)
+ end
end
end
end
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 3596bf3..0b18914 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -10,6 +10,8 @@ class ContainerRequest < ArvadosModel
include CommonApiTemplate
include WhitelistUpdate
+ belongs_to :container, foreign_key: :container_uuid, primary_key: :uuid
+
serialize :properties, Hash
serialize :environment, Hash
serialize :mounts, Hash
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index ed86bef..0e13ee9 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -473,10 +473,12 @@ class ContainerTest < ActiveSupport::TestCase
end
test "Container queued cancel" do
- c, _ = minimal_new
+ c, cr = minimal_new({container_count_max: 1})
set_user_from_auth :dispatch1
assert c.update_attributes(state: Container::Cancelled), show_errors(c)
check_no_change_from_cancelled c
+ cr.reload
+ assert_equal ContainerRequest::Final, cr.state
end
test "Container queued count" do
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list