From: Brett Smith <brett@curoverse.com>
Date: Mon, 16 Jun 2014 22:01:40 +0000 (-0400)
Subject: 2879: API server can find_or_create Jobs based on Docker image.
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/1f2a831876fe1fd193c27639078d2de43cfd691b

2879: API server can find_or_create Jobs based on Docker image.
---

diff --git a/services/api/app/controllers/arvados/v1/jobs_controller.rb b/services/api/app/controllers/arvados/v1/jobs_controller.rb
index 6fddba7a56..a93167a02e 100644
--- a/services/api/app/controllers/arvados/v1/jobs_controller.rb
+++ b/services/api/app/controllers/arvados/v1/jobs_controller.rb
@@ -28,27 +28,26 @@ class Arvados::V1::JobsController < ApplicationController
     end
 
     if params[:find_or_create]
-      # Convert old special-purpose creation parameters to the new
-      # filters-based method.
+      # Translate older creation parameters and special range operators
+      # into standard filters.
       minimum_script_version = params[:minimum_script_version]
       exclude_script_versions = params.fetch(:exclude_script_versions, [])
-      @filters.select do |(col_name, operand, operator)|
-        case col_name
-        when "script_version"
-          case operand
-          when "in range"
-            minimum_script_version = operator
-            false
-          when "not in", "not in range"
-            begin
-              exclude_script_versions += operator
-            rescue TypeError
-              exclude_script_versions << operator
-            end
-            false
-          else
-            true
+      @filters.select do |filter|
+        case filter[0..1]
+        when ["script_version", "in range"]
+          minimum_script_version = filter.last
+          false
+        when ["script_version", "not in"], ["script_version", "not in range"]
+          begin
+            exclude_script_versions += filter.last
+          rescue TypeError
+            exclude_script_versions << filter.last
           end
+          false
+        when ["docker_image_locator", "in range"], ["docker_image_locator", "not in range"]
+          filter[1].sub!(/ range$/, '')
+          filter[2] = Collection.uuids_for_docker_image(filter[2])
+          true
         else
           true
         end
@@ -64,7 +63,19 @@ class Arvados::V1::JobsController < ApplicationController
       if @filters.select { |f| f.first == "script" }.empty?
         @filters.append(["script", "=", resource_attrs[:script]])
       end
+      if @filters.select { |f| f.first == "docker_image_locator" }.empty?
+        if image_search = resource_attrs[:runtime_constraints].andand["docker_image"]
+          image_tag = resource_attrs[:runtime_constraints]["docker_image_tag"]
+          image_locator =
+            Collection.uuids_for_docker_image(image_search, image_tag).last
+          return super if image_locator.nil?  # We won't find anything to reuse.
+          @filters.append(["docker_image_locator", "=", image_locator])
+        else
+          @filters.append(["docker_image_locator", "=", nil])
+        end
+      end
 
+      # Search for a reusable Job, and return it if found.
       @objects = Job.readable_by(current_user)
       apply_filters
       @object = nil
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 64a6bb0530..2d573e536f 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -148,7 +148,7 @@ class Collection < ArvadosModel
     [hash_part, size_part].compact.join '+'
   end
 
-  def self.for_latest_docker_image(search_term, search_tag=nil, readers=nil)
+  def self.uuids_for_docker_image(search_term, search_tag=nil, readers=nil)
     readers ||= [Thread.current[:user]]
     base_search = Link.
       readable_by(*readers).
@@ -161,7 +161,7 @@ class Collection < ArvadosModel
     coll_matches = base_search.
       where(link_class: "docker_image_hash", collections: {uuid: search_term})
     if match = coll_matches.first
-      return find_by_uuid(match.head_uuid)
+      return [match.head_uuid]
     end
 
     # Find Collections with matching Docker image repository+tag pairs.
@@ -176,20 +176,27 @@ class Collection < ArvadosModel
               "docker_image_hash", "#{search_term}%")
     end
 
-    # Select the image that was created most recently.  Note that the
-    # SQL search order and fallback timestamp values are chosen so
-    # that if image timestamps are missing, we use the image with the
-    # newest link.
-    latest_image_link = nil
-    latest_image_timestamp = "1900-01-01T00:00:00Z"
+    # Generate an order key for each result.  We want to order the results
+    # so that anything with an image timestamp is considered more recent than
+    # anything without; then we use the link's created_at as a tiebreaker.
+    results = {}
     matches.find_each do |link|
-      link_timestamp = link.properties.fetch("image_timestamp",
-                                             "1900-01-01T00:00:01Z")
-      if link_timestamp > latest_image_timestamp
-        latest_image_link = link
-        latest_image_timestamp = link_timestamp
+      sort_key = []
+      if timestamp = link.properties["image_timestamp"]
+        sort_key.push("Z", timestamp.to_s)
       end
