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

kcp-ci-bot merged 18 commits into
kcp-dev:mainfrom
ntnn:cluster-connection-cancellation

Conversation

@ntnn

@ntnn ntnn commented May 21, 2026 •

Copy link
Copy Markdown
Member

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

@mjudeikis mjudeikis left a comment

Copy link
Copy Markdown
Contributor

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

ntnn commented May 21, 2026

Copy link
Copy Markdown
Member Author

/retest

Flakes

@ntnn

ntnn commented May 21, 2026

Copy link
Copy Markdown
Member Author

/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

Copilot AI left a comment

Copy link
Copy Markdown

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 Resolved
Comment thread pkg/contextmanager/contextmanager.go Resolved
Comment thread test/e2e/workspace/inactive_test.go Resolved
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
ntnn added 2 commits May 22, 2026 11:40
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 2 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

ntnn commented May 22, 2026

Copy link
Copy Markdown
Member Author

/retest

Flake

@ntnn

ntnn commented May 22, 2026

Copy link
Copy Markdown
Member Author

/retest

Flake

@kcp-ci-bot
kcp-ci-bot merged commit 49b6b80 into kcp-dev:main May 22, 2026
14 checks passed
@ntnn
ntnn deleted the cluster-connection-cancellation branch May 22, 2026 14:16
@ntnn ntnn assigned ntnn and unassigned mjudeikis May 27, 2026
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