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 f33e2b6d87)
This commit is contained in:
Nicki Křížek 2025-10-23 15:08:35 +02:00
parent eabafffe37
commit 5e6ca56988
7 changed files with 37 additions and 180 deletions

View file

@ -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):

View file

@ -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

View file

@ -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<X>` string, where `<X>` 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

View file

@ -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

View file

@ -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

View file

@ -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.

View file

@ -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)