plugin/kubernetes: Handle endpoint tombstones #3887

chrisohaver · 2020-05-13T19:32:08Z

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

ToEndpoints() conversion was filtering out Endpoint deletion tombstones (where the object in the event is not an *api.Endpoint, but rather a cache.DeletedFinalStateUnknown). The tombstones are essentially indicators that the Endpoint was deleted, without a containing a full copy of the Endpoints object - just a key value. ToEndpoints() was returning a nil *object.Endpoints when these were encountered, which caused failures downstream when deleting from the index.

This fix is allows ToEndpoints() to pass the tombstones through, so they can be deleted properly from the clientState.

2. Which issues (if any) are related?

Fixes #3860

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>

miekg · 2020-05-13T19:52:47Z

thanks @chrisohaver . Was this knowable at the time when we did the refactoring to make the memory go down?
I think this warrants a copy of your commit message into the source code to warn future devs - possible pointing to code and or documentation this is a thing

chrisohaver · 2020-05-13T19:57:08Z

Looks like this may have been an issue since CoreDNS 1.2.3, when API object trimming was introduced. I would not recommend reverting back to a version before 1.2.3 to avoid this bug.

This bug should be relatively uncommon to encounter, however some things that could increase occurrence:

unstable network resulting in extended periods of disconnect between CoreDNS and k8s API.
a short etcd compaction interval in the k8s api server

chrisohaver · 2020-05-13T20:15:20Z

Was this knowable at the time when we did the refactoring to make the memory go down?

Theoretically? Probably.
Reasonably? Not sure. client-go is complex.

I think this warrants a copy of your commit message into the source code

Makes sense. I'll add it in ToEndpoints().

johnbelamaric · 2020-05-13T20:21:34Z

plugin/kubernetes/object/endpoint.go

