9848: Copy the output and log collections (if any) when finalizing a container request.

When reusing a container, ensure log and output are readable.

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index c1c3eae94b7769f35c90e874b0805e537847da71..a60ea427b78649ae15977b69bea99060e9c465c9 100644 (file)
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -5,6 +5,7 @@ class Container < ArvadosModel
   include KindAndEtag
   include CommonApiTemplate
   include WhitelistUpdate
   include KindAndEtag
   include CommonApiTemplate
   include WhitelistUpdate

+  extend CurrentApiClient

 
   serialize :environment, Hash
   serialize :mounts, Hash
 
   serialize :environment, Hash
   serialize :mounts, Hash


@@ -87,10 +88,34 @@ class Container < ArvadosModel
       where('mounts = ?', self.deep_sort_hash(attrs[:mounts]).to_yaml).
       where('runtime_constraints = ?', self.deep_sort_hash(attrs[:runtime_constraints]).to_yaml)
 
       where('mounts = ?', self.deep_sort_hash(attrs[:mounts]).to_yaml).
       where('runtime_constraints = ?', self.deep_sort_hash(attrs[:runtime_constraints]).to_yaml)
 

-    # Check for Completed candidates that only had consistent outputs.
+    # Check for Completed candidates that had consistent outputs.

     completed = candidates.where(state: Complete).where(exit_code: 0)
     completed = candidates.where(state: Complete).where(exit_code: 0)

-    if completed.select("output").group('output').limit(2).length == 1
-      return completed.order('finished_at asc').limit(1).first
+    outputs = completed.select('output').group('output').limit(2)
+    if outputs.count.count != 1
+      Rails.logger.debug("Found #{outputs.count.length} different outputs")
+    elsif Collection.
+        readable_by(current_user).
+        where(portable_data_hash: outputs.first.output).
+        count < 1
+      Rails.logger.info("Found reusable container(s) " +
+                        "but output #{outputs.first} is not readable " +
+                        "by user #{current_user.uuid}")
+    else
+      # Return the oldest eligible container whose log is still
+      # present and readable by current_user.
+      readable_pdh = Collection.
+        readable_by(current_user).
+        select('portable_data_hash')
+      completed = completed.
+        where("log in (#{readable_pdh.to_sql})").
+        order('finished_at asc').
+        limit(1)
+      if completed.first
+        return completed.first
+      else
+        Rails.logger.info("Found reusable container(s) but none with a log " +
+                          "readable by user #{current_user.uuid}")
+      end

     end
 
     # Check for Running candidates and return the most likely to finish sooner.
     end
 
     # Check for Running candidates and return the most likely to finish sooner.


@@ -284,13 +309,13 @@ class Container < ArvadosModel
       act_as_system_user do
         # Notify container requests associated with this container
         ContainerRequest.where(container_uuid: uuid,
       act_as_system_user do
         # Notify container requests associated with this container
         ContainerRequest.where(container_uuid: uuid,

-                               :state => ContainerRequest::Committed).each do |cr|
+                               state: ContainerRequest::Committed).each do |cr|

           cr.container_completed!
         end
 
         # Try to cancel any outstanding container requests made by this container.
         ContainerRequest.where(requesting_container_uuid: uuid,
           cr.container_completed!
         end
 
         # Try to cancel any outstanding container requests made by this container.
         ContainerRequest.where(requesting_container_uuid: uuid,

-                               :state => ContainerRequest::Committed).each do |cr|
+                               state: ContainerRequest::Committed).each do |cr|

           cr.priority = 0
           cr.save
         end
           cr.priority = 0
           cr.save
         end

diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 872b3c56cf5a66380218a6c5a265fe3828d647bc..1fe8365121054e75bc326df7ca83eff4e52bcba7 100644 (file)
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -64,10 +64,24 @@ class ContainerRequest < ArvadosModel
     %w(modified_by_client_uuid container_uuid requesting_container_uuid)
   end
 
     %w(modified_by_client_uuid container_uuid requesting_container_uuid)
   end
 

+  # Finalize the container request after the container has
+  # finished/cancelled.

   def container_completed!
   def container_completed!

