send bookmark right now after sending all items in watchCache store #127012

Chaunceyctx · 2024-08-30T01:30:43Z

What type of PR is this?

What this PR does / why we need it:

/kind bug

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

send bookmark right now after sending all items in watchCache store

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

// before this pr:
// Bookmark event is only generated by Cacher.dispatchEvents goroutine. And it will re-calculate the next time when bookmark should be sent.
// DispatchEvents goroutine will set bookmark event nextTime from immediately to defaultBookmarkFrequency(1min) after sending initial events end bookmark event.
// So reflector in client will not receive another bookmark event within 1 minute after receiving the initial events end bookmark event.
// And client can receive only non-bookmark event within 1 minute.

// after this pr:
// Initial events end bookmark event is generated by cacheWatcher.processInterval goroutine. And ordinary bookmark event is still generated by Cacher.dispatchEvents goroutine.
// After sending all events in cacherInterval, cacheWatcher.processInterval goroutine tries to send initial events end bookmark event. Before cacheWatcher.state is set to cacheWatcherBookmarkSent,
// Cacher.dispatchEvents goroutine may re-calculate the next time when bookmark should be sent. And it will still believe that bookmark should be sent immediately.
// Within the 1s-1.25s bookmark tick time, current unit test creates pods defined in scenario.podsAfterEstablishingWatch field.
// When bookmark tick is triggerred in 1s-1.25s , apiserver may send a non-initial bookmark event(resourceVersion can be equal to any pods' resourceVersion defined in scenario.podsAfterEstablishingWatch) to
// client. So we can not make sure that client will not receive another bookmark event within 1 minute after receiving the initial events end bookmark event.

// In a word, this phenomenon doesn't affect the function of watchlist feature. It's just that we need to pay attention to this phenomenon when writing ut cases

linux-foundation-easycla · 2024-08-30T01:30:47Z

The committers listed above are authorized under a signed CLA.

✅ login: Chaunceyctx / name: Chauncey (7239202)

k8s-ci-robot · 2024-08-30T01:30:53Z

Hi @Chaunceyctx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

Chaunceyctx · 2024-08-30T01:33:25Z

gentle /ping @wojtek-t @p0lyn0mial @liggitt

xuzhenglun · 2024-08-30T02:53:45Z

/ok-to-test
/triage accepted
/priority important-longterm

Chaunceyctx · 2024-09-06T01:26:24Z

gentle /ping @wojtek-t @p0lyn0mial :)

wojtek-t

Sorry for delay

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

wojtek-t · 2024-09-11T07:29:06Z

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

+		if err != nil {
+			t.Fatalf("Failed to create watch: %v", err)
+		}
+		verifyWatchEvents(t, w, expectedWatchEvents)


Please don't create a new function - simply use testCheckResultsInStrictOrder

wojtek-t · 2024-09-11T07:30:19Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

@@ -193,7 +193,13 @@ func testCheckResultsInStrictOrder(t *testing.T, w watch.Interface, expectedEven
 	}
 }

-func testCheckNoMoreResults(t *testing.T, w watch.Interface) {
+func testCheckResultsWithDesiredAndActual(t *testing.T, expectedEvents []watch.Event, actualEvents []watch.Event) {


no need to introduce such function - there already is testCheckResultsInStrictOrder

wojtek-t · 2024-09-11T07:31:02Z

staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go

-			testCheckResultsInStrictOrder(t, w, scenario.expectedEventsAfterEstablishingWatch(createdPods))
-			testCheckNoMoreResults(t, w)
+
+			// before this pr:


we generally don't describe state before the PR - it's useless for future readers.

State before the PR can be documented in PR or commit description.

wojtek-t · 2024-09-11T08:10:44Z

staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go

+
+			// In a word, this phenomenon doesn't affect the function of watchlist feature. It's just that we need to pay attention to this phenomenon when writing ut cases
+
+			// get corresponding pod watch events


I initially started thinking if making bookmark period a configurable option and switching it off wouldn't be a better option for this test, but in the end, I wouldn't go this way.

That said, I would approach this slightly differently. In particular:

let's introduce testCheckResultWithIgnoreFunc function
it's supposed to be somewhat similar to testCheckResultFunc, but additionally takes ignore func(watch.Event) bool parameter [and it has a loop for getting items from watch and just ignores one for which the ignore function returns true)

let's simply use this new helper function here, passing function that ignores events with type Bookmark as the ignore function

Chaunceyctx · 2024-09-11T09:00:48Z

Sorry for delay

Don’t Worry about it. Thanks for your review. I will address all comments. :)

Chaunceyctx · 2024-09-11T16:57:05Z

/retest

some ci jobs are so weird. I have change testCheckNoMoreResults to TestCheckNoMoreResults in order to expose this method and reuse it. But some ci jobs can still detect testCheckNoMoreResults method in my code.

Chaunceyctx · 2024-09-11T17:06:54Z

/retest

some ci jobs are so weird. I have change testCheckNoMoreResults to TestCheckNoMoreResults in order to expose this method and reuse it. But some ci jobs can still detect testCheckNoMoreResults method in my code.

fine. I don't rebase master branch. lol

wojtek-t · 2024-09-20T11:35:57Z

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

+	}
+
+	// make cacher.dispatchEvents goroutine exit and bookmark event will only be created by processInterval method
+	cacher.stopCh <- struct{}{}


