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 from Orchestrator #69

Closed
MillsyBot opened this issue Sep 24, 2020 · 9 comments
Closed

Signal Handling from Orchestrator #69

MillsyBot opened this issue Sep 24, 2020 · 9 comments

Comments

@MillsyBot
Copy link

First of all: Awesome work, this an amazingly fast and awesome tool. Thanks for sharing this!

#! /bin/bash /usr/local/bin/consul-templaterb --wait=60 --no-vault-renew --vault-addr=${VAULT_ADDR} --consul-addr=${CONSUL_HTTP_ADDR} \ --sig-reload=USR2 --exec "/haproxy/bin/haproxy -f /haproxy/hap.conf" "$@"

and the variables being passed is
"--template", "/haproxy/hap.conf.erb:/haproxy/hap.conf", "--template", "/haproxy/routing.map.erb:/haproxy/routing.map", "--template", "/haproxy/secrets_generator.erb:/haproxy/secrets_generator"

This is a watered down version of our entrypoint running in a Ubuntu18.04 container. Our goal is to run this consul-templaterb rendering HAProxy configs and reloading the process when consul values are changed. This is generally working as expected, however we are running some tests and we are not seeing signal handling as expected.

We tried running consul-templaterb in the background and trapping the signals, however I am not sure that this was the intent of this when you wrote consul-templaterb.

When using an orchestrator what is your suggested deployment/signal handling?

@pierresouchay
Copy link
Contributor

Hello @MillsyBot,

Thanks a lot, it is really great to receive a nice comment about our work and it is a real pleasure to share our work with people being grateful.

We are using daily this tool with signals to regenerate on the fly our configurations for dozens of different very large configurations in many teams with many instances of Prometheus (Prometheus receiving the signal) and this is working for a very long time, so I suspect a big bug would have been notified, but it is still possible, especially since we are not using very high --wait values (max is around 10s).

I also added the signal reload for your exact usage some time ago, so it is exactly intended to do so. But there are a few warnings (that are supposed to be explained in the README.md, but I take the time to re-explain (and feel free to modify the README in a PR if it is not clear).

The most common problem

First, in your example, you are using --wait 60. This means that templates are evaluated every 1 minute, it has several impacts: it means that the template will be generated at a maximum of 1/m, so the signals will be sent every second at the maximum, whatever the number of changes/s. I suspect this is what you want, but globally, even in a very noisy cluster, haproxy will reload its config every 60s max. I never tried to use it with such high values in production (while it should not have any impact, it is still possible).

Second, the order of --sig-reload and --exec are very important:

if you consider this example:

consul-templaterb --exec /my/monitoring --sig-reload NONE --exec "python -m http.server 8080" [somefiles]

/my/monitoring and python servers would be launched once all files are properly generated and will receive signals each time one of those files is changing. However:

  • /my/monitoring is supposed to receive SIGHUP (the default value), because -sig-reload is after --exec /my/monitoring
  • python -m http.server 8080 will receive no signal (because --sig-reload NONE is before --exec python command).

It means that for instance appending --sig-reload TERM at the end of your commands has no effect because no other --exec flag is present).
This behavior, while powerful, is not very common (and I have been bitten a few times with it myself), and several times, when looking at it, I did not understand while my python command did receive SIGHUP while I wanted NONE, appended at the end of my command line.

Another possible issue

From the doc:

If you generate several files at the same time, the signal will be sent only once the rendering of all templates is completed, so if your process is using several configuration files, all files will be modified and consistent before the signal is sent to process.

It means that the signal is supposed to be generated when all the files are considered not dirty, it means all the data for all the files have been retrieved and properly computed.

Consider this example:

<%
counts = {}
services.each do |service_name, tags|
  counts[service_name] = service(service_name).count
end
%><%= JSON.pretty_print(counts) %>

If you have a cluster where 1 service is renamed every 30s and you used --wait 60, for instance:
service-30, then service-60, service-90 [...] service-3600

At startup, consul-templaterb ignores --wait and try to fetch data as fast as possible. But once the template has been rendered, it would watch.

every 60s, the template is evaluated, then wait...

so, at t+60: consul-templaterb finds new services service-30 and service-60, if schedules the retrival of those 2 additional services, mark the template has dirty (incomplete), does not render it and wait for 60s.
at t+120, consul-templaterb did get values for service-30 and service-60, but in the meantime, service-90 and service-120 have been added, so values are here, but 2 new schedules have been added service-90 and service-120, so template is still dirty, rescheduled in 60s... still no signal sent, if the list of services is changing fast enought and the --wait 60s is there, you might end with your templates being all rendered not that often, thus signal would be never sent, or very rarely. Note that in order to send the signal, consul-templaterb sends the signal ONLY if all templates are non-dirty. The fact that you are using several templates, means that at least one of those templates is not rendered, nothing happens

