Merge branch '4019-query-properties' closes #4019
authorPeter Amstutz <pamstutz@veritasgenetics.com>
Wed, 13 Dec 2017 16:12:56 +0000 (11:12 -0500)
committerPeter Amstutz <pamstutz@veritasgenetics.com>
Wed, 13 Dec 2017 16:12:58 +0000 (11:12 -0500)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

33 files changed:
.gitignore
apps/workbench/app/views/application/404.html.erb
apps/workbench/test/integration/errors_test.rb
build/run-build-docker-jobs-image.sh
sdk/cli/bin/crunch-job
sdk/cwl/arvados_cwl/_version.py [new file with mode: 0644]
sdk/go/arvados/collection_fs.go
sdk/go/arvados/collection_fs_test.go
sdk/python/arvados/api.py
sdk/python/arvados/commands/get.py
sdk/python/arvados/commands/put.py
sdk/python/arvados/keep.py
sdk/python/arvados/util.py
sdk/python/tests/test_api.py
sdk/python/tests/test_arv_get.py
sdk/python/tests/test_arv_put.py
sdk/python/tests/test_events.py
sdk/python/tests/test_keep_client.py
services/api/app/controllers/arvados/v1/collections_controller.rb
services/api/app/controllers/arvados/v1/groups_controller.rb
services/api/app/models/arvados_model.rb
services/api/app/models/collection.rb
services/api/app/models/container.rb
services/api/app/models/container_request.rb
services/api/lib/sweep_trashed_collections.rb
services/api/test/fixtures/container_requests.yml
services/api/test/functional/arvados/v1/collections_controller_test.rb
services/api/test/unit/collection_test.rb
services/api/test/unit/container_test.rb
services/nodemanager/arvnodeman/computenode/dispatch/transitions.py
services/nodemanager/arvnodeman/daemon.py
services/nodemanager/arvnodeman/nodelist.py
services/nodemanager/tests/test_computenode_dispatch.py

index 0e876bb6f4d430eea2a79bbe34b0ee3f37c8a6fc..e61f485237b6b1145e3527982d0fbbbadaf56727 100644 (file)
@@ -23,7 +23,6 @@ sdk/java/target
 sdk/java/log
 tmp
 sdk/cli/binstubs/
-sdk/cwl/arvados_cwl/_version.py
 services/api/config/arvados-clients.yml
 *#*
 .DS_Store
index 283f5d59fd78a228a662d10f6823a88e85af7bf7..61cbd670050c5d391e7699d3cf8947f04e994502 100644 (file)
@@ -16,20 +16,29 @@ SPDX-License-Identifier: AGPL-3.0 %>
    end
 %>
 
-  <% if check_trash.andand.any? %>
-    <h2>Trashed</h2>
-
-      <% object = check_trash.first %>
+  <% untrash_object = nil %>
 
+  <% if check_trash.andand.any? %>
+    <% object = check_trash.first %>
+    <% if object.respond_to?(:is_trashed) && object.is_trashed %>
       <% untrash_object = object %>
-      <% while !untrash_object.is_trashed %>
-        <% owner = Group.where(uuid: untrash_object.owner_uuid).include_trash(true).first %>
-        <% if owner.nil? then %>
+    <% else %>
+      <% owner = object %>
+      <% while true %>
+        <% owner = Group.where(uuid: owner.owner_uuid).include_trash(true).first %>
+        <% if owner.nil? %>
           <% break %>
-        <% else %>
+        <% end %>
+        <% if owner.is_trashed %>
           <% untrash_object = owner %>
+          <% break %>
         <% end %>
       <% end %>
+    <% end %>
+  <% end %>
+
+  <% if !untrash_object.nil? %>
+    <h2>Trashed</h2>
 
       <% untrash_name = if !untrash_object.name.blank? then
                  "'#{untrash_object.name}'"
index f9054d0ac697d4e789349b43a9a9110114ee07bf..81d4bbbaa1735dd98ce2ccbb7a54b25b8675a43b 100644 (file)
@@ -112,4 +112,17 @@ class ErrorsTest < ActionDispatch::IntegrationTest
     # out of the popup now and should be back in the error page
     assert_text 'fiddlesticks'
   end
+
+  test "showing a trashed collection UUID gives untrash button" do
+    visit(page_with_token("active", "/collections/zzzzz-4zz18-trashedproj2col"))
+    assert(page.has_text?(/You must untrash the owner project to access this/i),
+           "missing untrash instructions")
+  end
+
+  test "showing a trashed container request gives untrash button" do
+    visit(page_with_token("active", "/container_requests/zzzzz-xvhdp-cr5trashedcontr"))
+    assert(page.has_text?(/You must untrash the owner project to access this/i),
+           "missing untrash instructions")
+  end
+
 end
index 90079268f896f986d2c3e876c2a4d860dcc21c46..fb970affb4c4bbe94ef5a7281a2389732044aae9 100755 (executable)
@@ -121,23 +121,20 @@ cd "$WORKSPACE"
 python_sdk_ts=$(cd sdk/python && timestamp_from_git)
 cwl_runner_ts=$(cd sdk/cwl && timestamp_from_git)
 
-python_sdk_version=$(cd sdk/python && nohash_version_from_git 0.1)-2
-cwl_runner_version=$(cd sdk/cwl && nohash_version_from_git 1.0)-3
+python_sdk_version=$(cd sdk/python && nohash_version_from_git 0.1)
+cwl_runner_version=$(cd sdk/cwl && nohash_version_from_git 1.0)
 
 if [[ $python_sdk_ts -gt $cwl_runner_ts ]]; then
-    cwl_runner_version=$(cd sdk/python && nohash_version_from_git 1.0)-3
-    gittag=$(git log --first-parent --max-count=1 --format=format:%H sdk/python)
-else
-    gittag=$(git log --first-parent --max-count=1 --format=format:%H sdk/cwl)
+    cwl_runner_version=$(cd sdk/python && nohash_version_from_git 1.0)
 fi
 
 echo cwl_runner_version $cwl_runner_version python_sdk_version $python_sdk_version
 
 cd docker/jobs
 docker build $NOCACHE \
-       --build-arg python_sdk_version=$python_sdk_version \
-       --build-arg cwl_runner_version=$cwl_runner_version \
-       -t arvados/jobs:$gittag .
+       --build-arg python_sdk_version=${python_sdk_version}-2 \
+       --build-arg cwl_runner_version=${cwl_runner_version}-3 \
+       -t arvados/jobs:$cwl_runner_version .
 
 ECODE=$?
 
@@ -160,7 +157,7 @@ if docker --version |grep " 1\.[0-9]\." ; then
     FORCE=-f
 fi
 
-docker tag $FORCE arvados/jobs:$gittag arvados/jobs:latest
+docker tag $FORCE arvados/jobs:$cwl_runner_version arvados/jobs:latest
 
 ECODE=$?
 
@@ -183,7 +180,7 @@ else
         ## even though credentials are already in .dockercfg
         docker login -u arvados
 
-        docker_push arvados/jobs:$gittag
+        docker_push arvados/jobs:$cwl_runner_version
         docker_push arvados/jobs:latest
         title "upload arvados images finished (`timer`)"
     else
index f2e9fc2878c4ded51ce3069e15620061eabf5910..9343fcfbfd2f97bc182daa788f5c45f74b8ae078 100755 (executable)
@@ -1,10 +1,9 @@
 #!/usr/bin/env perl
