Merge branch '12167-python-request-id'
authorTom Clegg <tclegg@veritasgenetics.com>
Mon, 11 Dec 2017 21:16:13 +0000 (16:16 -0500)
committerTom Clegg <tclegg@veritasgenetics.com>
Mon, 11 Dec 2017 21:16:13 +0000 (16:16 -0500)
refs #12167

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

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

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):