@@ -44,10 +44,10 @@ type EndpointPort struct {
 func EndpointsKey(name, namespace string) string { return name + "." + namespace }

 // ToEndpoints converts an api.Endpoints to a *Endpoints.
-func ToEndpoints(obj interface{}) (*api.Endpoints, *Endpoints) {
+func ToEndpoints(obj interface{}) (*api.Endpoints, interface{}) {


This is kind of a weird change. I think it would be more readable if we changed the signature to func ToEndpoints(*api.Endpoints) (*Endpoints), do the casting in the caller, and just not call it if obj is not a *api.Endpoints.

Assuming as I believe ToEndpoints is only called once anyway.

yeah - i was just making this change. :)

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

codecov-io · 2020-05-13T20:46:45Z

Codecov Report

Merging #3887 into master will decrease coverage by 0.03%.
The diff coverage is 31.25%.

@@            Coverage Diff             @@
##           master    #3887      +/-   ##
==========================================
- Coverage   56.78%   56.75%   -0.04%     
==========================================
  Files         222      222              
  Lines       11305    11316      +11     
==========================================
+ Hits         6420     6422       +2     
- Misses       4391     4398       +7     
- Partials      494      496       +2

Impacted Files	Coverage Δ
plugin/kubernetes/controller.go	`46.64% <31.25%> (-0.40%)`	⬇️
plugin/file/reload.go	`69.44% <0.00%> (-5.56%)`	⬇️

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 f534c3f...94cdf8a. Read the comment docs.

johnbelamaric · 2020-05-13T21:33:10Z

plugin/kubernetes/object/endpoint.go

-		return nil, nil
-	}
-
+func ToEndpoints(end *api.Endpoints) (*api.Endpoints, *Endpoints) {


I still don't get why we pass in end and then just return it.

I mean, why do we need to return it?

I mean, why do we need to return it?

You're right. I should do more refactoring here...

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

chrisohaver · 2020-05-13T22:00:50Z

I've further jostled the code around to reduce verbosity and scope of changes.
... need to retest it to verify things still work. (checked)

zouyee · 2020-05-14T07:47:32Z

Many controllers have examples of handling this scenario

https://github.com/kubernetes/kubernetes/blob/f975b4b0dff48c6a580593f3bc05220c70afa367/pkg/controller/disruption/disruption.go#L363-L379

chrisohaver · 2020-05-14T12:10:21Z

Many controllers have examples of handling this scenario

Yes, I have essentially based this change on such examples.

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

chrisohaver · 2020-05-14T13:31:37Z

The latest iteration moves the delta object inspection to inside the switch cases. I think it reads better. Note that now it sends the (possibly outdated) endpoints object from the tombstone to store.Delete() instead of passing in the tombstone itself (as is done in client-go's informer processor, from where this was originally copied), but this is ok , because Delete only uses immutable key fields.

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

johnbelamaric · 2020-05-14T17:44:38Z

plugin/kubernetes/controller.go

+								if err := clientState.Delete(tombstone); err != nil {
+									return err
+								}
+								apiEndpoints, ok = tombstone.Obj.(*api.Endpoints)


I don't fully understand this, why do we need this part if we already deleted it with the tombstone?

It hasn't been deleted yet at this point. It gets deleted later with clientState.Delete(obj). If we don't call clientState.Delete, it will remain in the cache.

So what does clientState.Delete(tombstone) do?

arg! - I left that in by accident. Thanks for catching that. I'll remove it and run the test again.

chrisohaver · 2020-05-14T18:23:42Z

Not sure why I have not seen this for Service objects in my tests - perhaps the API doesn't send tombstones for Service objects, just the endpoints.

I have not tested Pods, but I suspect we'll see this issue there too. But I want to get this PR squared away in a state that is acceptable before replicating it to Pods and Services.

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

johnbelamaric · 2020-05-14T19:39:28Z

Ok, that looks better!

ekeih · 2020-05-15T09:34:42Z

Thanks for your work on that @chrisohaver! This fix will make live easier for us, we recently saw this issue a lot on clusters running in Azure VMs.
I don't know how the release cycle/management of coredns usually works. Is there any way to get a patch release in the near future? Or do you release in fixed intervals?

* check for nil Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * handle tombstone Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * move casting to caller. add comments. Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * more sanding Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * more scrubbing Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * move object unwraping to switch cases Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * oops remove debug Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * remove cruft Signed-off-by: Chris O'Haver <cohaver@infoblox.com>

Bug 1868315: Back port Handle endpoint tombstones (upstream coredns#3887)

* check for nil Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * handle tombstone Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * move casting to caller. add comments. Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * more sanding Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * more scrubbing Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * move object unwraping to switch cases Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * oops remove debug Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * remove cruft Signed-off-by: Chris O'Haver <cohaver@infoblox.com>

…-to-release-4.5 [release-4.5] Bug 1869309: Back port Handle endpoint tombstones (upstream coredns#3887)

…-to-release-4.4 [release-4.4] Bug 1869310: Back port Handle endpoint tombstones (upstream coredns#3887)

* check for nil Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * handle tombstone Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * move casting to caller. add comments. Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * more sanding Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * more scrubbing Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * move object unwraping to switch cases Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * oops remove debug Signed-off-by: Chris O'Haver <cohaver@infoblox.com> * remove cruft Signed-off-by: Chris O'Haver <cohaver@infoblox.com>

chrisohaver added 2 commits May 12, 2020 16:45

check for nil

fb79a4f

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

handle tombstone

4f60d72

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

chrisohaver requested review from bradbeam, johnbelamaric, miekg, rajansandeep and yongtang as code owners May 13, 2020 19:32

This was referenced May 13, 2020

CoreDNS pods panic with no probe failures #3860

Closed

plugin/kubernetes: nil check before MetaNamespaceKeyFunc #3886

Closed

johnbelamaric reviewed May 13, 2020

View reviewed changes

move casting to caller. add comments.

00eab80

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

johnbelamaric reviewed May 13, 2020

View reviewed changes

chrisohaver added 2 commits May 13, 2020 17:43

more sanding

3d3fa9e

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

more scrubbing

94cdf8a

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

chrisohaver marked this pull request as draft May 13, 2020 22:01

chrisohaver marked this pull request as ready for review May 13, 2020 22:53

move object unwraping to switch cases

375c527

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

oops remove debug

2e204d7

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

johnbelamaric reviewed May 14, 2020

View reviewed changes

remove cruft

3b9b9e0

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

johnbelamaric merged commit bb7ee50 into coredns:master May 14, 2020

This was referenced May 15, 2020

plugin/kubernetes: handle tombstones in default processor #3890

Merged

Data is not synced when CoreDNS reconnects to kubernetes api server after protracted disconnection #3879

Closed

chrisohaver mentioned this pull request Jun 3, 2020

plugin/kubernetes: fix tombstone unwrapping #3924

Merged

chrisohaver mentioned this pull request Jul 24, 2020

invalid memory address or nil pointer dereference panic followed by timeouts in k8s API access #4022

Closed

sgreene570 mentioned this pull request Aug 13, 2020

Bug 1868315: Back port Handle endpoint tombstones (upstream #3887) openshift/coredns#37

Merged

openshift-merge-robot added a commit to openshift/coredns that referenced this pull request Aug 17, 2020

Merge pull request #37 from sgreene570/bz-1868315

b5c26d3

Bug 1868315: Back port Handle endpoint tombstones (upstream coredns#3887)

openshift-ci-robot mentioned this pull request Aug 17, 2020

[release-4.5] Bug 1869309: Back port Handle endpoint tombstones (upstream #3887) openshift/coredns#38

Merged

openshift-ci-robot mentioned this pull request Aug 17, 2020

[release-4.4] Bug 1869310: Back port Handle endpoint tombstones (upstream #3887) openshift/coredns#39

Merged

openshift-merge-robot added a commit to openshift/coredns that referenced this pull request Aug 20, 2020

Merge pull request #38 from openshift-cherrypick-robot/cherry-pick-37…

1d1e4c5

…-to-release-4.5 [release-4.5] Bug 1869309: Back port Handle endpoint tombstones (upstream coredns#3887)

openshift-merge-robot added a commit to openshift/coredns that referenced this pull request Sep 12, 2020

Merge pull request #39 from openshift-cherrypick-robot/cherry-pick-37…

38a1d51

…-to-release-4.4 [release-4.4] Bug 1869310: Back port Handle endpoint tombstones (upstream coredns#3887)

chrisohaver deleted the endpoint-tombstone branch January 9, 2021 14:42

plugin/kubernetes: Handle endpoint tombstones #3887

plugin/kubernetes: Handle endpoint tombstones #3887

Uh oh!

Conversation

chrisohaver commented May 13, 2020

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

2. Which issues (if any) are related?

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

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

Uh oh!

miekg commented May 13, 2020

Uh oh!

chrisohaver commented May 13, 2020

Uh oh!

chrisohaver commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisohaver commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zouyee commented May 14, 2020

Uh oh!

chrisohaver commented May 14, 2020

Uh oh!

chrisohaver commented May 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisohaver commented May 14, 2020

Uh oh!

johnbelamaric commented May 14, 2020

Uh oh!

ekeih commented May 15, 2020

Uh oh!

Uh oh!

codecov-io commented May 13, 2020 •

edited

Loading

chrisohaver commented May 13, 2020 •

edited

Loading