Cherry-pick of #120897 #123935 #123887 #123994: Serve watch without resourceVersion from cache and introduce a WatchFromStorageWithoutResourceVersion feature gate to allow serving watch from storage. #123973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/lgtm |
|
LGTM label has been added. Git tree hash: 4bc6688458dc91c1d378c62f6077f48994df2d31
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
timeout on tests :( |
liggitt
reviewed
Mar 18, 2024
| defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.WatchFromStorageWithoutResourceVersion, false)() | ||
| store, terminate := testSetupWithEtcdAndCreateWrapper(t) | ||
| t.Cleanup(terminate) | ||
| storagetesting.RunWatchSemantics(context.TODO(), t, store) |
There was a problem hiding this comment.
doesn't this test take a long time? does running it twice now exceed package testing time?
There was a problem hiding this comment.
Yes, it takes 42s :(
There was a problem hiding this comment.
need to fix the test in master first ... the doubling of RunWatchSemantics has made this package timeout ~50% of runs since https://github.com/kubernetes/kubernetes/pull/123935/files#r1530224210 merged
There was a problem hiding this comment.
I expect that this is related to additional tests present on the main branch. Where I can check the current test runtime?
There was a problem hiding this comment.
Sent #123994 and included it into this PR.
There was a problem hiding this comment.
Can you keep each commit picked from master distinct, and update the description with all the master PRs now included in this PR?
There was a problem hiding this comment.
I assumed that just undoing the second run of TestWatchSemantics might get us below timeout. Pulled all the fixes to be safe.
…romStorageWithoutResourceVersion feature gate to allow serving watch from storage.
…acher package. It turns out that kube has a custom timeout for tests of 3 minutes. The tests in the cacher package are utilizing nearly the entire time and are being terminated, resulting in failing jobs. Before the change, the TestWatchSemantics took ~43s to run. With this simple change, it now takes ~18s. When we created the tests, we didn't measure the running time and assumed that waiting 1 second on a watch channel to make sure no more events are received was sufficient. This PR decreases the waiting time to 300 milliseconds. Modern computers can perform many tasks within that time. In addition to that, the tests are serial in nature, meaning that there is no other actor that could add items to the database, which could result in receiving new items. After the change the total running time decreased by 17%. Before the tests needed ~176s after they need ~146s. The changes also improved TestWatchSemanticInitialEventsExtended.
d8ae9f8
to
d9ca300
Compare
|
/triage accepted |
|
/king bug |
54bbd36
to
cf2a337
Compare
|
/retest |
1 similar comment
|
/retest |
|
/lgtm @serathius can you propagate this to 1.28 and 1.27 now that CI is green? |
|
LGTM label has been added. Git tree hash: 50e77ffc62a59e052defbd75fe7d7fd9da8d959e
|
|
/cherry-pick-approved |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry pick of #120897 #123935 #123887 #123994 on release-1.29.
#120897: Ensure that initial events are sorted for WatchList
#123935: Serve watch without resourceVersion from cache and introduce a WatchFromStorageWithoutResourceVersion feature gate to allow serving watch from storage
#123887: apiserver/storage/cacher: decrease the running time of tests in the cacher package
#123994: Undo double run of the TestWatchSemantics test to avoid hitting timeout
For details on the cherry pick process, see the cherry pick requests page.