This might be fixed in at least 3 ways:

  • Hack the file consul-templaterb and change https://github.com/criteo/consul-templaterb/blob/master/bin/consul-templaterb#L69 to use min-value = 180 (refresh at the maximum the list of services every 3 minutes) => would work for my example, since the value would be greater than 60s, new services would never popup every 30s and it would converge with a maximum 60s. If it is the case, a clean fix would be to be able to override from command line those values and it would work in any case. Another possible fix would be to have values here that are always at least 1.5 times the --wait value
  • Do not use signals and use a binary instead, in this binary ensure to reload the command ONLY if some time has been spent. So, you could use --wait 60 BUT reload haproxy only every 60s, example --template haproxy.conf.erb:haproxy.conf::./reload_haproxy_when_needed.sh

with ./reload_haproxy_when_needed.shbeing (not tested):

#!/bin/bash

# Test if .last_reload is older than 1 min
if test "`find .last_reload -mmin +1`"
then
  kill -USR2 $(pidof haproxy)
  touch .last_reload
fi

=> thus, each time haproxy.conf would be modified, the script would be launch, but only reload haproxy every 60s max, meaning that you could use --wait 1 to have faster changes.

If it something else

You might consider running it with debug flags (which displays if there are some dirty templates), and if you don't find, try posting the debug messages here.

About --wait

wait is a bit tricky as it both handles files changes and evaluation of templates. This is clearly not the best idea I had to link both, and initially, it was more about either reducing a bit CPU usage on very complex templates (but not changing data all the time) OR avoid reloading the process all the time (but more with 10s than 60s).

This might be fixed by having an option to delay --sig-reload to launch only every x seconds (might not be that hard) and decorrelate it from --wait.

I would be glad to accept such PR if you are interested

Best
Pierre

pierresouchay added a commit that referenced this issue Sep 25, 2020
…y signals to subprocess.

This avoids for instance reloading too much a HAProxy configuration
without having to play with `-w` as described in issue
[#69](#69)
@pierresouchay
Copy link
Contributor

Hello @MillsyBot,

I added support from what I described in my previous comment in 70808c6

This will let you use -w 1 (render every second), but send to the process signals every 60s, using the new flag -W 60,
will be available in the next coming release (in a few minutes). This might solve your issues if my theory is correct.

pierresouchay added a commit that referenced this issue Sep 25, 2020
NEW FEATURES:

 * Added `-W` or `--wait-between-reload-signal` to avoid sending too many signals
   to a process executed. This avoids for instance reloading too much a HAProxy configuration
   without having to play with `-w` as described in [#69](#69)

BUGFIX:

 * Removed warnings at runtime with Ruby 2.7+
 * Minor JS fix in Consul-UI (Added missing unused parameter to function `serviceTitleGenerator`)
@robloxrob
Copy link

Thanks for all the love here! This really makes my day.

@pierresouchay
Copy link
Contributor

@robloxrob glad it helps

@MillsyBot
Copy link
Author

@pierresouchay WOW, thank you so much for the detailed response, and for releasing a new version addressing my issues! So cool, I will buy you a beer at the next HAProxy Conf (we meet there last year).

Out of curiosity: what type of performance impact are you seeing with HAProxy reloading every 1sec? Are the impacts significant to connection reuse when the FD are constantly passing from one process to another?

@pierresouchay
Copy link
Contributor

@MillsyBot It really depends on the versions. In my tests a few years ago (Version 1.7.x), it used to break many things (including opened connections, so while it is ok to loose a connection or 2 every minute, not acceptable every sec), in recent, HAProxy has been doing lots of work to avoid this (esp. with 2.x releases), so I suspect it is not such a big issue with recent kernels and recent HAProxy versions.

On our side, we know use systems interconnected with Consul to pilot HAProxy (most notably: https://github.com/haproxytech/haproxy-consul-connect that uses dataplane recent additions to change on the fly HAProxy config)

But anyway, we also had this exact same issue with our prometheus instances (we used -wait 10s as well), so I think it is a welcomed addition anyway, avoiding lots of crappy workarounds.

@pierresouchay
Copy link
Contributor

@MillsyBot Don't forget to tell me if it is fixing your issue, in that case, please close the issue!
Thank you!

@MillsyBot
Copy link
Author

@pierresouchay Super helpful.

@MillsyBot
Copy link
Author

@chuckyz says thanks as well.

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

No branches or pull requests

3 participants