+      sort_key.push("Y", link.created_at.to_s(:db))
+      results[link] = sort_key.join("")
+    end
+    results.keys.sort_by { |link| results[link] }.map { |link| link.head_uuid }
+  end
+
+  def self.for_latest_docker_image(search_term, search_tag=nil, readers=nil)
+    image_uuid = uuids_for_docker_image(search_term, search_tag, readers).last
+    if image_uuid.nil?
+      nil
+    else
+      find_by_uuid(image_uuid)
     end
-    latest_image_link.nil? ? nil : find_by_uuid(latest_image_link.head_uuid)
   end
 end
diff --git a/services/api/test/fixtures/jobs.yml b/services/api/test/fixtures/jobs.yml
index 3ad77460f3..c9b258851a 100644
--- a/services/api/test/fixtures/jobs.yml
+++ b/services/api/test/fixtures/jobs.yml
@@ -125,6 +125,18 @@ previous_job_run:
   success: true
   output: ea10d51bcf88862dbcc36eb292017dfd+45
 
+previous_docker_job_run:
+  uuid: zzzzz-8i9sb-k6emstgk4kw4yhi
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  script: hash
+  script_version: 4fe459abe02d9b365932b8f5dc419439ab4e2577
+  script_parameters:
+    input: fa7aeb5140e2848d39b416daeef4ffc5+45
+    an_integer: "1"
+  success: true
+  output: ea10d51bcf88862dbcc36eb292017dfd+45
+  docker_image_locator: fa3c1a9cb6783f85f2ecda037e07b8c3+167
+
 previous_job_run_no_output:
   uuid: zzzzz-8i9sb-cjs4pklxxjykppp
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
diff --git a/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb b/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb
index 363c46832b..e534adc623 100644
--- a/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/job_reuse_controller_test.rb
@@ -342,4 +342,73 @@ class Arvados::V1::JobReuseControllerTest < ActionController::TestCase
     assert_not_equal 'zzzzz-8i9sb-cjs4pklxxjykqqq', new_job['uuid']
     assert_equal '4fe459abe02d9b365932b8f5dc419439ab4e2577', new_job['script_version']
   end
+
+  test "can reuse a Job with a Docker image" do
+    post(:create, {
+           job: {
+             script: "hash",
+             script_version: "4fe459abe02d9b365932b8f5dc419439ab4e2577",
+             repository: "foo",
+             script_parameters: {
+               input: 'fa7aeb5140e2848d39b416daeef4ffc5+45',
+               an_integer: '1'
+             },
+             runtime_constraints: {
+               docker_image: 'arvados/apitestfixture',
+             }
+           },
+           find_or_create: true,
+         })
+    assert_response :success
+    new_job = assigns(:object)
+    assert_not_nil new_job
+    target_job = jobs(:previous_docker_job_run)
+    [:uuid, :script_version, :docker_image_locator].each do |attr|
+      assert_equal(target_job.send(attr), new_job.send(attr))
+    end
+  end
+
+  test "can reuse a Job with a Docker image hash filter" do
+    post(:create, {
+           job: {
+             script: "hash",
+             script_version: "4fe459abe02d9b365932b8f5dc419439ab4e2577",
+             repository: "foo",
+             script_parameters: {
+               input: 'fa7aeb5140e2848d39b416daeef4ffc5+45',
+               an_integer: '1'
+             },
+           },
+           filters: [["docker_image_locator", "in range",
+                      links(:docker_image_collection_hash).name]],
+           find_or_create: true,
+         })
+    assert_response :success
+    new_job = assigns(:object)
+    assert_not_nil new_job
+    target_job = jobs(:previous_docker_job_run)
+    [:uuid, :script_version, :docker_image_locator].each do |attr|
+      assert_equal(target_job.send(attr), new_job.send(attr))
+    end
+  end
+
+  test "new job with unknown Docker image filter" do
+    post(:create, {
+           job: {
+             script: "hash",
+             script_version: "4fe459abe02d9b365932b8f5dc419439ab4e2577",
+             repository: "foo",
+             script_parameters: {
+               input: 'fa7aeb5140e2848d39b416daeef4ffc5+45',
+               an_integer: '1'
+             },
+           },
+           filters: [["docker_image_locator", "in range", "_nonexistentname_"]],
+           find_or_create: true,
+         })
+    assert_response :success
+    new_job = assigns(:object)
+    assert_not_nil new_job
+    assert_not_equal(jobs(:previous_docker_job_run).uuid, new_job.uuid)
+  end
 end