From a108e6440f6cd3fc484db494885ca391f5475420 Mon Sep 17 00:00:00 2001
From: Felix Edel <felix.edel@bmw.de>
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