diff --git a/doc/ChangeLog.md b/doc/ChangeLog.md index 823d7b68a..6a44064b6 100644 --- a/doc/ChangeLog.md +++ b/doc/ChangeLog.md @@ -10,6 +10,8 @@ All notable changes to the project are documented in this file. - Upgrade Linux kernel to 6.18.49 (LTS) - Upgrade FRR to 10.5.5 +- Add per-server `minpoll` and `maxpoll` options to the NTP client + configuration, for tuning how often each server is polled [v26.08.0][] - 2026-09-01 ------------------------- diff --git a/doc/system.md b/doc/system.md index f2394086e..4407ac863 100644 --- a/doc/system.md +++ b/doc/system.md @@ -488,6 +488,17 @@ metrics (default config). * `prefer true`: The NTP client will try to use the preferred server as the primary source unless it becomes unreachable or unusable. +Each server also accepts `minpoll` and `maxpoll` options, bounding how +often it is polled, expressed as log2 seconds: + +
admin@example:/config/system/ntp/> set server ntp-pool minpoll 4
+admin@example:/config/system/ntp/> set server ntp-pool maxpoll 8
+
+ +The defaults, 6 (64 seconds) and 10 (1024 seconds), suit most +deployments. Lower values give faster convergence and failover on local +networks, at the cost of more NTP traffic. + ### Show NTP Sources diff --git a/src/confd/src/system.c b/src/confd/src/system.c index 3890685d2..5bb7a0dd2 100644 --- a/src/confd/src/system.c +++ b/src/confd/src/system.c @@ -287,6 +287,18 @@ static int change_clock(sr_session_ctx_t *session, struct lyd_node *config, stru return rc; } +/* Emit ' OPTION VALUE' for a per-server leaf, if set */ +static void ntp_option(FILE *fp, sr_session_ctx_t *session, const char *xpath, + const char *leaf, const char *option) +{ + char *val = srx_get_str(session, "%s/%s", xpath, leaf); + + if (val) { + fprintf(fp, " %s %s", option, val); + free(val); + } +} + static int change_ntp_client(sr_session_ctx_t *session, struct lyd_node *config, struct lyd_node *diff, sr_event_t event, struct confd *confd) { sr_change_iter_t *iter = NULL; @@ -392,11 +404,7 @@ static int change_ntp_client(sr_session_ctx_t *session, struct lyd_node *config, free(type); free(ptr); - ptr = srx_get_str(session, "%s/udp/port", xpath); - if (ptr) { - fprintf(fp, " port %s", ptr); - free(ptr); - } + ntp_option(fp, session, xpath, "udp/port", "port"); } if (server) { @@ -404,6 +412,8 @@ static int change_ntp_client(sr_session_ctx_t *session, struct lyd_node *config, fprintf(fp, " iburst"); if (srx_enabled(session, "%s/prefer", xpath) > 0) fprintf(fp, " prefer"); + ntp_option(fp, session, xpath, "infix-system:minpoll", "minpoll"); + ntp_option(fp, session, xpath, "infix-system:maxpoll", "maxpoll"); } fprintf(fp, "\n"); fclose(fp); diff --git a/src/confd/yang/confd.inc b/src/confd/yang/confd.inc index 88cada9d8..86286ff30 100644 --- a/src/confd/yang/confd.inc +++ b/src/confd/yang/confd.inc @@ -43,7 +43,7 @@ MODULES=( "infix-firewall-icmp-types@2025-04-26.yang" "infix-meta@2025-12-10.yang" "infix-services@2026-06-17.yang" - "infix-system@2026-06-17.yang" + "infix-system@2026-09-01.yang" "ieee802-ethernet-interface@2025-09-10.yang" "ieee802-ethernet-phy-type@2025-09-10.yang" "infix-ethernet-interface@2026-05-21.yang" diff --git a/src/confd/yang/confd/infix-system.yang b/src/confd/yang/confd/infix-system.yang index b79a2fcad..84e6f3d51 100644 --- a/src/confd/yang/confd/infix-system.yang +++ b/src/confd/yang/confd/infix-system.yang @@ -32,6 +32,10 @@ module infix-system { contact "kernelkit@googlegroups.com"; description "Infix augments and deviations to ietf-system."; + revision 2026-09-01 { + description "Add per-server minpoll/maxpoll to NTP client configuration."; + reference "internal"; + } revision 2026-06-17 { description "Add scheduled-reboot, triggered from a referenced schedule."; reference "internal"; @@ -205,6 +209,14 @@ module infix-system { * Typedefs */ + typedef poll-interval { + type int8 { + range "-7..24"; + } + units "log2 seconds"; + description "NTP poll interval, as log2 seconds."; + } + typedef crypt-hash { type string { pattern @@ -370,6 +382,34 @@ module infix-system { } } + augment "/sys:system/sys:ntp/sys:server" { + description "Per-server poll interval limits for the NTP client."; + + leaf minpoll { + type poll-interval; + default "6"; + description + "Minimum poll interval, default 6 (64 seconds). Values below + 6 should not be used with public servers on the Internet."; + reference + "RFC 5905: Network Time Protocol Version 4: Protocol and + Algorithms Specification, Section 7.2"; + } + + leaf maxpoll { + must ". >= ../minpoll" { + error-message "maxpoll must be greater than or equal to minpoll"; + } + type poll-interval; + default "10"; + description + "Maximum poll interval, default 10 (1024 seconds)."; + reference + "RFC 5905: Network Time Protocol Version 4: Protocol and + Algorithms Specification, Section 7.2"; + } + } + augment "/sys:system/sys:authentication/sys:user" { description "Augment of ietf-system to support setting login shell for users."; leaf shell { diff --git a/src/confd/yang/confd/infix-system@2026-06-17.yang b/src/confd/yang/confd/infix-system@2026-09-01.yang similarity index 100% rename from src/confd/yang/confd/infix-system@2026-06-17.yang rename to src/confd/yang/confd/infix-system@2026-09-01.yang diff --git a/test/.env b/test/.env index 857d9e128..16ef67f13 100644 --- a/test/.env +++ b/test/.env @@ -2,7 +2,7 @@ # shellcheck disable=SC2034,SC2154 # Current container image -INFIX_TEST=ghcr.io/kernelkit/infix-test:2.10 +INFIX_TEST=ghcr.io/kernelkit/infix-test:2.11 ixdir=$(readlink -f "$testdir/..") logdir=$(readlink -f "$testdir/.log") diff --git a/test/case/ntp/client_stratum_selection/test.adoc b/test/case/ntp/client_stratum_selection/test.adoc index 07a9ecc3a..1dfbdc2ed 100644 --- a/test/case/ntp/client_stratum_selection/test.adoc +++ b/test/case/ntp/client_stratum_selection/test.adoc @@ -10,14 +10,22 @@ stratum level. This test validates NTP clock selection algorithm by configuring a client to sync from two servers with different stratum levels: -- srv1: Test PC running BusyBox ntpd (stratum ~1 via -l flag) -- srv2: NTP server DUT syncing from srv1 (stratum ~2) +- srv1: Test PC running chronyd, serving its local clock at stratum 5 + with an honest root distance so clients tolerate startup transients +- srv2: NTP server DUT syncing from srv1 (stratum 6) - client: NTP client DUT syncing from both servers Both servers sync to the same time source (srv2 syncs from srv1), ensuring time agreement and avoiding the "falseticker" problem. The client should then select srv1 (lower stratum) as its sync source. +NOTE: srv1 serves the test PC's system clock, so the test depends on that +clock being stable for the duration of the run. A host time daemon that +applies discrete corrections, e.g. systemd-timesyncd, makes the client +flag srv1 as unstable and refuse to select it. Test PCs should keep the +clock free-running during the test, or discipline it with a slewing +daemon such as chronyd. + ==== Topology image::topology.svg[NTP Client Stratum Selection topology, align=center, scaledwidth=75%] diff --git a/test/case/ntp/client_stratum_selection/test.py b/test/case/ntp/client_stratum_selection/test.py index 42424d2f1..081a2c7fc 100755 --- a/test/case/ntp/client_stratum_selection/test.py +++ b/test/case/ntp/client_stratum_selection/test.py @@ -7,14 +7,22 @@ This test validates NTP clock selection algorithm by configuring a client to sync from two servers with different stratum levels: -- srv1: Test PC running BusyBox ntpd (stratum ~1 via -l flag) -- srv2: NTP server DUT syncing from srv1 (stratum ~2) +- srv1: Test PC running chronyd, serving its local clock at stratum 5 + with an honest root distance so clients tolerate startup transients +- srv2: NTP server DUT syncing from srv1 (stratum 6) - client: NTP client DUT syncing from both servers Both servers sync to the same time source (srv2 syncs from srv1), ensuring time agreement and avoiding the "falseticker" problem. The client should then select srv1 (lower stratum) as its sync source. +NOTE: srv1 serves the test PC's system clock, so the test depends on that +clock being stable for the duration of the run. A host time daemon that +applies discrete corrections, e.g. systemd-timesyncd, makes the client +flag srv1 as unstable and refuse to select it. Test PCs should keep the +clock free-running during the test, or discipline it with a slewing +daemon such as chronyd. + """ import infamy @@ -24,11 +32,14 @@ # Network configuration ips = { - "srv1": "192.168.1.1", # BusyBox ntpd on test PC + "srv1": "192.168.1.1", # chronyd on test PC "srv2": "192.168.1.2", # Infix NTP server "client": "192.168.1.3" # Infix NTP client } +# 16 s polls so selection re-evaluates quickly after iburst +POLL = {"infix-system:minpoll": 4, "infix-system:maxpoll": 6} + with infamy.Test() as test: with test.step("Set up topology and attach to devices"): env = infamy.Env() @@ -78,14 +89,29 @@ "unicast-configuration": [{ "address": ips["srv1"], # Sync from srv1 "type": "uc-server", - "iburst": True + "iburst": True, + # Poll every 16 s so sub-threshold + # offsets drain quickly (corrections + # are spread over ~3 poll intervals) + "minpoll": 4, + "maxpoll": 6 }] } } }) with test.step("Wait for srv2 to sync from srv1"): - until(lambda: ntp.server_has_associations(srv2), attempts=60) + # Converged, not just associated, see the docstring of + # server_source_synced. iburst + makestep take 10-20 s, + # the rest of the budget is only used when broken + try: + until(lambda: ntp.server_source_synced(srv2, ips["srv1"]), + attempts=60) + except Exception: + print("DEBUG: srv2 did not converge on srv1. Associations:") + for assoc in ntp.server_get_associations(srv2): + print(f" {assoc}") + raise with test.step("Configure client to sync from both servers"): client.put_config_dicts({ @@ -113,13 +139,15 @@ "udp": { "address": ips["srv1"] }, - "iburst": True + "iburst": True, + **POLL }, { "name": "srv2", "udp": { "address": ips["srv2"] }, - "iburst": True + "iburst": True, + **POLL }] } } @@ -127,28 +155,25 @@ }) with test.step("Wait for client to see both servers"): - until(lambda: ntp.number_of_sources(client) == 2, attempts=60) + until(lambda: ntp.number_of_sources(client) == 2, attempts=30) with test.step("Wait for srv2 stratum to stabilize"): # Ensure srv2 has synced with srv1 and is advertising # stratum 2. This prevents race where both advertise # stratum 1, causing wrong selection def check_stratums(): - srv1 = ntp.get_source_by_address(client, ips["srv1"]) - srv2 = ntp.get_source_by_address(client, ips["srv2"]) - - if not srv1 or not srv2: - return False - - srv1_stratum = srv1.get("stratum") - srv2_stratum = srv2.get("stratum") + stratum = {src.get("address"): src.get("stratum") + for src in ntp.get_sources(client)} + srv1_stratum = stratum.get(ips["srv1"]) + srv2_stratum = stratum.get(ips["srv2"]) # Both must have valid stratums and srv1 < srv2 - if srv1_stratum and srv2_stratum and srv1_stratum < srv2_stratum: - return True - return False + return bool(srv1_stratum and srv2_stratum + and srv1_stratum < srv2_stratum) - until(check_stratums, attempts=60) + # srv2 synced before the client was configured, so the + # client's iburst samples already carry stratum 6 + until(check_stratums, attempts=30) print(f"srv1 and srv2 stratums verified as different") with test.step("Verify client selects srv1 (lower stratum)"): @@ -159,15 +184,15 @@ def srv1_selected(): return None try: - selected = until(srv1_selected, attempts=120) + # Selection normally happens at the end of iburst; + # with minpoll 4 this covers two extra 16 s poll + # cycles plus slack + selected = until(srv1_selected, attempts=45) except Exception: # Timeout - print diagnostic info - sources = ntp.get_sources(client) - print("DEBUG: Failed to select srv1. Source details:") - for src in sources: - print(f" {src.get('address')}: stratum={src.get('stratum')}, " - f"state={src.get('state')}, poll={src.get('poll')}, " - f"offset={src.get('offset')}") + print("DEBUG: Failed to select srv1. Sources:") + for src in ntp.get_sources(client): + print(f" {src}") raise assert selected is not None, "srv1 was not selected" diff --git a/test/docker/Dockerfile b/test/docker/Dockerfile index c1750270c..1267d7c05 100644 --- a/test/docker/Dockerfile +++ b/test/docker/Dockerfile @@ -3,6 +3,7 @@ FROM alpine:3.18.0 # NOTE: please add packages alphabetically! RUN apk add --no-cache \ busybox-extras \ + chrony \ curl \ dhcp-server-vanilla \ dnsmasq \ diff --git a/test/infamy/ntp.py b/test/infamy/ntp.py index c05e6cd3d..291232827 100644 --- a/test/infamy/ntp.py +++ b/test/infamy/ntp.py @@ -95,41 +95,52 @@ def server_query(netns, server_ip, expected_stratum=None): return True -def server_has_associations(target): - """Verify NTP server (ietf-ntp) has any associations.""" +def server_get_associations(target): + """Get list of NTP associations (ietf-ntp) from operational state.""" try: data = target.get_data("/ietf-ntp:ntp/associations") if not data: - return False - - associations = data.get("ntp", {}).get("associations", {}).get("association", []) - return len(associations) > 0 + return [] + return data.get("ntp", {}).get("associations", {}).get("association", []) except Exception: - return False + return [] -def server_has_peer(target, peer_address): - """Verify NTP server (ietf-ntp) has a peer association with given address.""" - try: - data = target.get_data("/ietf-ntp:ntp/associations") - if not data: - return False +def server_has_associations(target): + """Verify NTP server (ietf-ntp) has any associations.""" + return len(server_get_associations(target)) > 0 + - associations = data.get("ntp", {}).get("associations", {}).get("association", []) - if not associations: +def server_source_synced(target, address, max_offset_ms=50.0): + """Verify NTP server (ietf-ntp) has selected the given source and + converged on it, i.e., the estimated offset is small. + + Waiting only for the association to exist is not enough: the server + may still be stepping/slewing its clock, serving time that moves + around. Any client sampling both this server and its upstream + during that window collects inconsistent measurements and chronyd + flags the upstream as having too much variability, excluding it + from selection for several poll intervals. + """ + for assoc in server_get_associations(target): + if assoc.get("address") != address: + continue + if not assoc.get("prefer") or assoc.get("offset") is None: return False + return abs(float(assoc["offset"])) <= max_offset_ms - # Check if peer association exists with the given address - # local-mode will be "ietf-ntp:active" or "active" depending on namespace handling - for assoc in associations: - local_mode = assoc.get("local-mode", "") - if (assoc.get("address") == peer_address and - (local_mode == "ietf-ntp:active" or local_mode == "active")): - return True + return False - return False - except Exception: - return False + +def server_has_peer(target, peer_address): + """Verify NTP server (ietf-ntp) has a peer association with given address.""" + # local-mode is "ietf-ntp:active" or "active" depending on namespace handling + for assoc in server_get_associations(target): + if (assoc.get("address") == peer_address and + assoc.get("local-mode", "") in ("ietf-ntp:active", "active")): + return True + + return False def server_peer_reachable(target, peer_address): diff --git a/test/infamy/ntp_server.py b/test/infamy/ntp_server.py index baa01d85f..b5e0bdcdb 100644 --- a/test/infamy/ntp_server.py +++ b/test/infamy/ntp_server.py @@ -1,24 +1,87 @@ """Start NTP server in the background""" -import subprocess +import contextlib +import os +import tempfile +import time + class Server: - def __init__(self, netns, iface="iface"): - self.iface = iface + """chronyd serving its local clock, never touching it (-x). + + stratum and distance control what is advertised to clients: the + stratum of the served time and the root distance (accuracy claim). + An honest, wide distance keeps clients from flagging the source as + unstable ('~') while their own clocks are still settling, which + BusyBox ntpd provoked by claiming near-zero dispersion. + """ + + def __init__(self, netns, stratum=5, distance=1.0): self.process = None self.netns = netns + self.stratum = stratum + self.distance = distance + self.rundir = None + self.logfile = None + self.pidfile = None def __enter__(self): self.start() + return self def __exit__(self, _, __, ___): self.stop() def start(self): - cmd=f"ntpd -w -n -l -I {self.iface}" - self.process = self.netns.popen(cmd.split(" "),stderr=subprocess.DEVNULL) + # Instances in different netns share the filesystem, so the + # pidfile is per-instance. It lives in /run/chrony because a + # host AppArmor profile for chronyd, present when the host runs + # chrony, attaches by binary path even inside the test container + # and allows no other pidfile location. The command socket and + # port are disabled, nothing talks to chronyc here, and + # -f /dev/null keeps the image's default config out of it. + self.rundir = tempfile.TemporaryDirectory(prefix="chronyd-") + log = f"{self.rundir.name}/chronyd.log" + piddir = "/run/chrony" + try: + os.makedirs(piddir, exist_ok=True) + except OSError: + piddir = self.rundir.name + self.pidfile = f"{piddir}/{os.path.basename(self.rundir.name)}.pid" + cmd = [ + "chronyd", "-d", "-x", "-f", "/dev/null", "-u", "root", + f"local stratum {self.stratum} distance {self.distance}", + "allow", + "cmdport 0", + "bindcmdaddress /", + f"pidfile {self.pidfile}", + ] + self.logfile = open(log, "w") + self.process = self.netns.popen(cmd, stderr=self.logfile) + + # chronyd exits immediately on bad options or a missing binary + # behind an exec wrapper; fail loudly instead of serving nothing + time.sleep(1) + if self.process.poll() is not None: + with open(log) as f: + output = f.read().strip() + code = self.process.returncode + self.stop() + raise RuntimeError( + f"chronyd failed to start (exit {code}): " + f"{output or 'no output; is chrony installed in the test environment?'}") def stop(self): if self.process: self.process.terminate() self.process.wait() self.process = None + if self.logfile: + self.logfile.close() + self.logfile = None + if self.pidfile: + with contextlib.suppress(OSError): + os.unlink(self.pidfile) + self.pidfile = None + if self.rundir: + self.rundir.cleanup() + self.rundir = None