+# -*- mode: perl; perl-indent-level: 2; indent-tabs-mode: nil; -*-
 # Copyright (C) The Arvados Authors. All rights reserved.
 #
 # SPDX-License-Identifier: AGPL-3.0
 
-# -*- mode: perl; perl-indent-level: 2; indent-tabs-mode: nil; -*-
-
 =head1 NAME
 
 crunch-job: Execute job steps, save snapshots as requested, collate output.
@@ -1188,6 +1187,8 @@ sub reapchildren
                     . $slot[$proc{$pid}->{slot}]->{cpu});
     my $jobstepidx = $proc{$pid}->{jobstepidx};
 
+    readfrompipes_after_exit ($jobstepidx);
+
     $children_reaped++;
     my $elapsed = time - $proc{$pid}->{time};
     my $Jobstep = $jobstep[$jobstepidx];
@@ -1259,7 +1260,6 @@ sub reapchildren
     $Jobstep->{finishtime} = time;
     $Jobstep->{'arvados_task'}->{finished_at} = strftime "%Y-%m-%dT%H:%M:%SZ", gmtime($Jobstep->{finishtime});
     retry_op(sub { $Jobstep->{'arvados_task'}->save; }, "job_tasks.update API");
-    process_stderr_final ($jobstepidx);
     Log ($jobstepidx, sprintf("task output (%d bytes): %s",
                               length($Jobstep->{'arvados_task'}->{output}),
                               $Jobstep->{'arvados_task'}->{output}));
@@ -1562,9 +1562,27 @@ sub preprocess_stderr
 }
 
 
