From f782a2505422ad9c853c4c416640c41f3b1e7e79 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Tue, 14 Feb 2017 15:16:22 -0500 Subject: [PATCH] 11017: When compute nodes use image format v2, prefer migrated docker images. --- services/api/app/models/collection.rb | 18 ++++++ services/api/app/models/container_request.rb | 2 +- services/api/app/models/job.rb | 18 ++++-- .../test/helpers/docker_migration_helper.rb | 13 +++++ .../api/test/unit/container_request_test.rb | 19 +++++++ services/api/test/unit/job_test.rb | 56 +++++++++++++++++++ 6 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 services/api/test/helpers/docker_migration_helper.rb diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb index f212e3358a..9b081dbd2e 100644 --- a/services/api/app/models/collection.rb +++ b/services/api/app/models/collection.rb @@ -2,6 +2,7 @@ require 'arvados/keep' require 'sweep_trashed_collections' class Collection < ArvadosModel + extend CurrentApiClient extend DbCurrentTime include HasUuid include KindAndEtag @@ -372,6 +373,23 @@ class Collection < ArvadosModel find_all_for_docker_image(search_term, search_tag, readers).first end + # If the given pdh is an old-format docker image, old-format images + # aren't supported by the compute nodes according to site config, + # and a migrated new-format image is available, return the migrated + # image's pdh. Otherwise, just return pdh. + def self.docker_migration_pdh(read_users, pdh) + if Rails.configuration.docker_image_formats.include?('v1') + return pdh + end + Collection.readable_by(*read_users). + joins('INNER JOIN links ON head_uuid=portable_data_hash'). + where('tail_uuid=? AND link_class=? AND links.owner_uuid=?', + pdh, 'docker_image_migration', system_user_uuid). + order('links.created_at desc'). + select('portable_data_hash'). + first.andand.portable_data_hash || pdh + end + def self.searchable_columns operator super - ["manifest_text"] end diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb index a264bbfe81..6122c34661 100644 --- a/services/api/app/models/container_request.rb +++ b/services/api/app/models/container_request.rb @@ -229,7 +229,7 @@ class ContainerRequest < ArvadosModel if !coll raise ArvadosModel::UnresolvableContainerError.new "docker image #{container_image.inspect} not found" end - return coll.portable_data_hash + return Collection.docker_migration_pdh([current_user], coll.portable_data_hash) end def set_container diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb index 2ae7139281..5b9f9c15f0 100644 --- a/services/api/app/models/job.rb +++ b/services/api/app/models/job.rb @@ -215,7 +215,8 @@ class Job < ArvadosModel else image_locator = nil end - filters << ["docker_image_locator", "=", image_locator] + filters << ["docker_image_locator", "=", + Collection.docker_migration_pdh(read_users, image_locator)] if sdk_version = attrs[:runtime_constraints].andand["arvados_sdk_version"] filters += default_git_filters("arvados_sdk_version", "arvados", sdk_version) end @@ -390,10 +391,11 @@ class Job < ArvadosModel end def find_docker_image_locator - runtime_constraints['docker_image'] = - Rails.configuration.default_docker_image_for_jobs if ((runtime_constraints.is_a? Hash) and - (runtime_constraints['docker_image']).nil? and - Rails.configuration.default_docker_image_for_jobs) + if runtime_constraints.is_a? Hash + runtime_constraints['docker_image'] ||= + Rails.configuration.default_docker_image_for_jobs + end + resolve_runtime_constraint("docker_image", :docker_image_locator) do |image_search| image_tag = runtime_constraints['docker_image_tag'] @@ -403,6 +405,12 @@ class Job < ArvadosModel [false, "not found for #{image_search}"] end end + Rails.logger.info("docker_image_locator is #{docker_image_locator}") + if docker_image_locator && docker_image_locator_changed? + self.docker_image_locator = + Collection.docker_migration_pdh([current_user], docker_image_locator) + end + Rails.logger.info("docker_image_locator is #{docker_image_locator}") end def permission_to_update diff --git a/services/api/test/helpers/docker_migration_helper.rb b/services/api/test/helpers/docker_migration_helper.rb new file mode 100644 index 0000000000..b37f725741 --- /dev/null +++ b/services/api/test/helpers/docker_migration_helper.rb @@ -0,0 +1,13 @@ +module DockerMigrationHelper + include CurrentApiClient + + def add_docker19_migration_link + act_as_system_user do + assert(Link.create!(owner_uuid: system_user_uuid, + link_class: 'docker_image_migration', + name: 'migrate_1.9_1.10', + tail_uuid: collections(:docker_image).portable_data_hash, + head_uuid: collections(:docker_image_1_12).portable_data_hash)) + end + end +end diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index da4f0bb7a8..328273bfc2 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -1,6 +1,9 @@ require 'test_helper' +require 'helpers/docker_migration_helper' class ContainerRequestTest < ActiveSupport::TestCase + include DockerMigrationHelper + def create_minimal_req! attrs={} defaults = { command: ["echo", "foo"], @@ -413,6 +416,22 @@ class ContainerRequestTest < ActiveSupport::TestCase end end + test "migrated docker image" do + Rails.configuration.docker_image_formats = ['v2'] + add_docker19_migration_link + + set_user_from_auth :active + cr = create_minimal_req!(command: ["true", "1"], + container_image: collections(:docker_image).portable_data_hash) + assert_equal(cr.send(:container_image_for_container), + collections(:docker_image_1_12).portable_data_hash) + + cr = create_minimal_req!(command: ["true", "2"], + container_image: links(:docker_image_collection_tag).name) + assert_equal(cr.send(:container_image_for_container), + collections(:docker_image_1_12).portable_data_hash) + end + test "requestor can retrieve container owned by dispatch" do assert_not_empty Container.readable_by(users(:admin)).where(uuid: containers(:running).uuid) assert_not_empty Container.readable_by(users(:active)).where(uuid: containers(:running).uuid) diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb index 761953e8eb..7a5699d9d9 100644 --- a/services/api/test/unit/job_test.rb +++ b/services/api/test/unit/job_test.rb @@ -1,7 +1,9 @@ require 'test_helper' require 'helpers/git_test_helper' +require 'helpers/docker_migration_helper' class JobTest < ActiveSupport::TestCase + include DockerMigrationHelper include GitTestHelper BAD_COLLECTION = "#{'f' * 32}+0" @@ -411,6 +413,60 @@ class JobTest < ActiveSupport::TestCase "Job with SDK constraint valid after clearing Docker image") end + test "use migrated docker image if requesting old-format image by tag" do + Rails.configuration.docker_image_formats = ['v2'] + add_docker19_migration_link + job = Job.create!( + job_attrs( + script: 'foo', + runtime_constraints: { + 'docker_image' => links(:docker_image_collection_tag).name})) + assert(job.valid?) + assert_equal(job.docker_image_locator, collections(:docker_image_1_12).portable_data_hash) + end + + test "use migrated docker image if requesting old-format image by pdh" do + Rails.configuration.docker_image_formats = ['v2'] + add_docker19_migration_link + job = Job.create!( + job_attrs( + script: 'foo', + runtime_constraints: { + 'docker_image' => collections(:docker_image).portable_data_hash})) + assert(job.valid?) + assert_equal(job.docker_image_locator, collections(:docker_image_1_12).portable_data_hash) + end + + [[:docker_image, :docker_image, :docker_image_1_12], + [:docker_image_1_12, :docker_image, :docker_image_1_12], + [:docker_image, :docker_image_1_12, :docker_image_1_12], + [:docker_image_1_12, :docker_image_1_12, :docker_image_1_12], + ].each do |existing_image, request_image, expect_image| + test "if a #{existing_image} job exists, #{request_image} yields #{expect_image} after migration" do + Rails.configuration.docker_image_formats = ['v1'] + oldjob = Job.create!( + job_attrs( + script: 'foobar1', + runtime_constraints: { + 'docker_image' => collections(existing_image).portable_data_hash})) + oldjob.reload + assert_equal(oldjob.docker_image_locator, + collections(existing_image).portable_data_hash) + + Rails.configuration.docker_image_formats = ['v2'] + add_docker19_migration_link + + newjob = Job.create!( + job_attrs( + script: 'foobar1', + runtime_constraints: { + 'docker_image' => collections(request_image).portable_data_hash})) + newjob.reload + assert_equal(newjob.docker_image_locator, + collections(expect_image).portable_data_hash) + end + end + test "can't create job with SDK version assigned directly" do check_creation_prohibited(arvados_sdk_version: SDK_MASTER) end -- 2.30.2