Handle cluster migrations gracefully in front-proxy by ntnn · Pull Request #4219 · kcp-dev/kcp

ntnn · 2026-06-20T20:33:37Z

Summary

Fixes the flake with the pre-migration watch.

What Type of PR Is This?

/kind bug
/kind flake

Related Issue(s)

Fixes #

Release Notes

NONE

ntnn · 2026-06-20T22:11:15Z

/retest

ntnn · 2026-06-20T22:52:49Z

/restart

ntnn · 2026-06-21T04:37:08Z

/retest

time=2026-06-20T21:50:58.066Z level=ERROR source=manager.go:259 msg="error reloading target set" component="scrape manager" err="invalid config id:kcp-shared-TestUserWithWarrants-1781992249"

gman0 · 2026-06-24T14:41:06Z

+		// This is only required for watches.
+		// Lists with a lower RV will get appropriate objects from the shard.
+		// Lists with a higher RV will get a 504 from the shard.
+		if index.RecentlyMigrated(result.Cluster) && isInProgressWatch(req) {


Hmm, what about clients that HAVE done a list before, already know a good RV on the new shard, and want to watch from there on; all while this 90s period is ticking? Maybe the error message could be a bit more helpful, like: ...; watch with resourceVersion temporarily unavailable, or something along those lines.

This could be called a pretty hard nit-picking, but a random user, looking at logs of their client might not know what is going on.

Lists shouldn't be affected, they are just passed through to the shard and as mentioned in the comment they'll either have a lower RV than the new shard, in which case they'll get a diff to the current RV of the new shard.
If they have a higher RV than the new shard the shard will respond with a 504, causing the client to do a relist.

The original implementation also rejected lists but the more I read the code for reflectors the less it made sense to handle lists instead of letting them through^^

@ntnn, what I meant is that the user does List, knows the correct RV, and does a subsequent Watch starting from that point in time.

It's fine for Watch to reject - I don't disagree with that. Just the log msg is hiding the real reason for the rejection and would (at least for me) read as something unrelated being wrong, whereas in fact it's only a temporary thing.

I updated the error message, I think its clearer now

gman0 · 2026-06-24T14:56:11Z

+	// A watch opened *after* the migration must be live, not stuck in a sticky
+	// cancelled state. This guards against using a sticky Cancel instead of a
+	// one-shot Delete, which would permanently kill all watches for the cluster.


Huh? Sounds like the agent had a bit more context than what's in the comment here :P

:D Sortof yes, the comments in the original code (from where I copied the context handling) talked about the sticky cancellation and thats what it ran with when building the tests :D

Yeah but can you pls expand on that thought INSIDE the comment? :D Otherwise nobody knows what that means. Or maybe it's just that I'm uneducated and this is common knowledge?

True :D Updated with a comment on how the contextmanager handles the context

mjudeikis

/lgtm
/approve

kcp-ci-bot · 2026-07-13T07:45:21Z

LGTM label has been added.

Details

Git tree hash: 56d5c699a0bd63491663fc41e8642ff28479d112

gman0 · 2026-07-13T08:04:44Z

@mjudeikis were the comments I sent resolved, or the review overall was deemed without merit and it was decided that this can go ahead?

mjudeikis · 2026-07-13T08:37:41Z

@mjudeikis were the comments I sent resolved, or the review overall was deemed without merit and it was decided that this can go ahead?

Sorry, I think I tagged this by mistake while going via all backlist
/approve cancel

mjudeikis · 2026-09-10T13:53:08Z

/lgtm
/approve

I need this as its keep flaking in CI

kcp-ci-bot · 2026-09-10T13:53:14Z

LGTM label has been added.

Details

Git tree hash: db98c462c4c231f66a6b46f1dd4d8a03abec9d28

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

ntnn · 2026-09-10T14:27:27Z

/retest

That looks like infra errors?

mjudeikis · 2026-09-10T14:46:46Z

/retest
Ci infra

kcp-ci-bot · 2026-09-10T14:59:21Z

@ntnn: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-test-upgrade	`a57e5af`	link	false	`/test pull-kcp-test-upgrade`

Full PR test history

Details

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. I understand the commands that are listed here.

mjudeikis · 2026-09-11T06:08:58Z

/lgtm
/approve

kcp-ci-bot · 2026-09-11T06:09:04Z

LGTM label has been added.

Details

Git tree hash: bfe1a36d6961993c5a02c3d39091175df1a7879b

kcp-ci-bot · 2026-09-11T06:09:07Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mjudeikis]

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

ntnn force-pushed the watch-through-fp branch from cc84c73 to cf366e0 Compare June 20, 2026 21:39

ntnn mentioned this pull request Jun 22, 2026

VAP/MAP improvements #4217

Merged

gman0 reviewed Jun 24, 2026

View reviewed changes

platform-mesh-publisher Bot added this to Platform Mesh & Kube Projects - Backlog Jun 29, 2026

mjudeikis reviewed Jul 13, 2026

View reviewed changes

kcp-ci-bot assigned mjudeikis Jul 13, 2026

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2026

kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2026

kcp-ci-bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2026

kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2026

kcp-ci-bot requested a review from mjudeikis September 10, 2026 13:49

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2026

kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2026

ntnn added 3 commits September 10, 2026 16:05

Handle cluster migrations gracefully in front-proxy

65310cd

Signed-off-by: Nelo-T. Wallus <red.brush9525@fastmail.com> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Clarify error message for watches with a resource version set

5f03799

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Clarify context verification in test

0b372eb

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

ntnn added 2 commits September 10, 2026 16:05

Use t.Cleanup

bf7c987

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Use NewRequestWithContext

a57e5af

Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

ntnn force-pushed the watch-through-fp branch from f469cf5 to a57e5af Compare September 10, 2026 14:05

kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2026

ntnn requested a review from gman0 September 10, 2026 14:05

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2026

kcp-ci-bot merged commit 11c2789 into kcp-dev:main Sep 11, 2026
16 of 17 checks passed

github-project-automation Bot moved this to Done in Platform Mesh & Kube Projects - Backlog Sep 11, 2026

Conversation

ntnn commented Jun 20, 2026

Summary

What Type of PR Is This?

Related Issue(s)

Release Notes

Uh oh!

ntnn commented Jun 20, 2026

Uh oh!

ntnn commented Jun 20, 2026

Uh oh!

ntnn commented Jun 21, 2026

Uh oh!

gman0 Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjudeikis left a comment

Choose a reason for hiding this comment

Uh oh!

kcp-ci-bot commented Jul 13, 2026

Uh oh!

gman0 commented Jul 13, 2026

Uh oh!

mjudeikis commented Jul 13, 2026

Uh oh!

mjudeikis commented Sep 10, 2026

Uh oh!

kcp-ci-bot commented Sep 10, 2026

Uh oh!

ntnn commented Sep 10, 2026

Uh oh!

mjudeikis commented Sep 10, 2026

Uh oh!

kcp-ci-bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjudeikis commented Sep 11, 2026

Uh oh!

kcp-ci-bot commented Sep 11, 2026

Uh oh!

kcp-ci-bot commented Sep 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gman0 Jun 24, 2026 •

edited

Loading

kcp-ci-bot commented Sep 10, 2026 •

edited

Loading