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.

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
+configured NTP sources to IP addresses again and replace any addresses missing
-the machine in a different network.
+in the list of resolved addresses.
 +
-Sources that stop responding will be replaced with newly resolved addresses
+Sources that stop responding are replaced with newly resolved addresses
-automatically after 8 polling intervals, but this command can still be useful
+automatically after 8 polling intervals. This command can be used to replace
-to replace them immediately and not wait until they are marked as unreachable.
+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

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);
   }
 }
 

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