Skip to content

Add pkg/contextmanager#4140

Merged
kcp-ci-bot merged 18 commits into
kcp-dev:mainfrom
ntnn:cluster-connection-cancellation
May 22, 2026
Merged

Add pkg/contextmanager#4140
kcp-ci-bot merged 18 commits into
kcp-dev:mainfrom
ntnn:cluster-connection-cancellation

Conversation

@ntnn
Copy link
Copy Markdown
Member

@ntnn ntnn commented May 21, 2026

Summary

For migrating logical clusters we need to break client watches.

While implementing this solution I remembered the inactive annotation on logical clusters, which suffers the same problem.

This is the feature cut out of the migration feature branch.

I think breaking the wildcard watches as well is a bit heavy handed but required, as a wildcard watch might target objects in a cluster being marked as inactive (or being migrated).

What Type of PR Is This?

/kind bug
/kind feature

Related Issue(s)

Fixes #

Release Notes

New logical cluster annotation `LogicalClusterInactiveAnnotationKey` (`core.kcp.io/inactive`) and a respecitve phase to mark logical clusters as inactive
Deprecated the internal annotation `internal.kcp.io/inactive`, replaced with `LogicalClusterInactiveAnnotationKey`
Active client watches will be cancelled on logical clusters that are marked as inactive
Active wildcard watches will be cancelled when a logical cluster is marked as inactive

@kcp-ci-bot kcp-ci-bot 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. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2026
@ntnn ntnn changed the title Add pkg/contextmanager [WIP] Add pkg/contextmanager May 21, 2026
@kcp-ci-bot kcp-ci-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2026
@ntnn ntnn added this to tbd May 21, 2026
@ntnn ntnn moved this to In review in tbd May 21, 2026
@ntnn ntnn force-pushed the cluster-connection-cancellation branch from fb9feb2 to 9bcdb32 Compare May 21, 2026 09:58
@ntnn ntnn changed the title [WIP] Add pkg/contextmanager Add pkg/contextmanager May 21, 2026
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2026
Copy link
Copy Markdown
Contributor

@mjudeikis mjudeikis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2026
@kcp-ci-bot
Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 01b795c8a893ae167a3b34e04c7fa586d541e9d3

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2026
@ntnn
Copy link
Copy Markdown
Member Author

ntnn commented May 21, 2026

/retest

Flakes

@ntnn
Copy link
Copy Markdown
Member Author

ntnn commented May 21, 2026

/test pull-kcp-test-e2e-multiple-runs
flake

@ntnn ntnn force-pushed the cluster-connection-cancellation branch from 9bcdb32 to e11e9ca Compare May 22, 2026 07:27
@kcp-ci-bot kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label May 22, 2026
@kcp-ci-bot kcp-ci-bot requested a review from mjudeikis May 22, 2026 07:27
@ntnn ntnn requested a review from Copilot May 22, 2026 07:28
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a per-logical-cluster context management mechanism so the server can proactively cancel long-lived client requests (notably watches) when a logical cluster is marked inactive (and during certain lifecycle transitions like deleting). It adds a small generic context manager package, wires a new request filter into the main handler chain, and triggers cancellations from the LogicalCluster reconciler, with an accompanying e2e test update.

Changes:

  • Add pkg/contextmanager to create/cancel contexts keyed by logical cluster (and wildcard).
  • Add a new server filter to wrap each request with a per-cluster managed context, and cancel all managed contexts during server shutdown.
  • Extend LogicalCluster phase reconciliation to set an Inactive phase and cancel per-cluster and wildcard contexts when the inactive annotation is applied; update e2e coverage for per-cluster watch termination.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/e2e/workspace/inactive_test.go Opens a watch before inactivation and asserts it is terminated; verifies watches can be reopened after re-activation.
staging/src/github.com/kcp-dev/sdk/apis/core/v1alpha1/logicalcluster_types.go Adds LogicalClusterPhaseInactive constant and documentation.
pkg/server/server.go Adds a pre-shutdown hook to cancel all managed contexts.
pkg/server/filters/perclustercontext.go New filter to derive request contexts from both request context and cluster-keyed contexts.
pkg/server/filters/inactivelogicalcluster.go Comment update clarifying annotation-based early blocking.
pkg/server/controllers.go Wires the shared ClusterContextManager into the logicalcluster controller.
pkg/server/config.go Instantiates ClusterContextManager and inserts WithPerClusterContext into the handler chain.
pkg/reconciler/core/logicalcluster/logicalcluster_reconcile.go Passes context manager into the phase reconciler.
pkg/reconciler/core/logicalcluster/logicalcluster_reconcile_phase.go Cancels per-cluster + wildcard contexts when a cluster becomes inactive; adds phase transitions for Inactive.
pkg/reconciler/core/logicalcluster/logicalcluster_controller.go Extends controller constructor/state to accept/store the context manager.
pkg/contextmanager/doc.go Package documentation for the new context manager.
pkg/contextmanager/contextmanager.go Implements the generic keyed context manager with cancellation support.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/server/filters/perclustercontext.go Outdated
Comment thread pkg/server/filters/inactivelogicalcluster.go Outdated
Comment thread pkg/contextmanager/contextmanager.go Outdated
Comment thread test/e2e/workspace/inactive_test.go Outdated
ntnn added 2 commits May 22, 2026 11:09
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 <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
@ntnn ntnn force-pushed the cluster-connection-cancellation branch from e11e9ca to 51e8203 Compare May 22, 2026 09:11
@kcp-ci-bot kcp-ci-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 22, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

pkg/contextmanager/rootCtx_test.go:177

  • sync.WaitGroup does not have a Go method; this test won’t compile. Use wg.Add(1) before starting each goroutine and defer wg.Done() inside the goroutine instead of wg.Go(...).
	for range readers {
		wg.Go(func() {
			for {
				select {
				case <-stop:

Comment thread pkg/server/filters/inactivelogicalcluster.go Outdated
Comment thread pkg/contextmanager/rootCtx_test.go
Comment thread pkg/contextmanager/contextmanager.go Outdated
@ntnn ntnn force-pushed the cluster-connection-cancellation branch 6 times, most recently from d856008 to 7f44781 Compare May 22, 2026 11:24
ntnn added 13 commits May 22, 2026 13:58
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 <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
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 <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
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 <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
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 <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
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 <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
… requests, scoped watch and wildcard watch

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

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 <red.brush9525@fastmail.com>
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
@ntnn ntnn force-pushed the cluster-connection-cancellation branch from 7f44781 to 4000587 Compare May 22, 2026 12:03
@mjudeikis
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2026
@kcp-ci-bot
Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 2ec5764d2dbaaa70f83aa87b76dcb4fdfac9d4bc

@kcp-ci-bot
Copy link
Copy Markdown
Contributor

[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:

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

@ntnn
Copy link
Copy Markdown
Member Author

ntnn commented May 22, 2026

/retest

Flake

@ntnn
Copy link
Copy Markdown
Member Author

ntnn commented May 22, 2026

/retest

Flake

@kcp-ci-bot kcp-ci-bot merged commit 49b6b80 into kcp-dev:main May 22, 2026
14 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in tbd May 22, 2026
@ntnn ntnn deleted the cluster-connection-cancellation branch May 22, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants