search

containers / aardvark-dns

Authoritative dns server for A/AAAA container records. Forwards other request to host's /etc/resolv.conf
Apache License 2.0
176 stars 31 forks source link

Don't tear down all server threads on SIGHUP #449

Closed kyrias closed 4 months ago

kyrias commented 5 months ago

Instead we parse the new config, replace the in-memory configuration state, stop all old threads for IPs which are no longer in the configuration, and start any new ones for IPs which were added.

Resolves #389.

kyrias commented 5 months ago

Thanks for doing the work. I am not familiar with the async changes that you did and I a short on time so it may take a bit until I get this properly reviewed.

There aren't really any real async changes here to be fair, but no hurry.

Though the way aardvark is currently structured there is literally zero advantage to using any async code at all, since it's currently spawning a new tokio runtime for each thread and then essentially running a single task of straight-line code, so currently it's sort of getting the worst of both worlds.

I would argue that it should either be restructured to have a single multithreaded runtime with a task per server listened IP, or be restructured to be entirely sync.

In the meantime did you actually test that this fixes the intermittent dns issues?

Yes, we cannot reproduce that issue with these changes.

Luap99 commented 5 months ago

I would argue that it should either be restructured to have a single multithreaded runtime with a task per server listened IP, or be restructured to be entirely sync.

Yes agreed. Keep in mind this all was written by us when we had basically no rust experience so things not working properly here is no surprise. I think async makes sense but yeah it should be one runtime.

Luap99 commented 5 months ago

But nothing you have to do as the issues are pre-existing, I will try to get this reviewed properly in the next days

Luap99 commented 5 months ago

/packit test

Luap99 commented 5 months ago

/packit build

Luap99 commented 4 months ago

/packit test

openshift-ci[bot] commented 4 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyrias, Luap99

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](https://github.com/containers/aardvark-dns/blob/main/OWNERS)~~ [Luap99] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
mheon commented 4 months ago

/lgtm

packit-as-a-service[bot] commented 4 months ago

podman system tests failed on RHEL. @containers/packit-build please check.

svenstaro commented 4 months ago

Could we please get a release with this in it soon? Currently aardvark-dns is actually pretty broken.

mheon commented 4 months ago

We're planning a Podman release the week of May 20th, so our next planned Netavark and Aardvark drops would coincide with that. We could do an async release earlier if this is critical.

svenstaro commented 4 months ago

It seems to me this warrants a patch release before that seeing that this is currently really broken.

cpetestewart commented 4 months ago

PLEASE get us a patch. I have been tearing my hair out for the last few months trying to track down this issue ever since our org had to switch from docker to podman. It was only now I found this PR that explains the problem.

cpetestewart commented 3 months ago

Is this now in the latest podman release? When I pulled down a fresh image, it said it was updated 12 days ago.

mheon commented 3 months ago

No. We are planning a release next week (likely wednesday) to coincide with Podman 5.1