Send correct resource version for delete events from watch cache #58547

liggitt · 2018-01-19T22:44:52Z

Fixes #58545
Fixes kubernetes/client-go#338

the watch cache filtering is returning the previous object content intact, including resource version. this is the logic the watch cache uses:

switch {
case curObjPasses && !oldObjPasses:
	watchEvent = watch.Event{Type: watch.Added, Object: event.Object.DeepCopyObject()}
case curObjPasses && oldObjPasses:
	watchEvent = watch.Event{Type: watch.Modified, Object: event.Object.DeepCopyObject()}
case !curObjPasses && oldObjPasses:
	watchEvent = watch.Event{Type: watch.Deleted, Object: event.PrevObject.DeepCopyObject()}
}

when processing a delete event, we should be sending the old object's content but with the event's resource version set in it. corresponding logic exists in the uncached stores:

kubernetes/staging/src/k8s.io/apiserver/pkg/storage/etcd/etcd_watcher.go

Lines 401 to 403 in 77ac663

    
           if err := w.versioner.UpdateObject(oldObj, res.Node.ModifiedIndex); err != nil { 
        
           	utilruntime.HandleError(fmt.Errorf("failure to version api object (%d) %#v: %v", res.Node.ModifiedIndex, oldObj, err)) 
        
           }

kubernetes/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher.go

Lines 373 to 378 in 77ac663

    
           // Note that this sends the *old* object with the etcd revision for the time at 
        
           // which it gets deleted. 
        
           oldObj, err = decodeObj(wc.watcher.codec, wc.watcher.versioner, data, e.rev) 
        
           if err != nil { 
        
           	return nil, nil, err 
        
           }

Fixes an issue where the resourceVersion of an object in a DELETE watch event was not the resourceVersion of the delete itself, but of the last update to the object. This could disrupt the ability of clients to re-establish watches properly.

liggitt · 2018-01-19T23:12:08Z

@kubernetes/sig-api-machinery-bugs @kubernetes/sig-api-machinery-pr-reviews

liggitt · 2018-01-19T23:14:48Z

cc @smarterclayton @wojtek-t

liggitt · 2018-01-19T23:15:16Z

/status approved-for-milestone

liggitt · 2018-01-19T23:18:08Z

staging/src/k8s.io/apiserver/pkg/storage/cacher_whitebox_test.go

