Add pkg/contextmanager#4140
Conversation
fb9feb2 to
9bcdb32
Compare
|
LGTM label has been added. DetailsGit tree hash: 01b795c8a893ae167a3b34e04c7fa586d541e9d3 |
|
/retest Flakes |
|
/test pull-kcp-test-e2e-multiple-runs |
9bcdb32 to
e11e9ca
Compare
There was a problem hiding this comment.
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/contextmanagerto 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
Inactivephase 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.
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>
e11e9ca to
51e8203
Compare
There was a problem hiding this comment.
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.WaitGroupdoes not have aGomethod; this test won’t compile. Usewg.Add(1)before starting each goroutine anddefer wg.Done()inside the goroutine instead ofwg.Go(...).
for range readers {
wg.Go(func() {
for {
select {
case <-stop:
d856008 to
7f44781
Compare
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>
7f44781 to
4000587
Compare
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 2ec5764d2dbaaa70f83aa87b76dcb4fdfac9d4bc |
|
[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 |
|
/retest Flake |
|
/retest Flake |
Summary
For migrating logical clusters we need to break client watches.
While implementing this solution I remembered the
inactiveannotation 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