Re: [chrony-dev] [PATCH] local threshold option
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 +, 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
Date: Mon, 25 Mar 2024 19:05:52 +
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:
hand
Re: [chrony-dev] [PATCH] local threshold option
On Fri, Mar 29, 2024 at 01:44:34AM +, Andy Fiddaman wrote:
> 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.
I applied the patch with some fixes. I hope that's ok.
> @@ -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;
The corresponding change is missing in client.c
>/* 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)) {
This is missing local_activate_ok to prevent unsynchronized status
when the distance check passes but activation not. I added some tests
for that in a separate commit, so hopefully it works as expected.
>
> *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;
>
Thanks,
--
Miroslav Lichvar
--
To unsubscribe email chrony-dev-requ...@chrony.tuxfamily.org with "unsubscribe"
in the subject.
For help email chrony-dev-requ...@chrony.tuxfamily.org with "help" in the
subject.
Trouble? Email listmas...@chrony.tuxfamily.org.
Re: [chrony-dev] [PATCH] local threshold option
On Thu, 28 Mar 2024, Miroslav Lichvar wrote:
> On Mon, Mar 25, 2024 at 07:10:31PM +, 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
Date: Mon, 25 Mar 2024 19:05:52 +
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
*
Re: [chrony-dev] [PATCH] local threshold option
On Mon, Mar 25, 2024 at 07:10:31PM +, 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?
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?
> --- a/candm.h
> +++ b/candm.h
> @@ -237,6 +237,7 @@ typedef struct {
>int32_t stratum;
>Float distance;
>int32_t orphan;
> + Float threshold;
>int32_t EOR;
> } REQ_Local;
This is an incompatible change in the request, so there should be also
a new code defined (REQ_LOCAL3). You could also add a few reserved
fields to the command, so next time it could keep the code if the
original behavior is preserved by interpreting the old reserved
field.
> @@ -1699,6 +1699,11 @@ An example of the directive is:
>
> local stratum 10 orphan distance 0.1
>
> +*threshold* _distance_:::
> +This option sets an activating threshold for the local reference. The local
> +reference will not be used until the root distance drops below the configured
> +threshold once. This can be used to prevent the local reference being
> activated
> +on a server which has never been synchronised with an upstream server.
This should mention the special value of 0 and that it is the default.
> @@ -1158,7 +1163,11 @@ REF_GetReferenceParams
> *root_delay = our_root_delay;
> *root_dispersion = dispersion;
>
> - } else if (enable_local_stratum) {
> +if (distance > 0 && distance < local_threshold)
> + local_threshold_met = 1;
This should be earlier in the function so the new condition below can
trigger on the first pass?
> +
> + } else if (enable_local_stratum &&
> + (local_threshold == 0 || local_threshold_met)) {
Please use 0.0 in places where a floating-point variable is compared.
Thanks,
--
Miroslav Lichvar
--
To unsubscribe email chrony-dev-requ...@chrony.tuxfamily.org with "unsubscribe"
in the subject.
For help email chrony-dev-requ...@chrony.tuxfamily.org with "help" in the
subject.
Trouble? Email listmas...@chrony.tuxfamily.org.
Re: [chrony-dev] [PATCH] local threshold option
On Thu, 21 Mar 2024, Miroslav Lichvar wrote:
> > The first, "local oncesynced" prevents the local reference being activated
> > unless time was synchronised at some point in the past.
>
> This makes sense to me, although I think it might be better to
> generalize it as an activating root distance, which would completement
> the distance option (threshold).
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
Thanks,
Andy
From d094cb53ab1632fc741cb60cf92520e9d3c84880 Mon Sep 17 00:00:00 2001
From: Andy Fiddaman
Date: Mon, 25 Mar 2024 19:05:52 +
Subject: [PATCH] Add "local threshold" option
---
candm.h | 1 +
client.c | 5 +++--
cmdmon.c | 3 ++-
cmdparse.c | 6 +-
cmdparse.h | 2 +-
conf.c | 6 --
conf.h | 2 +-
doc/chrony.conf.adoc | 5 +
reference.c | 22 --
reference.h | 2 +-
10 files changed, 39 insertions(+), 15 deletions(-)
diff --git a/candm.h b/candm.h
index 0894ad5..64e3200 100644
--- a/candm.h
+++ b/candm.h
@@ -237,6 +237,7 @@ typedef struct {
int32_t stratum;
Float distance;
int32_t orphan;
+ Float threshold;
int32_t EOR;
} REQ_Local;
diff --git a/client.c b/client.c
index 0231b9e..0674263 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, threshold = 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, &threshold)) {
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.threshold = UTI_FloatHostToNetwork(threshold);
return 1;
}
diff --git a/cmdmon.c b/cmdmon.c
index 716775f..464aeb5 100644
--- a/cmdmon.c
+++ b/cmdmon.c
@@ -531,7 +531,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.threshold));
} else {
REF_DisableLocal();
}
diff --git a/cmdparse.c b/cmdparse.c
index 0a80fc0..d811948 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
*threshold)
{
int n;
char *cmd;
*stratum = 10;
*distance = 1.0;
+ *threshold = 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, "threshold")) {
+ if (sscanf(line, "%lf%n", threshold, &n) != 1)
+return 0;
} else {
return 0;
}
diff --git a/cmdparse.h b/cmdparse.h
index 095a8e2..3d6bbe0 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 *threshold);
/* Remove extra white-space and comments */
extern void CPS_NormalizeLine(char *line);
diff --git a/conf.c b/conf.c
index 8849bdc..7af484b 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_threshold;
/* 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, &loc
