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

auth: refactor health checks monitoring done from LUA records #8249

Merged
merged 4 commits into from
Dec 9, 2019

Conversation

chbruyand
Copy link
Member

Short description

This PR puts TCP/IP and URL health checks in a single thread (instead of 1 thread per check).

This also removes checks that have been unused for a given period of time (currently 1 hour). This will prevent checks from modified or deleted records to keep being performed by the authoritative server (fix #7988).

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@chbruyand chbruyand added this to the auth-4.2.x milestone Aug 29, 2019
@chbruyand chbruyand requested a review from Habbie August 29, 2019 10:44
pdns/lua-record.cc Show resolved Hide resolved
pdns/lua-record.cc Outdated Show resolved Hide resolved
@chbruyand
Copy link
Member Author

Thanks for your review. I added an extra check on insertion when we have the lock and changed how we store items to be deleted.

@pfak
Copy link

pfak commented Aug 31, 2019

This also removes checks that have been unused for a given period of time (currently 1 hour).

Not a fan of this behaviour. But, that being said. I'm not a fan of the current behaviour of not running a check until someones tries to look up the record. Is it possible to make this configurable? I would prefer ALL checks to be run if the record exists, and/or the existing behaviour.

@Someguy123
Copy link
Contributor

The configurable health check would be great - it would help resolve the issue I've just reported: #8261

@sbakhos
Copy link
Contributor

sbakhos commented Sep 5, 2019

If possible I would like to configure the timeout for the health check too. We're looking at implementing this and noticed in our lab a lot of flapping on some of the longer checks (about 500ms).

@chbruyand
Copy link
Member Author

One or two serious reviews would be more than welcome. Also I don't understand why this broke the dnsdist circleci tests :/

@Habbie
Copy link
Member

Habbie commented Sep 12, 2019

Also I don't understand why this broke the dnsdist circleci tests :/

Can you see if a rebase fixes that? We had/have an issue with CircleCI running commit X tests against commit Y builds, and we merged an improvement for that on master yesterday.

@chbruyand
Copy link
Member Author

Also I don't understand why this broke the dnsdist circleci tests :/

Can you see if a rebase fixes that? We had/have an issue with CircleCI running commit X tests against commit Y builds, and we merged an improvement for that on master yesterday.

Looks like it works (c) now...

docs/settings.rst Outdated Show resolved Hide resolved
docs/settings.rst Outdated Show resolved Hide resolved
pdns/common_startup.cc Outdated Show resolved Hide resolved
pdns/lua-record.cc Show resolved Hide resolved
pdns/common_startup.cc Outdated Show resolved Hide resolved
pdns/lua-record.cc Show resolved Hide resolved
@chbruyand
Copy link
Member Author

Thanks for your review. Invalid options (like source) will now trigger an error before being inserted in the check list.

pdns/lua-record.cc Outdated Show resolved Hide resolved
pdns/lua-record.cc Outdated Show resolved Hide resolved
pdns/lua-record.cc Outdated Show resolved Hide resolved
@Habbie Habbie modified the milestones: auth-4.2.x, auth-4.3.0-alpha1 Dec 6, 2019
@Habbie
Copy link
Member

Habbie commented Dec 9, 2019

rebased

@Habbie Habbie merged commit ccd8711 into PowerDNS:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LUA ifurlup health check does not clear checks if URL changes or record is deleted
6 participants