close stopCh instead.

Or you can probably even call cacher.Stop()

I apologize for this mistake. In fact, we cannot close stopCh because watchCache will become not ready, affecting watch request. For sendInitialEvents=false, AllowBookmarks=true watch request, the bookmark sending frequency is 1 minute, and I control the watchevent receive timeout to 5 seconds, which is equivalent to ensuring that bookmarks are only generated by the processInterval method. So I just delete this code. : )

/gentle ping @wojtek-t

wojtek-t · 2024-09-20T11:36:03Z

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

+		if err != nil {
+			t.Fatalf("Failed to create watch: %v", err)
+		}
+		verifyWatchEvents(t, w, expectedWatchEvents)


Chaunceyctx · 2024-09-21T01:20:08Z

/retest

wojtek-t

Just few more nits - other than that LGTM.

wojtek-t · 2024-09-25T10:23:45Z

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

+	}
+
+	for i := range scenarios {
+		expectedWatchEvents := expectedPodEvents


for _, tc := range scenarios { t.Run(tc.name, func(t *testing.T) { ... } }

wojtek-t · 2024-09-25T10:24:09Z

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

+		}
+		storagetesting.TestCheckResultsInStrictOrder(t, w, expectedWatchEvents)
+		storagetesting.TestCheckNoMoreResults(t, w)
+		cancel()


with the comment I mentioned above, make it a defer cancel() next to where context is created

wojtek-t · 2024-09-25T10:25:56Z

staging/src/k8s.io/apiserver/pkg/storage/testing/utils.go

+			} else {
+				t.Fatalf("cannot receive correct event, expect no event, but get a event: %+v", event)
+			}
+		case <-time.After(2 * time.Second):


We still didn't fully address the problem of long-running tests and exceeding the timeout for single package tests.

So let's for now use something shorter - please do the same thing as in testCheckNoMoreResults

I noticed that the timeout used in the testCheckNoMoreResults function is 100ms, but we cannot guarantee that the normal bookmark event (not the initial bookmark event) will be received within 100ms. It is possible that it will be received at 101ms, 102ms, or 103ms ..., which could cause the subsequent testCheckNoMoreResults function to fail. Therefore, I still use 100ms in the testCheckResultWithIgnoreFunc function to avoid package test taking too long, and I add an ignore function argument to testCheckNoMoreResults and rename it to testCheckNoMoreResultsWithIgnoreFunc to ignore the normal bookmark event. :)

wojtek-t · 2024-09-25T10:27:04Z

staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go

@@ -1512,8 +1512,10 @@ func RunWatchSemantics(ctx context.Context, t *testing.T, store storage.Interfac
 				require.NoError(t, err, "failed to add a pod: %v")
 				createdPods = append(createdPods, out)
 			}