-    # may implement retry logic here in the future.
-    self.state = ContainerRequest::Final
-    self.save!
+    update_attributes!(state: ContainerRequest::Final)
+    c = Container.find_by_uuid(container_uuid)
+    ['output', 'log'].each do |out_type|
+      pdh = c.send(out_type)
+      next if pdh.nil?
+      manifest = Collection.where(portable_data_hash: pdh).first.manifest_text
+      Collection.create!(owner_uuid: owner_uuid,
+                         manifest_text: manifest,
+                         portable_data_hash: pdh,
+                         name: "Container #{out_type} for request #{uuid}",
+                         properties: {
+                           'type' => out_type,
+                           'container_request' => uuid,
+                         })
+    end

   end
 
   protected
   end
 
   protected

diff --git a/services/api/test/fixtures/containers.yml b/services/api/test/fixtures/containers.yml
index 87098dcf53e96e693635bfd8ce355dbb2f012b36..29266d3ab8f50f87586086c8702c676c2d7a7cb1 100644 (file)
--- a/services/api/test/fixtures/containers.yml
+++ b/services/api/test/fixtures/containers.yml
@@ -74,7 +74,7 @@ completed:
   container_image: test
   cwd: test
   log: ea10d51bcf88862dbcc36eb292017dfd+45
   container_image: test
   cwd: test
   log: ea10d51bcf88862dbcc36eb292017dfd+45

-  output: zzzzz-4zz18-znfnqtbbv4spc3w
+  output: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45

   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:
   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:


@@ -93,7 +93,7 @@ completed_older:
   finished_at: 2016-01-14 11:12:13.111111111 Z
   container_image: test
   cwd: test
   finished_at: 2016-01-14 11:12:13.111111111 Z
   container_image: test
   cwd: test

-  output: test
+  output: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45

   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:
   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:


@@ -110,7 +110,7 @@ requester:
   updated_at: 2016-01-11 11:11:11.111111111 Z
   container_image: test
   cwd: test
   updated_at: 2016-01-11 11:11:11.111111111 Z
   container_image: test
   cwd: test

-  output: test
+  output: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45

   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:
   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:


@@ -127,7 +127,7 @@ requester_container:
   updated_at: 2016-01-11 11:11:11.111111111 Z
   container_image: test
   cwd: test
   updated_at: 2016-01-11 11:11:11.111111111 Z
   container_image: test
   cwd: test

-  output: test
+  output: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45

   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:
   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:


@@ -145,7 +145,7 @@ failed_container:
   updated_at: 2016-01-11 11:11:11.111111111 Z
   container_image: test
   cwd: test
   updated_at: 2016-01-11 11:11:11.111111111 Z
   container_image: test
   cwd: test

-  output: test
+  output: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45

   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:
   output_path: test
   command: ["echo", "hello"]
   runtime_constraints:

diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index e44084eaf3e17f7a982fad82eeac0964b0b5361b..372d94a3bbe2ba9355cc9a6b4f8cec8a5548a051 100644 (file)
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -215,7 +215,10 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
   test "Request is finalized when its container is completed" do
     set_user_from_auth :active
 
   test "Request is finalized when its container is completed" do
     set_user_from_auth :active

-    cr = create_minimal_req!(priority: 1, state: "Committed")
+    project = groups(:private)
+    cr = create_minimal_req!(owner_uuid: project.uuid,
+                             priority: 1,
+                             state: "Committed")

 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)


@@ -228,11 +231,19 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal "Committed", cr.state
 
     act_as_system_user do
     assert_equal "Committed", cr.state
 
     act_as_system_user do

-      c.update_attributes!(state: Container::Complete)
+      c.update_attributes!(state: Container::Complete,
+                           output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
+                           log: 'fa7aeb5140e2848d39b416daeef4ffc5+45')

     end
 
     cr.reload
     assert_equal "Final", cr.state
     end
 
     cr.reload
     assert_equal "Final", cr.state

+    ['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,
+                                       owner_uuid: project.uuid).count,
+                   "Container #{out_type} should be copied to #{project.uuid}")
+    end

   end
 
   test "Container makes container request, then is cancelled" do
   end
 
   test "Container makes container request, then is cancelled" do

diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 85741bb339a1288e9d40fba3ee763ef06b4eade4..8894ed9d4c0e16dc32a8b5bbc95d0a1371d3f41f 100644 (file)
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -122,7 +122,7 @@ class ContainerTest < ActiveSupport::TestCase
       state: Container::Complete,
       exit_code: 0,
       log: 'ea10d51bcf88862dbcc36eb292017dfd+45',
       state: Container::Complete,
       exit_code: 0,
       log: 'ea10d51bcf88862dbcc36eb292017dfd+45',

