diff mbox series

package/ntpsec: add systemd unit file

Message ID 20220728113422.6739-1-guillaume.bressaix@gmail.com
State Changes Requested
Headers show
Series package/ntpsec: add systemd unit file | expand

Commit Message

Guillaume Bres July 28, 2022, 11:34 a.m. UTC
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
---
Use -d to daemonize, use -g to allow a "big leap".
Prior synchronization, system date is usually set to 01/01/1970.
By default, ntpsec does not allow that big of a leap,
and we never obtain synchronization without -g.
Ideally, you want some sort of mechanism to only use
-g on first (ever) boot.
---
 package/ntpsec/ntpd.service | 10 ++++++++++
 package/ntpsec/ntpsec.mk    |  5 +++++
 2 files changed, 15 insertions(+)
 create mode 100644 package/ntpsec/ntpd.service

Comments

Yann E. MORIN July 29, 2022, 8:12 p.m. UTC | #1
Guillaume, All,

On 2022-07-28 13:34 +0200, Guillaume W. Bres spake thusly:
> Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
> ---
> Use -d to daemonize, use -g to allow a "big leap".
> Prior synchronization, system date is usually set to 01/01/1970.
> By default, ntpsec does not allow that big of a leap,
> and we never obtain synchronization without -g.
> Ideally, you want some sort of mechanism to only use
> -g on first (ever) boot.

Thos explanations should not be after a --- line, as they rally explain
the change, so really must be oin the commit log, not a post-commit
note.

For the -g option, I would rephrase as such:

    Devices that have no RTC, or one that is so bad that it drifts very
    fast, the system time prior to synchronisation is ways off, so that
    ntpsec refuses to apply a giant leap to the current time, unless
    forced with -g.

Idally, the user should be able to easy override that, so I'd add an
environment file where the user can just set additional options (or
remove them); see what it does with my final proposal, below...

However, I think using the -d option as you did is incorrect.

Indeed, the default for a systemd service unit, is Type=simple. This
means that systemd does expect the program in ExecStart to be ready as
soon as systemd calls execve() on it. If the program forks and the
parent exits, then system will consider the system to be terminated, but
will whine that there is a lingering process in the cgroup (to be
confirmed). With Type=simple, its systemd that is responsible for the
daemonisation of the program.

