Re: [chrony-dev] [PATCH] local threshold option

[Andy Fiddaman <illumos@xxxxxxxxxxxx>]

On Fri, 29 Mar 2024, Andy Fiddaman wrote:

>
> On Thu, 28 Mar 2024, Miroslav Lichvar wrote:
>
> > On Mon, Mar 25, 2024 at 07:10:31PM +0000, Andy Fiddaman wrote:
> > > How does the attached patch look? This adds `local threshold` as you
> > > suggested, that defaults to 0 which means it is not used. If it's set,
> > > then the local reference is not activated until the root distance drops
> > > below the value once.
> > >
> > > In our configuration, we can then use something like:
> > >
> > >     local stratum 10 distance 1.0 threshold 1.0
> >
> > The distance option is already described as a threshold, so I think a
> > different name might work better. How about trigger, start or activate?
>
> I like 'activate', that's much clearer.
>
> > Let's think a bit about possible future options. We might need a
> > timeout-based activation, e.g. wait for 10 minutes before activating
> > local. It could be a second parameter of this option or a separate
> > option. What naming would be most consistent?
>
> If such things were required, I think it would probably be better
> to group them with 'activate'; as in having the timer as an extra
> parameter. It's hard to predict what might be needed in the future
> though. The same goes for the potential padding of the local
> control message.
>
> I've attached the latest patch.
>
> I'm happy to go back and do something here if you'd like, but also
> please feel free to adjust what I've done here to better fit the
> goals of the project.

The last patch was missing a couple of things, and did not preserve support
for older clients that might be using REQ_LOCAL2. I've updated the patch
to address those.

Andy

From 9083a01a09d2cbae80669feccb0e2a29e200e980 Mon Sep 17 00:00:00 2001
From: Andy Fiddaman <illumos@xxxxxxxxxxxx>
Date: Mon, 25 Mar 2024 19:05:52 +0000
Subject: [PATCH] Add "local activate" option

---
 candm.h              |  4 +++-
 client.c             |  7 ++++---
 cmdmon.c             | 12 ++++++++----
 cmdparse.c           |  6 +++++-
 cmdparse.h           |  2 +-
 conf.c               |  6 ++++--
 conf.h               |  2 +-
 doc/chrony.conf.adoc | 21 ++++++++++++++-------
 pktlength.c          |  3 ++-
 reference.c          | 21 +++++++++++++++------
 reference.h          |  2 +-
 11 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/candm.h b/candm.h
index 0894ad5..b4e41f1 100644
--- a/candm.h
+++ b/candm.h
@@ -111,7 +111,8 @@
 #define REQ_DOFFSET2 71
 #define REQ_MODIFY_SELECTOPTS 72
 #define REQ_MODIFY_OFFSET 73
