fix(live): propagate caller ctx into ResourceGroup inventory ops#4501
Open
Jaisheesh-2006 wants to merge 3 commits intokptdev:mainfrom
Open
fix(live): propagate caller ctx into ResourceGroup inventory ops#4501Jaisheesh-2006 wants to merge 3 commits intokptdev:mainfrom
Jaisheesh-2006 wants to merge 3 commits intokptdev:mainfrom
Conversation
pkg/live/inventoryrg.go passed context.TODO() to all 7 of its live cluster API calls, so Ctrl-C and command-level timeouts could not cancel an in-flight apply/destroy/plan. The upstream inventory.Storage interface doesn't accept a ctx parameter, so carry ctx on the InventoryResourceGroup struct and inject it at wrap time via a new WrapInventoryObjWithContext factory. Legacy callers without a ctx fall back to context.Background(). ResourceGroupCRDMatched takes ctx directly. All 6 callers updated. Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
Jaisheesh-2006
requested review from
efiacor,
kispaljr,
liamfallon and
mozesl-nokia
as code owners
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
Bot
added
size:L
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR ensures caller cancellation/timeouts propagate into live Kubernetes inventory operations by threading a real context.Context through ResourceGroup-based inventory calls instead of using context.TODO().
Changes:
- Store a caller
ctxon inventory wrappers/factories and use it for live K8s API calls inApply/ApplyWithPrune. - Update multiple command paths to pass their existing command context into inventory client creation and CRD-match checks.
- Add focused unit tests validating context storage and fallback behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/live/planner/cluster.go | Plumbs caller ctx into inventory wrapper when building planner’s inventory client. |
| pkg/live/inventoryrg.go | Stores ctx on InventoryResourceGroup and uses it for dynamic client calls; updates CRD match helper to accept ctx. |
| pkg/live/inventory-client-factory.go | Adds context-aware factory constructor and uses ctx-aware wrapper when provided. |
| commands/live/apply/cmdapply.go | Passes runner ctx into CRD check and inventory client creation for apply. |
| commands/live/destroy/cmddestroy.go | Passes runner ctx into inventory client creation for destroy. |
| commands/live/livecmd.go | Constructs inventory factory with command ctx. |
| commands/alpha/live/plan/command.go | Updates planner construction to pass runner ctx. |
| commands/live/migrate/migratecmd.go | Updates inventory client factory funcs to accept ctx and passes runner ctx through. |
| commands/live/migrate/migratecmd_test.go | Updates test stubs for new ctx-accepting inventory client funcs. |
| pkg/live/inventoryrg_test.go | Adds unit tests for context propagation and fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Apply/ApplyWithPrune/ResourceGroupCRDMatched passed context.TODO(), so Ctrl-C and command-level timeouts could not cancel in-flight apply/destroy/plan calls. Carry ctx on the InventoryResourceGroup struct and inject it via new WrapInventoryObjWithContext, NewClusterClientFactoryWithContext, ResourceGroupCRDMatchedWithContext, and NewClusterPlannerWithContext. Legacy entry points kept as thin back-compat wrappers; nil ctx normalized to Background(). Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
Jaisheesh-2006
force-pushed
the
fix/live-ctx-propagation
branch
from
3a21b69 to
f39ab39
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
pkg/live/inventoryrg.gowas usingcontext.TODO()for every live Kubernetes API call it makes — 7 call sites coveringApply,ApplyWithPrune, and theResourceGroupCRD match check. This PR threads the caller's real context through each of those calls.Because the upstream
inventory.Storage/inventory.ClientFactoryinterfaces do not accept actxparameter, the fix:ctxonInventoryResourceGroupandClusterClientFactorystructs.WrapInventoryObjWithContext(ctx)andNewClusterClientFactoryWithContext(ctx).context.Background()when the legacy constructors are used, so the change is fully backward-compatible.ctxparameter directly toResourceGroupCRDMatched(not interface-constrained).Six callers updated to pass their existing command ctx:
commands/live/apply,commands/live/destroy,commands/live/migrate,commands/live/livecmd,commands/alpha/live/plan, andpkg/live/planner.Three unit tests added in
pkg/live/inventoryrg_test.go:TestWrapInventoryObjWithContext_StoresContext— verifies the factory persists the passed ctx onto the struct.TestWrapInventoryObj_LeavesContextNil— locks in legacy behavior for callers that haven't migrated.TestContextOrBackground— asserts the fallback path and, crucially, that cancellation on the caller's ctx propagates through (cancel()firesDone()).Motivation
Before this change, pressing
Ctrl-Cduringkpt live apply/destroy/planagainst a slow or unreachable cluster could not cancel the in-flight API call — the CLI would block until the network-level timeout fired. Anycontext.WithTimeoutthe caller wired at the command level was silently discarded at this boundary.context.TODO()is a placeholder from the Go stdlib. This PR fulfills that TODO.Discovered via a
grep-level audit ofmainforcontext.TODO()/context.Background()usage in production code paths.AI use
I have used AI in the creation of this PR.
I have used the following AI tools:
mainfor context-propagation gaps, to plan the interface-safe fix (struct-stored ctx + context-aware factory), to apply the refactor across the 10 affected files, and to author the unit tests.I reviewed every change and can explain the design (why struct-stored ctx instead of adding it to
Apply's signature) and the test assertions independently.