From 2f13ce22dc57569248c6f94a8dc91848bfe07b0a Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:41:55 -0400 Subject: [PATCH] fix(render): stop every function runtime gracefully and always remove Remove-policy containers FunctionAddresses.Stop returned on the first runtime Stop error, leaving the remaining runtimes running. It now attempts every runtime and returns all failures joined. StopFunctionRuntimes shared a single 5s deadline across all runtimes, and ContainerStop used the daemon's default 10s grace period. A container slow to exit on SIGTERM made ContainerStop fail, and the ContainerRemove that should follow was skipped. The Stop and Remove cleanup policies now stop with an explicit 3s grace period. Remove then force removes the container whether or not the stop succeeded, so a slow SIGTERM can no longer skip removal. Removal success means success; if removal fails, the removal and stop errors are joined. The Docker runtime bounds its own cleanup: ContainerStop and ContainerRemove each get their own timeout, so a stop that hangs can't leave removal without time to run. StopFunctionRuntimes only detaches the caller's cancellation, so cleanup still runs after the render context is cancelled; it no longer needs to know the Docker runtime's timings. This changes the exported signature of StopFunctionRuntimes from (logging.Logger, *FunctionAddresses) to (context.Context, *FunctionAddresses) error, matching other cleanup functions in the repo. The xr and op render commands log the returned error as before. Also correct the doc comments: Remove, not Stop, is the default cleanup policy. Fixes #397 Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/op/cmd.go | 6 +- cmd/crossplane/render/render.go | 23 ++- cmd/crossplane/render/render_test.go | 148 +++++++++++++++++++ cmd/crossplane/render/runtime_docker.go | 66 +++++++-- cmd/crossplane/render/runtime_docker_test.go | 71 +++++++-- cmd/crossplane/render/xr/cmd.go | 6 +- 6 files changed, 284 insertions(+), 36 deletions(-) diff --git a/cmd/crossplane/render/op/cmd.go b/cmd/crossplane/render/op/cmd.go index 2274af1d..63a045e2 100644 --- a/cmd/crossplane/render/op/cmd.go +++ b/cmd/crossplane/render/op/cmd.go @@ -218,7 +218,11 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte if err != nil { return errors.Wrap(err, "cannot start function runtimes") } - defer render.StopFunctionRuntimes(log, fnAddrs) + defer func() { + if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { + log.Info("Error stopping function runtimes", "error", err) + } + }() // Build and execute the render request. in := render.OperationInputs{ diff --git a/cmd/crossplane/render/render.go b/cmd/crossplane/render/render.go index 90d406ea..bed3f635 100644 --- a/cmd/crossplane/render/render.go +++ b/cmd/crossplane/render/render.go @@ -21,7 +21,6 @@ package render import ( "context" "strings" - "time" fnv1 "github.com/crossplane/function-sdk-go/proto/v1" corev1 "k8s.io/api/core/v1" @@ -105,14 +104,16 @@ func (fa *FunctionAddresses) Addresses() map[string]string { return fa.addrs } -// Stop all function runtimes. +// Stop all function runtimes. Every runtime is stopped even if some fail; the +// returned error joins all failures. Each runtime bounds its own cleanup. func (fa *FunctionAddresses) Stop(ctx context.Context) error { + var errs []error for name, rctx := range fa.contexts { if err := rctx.Stop(ctx); err != nil { - return errors.Wrapf(err, "cannot stop function %q runtime (target %q)", name, rctx.Target) + errs = append(errs, errors.Wrapf(err, "cannot stop function %q runtime (target %q)", name, rctx.Target)) } } - return nil + return errors.Join(errs...) } // StartFunctionRuntimes starts the runtime for each function and returns their @@ -167,16 +168,14 @@ func injectNetworkAnnotation(fns []pkgv1.Function, networkName string) { } } -// StopFunctionRuntimes stops all function runtimes with a timeout. -func StopFunctionRuntimes(log logging.Logger, fa *FunctionAddresses) { +// StopFunctionRuntimes stops all function runtimes and returns all failures +// joined. Cleanup runs even if ctx is already cancelled, so it can follow an +// interrupted render. Each runtime bounds its own cleanup. +func StopFunctionRuntimes(ctx context.Context, fa *FunctionAddresses) error { if fa == nil { - return - } - stopCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - if err := fa.Stop(stopCtx); err != nil { - log.Info("Error stopping function runtimes", "error", err) + return nil } + return fa.Stop(context.WithoutCancel(ctx)) } // OverrideFunctionAnnotations applies annotation overrides from flags to diff --git a/cmd/crossplane/render/render_test.go b/cmd/crossplane/render/render_test.go index 6d87ae2d..c54f44a8 100644 --- a/cmd/crossplane/render/render_test.go +++ b/cmd/crossplane/render/render_test.go @@ -1,12 +1,15 @@ package render import ( + "context" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" ) @@ -91,3 +94,148 @@ func TestOverrideFunctionAnnotations(t *testing.T) { func functionWithAnnotations(annotations map[string]string) pkgv1.Function { return pkgv1.Function{ObjectMeta: metav1.ObjectMeta{Annotations: annotations}} } + +func TestFunctionAddressesStop(t *testing.T) { + errA := errors.New("boom a") + errB := errors.New("boom b") + + stopReturns := func(err error) RuntimeContext { + return RuntimeContext{Target: "fn:9443", Stop: func(context.Context) error { return err }} + } + + type args struct { + contexts map[string]RuntimeContext + } + type want struct { + // errs are the errors the returned error must wrap. When empty, Stop + // must return nil. + errs []error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "AllSucceed": { + reason: "Stop should return nil when every runtime stops.", + args: args{ + contexts: map[string]RuntimeContext{ + "ok-a": stopReturns(nil), + "ok-b": stopReturns(nil), + }, + }, + }, + "SomeFail": { + reason: "Stop should stop every runtime even if some fail, and return every failure.", + args: args{ + contexts: map[string]RuntimeContext{ + "ok-a": stopReturns(nil), + "fail-a": stopReturns(errA), + "ok-b": stopReturns(nil), + "fail-b": stopReturns(errB), + }, + }, + want: want{ + errs: []error{errA, errB}, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // Repeat so Go's randomized map iteration can't hide a regression + // to returning on the first error. + for range 20 { + fa := &FunctionAddresses{contexts: tc.args.contexts} + + err := fa.Stop(t.Context()) + + wantErrs := tc.want.errs + if len(wantErrs) == 0 { + wantErrs = []error{nil} + } + for _, want := range wantErrs { + if diff := cmp.Diff(want, err, cmpopts.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nStop(...): -want error, +got error:\n%s", tc.reason, diff) + } + } + } + }) + } +} + +func TestStopFunctionRuntimes(t *testing.T) { + errBoom := errors.New("boom") + + type args struct { + // stopErr is what the runtime's Stop returns. + stopErr error + // cancelParent cancels the context passed to StopFunctionRuntimes + // before calling it. + cancelParent bool + // noRuntimes passes nil FunctionAddresses. + noRuntimes bool + } + type want struct { + // ctxErr is the error of the context the runtime's Stop was called + // with. + ctxErr error + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "ParentContextLive": { + reason: "StopFunctionRuntimes should stop each runtime with a live context.", + want: want{}, + }, + "ParentContextCancelled": { + reason: "StopFunctionRuntimes should still stop each runtime, with a live context, when the parent context is already cancelled.", + args: args{cancelParent: true}, + want: want{}, + }, + "StopError": { + reason: "StopFunctionRuntimes should return the error a runtime fails to stop with.", + args: args{stopErr: errBoom}, + want: want{err: errBoom}, + }, + "NilFunctionAddresses": { + reason: "StopFunctionRuntimes should do nothing when there are no runtimes.", + args: args{noRuntimes: true}, + want: want{}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // The context the runtime is stopped with is the property under + // test, so record it. + var got want + var fa *FunctionAddresses + if !tc.args.noRuntimes { + fa = &FunctionAddresses{contexts: map[string]RuntimeContext{ + "fn": {Target: "fn:9443", Stop: func(ctx context.Context) error { + got.ctxErr = ctx.Err() + return tc.args.stopErr + }}, + }} + } + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + if tc.args.cancelParent { + cancel() + } + + got.err = StopFunctionRuntimes(ctx, fa) + + if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(want{}), cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStopFunctionRuntimes(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/cmd/crossplane/render/runtime_docker.go b/cmd/crossplane/render/runtime_docker.go index 8972945b..27b02cec 100644 --- a/cmd/crossplane/render/runtime_docker.go +++ b/cmd/crossplane/render/runtime_docker.go @@ -24,6 +24,7 @@ import ( "net" "net/netip" "strings" + "time" "github.com/containerd/errdefs" "github.com/google/go-containerregistry/pkg/authn" @@ -87,12 +88,15 @@ type DockerCleanup string // Supported AnnotationKeyRuntimeDockerCleanup values. const ( - // AnnotationValueRuntimeDockerCleanupStop is the default. It stops the - // container once rendering is done. + // AnnotationValueRuntimeDockerCleanupStop stops the container once + // rendering is done, waiting up to containerStopGracePeriod for it to exit + // on SIGTERM before Docker kills it. AnnotationValueRuntimeDockerCleanupStop DockerCleanup = "Stop" - // AnnotationValueRuntimeDockerCleanupRemove stops and removes the - // container once rendering is done. + // AnnotationValueRuntimeDockerCleanupRemove is the default. It stops the + // container once rendering is done, waiting up to containerStopGracePeriod + // for it to exit on SIGTERM, then force removes it. The container is + // removed even if the graceful stop fails. AnnotationValueRuntimeDockerCleanupRemove DockerCleanup = "Remove" // AnnotationValueRuntimeDockerCleanupOrphan leaves the container running @@ -102,6 +106,22 @@ const ( AnnotationValueRuntimeDockerCleanupDefault = AnnotationValueRuntimeDockerCleanupRemove ) +const ( + // containerStopGracePeriod is how long the Stop and Remove cleanup + // policies wait for a Function container to exit on SIGTERM before Docker + // kills it. + containerStopGracePeriod = 3 * time.Second + + // containerStopTimeout bounds the ContainerStop request: the grace period + // plus time for Docker to kill the container. + containerStopTimeout = containerStopGracePeriod + 2*time.Second + + // containerRemoveTimeout bounds the ContainerRemove request. It's separate + // from containerStopTimeout so a stop that hangs can't leave removal + // without time to run. + containerRemoveTimeout = 10 * time.Second +) + // AnnotationKeyRuntimeDockerPullPolicy can be added to a Function to control how its runtime // image is pulled. const AnnotationKeyRuntimeDockerPullPolicy = "render.crossplane.io/runtime-docker-pull-policy" @@ -512,19 +532,27 @@ func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) { // Inline stop function stop := func(ctx context.Context) error { + grace := int(containerStopGracePeriod / time.Second) + stopOpts := client.ContainerStopOptions{Timeout: &grace} + switch r.Cleanup { case AnnotationValueRuntimeDockerCleanupOrphan: return nil case AnnotationValueRuntimeDockerCleanupStop: - if _, err := cli.ContainerStop(ctx, containerID, client.ContainerStopOptions{}); err != nil { + if err := stopContainer(ctx, cli, containerID, stopOpts); err != nil { return errors.Wrap(err, "cannot stop Docker container") } case AnnotationValueRuntimeDockerCleanupRemove: - if _, err := cli.ContainerStop(ctx, containerID, client.ContainerStopOptions{}); err != nil { - return errors.Wrap(err, "cannot stop Docker container") + // Give the container a chance to exit gracefully, then force + // remove it whether or not the stop succeeded, so a container + // that's slow to exit on SIGTERM can't cause removal to be + // skipped. A stop failure only matters if removal fails too. + var stopErr error + if err := stopContainer(ctx, cli, containerID, stopOpts); err != nil { + stopErr = errors.Wrap(err, "cannot stop Docker container") } - if _, err := cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{}); err != nil { - return errors.Wrap(err, "cannot remove Docker container") + if err := removeContainer(ctx, cli, containerID); err != nil { + return errors.Join(errors.Wrap(err, "cannot remove Docker container"), stopErr) } } @@ -534,6 +562,26 @@ func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) { return RuntimeContext{Target: containerAddr, Stop: stop}, nil } +// stopContainer stops a container, bounding the request by +// containerStopTimeout. +func stopContainer(ctx context.Context, cli containerCleanupClient, containerID string, opts client.ContainerStopOptions) error { + ctx, cancel := context.WithTimeout(ctx, containerStopTimeout) + defer cancel() + + _, err := cli.ContainerStop(ctx, containerID, opts) + return err +} + +// removeContainer force removes a container, bounding the request by +// containerRemoveTimeout. +func removeContainer(ctx context.Context, cli containerCleanupClient, containerID string) error { + ctx, cancel := context.WithTimeout(ctx, containerRemoveTimeout) + defer cancel() + + _, err := cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{Force: true}) + return err +} + type pullClient interface { ImagePull(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) } diff --git a/cmd/crossplane/render/runtime_docker_test.go b/cmd/crossplane/render/runtime_docker_test.go index 68c2e90c..2fbfd96e 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -22,6 +22,7 @@ import ( "net" "strconv" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -466,6 +467,12 @@ func TestRuntimeDockerStop(t *testing.T) { errStop := errors.New("stop boom") errRemove := errors.New("remove boom") + // The Stop and Remove policies give the container containerStopGracePeriod + // (3s) to exit, and Remove force removes it. + grace := 3 + wantStopOptions := client.ContainerStopOptions{Timeout: &grace} + wantRemoveOptions := client.ContainerRemoveOptions{Force: true} + type args struct { cleanup DockerCleanup // stop and remove are the cleanup client's ContainerStop and @@ -475,7 +482,9 @@ func TestRuntimeDockerStop(t *testing.T) { remove func(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) } type want struct { - err error + // errs are the errors the returned error must wrap. When empty, Stop + // must return nil. + errs []error } cases := map[string]struct { @@ -484,7 +493,7 @@ func TestRuntimeDockerStop(t *testing.T) { want want }{ "Stop": { - reason: "The Stop cleanup policy should stop the container and leave it in place.", + reason: "The Stop cleanup policy should stop the container with the grace period and leave it in place.", args: args{ cleanup: AnnotationValueRuntimeDockerCleanupStop, stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { @@ -501,11 +510,11 @@ func TestRuntimeDockerStop(t *testing.T) { }, }, want: want{ - err: errStop, + errs: []error{errStop}, }, }, "Remove": { - reason: "The Remove cleanup policy should stop the container, then remove it.", + reason: "The Remove cleanup policy should stop the container with the grace period, then force remove it.", args: args{ cleanup: AnnotationValueRuntimeDockerCleanupRemove, stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { @@ -517,15 +526,15 @@ func TestRuntimeDockerStop(t *testing.T) { }, }, "RemoveStopError": { - reason: "The Remove cleanup policy should stop the container before removing it, and return an error without removing it when it cannot be stopped.", + reason: "The Remove cleanup policy should still force remove the container when the graceful stop fails, and succeed if removal does.", args: args{ cleanup: AnnotationValueRuntimeDockerCleanupRemove, stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { return client.ContainerStopResult{}, errStop }, - }, - want: want{ - err: errStop, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, nil + }, }, }, "RemoveError": { @@ -540,7 +549,22 @@ func TestRuntimeDockerStop(t *testing.T) { }, }, want: want{ - err: errRemove, + errs: []error{errRemove}, + }, + }, + "RemoveStopAndRemoveError": { + reason: "The Remove cleanup policy should return both errors when the container can be neither stopped nor removed.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, errRemove + }, + }, + want: want{ + errs: []error{errRemove, errStop}, }, }, "Orphan": { @@ -558,14 +582,21 @@ func TestRuntimeDockerStop(t *testing.T) { MockContainerStart: startContainer(containerID), MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), } + // A stop error is discarded when removal succeeds, so report + // unexpected arguments with t.Errorf rather than as an error. if stop := tc.args.stop; stop != nil { cli.MockContainerStop = func(ctx context.Context, id string, options client.ContainerStopOptions) (client.ContainerStopResult, error) { if diff := cmp.Diff(containerID, id); diff != "" { t.Errorf("\n%s\nContainerStop(...): -want container ID, +got container ID:\n%s", tc.reason, diff) } - if diff := cmp.Diff(client.ContainerStopOptions{}, options); diff != "" { + if diff := cmp.Diff(wantStopOptions, options); diff != "" { t.Errorf("\n%s\nContainerStop(...): -want options, +got options:\n%s", tc.reason, diff) } + // The stop request must have its own deadline, so a stop + // that hangs can't stall cleanup. + if deadline, ok := ctx.Deadline(); !ok || time.Until(deadline) > containerStopTimeout { + t.Errorf("\n%s\nContainerStop(...): want a deadline within %s, got deadline %v (set: %t)", tc.reason, containerStopTimeout, deadline, ok) + } return stop(ctx, id, options) } } @@ -574,9 +605,14 @@ func TestRuntimeDockerStop(t *testing.T) { if diff := cmp.Diff(containerID, id); diff != "" { t.Errorf("\n%s\nContainerRemove(...): -want container ID, +got container ID:\n%s", tc.reason, diff) } - if diff := cmp.Diff(client.ContainerRemoveOptions{}, options); diff != "" { + if diff := cmp.Diff(wantRemoveOptions, options); diff != "" { t.Errorf("\n%s\nContainerRemove(...): -want options, +got options:\n%s", tc.reason, diff) } + // The remove request must have its own deadline, so a stop + // that hangs can't leave it without time to run. + if deadline, ok := ctx.Deadline(); !ok || time.Until(deadline) > containerRemoveTimeout || ctx.Err() != nil { + t.Errorf("\n%s\nContainerRemove(...): want a live context with a deadline within %s, got deadline %v (set: %t), err %v", tc.reason, containerRemoveTimeout, deadline, ok, ctx.Err()) + } return remove(ctx, id, options) } } @@ -596,8 +632,17 @@ func TestRuntimeDockerStop(t *testing.T) { } err = rctx.Stop(t.Context()) - if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { - t.Errorf("\n%s\nStop(...): -want error, +got error:\n%s", tc.reason, diff) + + // The returned error may join several errors, so check that it + // wraps each wanted error in turn. + wantErrs := tc.want.errs + if len(wantErrs) == 0 { + wantErrs = []error{nil} + } + for _, want := range wantErrs { + if diff := cmp.Diff(want, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStop(...): -want error, +got error:\n%s", tc.reason, diff) + } } }) } diff --git a/cmd/crossplane/render/xr/cmd.go b/cmd/crossplane/render/xr/cmd.go index 0a41121d..0c2435cb 100644 --- a/cmd/crossplane/render/xr/cmd.go +++ b/cmd/crossplane/render/xr/cmd.go @@ -278,7 +278,11 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte if err != nil { return errors.Wrap(err, "cannot start function runtimes") } - defer render.StopFunctionRuntimes(log, fnAddrs) + defer func() { + if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { + log.Info("Error stopping function runtimes", "error", err) + } + }() // Build and execute the render request. in := render.CompositionInputs{