-			testCheckResultsInStrictOrder(t, w, scenario.expectedEventsAfterEstablishingWatch(createdPods))
-			testCheckNoMoreResults(t, w)
+			testCheckResultWithIgnoreFunc(t, w, scenario.expectedEventsAfterEstablishingWatch(createdPods), func(watchEvent watch.Event) bool {


nit: define

var ignoreEventsFn := func(event watch.Event) bool { return event.Type == watch.Bookmark }

and use it here

Chaunceyctx · 2024-09-26T02:23:56Z

/retest

wojtek-t · 2024-09-26T06:46:44Z

/lgtm
/approve

k8s-ci-robot · 2024-09-26T06:46:50Z

LGTM label has been added.

Git tree hash: 850b4e6861b8d406b79824ab024c7a1698fa3667

k8s-ci-robot · 2024-09-26T06:46:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Chaunceyctx, wojtek-t

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:

~~staging/src/k8s.io/apiserver/pkg/storage/OWNERS~~ [wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 30, 2024

k8s-ci-robot requested review from liggitt and smarterclayton August 30, 2024 01:31

Chaunceyctx force-pushed the new-send-bookmark branch from 3329749 to 5c266bb Compare August 30, 2024 01:32

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 30, 2024

Chaunceyctx force-pushed the new-send-bookmark branch from 5c266bb to e720455 Compare August 30, 2024 01:37

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2024

Chaunceyctx force-pushed the new-send-bookmark branch from e720455 to fc4a502 Compare August 30, 2024 01:38

wojtek-t reviewed Sep 11, 2024

View reviewed changes

Chaunceyctx force-pushed the new-send-bookmark branch from 8d08005 to ffe1ff0 Compare September 11, 2024 16:05

Chaunceyctx force-pushed the new-send-bookmark branch from ffe1ff0 to e3098de Compare September 11, 2024 17:06

Chaunceyctx force-pushed the new-send-bookmark branch from e3098de to e4ced4a Compare September 12, 2024 01:32

wojtek-t reviewed Sep 20, 2024

View reviewed changes

Chaunceyctx force-pushed the new-send-bookmark branch 3 times, most recently from bcebd56 to 25113e8 Compare September 20, 2024 15:22

Chaunceyctx force-pushed the new-send-bookmark branch from 25113e8 to 973f20c Compare September 25, 2024 06:33

wojtek-t reviewed Sep 25, 2024

View reviewed changes

Chaunceyctx force-pushed the new-send-bookmark branch from 973f20c to 278f2a4 Compare September 26, 2024 01:41

Chaunceyctx force-pushed the new-send-bookmark branch 2 times, most recently from 59b0e4f to f476c78 Compare September 26, 2024 05:20

send bookmark right now after sending all items in watchCache store

7239202

Chaunceyctx force-pushed the new-send-bookmark branch from f476c78 to 7239202 Compare September 26, 2024 06:18

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2024

k8s-ci-robot merged commit 81ebfb3 into kubernetes:master Sep 26, 2024
14 checks passed

k8s-ci-robot added this to the v1.32 milestone Sep 26, 2024

Chaunceyctx mentioned this pull request Sep 28, 2024

failed to sync secret/configmap cache: timed out waiting for the condition when WatchList feature is enable #126958

Closed

npolshakova mentioned this pull request Oct 10, 2024

Update release notes draft to version v1.32.0-alpha.1 kubernetes/sig-release#2637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send bookmark right now after sending all items in watchCache store #127012

send bookmark right now after sending all items in watchCache store #127012

Chaunceyctx commented Aug 30, 2024 •

edited

Loading

linux-foundation-easycla bot commented Aug 30, 2024 •

edited

Loading

k8s-ci-robot commented Aug 30, 2024

Chaunceyctx commented Aug 30, 2024

xuzhenglun commented Aug 30, 2024

Chaunceyctx commented Sep 6, 2024

wojtek-t left a comment

wojtek-t Sep 11, 2024

wojtek-t Sep 20, 2024

wojtek-t Sep 11, 2024

wojtek-t Sep 11, 2024

wojtek-t Sep 11, 2024

Chaunceyctx commented Sep 11, 2024

Chaunceyctx commented Sep 11, 2024 •

edited

Loading

Chaunceyctx commented Sep 11, 2024

wojtek-t Sep 20, 2024

Chaunceyctx Sep 20, 2024 •

edited

Loading

wojtek-t Sep 20, 2024

Chaunceyctx commented Sep 21, 2024

wojtek-t left a comment

wojtek-t Sep 25, 2024

Chaunceyctx Sep 26, 2024

wojtek-t Sep 25, 2024

wojtek-t Sep 25, 2024

Chaunceyctx Sep 26, 2024 •

edited

Loading

wojtek-t Sep 25, 2024

Chaunceyctx commented Sep 26, 2024

wojtek-t commented Sep 26, 2024

k8s-ci-robot commented Sep 26, 2024

k8s-ci-robot commented Sep 26, 2024


		// In a word, this phenomenon doesn't affect the function of watchlist feature. It's just that we need to pay attention to this phenomenon when writing ut cases

		// get corresponding pod watch events

send bookmark right now after sending all items in watchCache store #127012

send bookmark right now after sending all items in watchCache store #127012

Conversation

Chaunceyctx commented Aug 30, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla bot commented Aug 30, 2024 • edited Loading

k8s-ci-robot commented Aug 30, 2024

Chaunceyctx commented Aug 30, 2024

xuzhenglun commented Aug 30, 2024

Chaunceyctx commented Sep 6, 2024

wojtek-t left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chaunceyctx commented Sep 11, 2024

Chaunceyctx commented Sep 11, 2024 • edited Loading

Chaunceyctx commented Sep 11, 2024

Choose a reason for hiding this comment

Chaunceyctx Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chaunceyctx commented Sep 21, 2024

wojtek-t left a comment

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

Choose a reason for hiding this comment

Chaunceyctx Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chaunceyctx commented Sep 26, 2024

wojtek-t commented Sep 26, 2024

k8s-ci-robot commented Sep 26, 2024

k8s-ci-robot commented Sep 26, 2024

Chaunceyctx commented Aug 30, 2024 •

edited

Loading

linux-foundation-easycla bot commented Aug 30, 2024 •

edited

Loading

Chaunceyctx commented Sep 11, 2024 •

edited

Loading

Chaunceyctx Sep 20, 2024 •

edited

Loading

Chaunceyctx Sep 26, 2024 •

edited

Loading