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

plugin/kubernetes: fix reload panic #4881

Merged
merged 3 commits into from Sep 22, 2021
Merged

Conversation

chrisohaver
Copy link
Member

Signed-off-by: Chris O'Haver cohaver@infoblox.com

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

Avoid panic when Corefile reloads.

2. Which issues (if any) are related?

#4880

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

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

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@codecov-commenter
Copy link

Codecov Report

Merging #4881 (7753cf6) into master (93c57b6) will decrease coverage by 0.04%.
The diff coverage is 61.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4881      +/-   ##
==========================================
- Coverage   55.70%   55.66%   -0.05%     
==========================================
  Files         224      232       +8     
  Lines       10016    14398    +4382     
==========================================
+ Hits         5579     8014    +2435     
- Misses       3978     5887    +1909     
- Partials      459      497      +38     
Impacted Files Coverage Δ
core/dnsserver/address.go 100.00% <ø> (ø)
core/dnsserver/config.go 0.00% <ø> (ø)
core/dnsserver/onstartup.go 0.00% <0.00%> (ø)
core/dnsserver/server.go 34.69% <0.00%> (+8.37%) ⬆️
plugin/auto/auto.go 0.00% <0.00%> (ø)
plugin/auto/zone.go 78.94% <ø> (+0.68%) ⬆️
plugin/backend_lookup.go 0.00% <0.00%> (ø)
plugin/bind/bind.go 50.00% <0.00%> (-50.00%) ⬇️
plugin/etcd/etcd.go 0.00% <ø> (ø)
plugin/etcd/setup.go 1.14% <0.00%> (-0.26%) ⬇️
... and 292 more

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 8ddb631...7753cf6. Read the comment docs.

@@ -31,7 +31,6 @@ var log = clog.NewWithPlugin(pluginName)
func init() { plugin.Register(pluginName, setup) }

func setup(c *caddy.Controller) error {
klog.InitFlags(nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, (not!). How can we prevent this being added back in again?

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@chrisohaver
Copy link
Member Author

How can we prevent this being added back in again?

I opened an issue to add a reload test to coredns/ci.
Also added a comment to the code.

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@miekg miekg merged commit e742b5e into coredns:master Sep 22, 2021
@chrisohaver
Copy link
Member Author

I opened an issue to add a reload test to coredns/ci.

Added ...

without this fix ...

=== RUN   TestReload
    reload_test.go:50: detected panic: .:53
        [INFO] plugin/reload: Running configuration MD5 = 2560a496a673b268ec3b774fd62a90aa
        CoreDNS-1.8.5
        linux/amd64, go1.17.1, ce156312
        [INFO] Reloading
        /coredns flag redefined: log_dir
        [PANIC] Restart: /coredns flag redefined: log_dir
--- FAIL: TestReload

with this fix ...

=== RUN   TestReload
--- PASS: TestReload

jinglina pushed a commit to jinglina/coredns that referenced this pull request Dec 23, 2021
* fix reload panic

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>

* add comment

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>

* o import ordering

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: jinglinax@163.com <jinglinax@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants