From 6058540f3d0edc405a1f1b8a96bd82ceca99c240 Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Fri, 1 Sep 2023 10:48:36 +0200 Subject: [PATCH 1/2] storage: document ProgressNotify from storage.ListOptions At first glance, it seems that the fields storage.ListOptions.ProgressNotify and storage.ListOptions.Predicate.AllowWatchBookmarks are the same. Unfortunately, this is not the case. This PR documents the differences and motivations for why these fields are actually distinct. --- .../src/k8s.io/apiserver/pkg/storage/interfaces.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go index 76123fde864..5489660809d 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go @@ -282,6 +282,19 @@ type ListOptions struct { Recursive bool // ProgressNotify determines whether storage-originated bookmark (progress notify) events should // be delivered to the users. The option is ignored for non-watch requests. + // + // Firstly, note that this field is different from the Predicate.AllowWatchBookmarks field. + // Secondly, this field is intended for internal clients only such as the watch cache. + // + // This means that external clients do not have the ability to set this field directly. + // For example by setting the allowWatchBookmarks query parameter. + // + // The motivation for this approach is the fact that the frequency + // of bookmark events from a storage like etcd might be very high. + // As the number of watch requests increases, the server load would also increase. + // + // Furthermore, the server is not obligated to provide bookmark events at all, + // as described in https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/956-watch-bookmark#proposal ProgressNotify bool // SendInitialEvents, when set together with Watch option, // begin the watch stream with synthetic init events to build the From 875b00137fdfbc74756a0fc5b1c9b2adbeb78e55 Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Fri, 1 Sep 2023 13:45:44 +0200 Subject: [PATCH 2/2] storage/etcd: add TestWatchDispatchBookmarkEvents unit test --- .../apiserver/pkg/storage/cacher/cacher_test.go | 2 +- .../apiserver/pkg/storage/etcd3/watcher_test.go | 11 +++++++++++ .../apiserver/pkg/storage/testing/watcher_tests.go | 4 ++-- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_test.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_test.go index 2a8f2b7e6fa..c4ca88ef699 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_test.go @@ -312,7 +312,7 @@ func TestNamespaceScopedWatch(t *testing.T) { func TestWatchDispatchBookmarkEvents(t *testing.T) { ctx, cacher, terminate := testSetup(t) t.Cleanup(terminate) - storagetesting.RunTestWatchDispatchBookmarkEvents(ctx, t, cacher) + storagetesting.RunTestWatchDispatchBookmarkEvents(ctx, t, cacher, true) } func TestWatchBookmarksWithCorrectResourceVersion(t *testing.T) { diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go index 7fa579aae3f..c5edcc6a0af 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go @@ -104,6 +104,17 @@ func TestProgressNotify(t *testing.T) { storagetesting.RunOptionalTestProgressNotify(ctx, t, store) } +// TestWatchDispatchBookmarkEvents makes sure that +// setting allowWatchBookmarks query param against +// etcd implementation doesn't have any effect. +func TestWatchDispatchBookmarkEvents(t *testing.T) { + clusterConfig := testserver.NewTestConfig(t) + clusterConfig.ExperimentalWatchProgressNotifyInterval = time.Second + ctx, store, _ := testSetup(t, withClientConfig(clusterConfig)) + + storagetesting.RunTestWatchDispatchBookmarkEvents(ctx, t, store, false) +} + func TestSendInitialEventsBackwardCompatibility(t *testing.T) { ctx, store, _ := testSetup(t) storagetesting.RunSendInitialEventsBackwardCompatibility(ctx, t, store) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go b/staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go index 4955dbcc570..1f08b11c2a5 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/testing/watcher_tests.go @@ -1042,7 +1042,7 @@ func RunTestNamespaceScopedWatch(ctx context.Context, t *testing.T, store storag // TODO(#109831): ProgressNotify feature is effectively implementing the same // // functionality, so we should refactor this functionality to share the same input. -func RunTestWatchDispatchBookmarkEvents(ctx context.Context, t *testing.T, store storage.Interface) { +func RunTestWatchDispatchBookmarkEvents(ctx context.Context, t *testing.T, store storage.Interface, expectedWatchBookmarks bool) { key, storedObj := testPropagateStore(ctx, t, store, &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test-ns"}}) startRV := storedObj.ResourceVersion @@ -1061,7 +1061,7 @@ func RunTestWatchDispatchBookmarkEvents(ctx context.Context, t *testing.T, store { name: "allowWatchBookmarks=true", timeout: 3 * time.Second, - expected: true, + expected: expectedWatchBookmarks, allowWatchBookmarks: true, }, }