fix: send initial-events-end bookmark for CachedResource virtual storage#3875
Conversation
The unwrappingWatch in the replication virtual workspace never sent a BOOKMARK event with the k8s.io/initial-events-end annotation after replaying the initial list of cached objects. This caused clients using the WatchList feature (sendInitialEvents=true), which is enabled by default since client-go v0.35.1 (Kubernetes 1.35), to log "event bookmark expired" warnings every 10 seconds. After registering the informer event handler, start a goroutine that waits for handler.HasSynced() (all initial items delivered), then sends a BOOKMARK with the initial-events-end annotation and the latest resource version seen during the initial list. Signed-off-by: Max Makarov <maxpain@linux.com>
|
Hi @maxpain. Thanks for your PR. I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
ae60692 to
738d519
Compare
|
/ok-to-test Thanks for the PR! |
|
/kind bug |
|
/retest |
|
/retest |
|
/lgtm Thanks! |
| // existing items. Clients using WatchList (sendInitialEvents=true) expect a | ||
| // BOOKMARK with the k8s.io/initial-events-end annotation to signal that the | ||
| // initial state has been fully delivered. | ||
| if innerListOpts != nil && innerListOpts.SendInitialEvents != nil && *innerListOpts.SendInitialEvents && innerListOpts.AllowWatchBookmarks { |
There was a problem hiding this comment.
| if innerListOpts != nil && innerListOpts.SendInitialEvents != nil && *innerListOpts.SendInitialEvents && innerListOpts.AllowWatchBookmarks { | |
| if ptr.Deref(innerListOpts.SendInitialEvents, false) && ptr.Deref(&innerListOpts.AllowWatchBookmarks, fasle) { |
There was a problem hiding this comment.
Just to make it read nicer. Otherwise lgtm, thanks for the PR!
There was a problem hiding this comment.
please add to follow-up por. will tag to get this ball rolling
There was a problem hiding this comment.
@mjudeikis-bot Create a follow-up PR implementing this change.
There was a problem hiding this comment.
@mjudeikis-bot do the needed ^ create a PR for this please
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 020c525b6bee5175ffce11561c76d61bf98a4174 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjudeikis The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@maxpain can you add release note please? :) |
|
/release-note Send initial-events-end bookmark for CachedResource virtual storage |
…Watch Simplify the SendInitialEvents nil-check using ptr.Deref from k8s.io/utils/ptr, which handles the nil pointer dereference idiom more readably. Suggested-by: ntnn Follows-up: kcp-dev#3875 Co-authored-by: Mangirdas Judeikis <mangirdas@judeikis.lt>
|
Done — opened #3879 with the |
uh oh |
|
/retest |
|
Thanks everyone for the reviews and the release note! Is there anything else needed from my side, or just waiting for CI to pass? |
|
Just waiting for CI to pass, the docker registry pod seems to have problems but that should resolve itself soon^(tm), otherwise someone will have look^^ |
|
/retest |
…Watch Simplify the SendInitialEvents nil-check using ptr.Deref from k8s.io/utils/ptr, which handles the nil pointer dereference idiom more readably. Suggested-by: ntnn Follows-up: kcp-dev#3875 Co-authored-by: Mangirdas Judeikis <mangirdas@judeikis.lt>
…Watch Simplify the SendInitialEvents nil-check using ptr.Deref from k8s.io/utils/ptr, which handles the nil pointer dereference idiom more readably. Suggested-by: ntnn Follows-up: kcp-dev#3875 Co-authored-by: Mangirdas Judeikis <mangirdas@judeikis.lt> Signed-off-by: mjudeikis-bot <mjudeikis-bot@faros.sh> Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt>
Summary
The
unwrappingWatchin the CachedResource virtual storage never sends a BOOKMARK event with thek8s.io/initial-events-endannotation after replaying the initial list of cached objects. This causes clients using the WatchList feature (sendInitialEvents=true), which is the default since client-go v0.35.1 (Kubernetes 1.35), to logevent bookmark expiredwarnings every 10 seconds.Fixes #3871
Changes
After registering the informer event handler, start a goroutine that waits for
handler.HasSynced()(all initial items delivered), then sends a BOOKMARK with theinitial-events-endannotation and the latest resource version seen during the initial list replay.resourceVersionviaatomic.Valueduring initial event replayWaitForCacheSync, send a BOOKMARK withk8s.io/initial-events-end: "true"annotationsendInitialEvents=true && allowWatchBookmarks=trueHow to reproduce
CachedAPIsfeature gateCachedResourcewith identity secretAPIExportwithstorage: virtualreferencing theCachedResourceEndpointSliceevent bookmark expiredwarnings in client logs every 10 seconds