ntp: avoid unneccessary replacements on refresh command

When the refresh command is issued, instead of trying to replace all
NTP sources as if they were unreachable or falsetickers, keep using the
current address if it is still returned by the resolver for the name.
This avoids unnecessary loss of measurements and switching to
potentially unreachable addresses.

diff --git a/doc/chronyc.adoc b/doc/chronyc.adoc
index 2a37e5cfbf42b0003f1b8ad8095c7048746037bc..482313653f7904586491f4097e91e6dbb3079c45 100644 (file)
--- a/doc/chronyc.adoc
+++ b/doc/chronyc.adoc
@@ -970,12 +970,17 @@ current set of sources. It is equivalent to the *polltarget* option in the
 
 [[refresh]]*refresh*::
 The *refresh* command can be used to force *chronyd* to resolve the names of
-configured sources to IP addresses again, e.g. after suspending and resuming
-the machine in a different network.
+configured NTP sources to IP addresses again and replace any addresses missing
+in the list of resolved addresses.
 +
-Sources that stop responding will be replaced with newly resolved addresses
-automatically after 8 polling intervals, but this command can still be useful
-to replace them immediately and not wait until they are marked as unreachable.
+Sources that stop responding are replaced with newly resolved addresses
+automatically after 8 polling intervals. This command can be used to replace
+them immediately, e.g. after suspending and resuming the machine in a different
+network.
++
+Note that with larger pools which do not have all their IPv4 or IPv6 addresses
+included in a single DNS response (e.g. pool.ntp.org), this command will
+replace the addresses even if they are still included in the pool.
 
 [[reload]]*reload* *sources*::
 The *reload sources* command causes *chronyd* to re-read all _*.sources_ files

diff --git a/ntp_sources.c b/ntp_sources.c
index fa562a1685570c3ba43ca89b897c8596cebfa905..91267dcc24fb6ad380a43a9694fe9445fd63c20f 100644 (file)
--- a/ntp_sources.c
+++ b/ntp_sources.c
@@ -96,6 +96,9 @@ struct UnresolvedSource {
   char *name;
   /* Flag indicating addresses should be used in a random order */
   int random_order;
+  /* Flag indicating current address should be replaced only if it is
+     no longer returned by the resolver */
+  int refreshment;
   /* Next unresolved source in the list */
   struct UnresolvedSource *next;
 };
@@ -517,6 +520,19 @@ process_resolved_name(struct UnresolvedSource *us, IPAddr *ip_addrs, int n_addrs
   unsigned short first = 0;
   int i, j;
 
+  /* Keep using the current address if it is being refreshed and it is
+     still included in the resolved addresses */
+  if (us->refreshment) {
+    assert(us->pool_id == INVALID_POOL);
+
+    for (i = 0; i < n_addrs; i++) {
+      if (UTI_CompareIPs(&us->address.ip_addr, &ip_addrs[i], NULL) == 0) {
+        DEBUG_LOG("%s still fresh", UTI_IPToString(&us->address.ip_addr));
+        return;
+      }
+    }
+  }
+
   if (us->random_order)
     UTI_GetRandomBytes(&first, sizeof (first));
 
@@ -773,6 +789,7 @@ NSR_AddSourceByName(char *name, int port, int pool, NTP_Source_Type type,
   us = MallocNew(struct UnresolvedSource);
   us->name = Strdup(name);
   us->random_order = 0;
+  us->refreshment = 0;
 
   remote_addr.ip_addr.family = IPADDR_ID;
   remote_addr.ip_addr.addr.id = ++last_address_id;
@@ -962,7 +979,7 @@ NSR_RemoveAllSources(void)
 /* ================================================== */
 
 static void
-resolve_source_replacement(SourceRecord *record)
+resolve_source_replacement(SourceRecord *record, int refreshment)
 {
   struct UnresolvedSource *us;
 
@@ -976,6 +993,7 @@ resolve_source_replacement(SourceRecord *record)
      stuck to a pair of addresses if the order doesn't change, or a group of
      IPv4/IPv6 addresses if the resolver prefers inaccessible IP family */
   us->random_order = record->tentative;
+  us->refreshment = refreshment;
   us->pool_id = INVALID_POOL;
   us->address = *record->remote_addr;
 
@@ -1015,7 +1033,7 @@ NSR_HandleBadSource(IPAddr *address)
   }
   last_replacement = now;
 
-  resolve_source_replacement(record);
+  resolve_source_replacement(record, 0);
 }
 
 /* ================================================== */
@@ -1031,7 +1049,7 @@ NSR_RefreshAddresses(void)
     if (!record->remote_addr)
       continue;
 
-    resolve_source_replacement(record);
+    resolve_source_replacement(record, 1);
   }
 }
 

diff --git a/test/simulation/147-refresh b/test/simulation/147-refresh
new file mode 100755 (executable)
index 0000000..f83a9c6
--- /dev/null
+++ b/test/simulation/147-refresh
@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+
+. ./test.common
+
+test_start "address refreshment"
+
+limit=1000
+servers=5
+client_conf="logdir tmp
+log measurements"
+client_server_conf="server nodes-1-2.net1.clk maxpoll 6
+pool nodes-3-4-5.net1.clk maxpoll 6 maxsources 2"
+client_chronyd_options="-d"
+chronyc_conf="refresh"
+chronyc_start=500
+
+run_test || test_fail
+check_chronyd_exit || test_fail
+check_source_selection || test_fail
+check_packet_interval || test_fail
+check_sync || test_fail
+
+check_file_messages "20.*192.168.123.1" 0 0 measurements.log || test_fail
+check_file_messages "20.*192.168.123.2" 15 17 measurements.log || test_fail
+check_file_messages "20.*192.168.123.[345]" 31 33 measurements.log || test_fail
+rm -f tmp/measurements.log
+if check_config_h 'FEAT_DEBUG 1'; then
+       check_log_messages "resolved_name.*still fresh" 3 3 || test_fail
+fi
+
+test_pass