From 8ac26f2a73322794508b3e6beceacae5c87bf3b5 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 3 May 2022 16:20:40 -0600 Subject: [PATCH 1/5] fix bug in url_helper --- cloudinit/url_helper.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index ebb6bc4a0ef..590073e198d 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -413,6 +413,7 @@ def dual_stack( return_exception = future.exception() if return_exception: last_exception = return_exception + exceptions.append(last_exception) else: return_result = future.result() if return_result: @@ -428,7 +429,7 @@ def dual_stack( LOG.warning( "Exception(s) %s during request to %s, " "raising last exception", - " ".join(exceptions), + " ".join([str(e) for e in exceptions]), returned_address, ) raise last_exception From 1b779207d35a067c0df2e99e831dd9205dbf7587 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 10 May 2022 20:08:56 -0600 Subject: [PATCH 2/5] add tests to verify warnings are emitted --- cloudinit/url_helper.py | 6 +- tests/unittests/test_url_helper.py | 136 ++++++++++++++++++++--------- 2 files changed, 99 insertions(+), 43 deletions(-) diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index 590073e198d..332474acde7 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -427,10 +427,8 @@ def dual_stack( # debugging if last_exception: LOG.warning( - "Exception(s) %s during request to %s, " - "raising last exception", - " ".join([str(e) for e in exceptions]), - returned_address, + f"Exception(s) {exceptions} during request to " + f"{returned_address}, raising last exception", ) raise last_exception else: diff --git a/tests/unittests/test_url_helper.py b/tests/unittests/test_url_helper.py index 5e09221938c..5a1be562a38 100644 --- a/tests/unittests/test_url_helper.py +++ b/tests/unittests/test_url_helper.py @@ -282,43 +282,18 @@ class TestDualStack: """ @pytest.mark.parametrize( - "func," - "addresses," - "stagger_delay," - "timeout," - "expected_val," - "expected_exc", + "func," "addresses," "stagger_delay," "timeout," "expected_val,", [ # Assert order based on timeout - (lambda x, _: x, ("one", "two"), 1, 1, "one", None), + (lambda x, _: x, ("one", "two"), 1, 1, "one"), # Assert timeout results in (None, None) - (lambda _a, _b: event.wait(1), ("one", "two"), 1, 0, None, None), - # Assert that exception in func is raised if all threads - # raise exception - # currently if all threads experience exception - # dual_stack() logs an error containing all exceptions - # but only raises the last exception to occur - ( - lambda _a, _b: 1 / 0, - ("one", "two"), - 0, - 1, - None, - ZeroDivisionError, - ), - # Verify "best effort behavior" - # dual_stack will temporarily ignore an exception in any of the - # request threads in hopes that a later thread will succeed - # this behavior is intended to allow a requests.ConnectionError - # exception from on endpoint to occur without preventing another - # thread from succeeding + (lambda _a, _b: event.wait(1), ("one", "two"), 1, 0, None), ( lambda a, _b: 1 / 0 if a == "one" else a, ("one", "two"), 0, 1, "two", - None, ), # Assert that exception in func is only raised # if neither thread gets a valid result @@ -328,7 +303,6 @@ class TestDualStack: 0, 1, "one", - None, ), # simulate a slow response to verify correct order ( @@ -337,7 +311,6 @@ class TestDualStack: 0, 1, "two", - None, ), # simulate a slow response to verify correct order ( @@ -346,7 +319,6 @@ class TestDualStack: 0, 1, "tri", - None, ), ], ) @@ -357,7 +329,6 @@ def test_dual_stack( stagger_delay, timeout, expected_val, - expected_exc, ): """Assert various failure modes behave as expected""" event.clear() @@ -369,13 +340,100 @@ def test_dual_stack( stagger_delay=stagger_delay, timeout=timeout, ) - if expected_exc: - with pytest.raises(expected_exc): - _, result = assert_time(gen) - assert expected_val == result - else: - _, result = assert_time(gen) - assert expected_val == result + _, result = assert_time(gen) + assert expected_val == result + + event.set() + + @pytest.mark.parametrize( + "func," + "addresses," + "stagger_delay," + "timeout," + "message," + "expected_exc", + [ + ( + lambda _a, _b: 1 / 0, + ("¯\\_(ツ)_/¯", "(╯°□°）╯︵ ┻━┻"), + 0, + 1, + "Exception(s) [ZeroDivisionError('division by zero'), " + "ZeroDivisionError('division by zero')] during request", + ZeroDivisionError, + ), + ( + lambda _a, _b: 1 / 0, + ("it", "really", "doesn't"), + 0, + 1, + "Exception(s) [ZeroDivisionError('division by zero'), " + "ZeroDivisionError('division by zero'), " + "ZeroDivisionError('division by zero')] during request", + ZeroDivisionError, + ), + ( + lambda _a, _b: [][0], + ("matter", "these"), + 0, + 1, + "Exception(s) [IndexError('list index out of range'), " + "IndexError('list index out of range')] during request", + IndexError, + ), + ( + lambda _a, _b: (_ for _ in ()).throw( + Exception('soap stone is not effective soap') + ), + ("are", "ignored"), + 0, + 1, + "Exception(s) [Exception('soap stone is not effective soap'), " + "Exception('soap stone is not effective soap')] during request", + Exception, + ), + ], + ) + def test_dual_stack_exceptions( + self, + func, + addresses, + stagger_delay, + timeout, + message, + expected_exc, + caplog, + ): + # Context: + # + # currently if all threads experience exception + # dual_stack() logs an error containing all exceptions + # but only raises the last exception to occur + # Verify "best effort behavior" + # dual_stack will temporarily ignore an exception in any of the + # request threads in hopes that a later thread will succeed + # this behavior is intended to allow a requests.ConnectionError + # exception from on endpoint to occur without preventing another + # thread from succeeding + event.clear() + + gen = partial( + dual_stack, + func, + addresses, + stagger_delay=stagger_delay, + timeout=timeout, + ) + with pytest.raises(expected_exc): + gen() # 1 + with caplog.at_level(logging.DEBUG): + try: + gen() # 2 + except expected_exc: + pass + finally: + assert 2 == len(caplog.records) + assert 2 == caplog.text.count(message) event.set() def test_dual_stack_staggered(self): From db94e90897e4bac2878a594266eb594661d642ca Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 11 May 2022 08:12:22 -0600 Subject: [PATCH 3/5] fmt --- cloudinit/url_helper.py | 6 ++++-- tests/unittests/test_url_helper.py | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index 332474acde7..04643895e76 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -427,8 +427,10 @@ def dual_stack( # debugging if last_exception: LOG.warning( - f"Exception(s) {exceptions} during request to " - f"{returned_address}, raising last exception", + "Exception(s) %s during request to " + "%s, raising last exception", + exceptions, + returned_address, ) raise last_exception else: diff --git a/tests/unittests/test_url_helper.py b/tests/unittests/test_url_helper.py index 5a1be562a38..6877aefc5c3 100644 --- a/tests/unittests/test_url_helper.py +++ b/tests/unittests/test_url_helper.py @@ -383,13 +383,13 @@ def test_dual_stack( ), ( lambda _a, _b: (_ for _ in ()).throw( - Exception('soap stone is not effective soap') + Exception("soapstone is not effective soap") ), ("are", "ignored"), 0, 1, - "Exception(s) [Exception('soap stone is not effective soap'), " - "Exception('soap stone is not effective soap')] during request", + "Exception(s) [Exception('soapstone is not effective soap'), " + "Exception('soapstone is not effective soap')] during request", Exception, ), ], From 8ec73deeaada55d39e2dff28f5f3e9b042ce78f6 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 11 May 2022 11:49:45 -0600 Subject: [PATCH 4/5] 3.6 workaround --- tests/unittests/test_url_helper.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/tests/unittests/test_url_helper.py b/tests/unittests/test_url_helper.py index 6877aefc5c3..94416c06645 100644 --- a/tests/unittests/test_url_helper.py +++ b/tests/unittests/test_url_helper.py @@ -358,8 +358,7 @@ def test_dual_stack( ("¯\\_(ツ)_/¯", "(╯°□°）╯︵ ┻━┻"), 0, 1, - "Exception(s) [ZeroDivisionError('division by zero'), " - "ZeroDivisionError('division by zero')] during request", + "division by zero", ZeroDivisionError, ), ( @@ -367,9 +366,7 @@ def test_dual_stack( ("it", "really", "doesn't"), 0, 1, - "Exception(s) [ZeroDivisionError('division by zero'), " - "ZeroDivisionError('division by zero'), " - "ZeroDivisionError('division by zero')] during request", + "division by zero", ZeroDivisionError, ), ( @@ -377,8 +374,7 @@ def test_dual_stack( ("matter", "these"), 0, 1, - "Exception(s) [IndexError('list index out of range'), " - "IndexError('list index out of range')] during request", + "list index out of range", IndexError, ), ( @@ -388,8 +384,7 @@ def test_dual_stack( ("are", "ignored"), 0, 1, - "Exception(s) [Exception('soapstone is not effective soap'), " - "Exception('soapstone is not effective soap')] during request", + "soapstone is not effective soap", Exception, ), ], @@ -417,6 +412,14 @@ def test_dual_stack_exceptions( # thread from succeeding event.clear() + # Note: python3.6 repr(Exception("test")) produces different output + # than later versions, so we cannot match exact message without + # some ugly manual exception repr() function, which I'd rather not do + # in dual_stack(), so we recreate expected messages manually here + # in a version-independant way for testing, the extra comma on old + # versions won't hurt anything + exc_list = str([expected_exc(message) for _ in addresses]) + expected_msg = f'Exception(s) {exc_list} during request' gen = partial( dual_stack, func, @@ -433,7 +436,7 @@ def test_dual_stack_exceptions( pass finally: assert 2 == len(caplog.records) - assert 2 == caplog.text.count(message) + assert 2 == caplog.text.count(expected_msg) event.set() def test_dual_stack_staggered(self): From 9a95590d11a3d6606a5a92e458a5de8e8b37bbde Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 11 May 2022 12:17:40 -0600 Subject: [PATCH 5/5] black --- tests/unittests/test_url_helper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unittests/test_url_helper.py b/tests/unittests/test_url_helper.py index 94416c06645..a9b9a85fbaf 100644 --- a/tests/unittests/test_url_helper.py +++ b/tests/unittests/test_url_helper.py @@ -419,7 +419,7 @@ def test_dual_stack_exceptions( # in a version-independant way for testing, the extra comma on old # versions won't hurt anything exc_list = str([expected_exc(message) for _ in addresses]) - expected_msg = f'Exception(s) {exc_list} during request' + expected_msg = f"Exception(s) {exc_list} during request" gen = partial( dual_stack, func,