From 88c44e80f786e1ebf47acd9e6dc62e237e1da441 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:27:51 -0400 Subject: [PATCH 1/3] refactor(render): inject Docker client interfaces for container and network operations RuntimeDocker.Start and the render network helpers each construct their own Docker client from the environment, so the container lifecycle and network setup/teardown can only be exercised against a live Docker daemon. That blocks unit-testing the follow-up fixes for #397, #398 and #401. Introduce narrow unexported interfaces covering exactly the moby client methods each site uses: containerClient (image pull, container inspect/create/start, plus containerCleanupClient for stop/remove) for RuntimeDocker, and networkClient (network create/remove) for the render network helpers. *client.Client satisfies both, enforced by compile-time assertions. RuntimeDocker gains an unexported dockerClient field; when nil, Start builds the real client from the environment exactly as before. createRenderNetwork and removeRenderNetwork now take the client as a parameter, and dockerRenderEngine gains an unexported networks field; when nil, Setup builds the real client only on the create-network branch, with the same error wrapping as before. No exported signature, the Engine interface, or the cleanup policy semantics change. Add function-field mocks for both interfaces: mockContainerClient next to the existing mockPullClient in runtime_docker_test.go, and mockNetworkClient in network_test.go. Add table tests that assert on results and sentinel errors, proving each seam is wired: network create/remove, Setup's create-network branch and its cleanup, RuntimeDocker.Start's create/start/inspect and image pull path, and its stop closure for the Stop, Remove and Orphan cleanup policies. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/engine_docker.go | 20 +- cmd/crossplane/render/engine_docker_test.go | 97 ++++- cmd/crossplane/render/network.go | 31 +- cmd/crossplane/render/network_test.go | 155 ++++++++ cmd/crossplane/render/runtime_docker.go | 41 ++- cmd/crossplane/render/runtime_docker_test.go | 356 +++++++++++++++++++ 6 files changed, 678 insertions(+), 22 deletions(-) diff --git a/cmd/crossplane/render/engine_docker.go b/cmd/crossplane/render/engine_docker.go index 9beb3e8c..ac60193d 100644 --- a/cmd/crossplane/render/engine_docker.go +++ b/cmd/crossplane/render/engine_docker.go @@ -63,6 +63,13 @@ type dockerRenderEngine struct { // (exit-3 partial output, *docker.ContainerExitError vs non-exit errors) // without a real Docker daemon. runner containerRunner + + // networks creates and removes the temporary Docker network Setup owns. + // Production callers leave it nil and Setup builds a real client from the + // environment only when it needs to create a network. Tests substitute a + // fake to exercise the create-network branch without a real Docker + // daemon. + networks networkClient } func (e *dockerRenderEngine) CheckContextSupport() error { @@ -95,7 +102,16 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f return func() {}, nil } - networkID, networkName, err := createRenderNetwork(ctx) + cli := e.networks + if cli == nil { + c, err := newNetworkClient() + if err != nil { + return func() {}, errors.Wrap(err, "cannot create Docker network for rendering") + } + cli = c + } + + networkID, networkName, err := createRenderNetwork(ctx, cli) if err != nil { return func() {}, errors.Wrap(err, "cannot create Docker network for rendering") } @@ -104,7 +120,7 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f injectNetworkAnnotation(fns, networkName) cleanup := func() { //nolint:contextcheck // Detached context for cleanup. - _ = removeRenderNetwork(context.Background(), networkID) + _ = removeRenderNetwork(context.Background(), cli, networkID) } return cleanup, nil diff --git a/cmd/crossplane/render/engine_docker_test.go b/cmd/crossplane/render/engine_docker_test.go index 73fcdddb..4d6d0d8a 100644 --- a/cmd/crossplane/render/engine_docker_test.go +++ b/cmd/crossplane/render/engine_docker_test.go @@ -23,10 +23,13 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/moby/moby/client" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" @@ -222,9 +225,8 @@ func TestDockerRenderEngineSetup(t *testing.T) { // call on the same engine stored its created network there. The branch // must annotate the supplied functions so their containers join the // network, never create a second network, and always return a no-op - // cleanup. The create-new-network branch is not covered here because it - // depends on a live Docker daemon; the broader render command tests - // exercise it integration-style. + // cleanup. The create-new-network branch is covered separately by + // TestDockerRenderEngineSetupCreatesNetwork. // // The MultiBatchAnnotatesAdditionalFunctions case simulates the // in-process multi-composition use case from crossplane/cli#96: a @@ -346,6 +348,95 @@ func TestDockerRenderEngineSetup(t *testing.T) { } } +func TestDockerRenderEngineSetupCreatesNetwork(t *testing.T) { + // When e.network is unset, Setup must create a temporary network through + // the engine's network client, record its name, annotate the supplied + // functions to join it, and return a cleanup that removes it through the + // same client. + errBoom := errors.New("boom") + + type args struct { + create func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) + } + type want struct { + err error + // created is whether Setup should create a network, annotate the + // functions to join it, and return a cleanup that removes it. + created bool + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "CreatesNetwork": { + reason: "Setup should create a network, annotate the functions to join it, and return a cleanup that removes it.", + args: args{ + create: createRenderNetworkReturns("network-id", nil), + }, + want: want{ + created: true, + }, + }, + "NetworkCreateError": { + reason: "Setup should return an error and a no-op cleanup, leaving the functions unannotated, when it cannot create the network.", + args: args{ + create: createRenderNetworkReturns("", errBoom), + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + // Cleanup discards NetworkRemove's result, so record whether it + // was called. A cleanup that shouldn't remove anything leaves + // MockNetworkRemove nil. + removed := false + cli := &mockNetworkClient{MockNetworkCreate: tc.args.create} + if tc.want.created { + cli.MockNetworkRemove = func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + if diff := cmp.Diff("network-id", networkID); diff != "" { + t.Errorf("\n%s\nNetworkRemove(...): -want network ID, +got network ID:\n%s", tc.reason, diff) + } + removed = true + return client.NetworkRemoveResult{}, nil + } + } + e := &dockerRenderEngine{log: logging.NewNopLogger(), networks: cli} + fns := []pkgv1.Function{functionWithAnnotations(nil)} + + cleanup, err := e.Setup(t.Context(), fns) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nSetup(...): -want error, +got error:\n%s", tc.reason, diff) + } + + wantFns := []pkgv1.Function{functionWithAnnotations(nil)} + if tc.want.created { + if !strings.HasPrefix(e.network, renderNetworkPrefix) { + t.Errorf("\n%s\nSetup(...): e.network = %q, want a network with prefix %q", tc.reason, e.network, renderNetworkPrefix) + } + wantFns = []pkgv1.Function{functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: e.network})} + } else if e.network != "" { + t.Errorf("\n%s\nSetup(...): e.network = %q, want it unset", tc.reason, e.network) + } + if diff := cmp.Diff(wantFns, fns); diff != "" { + t.Errorf("\n%s\nSetup(...): -want fns, +got fns:\n%s", tc.reason, diff) + } + + cleanup() + + if diff := cmp.Diff(tc.want.created, removed); diff != "" { + t.Errorf("\n%s\nSetup(...) cleanup: -want network removed, +got network removed:\n%s", tc.reason, diff) + } + }) + } +} + // nonExitError is a stand-in for non-*ContainerExitError failures (e.g. image // pull errors) returned by docker.RunContainer. type nonExitError struct{ msg string } diff --git a/cmd/crossplane/render/network.go b/cmd/crossplane/render/network.go index 454b122a..8ef17d68 100644 --- a/cmd/crossplane/render/network.go +++ b/cmd/crossplane/render/network.go @@ -46,15 +46,28 @@ func (f *EngineFlags) SetDefaultCrossplaneDockerNetwork(fns []pkgv1.Function) { } } -// createRenderNetwork creates a temporary Docker bridge network for render. -// Function containers and the Crossplane render container join this network so -// they can reach each other. Returns the network ID and name. -func createRenderNetwork(ctx context.Context) (string, string, error) { +// networkClient is the subset of the Docker client the render engine uses to +// manage its temporary Docker network. +type networkClient interface { + NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) + NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) +} + +var _ networkClient = (*client.Client)(nil) + +// newNetworkClient returns a real Docker client built from the environment. +func newNetworkClient() (networkClient, error) { cli, err := docker.NewClient() if err != nil { - return "", "", errors.Wrap(err, "cannot create Docker client") + return nil, errors.Wrap(err, "cannot create Docker client") } + return cli, nil +} +// createRenderNetwork creates a temporary Docker bridge network for render. +// Function containers and the Crossplane render container join this network so +// they can reach each other. Returns the network ID and name. +func createRenderNetwork(ctx context.Context, cli networkClient) (string, string, error) { name := fmt.Sprintf("crossplane-render-%s", rand.String(8)) resp, err := cli.NetworkCreate(ctx, name, client.NetworkCreateOptions{ @@ -68,11 +81,7 @@ func createRenderNetwork(ctx context.Context) (string, string, error) { } // removeRenderNetwork removes a temporary Docker network. -func removeRenderNetwork(ctx context.Context, networkID string) error { - cli, err := docker.NewClient() - if err != nil { - return errors.Wrap(err, "cannot create Docker client") - } - _, err = cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{}) +func removeRenderNetwork(ctx context.Context, cli networkClient, networkID string) error { + _, err := cli.NetworkRemove(ctx, networkID, client.NetworkRemoveOptions{}) return errors.Wrap(err, "cannot remove Docker network") } diff --git a/cmd/crossplane/render/network_test.go b/cmd/crossplane/render/network_test.go index fde13f0f..843f16f4 100644 --- a/cmd/crossplane/render/network_test.go +++ b/cmd/crossplane/render/network_test.go @@ -1,9 +1,15 @@ package render import ( + "context" + "strings" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/moby/moby/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" ) @@ -74,3 +80,152 @@ func TestSetDefaultCrossplaneDockerNetwork(t *testing.T) { }) } } + +type mockNetworkClient struct { + MockNetworkCreate func(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) + MockNetworkRemove func(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) +} + +func (m *mockNetworkClient) NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + return m.MockNetworkCreate(ctx, name, options) +} + +func (m *mockNetworkClient) NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + return m.MockNetworkRemove(ctx, networkID, options) +} + +var _ networkClient = &mockNetworkClient{} + +// renderNetworkPrefix is the prefix of the temporary network name +// createRenderNetwork generates. +const renderNetworkPrefix = "crossplane-render-" + +// createRenderNetworkReturns returns a MockNetworkCreate that returns the +// supplied network ID and error. It returns an error instead when it is not +// asked to create a render bridge network. +func createRenderNetworkReturns(id string, err error) func(context.Context, string, client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + return func(_ context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + if !strings.HasPrefix(name, renderNetworkPrefix) { + return client.NetworkCreateResult{}, errors.Errorf("NetworkCreate(...): name %q does not have prefix %q", name, renderNetworkPrefix) + } + if diff := cmp.Diff(client.NetworkCreateOptions{Driver: "bridge"}, options); diff != "" { + return client.NetworkCreateResult{}, errors.Errorf("NetworkCreate(...): -want options, +got options:\n%s", diff) + } + return client.NetworkCreateResult{ID: id}, err + } +} + +func TestCreateRenderNetwork(t *testing.T) { + errBoom := errors.New("boom") + + type args struct { + cli networkClient + } + type want struct { + id string + // namePrefix is a prefix the returned network name must have. The + // rest of the name is random. + namePrefix string + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "CreatesBridgeNetwork": { + reason: "createRenderNetwork should create a uniquely named bridge network through the supplied client and return its ID and name.", + args: args{ + cli: &mockNetworkClient{MockNetworkCreate: createRenderNetworkReturns("network-id", nil)}, + }, + want: want{ + id: "network-id", + namePrefix: renderNetworkPrefix, + }, + }, + "NetworkCreateError": { + reason: "createRenderNetwork should return an error when the client cannot create the network.", + args: args{ + cli: &mockNetworkClient{MockNetworkCreate: createRenderNetworkReturns("", errBoom)}, + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + id, networkName, err := createRenderNetwork(t.Context(), tc.args.cli) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\ncreateRenderNetwork(...): -want error, +got error:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.id, id); diff != "" { + t.Errorf("\n%s\ncreateRenderNetwork(...): -want ID, +got ID:\n%s", tc.reason, diff) + } + if !strings.HasPrefix(networkName, tc.want.namePrefix) { + t.Errorf("\n%s\ncreateRenderNetwork(...): name %q does not have prefix %q", tc.reason, networkName, tc.want.namePrefix) + } + }) + } +} + +func TestRemoveRenderNetwork(t *testing.T) { + errBoom := errors.New("boom") + + // removeNetworkReturns returns a MockNetworkRemove that returns the + // supplied error, or an error of its own when asked to remove any network + // other than network-id. + removeNetworkReturns := func(err error) func(context.Context, string, client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + return func(_ context.Context, networkID string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + if diff := cmp.Diff("network-id", networkID); diff != "" { + return client.NetworkRemoveResult{}, errors.Errorf("NetworkRemove(...): -want network ID, +got network ID:\n%s", diff) + } + return client.NetworkRemoveResult{}, err + } + } + + type args struct { + cli networkClient + networkID string + } + type want struct { + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "RemovesNetwork": { + reason: "removeRenderNetwork should remove the network with the supplied ID through the supplied client.", + args: args{ + cli: &mockNetworkClient{MockNetworkRemove: removeNetworkReturns(nil)}, + networkID: "network-id", + }, + }, + "NetworkRemoveError": { + reason: "removeRenderNetwork should return an error when the client cannot remove the network.", + args: args{ + cli: &mockNetworkClient{MockNetworkRemove: removeNetworkReturns(errBoom)}, + networkID: "network-id", + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := removeRenderNetwork(t.Context(), tc.args.cli, tc.args.networkID) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nremoveRenderNetwork(...): -want error, +got error:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/cmd/crossplane/render/runtime_docker.go b/cmd/crossplane/render/runtime_docker.go index 169fe3f5..8972945b 100644 --- a/cmd/crossplane/render/runtime_docker.go +++ b/cmd/crossplane/render/runtime_docker.go @@ -161,8 +161,33 @@ type RuntimeDocker struct { // and is reached via host port bindings. When set, the container joins // the specified network and is reached via its Docker hostname on port 9443. Network string + + // dockerClient manages the Function's container. Production callers leave + // it nil and Start builds a real client from the environment. Tests + // substitute a fake to exercise container lifecycle and cleanup handling + // without a real Docker daemon. + dockerClient containerClient +} + +// containerClient is the subset of the Docker client RuntimeDocker uses to +// pull images and manage a Function's container. +type containerClient interface { + pullClient + containerCleanupClient + ContainerInspect(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) + ContainerCreate(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) + ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) +} + +// containerCleanupClient is the subset of the Docker client RuntimeDocker's +// stop function uses to clean up a Function's container. +type containerCleanupClient interface { + ContainerStop(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + ContainerRemove(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) } +var _ containerClient = (*client.Client)(nil) + // GetDockerPullPolicy extracts PullPolicy configuration from the supplied // Function. func GetDockerPullPolicy(fn pkgv1.Function) (DockerPullPolicy, error) { @@ -250,7 +275,7 @@ func GetRuntimeDocker(fn pkgv1.Function, log logging.Logger) (*RuntimeDocker, er var _ Runtime = &RuntimeDocker{} -func (r *RuntimeDocker) findContainer(ctx context.Context, cli *client.Client) (string, error) { +func (r *RuntimeDocker) findContainer(ctx context.Context, cli containerClient) (string, error) { if r.Name == "" { return "", nil } @@ -266,7 +291,7 @@ func (r *RuntimeDocker) findContainer(ctx context.Context, cli *client.Client) ( return inspect.Container.ID, nil } -func (r *RuntimeDocker) createContainer(ctx context.Context, cli *client.Client) (string, error) { +func (r *RuntimeDocker) createContainer(ctx context.Context, cli containerClient) (string, error) { r.log.Debug("Starting Docker container runtime setup", "image", r.Image) // Let Docker automatically allocate an available port on the bind address. @@ -377,7 +402,7 @@ func (r *RuntimeDocker) createContainer(ctx context.Context, cli *client.Client) } // startContainer ensures the container is running and returns its address. -func (r *RuntimeDocker) startContainer(ctx context.Context, cli *client.Client, containerID string) (string, error) { +func (r *RuntimeDocker) startContainer(ctx context.Context, cli containerClient, containerID string) (string, error) { // Start the container (idempotent - safe to call on running containers) if _, err := cli.ContainerStart(ctx, containerID, client.ContainerStartOptions{}); err != nil { return "", errors.Wrap(err, "cannot start Docker container") @@ -457,9 +482,13 @@ func (r *RuntimeDocker) getPullOptions() (client.ImagePullOptions, error) { // Start a Function as a Docker container. func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) { - cli, err := client.New(client.FromEnv) - if err != nil { - return RuntimeContext{}, errors.Wrap(err, "cannot create Docker client using environment variables") + cli := r.dockerClient + if cli == nil { + c, err := client.New(client.FromEnv) + if err != nil { + return RuntimeContext{}, errors.Wrap(err, "cannot create Docker client using environment variables") + } + cli = c } // Try to find an existing container with the supplied container name. diff --git a/cmd/crossplane/render/runtime_docker_test.go b/cmd/crossplane/render/runtime_docker_test.go index a05a6079..68c2e90c 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -18,13 +18,20 @@ package render import ( "context" + "io" + "net" + "strconv" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" "github.com/moby/moby/client" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/logging" pkgv1 "github.com/crossplane/crossplane/apis/v2/pkg/v1" @@ -40,6 +47,95 @@ func (m *mockPullClient) ImagePull(ctx context.Context, ref string, options clie var _ pullClient = &mockPullClient{} +type mockContainerClient struct { + MockImagePull func(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) + MockContainerInspect func(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) + MockContainerCreate func(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) + MockContainerStart func(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) + MockContainerStop func(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + MockContainerRemove func(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) +} + +func (m *mockContainerClient) ImagePull(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { + return m.MockImagePull(ctx, ref, options) +} + +func (m *mockContainerClient) ContainerInspect(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return m.MockContainerInspect(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerCreate(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return m.MockContainerCreate(ctx, options) +} + +func (m *mockContainerClient) ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) { + return m.MockContainerStart(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerStop(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) { + return m.MockContainerStop(ctx, containerID, options) +} + +func (m *mockContainerClient) ContainerRemove(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return m.MockContainerRemove(ctx, containerID, options) +} + +var _ containerClient = &mockContainerClient{} + +// imagePullDone is a MockImagePull that reports an image pull that has +// already completed. +func imagePullDone(context.Context, string, client.ImagePullOptions) (client.ImagePullResponse, error) { + return pullDoneResponse{}, nil +} + +// pullDoneResponse is an ImagePullResponse whose body is empty. Only the +// io.ReadCloser methods PullImage uses are implemented; the embedded +// interface is nil. +type pullDoneResponse struct { + client.ImagePullResponse +} + +func (pullDoneResponse) Read([]byte) (int, error) { return 0, io.EOF } + +func (pullDoneResponse) Close() error { return nil } + +// createContainerReturns returns a MockContainerCreate that creates a +// container with the supplied ID. +func createContainerReturns(id string) func(context.Context, client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return func(context.Context, client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return client.ContainerCreateResult{ID: id}, nil + } +} + +// startContainer returns a MockContainerStart that starts the container with +// the supplied ID, and returns an error for any other container. +func startContainer(id string) func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + return func(_ context.Context, containerID string, _ client.ContainerStartOptions) (client.ContainerStartResult, error) { + if diff := cmp.Diff(id, containerID); diff != "" { + return client.ContainerStartResult{}, errors.Errorf("ContainerStart(...): -want container ID, +got container ID:\n%s", diff) + } + return client.ContainerStartResult{}, nil + } +} + +// inspectContainerOnNetwork returns a MockContainerInspect that reports the +// container with the supplied ID as running with the supplied name, attached +// to the supplied Docker network. It returns an error for any other container. +func inspectContainerOnNetwork(id, name, networkName string) func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return func(_ context.Context, containerID string, _ client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + if diff := cmp.Diff(id, containerID); diff != "" { + return client.ContainerInspectResult{}, errors.Errorf("ContainerInspect(...): -want container ID, +got container ID:\n%s", diff) + } + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: id, + Name: "/" + name, + NetworkSettings: &container.NetworkSettings{ + Networks: map[string]*network.EndpointSettings{networkName: {}}, + }, + }}, nil + } +} + func TestGetRuntimeDocker(t *testing.T) { type args struct { fn pkgv1.Function @@ -246,3 +342,263 @@ func TestGetRuntimeDocker(t *testing.T) { }) } } + +func TestRuntimeDockerStart(t *testing.T) { + const ( + image = "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0" + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + errBoom := errors.New("boom") + + type args struct { + pullPolicy DockerPullPolicy + cli *mockContainerClient + } + type want struct { + rctx RuntimeContext + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "CreatesAndStartsContainer": { + reason: "Start should create, start, and inspect the Function's container through the injected client, without pulling an image that is present.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + cli: &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + want: want{ + rctx: RuntimeContext{Target: net.JoinHostPort(containerName, strconv.Itoa(FunctionPort))}, + }, + }, + "PullsImage": { + reason: "Start should pull the Function's image through the injected client when the pull policy is Always.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyAlways, + cli: &mockContainerClient{ + MockImagePull: func(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { + if diff := cmp.Diff(image, ref); diff != "" { + return nil, errors.Errorf("ImagePull(...): -want ref, +got ref:\n%s", diff) + } + return imagePullDone(ctx, ref, options) + }, + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + }, + }, + want: want{ + rctx: RuntimeContext{Target: net.JoinHostPort(containerName, strconv.Itoa(FunctionPort))}, + }, + }, + "PullError": { + reason: "Start should return an error, without creating a container, when the injected client cannot pull the image.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyAlways, + cli: &mockContainerClient{ + MockImagePull: func(context.Context, string, client.ImagePullOptions) (client.ImagePullResponse, error) { + return nil, errBoom + }, + }, + }, + want: want{ + err: errBoom, + }, + }, + "StartError": { + reason: "Start should return an error when the injected client cannot start the container.", + args: args{ + pullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + cli: &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + return client.ContainerStartResult{}, errBoom + }, + }, + }, + want: want{ + err: errBoom, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + r := &RuntimeDocker{ + Image: image, + Cleanup: AnnotationValueRuntimeDockerCleanupRemove, + PullPolicy: tc.args.pullPolicy, + Keychain: authn.NewMultiKeychain(), + Network: dockerNetwork, + log: logging.NewNopLogger(), + dockerClient: tc.args.cli, + } + + rctx, err := r.Start(t.Context()) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nStart(...): -want error, +got error:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.rctx, rctx, cmpopts.IgnoreFields(RuntimeContext{}, "Stop")); diff != "" { + t.Errorf("\n%s\nStart(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestRuntimeDockerStop(t *testing.T) { + const ( + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + errStop := errors.New("stop boom") + errRemove := errors.New("remove boom") + + type args struct { + cleanup DockerCleanup + // stop and remove are the cleanup client's ContainerStop and + // ContainerRemove. A nil mock must not be called. The test checks + // the arguments each non-nil mock is called with. + stop func(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) + remove func(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) + } + type want struct { + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "Stop": { + reason: "The Stop cleanup policy should stop the container and leave it in place.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupStop, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + }, + }, + "StopError": { + reason: "The Stop cleanup policy should return an error when the container cannot be stopped.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupStop, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + }, + want: want{ + err: errStop, + }, + }, + "Remove": { + reason: "The Remove cleanup policy should stop the container, then remove it.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, nil + }, + }, + }, + "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.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, errStop + }, + }, + want: want{ + err: errStop, + }, + }, + "RemoveError": { + reason: "The Remove cleanup policy should return an error when the container cannot be removed.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stop: func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil + }, + remove: func(context.Context, string, client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, errRemove + }, + }, + want: want{ + err: errRemove, + }, + }, + "Orphan": { + reason: "The Orphan cleanup policy should leave the container running without calling Docker.", + args: args{ + cleanup: AnnotationValueRuntimeDockerCleanupOrphan, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cli := &mockContainerClient{ + MockContainerCreate: createContainerReturns(containerID), + MockContainerStart: startContainer(containerID), + MockContainerInspect: inspectContainerOnNetwork(containerID, containerName, dockerNetwork), + } + 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 != "" { + t.Errorf("\n%s\nContainerStop(...): -want options, +got options:\n%s", tc.reason, diff) + } + return stop(ctx, id, options) + } + } + if remove := tc.args.remove; remove != nil { + cli.MockContainerRemove = func(ctx context.Context, id string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + 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 != "" { + t.Errorf("\n%s\nContainerRemove(...): -want options, +got options:\n%s", tc.reason, diff) + } + return remove(ctx, id, options) + } + } + r := &RuntimeDocker{ + Image: "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0", + Cleanup: tc.args.cleanup, + PullPolicy: AnnotationValueRuntimeDockerPullPolicyIfNotPresent, + Keychain: authn.NewMultiKeychain(), + Network: dockerNetwork, + log: logging.NewNopLogger(), + dockerClient: cli, + } + + rctx, err := r.Start(t.Context()) + if err != nil { + t.Fatalf("\n%s\nStart(...): unexpected error: %v", tc.reason, err) + } + + 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) + } + }) + } +} From 3978bbc95b68a10957a7b5adbc3b0efe309f4d0e Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:41:55 -0400 Subject: [PATCH 2/3] 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. StopFunctionRuntimes gives each runtime its own timeout, sized as the grace period plus a margin, derived from the caller's context without its cancellation. Cleanup therefore still runs after the render context is cancelled, but stays bounded. 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 | 50 +++++-- cmd/crossplane/render/render_test.go | 150 +++++++++++++++++++ cmd/crossplane/render/runtime_docker.go | 34 +++-- cmd/crossplane/render/runtime_docker_test.go | 60 ++++++-- cmd/crossplane/render/xr/cmd.go | 6 +- 6 files changed, 270 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..ff5f664f 100644 --- a/cmd/crossplane/render/render.go +++ b/cmd/crossplane/render/render.go @@ -94,6 +94,17 @@ type OperationOutputs struct { RequiredSchemas []*fnv1.SchemaSelector } +const ( + // runtimeStopMargin is how much longer than containerStopGracePeriod + // StopFunctionRuntimes waits for each runtime to stop. It covers killing + // and removing the container once the grace period has expired. + runtimeStopMargin = 5 * time.Second + + // runtimeStopTimeout bounds how long StopFunctionRuntimes waits for each + // runtime to stop. + runtimeStopTimeout = containerStopGracePeriod + runtimeStopMargin +) + // FunctionAddresses maps function names to their gRPC target addresses. type FunctionAddresses struct { addrs map[string]string @@ -105,14 +116,30 @@ 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. func (fa *FunctionAddresses) Stop(ctx context.Context) error { + return fa.stop(ctx, 0) +} + +// stop stops every function runtime and returns all failures joined. If +// timeout is positive each runtime gets its own timeout derived from ctx. +func (fa *FunctionAddresses) stop(ctx context.Context, timeout time.Duration) 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) + sctx, cancel := ctx, context.CancelFunc(func() {}) + if timeout > 0 { + sctx, cancel = context.WithTimeout(ctx, timeout) } + if err := rctx.Stop(sctx); err != nil { + errs = append(errs, errors.Wrapf(err, "cannot stop function %q runtime (target %q)", name, rctx.Target)) + } + cancel() } - return nil + if len(errs) == 0 { + return nil + } + return errors.Join(errs...) } // StartFunctionRuntimes starts the runtime for each function and returns their @@ -167,16 +194,15 @@ 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: each runtime gets its +// own timeout derived from ctx without its cancellation, so a slow runtime +// can't starve the others and cleanup stays bounded. +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), runtimeStopTimeout) } // 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..a47e4621 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,150 @@ 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 and hasDeadline describe the context the runtime's Stop + // was called with. + ctxErr error + hasDeadline bool + err error + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "ParentContextLive": { + reason: "StopFunctionRuntimes should stop each runtime with a context bounded by its own timeout.", + want: want{hasDeadline: true}, + }, + "ParentContextCancelled": { + reason: "StopFunctionRuntimes should still stop each runtime, with a live but bounded context, when the parent context is already cancelled.", + args: args{cancelParent: true}, + want: want{hasDeadline: true}, + }, + "StopError": { + reason: "StopFunctionRuntimes should return the error a runtime fails to stop with.", + args: args{stopErr: errBoom}, + want: want{hasDeadline: true, 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() + _, got.hasDeadline = ctx.Deadline() + 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..414a0509 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,10 @@ const ( AnnotationValueRuntimeDockerCleanupDefault = AnnotationValueRuntimeDockerCleanupRemove ) +// containerStopGracePeriod is how long the Stop and Remove cleanup policies +// wait for a Function container to exit on SIGTERM before Docker kills it. +const containerStopGracePeriod = 3 * 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 +520,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 := cli.ContainerStop(ctx, 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 := cli.ContainerStop(ctx, 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 := cli.ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{Force: true}); err != nil { + return errors.Join(errors.Wrap(err, "cannot remove Docker container"), stopErr) } } diff --git a/cmd/crossplane/render/runtime_docker_test.go b/cmd/crossplane/render/runtime_docker_test.go index 68c2e90c..b8544e3a 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -466,6 +466,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 +481,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 +492,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 +509,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 +525,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 +548,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,12 +581,14 @@ 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) } return stop(ctx, id, options) @@ -574,7 +599,7 @@ 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) } return remove(ctx, id, options) @@ -596,8 +621,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{ From 030c086aa8783991c0de74c0209727c607e17d59 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:49:33 -0400 Subject: [PATCH 3/3] fix(render): warn on stderr when function runtimes can't be cleaned up StopFunctionRuntimes returns its error, but `crossplane render xr` and `crossplane render op` only reported it via log.Info, and the CLI's logger is a no-op unless --verbose is set. As a result both commands silently left Function containers behind when stopping them failed. Add WarnCleanupFailure, which writes a short warning (including the error) to a writer and does nothing for a nil error, and call it with kong's stderr from both commands' deferred cleanup alongside the existing log line. The exit code is unchanged. Fixes #399 Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/op/cmd.go | 1 + cmd/crossplane/render/render.go | 12 +++++++++++ cmd/crossplane/render/render_test.go | 30 ++++++++++++++++++++++++++++ cmd/crossplane/render/xr/cmd.go | 1 + 4 files changed, 44 insertions(+) diff --git a/cmd/crossplane/render/op/cmd.go b/cmd/crossplane/render/op/cmd.go index 63a045e2..c67a7853 100644 --- a/cmd/crossplane/render/op/cmd.go +++ b/cmd/crossplane/render/op/cmd.go @@ -221,6 +221,7 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte defer func() { if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { log.Info("Error stopping function runtimes", "error", err) + render.WarnCleanupFailure(k.Stderr, err) } }() diff --git a/cmd/crossplane/render/render.go b/cmd/crossplane/render/render.go index ff5f664f..5dc56a83 100644 --- a/cmd/crossplane/render/render.go +++ b/cmd/crossplane/render/render.go @@ -20,6 +20,8 @@ package render import ( "context" + "fmt" + "io" "strings" "time" @@ -223,3 +225,13 @@ func OverrideFunctionAnnotations(fns []pkgv1.Function, annotations []string) err } return nil } + +// WarnCleanupFailure writes a short warning about err to w. It does nothing if +// err is nil. Callers use it to surface function runtime cleanup failures that +// would otherwise only be visible with --verbose. +func WarnCleanupFailure(w io.Writer, err error) { + if err == nil { + return + } + _, _ = fmt.Fprintf(w, "warning: failed to clean up function runtimes; some resources (e.g. containers) may need to be removed manually: %v\n", err) +} diff --git a/cmd/crossplane/render/render_test.go b/cmd/crossplane/render/render_test.go index a47e4621..b5d6e5bd 100644 --- a/cmd/crossplane/render/render_test.go +++ b/cmd/crossplane/render/render_test.go @@ -2,6 +2,7 @@ package render import ( "context" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -241,3 +242,32 @@ func TestStopFunctionRuntimes(t *testing.T) { }) } } + +func TestWarnCleanupFailure(t *testing.T) { + cases := map[string]struct { + reason string + err error + want string + }{ + "NilError": { + reason: "No warning should be written when cleanup succeeded.", + err: nil, + want: "", + }, + "Error": { + reason: "A warning including the error should be written when cleanup failed.", + err: errors.New("boom"), + want: "warning: failed to clean up function runtimes; some resources (e.g. containers) may need to be removed manually: boom\n", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + b := &strings.Builder{} + WarnCleanupFailure(b, tc.err) + if diff := cmp.Diff(tc.want, b.String()); diff != "" { + t.Errorf("\n%s\nWarnCleanupFailure(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/cmd/crossplane/render/xr/cmd.go b/cmd/crossplane/render/xr/cmd.go index 0c2435cb..da3302bc 100644 --- a/cmd/crossplane/render/xr/cmd.go +++ b/cmd/crossplane/render/xr/cmd.go @@ -281,6 +281,7 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, sp terminal.SpinnerPrinte defer func() { if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { log.Info("Error stopping function runtimes", "error", err) + render.WarnCleanupFailure(k.Stderr, err) } }()