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.

Sign up for GitHub

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

Jump to bottom

Make watch cache treat resourceVersion consistent with uncached watch #24008

Merged
merged 1 commit into from
Apr 12, 2016
Merged

Make watch cache treat resourceVersion consistent with uncached watch #24008

merged 1 commit into from
Apr 12, 2016
+23 −18

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Apr 7, 2016

Fixes #24004

This makes the watch cache handle resourceVersion consistent with an uncached watch API call, and the documented behavior. Watching from resourceVersion=X delivers watch events after version X (X is not included):

// When specified with a watch call, shows changes that occur after that particular version of a resource.
// Defaults to changes from the beginning of history.
ResourceVersion string

@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Apr 7, 2016
@liggitt liggitt changed the title Make watch cache behave like uncached watch WIP - Make watch cache behave like uncached watch Apr 7, 2016
@liggitt liggitt mentioned this pull request Apr 7, 2016
Closed
@liggitt liggitt force-pushed the watch_cache branch 2 times, most recently from 35f7fb0 to 07f62c9 Compare April 8, 2016 00:02
@liggitt liggitt changed the title WIP - Make watch cache behave like uncached watch WIP - Make watch cache treat resourceVersion consistent with uncached watch Apr 8, 2016
@liggitt
Copy link
Member Author

liggitt commented Apr 8, 2016

cc @wojtek-t

@liggitt
Copy link
Member Author

liggitt commented Apr 8, 2016

looks like there are package-level tests that are ensuring incorrect behavior... working on fixing those up

@liggitt liggitt force-pushed the watch_cache branch 2 times, most recently from 4b9a9e0 to c133923 Compare April 8, 2016 00:09
@timothysc
Copy link
Member

😞

We should probably add a api-compatability test.

@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit fb88b2cce4ca58a7a73f5fff80e900e9d150d41b.

@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit 35f7fb0c4183ec6abf74d04b8224482668773afb.

@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit 07f62c94b85ec712d39de5c5192350e684f84da8.

@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit 4b9a9e01d2430561b892f929c9e025ee9b9fa1cb.

@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit c1339239c6860c998a8651465c2ee59a785f3c10.

@liggitt liggitt changed the title WIP - Make watch cache treat resourceVersion consistent with uncached watch Make watch cache treat resourceVersion consistent with uncached watch Apr 8, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 8, 2016
@liggitt
Copy link
Member Author

liggitt commented Apr 8, 2016

unit tests updated:

  • removed two watch events in cacher_test.go we shouldn't have been observing
  • shifted test data forward one version in watch_cache_test.go so the rest of the test code could stay as-is

wojtek-t
wojtek-t reviewed Apr 11, 2016
View reviewed changes
pkg/storage/cacher_test.go
verifyWatchEvent(t, nowWatcher, watch.Added, podFooPrime)

_ = updatePod(t, etcdStorage, podFooBis, fooUpdated)

t.Log("verifying")
Copy link
Member

Choose a reason for hiding this comment

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

please remove

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to keep these. If the verify calls fail, there is no way to tell which one failed

@wojtek-t
Copy link
Member

@liggitt - thanks for finding this issue
I added some comments to this PR, but generally lgtm

@liggitt
Copy link
Member Author

liggitt commented Apr 11, 2016

@wojtek-t anything else before I tag and open a cherrypick?

@wojtek-t
Copy link
Member

@liggitt - I would prefer changing those "verifying" logs to something more useful (to be honest I would prefer to remove them even more :P)

@wojtek-t
Copy link
Member

Once changed, feel free to self-apply lgtm label

@liggitt
Copy link
Member Author

liggitt commented Apr 12, 2016

removed the "verifying" lines, logged the calling line in verifyWatchEvent in failure cases

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2016
@wojtek-t
Copy link
Member

LGTM - thanks!

@liggitt liggitt mentioned this pull request Apr 12, 2016
Merged
@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit ada6023.

@k8s-github-robot
Copy link

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

@liggitt liggitt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 12, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit ada6023.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f4a421d into kubernetes:master Apr 12, 2016
@ncdc ncdc mentioned this pull request Apr 13, 2016
Closed
@zmerlynn zmerlynn added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 13, 2016
@liggitt liggitt deleted the watch_cache branch April 14, 2016 14:06
lavalamp added a commit that referenced this pull request Apr 14, 2016
@lavalamp
Merge pull request #24135 from liggitt/automated-cherry-pick-of-#2400…
22e7950 
…8-upstream-release-1.2

Automated cherry pick of #24008
@k8s-cherrypick-bot
Copy link

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

@erictune erictune mentioned this pull request Jul 2, 2016
Closed
@erictune
Copy link
Member

erictune commented Jul 2, 2016

@liggitt Does this PR require action by the user when upgrading to 1.3.0? If so, please edit your first comment to have a release-note block, like in #28132. If not, please change to release-note-none or release-note label.

@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jul 2, 2016
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@lavalamp
Merge pull request kubernetes#24135 from liggitt/automated-cherry-pic…
bcbace8 
…k-of-#24008-upstream-release-1.2

Automated cherry pick of kubernetes#24008
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@lavalamp
Merge pull request kubernetes#24135 from liggitt/automated-cherry-pic…
e7bbf25 
…k-of-#24008-upstream-release-1.2

Automated cherry pick of kubernetes#24008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wojtek-t wojtek-t

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.