The correct solution in this is either one of the following:

 1. ntpsec is left in the foreground, and Type is explicitly set to
    'simple' (or better yet, 'exec', and we do not use -d

 2. ntpsec is set to fork with -d, but then Type is set to 'forking'

The second solution is nice when the service is going to be used by
dependent units (e.g. a DB server that will be used by further units
later in the boot); in tha case, the program is suposed to fork after it
is ready to server requests, so solution 2 is nice.

However, for ntpsec, I don't think it makes much sense, because it won't
"serve" anything locally. Still, I think it is still better to use
solution 2, because, supposedly, ntpsec only forks when it has
eventually done its initialisation step, so we know it is mostly ready.

So:

    [Service]
    Type=forking
    EnvironmentFile=/etc/defaults/ntpsec
    ExecStart=/usr/sbin/ntpd -d $NTPSEC_OPTIONS

and then in /etc/defaults/ntpsec:

    NTPSEC_OPTIONS="-g"

Care to check, test, and resubmit, please.

Regards,
Yann E. MORIN.

> ---
>  package/ntpsec/ntpd.service | 10 ++++++++++
>  package/ntpsec/ntpsec.mk    |  5 +++++
>  2 files changed, 15 insertions(+)
>  create mode 100644 package/ntpsec/ntpd.service
> 
> diff --git a/package/ntpsec/ntpd.service b/package/ntpsec/ntpd.service
> new file mode 100644
> index 0000000..3987085
> --- /dev/null
> +++ b/package/ntpsec/ntpd.service
> @@ -0,0 +1,10 @@
> +[Unit]
> +Description=NTPSec
> +After=network.target
> +Conflicts=systemd-timesyncd.service
> +
> +[Service]
> +ExecStart=/usr/sbin/ntpd -d -g
> +
> +[Install]
> +WantedBy=multi-user.target
> diff --git a/package/ntpsec/ntpsec.mk b/package/ntpsec/ntpsec.mk
> index a0d0662..a0fc49b 100644
> --- a/package/ntpsec/ntpsec.mk
> +++ b/package/ntpsec/ntpsec.mk
> @@ -62,6 +62,11 @@ define NTPSEC_INSTALL_INIT_SYSV
>  	$(INSTALL) -D -m 755 package/ntpsec/S49ntpd $(TARGET_DIR)/etc/init.d/S49ntpd
>  endef
>  
> +define NTPSEC_INSTALL_INIT_SYSTEMD
> +	$(INSTALL) -D -m 0644 package/ntpsec/ntpd.service \
> +		$(TARGET_DIR)/usr/lib/systemd/system/ntpd.service
> +endef
> +
>  define NTPSEC_USERS
>  	ntp -1 ntp -1 * - - - ntpd user
>  endef
> -- 
> 1.8.3.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
James Hilliard July 29, 2022, 8:16 p.m. UTC | #2
On Thu, Jul 28, 2022 at 5:35 AM Guillaume W. Bres
<guillaume.bressaix@gmail.com> wrote:
>
> Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
> ---
> Use -d to daemonize, use -g to allow a "big leap".
> Prior synchronization, system date is usually set to 01/01/1970.
> By default, ntpsec does not allow that big of a leap,
> and we never obtain synchronization without -g.
> Ideally, you want some sort of mechanism to only use
> -g on first (ever) boot.
> ---
>  package/ntpsec/ntpd.service | 10 ++++++++++
>  package/ntpsec/ntpsec.mk    |  5 +++++
>  2 files changed, 15 insertions(+)
>  create mode 100644 package/ntpsec/ntpd.service
>
> diff --git a/package/ntpsec/ntpd.service b/package/ntpsec/ntpd.service
> new file mode 100644
> index 0000000..3987085
> --- /dev/null
> +++ b/package/ntpsec/ntpd.service
> @@ -0,0 +1,10 @@
> +[Unit]
> +Description=NTPSec
> +After=network.target
> +Conflicts=systemd-timesyncd.service
> +
> +[Service]
> +ExecStart=/usr/sbin/ntpd -d -g

You probably need to turn off dnssec validation for hostname lookups to work
prior to ntp syncing.

You can do that like this:
https://github.com/buildroot/buildroot/blob/2022.05.1/package/openntpd/ntpd.service#L8-L11

> +
> +[Install]
> +WantedBy=multi-user.target
> diff --git a/package/ntpsec/ntpsec.mk b/package/ntpsec/ntpsec.mk
> index a0d0662..a0fc49b 100644
> --- a/package/ntpsec/ntpsec.mk
> +++ b/package/ntpsec/ntpsec.mk
> @@ -62,6 +62,11 @@ define NTPSEC_INSTALL_INIT_SYSV
>         $(INSTALL) -D -m 755 package/ntpsec/S49ntpd $(TARGET_DIR)/etc/init.d/S49ntpd
>  endef
>
> +define NTPSEC_INSTALL_INIT_SYSTEMD
> +       $(INSTALL) -D -m 0644 package/ntpsec/ntpd.service \
> +               $(TARGET_DIR)/usr/lib/systemd/system/ntpd.service
> +endef
> +
>  define NTPSEC_USERS
>         ntp -1 ntp -1 * - - - ntpd user
>  endef
> --
> 1.8.3.1
>
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
diff mbox series

Patch

diff --git a/package/ntpsec/ntpd.service b/package/ntpsec/ntpd.service
new file mode 100644
index 0000000..3987085
--- /dev/null
+++ b/package/ntpsec/ntpd.service
@@ -0,0 +1,10 @@ 
+[Unit]
+Description=NTPSec
+After=network.target
+Conflicts=systemd-timesyncd.service
+
+[Service]
+ExecStart=/usr/sbin/ntpd -d -g
+
+[Install]
+WantedBy=multi-user.target
diff --git a/package/ntpsec/ntpsec.mk b/package/ntpsec/ntpsec.mk
index a0d0662..a0fc49b 100644
--- a/package/ntpsec/ntpsec.mk
+++ b/package/ntpsec/ntpsec.mk
@@ -62,6 +62,11 @@  define NTPSEC_INSTALL_INIT_SYSV
 	$(INSTALL) -D -m 755 package/ntpsec/S49ntpd $(TARGET_DIR)/etc/init.d/S49ntpd
 endef
 
+define NTPSEC_INSTALL_INIT_SYSTEMD
+	$(INSTALL) -D -m 0644 package/ntpsec/ntpd.service \
+		$(TARGET_DIR)/usr/lib/systemd/system/ntpd.service
+endef
+
 define NTPSEC_USERS
 	ntp -1 ntp -1 * - - - ntpd user
 endef