[ARVADOS] created: 1.3.0-1087-g2870aa32d
Git user
git at public.curoverse.com
Mon Jun 17 19:09:12 UTC 2019
at 2870aa32d3ae875aac19af2da1b2297b9e57689f (commit)
commit 2870aa32d3ae875aac19af2da1b2297b9e57689f
Author: Peter Amstutz <pamstutz at veritasgenetics.com>
Date: Mon Jun 17 15:08:53 2019 -0400
15295: Check that keep references are well formed
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/sdk/cwl/arvados_cwl/pathmapper.py b/sdk/cwl/arvados_cwl/pathmapper.py
index 56c15a4a4..4cd204f7d 100644
--- a/sdk/cwl/arvados_cwl/pathmapper.py
+++ b/sdk/cwl/arvados_cwl/pathmapper.py
@@ -42,13 +42,13 @@ def trim_listing(obj):
if obj.get("location", "").startswith("keep:") and "listing" in obj:
del obj["listing"]
+collection_pdh_path = re.compile(r'^keep:[0-9a-f]{32}\+\d+/.+$')
+collection_pdh_pattern = re.compile(r'^keep:([0-9a-f]{32}\+\d+)(/.*)?')
+collection_uuid_pattern = re.compile(r'^keep:([a-z0-9]{5}-4zz18-[a-z0-9]{15})(/.*)?$')
class ArvPathMapper(PathMapper):
"""Convert container-local paths to and from Keep collection ids."""
- pdh_path = re.compile(r'^keep:[0-9a-f]{32}\+\d+/.+$')
- pdh_dirpath = re.compile(r'^keep:[0-9a-f]{32}\+\d+(/.*)?$')
-
def __init__(self, arvrunner, referenced_files, input_basedir,
collection_pattern, file_pattern, name=None, single_collection=False):
self.arvrunner = arvrunner
@@ -66,13 +66,17 @@ class ArvPathMapper(PathMapper):
if "#" in src:
src = src[:src.index("#")]
- if isinstance(src, basestring) and ArvPathMapper.pdh_dirpath.match(src):
- self._pathmap[src] = MapperEnt(src, self.collection_pattern % urllib.parse.unquote(src[5:]), srcobj["class"], True)
- if arvados_cwl.util.collectionUUID in srcobj:
- self.pdh_to_uuid[src.split("/", 1)[0][5:]] = srcobj[arvados_cwl.util.collectionUUID]
-
debug = logger.isEnabledFor(logging.DEBUG)
+ if isinstance(src, basestring) and src.startswith("keep:"):
+ if collection_pdh_pattern.match(src):
+ self._pathmap[src] = MapperEnt(src, self.collection_pattern % urllib.parse.unquote(src[5:]), srcobj["class"], True)
+ if arvados_cwl.util.collectionUUID in srcobj:
+ self.pdh_to_uuid[src.split("/", 1)[0][5:]] = srcobj[arvados_cwl.util.collectionUUID]
+ elif not collection_uuid_pattern.match(src):
+ with SourceLine(srcobj, "location", WorkflowException, debug):
+ raise WorkflowException("Invalid keep reference '%s'" % src)
+
if src not in self._pathmap:
if src.startswith("file:"):
# Local FS ref, may need to be uploaded or may be on keep
diff --git a/sdk/cwl/arvados_cwl/runner.py b/sdk/cwl/arvados_cwl/runner.py
index 912faf0e8..5e42df624 100644
--- a/sdk/cwl/arvados_cwl/runner.py
+++ b/sdk/cwl/arvados_cwl/runner.py
@@ -44,7 +44,7 @@ from .util import collectionUUID
import ruamel.yaml as yaml
import arvados_cwl.arvdocker
-from .pathmapper import ArvPathMapper, trim_listing
+from .pathmapper import ArvPathMapper, trim_listing, collection_pdh_pattern, collection_uuid_pattern
from ._version import __version__
from . import done
from . context import ArvRuntimeContext
@@ -194,9 +194,6 @@ def discover_secondary_files(fsaccess, builder, inputs, job_order, discovered=No
if isinstance(primary, (Mapping, Sequence)):
set_secondary(fsaccess, builder, inputschema, None, primary, discovered)
-collection_uuid_pattern = re.compile(r'^keep:([a-z0-9]{5}-4zz18-[a-z0-9]{15})(/.*)?$')
-collection_pdh_pattern = re.compile(r'^keep:([0-9a-f]{32}\+\d+)(/.*)?')
-
def upload_dependencies(arvrunner, name, document_loader,
workflowobj, uri, loadref_run,
include_primary=True, discovered_secondaryfiles=None):
diff --git a/sdk/cwl/tests/15295-bad-keep-ref.cwl b/sdk/cwl/tests/15295-bad-keep-ref.cwl
new file mode 100644
index 000000000..53c73bb19
--- /dev/null
+++ b/sdk/cwl/tests/15295-bad-keep-ref.cwl
@@ -0,0 +1,12 @@
+cwlVersion: v1.0
+class: CommandLineTool
+requirements:
+ - class: InlineJavascriptRequirement
+arguments:
+ - ls
+ - -l
+ - $(inputs.hello)
+inputs:
+ hello:
+ type: File
+outputs: []
diff --git a/sdk/cwl/tests/arvados-tests.yml b/sdk/cwl/tests/arvados-tests.yml
index d649c3bf6..0eb606d25 100644
--- a/sdk/cwl/tests/arvados-tests.yml
+++ b/sdk/cwl/tests/arvados-tests.yml
@@ -298,3 +298,9 @@
}
tool: 15241-writable-dir.cwl
doc: Test for writable collections
+
+- job: badkeep.yml
+ output: {}
+ should_fail: true
+ tool: 15295-bad-keep-ref.cwl
+ doc: Test checking for invalid keepref
diff --git a/sdk/cwl/tests/badkeep.yml b/sdk/cwl/tests/badkeep.yml
new file mode 100644
index 000000000..7f6378a94
--- /dev/null
+++ b/sdk/cwl/tests/badkeep.yml
@@ -0,0 +1,3 @@
+hello:
+ class: File
+ location: keep:/4d8a70b1e63b2aad6984e40e338e2373+69/hello.txt
commit b441daaf99f0596170a083d798ba3276f3dbe565
Author: Peter Amstutz <pamstutz at veritasgenetics.com>
Date: Mon Jun 17 14:28:53 2019 -0400
15295: Fix updating and checking lock_count when updating state
Previously lock_count was only updated on explicit lock or unlock
calls. Now updates to state will correctly update or check
lock_count.
Also tweak row locking for containers, remove redudant reload, also
make sure lock and unlock take a lock.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index c730c87b5..041f55947 100644
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -30,7 +30,6 @@ class Arvados::V1::ContainersController < ApplicationController
def update
@object.with_lock do
- @object.reload
super
end
end
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 2bbdd0a07..619abcc86 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -31,6 +31,8 @@ class Container < ArvadosModel
before_validation :fill_field_defaults, :if => :new_record?
before_validation :set_timestamps
+ before_validation :check_lock
+ before_validation :check_unlock
validates :command, :container_image, :output_path, :cwd, :priority, { presence: true }
validates :priority, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
validate :validate_runtime_status
@@ -73,6 +75,7 @@ class Container < ArvadosModel
t.add :scheduling_parameters
t.add :runtime_user_uuid
t.add :runtime_auth_scopes
+ t.add :lock_count
end
# Supported states for a container
@@ -335,47 +338,41 @@ class Container < ArvadosModel
nil
end
- def check_lock_fail
- if self.state != Queued
- raise LockFailedError.new("cannot lock when #{self.state}")
- elsif self.priority <= 0
- raise LockFailedError.new("cannot lock when priority<=0")
+ def lock
+ self.with_lock do
+ if self.state != Queued
+ raise LockFailedError.new("cannot lock when #{self.state}")
+ end
+ self.update_attributes!(state: Locked)
end
end
- def lock
- # Check invalid state transitions once before getting the lock
- # (because it's cheaper that way) and once after getting the lock
- # (because state might have changed while acquiring the lock).
- check_lock_fail
- transaction do
- reload
- check_lock_fail
- update_attributes!(state: Locked, lock_count: self.lock_count+1)
+ def check_lock
+ if state_was == Queued and state == Locked
+ if self.priority <= 0
+ raise LockFailedError.new("cannot lock when priority<=0")
+ end
+ self.lock_count = self.lock_count+1
end
end
- def check_unlock_fail
- if self.state != Locked
- raise InvalidStateTransitionError.new("cannot unlock when #{self.state}")
- elsif self.locked_by_uuid != current_api_client_authorization.uuid
- raise InvalidStateTransitionError.new("locked by a different token")
+ def unlock
+ self.with_lock do
+ if self.state != Locked
+ raise InvalidStateTransitionError.new("cannot unlock when #{self.state}")
+ end
+ self.update_attributes!(state: Queued)
end
end
- def unlock
- # Check invalid state transitions twice (see lock)
- check_unlock_fail
- transaction do
- reload(lock: 'FOR UPDATE')
- check_unlock_fail
- if self.lock_count < Rails.configuration.Containers.MaxDispatchAttempts
- update_attributes!(state: Queued)
- else
- update_attributes!(state: Cancelled,
- runtime_status: {
- error: "Container exceeded 'max_container_dispatch_attempts' (lock_count=#{self.lock_count}."
- })
+ def check_unlock
+ if state_was == Locked and state == Queued
+ if self.locked_by_uuid != current_api_client_authorization.uuid
+ raise InvalidStateTransitionError.new("locked by a different token")
+ end
+ if self.lock_count >= Rails.configuration.Containers.MaxDispatchAttempts
+ self.state = Cancelled
+ self.runtime_status = {error: "Failed to start container. Cancelled after exceeding 'Containers.MaxDispatchAttempts' (lock_count=#{self.lock_count})"}
end
end
end
commit 5a65708195cd5d0c6a588fab96ae441f3cf0bd04
Author: Peter Amstutz <pamstutz at veritasgenetics.com>
Date: Fri Jun 14 11:09:13 2019 -0400
15295: Needs locks_count
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index fc5614d94..c730c87b5 100644
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -39,7 +39,7 @@ class Arvados::V1::ContainersController < ApplicationController
super
if action_name == 'lock' || action_name == 'unlock'
# Avoid loading more fields than we need
- @objects = @objects.select(:id, :uuid, :state, :priority, :auth_uuid, :locked_by_uuid)
+ @objects = @objects.select(:id, :uuid, :state, :priority, :auth_uuid, :locked_by_uuid, :lock_count)
@select = %w(uuid state priority auth_uuid locked_by_uuid)
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list