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

[Andy Fiddaman <illumos@xxxxxxxxxxxx>]

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.

Thanks,

Andy

From 299716042e487bbc7203322243769ff81526052d 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             |  5 +++--
 cmdmon.c             |  8 +++++---
 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, 54 insertions(+), 26 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..1893df7 100644
--- a/client.c
+++ b/client.c
@@ -755,11 +755,11 @@ 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");
@@ -771,6 +771,7 @@ process_cmd_local(CMD_Request *msg, char *line)
   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..aaf6996 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 */
 };
 
 /* ================================================== */
@@ -531,7 +532,8 @@ handle_local(CMD_Request *rx_message, CMD_Reply *tx_message)
   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),
+                    UTI_FloatNetworkToHost(rx_message->data.local.activate));
   } else {
     REF_DisableLocal();
   }
@@ -1648,8 +1650,8 @@ read_from_cmd_socket(int sock_fd, int event, void *anything)
         case REQ_SETTIME:
           handle_settime(&rx_message, &tx_message);
           break;
-        
-        case REQ_LOCAL2:
+
+        case REQ_LOCAL3:
           handle_local(&rx_message, &tx_message);
           break;
 
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..c9b48c7 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 */
+  { 0, 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