On Wed, Apr 03, 2024 at 10:39:16PM +0200, Tim Duesterhus wrote:
> As per the `sd_notify` manual:
> 
> > A field carrying the monotonic timestamp (as per CLOCK_MONOTONIC) formatted
> > in decimal in μs, when the notification message was generated by the client.
> > This is typically used in combination with "RELOADING=1", to allow the
> > service manager to properly synchronize reload cycles. See 
> > systemd.service(5)
> > for details, specifically "Type=notify-reload".
> 
> Thus this change allows users with a recent systemd to switch to
> `Type=notify-reload`, should they desire to do so. Correct behavior was
> verified with a Fedora 39 VM.
> 
> see systemd/systemd#25916
> ---
>  src/haproxy.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/haproxy.c b/src/haproxy.c
> index 0fcc3e5416..a5f1e79ef9 100644
> --- a/src/haproxy.c
> +++ b/src/haproxy.c
> @@ -844,8 +844,17 @@ void mworker_reload(int hardreload)
>       }
>  
>  #if defined(USE_SYSTEMD)
> -     if (global.tune.options & GTUNE_USE_SYSTEMD)
> -             sd_notify(0, "RELOADING=1\nSTATUS=Reloading Configuration.\n");
> +     if (global.tune.options & GTUNE_USE_SYSTEMD) {
> +             struct timespec ts;
> +
> +             (void)clock_gettime(CLOCK_MONOTONIC, &ts);
> +
> +             sd_notifyf(0,
> +                        "RELOADING=1\n"
> +                            "STATUS=Reloading Configuration.\n"
> +                            "MONOTONIC_USEC=%" PRIu64 "\n",
> +                        (ts.tv_sec * 1000000ULL + ts.tv_nsec / 1000ULL));
> +     }
>  #endif
>       mworker_reexec(hardreload);
>  }

It looks like they still failed to implement a correct reload status
with this mode. Honestly I don't get it, to me it does not improve
anything :/

Given that the only signal we can send is READY=1 and that only a
failure of a ExecReload= command would set a reloading error, it does
exactly the same as the current kill -USR2 method.

I think only implementing a synchronous `haproxyctl reload` command
which uses the master CLI could improve the situation, only that could
return a failure and emits the error output...

I'm not against merging this, but I don't see any change comparing to the
current model?

-- 
William Lallemand

Reply via email to