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/reload: Graceful reload of imported files #3068

Merged
merged 2 commits into from Jul 30, 2019

Conversation

@erikwilson
Copy link
Contributor

commented Jul 30, 2019

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

Graceful reload of imports.
If an imported file changes the reload plugin should detect the change.
https://github.com/coredns/coredns/tree/master/plugin/import

2. Which issues (if any) are related?

#2633
rancher/k3s#462
Azure/AKS#841

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

Import support on reload:
https://github.com/coredns/coredns/blob/master/plugin/reload/README.md#bugs

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

Unknown

Parse Corefile for graceful reloads
Check for changes in imports on reload by parsing the Corefile and
use the md5 of the marshalled server blocks.

@corbot corbot bot requested a review from johnbelamaric Jul 30, 2019

@corbot

This comment has been minimized.

Copy link

commented Jul 30, 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 johnbelamaric (via plugin/reload/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.

@erikwilson erikwilson changed the title Graceful reload of imported files plugin/reload: Graceful reload of imported files Jul 30, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 30, 2019

Codecov Report

Merging #3068 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3068      +/-   ##
==========================================
- Coverage   55.95%   55.88%   -0.07%     
==========================================
  Files         205      205              
  Lines       10105    10117      +12     
==========================================
  Hits         5654     5654              
- Misses       4040     4052      +12     
  Partials      411      411
Impacted Files Coverage Δ
plugin/reload/reload.go 13.55% <0%> (-3.47%) ⬇️

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 4fda953...2d822be. Read the comment docs.

if err != nil {
return nil, err
}
return json.Marshal(serverBlocks)

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Jul 30, 2019

Member

Are we sure this is deterministic? That is, are we sure it always results in the same ordering? Otherwise we could end up with continuously reloading. We may need the MD5 to navigate through any map structure, sorting the map keys and creating a canonical string representation of the whole file.

This comment has been minimized.

Copy link
@erikwilson

erikwilson Jul 30, 2019

Author Contributor

From my understanding json.Marshal is deterministic, although the documentation does a poor job at describing this. There is a part of the Marshal docs that states map keys are sorted and used as JSON object keys (https://github.com/golang/go/blob/5fae09b7386de26db59a1184f62fc7b22ec7667b/src/encoding/json/encode.go#L135). Looking at the code it appears map keys and struct fields are sorted going back to at least Go v1.4.

Information added to README about the behavior with version context.

@johnbelamaric
Copy link
Member

left a comment

I don't think we can rely on json.Marshal always producing identical output for the same structs. But I could be wrong.

Please update the readme to include reflect the change as well.

@johnbelamaric
Copy link
Member

left a comment

Ok, one minor tweak and I am good with it.

@@ -89,8 +89,9 @@ closed in step 1; so the health endpoint is broken. The same can hopen in the pr

In general be careful with assigning new port and expecting reload to work fully.

Also any `import` statement is not discovered by this plugin. This means if any of these imported files
changes the *reload* plugin is ignorant of that fact.
In CoreDNS v1.6.x and earlier any `import` statements are not discovered by this plugin.

This comment has been minimized.

Copy link
@johnbelamaric

This comment has been minimized.

Copy link
@erikwilson

erikwilson Jul 30, 2019

Author Contributor

Updated, thanks!

Reload plugin README update
Add information on support for graceful reloads with imported files,
only supported in >= v1.7.0.

@erikwilson erikwilson force-pushed the erikwilson:import-graceful-reload branch from 65894f8 to 2d822be Jul 30, 2019

@johnbelamaric

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

/lgtm
/merge

@corbot

corbot bot approved these changes Jul 30, 2019

Copy link

left a comment

LGTM by johnbelamaric

@corbot corbot bot merged commit 367d285 into coredns:master Jul 30, 2019

4 checks passed

ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 55.88% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.