From d6db40a49670b9d5612a6012c0e33639c1fabd4c Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Thu, 20 Nov 2014 09:52:53 -0500 Subject: [PATCH] 4027: API Jobs that specify SDK version must also use Docker. Because compute nodes will be upgraded regularly, we don't want to guarantee that we'll have the necessary tools to run older SDK versions. The primary motivation for this feature is to install SDKs into Docker images, so we require that to be specified. This commit removes a few `job.valid?` assertions from the job unit tests. These are redundant when they come after `Job.create!` because that will raise an exception if the job isn't valid. --- doc/api/schema/Job.html.textile.liquid | 2 +- services/api/app/models/job.rb | 8 +++--- .../arvados/v1/job_reuse_controller_test.rb | 5 +++- services/api/test/unit/job_test.rb | 25 ++++++++++++++++--- 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/doc/api/schema/Job.html.textile.liquid b/doc/api/schema/Job.html.textile.liquid index e2363ac818..e5830415a4 100644 --- a/doc/api/schema/Job.html.textile.liquid +++ b/doc/api/schema/Job.html.textile.liquid @@ -53,7 +53,7 @@ h3. Runtime constraints table(table table-bordered table-condensed). |_. Key|_. Type|_. Description|_. Implemented| -|arvados_sdk_version|string|The Git version of the SDKs to use from the Arvados git repository. See "Specifying Git versions":#script_version for more detail about acceptable ways to specify a commit.|| +|arvados_sdk_version|string|The Git version of the SDKs to use from the Arvados git repository. See "Specifying Git versions":#script_version for more detail about acceptable ways to specify a commit. If you use this, you must also specify a @docker_image@ constraint (see below).|| |docker_image|string|The Docker image that this Job needs to run. If specified, Crunch will create a Docker container from this image, and run the Job's script inside that. The Keep mount and work directories will be available as volumes inside this container. The image must be uploaded to Arvados using @arv keep docker@. You may specify the image in any format that Docker accepts, such as @arvados/jobs@, @debian:latest@, or the Docker image id. Alternatively, you may specify the UUID or portable data hash of the image Collection, returned by @arv keep docker@.|✓| |min_nodes|integer||✓| |max_nodes|integer||| diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb index 6e01de9213..4a464f52a8 100644 --- a/services/api/app/models/job.rb +++ b/services/api/app/models/job.rb @@ -171,10 +171,12 @@ class Job < ArvadosModel :arvados_sdk_version) do |git_search| commits = Commit.find_commit_range(current_user, "arvados", nil, git_search, nil) - if commits.andand.any? - [true, commits.first] - else + if commits.nil? or commits.empty? [false, "#{git_search} does not resolve to a commit"] + elsif not runtime_constraints["docker_image"] + [false, "cannot be specified without a Docker image constraint"] + else + [true, commits.first] end end end 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 a3eebf3ed2..f9ecbf518e 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 @@ -671,7 +671,10 @@ class Arvados::V1::JobReuseControllerTest < ActionController::TestCase test "can't reuse job with older Arvados SDK version" do params = { script_version: "31ce37fe365b3dc204300a3e4c396ad333ed0556", - runtime_constraints: {"arvados_sdk_version" => "master"}, + runtime_constraints: { + "arvados_sdk_version" => "master", + "docker_image" => links(:docker_image_collection_tag).name, + }, } check_new_job_created_from(job: params) end diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb index 65ca9357dc..e885b694d2 100644 --- a/services/api/test/unit/job_test.rb +++ b/services/api/test/unit/job_test.rb @@ -189,7 +189,6 @@ class JobTest < ActiveSupport::TestCase ].each do |parameters| test "verify job status #{parameters}" do job = Job.create! job_attrs - assert job.valid?, job.errors.full_messages.to_s assert_equal 'Queued', job.state, "job.state" parameters.each do |parameter| @@ -280,11 +279,9 @@ class JobTest < ActiveSupport::TestCase test "verify job queue position" do job1 = Job.create! job_attrs - assert job1.valid?, job1.errors.full_messages.to_s assert_equal 'Queued', job1.state, "Incorrect job state for newly created job1" job2 = Job.create! job_attrs - assert job2.valid?, job2.errors.full_messages.to_s assert_equal 'Queued', job2.state, "Incorrect job state for newly created job2" assert_not_nil job1.queue_position, "Expected non-nil queue position for job1" @@ -296,7 +293,10 @@ class JobTest < ActiveSupport::TestCase SDK_TAGGED = "00634b2b8a492d6f121e3cf1d6587b821136a9a7" def sdk_constraint(version) - {runtime_constraints: {"arvados_sdk_version" => version}} + {runtime_constraints: { + "arvados_sdk_version" => version, + "docker_image" => links(:docker_image_collection_tag).name, + }} end def check_job_sdk_version(expected) @@ -347,6 +347,23 @@ class JobTest < ActiveSupport::TestCase assert_nil(job.arvados_sdk_version) end + test "job with SDK constraint, without Docker image is invalid" do + sdk_attrs = sdk_constraint("master") + sdk_attrs[:runtime_constraints].delete("docker_image") + job = Job.create(job_attrs(sdk_attrs)) + refute(job.valid?, "Job valid with SDK version, without Docker image") + sdk_errors = job.errors.messages[:arvados_sdk_version] || [] + refute_empty(sdk_errors.grep(/\bDocker\b/), + "no Job SDK errors mention that Docker is required") + end + + test "invalid to clear Docker image constraint when SDK constraint exists" do + job = Job.create!(job_attrs(sdk_constraint("master"))) + job.runtime_constraints.delete("docker_image") + refute(job.valid?, + "Job with SDK constraint valid after clearing Docker image") + end + test "can't create job with SDK version assigned directly" do check_creation_prohibited(arvados_sdk_version: SDK_MASTER) end -- 2.39.5