[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

plugin/reload: avoid race conditions if imported files change during reload / fix issues with sigusr1 reload #6244

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

plugin/reload: avoid race conditions if imported files change during reload / fix issues with sigusr1 reload #6244

wants to merge 10 commits into from

Conversation

cbarbian-sap
Copy link

1. Why is this pull request needed and what does it do?

Try to fix race condition described in #6243

2. Which issues (if any) are related?

#6243

3. Which documentation changes (if any) need to be made?

None I think

4. Does this introduce a backward incompatible change or deprecation?

Hopefully not

@cbarbian-sap cbarbian-sap mentioned this pull request Aug 7, 2023
Open
@chrisohaver
Copy link
Member
chrisohaver commented Aug 8, 2023 •
edited
Loading

Thanks for the fix!

Adding a test may be a pain, but I think one approach would be to craft an ad-hoc test plugin with a locked OnShutdown function that the executing test can unlock (i.e. pausing the reload to induce the race condition).

@cbarbian-sap
plugin/reload: avoid race conditions if imported files change during …
6d48ee4 
…reload

Signed-off-by: Christoph Barbian <christoph.barbian@sap.com>
@cbarbian-sap
plugin/reload: add test for reloading with imported files
f12995a 
Signed-off-by: Christoph Barbian <christoph.barbian@sap.com>
@cbarbian-sap
Copy link
Author

I have added a corresponding test case. As you suggested, it uses a plugin (called dummy) which basically reads one integer argument from the configuration; such as

dummy 1

In addition the plugin registers an OnShutdown callback, which updates that imported file by incrementing the integer argument, not forever, but the first couple of times. This ensures that the problematic race condition reliably occurs
(I think the locking thing you proposed is not even required to make this happen).

The test then starts up a coredns instance with a simple Corefile:

.:0 {
  import *.conf
  reload 2s 1s
}

and kicks off a change in the imported file, and waits until the plugin is finally running with the latest configuration version.

With the upstream logic, the test fails, with the changed logic in the PR it succeeds.

@cbarbian-sap
Copy link
Author

However I do not completely understand why the test is failing.
Running it individually always works:

go test -count 1 -v -run ^TestReloadWithImports$ ./test

However running the package tests as a whole never works. Could it be the case that this is because the global plugins (in that case, reload) are then shared across the various tests in the package, which is problematic if they carry a state?

If so, what could I do to overcome this ?

@cbarbian-sap
plugin/reload move test case to dedicated package
b88e737 
Signed-off-by: Christoph Barbian <christoph.barbian@sap.com>
@cbarbian-sap
Copy link
Author

It really seems to be impossible to use certain plugins (at least the reload plugin is affected) in that shared ./test package in several test cases resp. in different caddy server instances (e.g. because the callbacks used by the plugin are registered only once, with the very first instance using it).

At the moment the only test case using the reload plugin in ./test seems to be readme_test.go, and that one only instantiates the plugin, but does not really perform any functional tests. This is why it seems to work at the moment.

Anyway, to overcome the problem, I moved the test introduced by this PR into a separate package, ./test/reload....

@codecov
Copy link
codecov bot commented Aug 11, 2023 •
edited
Loading

Codecov Report

Merging #6244 (c8b48d7) into master (93c57b6) will increase coverage by 2.56%.
Report is 1021 commits behind head on master.
The diff coverage is 56.45%.

❗ Current head c8b48d7 differs from pull request most recent head 2bdd3ea. Consider uploading reports for the commit 2bdd3ea to get more accurate results

@@            Coverage Diff             @@
##           master    #6244      +/-   ##
==========================================
+ Coverage   55.70%   58.26%   +2.56%     
==========================================
  Files         224      251      +27     
  Lines       10016    16399    +6383     
==========================================
+ Hits         5579     9555    +3976     
- Misses       3978     6257    +2279     
- Partials      459      587     +128
Files Changed Coverage Δ
core/dnsserver/config.go 0.00% <ø> (ø)
core/dnsserver/register.go 13.27% <0.00%> (-4.71%) ⬇️
plugin/auto/auto.go 0.00% <0.00%> (ø)
plugin/auto/xfr.go 0.00% <0.00%> (ø)
plugin/auto/zone.go 78.94% <ø> (+0.68%) ⬆️
plugin/autopath/autopath.go 73.61% <ø> (-1.39%) ⬇️
plugin/azure/azure.go 10.61% <0.00%> (-0.57%) ⬇️
plugin/azure/setup.go 56.36% <0.00%> (-4.14%) ⬇️
plugin/backend_lookup.go 0.00% <0.00%> (ø)
plugin/bind/bind.go 50.00% <0.00%> (-50.00%) ⬇️
... and 77 more

... and 169 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cbarbian-sap cbarbian-sap marked this pull request as draft August 19, 2023 21:30
@cbarbian-sap cbarbian-sap mentioned this pull request Aug 20, 2023
Open
@cbarbian-sap
Copy link
Author

Following the discussion in #6243 I created a PR on coredns/caddy: coredns/caddy#4, and refactored the reload logic to make use of it.

NOTES:

This was referenced Aug 20, 2023
Closed
Open
@cbarbian-sap cbarbian-sap changed the title plugin/reload: avoid race conditions if imported files change during reload plugin/reload: avoid race conditions if imported files change during reload / fix issues with sigusr1 reload Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric johnbelamaric is a code owner

@bradbeam bradbeam Awaiting requested review from bradbeam bradbeam is a code owner

@chrisohaver chrisohaver Awaiting requested review from chrisohaver chrisohaver is a code owner

@dilyevsky dilyevsky Awaiting requested review from dilyevsky dilyevsky is a code owner

@jameshartig jameshartig Awaiting requested review from jameshartig jameshartig is a code owner

@greenpau greenpau Awaiting requested review from greenpau greenpau is a code owner

@Isolus Isolus Awaiting requested review from Isolus Isolus is a code owner

@miekg miekg Awaiting requested review from miekg miekg is a code owner

@pmoroney pmoroney Awaiting requested review from pmoroney pmoroney is a code owner

@rajansandeep rajansandeep Awaiting requested review from rajansandeep rajansandeep is a code owner

@stp-ip stp-ip Awaiting requested review from stp-ip stp-ip is a code owner

@SuperQ SuperQ Awaiting requested review from SuperQ SuperQ is a code owner

@yongtang yongtang Awaiting requested review from yongtang yongtang is a code owner

@Tantalor93 Tantalor93 Awaiting requested review from Tantalor93 Tantalor93 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cbarbian-sap @chrisohaver