-sub process_stderr_final
+# Read whatever is still available on its stderr+stdout pipes after
+# the given child process has exited.
+sub readfrompipes_after_exit
 {
   my $jobstepidx = shift;
+
+  # The fact that the child has exited allows some convenient
+  # simplifications: (1) all data must have already been written, so
+  # there's no need to wait for more once sysread returns 0; (2) the
+  # total amount of data available is bounded by the pipe buffer size,
+  # so it's safe to read everything into one string.
+  my $buf;
+  while (0 < sysread ($reader{$jobstepidx}, $buf, 65536)) {
+    $jobstep[$jobstepidx]->{stderr_at} = time;
+    $jobstep[$jobstepidx]->{stderr} .= $buf;
+  }
+  if ($jobstep[$jobstepidx]->{stdout_r}) {
+    while (0 < sysread ($jobstep[$jobstepidx]->{stdout_r}, $buf, 65536)) {
+      $jobstep[$jobstepidx]->{stdout_captured} .= $buf;
+    }
+  }
   preprocess_stderr ($jobstepidx);
 
   map {
@@ -2036,8 +2054,7 @@ sub srun_sync
   }
   my $exited = $?;
 
-  1 while readfrompipes();
-  process_stderr_final ($jobstepidx);
+  readfrompipes_after_exit ($jobstepidx);
 
   Log (undef, "$label: exit ".exit_status_s($exited));
 
diff --git a/sdk/cwl/arvados_cwl/_version.py b/sdk/cwl/arvados_cwl/_version.py
new file mode 100644 (file)
index 0000000..652a291
--- /dev/null
@@ -0,0 +1,7 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: Apache-2.0
+
+import pkg_resources
+
+__version__ = pkg_resources.require('arvados-cwl-runner')[0].version
index 28629e33b20f31189ac460d02ab2868fdd85db96..7bbbaa492c71298157f3cfebc96dfade89966ae0 100644 (file)
@@ -864,7 +864,7 @@ func (dn *dirnode) loadManifest(txt string) error {
                                return fmt.Errorf("line %d: bad locator %q", lineno, token)
                        }
 
-                       toks := strings.Split(token, ":")
+                       toks := strings.SplitN(token, ":", 3)
                        if len(toks) != 3 {
                                return fmt.Errorf("line %d: bad file segment %q", lineno, token)
                        }
index f1a34754f732bd9d94a2589e72aef164424827b1..57ba3255947439ded25834e6b8841cd51fe70f48 100644 (file)
@@ -77,6 +77,21 @@ func (s *CollectionFSSuite) TestHttpFileSystemInterface(c *check.C) {
        c.Check(ok, check.Equals, true)
 }
 
+func (s *CollectionFSSuite) TestColonInFilename(c *check.C) {
+       fs, err := (&Collection{
+               ManifestText: "./foo:foo 3858f62230ac3c915f300c664312c63f+3 0:3:bar:bar\n",
+       }).FileSystem(s.client, s.kc)
+       c.Assert(err, check.IsNil)
+
+       f, err := fs.Open("/foo:foo")
+       c.Assert(err, check.IsNil)
+
+       fis, err := f.Readdir(0)
+       c.Check(err, check.IsNil)
+       c.Check(len(fis), check.Equals, 1)
+       c.Check(fis[0].Name(), check.Equals, "bar:bar")
+}
+
 func (s *CollectionFSSuite) TestReaddirFull(c *check.C) {
        f, err := s.fs.Open("/dir1")
        c.Assert(err, check.IsNil)
@@ -928,7 +943,7 @@ func (s *CollectionFSSuite) TestBrokenManifests(c *check.C) {
                ".  0:0:foo\n",
                ". 0:0:foo 0:0:bar\n",
                ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo\n",
-               ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo:bar\n",
+               ". d41d8cd98f00b204e9800998ecf8427e+0 :0:0:foo\n",
                ". d41d8cd98f00b204e9800998ecf8427e+0 foo:0:foo\n",
                ". d41d8cd98f00b204e9800998ecf8427e+0 0:foo:foo\n",
                ". d41d8cd98f00b204e9800998ecf8427e+1 0:1:foo 1:1:bar\n",
index e69f1a112d401c21875b2ab310e444fef2f1ddc6..4611a1aadf80043eb9afdeeaff727b27a09eecbc 100644 (file)
@@ -52,19 +52,18 @@ class OrderedJsonModel(apiclient.model.JsonModel):
         return body
 
 
-def _intercept_http_request(self, uri, method="GET", **kwargs):
+def _intercept_http_request(self, uri, method="GET", headers={}, **kwargs):
     if (self.max_request_size and
         kwargs.get('body') and
         self.max_request_size < len(kwargs['body'])):
         raise apiclient_errors.MediaUploadSizeError("Request size %i bytes exceeds published limit of %i bytes" % (len(kwargs['body']), self.max_request_size))
 
-    if 'headers' not in kwargs:
-        kwargs['headers'] = {}
-
     if config.get("ARVADOS_EXTERNAL_CLIENT", "") == "true":
-        kwargs['headers']['X-External-Client'] = '1'
+        headers['X-External-Client'] = '1'
 
-    kwargs['headers']['Authorization'] = 'OAuth2 %s' % self.arvados_api_token
+    headers['Authorization'] = 'OAuth2 %s' % self.arvados_api_token
+    if not headers.get('X-Request-Id'):
+        headers['X-Request-Id'] = self._request_id()
 
     retryable = method in [
         'DELETE', 'GET', 'HEAD', 'OPTIONS', 'PUT']
@@ -83,7 +82,7 @@ def _intercept_http_request(self, uri, method="GET", **kwargs):
     for _ in range(retry_count):
         self._last_request_time = time.time()
         try:
-            return self.orig_http_request(uri, method, **kwargs)
+            return self.orig_http_request(uri, method, headers=headers, **kwargs)
         except http.client.HTTPException:
             _logger.debug("Retrying API request in %d s after HTTP error",
                           delay, exc_info=True)
@@ -101,7 +100,7 @@ def _intercept_http_request(self, uri, method="GET", **kwargs):
         delay = delay * self._retry_delay_backoff
 
     self._last_request_time = time.time()
-    return self.orig_http_request(uri, method, **kwargs)
+    return self.orig_http_request(uri, method, headers=headers, **kwargs)
 
 def _patch_http_request(http, api_token):
     http.arvados_api_token = api_token
@@ -113,6 +112,7 @@ def _patch_http_request(http, api_token):
     http._retry_delay_initial = RETRY_DELAY_INITIAL
     http._retry_delay_backoff = RETRY_DELAY_BACKOFF
     http._retry_count = RETRY_COUNT
+    http._request_id = util.new_request_id
     return http
 
 # Monkey patch discovery._cast() so objects and arrays get serialized
@@ -148,7 +148,8 @@ def http_cache(data_type):
         return None
     return cache.SafeHTTPCache(path, max_age=60*60*24*2)
 
-def api(version=None, cache=True, host=None, token=None, insecure=False, **kwargs):
+def api(version=None, cache=True, host=None, token=None, insecure=False,
+        request_id=None, **kwargs):
     """Return an apiclient Resources object for an Arvados instance.
 
     :version:
@@ -168,6 +169,12 @@ def api(version=None, cache=True, host=None, token=None, insecure=False, **kwarg
     :insecure:
       If True, ignore SSL certificate validation errors.
 
+    :request_id:
+      Default X-Request-Id header value for outgoing requests that
+      don't already provide one. If None or omitted, generate a random
+      ID. When retrying failed requests, the same ID is used on all
+      attempts.
+
     Additional keyword arguments will be passed directly to
     `apiclient_discovery.build` if a new Resource object is created.
     If the `discoveryServiceUrl` or `http` keyword arguments are
@@ -192,7 +199,8 @@ def api(version=None, cache=True, host=None, token=None, insecure=False, **kwarg
     elif host and token:
         pass
     elif not host and not token:
-        return api_from_config(version=version, cache=cache, **kwargs)
+        return api_from_config(
+            version=version, cache=cache, request_id=request_id, **kwargs)
     else:
         # Caller provided one but not the other
         if not host:
@@ -218,8 +226,10 @@ def api(version=None, cache=True, host=None, token=None, insecure=False, **kwarg
     svc = apiclient_discovery.build('arvados', version, cache_discovery=False, **kwargs)
     svc.api_token = token
     svc.insecure = insecure
+    svc.request_id = request_id
     kwargs['http'].max_request_size = svc._rootDesc.get('maxRequestSize', 0)
     kwargs['http'].cache = None
+    kwargs['http']._request_id = lambda: svc.request_id or util.new_request_id()
     return svc
 
 def api_from_config(version=None, apiconfig=None, **kwargs):
index 881fdd6ad0f968eddf3ab810d90e62df70b63895..1e527149168daa8d1a892abf0638517936891d79 100755 (executable)
@@ -81,6 +81,10 @@ Overwrite existing files while writing. The default behavior is to
 refuse to write *anything* if any of the output files already
 exist. As a special case, -f is not needed to write to stdout.
 """)
+group.add_argument('-v', action='count', default=0,
+                    help="""
+Once for verbose mode, twice for debug mode.
+""")
 group.add_argument('--skip-existing', action='store_true',
                    help="""
 Skip files that already exist. The default behavior is to refuse to
@@ -140,8 +144,13 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
         stdout = stdout.buffer
 
     args = parse_arguments(arguments, stdout, stderr)
+    logger.setLevel(logging.WARNING - 10 * args.v)
+
+    request_id = arvados.util.new_request_id()
+    logger.info('X-Request-Id: '+request_id)
+
     if api_client is None:
-        api_client = arvados.api('v1')
+        api_client = arvados.api('v1', request_id=request_id)
 
     r = re.search(r'^(.*?)(/.*)?$', args.locator)
     col_loc = r.group(1)
@@ -157,14 +166,15 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
                 open_flags |= os.O_EXCL
             try:
                 if args.destination == "-":
-                    write_block_or_manifest(dest=stdout, src=col_loc,
-                                            api_client=api_client, args=args)
+                    write_block_or_manifest(
+                        dest=stdout, src=col_loc,
+                        api_client=api_client, args=args)
                 else:
                     out_fd = os.open(args.destination, open_flags)
                     with os.fdopen(out_fd, 'wb') as out_file:
-                        write_block_or_manifest(dest=out_file,
-                                                src=col_loc, api_client=api_client,
-                                                args=args)
+                        write_block_or_manifest(
+                            dest=out_file, src=col_loc,
+                            api_client=api_client, args=args)
             except (IOError, OSError) as error:
                 logger.error("can't write to '{}': {}".format(args.destination, error))
                 return 1
@@ -180,7 +190,8 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
         return 0
 
     try:
-        reader = arvados.CollectionReader(col_loc, num_retries=args.retries)
+        reader = arvados.CollectionReader(
+            col_loc, api_client=api_client, num_retries=args.retries)
     except Exception as error:
         logger.error("failed to read collection: {}".format(error))
         return 1
@@ -305,5 +316,6 @@ def write_block_or_manifest(dest, src, api_client, args):
         dest.write(kc.get(src, num_retries=args.retries))
     else:
         # collection UUID or portable data hash
-        reader = arvados.CollectionReader(src, num_retries=args.retries)
+        reader = arvados.CollectionReader(
+            src, api_client=api_client, num_retries=args.retries)
         dest.write(reader.manifest_text(strip=args.strip_manifest).encode())
index ec4ae8fb6f971a8d19307a58311b2dec8eed70bc..97ff8c6f3fe12eaa5d936a4dcdeb418d4eb036c3 100644 (file)
@@ -193,9 +193,10 @@ Display machine-readable progress on stderr (bytes and, if known,
 total data size).
 """)
 
-_group.add_argument('--silent', action='store_true',
-                    help="""
-Do not print any debug messages to console. (Any error messages will still be displayed.)
+run_opts.add_argument('--silent', action='store_true',
+                      help="""
+Do not print any debug messages to console. (Any error messages will
+still be displayed.)
 """)
 
 _group = run_opts.add_mutually_exclusive_group()
@@ -398,7 +399,7 @@ class ArvPutUploadJob(object):
     }
 
     def __init__(self, paths, resume=True, use_cache=True, reporter=None,
-                 name=None, owner_uuid=None,
+                 name=None, owner_uuid=None, api_client=None,
                  ensure_unique_name=False, num_retries=None,
                  put_threads=None, replication_desired=None,
                  filename=None, update_time=60.0, update_collection=None,
@@ -421,6 +422,7 @@ class ArvPutUploadJob(object):
         self.replication_desired = replication_desired
         self.put_threads = put_threads
         self.filename = filename
+        self._api_client = api_client
         self._state_lock = threading.Lock()
         self._state = None # Previous run state (file list & manifest)
         self._current_files = [] # Current run file list
@@ -775,7 +777,8 @@ class ArvPutUploadJob(object):
         if update_collection and re.match(arvados.util.collection_uuid_pattern,
                                           update_collection):
             try:
-                self._remote_collection = arvados.collection.Collection(update_collection)
+                self._remote_collection = arvados.collection.Collection(
+                    update_collection, api_client=self._api_client)
             except arvados.errors.ApiError as error:
                 raise CollectionUpdateError("Cannot read collection {} ({})".format(update_collection, error))
             else:
@@ -822,7 +825,11 @@ class ArvPutUploadJob(object):
                 # No cache file, set empty state
                 self._state = copy.deepcopy(self.EMPTY_STATE)
             # Load the previous manifest so we can check if files were modified remotely.
-            self._local_collection = arvados.collection.Collection(self._state['manifest'], replication_desired=self.replication_desired, put_threads=self.put_threads)
+            self._local_collection = arvados.collection.Collection(
+                self._state['manifest'],
+                replication_desired=self.replication_desired,
+                put_threads=self.put_threads,
+                api_client=self._api_client)
 
     def collection_file_paths(self, col, path_prefix='.'):
         """Return a list of file paths by recursively go through the entire collection `col`"""
@@ -977,8 +984,12 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
     else:
         logger.setLevel(logging.INFO)
     status = 0
+
+    request_id = arvados.util.new_request_id()
+    logger.info('X-Request-Id: '+request_id)
+
     if api_client is None:
-        api_client = arvados.api('v1')
+        api_client = arvados.api('v1', request_id=request_id)
 
     # Determine the name to use
     if args.name:
@@ -1063,6 +1074,7 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
                                  use_cache = args.use_cache,
                                  filename = args.filename,
                                  reporter = reporter,
+                                 api_client = api_client,
                                  num_retries = args.retries,
                                  replication_desired = args.replication,
                                  put_threads = args.threads,
index e6e93f080659abf9a51cec9d4425cafe8bdc976b..351f7f5dda8a96ebb805fd4d4896380cb3addbb8 100644 (file)
@@ -291,7 +291,8 @@ class KeepClient(object):
 
         def __init__(self, root, user_agent_pool=queue.LifoQueue(),
                      upload_counter=None,
-                     download_counter=None, **headers):
+                     download_counter=None,
+                     headers={}):
             self.root = root
             self._user_agent_pool = user_agent_pool
             self._result = {'error': None}
@@ -920,7 +921,7 @@ class KeepClient(object):
         _logger.debug("{}: {}".format(locator, sorted_roots))
         return sorted_roots
 
-    def map_new_services(self, roots_map, locator, force_rebuild, need_writable, **headers):
+    def map_new_services(self, roots_map, locator, force_rebuild, need_writable, headers):
         # roots_map is a dictionary, mapping Keep service root strings
         # to KeepService objects.  Poll for Keep services, and add any
         # new ones to roots_map.  Return the current list of local
@@ -933,7 +934,7 @@ class KeepClient(object):
                     root, self._user_agent_pool,
                     upload_counter=self.upload_counter,
                     download_counter=self.download_counter,
-                    **headers)
+                    headers=headers)
         return local_roots
 
     @staticmethod
@@ -963,14 +964,14 @@ class KeepClient(object):
             return None
 
     @retry.retry_method
-    def head(self, loc_s, num_retries=None):
-        return self._get_or_head(loc_s, method="HEAD", num_retries=num_retries)
+    def head(self, loc_s, **kwargs):
+        return self._get_or_head(loc_s, method="HEAD", **kwargs)
 
     @retry.retry_method
-    def get(self, loc_s, num_retries=None):
-        return self._get_or_head(loc_s, method="GET", num_retries=num_retries)
+    def get(self, loc_s, **kwargs):
+        return self._get_or_head(loc_s, method="GET", **kwargs)
 
-    def _get_or_head(self, loc_s, method="GET", num_retries=None):
+    def _get_or_head(self, loc_s, method="GET", num_retries=None, request_id=None):
         """Get data from Keep.
 
         This method fetches one or more blocks of data from Keep.  It
@@ -1005,6 +1006,12 @@ class KeepClient(object):
 
         self.misses_counter.add(1)
 
+        headers = {
+            'X-Request-Id': (request_id or
+                             (hasattr(self, 'api_client') and self.api_client.request_id) or
+                             arvados.util.new_request_id()),
+        }
+
         # If the locator has hints specifying a prefix (indicating a
         # remote keepproxy) or the UUID of a local gateway service,
         # read data from the indicated service(s) instead of the usual
@@ -1021,7 +1028,8 @@ class KeepClient(object):
         roots_map = {
             root: self.KeepService(root, self._user_agent_pool,
                                    upload_counter=self.upload_counter,
-                                   download_counter=self.download_counter)
+                                   download_counter=self.download_counter,
+                                   headers=headers)
             for root in hint_roots
         }
 
@@ -1040,7 +1048,8 @@ class KeepClient(object):
                 sorted_roots = self.map_new_services(
                     roots_map, locator,
                     force_rebuild=(tries_left < num_retries),
-                    need_writable=False)
+                    need_writable=False,
+                    headers=headers)
             except Exception as error:
                 loop.save_result(error)
                 continue
@@ -1084,7 +1093,7 @@ class KeepClient(object):
                 "failed to read {}".format(loc_s), service_errors, label="service")
 
     @retry.retry_method
