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{