From 5e6ca5698816adbc46af54a7befacd5c075bdc5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicki=20K=C5=99=C3=AD=C5=BEek?= Date: Thu, 23 Oct 2025 15:08:35 +0200 Subject: [PATCH] Refactor NamedInstance.rndc() to use EnvCmd() interface To unify the command handling, utilize EnvCmd() to handle rndc commands: 1. Remove isctest.rndc abstractions. They were intended for an upcoming python-only implementation. A couple of years later, it doesn't seem to be coming any time soon, so let's stick with the interface that makes sense today, i.e. use the same command handling interface everywhere. 2. Remove the specialized rndc.log in favor of the generic logging already implemented by isctest.run.cmd(). I believe the cause of the many rndc(log=False) invocations was that nobody wanted this extra file. Yet, logging everything by default makes sense for debugging, unless there's a good reason not to. In almost all cases, logging was switched to the default (enabled). 3. With the NamedInstance.rndc() call now returning CmdResult rather than combined stdout+stderr string, adjust all the invocations to use `.out` or `.err` as necessary. 4. Replace some manual rndc invocation and its base argument construction with the standardized nsX.rndc() call. 5. In cases where rndc is expected to fail, utilize raise_on_exception=False and check the `.rc` from the result, rather than handling an exception. 6. In addzone/tests_rndc_deadlock.py, refactor the test slightly to avoid using EnvCmd() entirely to avoid spamming the logs. This test calls rndc in a loop from multiple threads and such test case is an exception which doesn't warrant changing the `isctest.run.cmd()` implementation. (cherry picked from commit f33e2b6d877146576d9384f21dbe1ff075df6205) --- .../system/addzone/tests_rndc_deadlock.py | 15 ++- bin/tests/system/isctest/__init__.py | 1 - bin/tests/system/isctest/instance.py | 96 ++++--------------- bin/tests/system/isctest/rndc.py | 69 ------------- bin/tests/system/keepalive/tests_keepalive.py | 12 +-- bin/tests/system/shutdown/tests_shutdown.py | 7 +- .../system/stress/tests_stress_update.py | 17 +--- 7 files changed, 37 insertions(+), 180 deletions(-) delete mode 100644 bin/tests/system/isctest/rndc.py diff --git a/bin/tests/system/addzone/tests_rndc_deadlock.py b/bin/tests/system/addzone/tests_rndc_deadlock.py index 3b987d3912..b65a074401 100755 --- a/bin/tests/system/addzone/tests_rndc_deadlock.py +++ b/bin/tests/system/addzone/tests_rndc_deadlock.py @@ -10,12 +10,12 @@ # information regarding copyright ownership. import concurrent.futures +import os +import subprocess import time import pytest -import isctest - pytestmark = pytest.mark.extra_artifacts( [ "ns*/*.nzf*", @@ -43,20 +43,19 @@ def rndc_loop(test_state, domain, ns3): ["delzone", domain], ] + args = [os.environ["RNDC"]] + ns3.rndc_args.split() while not test_state["finished"]: for command in rndc_commands: - ns3.rndc(" ".join(command), ignore_errors=True, log=False) + # avoid using ns3.rndc() directly to avoid log spam + subprocess.run(args + " ".join(command), timeout=10, check=False) def check_if_server_is_responsive(ns3): """ Check if server status can be successfully retrieved using "rndc status" """ - try: - ns3.rndc("status", log=False) - return True - except isctest.rndc.RNDCException: - return False + cmd = ns3.rndc("status", raise_on_exception=False) + return cmd.rc == 0 def test_rndc_deadlock(servers): diff --git a/bin/tests/system/isctest/__init__.py b/bin/tests/system/isctest/__init__.py index be94f1edfb..27b915d672 100644 --- a/bin/tests/system/isctest/__init__.py +++ b/bin/tests/system/isctest/__init__.py @@ -12,7 +12,6 @@ from . import check from . import instance from . import query -from . import rndc from . import run from . import template from . import log diff --git a/bin/tests/system/isctest/instance.py b/bin/tests/system/isctest/instance.py index a29f7cfdb5..63eb21d127 100644 --- a/bin/tests/system/isctest/instance.py +++ b/bin/tests/system/isctest/instance.py @@ -11,14 +11,14 @@ # See the COPYRIGHT file distributed with this work for additional # information regarding copyright ownership. -from typing import NamedTuple, Optional +from typing import NamedTuple -import logging import os +from pathlib import Path import re -from .rndc import RNDCBinaryExecutor, RNDCException, RNDCExecutor -from .log import info, WatchLogFromStart, WatchLogFromHere +from .log import WatchLogFromStart, WatchLogFromHere +from .run import CmdResult, EnvCmd from .text import TextFile @@ -43,8 +43,6 @@ class NamedInstance: self, identifier: str, ports: NamedPorts = NamedPorts(), - rndc_logger: Optional[logging.Logger] = None, - rndc_executor: Optional[RNDCExecutor] = None, ) -> None: """ `identifier` must be an `ns` string, where `` is an integer @@ -53,18 +51,18 @@ class NamedInstance: `ports` is the `NamedPorts` instance listing the UDP/TCP ports on which this `named` instance is listening for various types of traffic (both DNS traffic and RNDC commands). - - `rndc_logger` is the `logging.Logger` to use for logging RNDC - commands sent to this `named` instance. - - `rndc_executor` is an object implementing the `RNDCExecutor` interface - that is used for executing RNDC commands on this `named` instance. """ self.ip = self._identifier_to_ip(identifier) self.ports = ports self.log = TextFile(os.path.join(identifier, "named.run")) - self._rndc_executor = rndc_executor or RNDCBinaryExecutor() - self._rndc_logger = rndc_logger + + self._rndc_conf = Path("../_common/rndc.conf").absolute() + self._rndc = EnvCmd("RNDC", self.rndc_args) + + @property + def rndc_args(self) -> str: + """Base arguments for calling RNDC to control the instance.""" + return f"-c {self._rndc_conf} -s {self.ip} -p {self.ports.rndc}" @staticmethod def _identifier_to_ip(identifier: str) -> str: @@ -73,52 +71,16 @@ class NamedInstance: raise ValueError("Invalid named instance identifier" + identifier) return "10.53.0." + regex_match.group("index") - def rndc(self, command: str, ignore_errors: bool = False, log: bool = True) -> str: + def rndc(self, command: str, timeout=10, **kwargs) -> CmdResult: """ Send `command` to this named instance using RNDC. Return the server's response. - If the RNDC command fails, an `RNDCException` is raised unless - `ignore_errors` is set to `True`. - - The RNDC command will be logged to `rndc.log` (along with the server's - response) unless `log` is set to `False`. - - ```python - def test_foo(servers): - # Send the "status" command to ns1. An `RNDCException` will be - # raised if the RNDC command fails. This command will be logged. - response = servers["ns1"].rndc("status") - - # Send the "thaw foo" command to ns2. No exception will be raised - # in case the RNDC command fails. This command will be logged - # (even if it fails). - response = servers["ns2"].rndc("thaw foo", ignore_errors=True) - - # Send the "stop" command to ns3. An `RNDCException` will be - # raised if the RNDC command fails, but this command will not be - # logged (the server's response will still be returned to the - # caller, though). - response = servers["ns3"].rndc("stop", log=False) - - # Send the "halt" command to ns4 in "fire & forget mode": no - # exceptions will be raised and no logging will take place (the - # server's response will still be returned to the caller, though). - response = servers["ns4"].rndc("stop", ignore_errors=True, log=False) - ``` + To suppress exceptions, redirect outputs, control logging change + timeout etc. use keyword arguments which are passed to + isctest.cmd.run(). """ - try: - response = self._rndc_executor.call(self.ip, self.ports.rndc, command) - if log: - self._rndc_log(command, response) - except RNDCException as exc: - response = str(exc) - if log: - self._rndc_log(command, response) - if not ignore_errors: - raise - - return response + return self._rndc(command, timeout=timeout, **kwargs) def watch_log_from_start( self, timeout: float = WatchLogFromStart.DEFAULT_TIMEOUT @@ -138,28 +100,12 @@ class NamedInstance: """ return WatchLogFromHere(self.log.path, timeout) - def reconfigure(self, **kwargs) -> None: + def reconfigure(self, **kwargs) -> CmdResult: """ Reconfigure this named `instance` and wait until reconfiguration is - finished. Raise an `RNDCException` if reconfiguration fails. + finished. """ with self.watch_log_from_here() as watcher: - self.rndc("reconfig", **kwargs) + cmd = self.rndc("reconfig", **kwargs) watcher.wait_for_line("any newly configured zones are now loaded") - - def _rndc_log(self, command: str, response: str) -> None: - """ - Log an `rndc` invocation (and its output) to the `rndc.log` file in the - current working directory. - """ - fmt = '%(ip)s: "%(command)s"\n%(separator)s\n%(response)s%(separator)s' - args = { - "ip": self.ip, - "command": command, - "separator": "-" * 80, - "response": response, - } - if self._rndc_logger is None: - info(fmt, args) - else: - self._rndc_logger.info(fmt, args) + return cmd diff --git a/bin/tests/system/isctest/rndc.py b/bin/tests/system/isctest/rndc.py deleted file mode 100644 index d4a0a1bd77..0000000000 --- a/bin/tests/system/isctest/rndc.py +++ /dev/null @@ -1,69 +0,0 @@ -# Copyright (C) Internet Systems Consortium, Inc. ("ISC") -# -# SPDX-License-Identifier: MPL-2.0 -# -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this -# file, you can obtain one at https://mozilla.org/MPL/2.0/. -# -# See the COPYRIGHT file distributed with this work for additional -# information regarding copyright ownership. - -import abc -import os -import subprocess - - -class RNDCExecutor(abc.ABC): - """ - An interface which RNDC executors have to implement in order for the - `NamedInstance` class to be able to use them. - """ - - @abc.abstractmethod - def call(self, ip: str, port: int, command: str) -> str: - """ - Send RNDC `command` to the `named` instance at `ip:port` and return the - server's response. - """ - - -class RNDCException(Exception): - """ - Raised by classes implementing the `RNDCExecutor` interface when sending an - RNDC command fails for any reason. - """ - - -class RNDCBinaryExecutor(RNDCExecutor): - """ - An `RNDCExecutor` which sends RNDC commands to servers using the `rndc` - binary. - """ - - def __init__(self) -> None: - """ - This class needs the `RNDC` environment variable to be set to the path - to the `rndc` binary to use. - """ - rndc_path = os.environ.get("RNDC", "/bin/false") - rndc_conf = os.path.join("..", "_common", "rndc.conf") - self._base_cmdline = [rndc_path, "-c", rndc_conf] - - def call(self, ip: str, port: int, command: str) -> str: - """ - Send RNDC `command` to the `named` instance at `ip:port` and return the - server's response. - """ - cmdline = self._base_cmdline[:] - cmdline.extend(["-s", ip]) - cmdline.extend(["-p", str(port)]) - cmdline.extend(command.split()) - - try: - return subprocess.check_output( - cmdline, stderr=subprocess.STDOUT, timeout=10, encoding="utf-8" - ) - except subprocess.SubprocessError as exc: - msg = getattr(exc, "output", "RNDC exception occurred") - raise RNDCException(msg) from exc diff --git a/bin/tests/system/keepalive/tests_keepalive.py b/bin/tests/system/keepalive/tests_keepalive.py index 169810de60..d09d566f6e 100644 --- a/bin/tests/system/keepalive/tests_keepalive.py +++ b/bin/tests/system/keepalive/tests_keepalive.py @@ -20,7 +20,7 @@ pytestmark = pytest.mark.extra_artifacts( def test_dig_tcp_keepalive_handling(named_port, servers): def get_keepalive_options_received(): - servers["ns2"].rndc("stats", log=False) + servers["ns2"].rndc("stats") options_received = 0 with open("ns2/named.stats", "r", encoding="utf-8") as ns2_stats_file: for line in ns2_stats_file: @@ -55,11 +55,11 @@ def test_dig_tcp_keepalive_handling(named_port, servers): ) isctest.log.info("check a re-configured keepalive value") - response = servers["ns2"].rndc("tcp-timeouts 300 300 300 200", log=False) - assert "tcp-initial-timeout=300" in response - assert "tcp-idle-timeout=300" in response - assert "tcp-keepalive-timeout=300" in response - assert "tcp-advertised-timeout=200" in response + response = servers["ns2"].rndc("tcp-timeouts 300 300 300 200") + assert "tcp-initial-timeout=300" in response.out + assert "tcp-idle-timeout=300" in response.out + assert "tcp-keepalive-timeout=300" in response.out + assert "tcp-advertised-timeout=200" in response.out assert ( "; TCP KEEPALIVE: 20.0 secs" in dig("+tcp +keepalive foo.example. @10.53.0.2").out diff --git a/bin/tests/system/shutdown/tests_shutdown.py b/bin/tests/system/shutdown/tests_shutdown.py index 3d0c410ee0..52e574543e 100755 --- a/bin/tests/system/shutdown/tests_shutdown.py +++ b/bin/tests/system/shutdown/tests_shutdown.py @@ -71,11 +71,8 @@ def do_work(named_proc, resolver_ip, instance, kill_method, n_workers, n_queries # helper function, 'command' is the rndc command to run def launch_rndc(command): - try: - instance.rndc(command, log=False) - return 0 - except isctest.rndc.RNDCException: - return -1 + ret = instance.rndc(command, raise_on_exception=False) + return 0 if ret.rc == 0 else -1 # We're going to execute queries in parallel by means of a thread pool. # dnspython functions block, so we need to circumvent that. diff --git a/bin/tests/system/stress/tests_stress_update.py b/bin/tests/system/stress/tests_stress_update.py index 5bb413df33..ef93c57fa0 100644 --- a/bin/tests/system/stress/tests_stress_update.py +++ b/bin/tests/system/stress/tests_stress_update.py @@ -10,7 +10,6 @@ # information regarding copyright ownership. import concurrent.futures -import os import time import dns.update @@ -28,22 +27,8 @@ pytestmark = pytest.mark.extra_artifacts( def rndc_loop(test_state, server): - rndc = os.getenv("RNDC") - port = os.getenv("CONTROLPORT") - - cmdline = [ - rndc, - "-c", - "../_common/rndc.conf", - "-p", - port, - "-s", - server, - "reload", - ] - while not test_state["finished"]: - isctest.run.cmd(cmdline, raise_on_exception=False) + server.rndc("reload", raise_on_exception=False) time.sleep(1)