Honor starting resourceVersion in watch cache #24208

ncdc · 2016-04-13T17:02:34Z

Compare the requested resourceVersion to each event's resourceVersion to ensure events that occurred
in the past are not sent to the client.

Fixes #24194

cc @liggitt @wojtek-t

ncdc · 2016-04-13T17:03:05Z

I'd like to add a unit test for this but I've got a question about it at #24194 (comment)

liggitt · 2016-04-13T17:12:09Z

pkg/storage/cacher.go

@@ -555,6 +555,10 @@ func (c *cacheWatcher) process(initEvents []watchCacheEvent) {
 		if !ok {
 			return
 		}
+		if event.ResourceVersion <= resourceVersion {


can we just make the send conditional?

// only send events newer than resourceVersion if event.ResourceVersion > resourceVersion { c.sendWatchCacheEvent(event) }

k8s-github-robot · 2016-04-13T17:20:10Z

Labelling this PR as size/S

k8s-bot · 2016-04-13T17:38:10Z

GCE e2e build/test passed for commit edd8c916d1fe65b75da23567ee419384e9d0e8ae.

k8s-bot · 2016-04-13T17:59:53Z

GCE e2e build/test failed for commit 1703d32c2edbfc40edab8d00cbe7e5e5fcc374fe.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

ncdc · 2016-04-13T18:53:37Z

@k8s-bot e2e test this github issue: #23974

timothysc · 2016-04-13T19:09:51Z

Are there a positive and negative checks in the tests for this?

wojtek-t · 2016-04-13T19:10:03Z

Code changes LGTM - we just need some test for it.

ncdc · 2016-04-13T19:10:40Z

@timothysc will have units for it - just need to figure out how to write them w/o having them flake or take 30+ seconds

wojtek-t · 2016-04-13T19:10:53Z

@timothysc - see #24194 for discussion about test

k8s-bot · 2016-04-13T19:34:10Z

GCE e2e build/test passed for commit 1703d32c2edbfc40edab8d00cbe7e5e5fcc374fe.

liggitt · 2016-04-13T20:34:02Z

pkg/storage/cacher_test.go

+	podFoo := makeTestPod("foo")
+	fooCreated := updatePod(t, etcdStorage, podFoo, nil)
+
+	// Set up Watch starting at fooCreated.ResourceVersion + 1


update comment

k8s-github-robot · 2016-04-13T20:41:24Z

Labelling this PR as size/M

ncdc · 2016-04-13T20:41:50Z

@timothysc test added, PTAL

ncdc · 2016-04-13T20:45:31Z

Hmm the test might still pass without my fix... investigating.

ncdc · 2016-04-13T21:02:52Z

Fixed the test. Helps if you generate events for the right etcd path :-)

timothysc · 2016-04-13T21:13:38Z

pkg/storage/cacher_test.go

+	fooCreated := updatePod(t, etcdStorage, podFoo, nil)
+
+	// Set up Watch starting at fooCreated.ResourceVersion + 10
+	rv, err := storage.ParseWatchResourceVersion(fooCreated.ResourceVersion)


this logic could probably be reduced but it's a test so whatevers... re: parse + addition + convert

Yeah, I considered that but decided to be lazy 😄

I would do the same :)

k8s-bot · 2016-04-13T21:19:47Z

GCE e2e build/test passed for commit 2f2ee168f96411290b19a9af3ddb283e51cc9b73.

k8s-bot · 2016-04-13T21:26:34Z

GCE e2e build/test passed for commit 62a2ee0450deb7b9072038643729f72f67aa1d45.

k8s-bot · 2016-04-13T21:46:12Z

GCE e2e build/test passed for commit 0c23527da2f423904664e5bcce309ddc69da8b68.

timothysc · 2016-04-14T02:11:23Z

pkg/storage/cacher_test.go

+	case <-time.After(wait.ForeverTestTimeout):
+		t.Errorf("timed out waiting for event")
+	}
+}


could nit, not going to, will let @wojtek-t decide.

wojtek-t · 2016-04-14T13:34:49Z

@ncdc - can you please squash commits and rebase?

Compare the requested resourceVersion to each event's resourceVersion to ensure events that occurred in the past are not sent to the client.

ncdc · 2016-04-14T13:37:42Z

Rebased & squashed

wojtek-t · 2016-04-14T13:38:50Z

LGTM - thanks for fixing this!

