Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal handling #152

Open
zheka64 opened this issue May 24, 2023 · 7 comments
Open

Signal handling #152

zheka64 opened this issue May 24, 2023 · 7 comments

Comments

@zheka64
Copy link

zheka64 commented May 24, 2023

Would like to see signal processing for SIGQUIT and SIGTERM in subsequent versions for proper systemd unit operation. Currently, the systemd unit transitions to a failed state after stopping the service.
Here is a short patch for version 0.21.0:

--- main.go.orig    2023-05-24 16:14:47.781711964 +0300
+++ main.go 2023-05-24 14:09:05.000000000 +0300
@@ -181,6 +181,22 @@
    hup := make(chan os.Signal, 1)
    reloadCh = make(chan chan error)
    signal.Notify(hup, syscall.SIGHUP)
+   term_chan := make(chan os.Signal, 1)
+   signal.Notify(term_chan, syscall.SIGTERM, syscall.SIGQUIT)
+   go func(s <-chan os.Signal) {
+       for {
+           sig := <-s
+           switch sig {
+           case syscall.SIGTERM:
+               level.Info(logger).Log("msg", "Got SIGTERM. Exiting")
+               os.Exit(0)
+           case syscall.SIGQUIT:
+               level.Info(logger).Log("msg", "Got SIGQUIT. Exiting")
+               os.Exit(0)
+           }
+       }
+   }(term_chan)
+
    go func() {
        for {
            select {
@SuperQ
Copy link
Member

SuperQ commented May 24, 2023

I wonder if this should be something we add to the https://github.com/prometheus/exporter-toolkit.

@SuperQ SuperQ transferred this issue from prometheus/snmp_exporter May 24, 2023
@Bogay
Copy link

Bogay commented Jul 11, 2023

I think this issue is also related to prometheus/node_exporter#2356. IMO it's better to graceful shutdown the server instead of exiting 0 directly, and then return to the caller of ListenAndServe to decide how to terminate the process.

@SuperQ how do you think? If that's the proper way to fix this issue, I can open a PR.

@SuperQ
Copy link
Member

SuperQ commented Jul 11, 2023

Yes, I would like add a helper for signal handling here. I've been working on a top level toolkit package that will make the whole bootstrap setup easier.

@hhromic
Copy link

hhromic commented Jul 16, 2023

I was about to open a proposal for exporter-toolkit to adopt shutdownServerOnQuit() from the Push Gateway here:
https://github.com/prometheus/pushgateway/blob/ed566850d02ea75a32d6be3996ccec9dfddca9ff/main.go#L264-L279

I did a scan of Prometheus components and didn't find any of them implementing graceful shutdown, except for the Push Gateway. The PG does it quite nicely.

Adding the above function would allow to very easily add graceful shutdown to exporters like in the Push Gateway:

quitCh := make(chan struct{})
quitHandler := func(w http.ResponseWriter, r *http.Request) {  // handler not required but is good demo
    fmt.Fprintf(w, "Requesting termination... Goodbye!")
    close(quitCh)
}

mux := ...
server := &http.Server{Handler: mux}

go web.ShutdownServerOnQuit(server, quitCh, logger)
err := web.ListenAndServe(server, webConfig, logger)

// In the case of a graceful shutdown, do not log the error.
if err == http.ErrServerClosed {
    level.Info(logger).Log("msg", "HTTP server stopped")
} else {
    level.Error(logger).Log("msg", "HTTP server stopped", "err", err)
}

@SuperQ this could be very easily added to the work you are doing for the top-level package in Run().

@SuperQ
Copy link
Member

SuperQ commented Jul 17, 2023

That looks pretty good. Although we don't use http server mux consistently across exporters.

@hhromic
Copy link

hhromic commented Jul 17, 2023

Ah, the mux part is just an example :). All this function needs is an http.Server instance, a webconfig and a logger. Just like the existing ListenAndServe() function.

Would you like me to open a PR to add it to the 'web'' module?

@SuperQ
Copy link
Member

SuperQ commented Jul 17, 2023

I'm wondering if it would be better in the top level toolkit package, rather than the web package. What do you think @roidelapluie ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants