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/cache: fix negative cache masking cases #3744

Merged
merged 2 commits into from Mar 20, 2020

Conversation

chrisohaver
Copy link
Member

@chrisohaver chrisohaver commented Mar 13, 2020

Note: This PR fixes a bug that affects the default usage of cache, not just those who enable stale cache retrieval.

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

This fixes some bugs introduced when the stale cache retrieval feature was added (#3468):

  1. An expired (beyond stale) negative cache would mask positive cache, sending a query upstream thus missing the cache. This affects the default usage of cache. (plugin/cache: Expired denial cache item obscures success cache item - repeated hits to backend / miscount of misses #3701 ) Cache misses would also not be counted in this case.
  2. A stale negative cache entry could be returned to a client despite a fresh positive cache being available. ("serve_stale" option in the "cache" plugin behaves incorrectly #3586)

The first issue is fixed by tweaking the logic in getIgnoreTTL.
The other issue is fixed by having prefetch remove cache entries from the negative cache when writing to the positive cache.

The test added in this PR is based on the test in PR #3702 (thanks @huntharo !)
I also successfully tested this change with tests in PR #3702, which could therefore merge if this PR merges.

This PR should supersede #3736

2. Which issues (if any) are related?

Issues:
#3701
#3586

PRs:
#3702
#3736

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

none

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

no

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

PTAL @gonzalop

Copy link
Contributor

@gonzalop gonzalop left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the fix.

plugin/cache/cache.go Outdated Show resolved Hide resolved
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@codecov-io
Copy link

Codecov Report

Merging #3744 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3744      +/-   ##
==========================================
+ Coverage   56.41%   56.47%   +0.06%     
==========================================
  Files         220      220              
  Lines       11159    11163       +4     
==========================================
+ Hits         6295     6304       +9     
+ Misses       4379     4376       -3     
+ Partials      485      483       -2
Impacted Files Coverage Δ
plugin/cache/handler.go 92.95% <100%> (+7.04%) ⬆️
plugin/cache/cache.go 79.8% <100%> (+4.31%) ⬆️
plugin/forward/connect.go 81.69% <0%> (-4.23%) ⬇️
plugin/pkg/parse/host.go 77.41% <0%> (-0.92%) ⬇️

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 0858267...00b47ad. Read the comment docs.

@yifan-gu
Copy link

yifan-gu commented May 3, 2020

@chrisohaver Thank you for the fix!
FWIW, recently we also found that together with kubernetes' autopath, this bug will cause unexpected high cache miss rate, prob because the autopath generates some negative cache during its search, which masks the positive cache.

This was referenced Jul 28, 2020
nyodas pushed a commit to DataDog/coredns that referenced this pull request Oct 26, 2020
* fix negative cache masking cases

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

* remove unecessary param

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@chrisohaver chrisohaver deleted the fixstaleprefetch branch January 9, 2021 14:44
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

5 participants