-#define N_REQUEST_TYPES 74
+#define REQ_LOCAL3 74
+#define N_REQUEST_TYPES 75
 
 /* Structure used to exchange timespecs independent of time_t size */
 typedef struct {
@@ -237,6 +238,7 @@ typedef struct {
   int32_t stratum;
   Float distance;
   int32_t orphan;
+  Float activate;
   int32_t EOR;
 } REQ_Local;
 
diff --git a/client.c b/client.c
index 0231b9e..a1e213f 100644
--- a/client.c
+++ b/client.c
@@ -755,22 +755,23 @@ static int
 process_cmd_local(CMD_Request *msg, char *line)
 {
   int on_off, stratum = 0, orphan = 0;
-  double distance = 0.0;
+  double distance = 0.0, activate = 0.0;
 
   if (!strcmp(line, "off")) {
     on_off = 0;
-  } else if (CPS_ParseLocal(line, &stratum, &orphan, &distance)) {
+  } else if (CPS_ParseLocal(line, &stratum, &orphan, &distance, &activate)) {
     on_off = 1;
   } else {
     LOG(LOGS_ERR, "Invalid syntax for local command");
     return 0;
   }
 
-  msg->command = htons(REQ_LOCAL2);
+  msg->command = htons(REQ_LOCAL3);
   msg->data.local.on_off = htonl(on_off);
   msg->data.local.stratum = htonl(stratum);
   msg->data.local.distance = UTI_FloatHostToNetwork(distance);
   msg->data.local.orphan = htonl(orphan);
+  msg->data.local.activate = UTI_FloatHostToNetwork(activate);
 
   return 1;
 }
diff --git a/cmdmon.c b/cmdmon.c
index 716775f..8d7d7e5 100644
--- a/cmdmon.c
+++ b/cmdmon.c
@@ -146,6 +146,7 @@ static const char permissions[] = {
   PERMIT_AUTH, /* DOFFSET2 */
   PERMIT_AUTH, /* MODIFY_SELECTOPTS */
   PERMIT_AUTH, /* MODIFY_OFFSET */
+  PERMIT_AUTH, /* LOCAL3 */
 };
 
 /* ================================================== */
@@ -526,12 +527,14 @@ handle_settime(CMD_Request *rx_message, CMD_Reply *tx_message)
 /* ================================================== */
 
 static void
-handle_local(CMD_Request *rx_message, CMD_Reply *tx_message)
+handle_local(CMD_Request *rx_message, CMD_Reply *tx_message, uint16_t cmd)
 {
   if (ntohl(rx_message->data.local.on_off)) {
     REF_EnableLocal(ntohl(rx_message->data.local.stratum),
                     UTI_FloatNetworkToHost(rx_message->data.local.distance),
-                    ntohl(rx_message->data.local.orphan));
+                    ntohl(rx_message->data.local.orphan),
+                    cmd == REQ_LOCAL2 ? 0 :
+                    UTI_FloatNetworkToHost(rx_message->data.local.activate));
   } else {
     REF_DisableLocal();
   }
@@ -1648,9 +1651,10 @@ read_from_cmd_socket(int sock_fd, int event, void *anything)
         case REQ_SETTIME:
           handle_settime(&rx_message, &tx_message);
           break;
-        
+
         case REQ_LOCAL2:
-          handle_local(&rx_message, &tx_message);
+        case REQ_LOCAL3:
+          handle_local(&rx_message, &tx_message, rx_command);
           break;
 
         case REQ_MANUAL:
diff --git a/cmdparse.c b/cmdparse.c
index 0a80fc0..77447dc 100644
--- a/cmdparse.c
+++ b/cmdparse.c
@@ -296,13 +296,14 @@ CPS_ParseAllowDeny(char *line, int *all, IPAddr *ip, int *subnet_bits)
 /* ================================================== */
 
 int
-CPS_ParseLocal(char *line, int *stratum, int *orphan, double *distance)
+CPS_ParseLocal(char *line, int *stratum, int *orphan, double *distance, double *activate)
 {
   int n;
   char *cmd;
 
   *stratum = 10;
   *distance = 1.0;
+  *activate = 0.0;
   *orphan = 0;
 
   while (*line) {
@@ -319,6 +320,9 @@ CPS_ParseLocal(char *line, int *stratum, int *orphan, double *distance)
     } else if (!strcasecmp(cmd, "distance")) {
       if (sscanf(line, "%lf%n", distance, &n) != 1)
         return 0;
+    } else if (!strcasecmp(cmd, "activate")) {
+      if (sscanf(line, "%lf%n", activate, &n) != 1)
+        return 0;
     } else {
       return 0;
     }
diff --git a/cmdparse.h b/cmdparse.h
index 095a8e2..f0bc7a4 100644
--- a/cmdparse.h
+++ b/cmdparse.h
@@ -47,7 +47,7 @@ extern int CPS_GetSelectOption(char *option);
 extern int CPS_ParseAllowDeny(char *line, int *all, IPAddr *ip, int *subnet_bits);
 
 /* Parse a command to enable local reference */
-extern int CPS_ParseLocal(char *line, int *stratum, int *orphan, double *distance);
+extern int CPS_ParseLocal(char *line, int *stratum, int *orphan, double *distance, double *activate);
 
 /* Remove extra white-space and comments */
 extern void CPS_NormalizeLine(char *line);
diff --git a/conf.c b/conf.c
index 8849bdc..d73c6dd 100644
--- a/conf.c
+++ b/conf.c
@@ -129,6 +129,7 @@ static int enable_local=0;
 static int local_stratum;
 static int local_orphan;
 static double local_distance;
+static double local_activate;
 
 /* Threshold (in seconds) - if absolute value of initial error is less
    than this, slew instead of stepping */
@@ -1066,7 +1067,7 @@ parse_log(char *line)
 static void
 parse_local(char *line)
 {
-  if (!CPS_ParseLocal(line, &local_stratum, &local_orphan, &local_distance))
+  if (!CPS_ParseLocal(line, &local_stratum, &local_orphan, &local_distance, &local_activate))
     command_parse_error();
   enable_local = 1;
 }
@@ -2166,12 +2167,13 @@ CNF_GetCommandPort(void) {
 /* ================================================== */
 
 int
-CNF_AllowLocalReference(int *stratum, int *orphan, double *distance)
+CNF_AllowLocalReference(int *stratum, int *orphan, double *distance, double *activate)
 {
   if (enable_local) {
     *stratum = local_stratum;
     *orphan = local_orphan;
     *distance = local_distance;
+    *activate = local_activate;
     return 1;
   } else {
     return 0;
diff --git a/conf.h b/conf.h
index 4c0a787..2128a3e 100644
--- a/conf.h
+++ b/conf.h
@@ -108,7 +108,7 @@ extern double CNF_GetReselectDistance(void);
 extern double CNF_GetStratumWeight(void);
 extern double CNF_GetCombineLimit(void);
 
-extern int CNF_AllowLocalReference(int *stratum, int *orphan, double *distance);
+extern int CNF_AllowLocalReference(int *stratum, int *orphan, double *distance, double *activate);
 
 extern void CNF_SetupAccessRestrictions(void);
 
diff --git a/doc/chrony.conf.adoc b/doc/chrony.conf.adoc
index bd296bc..2c29ae7 100644
--- a/doc/chrony.conf.adoc
+++ b/doc/chrony.conf.adoc
@@ -1663,11 +1663,11 @@ be very close to real time. Stratum 2 computers are those which have a stratum
 of 10 indicates that the clock is so many hops away from a reference clock that
 its time is fairly unreliable.
 *distance* _distance_:::
-This option sets the threshold for the root distance which will activate the local
-reference. If *chronyd* was synchronised to some source, the local reference
-will not be activated until its root distance reaches the specified value (the
-rate at which the distance is increasing depends on how well the clock was
-tracking the source). The default value is 1 second.
+This option sets the threshold for the root distance which will activate the
+local reference. If *chronyd* was synchronised to some source, the local
+reference will not be activated until its root distance reaches the specified
+value (the rate at which the distance is increasing depends on how well the
+clock was tracking the source). The default value is 1 second.
 +
 The current root distance can be calculated from root delay and root dispersion
 (reported by the <<chronyc.adoc#tracking,*tracking*>> command in *chronyc*) as:
@@ -1675,6 +1675,14 @@ The current root distance can be calculated from root delay and root dispersion
 ----
 distance = delay / 2 + dispersion
 ----
+*activate* _distance_:::
+This option sets an activating root distance for the local reference. The
+local reference will not be used until the root distance drops below the
+configured value for the first time. This can be used to prevent the local
+reference from being activated on a server which has never been synchronised
+with an upstream server. The default value of 0.0 causes no activating
+distance to be used, such that the local reference is always eligible for
+activation.
 *orphan*:::
 This option enables a special '`orphan`' mode, where sources with stratum equal
 to the local _stratum_ are assumed to not serve real time. They are ignored
@@ -1692,12 +1700,11 @@ smallest reference ID will take over when its local reference mode activates
 +
 The *orphan* mode is compatible with the *ntpd*'s orphan mode (enabled by the
 *tos orphan* command).
-{blank}::
 +
 An example of the directive is:
 +
 ----
-local stratum 10 orphan distance 0.1
+local stratum 10 orphan distance 0.1 activate 0.5
 ----
 
 [[ntpsigndsocket]]*ntpsigndsocket* _directory_::
diff --git a/pktlength.c b/pktlength.c
index 3cca306..8961f4a 100644
--- a/pktlength.c
+++ b/pktlength.c
@@ -111,7 +111,7 @@ static const struct request_length request_lengths[] = {
   REQ_LENGTH_ENTRY(null, null),                 /* REFRESH */
   REQ_LENGTH_ENTRY(null, server_stats),         /* SERVER_STATS */
   { 0, 0 },                                     /* CLIENT_ACCESSES_BY_INDEX2 - not supported */
-  REQ_LENGTH_ENTRY(local, null),                /* LOCAL2 */
+  { offsetof(CMD_Request, data.local.activate), 0 }, /* LOCAL2 */
   REQ_LENGTH_ENTRY(ntp_data, ntp_data),         /* NTP_DATA */
   { 0, 0 },                                     /* ADD_SERVER2 */
   { 0, 0 },                                     /* ADD_PEER2 */
@@ -131,6 +131,7 @@ static const struct request_length request_lengths[] = {
   REQ_LENGTH_ENTRY(doffset, null),              /* DOFFSET2 */
   REQ_LENGTH_ENTRY(modify_select_opts, null),   /* MODIFY_SELECTOPTS */
   REQ_LENGTH_ENTRY(modify_offset, null),        /* MODIFY_OFFSET */
+  REQ_LENGTH_ENTRY(local, null),                /* LOCAL3 */
 };
 
 static const uint16_t reply_lengths[] = {
diff --git a/reference.c b/reference.c
index 1ac6cb9..39ef95e 100644
--- a/reference.c
+++ b/reference.c
@@ -54,6 +54,8 @@ static int enable_local_stratum;
 static int local_stratum;
 static int local_orphan;
 static double local_distance;
+static int local_activate_ok;
+static double local_activate;
 static struct timespec local_ref_time;
 static NTP_Leap our_leap_status;
 static int our_leap_sec;
@@ -207,6 +209,7 @@ REF_Initialise(void)
   our_frequency_sd = 0.0;
   our_offset_sd = 0.0;
   drift_file_age = 0.0;
+  local_activate_ok = 0;
 
   /* Now see if we can get the drift file opened */
   drift_file = CNF_GetDriftFile();
@@ -245,7 +248,8 @@ REF_Initialise(void)
 
   correction_time_ratio = CNF_GetCorrectionTimeRatio();
 
-  enable_local_stratum = CNF_AllowLocalReference(&local_stratum, &local_orphan, &local_distance);
+  enable_local_stratum = CNF_AllowLocalReference(&local_stratum, &local_orphan,
+                                                 &local_distance, &local_activate);
   UTI_ZeroTimespec(&local_ref_time);
 
   leap_when = 0;
@@ -1132,7 +1136,7 @@ REF_GetReferenceParams
  double *root_dispersion
 )
 {
-  double dispersion, delta;
+  double dispersion, delta, distance;
 
   assert(initialised);
 
@@ -1142,11 +1146,15 @@ REF_GetReferenceParams
     dispersion = 0.0;
   }
 
+  distance = our_root_delay / 2 + dispersion;
+
+  if (local_activate == 0.0 || (are_we_synchronised && distance < local_activate))
+    local_activate_ok = 1;
+
   /* Local reference is active when enabled and the clock is not synchronised
      or the root distance exceeds the threshold */
-
   if (are_we_synchronised &&
-      !(enable_local_stratum && our_root_delay / 2 + dispersion > local_distance)) {
+      !(enable_local_stratum && distance > local_distance)) {
 
     *is_synchronised = 1;
 
@@ -1158,7 +1166,7 @@ REF_GetReferenceParams
     *root_delay = our_root_delay;
     *root_dispersion = dispersion;
 
-  } else if (enable_local_stratum) {
+  } else if (enable_local_stratum && local_activate_ok) {
 
     *is_synchronised = 0;
 
@@ -1258,12 +1266,13 @@ REF_ModifyMakestep(int limit, double threshold)
 /* ================================================== */
 
 void
-REF_EnableLocal(int stratum, double distance, int orphan)
+REF_EnableLocal(int stratum, double distance, int orphan, double activate)
 {
   enable_local_stratum = 1;
   local_stratum = CLAMP(1, stratum, NTP_MAX_STRATUM - 1);
   local_distance = distance;
   local_orphan = !!orphan;
+  local_activate = activate;
   LOG(LOGS_INFO, "%s local reference mode", "Enabled");
 }
 
diff --git a/reference.h b/reference.h
index 73454d4..2eddcae 100644
--- a/reference.h
+++ b/reference.h
@@ -185,7 +185,7 @@ extern void REF_ModifyMaxupdateskew(double new_max_update_skew);
 /* Modify makestep settings */
 extern void REF_ModifyMakestep(int limit, double threshold);
 
-extern void REF_EnableLocal(int stratum, double distance, int orphan);
+extern void REF_EnableLocal(int stratum, double distance, int orphan, double activate);
 extern void REF_DisableLocal(void);
 
 /* Check if either of the current raw and cooked time, and optionally a
-- 
2.42.0