Send SIGUSR1 to dnsmasq periodically

[DamianSawicki]

This adds a feature flag --logInterval for periodic triggering dnsmasq to log its statistics by sending SIGUSR1 to it.

The new motivation for adding this feature comes from dnsmasq changes in v2.90: the new flag --max-tcp-connectionsand the related statistics of TCP connection utilisation added to the log output triggered by SIGUSR1.

The change was benchmarked (see the commit description) and no negative impact was observed.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[DamianSawicki]

/test all

[DamianSawicki]

The failed test fails identically at HEAD (issue #646).

[DamianSawicki]

/cc @aojea

[aojea]

I think you have several changes in the same commit that makes it hard to review.

In addition, what is the motivation to make this configurable, I don't think we should allow people to use this feature if is not for debugging, and if that is the case, you can perfectly do it with a simple bash script

[DamianSawicki]

Thank you very much for a quick reaction, Antonio!

I think you have several changes in the same commit that makes it hard to review.

Yeah, I replace nanny.X() with if err := nanny.X(); err != nil { log } for X in {Kill, Start}, which I'm happy to move to a separate commit, but everything else belongs together: there is a functionality change with the corresponding test, and the new test case requires expanding the testing fixture.

I don't think we should allow people to use this feature

You've mentioned running a bash script. Do I understand correctly that users can also run such a script sending SIGUSR1 to a dnsmasq instance? If so, why don't you want to allow them to use this feature?

In addition, what is the motivation to make this configurable

We can hardcode some interval, but I thought it would be safer to have things configurable, even just for turning the feature on/off.

The general motivation is to improve dnsmasq observability and not to require users to run custom scripts (or reach out to their cloud providers) to gain some insights about their kubedns load. Does it make sense to you?

[aojea]

I can understand if you expose these metrics with a metrics/prometheus endpoints, that allows you to set alerts and integrate with common monitoring stacks ... but you still needs to evaluate the performance implications on sending a periodic signal to the process

[bowei]

Does dnsmasq handle syscall interruption gracefully? e.g. on receipt of signal, the syscall is canceled with EINTR.

[DamianSawicki]

@bowei

Does dnsmasq handle syscall interruption gracefully? e.g. on receipt of signal, the syscall is canceled with EINTR.

Yes, dnsmasq mentions specific actions on SIGUSR1, SIGUSR2, andSIGHUP in its linux man page and it is a very mature piece of software, so I cannot imagine it handling signals incorrectly. Just to be 100% sure, I've just checked the codebase, and there are many of instances of errno == EINTR and errno != EINTR throughout.

@aojea

you still needs to evaluate the performance implications on sending a periodic signal to the process

The statistics logged by dnsmasq on SIGUSR1 contain not just the current utilisation, but also the maximal utilisation since the last SIGUSR1, so I was thinking about using ~5 min intervals in practice (1 min at least, while 1 h would still be better than nothing). The function dump_cache handling SIGUSR1 is cheap (unless dnsmasq runs in debug mode when it does a full cache dump). With such infrequent and minimal additional load and correct handling of EINTR, performance implications should be negligible.

I can understand if you expose these metrics with a metrics/prometheus endpoints, that allows you to set alerts and integrate with common monitoring stacks

For what you describe, the first step is still the current PR, which makes dnsmasq print statistics to logs, which we can later parse and expose with metrics/prometheus endpoints or set up log-based metrics. Do you think the current PR is only sensible if extended with these follow-up steps?

[aojea]

I don't know, it really feels awkward running a process in production that you are sigkilling periodically ... but I don't know if this pattern is also used elsewhere

[DamianSawicki]

that you are sigkilling periodically

For anyone reading this without much context, Antonio meant sending a signal periodically. The signal SUGUSR1 does not kill the process, it's just a way of communicating with it. Dnsmasq is a rather old piece of software, and that's one way of communication that it supports, but I agree that it may feel awkward today.

In any case, PRs to this repo are very rare, which means the resources are limited (two PRs with vulnerability fixes have been waiting for attention since July). The present PR (which actually was on my to-do list for months) is a rather low-hanging fruit to improve kubedns observability which is among priorities to help with supportability. Let's not rush and see what others have to say.

[aojea]

problems on typing fast, thanks for clarifying, I meant what Damian said 😅

[pwdtr]

+1, Good work, Damian!

[bowei]

If we can do a test and include the results in the commit message to demonstrate that sending the signal is benign:

• Send constant stream of dnsperf requests to dnsmasq
• Enable the periodic interval of SIGUSR1

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[DamianSawicki]

/remove-lifecycle stale

I need a bit of free time to run the benchmark suggested by Bowei.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DamianSawicki

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [DamianSawicki]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[DamianSawicki]

If we can do a test and include the results in the commit message to demonstrate that sending the signal is benign:

• Send constant stream of dnsperf requests to dnsmasq
• Enable the periodic interval of SIGUSR1

I benchmarked the change with SIGUSR1 sent as frequently as every second for both UDP and TCP (with up to 50 concurrent TCP connections) and observed no negative performance impact. Added details to commit description.

Please review 🙏

[DamianSawicki]

This is a (+70, -3) change, but since Antonio mentioned above that it includes several logical changes, I split it into 3 commits to further facilitate review:

• (+6, -3) clean-up,
• (+28, 0) main change,
• (+36, 0) test.

[DamianSawicki]

/ok-to-test

[DamianSawicki]

Hey @aojea @bowei @MrHohn! The PR is now ready for the final approval (and review perhaps, although there are no code changes compared to October: just rebased, added benchmarking results to a commit message, and spit the original commit in 3 parts), but since the PR has been inactive for almost 5 months, I'm pinging you so that it's not missed.