feat: propagate context through Runtime interface for cancellation support - #1288
feat: propagate context through Runtime interface for cancellation support#1288shwetha-s-poojary wants to merge 7 commits into
Conversation
mayuka-c
left a comment
mayuka-c
left a comment
There was a problem hiding this comment.
@shwetha-s-poojary Please ensure its thoroughly validated and there are no regressions
7e6bf28 to
98490f3
Compare
yussufsh
left a comment
yussufsh
left a comment
There was a problem hiding this comment.
Wondering if we already have the ctx in the podman and openshift client why are we not using it? If we are passing in each func call then please remove it from the client struct.
If you are referring to pc.Context and ctx, pc.Context on the Podman client is not a cancellation context - it carries the Podman connection handle, we have podmanCtx() to merge it with the caller's ctx to get both the connection value and cancellation.We can't remove it IMO. |
3eab0b2 to
1e3b4d7
Compare
…pport Signed-off-by: shwetha-s-poojary <shwetha.s-poojary@ibm.com>
Signed-off-by: shwetha-s-poojary <shwetha.s-poojary@ibm.com>
Add ctx context.Context as first parameter to all Runtime interface methods and update all implementations (openshift, podman, remote), the worker dispatch table, and every call site across the codebase. Signed-off-by: shwetha-s-poojary <shwetha.s-poojary@ibm.com>
Signed-off-by: shwetha-s-poojary <shwetha.s-poojary@ibm.com>
Signed-off-by: shwetha-s-poojary <shwetha.s-poojary@ibm.com>
1e3b4d7 to
326bce6
Compare
Signed-off-by: shwetha-s-poojary <shwetha.s-poojary@ibm.com>
| secretExistsFn := func(nameOrID string) (bool, error) { return rt.SecretExists(ctx, nameOrID) } | ||
| volumeExistsFn := func(nameOrID string) (bool, error) { return rt.VolumeExists(ctx, nameOrID) } | ||
|
|
||
| if msg := validateResourceExistence(ctx, expectedSecretNames, "secret", secretExistsFn); msg != "" { |
There was a problem hiding this comment.
Why is this need for func vars?
You can change the signature of validateResourceExistence() itself.
| } | ||
|
|
||
| // Wait before next poll | ||
| time.Sleep(pollInterval) |
There was a problem hiding this comment.
Minor concern: This waits for 5 seconds even if ctx is cancelled in future. If a signal arrives mid-sleep, the process will hang for up to 5 seconds before the next iteration. Let's add a TODO if it's complex, or fix it now.
| // This orchestrates: deployContext gets pod name and route info from templates, | ||
| // caddy.Context queries Caddy for route domains and HTTPS port. | ||
| func GetCatalogRouteInfo(rt *rt.PodmanClient) (map[string]string, string, error) { | ||
| func GetCatalogRouteInfo(ctx context.Context, rt *rt.PodmanClient) (map[string]string, string, error) { //nolint:revive |
There was a problem hiding this comment.
Why nolint?
Something related to previous code?
|
|
||
| // HealthCheck verifies Caddy is running and accessible. | ||
| func (c *caddyManager) HealthCheck() error { | ||
| func (c *caddyManager) HealthCheck(ctx context.Context) error { |
There was a problem hiding this comment.
You are missing adding client ctx for GetRouteByID and UnregisterRoute.
3 participants
Added
ctx context.Contextto all methods on theRuntimeinterface so that cancellation signals propagate correctly through the entire call chain.Why each group changed:
runtime/interface.go+ implementations (Podman, OpenShift) — core change; every method now acceptsctxso callers can cancel in-flight operationsapplication/podman&openshift(create.go,delete.go) — deployment path:ctxnow cancels image pulls, pod creation, and readiness waits mid-flightcli/podman/deploy.go,cli/helpers/helper.go— readiness wait loops; cancellation stops waiting immediatelyimage/— image pull respects cancellationcatalog/apiserver/services/deployment/,deletion/— API-triggered create/delete pathapplication/{podman,openshift}(info.go,logs.go,ps.go,start.go,stop.go) +catalog/cli/configure/podmanreset functions +catalog/apiserver/services/sync— accept and propagatectxfrom the cmd layer (cmd.Context()) so the full call chain is consistentValidation:
Note:
cmd.Context()currently behaves the same ascontext.Background()since OS signal wiring at the root command is not yet in place. This change establishes the correct plumbing so that when signal handling is added (tracked separately), cancellation will propagate end-to-end with no further changes required in the internal packages.