From a108e6440f6cd3fc484db494885ca391f5475420 Mon Sep 17 00:00:00 2001 From: Felix Edel Date: Thu, 4 Jun 2020 13:18:57 +0200 Subject: [PATCH] Return upload_results in upload-logs-swift role We are facing some issues where the log upload to swift fails, but the role is always succeeding. To get some more information about the upload failures, we let the upload() method return those to the Ansible module and provide them in the module's JSON result. The equivalent change in the test-upload-logs-swift [1] role is validated in [2]. [1] https://review.opendev.org/#/c/735503/1 [2] https://review.opendev.org/#/c/737441/ Change-Id: Ie0d4ea2f3365600eae0e572e4c0790b131d3b13e --- .../library/test_zuul_swift_upload.py | 61 +++++++++++++- .../library/zuul_swift_upload.py | 82 ++++++++++++------- roles/upload-logs-swift/tasks/main.yaml | 16 ++-- 3 files changed, 123 insertions(+), 36 deletions(-) diff --git a/roles/upload-logs-swift/library/test_zuul_swift_upload.py b/roles/upload-logs-swift/library/test_zuul_swift_upload.py index f7978b42d..cf088987a 100644 --- a/roles/upload-logs-swift/library/test_zuul_swift_upload.py +++ b/roles/upload-logs-swift/library/test_zuul_swift_upload.py @@ -24,9 +24,14 @@ import testtools import time import stat import fixtures +try: + from unittest import mock +except ImportError: + import mock +import requests from bs4 import BeautifulSoup -from .zuul_swift_upload import FileList, Indexer, FileDetail +from .zuul_swift_upload import FileList, Indexer, FileDetail, Uploader FIXTURE_DIR = os.path.join(os.path.dirname(__file__), @@ -411,3 +416,57 @@ class TestFileDetail(testtools.TestCase): self.assertEqual(time.gmtime(0), file_detail.last_modified) self.assertEqual(0, file_detail.size) + + +class MockUploader(Uploader): + """An uploader that uses a mocked cloud object""" + def __init__(self, container): + self.container = container + self.cloud = mock.Mock() + self.dry_run = False + self.public = True + self.delete_after = None + self.prefix = "" + self.url = 'http://dry-run-url.com/a/path/' + + +class TestUpload(testtools.TestCase): + + def test_upload_result(self): + uploader = MockUploader(container="container") + + # Let the upload method fail for the job-output.json, but not for the + # inventory.yaml file + def side_effect(container, name, **ignored): + if name == "job-output.json": + raise requests.exceptions.RequestException( + "Failed for a reason" + ) + uploader.cloud.create_object = mock.Mock( + side_effect=side_effect + ) + + # Get some test files to upload + files = [ + FileDetail( + os.path.join(FIXTURE_DIR, "logs/job-output.json"), + "job-output.json", + ), + FileDetail( + os.path.join(FIXTURE_DIR, "logs/zuul-info/inventory.yaml"), + "inventory.yaml", + ), + ] + + expected_failures = [ + { + "file": "job-output.json", + "error": ( + "Error posting file after multiple attempts: " + "Failed for a reason" + ), + }, + ] + + failures = uploader.upload(files) + self.assertEqual(expected_failures, failures) diff --git a/roles/upload-logs-swift/library/zuul_swift_upload.py b/roles/upload-logs-swift/library/zuul_swift_upload.py index 485f4fa47..51dcc43ee 100755 --- a/roles/upload-logs-swift/library/zuul_swift_upload.py +++ b/roles/upload-logs-swift/library/zuul_swift_upload.py @@ -682,20 +682,27 @@ class Uploader(): num_threads = min(len(file_list), MAX_UPLOAD_THREADS) threads = [] + # Keep track on upload failures + failures = [] + queue = queuelib.Queue() # add items to queue for f in file_list: queue.put(f) for x in range(num_threads): - t = threading.Thread(target=self.post_thread, args=(queue,)) + t = threading.Thread( + target=self.post_thread, args=(queue, failures) + ) threads.append(t) t.start() for t in threads: t.join() - def post_thread(self, queue): + return failures + + def post_thread(self, queue, failures): while True: try: file_detail = queue.get_nowait() @@ -703,13 +710,23 @@ class Uploader(): threading.current_thread(), file_detail) retry_function(lambda: self._post_file(file_detail)) - except requests.exceptions.RequestException: + except requests.exceptions.RequestException as e: + msg = "Error posting file after multiple attempts" # Do our best to attempt to upload all the files - logging.exception("Error posting file after multiple attempts") + logging.exception(msg) + failures.append({ + "file": file_detail.filename, + "error": "{}: {}".format(msg, e) + }) continue - except IOError: + except IOError as e: + msg = "Error opening file" # Do our best to attempt to upload all the files - logging.exception("Error opening file") + logging.exception(msg) + failures.append({ + "file": file_detail.filename, + "error": "{}: {}".format(msg, e) + }) continue except queuelib.Empty: # No more work to do @@ -801,8 +818,8 @@ def run(cloud, container, files, # Upload. uploader = Uploader(cloud, container, prefix, delete_after, public, dry_run) - uploader.upload(file_list) - return uploader.url + upload_failures = uploader.upload(file_list) + return uploader.url, upload_failures def ansible_main(): @@ -825,15 +842,17 @@ def ansible_main(): p = module.params cloud = get_cloud(p.get('cloud')) try: - url = run(cloud, p.get('container'), p.get('files'), - indexes=p.get('indexes'), - parent_links=p.get('parent_links'), - topdir_parent_link=p.get('topdir_parent_link'), - partition=p.get('partition'), - footer=p.get('footer'), - delete_after=p.get('delete_after', 15552000), - prefix=p.get('prefix'), - public=p.get('public')) + url, upload_failures = run( + cloud, p.get('container'), p.get('files'), + indexes=p.get('indexes'), + parent_links=p.get('parent_links'), + topdir_parent_link=p.get('topdir_parent_link'), + partition=p.get('partition'), + footer=p.get('footer'), + delete_after=p.get('delete_after', 15552000), + prefix=p.get('prefix'), + public=p.get('public') + ) except (keystoneauth1.exceptions.http.HttpError, requests.exceptions.RequestException): s = "Error uploading to %s.%s" % (cloud.name, cloud.config.region_name) @@ -844,8 +863,11 @@ def ansible_main(): msg=s, cloud=cloud.name, region_name=cloud.config.region_name) - module.exit_json(changed=True, - url=url) + module.exit_json( + changed=True, + url=url, + upload_failures=upload_failures, + ) def cli_main(): @@ -903,16 +925,18 @@ def cli_main(): if append_footer.lower() == 'none': append_footer = None - url = run(get_cloud(args.cloud), args.container, args.files, - indexes=not args.no_indexes, - parent_links=not args.no_parent_links, - topdir_parent_link=args.create_topdir_parent_link, - partition=args.partition, - footer=append_footer, - delete_after=args.delete_after, - prefix=args.prefix, - public=not args.no_public, - dry_run=args.dry_run) + url, _ = run( + get_cloud(args.cloud), args.container, args.files, + indexes=not args.no_indexes, + parent_links=not args.no_parent_links, + topdir_parent_link=args.create_topdir_parent_link, + partition=args.partition, + footer=append_footer, + delete_after=args.delete_after, + prefix=args.prefix, + public=not args.no_public, + dry_run=args.dry_run + ) print(url) diff --git a/roles/upload-logs-swift/tasks/main.yaml b/roles/upload-logs-swift/tasks/main.yaml index eba0d2a3a..2497ec41d 100644 --- a/roles/upload-logs-swift/tasks/main.yaml +++ b/roles/upload-logs-swift/tasks/main.yaml @@ -30,10 +30,14 @@ delete_after: "{{ zuul_log_delete_after | default(omit) }}" register: upload_results -- name: Return log URL to Zuul - delegate_to: localhost - zuul_return: - data: - zuul: - log_url: "{{ upload_results.url }}/" +- block: + - name: Return log URL to Zuul + delegate_to: localhost + zuul_return: + data: + zuul: + log_url: "{{ upload_results.url }}/" + - name: Print upload failures + debug: + var: upload_results.upload_failures when: upload_results is defined