k8s-cherrypick-bot · 2016-04-14T13:59:08Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

k8s-bot · 2016-04-14T14:05:21Z

GCE e2e build/test failed for commit 21f6fe03ffc9e5a09c2bf50d6bd659e4c002c936.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

k8s-bot · 2016-04-14T14:11:43Z

GCE e2e build/test passed for commit 049e63d.

k8s-github-robot · 2016-04-14T14:54:29Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-14T15:31:26Z

GCE e2e build/test passed for commit 049e63d.

k8s-github-robot · 2016-04-14T15:42:48Z

Automatic merge from submit-queue

…pstream-release-1.2 Automated cherry pick of #24208

…f-#24208-upstream-release-1.2 Automated cherry pick of kubernetes#24208

googlebot added the cla: yes label Apr 13, 2016

liggitt reviewed Apr 13, 2016
View reviewed changes

ncdc force-pushed the watch-cache-check-starting-rv branch from edd8c91 to 1703d32 Compare April 13, 2016 17:15

k8s-github-robot assigned smarterclayton Apr 13, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Apr 13, 2016

liggitt mentioned this pull request Apr 13, 2016

Finalize Kube items 3.2 openshift/origin#6766

Closed

85 tasks

wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 13, 2016

wojtek-t assigned wojtek-t and unassigned smarterclayton Apr 13, 2016

liggitt reviewed Apr 13, 2016
View reviewed changes

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 13, 2016

timothysc reviewed Apr 13, 2016
View reviewed changes

timothysc reviewed Apr 14, 2016
View reviewed changes

Honor starting resourceVersion in watch cache

049e63d

Compare the requested resourceVersion to each event's resourceVersion to ensure events that occurred in the past are not sent to the client.

ncdc force-pushed the watch-cache-check-starting-rv branch from 21f6fe0 to 049e63d Compare April 14, 2016 13:37

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2016

liggitt added the cherrypick-candidate label Apr 14, 2016

k8s-cherrypick-bot removed the cherrypick-candidate label Apr 14, 2016

liggitt added the cherrypick-candidate label Apr 14, 2016

liggitt added this to the v1.2 milestone Apr 14, 2016

k8s-github-robot merged commit d800dca into kubernetes:master Apr 14, 2016

ncdc mentioned this pull request Apr 15, 2016

Automated cherry pick of #24208 #24307

Merged

ncdc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 18, 2016

roberthbailey added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 19, 2016

zmerlynn added a commit that referenced this pull request Apr 20, 2016

Merge pull request #24307 from ncdc/automated-cherry-pick-of-#24208-u…

03d2a7c

…pstream-release-1.2 Automated cherry pick of #24208

zmerlynn removed the cherrypick-candidate label Apr 20, 2016

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016

Merge pull request kubernetes#24307 from ncdc/automated-cherry-pick-o…

fffd0ce

…f-#24208-upstream-release-1.2 Automated cherry pick of kubernetes#24208

ncdc deleted the watch-cache-check-starting-rv branch February 13, 2017 17:24

shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017

Merge pull request kubernetes#24307 from ncdc/automated-cherry-pick-o…

8949db3

…f-#24208-upstream-release-1.2 Automated cherry pick of kubernetes#24208

Honor starting resourceVersion in watch cache #24208

Honor starting resourceVersion in watch cache #24208

Conversation

ncdc commented Apr 13, 2016

ncdc commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Apr 13, 2016

k8s-bot commented Apr 13, 2016

k8s-bot commented Apr 13, 2016

ncdc commented Apr 13, 2016

timothysc commented Apr 13, 2016

wojtek-t commented Apr 13, 2016

ncdc commented Apr 13, 2016

wojtek-t commented Apr 13, 2016

k8s-bot commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Apr 13, 2016

ncdc commented Apr 13, 2016

ncdc commented Apr 13, 2016

ncdc commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Apr 13, 2016

k8s-bot commented Apr 13, 2016

k8s-bot commented Apr 13, 2016

Choose a reason for hiding this comment

wojtek-t commented Apr 14, 2016

ncdc commented Apr 14, 2016

wojtek-t commented Apr 14, 2016

k8s-cherrypick-bot commented Apr 14, 2016

k8s-bot commented Apr 14, 2016

k8s-bot commented Apr 14, 2016

k8s-github-robot commented Apr 14, 2016

k8s-bot commented Apr 14, 2016

k8s-github-robot commented Apr 14, 2016