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/auto: Reload zones every one minute #2516

Merged
merged 4 commits into from Feb 17, 2019

Conversation

Projects
None yet
4 participants
@mrasu
Copy link
Contributor

mrasu commented Jan 30, 2019

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

However README says "Default is one minute.", zone data is not reloaded without declaring reload option explicitly.
By this change, data is reloaded every one minute by default.

2. Which issues (if any) are related?

#2471

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

None

plugin/auto: Reload zones every one minute
However README says "Default is one minute.", zone data is not reloaded without declaring `reload` option explicitly.
By this change, data is reloaded every one minute by default.

@corbot corbot bot requested a review from miekg Jan 30, 2019

@corbot

This comment has been minimized.

Copy link

corbot bot commented Jan 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 miekg (via plugin/auto/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.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 30, 2019

Codecov Report

Merging #2516 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2516      +/-   ##
==========================================
+ Coverage   55.29%   55.47%   +0.17%     
==========================================
  Files         204      204              
  Lines       10334    10362      +28     
==========================================
+ Hits         5714     5748      +34     
+ Misses       4199     4193       -6     
  Partials      421      421
Impacted Files Coverage Δ
plugin/auto/setup.go 56.36% <100%> (+8.94%) ⬆️
plugin/kubernetes/setup.go 70.46% <0%> (+0.46%) ⬆️
plugin/route53/route53.go 85.18% <0%> (+2.77%) ⬆️
plugin/etcd/setup.go 66.27% <0%> (+5.46%) ⬆️

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 b455f86...124fa71. Read the comment docs.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Jan 30, 2019

This fixes part of the problem. Question is, if this should be fixed differently.

I agree it should set a default, but we might want to fix a bit more.

  • fix documentation that reload is the SOA reload of changed files and timeout is the removal and addition timer
  • use timeout or reload to set the other, if not both are set (better default as this seems expected)
  • optionally fix it and only rely on reload and deprecate timeout
Fix more
* Use `timeout` or `reload` to set the other, if not both are set.
* Respect `reload` if both are set.
@mrasu

This comment has been minimized.

Copy link
Contributor Author

mrasu commented Jan 31, 2019

@stp-ip Thanks for feedback!

Following your response, I change behavior of timeout and reload.
But because I don't know a way to "deprecate" in this project, I'm not sure my change is exactly what you says.
Are belows fine for you?

After my change:

  • when reload and timeout are set -> both use the value of reload.
  • when only reload is set -> both use the value of reload.
  • when only timeout is set -> both use the value of timeout.
  • when reload and timeout are not set -> both are one minute.

If this is fine, I will update documentation.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Jan 31, 2019

Paging @miekg as this might be a breaking or deprecation change depending on the decision.

when reload and timeout are set -> both use the value of reload.

I would say, if both are set, that might be intentional. So overwriting timeout with reload might create unexpected outcomes until it's deprecated.
So first deprecate and then overwrite timeout with reload.

when only reload is set -> both use the value of reload.

+1

when only timeout is set -> both use the value of timeout.

+1

when reload and timeout are not set -> both are one minute

+1

In general I think we should deprecate TIMEOUT.
The general process in my mind:

  • document deprecation "reload will be used, if not defined, and it will be removed in a subsequent version"
  • print log message that TIMEOUT within plugin/auto is deprecated and RELOAD should be used + a link to the docs
  • 1st next release according to deprecation policy (https://github.com/coredns/coredns#deprecation-policy) overwrite TIMEOUT always with RELOAD
  • 2nd next release according to deprecation policy remove TIMEOUT and let the config fail, if it's set

@mrasu mrasu force-pushed the mrasu:auto_reload branch from 19bbc38 to 96ebc61 Feb 1, 2019

Deprecate TIMEOUT
* Log if TIMEOUT is specified
* Update README

@mrasu mrasu force-pushed the mrasu:auto_reload branch from 96ebc61 to f385ee7 Feb 1, 2019

@mrasu

This comment has been minimized.

Copy link
Contributor Author

mrasu commented Feb 1, 2019

@stp-ip done!

name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** specifies how often
CoreDNS should scan the directory; the default is every 60 seconds. This value is in seconds.
The minimum value is 1 second.
name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** deprecated.

This comment has been minimized.

@miekg

miekg Feb 1, 2019

Member

The sentence starting with TIMEOUT looks broken

This comment has been minimized.

@stp-ip

stp-ip Feb 8, 2019

Member
Suggested change
name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** deprecated.
name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** is deprecated and will be removed in a subsequent version.
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Feb 1, 2019

we should just make sure reload = 60 is set and used. Why is this not the case?
We don't need to deprecate anything and add a new option, current set is fine.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Feb 1, 2019

we should just make sure reload = 60 is set and used. Why is this not the case?
We don't need to deprecate anything and add a new option, current set is fine.

There are 2 options both with different outcomes.
The idea is to remove TIMEOUT or at least deprecate it. No need to keep a config value, that we basically overwrite in the long term.
My current idea is that, if TIMEOUT is set to something this should still take effect. Then we deprecate and later remote or overwrite the TIMEOUT with the value of RELOAD to make the transition easier.

So the main way to define both reloading files and reloading records is done via RELOAD.

The 60s default was in there, but was changed to nil. Not sure why so? Defaulting to 60s seems fine.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Feb 1, 2019

Correction. The defaulting to 60s is done. It was just moved.

TL;DR:
If TIMEOUT is set, but not RELOAD -> it works
If RELOAD is set, but not TIMEOUT -> it works
If none is set, we default to 60s -> it works
If TIMEOUT and RELOAD is set, we honor the settings -> it works depending on user settings

@mrasu

This comment has been minimized.

Copy link
Contributor Author

mrasu commented Feb 3, 2019

Actually, my first commit (c59bc4a) just set 60 seconds as a default.
I deprecate TIMEOUT following with advice.
So either is fine

Honestly, because two values are for almost same purpose, I think it's fair to deprecate TIMEOUT and use the same configuration for watching files too.
But because I'm pretty new to this repository, I cannot judge whether is better.
So I'd like you to decide which is better.

@miekg

This comment has been minimized.

Copy link
Member

miekg commented Feb 3, 2019

ok, I agree with deprecating TIMEOUT, we can offically drop no_reload as well at the same time. Need a release cycle (to announce and then drop) for it to become active.

What should I write in the release notes?

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Feb 5, 2019

Honestly, because two values are for almost same purpose, I think it's fair to deprecate TIMEOUT and use the same configuration for watching files too.
But because I'm pretty new to this repository, I cannot judge whether is better.
So I'd like you to decide which is better.

I think we agree that the purpose is similar, but changing it how it might be more logical might still break people. This is a change in how it works today so having this backwards compatible until it's deprecated seems better.

I'll test this again this week.

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Feb 5, 2019

ok, I agree with deprecating TIMEOUT, we can offically drop no_reload as well at the same time. Need a release cycle (to announce and then drop) for it to become active.

What should I write in the release notes?

Suggestion:
Deprecation: plugin/auto deprecates TIMEOUT and NO_RELOAD and recommends the use of RELOAD. This brings plugin/file and plugin/auto to use the same syntax for reload intervals.

  • test current implementation on backwards compatibility and deprecation (me)
  • implementation
  • docs
  • release notes
  • create issue for next release to remove deprecated code (including NO_RELOAD from plugin/file)
@mrasu

This comment has been minimized.

Copy link
Contributor Author

mrasu commented Feb 5, 2019

Sorry for late reply. I'm sick these days(ful?).
I think I cannot contribute until weekend.
please feel free to commit or create new PR if needed.

👍

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Feb 5, 2019

Sorry for late reply. I'm sick these days(ful?).
I think I cannot contribute until weekend.
please feel free to commit or create new PR if needed.

Get well. I'll take the time to test this PR and then we can iterate next week.

@miekg

This comment has been minimized.

Copy link
Member

miekg commented Feb 7, 2019

@stp-ip
Copy link
Member

stp-ip left a comment

Docs changes suggested.

name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** specifies how often
CoreDNS should scan the directory; the default is every 60 seconds. This value is in seconds.
The minimum value is 1 second.
name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** deprecated.

This comment has been minimized.

@stp-ip

stp-ip Feb 8, 2019

Member
Suggested change
name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** deprecated.
name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** is deprecated and will be removed in a subsequent version.
name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** deprecated.
`reload` will be used, if not defined, and it will be removed in a subsequent version
(it specifies how often CoreDNS should scan the directory to watch for file removal and addition;
the default is every 60 seconds. This value is in seconds. The minimum value is 1 second.)
* `reload` interval to perform reload of zone if SOA version changes. Default is one minute.

This comment has been minimized.

@stp-ip

stp-ip Feb 8, 2019

Member
Suggested change
* `reload` interval to perform reload of zone if SOA version changes. Default is one minute.
* `reload` interval to perform reloads of zones if SOA version changes and zonefiles. Default is one minute.
@mrasu

This comment has been minimized.

Copy link
Contributor Author

mrasu commented Feb 9, 2019

@stp-ip Thanks! changed!

@mrasu

This comment has been minimized.

Copy link
Contributor Author

mrasu commented Feb 16, 2019

@stp-ip ping

@stp-ip

stp-ip approved these changes Feb 16, 2019

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Feb 16, 2019

Thanks for the ping. This seems to have slipped through my notifications.

/approve
/lgtm

@corbot

corbot bot approved these changes Feb 16, 2019

Copy link

corbot bot left a comment

LGTM by stp-ip

@corbot

corbot bot approved these changes Feb 16, 2019

Copy link

corbot bot left a comment

LGTM by stp-ip

@stp-ip

This comment has been minimized.

Copy link
Member

stp-ip commented Feb 17, 2019

/merge

@corbot corbot bot merged commit 362d7e9 into coredns:master Feb 17, 2019

4 checks passed

codecov/project 55.47% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coredns/ci Integration test passed 16 tests and 52 subtests.
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
You can’t perform that action at this time.