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

Add a serve_stale option for plugin/cache #3468

Merged
merged 12 commits into from Nov 29, 2019
Merged

Conversation

@gonzalop
Copy link
Contributor

gonzalop commented Nov 18, 2019

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

Implements #3158 by adding a serve_stale option to the cache directive.
From unbound's man page:

serve-expired:
If enabled, unbound attempts to serve old responses from cache with a TTL of 0 in
the response without waiting for the actual resolution to finish. The actual
resolution answer ends up in the cache later on. Default is "no".

2. Which issues (if any) are related?

#3158

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

Yes, the PR includes a documentation update.

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

No.

@corbot corbot bot requested a review from miekg Nov 18, 2019
@corbot

This comment has been minimized.

Copy link

corbot bot commented Nov 18, 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/cache/OWNERS) for a review.
Note this is not an exclusive request. Anyone is free to provide a review of this pull request.

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.

@gonzalop gonzalop changed the title Serve expired Add a serve_expired option for plugin/cache Nov 18, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 18, 2019

Codecov Report

Merging #3468 into master will increase coverage by 0.09%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3468      +/-   ##
==========================================
+ Coverage   56.57%   56.67%   +0.09%     
==========================================
  Files         221      221              
  Lines       10992    11028      +36     
==========================================
+ Hits         6219     6250      +31     
- Misses       4290     4294       +4     
- Partials      483      484       +1
Impacted Files Coverage Δ
plugin/cache/cache.go 75.49% <ø> (ø) ⬆️
plugin/cache/handler.go 85.71% <84.84%> (-3.42%) ⬇️
plugin/cache/setup.go 57.03% <92.3%> (+4.44%) ⬆️

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 768ca99...764278c. Read the comment docs.

@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch 2 times, most recently from 6f46b7b to 9ed26b0 Nov 18, 2019
@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch 3 times, most recently from e644bcc to 3adf193 Nov 19, 2019
plugin/cache/README.md Outdated Show resolved Hide resolved
@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch from 49a77d5 to 16da711 Nov 19, 2019
Copy link
Member

chrisohaver left a comment

Thanks for the contribution! LGTM. Although IMO we should get an approval from one of the plugin OWNERS before merging ...

@gonzalop

This comment has been minimized.

Copy link
Contributor Author

gonzalop commented Nov 20, 2019

Ack. Thank you, Chris, for reviewing.

plugin/cache/README.md Outdated Show resolved Hide resolved
plugin/cache/README.md Outdated Show resolved Hide resolved
plugin/cache/README.md Outdated Show resolved Hide resolved
plugin/cache/handler.go Outdated Show resolved Hide resolved
plugin/cache/setup_test.go Outdated Show resolved Hide resolved
@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch from ec029da to 527b4a6 Nov 21, 2019
@gonzalop gonzalop changed the title Add a serve_expired option for plugin/cache Add a serve_stale option for plugin/cache Nov 21, 2019
@gonzalop gonzalop requested a review from miekg Nov 21, 2019
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 21, 2019

Copy link
Member

miekg left a comment

looks much better, thanks, still some (style) comments though

plugin/cache/README.md Outdated Show resolved Hide resolved
plugin/cache/cache.go Outdated Show resolved Hide resolved
plugin/cache/handler.go Outdated Show resolved Hide resolved
plugin/cache/setup.go Outdated Show resolved Hide resolved
plugin/cache/setup.go Outdated Show resolved Hide resolved
plugin/cache/setup.go Outdated Show resolved Hide resolved
@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch 3 times, most recently from 663e4ab to d1eab4d Nov 21, 2019
@gonzalop gonzalop requested a review from miekg Nov 21, 2019
@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch 4 times, most recently from 84c08e0 to cedd4e4 Nov 21, 2019
gonzalop added 2 commits Nov 18, 2019
Add a new block option 'serve_expired' (values: 'yes' or 'no') that
enables serving requests from cache, even if stale, while still
refreshing the cache if possible.

This is useful to survive connectivity issues to upstream servers.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
Set prefetch to true for the name resolution triggered when serving a
stale request.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
gonzalop added 9 commits Nov 18, 2019
New counter 'served_expired_total' incremented when we serve a query
from expired cached data.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
The duration is how old an item can be fo it to be considered as stale
but not too much.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
Ditto.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
New tests to show how serving a cached item works even when there are no
more healthy backends left.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
Found by CI!

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
plugin/cache serve_expired explained.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
It now reflects more accurately what we do while keeping the word count
low.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
* s/serve_expired/serve_stale/
* removed the yes/no argument from the option.
* made the doc less pedantic.
* documented the new metric.
* call get only once -> new getItem

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
* The duration is now optional and defaults to 1h.
* getIgnoreTTL increments the same metrics as get.
* made the setup test more compact.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch from cedd4e4 to 73decd3 Nov 22, 2019
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 22, 2019

@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch 3 times, most recently from f16f22d to f8547e8 Nov 22, 2019
Remove the serveStale field. Not needed.

Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
@gonzalop gonzalop force-pushed the gonzalop:serve_expired branch from f8547e8 to 764278c Nov 22, 2019
@gonzalop

This comment has been minimized.

Copy link
Contributor Author

gonzalop commented Nov 28, 2019

Anything else I should do?

@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 29, 2019

I'm trying out the new and improved corbot (github.com/miekg/dreck)

/lgtm
/merge

@corbot corbot bot merged commit b4df2d0 into coredns:master Nov 29, 2019
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 56.67% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 29, 2019

hmmm the lgtm led to a

[ERROR] dreck: POST https://api.github.com/repos/coredns/coredns/pulls/3468/reviews: 422 Unprocessable Entity [{Resource: Field: Code: Message:}]

which is REST for 'no' ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.