-    def put(self, data, copies=2, num_retries=None):
+    def put(self, data, copies=2, num_retries=None, request_id=None):
         """Save data in Keep.
 
         This method will get a list of Keep services from the API server, and
@@ -1114,9 +1123,12 @@ class KeepClient(object):
             return loc_s
         locator = KeepLocator(loc_s)
 
-        headers = {}
-        # Tell the proxy how many copies we want it to store
-        headers['X-Keep-Desired-Replicas'] = str(copies)
+        headers = {
+            'X-Request-Id': (request_id or
+                             (hasattr(self, 'api_client') and self.api_client.request_id) or
+                             arvados.util.new_request_id()),
+            'X-Keep-Desired-Replicas': str(copies),
+        }
         roots_map = {}
         loop = retry.RetryLoop(num_retries, self._check_loop_result,
                                backoff_start=2)
@@ -1125,7 +1137,9 @@ class KeepClient(object):
             try:
                 sorted_roots = self.map_new_services(
                     roots_map, locator,
-                    force_rebuild=(tries_left < num_retries), need_writable=True, **headers)
+                    force_rebuild=(tries_left < num_retries),
+                    need_writable=True,
+                    headers=headers)
             except Exception as error:
                 loop.save_result(error)
                 continue
index 1a973586051769e816103553e22326839a0c3670..66da2d12af2082689179bbf1b89dd6285c1edbd9 100644 (file)
@@ -2,10 +2,14 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
+from __future__ import division
+from builtins import range
+
 import fcntl
 import hashlib
 import httplib2
 import os
+import random
 import re
 import subprocess
 import errno
@@ -399,3 +403,16 @@ def ca_certs_path(fallback=httplib2.CA_CERTS):
         if os.path.exists(ca_certs_path):
             return ca_certs_path
     return fallback
+
+def new_request_id():
+    rid = "req-"
+    # 2**104 > 36**20 > 2**103
+    n = random.getrandbits(104)
+    for _ in range(20):
+        c = n % 36
+        if c < 10:
+            rid += chr(c+ord('0'))
+        else:
+            rid += chr(c+ord('a')-10)
+        n = n // 36
+    return rid
index b467f32a4e5bac4756cd82211c1565ede83365a5..8d3142ab6aa49980babae66e255d2f183224109e 100644 (file)
@@ -142,6 +142,33 @@ class RetryREST(unittest.TestCase):
         self.assertEqual(sleep.call_args_list,
                          [mock.call(RETRY_DELAY_INITIAL)])
 
+    @mock.patch('time.sleep')
+    def test_same_automatic_request_id_on_retry(self, sleep):
+        self.api._http.orig_http_request.side_effect = (
+            socket.error('mock error'),
+            self.request_success,
+        )
+        self.api.users().current().execute()
+        calls = self.api._http.orig_http_request.call_args_list
+        self.assertEqual(len(calls), 2)
+        self.assertEqual(
+            calls[0][1]['headers']['X-Request-Id'],
+            calls[1][1]['headers']['X-Request-Id'])
+        self.assertRegex(calls[0][1]['headers']['X-Request-Id'], r'^req-[a-z0-9]{20}$')
+
+    @mock.patch('time.sleep')
+    def test_provided_request_id_on_retry(self, sleep):
+        self.api.request_id='fake-request-id'
+        self.api._http.orig_http_request.side_effect = (
+            socket.error('mock error'),
+            self.request_success,
+        )
+        self.api.users().current().execute()
+        calls = self.api._http.orig_http_request.call_args_list
+        self.assertEqual(len(calls), 2)
+        for call in calls:
+            self.assertEqual(call[1]['headers']['X-Request-Id'], 'fake-request-id')
+
     @mock.patch('time.sleep')
     def test_socket_error_retry_delay(self, sleep):
         self.api._http.orig_http_request.side_effect = socket.error('mock')
index 5aa223a2eaf7fc1c444ff296641dbd5d344a228f..733cd6478c155131f9f29f4b3e7adedf6fa0508e 100644 (file)
@@ -5,6 +5,7 @@
 from __future__ import absolute_import
 from future.utils import listitems
 import io
+import logging
 import mock
 import os
 import re
@@ -30,7 +31,14 @@ class ArvadosGetTestCase(run_test_server.TestCaseWithServers,
         self.tempdir = tempfile.mkdtemp()
         self.col_loc, self.col_pdh, self.col_manifest = self.write_test_collection()
 
+        self.stdout = tutil.BytesIO()
+        self.stderr = tutil.StringIO()
+        self.loggingHandler = logging.StreamHandler(self.stderr)
+        self.loggingHandler.setFormatter(logging.Formatter('%(levelname)s: %(message)s'))
+        logging.getLogger().addHandler(self.loggingHandler)
+
     def tearDown(self):
+        logging.getLogger().removeHandler(self.loggingHandler)
         super(ArvadosGetTestCase, self).tearDown()
         shutil.rmtree(self.tempdir)
 
@@ -52,8 +60,10 @@ class ArvadosGetTestCase(run_test_server.TestCaseWithServers,
                 c.manifest_text(strip=strip_manifest))
 
     def run_get(self, args):
-        self.stdout = tutil.BytesIO()
-        self.stderr = tutil.StringIO()
+        self.stdout.seek(0, 0)
+        self.stdout.truncate(0)
+        self.stderr.seek(0, 0)
+        self.stderr.truncate(0)
         return arv_get.main(args, self.stdout, self.stderr)
 
     def test_version_argument(self):
@@ -184,3 +194,15 @@ class ArvadosGetTestCase(run_test_server.TestCaseWithServers,
         self.assertEqual(0, r)
         self.assertEqual(b'', stdout.getvalue())
         self.assertFalse(stderr.write.called)
+
+    request_id_regex = r'INFO: X-Request-Id: req-[a-z0-9]{20}\n'
+
+    def test_request_id_logging_on(self):
+        r = self.run_get(["-v", "{}/".format(self.col_loc), self.tempdir])
+        self.assertEqual(0, r)
+        self.assertRegex(self.stderr.getvalue(), self.request_id_regex)
+
+    def test_request_id_logging_off(self):
+        r = self.run_get(["{}/".format(self.col_loc), self.tempdir])
+        self.assertEqual(0, r)
+        self.assertNotRegex(self.stderr.getvalue(), self.request_id_regex)
index 346167846cd5ed185453ae85553a1676718e53d6..0c01619b4bd26798d8c74ba8bc4e584b626956de 100644 (file)
@@ -12,6 +12,7 @@ import apiclient
 import datetime
 import hashlib
 import json
+import logging
 import mock
 import os
 import pwd
@@ -581,8 +582,10 @@ class ArvadosPutTest(run_test_server.TestCaseWithServers,
     Z_UUID = 'zzzzz-zzzzz-zzzzzzzzzzzzzzz'
 
     def call_main_with_args(self, args):
-        self.main_stdout = tutil.StringIO()
-        self.main_stderr = tutil.StringIO()
+        self.main_stdout.seek(0, 0)
+        self.main_stdout.truncate(0)
+        self.main_stderr.seek(0, 0)
+        self.main_stderr.truncate(0)
         return arv_put.main(args, self.main_stdout, self.main_stderr)
 
     def call_main_on_test_file(self, args=[]):
@@ -598,8 +601,14 @@ class ArvadosPutTest(run_test_server.TestCaseWithServers,
         super(ArvadosPutTest, self).setUp()
         run_test_server.authorize_with('active')
         arv_put.api_client = None
+        self.main_stdout = tutil.StringIO()
+        self.main_stderr = tutil.StringIO()
+        self.loggingHandler = logging.StreamHandler(self.main_stderr)
+        self.loggingHandler.setFormatter(logging.Formatter('%(levelname)s: %(message)s'))
+        logging.getLogger().addHandler(self.loggingHandler)
 
     def tearDown(self):
+        logging.getLogger().removeHandler(self.loggingHandler)
         for outbuf in ['main_stdout', 'main_stderr']:
             if hasattr(self, outbuf):
                 getattr(self, outbuf).close()
@@ -697,6 +706,15 @@ class ArvadosPutTest(run_test_server.TestCaseWithServers,
             self.assertLess(0, coll_save_mock.call_count)
             self.assertEqual("", self.main_stdout.getvalue())
 
+    def test_request_id_logging(self):
+        matcher = r'INFO: X-Request-Id: req-[a-z0-9]{20}\n'
+
+        self.call_main_on_test_file()
+        self.assertRegex(self.main_stderr.getvalue(), matcher)
+
+        self.call_main_on_test_file(['--silent'])
+        self.assertNotRegex(self.main_stderr.getvalue(), matcher)
+
 
 class ArvPutIntegrationTest(run_test_server.TestCaseWithServers,
                             ArvadosBaseTestCase):
index 7f6f71348e2bce794b674ccf2394aada1027343f..35614346d5c3e73465fc9aeb9f45932eb17d59cb 100644 (file)
@@ -163,7 +163,7 @@ class WebsocketTest(run_test_server.TestCaseWithServers):
         """Convert minutes-east-of-UTC to RFC3339- and ISO-compatible time zone designator"""
         return '{:+03d}:{:02d}'.format(offset//60, offset%60)
 
-    # Test websocket reconnection on (un)execpted close
+    # Test websocket reconnection on (un)expected close
     def _test_websocket_reconnect(self, close_unexpected):
         run_test_server.authorize_with('active')
         events = queue.Queue(100)
index c2c47282537f7533b9f9325d871503f471c4eca2..e0bb734b21fbf2671c51a4ce22dd5c954432a488 100644 (file)
@@ -23,6 +23,7 @@ import urllib.parse
 
 import arvados
 import arvados.retry
+import arvados.util
 from . import arvados_testutil as tutil
 from . import keepstub
 from . import run_test_server
@@ -517,6 +518,77 @@ class KeepClientServiceTestCase(unittest.TestCase, tutil.ApiClientMock):
         self.assertEqual(1, req_mock.call_count)
 
 
+@tutil.skip_sleep
+class KeepXRequestIdTestCase(unittest.TestCase, tutil.ApiClientMock):
+    def setUp(self):
+        self.api_client = self.mock_keep_services(count=2)
+        self.keep_client = arvados.KeepClient(api_client=self.api_client)
+        self.data = b'xyzzy'
+        self.locator = '1271ed5ef305aadabc605b1609e24c52'
+        self.test_id = arvados.util.new_request_id()
+        self.assertRegex(self.test_id, r'^req-[a-z0-9]{20}$')
+        # If we don't set request_id to None explicitly here, it will
+        # return <MagicMock name='api_client_mock.request_id'
+        # id='123456789'>:
+        self.api_client.request_id = None
+
+    def test_default_to_api_client_request_id(self):
+        self.api_client.request_id = self.test_id
+        with tutil.mock_keep_responses(self.locator, 200, 200) as mock:
+            self.keep_client.put(self.data)
+        self.assertEqual(2, len(mock.responses))
+        for resp in mock.responses:
+            self.assertProvidedRequestId(resp)
+
+        with tutil.mock_keep_responses(self.data, 200) as mock:
+            self.keep_client.get(self.locator)
+        self.assertProvidedRequestId(mock.responses[0])
+
+        with tutil.mock_keep_responses(b'', 200) as mock:
+            self.keep_client.head(self.locator)
+        self.assertProvidedRequestId(mock.responses[0])
+
+    def test_explicit_request_id(self):
+        with tutil.mock_keep_responses(self.locator, 200, 200) as mock:
+            self.keep_client.put(self.data, request_id=self.test_id)
+        self.assertEqual(2, len(mock.responses))
+        for resp in mock.responses:
+            self.assertProvidedRequestId(resp)
+
+        with tutil.mock_keep_responses(self.data, 200) as mock:
+            self.keep_client.get(self.locator, request_id=self.test_id)
+        self.assertProvidedRequestId(mock.responses[0])
+
+        with tutil.mock_keep_responses(b'', 200) as mock:
+            self.keep_client.head(self.locator, request_id=self.test_id)
+        self.assertProvidedRequestId(mock.responses[0])
+
+    def test_automatic_request_id(self):
+        with tutil.mock_keep_responses(self.locator, 200, 200) as mock:
+            self.keep_client.put(self.data)
+        self.assertEqual(2, len(mock.responses))
+        for resp in mock.responses:
+            self.assertAutomaticRequestId(resp)
+
+        with tutil.mock_keep_responses(self.data, 200) as mock:
+            self.keep_client.get(self.locator)
+        self.assertAutomaticRequestId(mock.responses[0])
+
+        with tutil.mock_keep_responses(b'', 200) as mock:
+            self.keep_client.head(self.locator)
+        self.assertAutomaticRequestId(mock.responses[0])
+
+    def assertAutomaticRequestId(self, resp):
+        hdr = [x for x in resp.getopt(pycurl.HTTPHEADER)
+               if x.startswith('X-Request-Id: ')][0]
+        self.assertNotEqual(hdr, 'X-Request-Id: '+self.test_id)
+        self.assertRegex(hdr, r'^X-Request-Id: req-[a-z0-9]{20}$')
+
+    def assertProvidedRequestId(self, resp):
+        self.assertIn('X-Request-Id: '+self.test_id,
+                      resp.getopt(pycurl.HTTPHEADER))
+
+
 @tutil.skip_sleep
 class KeepClientRendezvousTestCase(unittest.TestCase, tutil.ApiClientMock):
 
index 3803d37691132782e927e6e32cbb5fbee80657c7..fb75007dc6738ab984d5de19bf347fb9b672c975 100644 (file)
@@ -28,7 +28,7 @@ class Arvados::V1::CollectionsController < ApplicationController
 
   def find_objects_for_index
     if params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name)
-      @objects = Collection.readable_by(*@read_users, {include_trash: true, query_on: Collection.unscoped})
+      @objects = Collection.readable_by(*@read_users, {include_trash: true})
     end
     super
   end
index 8e195ff990ac6c9fc80f09e5336703781c020e80..ec3b69ab052506b54798689d168fb136e0e33321 100644 (file)
@@ -163,12 +163,7 @@ class Arvados::V1::GroupsController < ApplicationController
         end
       end.compact
 
-      query_on = if klass == Collection and params[:include_trash]
-                   klass.unscoped
-                 else
-                   klass
-                 end
-      @objects = query_on.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
+      @objects = klass.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
                  order(request_order).where(where_conds)
 
       klass_limit = limit_all - all_objects.count
index 08d7e9345dbb8178d6891821277e6a01958aeacd..05deba7bc153b50f5af256dd7a3cc97f1e942454 100644 (file)
@@ -255,36 +255,44 @@ class ArvadosModel < ActiveRecord::Base
     # Collect the UUIDs of the authorized users.
     sql_table = kwargs.fetch(:table_name, table_name)
     include_trash = kwargs.fetch(:include_trash, false)
-    query_on = kwargs.fetch(:query_on, self)
 
     sql_conds = []
     user_uuids = users_list.map { |u| u.uuid }
 
+    exclude_trashed_records = if !include_trash and (sql_table == "groups" or sql_table == "collections") then
+                                # Only include records that are not explicitly trashed
+                                "AND #{sql_table}.is_trashed = false"
+                              else
+                                ""
+                              end
+
     if users_list.select { |u| u.is_admin }.any?
       if !include_trash
-        # exclude rows that are explicitly trashed.
         if sql_table != "api_client_authorizations"
-          sql_conds.push "NOT EXISTS(SELECT 1
-                  FROM #{PERMISSION_VIEW}
-                  WHERE trashed = 1 AND
-                  (#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"
+          # Exclude rows where the owner is trashed
+          sql_conds.push "NOT EXISTS(SELECT 1 "+
+                  "FROM #{PERMISSION_VIEW} "+
+                  "WHERE trashed = 1 AND "+
+                  "(#{sql_table}.owner_uuid = target_uuid)) "+
+                  exclude_trashed_records
         end
       end
     else
-      if include_trash
-        trashed_check = ""
-      else
-        trashed_check = "AND trashed = 0"
-      end
-
-      if sql_table != "api_client_authorizations" and sql_table != "groups"
-        owner_check = "OR (target_uuid = #{sql_table}.owner_uuid AND target_owner_uuid IS NOT NULL)"
-      else
-        owner_check = ""
-      end
+      trashed_check = if !include_trash then
+                        "AND trashed = 0"
+                      else
+                        ""
+                      end
+
+      owner_check = if sql_table != "api_client_authorizations" and sql_table != "groups" then
+                      "OR (target_uuid = #{sql_table}.owner_uuid AND target_owner_uuid IS NOT NULL)"
+                    else
+                      ""
+                    end
 
       sql_conds.push "EXISTS(SELECT 1 FROM #{PERMISSION_VIEW} "+
-                     "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 #{trashed_check} AND (target_uuid = #{sql_table}.uuid #{owner_check}))"
+                     "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 #{trashed_check} AND (target_uuid = #{sql_table}.uuid #{owner_check})) "+
+                     exclude_trashed_records
 
       if sql_table == "links"
         # Match any permission link that gives one of the authorized
@@ -295,7 +303,7 @@ class ArvadosModel < ActiveRecord::Base
       end
     end
 
-    query_on.where(sql_conds.join(' OR '),
+    self.where(sql_conds.join(' OR '),
                     user_uuids: user_uuids,
                     permission_link_classes: ['permission', 'resources'])
   end
@@ -734,7 +742,7 @@ class ArvadosModel < ActiveRecord::Base
     if self == ArvadosModel
       # If called directly as ArvadosModel.find_by_uuid rather than via subclass,
       # delegate to the appropriate subclass based on the given uuid.
-      self.resource_class_for_uuid(uuid).unscoped.find_by_uuid(uuid)
+      self.resource_class_for_uuid(uuid).find_by_uuid(uuid)
     else
       super
     end
index ce7e9fe5badf43735e9e84ce6e1bb6d22c492412..c5fd96eca4747d866795e54ee0931cc4e63ff4f3 100644 (file)
@@ -24,9 +24,6 @@ class Collection < ArvadosModel
   validate :ensure_pdh_matches_manifest_text
   before_save :set_file_names
 
-  # Query only untrashed collections by default.
-  default_scope { where("is_trashed = false") }
-
   api_accessible :user, extend: :common do |t|
     t.add :name
     t.add :description
index b3739da9cfde9bc00a8a0e6e135b914398814e30..edcb8501a4621aa71be9eef5d68c719aac49a267 100644 (file)
@@ -437,12 +437,10 @@ class Container < ArvadosModel
     # that a container cannot "claim" a collection that it doesn't otherwise
     # have access to just by setting the output field to the collection PDH.
     if output_changed?
-      c = Collection.unscoped do
-        Collection.
-            readable_by(current_user).
+      c = Collection.
+            readable_by(current_user, {include_trash: true}).
             where(portable_data_hash: self.output).
             first
-      end
       if !c
         errors.add :output, "collection must exist and be readable by current user."
       end
index 25becb24a62682fa6e0ae9fe115099e7e22fceba..3596bf3d67551352c7ba404b3605c7dabe15956b 100644 (file)
@@ -120,9 +120,7 @@ class ContainerRequest < ArvadosModel
           trash_at = db_current_time + self.output_ttl
         end
       end
-      manifest = Collection.unscoped do
-        Collection.where(portable_data_hash: pdh).first.manifest_text
-      end
+      manifest = Collection.where(portable_data_hash: pdh).first.manifest_text
 
       coll = Collection.new(owner_uuid: owner_uuid,
                             manifest_text: manifest,
index 84497a179dc06e24792a196cc053261eeaa31786..a899191db014286c40ea2a5bca5373b3768e3191 100644 (file)
@@ -9,10 +9,10 @@ module SweepTrashedCollections
 
   def self.sweep_now
     act_as_system_user do
-      Collection.unscoped.
+      Collection.
         where('delete_at is not null and delete_at < statement_timestamp()').
         destroy_all
-      Collection.unscoped.
+      Collection.
         where('is_trashed = false and trash_at < statement_timestamp()').
         update_all('is_trashed = true')
     end
index 836f840aea3d6a1f3686efa12f6b9feecd330a11..85e40ffe34d10f26198c3bab6523e234e7360eec 100644 (file)
@@ -743,6 +743,28 @@ uncommitted-with-required-and-optional-inputs:
     ram: 256000000
     API: true
 
+cr_in_trashed_project:
+  uuid: zzzzz-xvhdp-cr5trashedcontr
+  owner_uuid: zzzzz-j7d0g-trashedproject1
+  name: completed container request
+  state: Final
+  priority: 1
+  created_at: <%= 2.minute.ago.to_s(:db) %>
+  updated_at: <%= 1.minute.ago.to_s(:db) %>
+  modified_at: <%= 1.minute.ago.to_s(:db) %>
+  modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  container_image: test
+  cwd: test
+  output_path: test
+  command: ["echo", "hello"]
+  container_uuid: zzzzz-dz642-compltcontainer
+  log_uuid: zzzzz-4zz18-y9vne9npefyxh8g
+  output_uuid: zzzzz-4zz18-znfnqtbbv4spc3w
+  runtime_constraints:
+    vcpus: 1
+    ram: 123
+
+
 # Test Helper trims the rest of the file
 
 # Do not add your fixtures below this line as the rest of this file will be trimmed by test_helper
index 47f0887bd19ce5f4446088fa2290afda7fca8e40..e6ecea219b9da1ea4c71fee07dddf025aa78aab3 100644 (file)
@@ -991,7 +991,7 @@ EOS
       id: uuid,
     }
     assert_response 200
-    c = Collection.unscoped.find_by_uuid(uuid)
+    c = Collection.find_by_uuid(uuid)
     assert_operator c.trash_at, :<, db_current_time
     assert_equal c.delete_at, c.trash_at + Rails.configuration.blob_signature_ttl
   end
@@ -1003,7 +1003,7 @@ EOS
       id: uuid,
     }
     assert_response 200
-    c = Collection.unscoped.find_by_uuid(uuid)
+    c = Collection.find_by_uuid(uuid)
     assert_operator c.trash_at, :<, db_current_time
     assert_operator c.delete_at, :<, db_current_time
   end
@@ -1023,7 +1023,7 @@ EOS
         id: uuid,
       }
       assert_response 200
-      c = Collection.unscoped.find_by_uuid(uuid)
+      c = Collection.find_by_uuid(uuid)
       assert_operator c.trash_at, :<, db_current_time
       assert_operator c.delete_at, :>=, time_before_trashing + Rails.configuration.default_trash_lifetime
     end
index ba8f1e520e445142ee40218c49a52fdd6871385f..62e3755a3fb8793d3b05c2766fbc5c5705245315 100644 (file)
@@ -352,9 +352,12 @@ class CollectionTest < ActiveSupport::TestCase
       assert c.valid?
       uuid = c.uuid
 
+      c = Collection.readable_by(current_user).where(uuid: uuid)
+      assert_not_empty c, 'Should be able to find live collection'
+
       # mark collection as expired
-      c.update_attributes!(trash_at: Time.new.strftime("%Y-%m-%d"))
-      c = Collection.where(uuid: uuid)
+      c.first.update_attributes!(trash_at: Time.new.strftime("%Y-%m-%d"))
+      c = Collection.readable_by(current_user).where(uuid: uuid)
       assert_empty c, 'Should not be able to find expired collection'
 
       # recreate collection with the same name
@@ -419,7 +422,7 @@ class CollectionTest < ActiveSupport::TestCase
         if fixture_name == :expired_collection
           # Fixture-finder shorthand doesn't find trashed collections
           # because they're not in the default scope.
-          c = Collection.unscoped.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3ih')
+          c = Collection.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3ih')
         else
           c = collections(fixture_name)
         end
@@ -488,7 +491,7 @@ class CollectionTest < ActiveSupport::TestCase
       end
     end
     SweepTrashedCollections.sweep_now
-    c = Collection.unscoped.where('uuid=? and is_trashed=true', c.uuid).first
+    c = Collection.where('uuid=? and is_trashed=true', c.uuid).first
     assert c
     act_as_user users(:active) do
       assert Collection.create!(owner_uuid: c.owner_uuid,
@@ -498,9 +501,9 @@ class CollectionTest < ActiveSupport::TestCase
 
   test "delete in SweepTrashedCollections" do
     uuid = 'zzzzz-4zz18-3u1p5umicfpqszp' # deleted_on_next_sweep
-    assert_not_empty Collection.unscoped.where(uuid: uuid)
+    assert_not_empty Collection.where(uuid: uuid)
     SweepTrashedCollections.sweep_now
-    assert_empty Collection.unscoped.where(uuid: uuid)
+    assert_empty Collection.where(uuid: uuid)
   end
 
   test "delete referring links in SweepTrashedCollections" do
@@ -512,10 +515,10 @@ class CollectionTest < ActiveSupport::TestCase
                    name: 'something')
     end
     past = db_current_time
-    Collection.unscoped.where(uuid: uuid).
+    Collection.where(uuid: uuid).
       update_all(is_trashed: true, trash_at: past, delete_at: past)
-    assert_not_empty Collection.unscoped.where(uuid: uuid)
+    assert_not_empty Collection.where(uuid: uuid)
     SweepTrashedCollections.sweep_now
-    assert_empty Collection.unscoped.where(uuid: uuid)
+    assert_empty Collection.where(uuid: uuid)
   end
 end
index eb4f35fea3d293c922d07fc6d16981f81b5092ee..3e2b8c1d3a550e16cb5da3be907e75a4383d37d4 100644 (file)
@@ -596,7 +596,7 @@ class ContainerTest < ActiveSupport::TestCase
     c.lock
     c.update_attributes! state: Container::Running
 
-    output = Collection.unscoped.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3jk')
+    output = Collection.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3jk')
 
     assert output.is_trashed
     assert c.update_attributes output: output.portable_data_hash
@@ -609,7 +609,7 @@ class ContainerTest < ActiveSupport::TestCase
     c.lock
     c.update_attributes! state: Container::Running
 
-    output = Collection.unscoped.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3jr')
+    output = Collection.find_by_uuid('zzzzz-4zz18-mto52zx1s7sn3jr')
 
     Thread.current[:api_client_authorization] = ApiClientAuthorization.find_by_uuid(c.auth_uuid)
     Thread.current[:user] = User.find_by_id(Thread.current[:api_client_authorization].user_id)
index 3a398a5c88d2dab37a500e17fbc58a6f78076cbe..93f50c13ed0625b09da07152f0a5e228de2c7564 100644 (file)
@@ -53,4 +53,17 @@ transitions = {
  ('unpaired', 'open', 'boot exceeded', 'not idle'): "START_SHUTDOWN",
  ('unpaired', 'open', 'boot wait', 'idle exceeded'): None,
  ('unpaired', 'open', 'boot wait', 'idle wait'): None,
- ('unpaired', 'open', 'boot wait', 'not idle'): None}
+ ('unpaired', 'open', 'boot wait', 'not idle'): None,
+
+ ('fail', 'closed', 'boot exceeded', 'idle exceeded'): "START_SHUTDOWN",
+ ('fail', 'closed', 'boot exceeded', 'idle wait'): "START_SHUTDOWN",
+ ('fail', 'closed', 'boot exceeded', 'not idle'): "START_SHUTDOWN",
+ ('fail', 'closed', 'boot wait', 'idle exceeded'): "START_SHUTDOWN",
+ ('fail', 'closed', 'boot wait', 'idle wait'): "START_SHUTDOWN",
+ ('fail', 'closed', 'boot wait', 'not idle'): "START_SHUTDOWN",
+ ('fail', 'open', 'boot exceeded', 'idle exceeded'): "START_SHUTDOWN",
+ ('fail', 'open', 'boot exceeded', 'idle wait'): "START_SHUTDOWN",
+ ('fail', 'open', 'boot exceeded', 'not idle'): "START_SHUTDOWN",
+ ('fail', 'open', 'boot wait', 'idle exceeded'): "START_SHUTDOWN",
+ ('fail', 'open', 'boot wait', 'idle wait'): "START_SHUTDOWN",
+ ('fail', 'open', 'boot wait', 'not idle'): "START_SHUTDOWN"}
index ca3029d9e1bc3c376b119cca367b3767f3a8bb45..dd441edb6b70f1df4c8144e818bdd0501c443ffc 100644 (file)
@@ -280,6 +280,7 @@ class NodeManagerDaemonActor(actor_class):
             "unpaired": 0,
             "busy": 0,
             "idle": 0,
+            "fail": 0,
             "down": 0,
             "shutdown": 0
         }
@@ -321,7 +322,7 @@ class NodeManagerDaemonActor(actor_class):
                           counts["unpaired"],
                           counts["idle"],
                           busy_count,
-                          counts["down"],
+                          counts["down"]+counts["fail"],
                           counts["shutdown"])
 
         if over_max >= 0:
@@ -482,7 +483,7 @@ class NodeManagerDaemonActor(actor_class):
                 # grace period without a ping, so shut it down so we can boot a new
                 # node in its place.
                 self._begin_node_shutdown(node_actor, cancellable=False)
-            elif node_actor.in_state('down').get():
+            elif node_actor.in_state('down', 'fail').get():
                 # Node is down and unlikely to come back.
                 self._begin_node_shutdown(node_actor, cancellable=False)
         except pykka.ActorDeadError as e:
index e06ec83b6238ac2304e0d93db2a23b2653739553..70ad54d789cff1e34e4f39beb759939b7b2bdf3d 100644 (file)
@@ -39,8 +39,8 @@ class ArvadosNodeListMonitorActor(clientactor.RemotePollLoopActor):
                              'mix',   'mix*',
                              'drng',  'drng*'):
                     nodestates[nodename] = 'busy'
-                elif state == 'idle':
-                    nodestates[nodename] = 'idle'
+                elif state in ('idle', 'fail'):
+                    nodestates[nodename] = state
                 else:
                     nodestates[nodename] = 'down'
             except ValueError:
index e4037d11a1a90e4f1c5edef811ff1697851afa56..4b352059e629bbc22c667347812d7046960c0da7 100644 (file)
@@ -444,6 +444,13 @@ class ComputeNodeMonitorActorTestCase(testutil.ActorTestMixin,
         self.assertEquals((True, "node state is ('idle', 'open', 'boot wait', 'idle exceeded')"),
                           self.node_actor.shutdown_eligible().get(self.TIMEOUT))
 
+    def test_shutdown_when_node_state_fail(self):
+        self.make_actor(5, testutil.arvados_node_mock(
+            5, crunch_worker_state='fail'))
+        self.shutdowns._set_state(True, 600)
+        self.assertEquals((True, "node state is ('fail', 'open', 'boot wait', 'idle exceeded')"),
+                          self.node_actor.shutdown_eligible().get(self.TIMEOUT))
+
     def test_no_shutdown_when_node_state_stale(self):
         self.make_actor(6, testutil.arvados_node_mock(6, age=90000))
         self.shutdowns._set_state(True, 600)