@@ -73,79 +77,88 @@ func TestCacheWatcherHandlesFiltering(t *testing.T) {
 		{
 			events: []*watchCacheEvent{


this diff is best viewed without whitespace:
https://github.com/kubernetes/kubernetes/pull/58547/files?w=1

this makes the test data accurate (events have resource versions), and actually tests for the result we want (delete event is delivered with the resource version of the event that caused it, not the last modified resourceVersion of the object, otherwise watch events would be able to have resourceVersions that went back in time, and caused re-watch to start from old points in history)

fejta-bot · 2018-01-22T20:23:32Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

lavalamp · 2018-01-22T21:04:09Z

cc @jpbetz @wenjiaswe

tnozicka · 2018-01-22T22:05:27Z

/retest

k8s-github-robot · 2018-01-23T00:29:20Z

/test all [submit-queue is verifying that this PR is safe to merge]

wanghaoran1988 · 2018-01-23T02:40:32Z

/test pull-kubernetes-e2e-kops-aws

k8s-github-robot · 2018-01-23T03:37:14Z

/test all [submit-queue is verifying that this PR is safe to merge]

liggitt · 2018-01-23T04:43:26Z

/retest

k8s-github-robot · 2018-01-23T04:49:29Z

Automatic merge from submit-queue (batch tested with PRs 58547, 57228, 58528, 58499, 58618). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2018-01-23T05:17:52Z

@liggitt: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`57998d2`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…7-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #58547: Send correct resource version for delete events from watch Cherry pick of #58547 on release-1.8. #58547: Send correct resource version for delete events from watch

@liggitt

Automatic merge from submit-queue (batch tested with PRs 18233, 18068, 18228, 18227). UPSTREAM: 58547: Send correct resource version for delete events from watch cache Backport of kubernetes/kubernetes#58547 Watch cache was returning incorrect (old) ResourceVersion on "deleted" events breaking informers that were going back in time. This fixes it. /assign @liggitt /cc @mfojtik Fixes #17581 #16003 and likely others

@liggitt

…e-fix-58547 Automatic merge from submit-queue (batch tested with PRs 18233, 18068, 18228, 18227). UPSTREAM: 58547: Send correct resource version for delete events from watch cache Backport of kubernetes#58547 Watch cache was returning incorrect (old) ResourceVersion on "deleted" events breaking informers that were going back in time. This fixes it. /assign @liggitt /cc @mfojtik Fixes openshift/origin#17581 openshift/origin#16003 and likely others Origin-commit: 042a63f8c1effc2fb911ce2cf494458872e9f8a3

…7-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #58547: Send correct resource version for delete events from watch Cherry pick of #58547 on release-1.9. #58547: Send correct resource version for delete events from watch

k8s-cherrypick-bot · 2018-01-26T02:34:28Z

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

…ect resource version for delete events from watch Origin-commit: 346464d764b9f9a96ea29d41a8a3e3ca1b219468

@liggitt

…e-fix-58547-release-3.8 Automatic merge from submit-queue. [3.8] UPSTREAM: 58571: Automated cherry pick of kubernetes#58547: Send correct resource version for delete events from watch Backport of openshift/origin#18233 via kubernetes#58571 /assign @liggitt Origin-commit: 360fac5be27896518ad01b63a0ea132174af23e8

@liggitt

…e-fix-58547 Automatic merge from submit-queue (batch tested with PRs 18233, 18068, 18228, 18227). UPSTREAM: 58547: Send correct resource version for delete events from watch cache Backport of kubernetes#58547 Watch cache was returning incorrect (old) ResourceVersion on "deleted" events breaking informers that were going back in time. This fixes it. /assign @liggitt /cc @mfojtik Fixes openshift/origin#17581 openshift/origin#16003 and likely others Origin-commit: 042a63f8c1effc2fb911ce2cf494458872e9f8a3

@liggitt

Automatic merge from submit-queue (batch tested with PRs 18233, 18068, 18228, 18227). UPSTREAM: 58547: Send correct resource version for delete events from watch cache Backport of kubernetes/kubernetes#58547 Watch cache was returning incorrect (old) ResourceVersion on "deleted" events breaking informers that were going back in time. This fixes it. /assign @liggitt /cc @mfojtik Fixes openshift/origin#17581 openshift/origin#16003 and likely others Origin-commit: 042a63f8c1effc2fb911ce2cf494458872e9f8a3 Kubernetes-commit: b1d49808af3db35be42e4b705953d656a21bc201

…e, fixed official version kubernetes#58547

k8s-github-robot assigned fgrzadkowski and xiang90 Jan 19, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2018

Send correct resource version for delete events from watch cache

57998d2

liggitt force-pushed the watch-cache-delete-resourceversion branch from e081817 to 57998d2 Compare January 19, 2018 23:08

liggitt changed the title ~~WIP - Send correct resource version for delete events from watch cache~~ Send correct resource version for delete events from watch cache Jan 19, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2018

liggitt added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Jan 19, 2018

liggitt added this to the v1.9 milestone Jan 19, 2018

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 19, 2018

k8s-github-robot added the milestone/needs-approval label Jan 19, 2018

liggitt assigned smarterclayton and wojtek-t and unassigned xiang90 and fgrzadkowski Jan 19, 2018

k8s-ci-robot added the status/approved-for-milestone label Jan 19, 2018

liggitt commented Jan 19, 2018

View reviewed changes

k8s-github-robot removed the milestone/needs-approval label Jan 20, 2018

k8s-github-robot merged commit 619305f into kubernetes:master Jan 23, 2018

tnozicka mentioned this pull request Jan 23, 2018

UPSTREAM: 58547: Send correct resource version for delete events from watch cache openshift/origin#18233

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Jan 26, 2018

liggitt deleted the watch-cache-delete-resourceversion branch January 26, 2018 04:57

chazsconi mentioned this pull request Feb 12, 2018

Add support for watch requests obmarg/kazan#14

Merged

phsiao mentioned this pull request Mar 4, 2018

controller-manager has stale jobs and can't delete them #59676

Closed

wenjiaswe mentioned this pull request May 2, 2018

client-go should ignore the ResourceVersion of DELETE event when watching #63356

Closed

This was referenced May 23, 2018

avoid wrongly replaying old events in kube-controller-manager #64209

Closed

EndPoint unavailable for a long time (almost 2h) for a correct Service #57897

Closed

sycki added a commit to goodrain/kubernetes that referenced this pull request Jul 4, 2018

[REV] Send correct resource version for delete events from watch cach…

3e3e996

…e, fixed official version kubernetes#58547

liggitt mentioned this pull request Dec 8, 2018

gluster endpoints are disappearing #71871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send correct resource version for delete events from watch cache #58547

Send correct resource version for delete events from watch cache #58547

liggitt commented Jan 19, 2018 •

edited

Loading

liggitt commented Jan 19, 2018

liggitt commented Jan 19, 2018

liggitt commented Jan 19, 2018

liggitt Jan 19, 2018 •

edited

Loading

fejta-bot commented Jan 22, 2018

lavalamp commented Jan 22, 2018

tnozicka commented Jan 22, 2018

k8s-github-robot commented Jan 23, 2018

wanghaoran1988 commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

liggitt commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

k8s-ci-robot commented Jan 23, 2018

k8s-cherrypick-bot commented Jan 26, 2018

	if err := w.versioner.UpdateObject(oldObj, res.Node.ModifiedIndex); err != nil {
	utilruntime.HandleError(fmt.Errorf("failure to version api object (%d) %#v: %v", res.Node.ModifiedIndex, oldObj, err))
	}

	// Note that this sends the old object with the etcd revision for the time at
	// which it gets deleted.
	oldObj, err = decodeObj(wc.watcher.codec, wc.watcher.versioner, data, e.rev)
	if err != nil {
	return nil, nil, err
	}

		@@ -73,79 +77,88 @@ func TestCacheWatcherHandlesFiltering(t *testing.T) {
		{
		events: []*watchCacheEvent{

Send correct resource version for delete events from watch cache #58547

Send correct resource version for delete events from watch cache #58547

Conversation

liggitt commented Jan 19, 2018 • edited Loading

liggitt commented Jan 19, 2018

liggitt commented Jan 19, 2018

liggitt commented Jan 19, 2018

liggitt Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

fejta-bot commented Jan 22, 2018

lavalamp commented Jan 22, 2018

tnozicka commented Jan 22, 2018

k8s-github-robot commented Jan 23, 2018

wanghaoran1988 commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

liggitt commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

k8s-ci-robot commented Jan 23, 2018

k8s-cherrypick-bot commented Jan 26, 2018

liggitt commented Jan 19, 2018 •

edited

Loading

liggitt Jan 19, 2018 •

edited

Loading