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

Enable debug globally if enabled in any server config #4007

Merged
merged 2 commits into from Aug 24, 2020

Conversation

olivierlemasle
Copy link
Contributor

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

Enable debug globally if enabled in any server config. It was currently enabled only if the plugin debug was enabled in the last server config of the Corefile.

This will still disable debug on reload, if debug is no more present.

Example with Corefile:

example.net {
    hosts
    debug
}

. {
    whoami
}

Logs (with debug message):

[DEBUG] plugin/hosts: Parsed hosts file into 0 entries
example.net.:1053
.:1053
CoreDNS-1.7.0
linux/amd64, go1.14.3, 5b4b3569

Remove debug in the Corefile and reload with SIGHUP1:

Logs (with no more debug message):

[INFO] SIGUSR1: Reloading
[INFO] Reloading
[INFO] Reloading complete

2. Which issues (if any) are related?

Fixes #4006

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

N/A

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

No

It was currently enabled only if the plugin debug
was enabled in the last server config of the Corefile.

Signed-off-by: Olivier Lemasle <o.lemasle@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2020

Codecov Report

Merging #4007 into master will increase coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4007      +/-   ##
==========================================
+ Coverage   56.85%   57.11%   +0.25%     
==========================================
  Files         224      222       -2     
  Lines       11349    11277      -72     
==========================================
- Hits         6453     6441      -12     
+ Misses       4401     4346      -55     
+ Partials      495      490       -5     
Impacted Files Coverage Δ
core/dnsserver/server.go 13.19% <100.00%> (+2.15%) ⬆️
plugin/metrics/metrics.go 64.06% <0.00%> (-7.94%) ⬇️
plugin/file/reload.go 69.44% <0.00%> (-5.56%) ⬇️
plugin/grpc/grpc.go 63.79% <0.00%> (-1.67%) ⬇️
plugin/trace/trace.go 79.06% <0.00%> (ø)
plugin/backend_lookup.go 0.00% <0.00%> (ø)
plugin/health/overloaded.go 57.89% <0.00%> (ø)
plugin/template/template.go 88.42% <0.00%> (ø)
plugin/kubernetes/metrics.go 100.00% <0.00%> (ø)
plugin/template/metrics.go
... and 13 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 2d5f12d...26dc724. Read the comment docs.

@miekg
Copy link
Member

miekg commented Jul 14, 2020

I'm a bit confused the title says 'enable' but the patch only deals with disable?

@olivierlemasle
Copy link
Contributor Author

@miekg I hope #4006 explains better the issue 😊

The goal is to have the "debug mode" enabled if it is configured in one of the configuration blocks of a Corefile.
Actually, with a Corefile as below, the "debug mode" is currently enabled and then immediately disabled:

example.net {
  # enable debug
  debug
}

. {
  # No "debug"
}

That's the issue I fix with this PR, by disabling "debug" ONLY IF it is not present in any configuration block.

@miekg
Copy link
Member

miekg commented Jul 16, 2020

ack ,that explains it better.

Can you think of a test to add?

@miekg
Copy link
Member

miekg commented Jul 21, 2020

probably also warrants documentation changes in the debug readme to explain this global behavior

@olivierlemasle
Copy link
Contributor Author

Ok, I'll add documentation, and I will try to add tests if it makes sense.

@miekg
Copy link
Member

miekg commented Jul 31, 2020

any luck?

@olivierlemasle
Copy link
Contributor Author

Sorry, I guess I will not have the time today, and then I'm in vacation for the next 2 weeks. I'll do it on August 17th.

@miekg
Copy link
Member

miekg commented Jul 31, 2020

enjoy your holiday, I might kick off a smaller 1.7.1 release before that though, but then it will be in 1.7.2 or 1.8.0

Signed-off-by: Olivier Lemasle <o.lemasle@gmail.com>
@olivierlemasle
Copy link
Contributor Author

@miekg Sorry for the delay; I've finally updated the PR according to your comments.

@miekg
Copy link
Member

miekg commented Aug 24, 2020

thanks! And merged!

@miekg miekg merged commit f36715e into coredns:master Aug 24, 2020
@olivierlemasle olivierlemasle deleted the fix-debug branch August 24, 2020 08:26
nyodas pushed a commit to DataDog/coredns that referenced this pull request Oct 26, 2020
* Enable debug globally if enabled in any server config

It was currently enabled only if the plugin debug
was enabled in the last server config of the Corefile.

Signed-off-by: Olivier Lemasle <o.lemasle@gmail.com>

* Add test and update debug's README

Signed-off-by: Olivier Lemasle <o.lemasle@gmail.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.

Debug is enabled only if present in the last server configuration
3 participants