* Re: [Buildroot] [PATCH] package/ntpsec: add systemd unit file
2022-07-28 11:34 [Buildroot] [PATCH] package/ntpsec: add systemd unit file Guillaume W. Bres
@ 2022-07-29 20:12 ` Yann E. MORIN
2022-07-29 20:16 ` James Hilliard
1 sibling, 0 replies; 3+ messages in thread
From: Yann E. MORIN @ 2022-07-29 20:12 UTC (permalink / raw)
To: Guillaume W. Bres; +Cc: buildroot, Peter Seiderer, Thomas Petazzoni
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
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Buildroot] [PATCH] package/ntpsec: add systemd unit file
2022-07-28 11:34 [Buildroot] [PATCH] package/ntpsec: add systemd unit file Guillaume W. Bres
2022-07-29 20:12 ` Yann E. MORIN
@ 2022-07-29 20:16 ` James Hilliard
1 sibling, 0 replies; 3+ messages in thread
From: James Hilliard @ 2022-07-29 20:16 UTC (permalink / raw)
To: Guillaume W. Bres; +Cc: Buildroot, Peter Seiderer, Thomas Petazzoni
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
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 3+ messages in thread