Add systemd socket listener activation

[DaAwesomeP]

I called the function UseActivatedSocketAndServe. Let me know if I should change that. Uses the latest version of the systemd module (v22).

Discussion at node_exporter: prometheus/node_exporter#2393

Per CONTRIBUTING.MD, @SuperQ @roidelapluie

Signed-off-by: Perry Naseck [email protected]

[roidelapluie]

It sounds like we might need to change the tls config in kingpin flags to be a struct and add a flag for this, then pass it to ListenAndServe and do the right thing here.

The flag should be optional as we don't want it e.g. in Prometheus and non-linux systems.

[SuperQ]

It would be nice if this was integrated into the web ListenAndServe method. The idea of this toolkit is to minimize the boilerplate needed in exporters by having opinionated setup.

One thing I was thinking about here is we currently only support one listenAddress. There have been some requests to allow multiple listen addresses to be specified. We could do a bit of a breaking change to support both socket activation and multiple addresses.

[DaAwesomeP]

I see, so allow for multiple listenAddress and one address is a reserved keyword to enable socket activation?

[DaAwesomeP]

Systemd can also provide multiple activated sockets (of which I am currently erroring if there are multiple), so that would play very nicely into this.

[DaAwesomeP]

I took a try at it, but I think I need some guidance with how you would like this to work to support multiple sockets. Please forgive any incorrect assumptions; I am still quite new to Go.

It seems that Serve() can only take one listener, which means this will require multiple Serve() instances (unless there is some other method). Should I return an array of servers instead of a single one? That is definitely a big breaking change.

[DaAwesomeP]

Ah, ok this will require proper concurrency. I will keep reading and experimenting and hopefully end up at a pull, but this is definitely outside my current skill-level. Anyone who stumbles upon this pull in the meantime should please feel free to contribute it themselves and I can rebase and incorporate the systemd activation part.

[roidelapluie]

Could a simple unix socker approach work too or do we need to be systemd specific?

[DaAwesomeP]

The difficulty seems to be with having multiple HTTP server instances, not the listeners. I am making progress and should hopefully have something to test next week.

[DaAwesomeP]

OK! This is now working for multiple systemd activated sockets. It uses the existing Serve() function, so it is not a breaking change. It can theoretically support multiple non-systemd listeners as well, but I still need to add that function.

But adding that function doesn't really fit well into the current way exporters use the toolkit. The current way seems to be:

1. Exporters set up kingpin flags like --web.listen-address
2. Exporters set up &http.Server{} with the specific address
3. Exporters call web.ListenAndServ()

My proposal would be to:

1. Move these flags to this module
2. Allow multiple instances of --web.listen-address for listening on multiple ports without systemd activation
3. Provide a function web.ListenAndServAll() (or some other name) that parses these flags and begins serving on all the necessary listeners and ports.

This would be a breaking change, but it would ensure that all exporters start this part the same way and that any future changes or features would not introduce breaking changes to the individual exporter code.

[DaAwesomeP]

@roidelapluie @SuperQ ping, please provide feedback with what you prefer for implementing the required options.

[roidelapluie]

if we have a ListenAndServAll, then we don't need to break existing setups.

It is acceptable to me to change what https://github.com/prometheus/exporter-toolkit/blob/master/web/kingpinflag/flag.go returns. It can return a pointer to a struct.

[DaAwesomeP]

Great, I will try this out this coming week.

[DaAwesomeP]

Hello! I have completed the changes. Please let me know if this looks good or if I should make any adjustements.

I have implemented:

• A struct for exporter-toolkit flags. This removes the need for the exporter implementation to worry about flags like web.listen-address and instead they only need to do something like:

  toolkitFlags = kingpinflag.AddFlags(kingpin.CommandLine)

• ListenAndServe now requires much less boilerplate in the exporter implementation. Exporters no longer need to worry about specifying a port to listen on, etc. It is simply:

  server := &http.Server{}
    if err := web.ListenAndServe(server, toolkitFlags, logger); err != nil {
    	level.Error(logger).Log("err", err)
    	os.Exit(1)
    }

  This is a major breaking change. However, this method should be more resilient to breaking changes going forward.
• ListenAndServe now uses toolkitFlags (FlagStruct) to determine ports to listen on, systemd socket activation, and TLS config file path.
• Implementing new flags and features are now much less likely to induce breaking changes in exporter implementations, as new flags may be added to FlagStruct without changing the exporter implementation. Help messages for exporter-toolkit flags will now also be unified across all exporters (for flags like web.listen-address).
• A user may specify multiple --web.listen-address to listen on multiple port listeners.
• A user may optionally specify --web.systemd-socket to listen with systemd activation instead of port listeners.

I will push a working example of node_exporter to prometheus/node_exporter#2393 shortly.

[DaAwesomeP]

Requesting Review from @discordianfish, @roidelapluie, or @SuperQ

[SuperQ]

Minor nit on docs, otherwise LGTM.

[DaAwesomeP]

@SuperQ Updated and pushed minor doc change!

[SuperQ]

Minor nit, but otherwise LGTM.

[DaAwesomeP]

ping @roidelapluie @SuperQ are we good to go?

[SuperQ]

Going to close and re-open to try and trigger CI.

[DaAwesomeP]

@SuperQ Now passing tests! 🎉

[SuperQ]

Nice, I think this is ready to go. Thanks for all of the work here, I'm really looking forward to having this done.

But I want to give @roidelapluie one more chance to look at it.

[DaAwesomeP]

@roidelapluie @SuperQ let me know if any further changes are needed! Thanks!

[roidelapluie]

Thanks!