Mailing List Archive

[Bug 2641] Add systemd notify code to to track running server
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

Damien Miller <djm@mindrot.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #2896|0 |1
is obsolete| |
Attachment #2950|0 |1
is obsolete| |
Attachment #3099|0 |1
is obsolete| |

--- Comment #13 from Damien Miller <djm@mindrot.org> ---
Created attachment 3798
--> https://bugzilla.mindrot.org/attachment.cgi?id=3798&action=edit
standalone systemd notifications

This implements the equivalent of sd_notify() without bringing in the
rest of systemd bloat. It it also signal-handler safe, which is not the
case for the originally proposed diffs.

Lightly tested.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

Luca Boccassi <luca.boccassi@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |luca.boccassi@gmail.com

--- Comment #14 from Luca Boccassi <luca.boccassi@gmail.com> ---
Thanks for working on that, will be great to have native support for
the readiness protocol.

One review comment: unless I'm missing it because it's handled outside
of the patch context, after a RELOADING=1, when the reload operation is
complete, a READY=1 needs to be sent too:

https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html#RELOADING=1

While there, it would be really nice if the RELOADING=1 message also
included MONOTONIC_USEC=<timestamp> (CLOCK_MONOTONIC in usec as a
decimal string), which is used for accurate synchronization. IE, write
a string like "RELOADING=1\nMONOTONIC_USEC=1234...". This will enable
the unit to be of Type=notify-reload which adds some nice features.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

Sam James <sam@gentoo.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |sam@gentoo.org

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #15 from Damien Miller <djm@mindrot.org> ---
I think the READY=1 will be sent implicitly after sshd restarts

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #16 from Damien Miller <djm@mindrot.org> ---
(In reply to Luca Boccassi from comment #14)
> While there, it would be really nice if the RELOADING=1 message also
> included MONOTONIC_USEC=<timestamp> (CLOCK_MONOTONIC in usec as a
> decimal string), which is used for accurate synchronization. IE,
> write a string like "RELOADING=1\nMONOTONIC_USEC=1234...". This will
> enable the unit to be of Type=notify-reload which adds some nice
> features.

That's more tricky as the reload is called from signal handler context
and we can't use snprint() there to format the usec part of the
message. We'd have to refactor how sshd manages SIGHUP restarts.

That would make some other things easier, but it's still a bigger
change.

Anyway, if some of the distro people on this bug can report on whether
the patch is okay, then we can move forward with this and finesse it
later.

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

Arkadiusz Mi?kiewicz <arekm@maven.pl> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |arekm@maven.pl

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

Richard W.M. Jones <rjones@redhat.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |rjones@redhat.com

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #17 from Colin Watson <cjwatson@debian.org> ---
I don't see any problems from eyeballing the patch. I've pushed a
version of the Debian packaging with this (and consequent
modifications; we also have a socket activation patch from Ubuntu, but
reworking that to avoid libsystemd wasn't too hard) to
https://salsa.debian.org/ssh-team/openssh/-/tree/without-libsystemd,
though so far I've only checked that it passes the regression tests.

https://salsa.debian.org/ssh-team/openssh/-/jobs/5521815 has .debs for
people who feel comfortable installing things from random CI jobs.
Obviously I don't recommend installing those on production, but it's
probably OK to do so in a container/VM. I'll look more once I've had
some sleep.

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #18 from Colin Watson <cjwatson@debian.org> ---
I've done some testing and this does seem to basically work.

The one thing I'd point out is following on from Luca's comment:
RELOADING=1 is ignored if you don't also send MONOTONIC_USEC=. So if
you're not going to send that (and I understand the reasons), you might
as well not bother sending RELOADING=1 either; we'll just have to stick
with Type=notify rather than Type=notify-reload for now, which wouldn't
be a regression.

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #19 from Luca Boccassi <luca.boccassi@gmail.com> ---
(In reply to Colin Watson from comment #18)
> I've done some testing and this does seem to basically work.
>
> The one thing I'd point out is following on from Luca's comment:
> RELOADING=1 is ignored if you don't also send MONOTONIC_USEC=. So
> if you're not going to send that (and I understand the reasons), you
> might as well not bother sending RELOADING=1 either; we'll just have
> to stick with Type=notify rather than Type=notify-reload for now,
> which wouldn't be a regression.

Mmmh hang on I don't think that should be the case. The MONOTONIC_USEC
is for the Type=notify-reload workflow, that automatically hooks sighup
to the service, and is newer. But RELOADING=1 -> READY=1 by itself
should work with the older workflow where you manually specify an
ExecReload=kill -HUP $MAINPID in the unit.

Let me get your packages and test this.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #20 from Colin Watson <cjwatson@debian.org> ---
Actually, I noticed a slight race here. You're sending the readiness
notification from platform_pre_listen; but, as the name implies, this
is called _before_ the server has started listening. The point of the
readiness protocol is that the notification is only sent once the
server is ready to accept connections.

The notification should be moved to after the listen sockets are bound.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #21 from Colin Watson <cjwatson@debian.org> ---
(In reply to Luca Boccassi from comment #19)
> Mmmh hang on I don't think that should be the case. The
> MONOTONIC_USEC is for the Type=notify-reload workflow, that
> automatically hooks sighup to the service, and is newer. But
> RELOADING=1 -> READY=1 by itself should work with the older workflow
> where you manually specify an ExecReload=kill -HUP $MAINPID in the
> unit.

Ah, you may be right. I was just going by looking at the code and
hadn't actually tested removing RELOADING=1. Probably best to leave it
in then.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #22 from Luca Boccassi <luca.boccassi@gmail.com> ---
(In reply to Colin Watson from comment #21)
> (In reply to Luca Boccassi from comment #19)
> > Mmmh hang on I don't think that should be the case. The
> > MONOTONIC_USEC is for the Type=notify-reload workflow, that
> > automatically hooks sighup to the service, and is newer. But
> > RELOADING=1 -> READY=1 by itself should work with the older workflow
> > where you manually specify an ExecReload=kill -HUP $MAINPID in the
> > unit.
>
> Ah, you may be right. I was just going by looking at the code and
> hadn't actually tested removing RELOADING=1. Probably best to leave
> it in then.

I have tested the packages you published and the reloading notification
is working:

Mar 31 14:34:28 localhost systemd[1]: ssh.service: Trying to enqueue
job ssh.service/reload/replace
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Installed new job
ssh.service/reload as 1333
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Enqueued job
ssh.service/reload as 1333
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Will spawn child
(service_enter_reload): /usr/sbin/sshd
Mar 31 14:34:28 localhost systemd[1]: ssh.service: About to execute:
/usr/sbin/sshd -t
Mar 31 14:34:28 localhost (sshd)[3824]: Found cgroup2 on
/sys/fs/cgroup/, full unified hierarchy
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Forked
/usr/sbin/sshd as 3824 (without CLONE_INTO_CGROUP)
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Changed running ->
reload
Mar 31 14:34:28 localhost systemd[1]: Reloading ssh.service - OpenBSD
Secure Shell server...
Mar 31 14:34:28 localhost (sshd)[3824]: Found cgroup2 on
/sys/fs/cgroup/, full unified hierarchy
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Child 3824 belongs
to ssh.service.
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Control process
exited, code=exited, status=0/SUCCESS (success)
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Running next control
command for state reload.
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Will spawn child
(service_run_next_control): /bin/kill
Mar 31 14:34:28 localhost systemd[1]: ssh.service: About to execute:
/bin/kill -HUP "\$MAINPID"
Mar 31 14:34:28 localhost (kill)[3826]: Found cgroup2 on
/sys/fs/cgroup/, full unified hierarchy
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Forked /bin/kill as
3826 (without CLONE_INTO_CGROUP)
Mar 31 14:34:28 localhost (kill)[3826]: Found cgroup2 on
/sys/fs/cgroup/, full unified hierarchy
Mar 31 14:34:28 localhost sshd[3812]: Received SIGHUP; restarting.
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Got notification
message from PID 3812 (RELOADING=1)
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Child 3826 belongs
to ssh.service.
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Control process
exited, code=exited, status=0/SUCCESS (success)
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Got final SIGCHLD
for state reload.
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Changed reload ->
reload-notify
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Got notification
message from PID 3812 (READY=1)
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Changed
reload-notify -> running
Mar 31 14:34:28 localhost systemd[1]: ssh.service: Job 1333
ssh.service/reload finished, result=done
Mar 31 14:34:28 localhost systemd[1]: Reloaded ssh.service - OpenBSD
Secure Shell server.
Mar 31 14:34:28 localhost sshd[3812]: Server listening on 0.0.0.0 port
22.
Mar 31 14:34:28 localhost sshd[3812]: Server listening on :: port 22.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #23 from Luca Boccassi <luca.boccassi@gmail.com> ---
(In reply to Colin Watson from comment #20)
> Actually, I noticed a slight race here. You're sending the
> readiness notification from platform_pre_listen; but, as the name
> implies, this is called _before_ the server has started listening.
> The point of the readiness protocol is that the notification is only
> sent once the server is ready to accept connections.
>
> The notification should be moved to after the listen sockets are
> bound.

Yes, good catch, this should be fixed as it's important to avoid races
that the notification is delivered after everything is up and running
and ready to process requests.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #24 from Luca Boccassi <luca.boccassi@gmail.com> ---
Created attachment 3801
--> https://bugzilla.mindrot.org/attachment.cgi?id=3801&action=edit
standalone notify patch

The attached patch fixes the issue by creating a platform_post_listen()
hook, as suggested by Colin.
Tested in a Debian testing VM, seems to do the right thing, including
on reloading.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

Luca Boccassi <luca.boccassi@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #3801|0 |1
is obsolete| |

--- Comment #25 from Luca Boccassi <luca.boccassi@gmail.com> ---
Created attachment 3802
--> https://bugzilla.mindrot.org/attachment.cgi?id=3802&action=edit
standalone notify patch

Thinking about it, given there's no external dependency and the runtime
behaviour is a no-op unless the NOTIFY_SOCKET env var is set (which is
only set by systemd or systemd-compatible managers), I don't think the
new autoconf option is needed? There's no downside to always including
the implementation when building on Linux, like it's done with the OOM
adjustments.
New revision of the patch attached does just that.

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #26 from Colin Watson <cjwatson@debian.org> ---
Either version of Luca's patch looks fine to me.

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

Luca Boccassi <luca.boccassi@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #3802|0 |1
is obsolete| |

--- Comment #27 from Luca Boccassi <luca.boccassi@gmail.com> ---
Created attachment 3804
--> https://bugzilla.mindrot.org/attachment.cgi?id=3804&action=edit
standalone notify and timestamp patch

> That's more tricky as the reload is called from signal handler context and we can't use snprint() there to format the usec part of the message. We'd have to refactor how sshd manages SIGHUP restarts.
>
> That would make some other things easier, but it's still a bigger change.

I went back and had a look at this, and unless I am missing something
the reloading message is not being sent from the signal handler?

The handler is sighup_handler which just sets a boolean and returns,
following the usual pattern:

https://anongit.mindrot.org/openssh.git/tree/sshd.c#n298

but the notification message is sent from the platform_pre_restart()
hook, which is called from the main context from the main loop via
sighup_restart():

https://anongit.mindrot.org/openssh.git/tree/sshd.c#n304

This already does some logging, which uses format strings. Also
platform_pre_restart() already calls oom_adjust_restore() which also
uses format strings.

So I went ahead and did the necessary modifications in the latest
version, which also simplified the message handling as it can log
unconditionally now, and added the timestamp too.
I've tested this and seems to work just fine on Debian testing, I can
change ssh.service to Type=notify-reload and reloading works just fine,
including the state transitions.

--
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #30 from Michal Koutný <mkoutny@suse.com> ---
(In reply to Damien Miller from comment #28)
> Good catch about the sighup restart no longer running in a signal
> handler.

(In reply to Damien Miller from comment #13)
> ...
> It it also signal-handler safe, which is not the case for the originally proposed diffs.

The original diff (comment 10) already put the notification in
sighup_restart() not in sighup_handler(), i.e. still the same place
where platform_pre_restart() is called now, not a signal handler
context AFAICS.
platform_* hooks look like the appropriate places for these calls.

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #31 from Luca Boccassi <luca.boccassi@gmail.com> ---
Created attachment 3809
--> https://bugzilla.mindrot.org/attachment.cgi?id=3809&action=edit
standalone notify and timestamp patch

One more change, to support abstract namespace sockets (for containers)
as per protocol defined at
https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Notes

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs
[Bug 2641] Add systemd notify code to to track running server [ In reply to ]
https://bugzilla.mindrot.org/show_bug.cgi?id=2641

--- Comment #34 from Luca Boccassi <luca.boccassi@gmail.com> ---
(In reply to Damien Miller from comment #33)
> Committed as 08f579231cd38 and will be in OpenSSH-9.8, due around
> June/July.

Thank you!

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs