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

Reload started twice can generate panic #2656

Merged
merged 1 commit into from
Mar 7, 2019
Merged

Reload started twice can generate panic #2656

merged 1 commit into from
Mar 7, 2019

Conversation

fturib
Copy link
Contributor

@fturib fturib commented Mar 6, 2019 •
edited
Loading

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

Fix problem of reload kicked-off twice.
Since last release of Caddy, the event InstanceStartupEvent is triggered directly by Caddy when starting the Instance. CoreDNS MUST not trigger that event again.

2. Which issues (if any) are related?

#2654

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

None. It is fixing an bug.

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

No.

@corbot corbot bot requested a review from varyoo March 6, 2019 00:52
@corbot
Copy link

corbot bot commented Mar 6, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked varyoo (via /OWNERS) for a review.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

@fturib
Copy link
Contributor Author

fturib commented Mar 6, 2019

I could reproduce locally the bug mentionned.
With Corefile:

.:5555 {
  reload 2s
}

console output:

ftur-MBP13:coredns ftur$ ./coredns 
.:5555
2019-03-05T19:42:10.344-05:00 [INFO] plugin/reload: Running configuration MD5 = 9f333dbbcc0d1e1c830bf6d3e6a3428a
2019-03-05T19:42:10.344-05:00 [INFO] CoreDNS-1.4.0
2019-03-05T19:42:10.344-05:00 [INFO] darwin/amd64, go1.12, 9dd28894
CoreDNS-1.4.0
darwin/amd64, go1.12, 9dd28894
2019-03-05T19:42:10.344-05:00 [INFO] plugin/reload: Running configuration MD5 = 9f333dbbcc0d1e1c830bf6d3e6a3428a
[INFO] Reloading
[INFO] Reloading
2019-03-05T19:42:17.747-05:00 [INFO] plugin/reload: Running configuration MD5 = 65e261fc961e4a87e245610646c518e7
[INFO] Reloading complete
panic: sync: negative WaitGroup counter

goroutine 57 [running]:
sync.(*WaitGroup).Add(0xc000174630, 0xffffffffffffffff)
        /usr/local/Cellar/go/1.12/libexec/src/sync/waitgroup.go:74 +0x135
sync.(*WaitGroup).Done(...)
        /usr/local/Cellar/go/1.12/libexec/src/sync/waitgroup.go:99
github.com/coredns/coredns/core/dnsserver.(*Server).Stop.func1(0xc000174600, 0xc0002061e0)
        /Users/ftur/go/src/github.com/coredns/coredns/core/dnsserver/server.go:172 +0x40
created by github.com/coredns/coredns/core/dnsserver.(*Server).Stop
        /Users/ftur/go/src/github.com/coredns/coredns/core/dnsserver/server.go:171 +0x7c

@fturib
Copy link
Contributor Author

fturib commented Mar 6, 2019

With the fix, i can observe the behavior os back to normal.

ftur-MBP13:coredns ftur$ ./coredns 
.:5555
2019-03-05T19:43:29.410-05:00 [INFO] plugin/reload: Running configuration MD5 = 65e261fc961e4a87e245610646c518e7
2019-03-05T19:43:29.410-05:00 [INFO] CoreDNS-1.4.0
2019-03-05T19:43:29.410-05:00 [INFO] darwin/amd64, go1.12, 9dd28894-dirty
CoreDNS-1.4.0
darwin/amd64, go1.12, 9dd28894-dirty
[INFO] Reloading
2019-03-05T19:43:36.758-05:00 [INFO] plugin/reload: Running configuration MD5 = 9f333dbbcc0d1e1c830bf6d3e6a3428a
[INFO] Reloading complete
[INFO] Reloading
2019-03-05T19:43:43.423-05:00 [INFO] plugin/reload: Running configuration MD5 = 65e261fc961e4a87e245610646c518e7
[INFO] Reloading complete

It will need an Integration Test to be permanently validated on PRs.

@codecov-io
Copy link

Codecov Report

Merging #2656 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2656      +/-   ##
==========================================
+ Coverage   55.93%   55.95%   +0.02%     
==========================================
  Files         194      194              
  Lines        9962     9961       -1     
==========================================
+ Hits         5572     5574       +2     
+ Misses       3979     3977       -2     
+ Partials      411      410       -1
Impacted Files Coverage Δ
coremain/run.go 31.57% <ø> (+0.27%) ⬆️
plugin/file/reload.go 76.31% <0%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dd2889...16a220b. Read the comment docs.

@miekg
Copy link
Member

miekg commented Mar 6, 2019 via email

@miekg miekg mentioned this pull request Mar 7, 2019
Closed
@miekg miekg merged commit 97e2a53 into coredns:master Mar 7, 2019
@miekg miekg mentioned this pull request Mar 7, 2019
Closed
@fturib fturib mentioned this pull request Mar 28, 2019
Closed
Jason-ZW pushed a commit to rancher/coredns that referenced this pull request Apr 17, 2019
@rancher-sy-bot
- remove caddy event, It is now triggered directly by Caddy in the St…
65e98a1 
…art function of the Instance (coredns#2656)
dna2github pushed a commit to dna2fork/coredns that referenced this pull request Jul 19, 2019
@dna2github
- remove caddy event, It is now triggered directly by Caddy in the St…
96c55d4 
…art function of the Instance (coredns#2656)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varyoo varyoo Awaiting requested review from varyoo

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

Successfully merging this pull request may close these issues.
3 participants
@fturib @codecov-io @miekg