Repository navigation
Return NotFound to WatchList clients when a shard does not serve the resource - #4384
Conversation
kcp-ci-bot
added
dco-signoff: yes
|
Hi @waldemar-kindler. 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. |
|
/ok-to-test |
kcp-ci-bot
added
ok-to-test
…resource A watch for a resource that is not served on this shard is answered with an empty watch that yields no events and stays open, so that a wildcard consumer aggregating across shards is not wedged by a shard that simply holds none of these objects. A WatchList client cannot use such a stream. It treats the watch as its initial list and waits for the server to close that list with an "initial-events-end" bookmark before it considers its store synced. An empty watch sends no such bookmark, and never errors either, so nothing makes the client relist — the comment's "objects that later appear on this shard are picked up on the next relist" never happens, because there is no next relist. The client waits forever: reflector.go:327: awaiting required bookmark event for initial events stream, no events received for 10.004121875s That is reachable in a normal deployment. A controller consuming an APIExport through its virtual workspace starts before any workspace has bound the export, so the endpoint slice is still empty; when the first binding appears, the controller connects and establishes a watch for a resource this shard does not serve yet. Its informers never sync and it never reconciles anything, with no error anywhere to explain it. Surface the NotFound to those clients instead: they retry, and pick the resource up once the shard serves it. Ordinary watches keep the empty watch and the behaviour that was added for them. An alternative would be for the empty watch to emit the initial-events-end bookmark itself, which would let a WatchList client sync against a shard that holds nothing. That needs a resourceVersion to report as synced-at, and I did not want to invent one; if there is a right value to use here, that is the better fix. Signed-off-by: Waldemar Kindler <waldemar.kindler@think-ahead.tech>
47405dc to
fb5540b
Compare
|
/test pull-kcp-lint |
|
Thanks @waldemar-kindler for this. Where did you see this? feel link is not working for me. Can you just double-check this still failing on |
|
Tried it with today's main branch. Sorry for the internal link. Project is not published yet. |
|
Ok, let see what CI says. |
|
/test all |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 530788cac86a8ed9ba58993f0b4350a96aee2f27 |
|
[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 |
kcp-ci-bot
added
the
approved
kcp-ci-bot
added
release-note
|
Next time please fill the PR template as CI will not allow to merge it without it |
I'm so sorry. Thanks for letting me know. Will do next time and thanks for the super quick response! |
Use kcp, kcp-front-proxy and cache-server from the ghcr.io/kcp-dev/kcp:4ac7658dd image in the envtest suites until a v0.33.x release has kcp-dev/kcp#4384. Retry WorkspaceType creation in the account-operator and security-operator e2e setup. The bind permission check can lag behind a new APIExport on another shard. Signed-off-by: Iskren <iskren@kubermatic.com>
Backend: sigs.k8s.io/multicluster-runtime v0.24.1 with the kcp github.com/kcp-dev/multicluster-provider v0.8.0 apiexport provider, on sigs.k8s.io/controller-runtime v0.24.1. Chosen over the client-go fallback because these are the newest releases built on k8s.io/* v0.36 (no k8s minor bump; multicluster-runtime v0.25 needs v0.37), the provider builds against kcp-dev/sdk v0.33.1, and kcp v0.33 creates the default APIExportEndpointSlice per APIExport, so the provider discovers every workspace that binds an export through the APIExport virtual workspace, including workspaces created after the core started. kcp v0.33.x mitigation (kcp-dev/kcp#4384, fixed only on kcp main): an APIExport virtual workspace answers a WatchList request for an export that no workspace binds yet with an empty watch and no initial-events bookmark, so an informer started before the first binding never syncs. NewManager turns the client-go WatchListClient gate off in code (once, for the process, before the manager builds any informer), so the runtime's reflectors use List+Watch. The gate is process wide in client-go; the core's other reflectors fall back to List+Watch too. internal/controllers/runtime wraps it: Manager (one multi provider, one apiexport provider per APIExport), For[T] with a typed Reconciler[T], Request with cluster, workspace path and name, Result.RequeueAfter, Owns[C], Watches[R](mapFunc), Export, FieldManager (default socialweb.computer/<controller>), Finalizer, Backoff and Workers options, SSA status and spec apply helpers, and a per-item exponential backoff on error. No ticker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kcp now runs from kcp-dev/kcp main at 74e9fb9c03f7. kcp-dev/sdk 91bab06689ae (Kcp-commit f32a28e56fb9) is the sdk of that tree: no file under staging/src/github.com/kcp-dev/sdk changed between f32a28e56fb9 and 74e9fb9c03f7. Its pseudo-version v0.28.1-0.20261021074416-91bab06689ae sorts below v0.33.1, so go.mod pins it with a replace. Its kcp-dev/apimachinery/v2 commit 31b7d7f63736 is v2.33.1, and it stays on k8s.io/* v0.36. multicluster-provider moves to main bfda949c3fd1 (v0.8.1-0.20260813225908-bfda949c3fd1): it builds against that sdk, stays on controller-runtime and multicluster-runtime v0.24.1, retries endpoints that are not ready yet and no longer logs whole objects at info level. kcp main has kcp-dev/kcp#4384 and #4376, so the runtime no longer turns the client-go WatchListClient gate off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/cherry-pick release-0.33 |
|
@ntnn: new pull request created: #4546 DetailsIn response to this:
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. |
|
@mjudeikis: new pull request could not be created: failed to create pull request against kcp-dev/kcp#release-0.33 from head kcp-ci-bot:cherry-pick-4384-to-release-0.33: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for kcp-ci-bot:cherry-pick-4384-to-release-0.33."}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request","status":"422"} DetailsIn response to this:
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. |
Summary
A controller consuming an APIExport through its virtual workspace never syncs its informers, and never reconciles anything, if it starts before any workspace has bound that export. There is no error anywhere — it simply sits there.
DefaultDynamicDelegatedStoreFuncs' watcher answers aNotFoundfrom the delegate with an empty watch that yields no events and stays open, added in 8d385dd so that a wildcard consumer aggregating across shards is not wedged by a shard that holds none of these objects.A WatchList client cannot use that stream. It treats the watch as its initial list and waits for the server to close the list with an
initial-events-endbookmark before considering its store synced. An empty watch sends no bookmark and never errors, so nothing makes it relist — the comment's "objects that later appear on this shard are picked up on the next relist" never happens, because there is no next relist:This is easy to hit in a normal deployment: a controller starts, the
APIExportEndpointSliceis still empty, the first binding appears, the controller connects and watches a resource this shard does not serve yet, and from then on its informers never sync.The change
Surface the
NotFoundto WatchList clients, which retry and pick the resource up once the shard serves it. Ordinary watches are untouched and keep the empty watch.Alternative, and a question for reviewers
The fuller fix would be for the empty watch to emit the
initial-events-endbookmark itself, so a WatchList client can sync against a shard that holds nothing. That needs a resourceVersion to report as synced-at, and I did not want to invent one — if there is a right value to use here, that is the better fix and I am happy to rework this.How it was found, and how to reproduce
Bisected from a downstream controller (fleet) whose integration suite passes on v0.32.3 and fails on v0.33.0. The variable is when the controller starts, each run in its own process:
Reproducing it needs only that ordering: publish an APIExport, start a controller against its virtual workspace with nothing bound yet, then create a workspace that binds it. On v0.32.3 the controller engages; on v0.33.0 it logs the reflector warning above every ten seconds until it gives up.
Everything else was ruled out by measurement along the way: the client's sdk/apimachinery version (fails with both 2.32.3 and the matching 2.33.0), multicluster-provider including its unreleased main, kcp main, single-shard vs sharded topology, and every hand-made LIST/WATCH shape — including a direct WatchList request, which returns its bookmark correctly when the resource is served.
Testing
NotFound. It fails without the change (expected a NotFound, got <nil>).pkg/forwardingregistrypackage tests pass.