Merge branch '12902-queued-to-cancelled'
authorTom Clegg <tclegg@veritasgenetics.com>
Thu, 1 Feb 2018 16:51:40 +0000 (11:51 -0500)
committerTom Clegg <tclegg@veritasgenetics.com>
Thu, 1 Feb 2018 16:51:40 +0000 (11:51 -0500)
refs #12902

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

apps/workbench/app/controllers/container_requests_controller.rb
apps/workbench/app/models/container_work_unit.rb
apps/workbench/app/views/container_requests/_show_recent_rows.html.erb
apps/workbench/app/views/projects/_show_dashboard.html.erb
apps/workbench/test/controllers/container_requests_controller_test.rb
apps/workbench/test/unit/work_unit_test.rb
services/api/app/models/container.rb
services/api/app/models/container_request.rb
services/api/test/fixtures/container_requests.yml
services/api/test/unit/container_test.rb

index f61596ecc766ce7b305f4db3d5e2a4e95c466749..783cafa117d9c23b3641f5f9883b90ea066be384 100644 (file)
@@ -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]
index a5b26f0d6d3d5534fd19e1df76d45ac97e7aadd8..dbc81c52a376940231094dbf4415e5625016814f 100644 (file)
@@ -58,7 +58,10 @@ class ContainerWorkUnit < ProxyWorkUnit
   end
 
   def can_cancel?
-    @proxied.is_a?(ContainerRequest) && @proxied.state == "Committed" && @proxied.priority > 0 && @proxied.editable?
+    @proxied.is_a?(ContainerRequest) &&
+      @proxied.state == "Committed" &&
+      (@proxied.priority > 0 || get(:state, @container) != 'Running') &&
+      @proxied.editable?
   end
 
   def container_uuid
@@ -95,14 +98,29 @@ class ContainerWorkUnit < ProxyWorkUnit
   end
 
   def state_label
-    ec = exit_code
-    return "Failed" if (ec && ec != 0)
-
-    state = get_combined(:state)
-
-    return "Queued" if state == "Locked"
-    return "Cancelled" if ((priority == 0) and (state == "Queued"))
-    state
+    if get(:state) == 'Final' && get(:state, @container) != 'Complete'
+      # Request was finalized before its container started (or the
+      # container was cancelled)
+      return 'Cancelled'
+    end
+    state = get(:state, @container) || get(:state, @proxied)
+    case state
+    when 'Locked', 'Queued'
+      if priority == 0
+        'On hold'
+      else
+        'Queued'
+      end
+    when 'Complete'
+      if exit_code == 0
+        state
+      else
+        'Failed'
+      end
+    else
+      # Cancelled, Running, or Uncommitted (no container assigned)
+      state
+    end
   end
 
   def exit_code
index 32de59cde831af32bb7d972afdf5169c11dadd0a..0212162fccb15e26d460b80d9969672b15959e70 100644 (file)
@@ -24,7 +24,7 @@ SPDX-License-Identifier: AGPL-3.0 %>
     <td>
       <span class="label label-<%= wu.state_bootstrap_class %>"><%= wu.state_label %></span>
     </td><td>
-      <%= link_to_if_arvados_object obj, friendly_name: true, link_text: if !obj.name.empty? then obj.name else obj.uuid end %>
+      <%= link_to_if_arvados_object obj, friendly_name: true, link_text: if obj.name && !obj.name.empty? then obj.name else obj.uuid end %>
     </td><td>
       <%= obj.description || '' %>
     </td><td>
index 713582654fee93b3c9bd11432701e002b6fc5d0c..e51cf5314d275137260f214f2e0bbbc6852bd373 100644 (file)
@@ -125,12 +125,15 @@ SPDX-License-Identifier: AGPL-3.0 %>
               </div>
 
               <div class="clearfix">
-                Started at <%= render_localized_date(wu.started_at || wu.created_at, "noseconds") %>.
-                <% wu_time = Time.now - (wu.started_at || wu.created_at) %>
-                Active for <%= render_runtime(wu_time, false) %>.
-
-                <div class="pull-right">
-                </div>
+                <% if wu.started_at %>
+                  Started at <%= render_localized_date(wu.started_at, "noseconds") %>
+                  Active for <%= render_runtime(Time.now - wu.started_at, false) %>.
+                <% else %>
+                  Created at <%= render_localized_date(wu.created_at, "noseconds") %>.
+                  <% if wu.state_label == 'Queued' %>
+                    Queued for <%= render_runtime(Time.now - wu.created_at, false) %>.
+                  <% end %>
+                <% end %>
               </div>
             </div>
             <% end %>
index 206352a2afa8d77abca7404abf5ad3143fbacf51..261169cd1f954c352aaba6e58e577c9d0a955b4d 100644 (file)
@@ -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
 
   [
index 5cf9499aad5201cb9e61074fc3294c49bc981785..9f660cd34434cb9cacf8a19774b4ce9522e57129 100644 (file)
@@ -21,7 +21,7 @@ class WorkUnitTest < ActiveSupport::TestCase
     [ContainerRequest, 'cr_for_requester', 'cwu', 1, "Complete", true, 1.0],
     [ContainerRequest, 'queued', 'cwu', 0, "Queued", nil, 0.0],   # priority 1
     [ContainerRequest, 'canceled_with_queued_container', 'cwu', 0, "Cancelled", false, 0.0],
-    [ContainerRequest, 'canceled_with_locked_container', 'cwu', 0, "Queued", nil, 0.0],
+    [ContainerRequest, 'canceled_with_locked_container', 'cwu', 0, "Cancelled", false, 0.0],
     [ContainerRequest, 'canceled_with_running_container', 'cwu', 1, "Running", nil, 0.0],
   ].each do |type, fixture, label, num_children, state, success, progress|
     test "children of #{fixture}" do
index edcb8501a4621aa71be9eef5d68c719aac49a267..b013776b98d3690db6cd5921bc8a3c11e6ce4ad4 100644 (file)
@@ -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
index 3596bf3d67551352c7ba404b3605c7dabe15956b..bcca40700bd9efbaf57c74332a94f3325763299a 100644 (file)
@@ -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
@@ -237,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
index 85e40ffe34d10f26198c3bab6523e234e7360eec..29ce4f5aea5ffb29489f38fc49e9235bfa979b00 100644 (file)
@@ -219,7 +219,7 @@ canceled_with_queued_container:
   uuid: zzzzz-xvhdp-canceledqueuedc
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
   name: canceled with queued container
-  state: Committed
+  state: Final
   priority: 0
   created_at: 2016-01-11 11:11:11.111111111 Z
   updated_at: 2016-01-11 11:11:11.111111111 Z
@@ -238,7 +238,7 @@ canceled_with_locked_container:
   uuid: zzzzz-xvhdp-canceledlocekdc
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
   name: canceled with locked container
-  state: Committed
+  state: Final
   priority: 0
   created_at: 2016-01-11 11:11:11.111111111 Z
   updated_at: 2016-01-11 11:11:11.111111111 Z
index ed86befbace2133bd06213b809279478418cf5a2..0e13ee950c3aa57a4eab704d1003b378e2b4f4d2 100644 (file)
@@ -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