From 9d64f1c1edf66e907c0e83f8b38a2e31b970d429 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 6 Apr 2023 14:01:43 +0200 Subject: [PATCH 1/3] Refactor shutdown test into more helper functions Improve code readability by splitting the test into more functions. Some could be re-used later on for more general-purpose subprocess handling or named checks. --- bin/tests/system/shutdown/tests_shutdown.py | 70 ++++++++++----------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/bin/tests/system/shutdown/tests_shutdown.py b/bin/tests/system/shutdown/tests_shutdown.py index d3aea399af..8a099067d1 100755 --- a/bin/tests/system/shutdown/tests_shutdown.py +++ b/bin/tests/system/shutdown/tests_shutdown.py @@ -131,6 +131,31 @@ def do_work(named_proc, resolver, rndc_cmd, kill_method, n_workers, n_queries): assert ret_code == 0 +def wait_for_named_loaded(resolver, retries=10): + for _ in range(retries): + try: + resolver.resolve("version.bind", "TXT", "CH") + return True + except (dns.resolver.NoNameservers, dns.exception.Timeout): + time.sleep(1) + return False + + +def wait_for_proc_termination(proc, max_timeout=10): + for _ in range(max_timeout): + if proc.poll() is not None: + return True + time.sleep(1) + + proc.send_signal(signal.SIGABRT) + for _ in range(max_timeout): + if proc.poll() is not None: + return True + time.sleep(1) + + return False + + def test_named_shutdown(named_port, control_port): # pylint: disable-msg=too-many-locals cfg_dir = os.path.join(os.getcwd(), "resolver") @@ -164,40 +189,15 @@ def test_named_shutdown(named_port, control_port): for kill_method in ("rndc", "sigterm"): named_cmdline = [named, "-c", cfg_file, "-f"] with subprocess.Popen(named_cmdline, cwd=cfg_dir) as named_proc: - # Ensure named is running - assert named_proc.poll() is None - # wait for named to finish loading - for _ in range(10): - try: - resolver.resolve("version.bind", "TXT", "CH") - break - except (dns.resolver.NoNameservers, dns.exception.Timeout): - time.sleep(1) - + assert named_proc.poll() is None, "named isn't running" + assert wait_for_named_loaded(resolver) do_work( - named_proc, resolver, rndc_cmd, kill_method, n_workers=12, n_queries=16 + named_proc, + resolver, + rndc_cmd, + kill_method, + n_workers=12, + n_queries=16, ) - - # Wait named to exit for a maximum of MAX_TIMEOUT seconds. - MAX_TIMEOUT = 10 - is_dead = False - for _ in range(MAX_TIMEOUT): - if named_proc.poll() is not None: - is_dead = True - break - time.sleep(1) - - if not is_dead: - named_proc.send_signal(signal.SIGABRT) - for _ in range(MAX_TIMEOUT): - if named_proc.poll() is not None: - is_dead = True - break - time.sleep(1) - if not is_dead: - named_proc.kill() - - assert is_dead - # Ensures that named exited gracefully. - # If it crashed (abort()) exitcode will be non zero. - assert named_proc.returncode == 0 + assert wait_for_proc_termination(named_proc) + assert named_proc.returncode == 0, "named crashed" From 836e6ed284b9f62c49b06db944db83d508d4a054 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 6 Apr 2023 14:05:30 +0200 Subject: [PATCH 2/3] Ensure named always terminates in the shutdown test Previously, if an exception would happen inside the `with` block, the error handler would wait indefinitely for the process to end. That would never happen, since the termination signal was never sent to named and the test would get stuck. Using the try-finally block ensures that the named process is always killed and any exception or errors will be handled gracefully. --- bin/tests/system/shutdown/tests_shutdown.py | 27 ++++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/bin/tests/system/shutdown/tests_shutdown.py b/bin/tests/system/shutdown/tests_shutdown.py index 8a099067d1..dee0fb3cf8 100755 --- a/bin/tests/system/shutdown/tests_shutdown.py +++ b/bin/tests/system/shutdown/tests_shutdown.py @@ -189,15 +189,18 @@ def test_named_shutdown(named_port, control_port): for kill_method in ("rndc", "sigterm"): named_cmdline = [named, "-c", cfg_file, "-f"] with subprocess.Popen(named_cmdline, cwd=cfg_dir) as named_proc: - assert named_proc.poll() is None, "named isn't running" - assert wait_for_named_loaded(resolver) - do_work( - named_proc, - resolver, - rndc_cmd, - kill_method, - n_workers=12, - n_queries=16, - ) - assert wait_for_proc_termination(named_proc) - assert named_proc.returncode == 0, "named crashed" + try: + assert named_proc.poll() is None, "named isn't running" + assert wait_for_named_loaded(resolver) + do_work( + named_proc, + resolver, + rndc_cmd, + kill_method, + n_workers=12, + n_queries=16, + ) + assert wait_for_proc_termination(named_proc) + assert named_proc.returncode == 0, "named crashed" + finally: # Ensure named is terminated in case of an exception + named_proc.kill() From dee49b7a1f50ac1cf9236a6b37e800320960ad6b Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 6 Apr 2023 14:33:27 +0200 Subject: [PATCH 3/3] Replace dnspython resolver.query with resolver.resolve The resolver.query() has been deprecated in favor of resolver.resolve(); used that. This is an omission from 3b1756d45012a66f08693c17145a8484ec68bd47 --- bin/tests/system/shutdown/tests_shutdown.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/tests/system/shutdown/tests_shutdown.py b/bin/tests/system/shutdown/tests_shutdown.py index dee0fb3cf8..499dca323a 100755 --- a/bin/tests/system/shutdown/tests_shutdown.py +++ b/bin/tests/system/shutdown/tests_shutdown.py @@ -95,7 +95,7 @@ def do_work(named_proc, resolver, rndc_cmd, kill_method, n_workers, n_queries): ) qname = relname + ".test" - futures[executor.submit(resolver.query, qname, "A")] = tag + futures[executor.submit(resolver.resolve, qname, "A")] = tag elif shutdown: # We attempt to stop named in the middle shutdown = False if kill_method == "rndc":