From c7f77b93f75f2a01adc70870aeddb62caaec5269 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 recording fakes for both interfaces in docker_fake_test.go and tests proving each seam is wired: network create/remove, Setup's create-network branch and its cleanup, and RuntimeDocker's 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/docker_fake_test.go | 140 +++++++++++++++++++ cmd/crossplane/render/engine_docker.go | 20 ++- cmd/crossplane/render/engine_docker_test.go | 47 ++++++- cmd/crossplane/render/network.go | 31 ++-- cmd/crossplane/render/network_test.go | 123 ++++++++++++++++ cmd/crossplane/render/runtime_docker.go | 41 +++++- cmd/crossplane/render/runtime_docker_test.go | 89 ++++++++++++ 7 files changed, 469 insertions(+), 22 deletions(-) create mode 100644 cmd/crossplane/render/docker_fake_test.go diff --git a/cmd/crossplane/render/docker_fake_test.go b/cmd/crossplane/render/docker_fake_test.go new file mode 100644 index 00000000..6785071b --- /dev/null +++ b/cmd/crossplane/render/docker_fake_test.go @@ -0,0 +1,140 @@ +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package render + +import ( + "context" + "io" + + "github.com/moby/moby/client" +) + +// fakeDockerCall records a single call received by a fake Docker client. +type fakeDockerCall struct { + // Method is the Docker client method name, e.g. "ContainerStop". + Method string + // Ref is the call's identifying argument: a container ID or name, a + // network ID or name, or an image reference. It is empty for + // ContainerCreate, which names its container through Options. + Ref string + // Options is the options struct the method was called with. + Options any +} + +// fakeContainerClient is a containerClient that records every call it +// receives in Calls. Each method delegates to its Mock function when set, and +// otherwise succeeds with a zero result (ImagePull returns an empty, already +// complete pull). +type fakeContainerClient 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) + + Calls []fakeDockerCall +} + +var _ containerClient = &fakeContainerClient{} + +func (f *fakeContainerClient) ImagePull(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "ImagePull", Ref: ref, Options: options}) + if f.MockImagePull != nil { + return f.MockImagePull(ctx, ref, options) + } + return fakeImagePullResponse{}, nil +} + +func (f *fakeContainerClient) ContainerInspect(ctx context.Context, containerID string, options client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "ContainerInspect", Ref: containerID, Options: options}) + if f.MockContainerInspect != nil { + return f.MockContainerInspect(ctx, containerID, options) + } + return client.ContainerInspectResult{}, nil +} + +func (f *fakeContainerClient) ContainerCreate(ctx context.Context, options client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "ContainerCreate", Options: options}) + if f.MockContainerCreate != nil { + return f.MockContainerCreate(ctx, options) + } + return client.ContainerCreateResult{}, nil +} + +func (f *fakeContainerClient) ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "ContainerStart", Ref: containerID, Options: options}) + if f.MockContainerStart != nil { + return f.MockContainerStart(ctx, containerID, options) + } + return client.ContainerStartResult{}, nil +} + +func (f *fakeContainerClient) ContainerStop(ctx context.Context, containerID string, options client.ContainerStopOptions) (client.ContainerStopResult, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "ContainerStop", Ref: containerID, Options: options}) + if f.MockContainerStop != nil { + return f.MockContainerStop(ctx, containerID, options) + } + return client.ContainerStopResult{}, nil +} + +func (f *fakeContainerClient) ContainerRemove(ctx context.Context, containerID string, options client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "ContainerRemove", Ref: containerID, Options: options}) + if f.MockContainerRemove != nil { + return f.MockContainerRemove(ctx, containerID, options) + } + return client.ContainerRemoveResult{}, nil +} + +// fakeImagePullResponse is an ImagePullResponse whose body is empty, i.e. a +// pull that has already completed. Only the io.ReadCloser methods PullImage +// uses are implemented; the embedded interface is nil. +type fakeImagePullResponse struct { + client.ImagePullResponse +} + +func (fakeImagePullResponse) Read([]byte) (int, error) { return 0, io.EOF } + +func (fakeImagePullResponse) Close() error { return nil } + +// fakeNetworkClient is a networkClient that records every call it receives in +// Calls. Each method delegates to its Mock function when set, and otherwise +// succeeds with a zero result. +type fakeNetworkClient 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) + + Calls []fakeDockerCall +} + +var _ networkClient = &fakeNetworkClient{} + +func (f *fakeNetworkClient) NetworkCreate(ctx context.Context, name string, options client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "NetworkCreate", Ref: name, Options: options}) + if f.MockNetworkCreate != nil { + return f.MockNetworkCreate(ctx, name, options) + } + return client.NetworkCreateResult{}, nil +} + +func (f *fakeNetworkClient) NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + f.Calls = append(f.Calls, fakeDockerCall{Method: "NetworkRemove", Ref: networkID, Options: options}) + if f.MockNetworkRemove != nil { + return f.MockNetworkRemove(ctx, networkID, options) + } + return client.NetworkRemoveResult{}, nil +} 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..6f6b0751 100644 --- a/cmd/crossplane/render/engine_docker_test.go +++ b/cmd/crossplane/render/engine_docker_test.go @@ -23,6 +23,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/moby/moby/client" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" @@ -222,9 +223,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 +346,47 @@ 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. + cli := &fakeNetworkClient{ + MockNetworkCreate: func(_ context.Context, _ string, _ client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + return client.NetworkCreateResult{ID: "network-id"}, nil + }, + } + e := &dockerRenderEngine{log: logging.NewNopLogger(), networks: cli} + fns := []pkgv1.Function{functionWithAnnotations(nil)} + + cleanup, err := e.Setup(t.Context(), fns) + if err != nil { + t.Fatalf("Setup(...): unexpected error: %v", err) + } + if len(cli.Calls) != 1 || cli.Calls[0].Method != "NetworkCreate" { + t.Fatalf("Setup(...): calls %+v, want a single NetworkCreate", cli.Calls) + } + + created := cli.Calls[0].Ref + if e.network != created { + t.Errorf("Setup(...): e.network = %q, want the created network %q", e.network, created) + } + wantFns := []pkgv1.Function{functionWithAnnotations(map[string]string{AnnotationKeyRuntimeDockerNetwork: created})} + if diff := cmp.Diff(wantFns, fns); diff != "" { + t.Errorf("Setup(...): fns -want, +got:\n%s", diff) + } + + cleanup() + + wantCalls := []fakeDockerCall{ + {Method: "NetworkCreate", Ref: created, Options: client.NetworkCreateOptions{Driver: "bridge"}}, + {Method: "NetworkRemove", Ref: "network-id", Options: client.NetworkRemoveOptions{}}, + } + if diff := cmp.Diff(wantCalls, cli.Calls); diff != "" { + t.Errorf("Setup(...) cleanup: -want calls, +got calls:\n%s", 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..484695d3 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,120 @@ func TestSetDefaultCrossplaneDockerNetwork(t *testing.T) { }) } } + +func TestCreateRenderNetwork(t *testing.T) { + errBoom := errors.New("boom") + + type want struct { + id string + calls []fakeDockerCall + err error + } + + cases := map[string]struct { + reason string + cli *fakeNetworkClient + want want + }{ + "CreatesBridgeNetwork": { + reason: "createRenderNetwork should create a uniquely named bridge network through the supplied client and return its ID and name.", + cli: &fakeNetworkClient{ + MockNetworkCreate: func(_ context.Context, _ string, _ client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + return client.NetworkCreateResult{ID: "network-id"}, nil + }, + }, + want: want{ + id: "network-id", + calls: []fakeDockerCall{{Method: "NetworkCreate", Options: client.NetworkCreateOptions{Driver: "bridge"}}}, + }, + }, + "NetworkCreateError": { + reason: "createRenderNetwork should return an error when the client cannot create the network.", + cli: &fakeNetworkClient{ + MockNetworkCreate: func(_ context.Context, _ string, _ client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + return client.NetworkCreateResult{}, errBoom + }, + }, + want: want{ + calls: []fakeDockerCall{{Method: "NetworkCreate", Options: client.NetworkCreateOptions{Driver: "bridge"}}}, + err: cmpopts.AnyError, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + id, networkName, err := createRenderNetwork(t.Context(), tc.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) + } + // The network name has a random suffix, so compare calls without + // it and check the name's shape separately. + if diff := cmp.Diff(tc.want.calls, tc.cli.Calls, cmpopts.IgnoreFields(fakeDockerCall{}, "Ref")); diff != "" { + t.Errorf("\n%s\ncreateRenderNetwork(...): -want calls, +got calls:\n%s", tc.reason, diff) + } + if len(tc.cli.Calls) == 0 { + return + } + created := tc.cli.Calls[0].Ref + if !strings.HasPrefix(created, "crossplane-render-") { + t.Errorf("\n%s\ncreateRenderNetwork(...): NetworkCreate name %q does not have prefix %q", tc.reason, created, "crossplane-render-") + } + if err == nil && networkName != created { + t.Errorf("\n%s\ncreateRenderNetwork(...): returned name %q, want the created network's name %q", tc.reason, networkName, created) + } + }) + } +} + +func TestRemoveRenderNetwork(t *testing.T) { + errBoom := errors.New("boom") + + type want struct { + calls []fakeDockerCall + err error + } + + cases := map[string]struct { + reason string + cli *fakeNetworkClient + want want + }{ + "RemovesNetwork": { + reason: "removeRenderNetwork should remove the network with the supplied ID through the supplied client.", + cli: &fakeNetworkClient{}, + want: want{ + calls: []fakeDockerCall{{Method: "NetworkRemove", Ref: "network-id", Options: client.NetworkRemoveOptions{}}}, + }, + }, + "NetworkRemoveError": { + reason: "removeRenderNetwork should return an error when the client cannot remove the network.", + cli: &fakeNetworkClient{ + MockNetworkRemove: func(_ context.Context, _ string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + return client.NetworkRemoveResult{}, errBoom + }, + }, + want: want{ + calls: []fakeDockerCall{{Method: "NetworkRemove", Ref: "network-id", Options: client.NetworkRemoveOptions{}}}, + err: cmpopts.AnyError, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := removeRenderNetwork(t.Context(), tc.cli, "network-id") + + 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) + } + if diff := cmp.Diff(tc.want.calls, tc.cli.Calls); diff != "" { + t.Errorf("\n%s\nremoveRenderNetwork(...): -want calls, +got calls:\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..43bbda43 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -22,6 +22,9 @@ import ( "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" @@ -246,3 +249,89 @@ func TestGetRuntimeDocker(t *testing.T) { }) } } + +func TestRuntimeDockerStop(t *testing.T) { + const ( + containerID = "container-id" + containerName = "fn-container" + dockerNetwork = "render-net" + ) + + // Start always creates, starts, and inspects the container through the + // injected client before returning the stop closure under test. + wantStartCalls := []fakeDockerCall{ + {Method: "ContainerCreate"}, + {Method: "ContainerStart", Ref: containerID}, + {Method: "ContainerInspect", Ref: containerID}, + } + + cases := map[string]struct { + reason string + cleanup DockerCleanup + want []fakeDockerCall + }{ + "Stop": { + reason: "The Stop cleanup policy should stop the container and leave it in place.", + cleanup: AnnotationValueRuntimeDockerCleanupStop, + want: []fakeDockerCall{ + {Method: "ContainerStop", Ref: containerID, Options: client.ContainerStopOptions{}}, + }, + }, + "Remove": { + reason: "The Remove cleanup policy should stop the container, then remove it.", + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + want: []fakeDockerCall{ + {Method: "ContainerStop", Ref: containerID, Options: client.ContainerStopOptions{}}, + {Method: "ContainerRemove", Ref: containerID, Options: client.ContainerRemoveOptions{}}, + }, + }, + "Orphan": { + reason: "The Orphan cleanup policy should leave the container running without calling Docker.", + cleanup: AnnotationValueRuntimeDockerCleanupOrphan, + want: nil, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cli := &fakeContainerClient{ + MockContainerCreate: func(_ context.Context, _ client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + return client.ContainerCreateResult{ID: containerID}, nil + }, + MockContainerInspect: func(_ context.Context, _ string, _ client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return client.ContainerInspectResult{Container: container.InspectResponse{ + Name: "/" + containerName, + NetworkSettings: &container.NetworkSettings{ + Networks: map[string]*network.EndpointSettings{dockerNetwork: {}}, + }, + }}, nil + }, + } + r := &RuntimeDocker{ + Image: "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0", + Cleanup: tc.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) + } + if diff := cmp.Diff(wantStartCalls, cli.Calls, cmpopts.IgnoreFields(fakeDockerCall{}, "Options")); diff != "" { + t.Fatalf("\n%s\nStart(...): -want calls, +got calls:\n%s", tc.reason, diff) + } + + cli.Calls = nil + if err := rctx.Stop(t.Context()); err != nil { + t.Fatalf("\n%s\nStop(...): unexpected error: %v", tc.reason, err) + } + if diff := cmp.Diff(tc.want, cli.Calls); diff != "" { + t.Errorf("\n%s\nStop(...): -want calls, +got calls:\n%s", tc.reason, diff) + } + }) + } +} From dac88ccb76c257bed074920d76de639ccaa33d53 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 | 107 +++++++++++++++++++ cmd/crossplane/render/runtime_docker.go | 34 ++++-- cmd/crossplane/render/runtime_docker_test.go | 88 ++++++++++++--- cmd/crossplane/render/xr/cmd.go | 6 +- 6 files changed, 253 insertions(+), 38 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..0ca7362d 100644 --- a/cmd/crossplane/render/render_test.go +++ b/cmd/crossplane/render/render_test.go @@ -1,12 +1,16 @@ package render import ( + "context" + "strings" "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 +95,106 @@ 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) { + errBoom := errors.New("boom") + + // Loop so Go's randomized map iteration can't hide a regression to + // returning on the first error. + for range 20 { + stopped := map[string]bool{} + mk := func(name string, err error) RuntimeContext { + return RuntimeContext{Target: name + ":9443", Stop: func(context.Context) error { + stopped[name] = true + return err + }} + } + fa := &FunctionAddresses{contexts: map[string]RuntimeContext{ + "ok-a": mk("ok-a", nil), + "fail-a": mk("fail-a", errBoom), + "ok-b": mk("ok-b", nil), + "fail-b": mk("fail-b", errBoom), + }} + + err := fa.Stop(t.Context()) + + if diff := cmp.Diff(map[string]bool{"ok-a": true, "fail-a": true, "ok-b": true, "fail-b": true}, stopped); diff != "" { + t.Fatalf("Stop(): -want stopped, +got stopped:\n%s", diff) + } + if err == nil { + t.Fatal("Stop(): want error, got nil") + } + for _, name := range []string{"fail-a", "fail-b"} { + if !strings.Contains(err.Error(), `"`+name+`"`) { + t.Errorf("Stop(): error %q does not mention %q", err, name) + } + } + for _, name := range []string{"ok-a", "ok-b"} { + if strings.Contains(err.Error(), `"`+name+`"`) { + t.Errorf("Stop(): error %q unexpectedly mentions %q", err, name) + } + } + } + + fa := &FunctionAddresses{contexts: map[string]RuntimeContext{ + "ok": {Stop: func(context.Context) error { return nil }}, + }} + if err := fa.Stop(t.Context()); err != nil { + t.Errorf("Stop(): want nil error when all succeed, got %v", err) + } +} + +func TestStopFunctionRuntimes(t *testing.T) { + type want struct { + stopped bool + ctxErr error + hasDeadline bool + err error + } + + cases := map[string]struct { + reason string + cancelParent bool + want want + }{ + "ParentContextLive": { + reason: "StopFunctionRuntimes should stop each runtime with a context bounded by its own timeout.", + want: want{stopped: true, hasDeadline: true}, + }, + "ParentContextCancelled": { + reason: "StopFunctionRuntimes should still stop each runtime, with a live but bounded context, when the parent context is already cancelled.", + cancelParent: true, + want: want{stopped: true, hasDeadline: true}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + var got want + fa := &FunctionAddresses{contexts: map[string]RuntimeContext{ + "fn": {Target: "fn:9443", Stop: func(ctx context.Context) error { + got.stopped = true + got.ctxErr = ctx.Err() + _, got.hasDeadline = ctx.Deadline() + return nil + }}, + }} + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + if tc.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) + } + }) + } + + if err := StopFunctionRuntimes(t.Context(), nil); err != nil { + t.Errorf("StopFunctionRuntimes(nil): want nil error, got %v", err) + } +} 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 43bbda43..707a55bb 100644 --- a/cmd/crossplane/render/runtime_docker_test.go +++ b/cmd/crossplane/render/runtime_docker_test.go @@ -19,6 +19,7 @@ package render import ( "context" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -28,6 +29,7 @@ import ( "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" @@ -257,6 +259,13 @@ func TestRuntimeDockerStop(t *testing.T) { dockerNetwork = "render-net" ) + errStop := errors.New("stop boom") + errRemove := errors.New("remove boom") + + grace := int(containerStopGracePeriod / time.Second) + stopCall := fakeDockerCall{Method: "ContainerStop", Ref: containerID, Options: client.ContainerStopOptions{Timeout: &grace}} + removeCall := fakeDockerCall{Method: "ContainerRemove", Ref: containerID, Options: client.ContainerRemoveOptions{Force: true}} + // Start always creates, starts, and inspects the container through the // injected client before returning the stop closure under test. wantStartCalls := []fakeDockerCall{ @@ -265,30 +274,70 @@ func TestRuntimeDockerStop(t *testing.T) { {Method: "ContainerInspect", Ref: containerID}, } + type want struct { + calls []fakeDockerCall + // err is the expected error message, or empty for no error. + err string + } + cases := map[string]struct { - reason string - cleanup DockerCleanup - want []fakeDockerCall + reason string + cleanup DockerCleanup + stopErr error + removeErr error + 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.", cleanup: AnnotationValueRuntimeDockerCleanupStop, - want: []fakeDockerCall{ - {Method: "ContainerStop", Ref: containerID, Options: client.ContainerStopOptions{}}, + want: want{calls: []fakeDockerCall{stopCall}}, + }, + "StopError": { + reason: "The Stop cleanup policy should return an error when the container cannot be stopped.", + cleanup: AnnotationValueRuntimeDockerCleanupStop, + stopErr: errStop, + want: want{ + calls: []fakeDockerCall{stopCall}, + err: errors.Wrap(errStop, "cannot stop Docker container").Error(), }, }, "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.", + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + want: want{calls: []fakeDockerCall{stopCall, removeCall}}, + }, + "RemoveStopError": { + reason: "The Remove cleanup policy should still force remove the container when the graceful stop fails, and succeed if removal does.", cleanup: AnnotationValueRuntimeDockerCleanupRemove, - want: []fakeDockerCall{ - {Method: "ContainerStop", Ref: containerID, Options: client.ContainerStopOptions{}}, - {Method: "ContainerRemove", Ref: containerID, Options: client.ContainerRemoveOptions{}}, + stopErr: errStop, + want: want{calls: []fakeDockerCall{stopCall, removeCall}}, + }, + "RemoveError": { + reason: "The Remove cleanup policy should return an error when the container cannot be removed.", + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + removeErr: errRemove, + want: want{ + calls: []fakeDockerCall{stopCall, removeCall}, + err: errors.Wrap(errRemove, "cannot remove Docker container").Error(), + }, + }, + "RemoveStopAndRemoveError": { + reason: "The Remove cleanup policy should return both errors when the container can be neither stopped nor removed.", + cleanup: AnnotationValueRuntimeDockerCleanupRemove, + stopErr: errStop, + removeErr: errRemove, + want: want{ + calls: []fakeDockerCall{stopCall, removeCall}, + err: errors.Join( + errors.Wrap(errRemove, "cannot remove Docker container"), + errors.Wrap(errStop, "cannot stop Docker container"), + ).Error(), }, }, "Orphan": { reason: "The Orphan cleanup policy should leave the container running without calling Docker.", cleanup: AnnotationValueRuntimeDockerCleanupOrphan, - want: nil, + want: want{}, }, } @@ -306,6 +355,12 @@ func TestRuntimeDockerStop(t *testing.T) { }, }}, nil }, + MockContainerStop: func(_ context.Context, _ string, _ client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, tc.stopErr + }, + MockContainerRemove: func(_ context.Context, _ string, _ client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + return client.ContainerRemoveResult{}, tc.removeErr + }, } r := &RuntimeDocker{ Image: "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0", @@ -326,11 +381,14 @@ func TestRuntimeDockerStop(t *testing.T) { } cli.Calls = nil - if err := rctx.Stop(t.Context()); err != nil { - t.Fatalf("\n%s\nStop(...): unexpected error: %v", tc.reason, err) + err = rctx.Stop(t.Context()) + + got := want{calls: cli.Calls} + if err != nil { + got.err = err.Error() } - if diff := cmp.Diff(tc.want, cli.Calls); diff != "" { - t.Errorf("\n%s\nStop(...): -want calls, +got calls:\n%s", tc.reason, diff) + if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(want{})); diff != "" { + t.Errorf("\n%s\nStop(...): -want, +got:\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 8a729e42ebac17c3cc389494cb434f96870742eb Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 2 Oct 2026 13:50:05 -0400 Subject: [PATCH 3/3] fix(render): stop already-started function runtimes when a later one fails to start StartFunctionRuntimes starts each Function's runtime in turn. If getting or starting a later runtime failed it returned the error without stopping the runtimes it had already started, so their containers were leaked and the caller had no FunctionAddresses to stop them with. On a get or start failure, stop the runtimes started so far via the same FunctionAddresses.stop helper StopFunctionRuntimes uses, with a context detached from cancellation (ctx may be why the start failed) and the per-runtime runtimeStopTimeout. The start error stays primary; any stop errors are joined to it. StartFunctionRuntimes is now a thin wrapper over startFunctionRuntimes, which takes the runtime getter as a parameter so the rollback can be tested. Fixes #396 Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/render.go | 22 ++++++++++-- cmd/crossplane/render/render_test.go | 52 ++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/cmd/crossplane/render/render.go b/cmd/crossplane/render/render.go index ff5f664f..70a8f819 100644 --- a/cmd/crossplane/render/render.go +++ b/cmd/crossplane/render/render.go @@ -146,18 +146,24 @@ func (fa *FunctionAddresses) stop(ctx context.Context, timeout time.Duration) er // gRPC addresses. The caller must call Stop on the returned FunctionAddresses // when done. func StartFunctionRuntimes(ctx context.Context, log logging.Logger, fns []pkgv1.Function) (*FunctionAddresses, error) { + return startFunctionRuntimes(ctx, log, fns, GetRuntime) +} + +// startFunctionRuntimes implements StartFunctionRuntimes, using getRuntime to +// get each Function's runtime. +func startFunctionRuntimes(ctx context.Context, log logging.Logger, fns []pkgv1.Function, getRuntime func(pkgv1.Function, logging.Logger) (Runtime, error)) (*FunctionAddresses, error) { addrs := make(map[string]string, len(fns)) contexts := make(map[string]RuntimeContext, len(fns)) for _, fn := range fns { - rt, err := GetRuntime(fn, log) + rt, err := getRuntime(fn, log) if err != nil { - return nil, errors.Wrapf(err, "cannot get runtime for Function %q", fn.GetName()) + return nil, stopStarted(ctx, &FunctionAddresses{addrs: addrs, contexts: contexts}, errors.Wrapf(err, "cannot get runtime for Function %q", fn.GetName())) } rctx, err := rt.Start(ctx) if err != nil { - return nil, errors.Wrapf(err, "cannot start Function %q", fn.GetName()) + return nil, stopStarted(ctx, &FunctionAddresses{addrs: addrs, contexts: contexts}, errors.Wrapf(err, "cannot start Function %q", fn.GetName())) } addrs[fn.GetName()] = rctx.Target @@ -167,6 +173,16 @@ func StartFunctionRuntimes(ctx context.Context, log logging.Logger, fns []pkgv1. return &FunctionAddresses{addrs: addrs, contexts: contexts}, nil } +// stopStarted stops the runtimes started before a later Function failed to +// start. It returns startErr, joined with any stop errors. The stop context is +// detached from ctx's cancellation, since ctx may be why the start failed. +func stopStarted(ctx context.Context, fa *FunctionAddresses, startErr error) error { + if err := fa.stop(context.WithoutCancel(ctx), runtimeStopTimeout); err != nil { + return errors.Join(startErr, err) + } + return startErr +} + // RewriteAddressesForDocker rewrites function addresses so they are reachable // from inside a Docker container. Addresses targeting localhost or 127.0.0.1 // are rewritten to host.docker.internal. diff --git a/cmd/crossplane/render/render_test.go b/cmd/crossplane/render/render_test.go index 0ca7362d..e3567cad 100644 --- a/cmd/crossplane/render/render_test.go +++ b/cmd/crossplane/render/render_test.go @@ -10,6 +10,7 @@ import ( 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" ) @@ -198,3 +199,54 @@ func TestStopFunctionRuntimes(t *testing.T) { t.Errorf("StopFunctionRuntimes(nil): want nil error, got %v", err) } } + +type fakeRuntime struct { + start func(ctx context.Context) (RuntimeContext, error) +} + +func (r fakeRuntime) Start(ctx context.Context) (RuntimeContext, error) { return r.start(ctx) } + +func TestStartFunctionRuntimesStopsStartedOnFailure(t *testing.T) { + stopped := map[string]bool{} + errBoom := errors.New("boom") + errStop := errors.New("stop failed") + + getRuntime := func(fn pkgv1.Function, _ logging.Logger) (Runtime, error) { + name := fn.GetName() + return fakeRuntime{start: func(_ context.Context) (RuntimeContext, error) { + if name == "fn-c" { + return RuntimeContext{}, errBoom + } + return RuntimeContext{Target: name, Stop: func(_ context.Context) error { + stopped[name] = true + if name == "fn-b" { + return errStop + } + return nil + }}, nil + }}, nil + } + + fns := []pkgv1.Function{ + {ObjectMeta: metav1.ObjectMeta{Name: "fn-a"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "fn-b"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "fn-c"}}, + } + + fa, err := startFunctionRuntimes(t.Context(), logging.NewNopLogger(), fns, getRuntime) + if fa != nil { + t.Errorf("StartFunctionRuntimes(...): want nil FunctionAddresses, got %v", fa) + } + if !errors.Is(err, errBoom) { + t.Errorf("StartFunctionRuntimes(...): want error wrapping start error %v, got %v", errBoom, err) + } + if !strings.HasPrefix(strings.TrimPrefix(err.Error(), "["), `cannot start Function "fn-c"`) { + t.Errorf("StartFunctionRuntimes(...): want start error first, got %v", err) + } + if !errors.Is(err, errStop) { + t.Errorf("StartFunctionRuntimes(...): want error joined with stop error %v, got %v", errStop, err) + } + if diff := cmp.Diff(map[string]bool{"fn-a": true, "fn-b": true}, stopped); diff != "" { + t.Errorf("StartFunctionRuntimes(...): -want stopped, +got stopped:\n%s", diff) + } +}