-      output: 'zzzzz-4zz18-znfnqtbbv4spc3w'
+      output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45'

     }
 
     c_older, _ = minimal_new(common_attrs)
     }
 
     c_older, _ = minimal_new(common_attrs)


@@ -148,7 +148,7 @@ class ContainerTest < ActiveSupport::TestCase
     completed_attrs = {
       state: Container::Complete,
       exit_code: 0,
     completed_attrs = {
       state: Container::Complete,
       exit_code: 0,

-      log: 'test',
+      log: 'ea10d51bcf88862dbcc36eb292017dfd+45',

     }
 
     c_output1, _ = minimal_new(common_attrs)
     }
 
     c_output1, _ = minimal_new(common_attrs)


@@ -157,11 +157,11 @@ class ContainerTest < ActiveSupport::TestCase
     set_user_from_auth :dispatch1
     c_output1.update_attributes!({state: Container::Locked})
     c_output1.update_attributes!({state: Container::Running})
     set_user_from_auth :dispatch1
     c_output1.update_attributes!({state: Container::Locked})
     c_output1.update_attributes!({state: Container::Running})

-    c_output1.update_attributes!(completed_attrs.merge({output: 'output 1'}))
+    c_output1.update_attributes!(completed_attrs.merge({output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45'}))

 
     c_output2.update_attributes!({state: Container::Locked})
     c_output2.update_attributes!({state: Container::Running})
 
     c_output2.update_attributes!({state: Container::Locked})
     c_output2.update_attributes!({state: Container::Running})

-    c_output2.update_attributes!(completed_attrs.merge({output: 'output 2'}))
+    c_output2.update_attributes!(completed_attrs.merge({output: 'fa7aeb5140e2848d39b416daeef4ffc5+45'}))

 
     reused = Container.find_reusable(common_attrs)
     assert_nil reused
 
     reused = Container.find_reusable(common_attrs)
     assert_nil reused


@@ -239,8 +239,8 @@ class ContainerTest < ActiveSupport::TestCase
     c_failed.update_attributes!({state: Container::Running})
     c_failed.update_attributes!({state: Container::Complete,
                                  exit_code: 42,
     c_failed.update_attributes!({state: Container::Running})
     c_failed.update_attributes!({state: Container::Complete,
                                  exit_code: 42,

-                                 log: "test",
-                                 output: "test"})
+                                 log: 'ea10d51bcf88862dbcc36eb292017dfd+45',
+                                 output: 'ea10d51bcf88862dbcc36eb292017dfd+45'})

     c_running.update_attributes!({state: Container::Locked})
     c_running.update_attributes!({state: Container::Running,
                                   progress: 0.15})
     c_running.update_attributes!({state: Container::Locked})
     c_running.update_attributes!({state: Container::Running,
                                   progress: 0.15})


@@ -259,14 +259,14 @@ class ContainerTest < ActiveSupport::TestCase
     c_completed.update_attributes!({state: Container::Running})
     c_completed.update_attributes!({state: Container::Complete,
                                     exit_code: 0,
     c_completed.update_attributes!({state: Container::Running})
     c_completed.update_attributes!({state: Container::Complete,
                                     exit_code: 0,

-                                    log: "ea10d51bcf88862dbcc36eb292017dfd+45",
-                                    output: "zzzzz-4zz18-znfnqtbbv4spc3w"})
+                                    log: 'ea10d51bcf88862dbcc36eb292017dfd+45',
+                                    output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45'})

     c_running.update_attributes!({state: Container::Locked})
     c_running.update_attributes!({state: Container::Running,
                                   progress: 0.15})
     reused = Container.find_reusable(common_attrs)
     assert_not_nil reused
     c_running.update_attributes!({state: Container::Locked})
     c_running.update_attributes!({state: Container::Running,
                                   progress: 0.15})
     reused = Container.find_reusable(common_attrs)
     assert_not_nil reused

-    assert_equal reused.uuid, c_completed.uuid
+    assert_equal c_completed.uuid, reused.uuid

   end
 
   test "find_reusable method should select running over locked container" do
   end
 
   test "find_reusable method should select running over locked container" do