[ARVADOS] updated: f85c6cf14fada8b2ad4b6b7e6e7e9cf9852ce83d
git at public.curoverse.com
git at public.curoverse.com
Mon Jul 14 17:12:28 EDT 2014
Summary of changes:
.../app/controllers/arvados/v1/jobs_controller.rb | 80 +++++++++++++---------
services/api/app/models/commit.rb | 8 +--
.../arvados/v1/job_reuse_controller_test.rb | 71 +++++++++++++++++++
3 files changed, 124 insertions(+), 35 deletions(-)
via f85c6cf14fada8b2ad4b6b7e6e7e9cf9852ce83d (commit)
via 29542968833ad233b21fc7fe2f86b1ba88334da2 (commit)
via 5c5cb0a226a165c0028bb2c55cba8f4dcc359be1 (commit)
from ac97e7087df8b48ee289c696b47973618a40ce73 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit f85c6cf14fada8b2ad4b6b7e6e7e9cf9852ce83d
Merge: ac97e70 2954296
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jul 14 17:11:16 2014 -0400
Merge branch '3195-bugfix-job-min-script-ver'
Closes #3195. There may be other issues here but we need more
information to debug them.
commit 29542968833ad233b21fc7fe2f86b1ba88334da2
Author: Brett Smith <brett at curoverse.com>
Date: Tue Jul 8 13:01:44 2014 -0400
Improve API server's git rev-list calls.
This doesn't completely silence errors, but it makes them less verbose
and prevents git from trying to look up paths, which we never want.
diff --git a/services/api/app/models/commit.rb b/services/api/app/models/commit.rb
index 6239011..8735741 100644
--- a/services/api/app/models/commit.rb
+++ b/services/api/app/models/commit.rb
@@ -46,7 +46,7 @@ class Commit < ActiveRecord::Base
# Get the commit hash for the upper bound
max_hash = nil
- IO.foreach("|git rev-list --max-count=1 #{maximum.shellescape}") do |line|
+ IO.foreach("|git rev-list --max-count=1 #{maximum.shellescape} --") do |line|
max_hash = line.strip
end
@@ -58,7 +58,7 @@ class Commit < ActiveRecord::Base
resolved_exclude = []
exclude.each do |e|
if git_check_ref_format(e)
- IO.foreach("|git rev-list --max-count=1 #{e.shellescape}") do |line|
+ IO.foreach("|git rev-list --max-count=1 #{e.shellescape} --") do |line|
resolved_exclude.push(line.strip)
end
else
@@ -71,7 +71,7 @@ class Commit < ActiveRecord::Base
if minimum
# Get the commit hash for the lower bound
min_hash = nil
- IO.foreach("|git rev-list --max-count=1 #{minimum.shellescape}") do |line|
+ IO.foreach("|git rev-list --max-count=1 #{minimum.shellescape} --") do |line|
min_hash = line.strip
end
@@ -79,7 +79,7 @@ class Commit < ActiveRecord::Base
next if !min_hash or !git_check_ref_format(min_hash)
# Now find all commits between them
- IO.foreach("|git rev-list #{min_hash.shellescape}..#{max_hash.shellescape}") do |line|
+ IO.foreach("|git rev-list #{min_hash.shellescape}..#{max_hash.shellescape} --") do |line|
hash = line.strip
commits.push(hash) if !resolved_exclude or !resolved_exclude.include? hash
end
commit 5c5cb0a226a165c0028bb2c55cba8f4dcc359be1
Author: Brett Smith <brett at curoverse.com>
Date: Tue Jul 8 09:49:16 2014 -0400
3195: Improve error handling in Job creation API.
* Express the default filters using high-level operators, then convert
them to SQL filters using the same conversion logic users get.
* Raise an ArgumentError when a git version search fails.
* Render normal error pages from ArgumentErrors.
diff --git a/services/api/app/controllers/arvados/v1/jobs_controller.rb b/services/api/app/controllers/arvados/v1/jobs_controller.rb
index feeb82d..3039433 100644
--- a/services/api/app/controllers/arvados/v1/jobs_controller.rb
+++ b/services/api/app/controllers/arvados/v1/jobs_controller.rb
@@ -26,32 +26,35 @@ class Arvados::V1::JobsController < ApplicationController
end
if params[:find_or_create]
- load_filters_param
+ return if false.equal?(load_filters_param)
if @filters.empty? # Translate older creation parameters into filters.
- @filters = [:repository, :script].map do |attrsym|
- [attrsym.to_s, "=", resource_attrs[attrsym]]
- end
- @filters.append(["script_version", "in",
- Commit.find_commit_range(current_user,
- resource_attrs[:repository],
- params[:minimum_script_version],
- resource_attrs[:script_version],
- params[:exclude_script_versions])])
+ @filters =
+ [["repository", "=", resource_attrs[:repository]],
+ ["script", "=", resource_attrs[:script]],
+ ["script_version", "in git",
+ params[:minimum_script_version] || resource_attrs[:script_version]],
+ ["script_version", "not in git", params[:exclude_script_versions]],
+ ].reject { |filter| filter.last.nil? }
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, @read_users).first
- return super if image_locator.nil? # We won't find anything to reuse.
- @filters.append(["docker_image_locator", "=", image_locator])
+ if image_tag = resource_attrs[:runtime_constraints]["docker_image_tag"]
+ image_search += ":#{image_tag}"
+ end
+ @filters.append(["docker_image_locator", "in docker", image_search])
else
@filters.append(["docker_image_locator", "=", nil])
end
- else # Check specified filters for some reasonableness.
- filter_names = @filters.map { |f| f.first }.uniq
- ["repository", "script"].each do |req_filter|
- if not filter_names.include?(req_filter)
- raise ArgumentError.new("#{req_filter} filter required")
- end
+ begin
+ load_job_specific_filters
+ rescue ArgumentError => error
+ return send_error(error.message)
+ end
+ end
+
+ # Check specified filters for some reasonableness.
+ filter_names = @filters.map { |f| f.first }.uniq
+ ["repository", "script"].each do |req_filter|
+ if not filter_names.include?(req_filter)
+ return send_error("#{req_filter} filter required")
end
end
@@ -152,7 +155,7 @@ class Arvados::V1::JobsController < ApplicationController
cancelled_at: nil,
success: nil
})
- load_filters_param
+ return if false.equal?(load_filters_param)
find_objects_for_index
index
end
@@ -163,9 +166,8 @@ class Arvados::V1::JobsController < ApplicationController
protected
- def load_filters_param
- # Convert Job-specific git and Docker filters into normal SQL filters.
- super
+ def load_job_specific_filters
+ # Convert Job-specific @filters entries into general SQL filters.
script_info = {"repository" => nil, "script" => nil}
script_range = {"exclude_versions" => []}
@filters.select! do |filter|
@@ -208,12 +210,28 @@ class Arvados::V1::JobsController < ApplicationController
end
end
last_version = begin resource_attrs[:script_version] rescue "HEAD" end
- @filters.append(["script_version", "in",
- Commit.find_commit_range(current_user,
- script_info["repository"],
- script_range["min_version"],
- last_version,
- script_range["exclude_versions"])])
+ version_range = Commit.find_commit_range(current_user,
+ script_info["repository"],
+ script_range["min_version"],
+ last_version,
+ script_range["exclude_versions"])
+ if version_range.nil?
+ raise ArgumentError.
+ new(["error searching #{script_info['repository']} from",
+ "#{script_range['min_version']} to #{last_version},",
+ "excluding #{script_range['exclude_versions']}"].join(" "))
+ end
+ @filters.append(["script_version", "in", version_range])
+ end
+ end
+
+ def load_filters_param
+ begin
+ super
+ load_job_specific_filters
+ rescue ArgumentError => error
+ send_error(error.message)
+ false
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 62bc866..a22a493 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
@@ -551,4 +551,75 @@ class Arvados::V1::JobReuseControllerTest < ActionController::TestCase
refute_includes(assigns(:objects).map { |job| job.uuid },
jobs(:previous_docker_job_run).uuid)
end
+
+ def create_foo_hash_job_params(params)
+ if not params.has_key?(:find_or_create)
+ params[:find_or_create] = true
+ end
+ job_attrs = params.delete(:job) || {}
+ params[:job] = {
+ script: "hash",
+ script_version: "4fe459abe02d9b365932b8f5dc419439ab4e2577",
+ repository: "foo",
+ script_parameters: {
+ input: 'fa7aeb5140e2848d39b416daeef4ffc5+45',
+ an_integer: '1',
+ },
+ }.merge(job_attrs)
+ params
+ end
+
+ def check_new_job_created_from(params)
+ start_time = Time.now
+ post(:create, create_foo_hash_job_params(params))
+ assert_response :success
+ new_job = assigns(:object)
+ assert_not_nil new_job
+ assert_operator(start_time, :<=, new_job.created_at)
+ new_job
+ end
+
+ def check_errors_from(params)
+ post(:create, create_foo_hash_job_params(params))
+ assert_includes(405..499, @response.code.to_i)
+ errors = json_response.fetch("errors", [])
+ assert(errors.any?, "no errors assigned from #{params}")
+ refute(errors.any? { |msg| msg =~ /^#<[A-Za-z]+: / },
+ "errors include raw exception")
+ errors
+ end
+
+ # 1de84a8 is on the b1 branch, after master's tip.
+ test "new job created from unsatisfiable minimum version filter" do
+ filter_h = BASE_FILTERS.merge("script_version" => ["in git", "1de84a8"])
+ check_new_job_created_from(filters: filters_from_hash(filter_h))
+ end
+
+ test "new job created from unsatisfiable minimum version parameter" do
+ check_new_job_created_from(minimum_script_version: "1de84a8")
+ end
+
+ test "new job created from unsatisfiable minimum version attribute" do
+ check_new_job_created_from(job: {minimum_script_version: "1de84a8"})
+ end
+
+ test "graceful error from nonexistent minimum version filter" do
+ filter_h = BASE_FILTERS.merge("script_version" =>
+ ["in git", "__nosuchbranch__"])
+ errors = check_errors_from(filters: filters_from_hash(filter_h))
+ assert(errors.any? { |msg| msg.include? "__nosuchbranch__" },
+ "bad refspec not mentioned in error message")
+ end
+
+ test "graceful error from nonexistent minimum version parameter" do
+ errors = check_errors_from(minimum_script_version: "__nosuchbranch__")
+ assert(errors.any? { |msg| msg.include? "__nosuchbranch__" },
+ "bad refspec not mentioned in error message")
+ end
+
+ test "graceful error from nonexistent minimum version attribute" do
+ errors = check_errors_from(job: {minimum_script_version: "__nosuchbranch__"})
+ assert(errors.any? { |msg| msg.include? "__nosuchbranch__" },
+ "bad refspec not mentioned in error message")
+ end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list