From 68088d62d24d05c95eae9b96ac640b960ff36301 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 3 Nov 2026 17:17:16 -0500 Subject: [PATCH 1/5] refactor(containerruntime): add NewSessionID and label domain constants Move session ID generation and the render.crossplane.io label domain into containerruntime so the run lease code and its callers share one source for both. The diff server and the render Docker runtime now use these constants instead of their own copies. No behavior change. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- internal/containerruntime/containerruntime.go | 21 +++++++++++++++++-- internal/diffserver/manager.go | 2 +- pkg/render/runtime_docker.go | 2 +- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/internal/containerruntime/containerruntime.go b/internal/containerruntime/containerruntime.go index 3fbe8e69..3c78cc04 100644 --- a/internal/containerruntime/containerruntime.go +++ b/internal/containerruntime/containerruntime.go @@ -23,10 +23,18 @@ limitations under the License. package containerruntime import ( + "crypto/rand" + "encoding/hex" "sync" "time" +) + +const ( + // DomainRender is the label and annotation domain render uses. + DomainRender = "render.crossplane.io" - "k8s.io/apimachinery/pkg/util/rand" + // DomainSimulate is the label and annotation domain simulate uses. + DomainSimulate = "simulate.crossplane.io" ) // Cleanup specifies what to do with a package's container once its run is @@ -96,11 +104,20 @@ const ( LabelValueManagedBy = "crossplane-cli" ) +// NewSessionID returns a new random session ID: 16 lowercase hex characters +// (64 random bits). +func NewSessionID() string { + b := make([]byte, 8) + _, _ = rand.Read(b) // Never returns an error. + + return hex.EncodeToString(b) +} + // SessionID returns an identifier unique to this process. It is the session // ID used for Docker resources when the caller doesn't supply one. // //nolint:gochecknoglobals // Must be stable for the life of the process. -var SessionID = sync.OnceValue(func() string { return rand.String(12) }) +var SessionID = sync.OnceValue(NewSessionID) // ManagedLabels returns the labels marking a Docker resource as managed by // the crossplane CLI under the given annotation domain: diff --git a/internal/diffserver/manager.go b/internal/diffserver/manager.go index 418e4462..6e143c36 100644 --- a/internal/diffserver/manager.go +++ b/internal/diffserver/manager.go @@ -101,7 +101,7 @@ const ( // labelDomain prefixes the labels simulate applies to the Docker containers // it creates. -const labelDomain = "simulate.crossplane.io" +const labelDomain = containerruntime.DomainSimulate // Labels applied to the Docker containers simulate creates, mirroring // render's, so leftovers (a crash, or an Orphan cleanup policy) can be diff --git a/pkg/render/runtime_docker.go b/pkg/render/runtime_docker.go index 17c79413..60270c16 100644 --- a/pkg/render/runtime_docker.go +++ b/pkg/render/runtime_docker.go @@ -35,7 +35,7 @@ const FunctionPort = 9443 // labelDomain prefixes the labels render applies to the Docker resources it // creates. -const labelDomain = "render.crossplane.io" +const labelDomain = containerruntime.DomainRender // Annotations that can be used to configure the Docker runtime. const ( From a527bf8f68dcf04386dd03a99bffd91b5d520fbc Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 3 Nov 2026 17:17:17 -0500 Subject: [PATCH 2/5] feat(containerruntime): lease runs and reclaim what dead runs left on Docker Add Run, which holds a file lock lease for a session ID while the process is alive. On start, a Run sweeps Docker for containers and networks labeled with session IDs whose lease is no longer held, so a crashed or SIGKILLed render no longer leaves them behind. The sweep honors each resource's cleanup policy: containers asked to be kept stay, along with the networks they still use, and unknown policies are kept rather than guessed at. Release removes the run's own resources and drops the lease. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- go.mod | 2 +- internal/containerruntime/export_test.go | 31 + internal/containerruntime/fake/fake.go | 23 +- internal/containerruntime/lease_unix_test.go | 154 +++ internal/containerruntime/lock_unix.go | 35 + internal/containerruntime/lock_windows.go | 34 + internal/containerruntime/run.go | 599 ++++++++++++ internal/containerruntime/run_test.go | 960 +++++++++++++++++++ 8 files changed, 1836 insertions(+), 2 deletions(-) create mode 100644 internal/containerruntime/export_test.go create mode 100644 internal/containerruntime/lease_unix_test.go create mode 100644 internal/containerruntime/lock_unix.go create mode 100644 internal/containerruntime/lock_windows.go create mode 100644 internal/containerruntime/run.go create mode 100644 internal/containerruntime/run_test.go diff --git a/go.mod b/go.mod index 2ee84228..a4cd7dd5 100644 --- a/go.mod +++ b/go.mod @@ -51,6 +51,7 @@ require ( github.com/willabides/kongplete v0.4.0 golang.org/x/mod v0.42.0 golang.org/x/sync v0.24.0 + golang.org/x/sys v0.49.0 golang.org/x/text v0.43.0 golang.org/x/tools v0.52.0 google.golang.org/genproto/googleapis/rpc v0.0.0-20260831171406-18b4a7587f8a @@ -319,7 +320,6 @@ require ( golang.org/x/exp v0.0.0-20260824195058-e88cd73687aa // indirect golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect - golang.org/x/sys v0.49.0 // indirect golang.org/x/term v0.47.0 // indirect golang.org/x/time v0.15.0 // indirect gomodules.xyz/jsonpatch/v2 v2.5.0 // indirect diff --git a/internal/containerruntime/export_test.go b/internal/containerruntime/export_test.go new file mode 100644 index 00000000..2b93becc --- /dev/null +++ b/internal/containerruntime/export_test.go @@ -0,0 +1,31 @@ +/* +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 containerruntime + +// TryLease reports whether the lease of run id in dir could be taken, i.e. +// whether no process holds it, and gives it straight back. +func TryLease(dir, id string) bool { + f, ok := tryLease(leasePath(dir, id)) + if ok { + _ = f.Close() + } + + return ok +} + +// LeasePath is the path of run id's lease file in dir. +var LeasePath = leasePath diff --git a/internal/containerruntime/fake/fake.go b/internal/containerruntime/fake/fake.go index f72687b4..b64a8793 100644 --- a/internal/containerruntime/fake/fake.go +++ b/internal/containerruntime/fake/fake.go @@ -36,9 +36,15 @@ type Client struct { 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) + MockContainerList func(ctx context.Context, options client.ContainerListOptions) (client.ContainerListResult, error) + MockNetworkList func(ctx context.Context, options client.NetworkListOptions) (client.NetworkListResult, error) + MockNetworkRemove func(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) } -var _ containerruntime.Client = &Client{} +var ( + _ containerruntime.Client = &Client{} + _ containerruntime.RunClient = &Client{} +) // ImagePull calls MockImagePull. func (c *Client) ImagePull(ctx context.Context, ref string, options client.ImagePullOptions) (client.ImagePullResponse, error) { @@ -70,6 +76,21 @@ func (c *Client) ContainerRemove(ctx context.Context, containerID string, option return c.MockContainerRemove(ctx, containerID, options) } +// ContainerList calls MockContainerList. +func (c *Client) ContainerList(ctx context.Context, options client.ContainerListOptions) (client.ContainerListResult, error) { + return c.MockContainerList(ctx, options) +} + +// NetworkList calls MockNetworkList. +func (c *Client) NetworkList(ctx context.Context, options client.NetworkListOptions) (client.NetworkListResult, error) { + return c.MockNetworkList(ctx, options) +} + +// NetworkRemove calls MockNetworkRemove. +func (c *Client) NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + return c.MockNetworkRemove(ctx, networkID, options) +} + // ImagePullDone is a MockImagePull that reports an image pull that has // already completed. func ImagePullDone(context.Context, string, client.ImagePullOptions) (client.ImagePullResponse, error) { diff --git a/internal/containerruntime/lease_unix_test.go b/internal/containerruntime/lease_unix_test.go new file mode 100644 index 00000000..28fe1302 --- /dev/null +++ b/internal/containerruntime/lease_unix_test.go @@ -0,0 +1,154 @@ +//go:build unix + +/* +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 containerruntime_test + +import ( + "bufio" + "context" + "fmt" + "io" + "os" + "os/exec" + "testing" + "time" + + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" + + "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + + cr "github.com/crossplane/cli/v2/internal/containerruntime" +) + +const ( + envLeaseHelperDir = "CONTAINERRUNTIME_LEASE_HELPER_DIR" + envLeaseHelperID = "CONTAINERRUNTIME_LEASE_HELPER_ID" + leaseHelperReady = "leased" +) + +// TestLeaseHelper is not a test: it runs as startLeaseHelper's child process, +// where it takes a lease, says so on stdout, and holds the lease until its +// stdin closes. The parent holds the other end of stdin, and the kernel +// closes it however the parent dies, so the child never outlives it. +func TestLeaseHelper(t *testing.T) { + dir, id := os.Getenv(envLeaseHelperDir), os.Getenv(envLeaseHelperID) + if dir == "" { + t.Skip("only runs as startLeaseHelper's child process") + } + + h := &host{} + r := cr.NewRun(dir, id, true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + if err := r.Acquire(t.Context()); err != nil { + t.Fatalf("Acquire(...): unexpected error: %v", err) + } + + _, _ = fmt.Fprintln(os.Stdout, leaseHelperReady) + + _, _ = io.Copy(io.Discard, os.Stdin) +} + +// TestLeaseAcrossProcesses pins that a lease protects a run from reapers in +// other processes while it lives, and that the kernel gives the lease up when +// the process dies, however it dies. A fake host can't span processes, so this +// process's host is seeded with resources labelled with the child's ID. +func TestLeaseAcrossProcesses(t *testing.T) { + dir, id := t.TempDir(), cr.NewSessionID() + + cmd := startLeaseHelper(t, dir, id) + + if cr.TryLease(dir, id) { + t.Fatal("TryLease(...): took the lease of a run that is alive in another process") + } + + if err := cmd.Process.Kill(); err != nil { + t.Fatalf("Kill(): %v", err) + } + + _ = cmd.Wait() + + h := &host{ + containers: []container.Summary{ctr("fn", running, cr.ContainerLabels(cr.DomainRender, id, cr.CleanupRemove), "net")}, + networks: []network.Summary{net("net", cr.ManagedLabels(cr.DomainRender, id))}, + } + + r := cr.NewRun(dir, cr.NewSessionID(), true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + if err := r.Acquire(t.Context()); err != nil { + t.Fatalf("Acquire(...) after the child died: unexpected error: %v", err) + } + + t.Cleanup(func() { _ = r.Release(context.Background(), false, "") }) + + cs, ns := h.left() + if len(cs) != 0 || len(ns) != 0 { + t.Errorf("Acquire(...) after the child died: left containers %v and networks %v, want none", cs, ns) + } + + if _, err := os.Stat(cr.LeasePath(dir, id)); !os.IsNotExist(err) { + t.Errorf("lease file of the dead child: Stat(...) = %v, want it removed", err) + } +} + +// startLeaseHelper runs TestLeaseHelper in a child process that leases run id +// in dir, and returns once the child holds the lease. The child is killed +// when the test ends. +func startLeaseHelper(t *testing.T, dir, id string) *exec.Cmd { + t.Helper() + + cmd := exec.Command(os.Args[0], "-test.run=^TestLeaseHelper$") + cmd.Env = append(os.Environ(), envLeaseHelperDir+"="+dir, envLeaseHelperID+"="+id) + cmd.Stderr = os.Stderr + + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatalf("StdinPipe(): %v", err) + } + + out, err := cmd.StdoutPipe() + if err != nil { + t.Fatalf("StdoutPipe(): %v", err) + } + + if err := cmd.Start(); err != nil { + t.Fatalf("Start(): %v", err) + } + + t.Cleanup(func() { + _ = stdin.Close() + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + + ready := make(chan string, 1) + go func() { + s := bufio.NewScanner(out) + s.Scan() + ready <- s.Text() + }() + + select { + case got := <-ready: + if got != leaseHelperReady { + t.Fatalf("child process: got %q before stdout ended, want %q", got, leaseHelperReady) + } + case <-time.After(30 * time.Second): + t.Fatalf("child process: did not print %q within 30s", leaseHelperReady) + } + + return cmd +} diff --git a/internal/containerruntime/lock_unix.go b/internal/containerruntime/lock_unix.go new file mode 100644 index 00000000..86794c26 --- /dev/null +++ b/internal/containerruntime/lock_unix.go @@ -0,0 +1,35 @@ +//go:build unix + +/* +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 containerruntime + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// leasesSupported is whether runs on this platform take leases. +const leasesSupported = true + +// lock takes an exclusive flock on f without blocking. A flock belongs to the open file, not the +// process, so two opens of one lease conflict even within a process, and the kernel drops the lock +// when the process exits, however it exits. +func lock(f *os.File) error { + return unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB) +} diff --git a/internal/containerruntime/lock_windows.go b/internal/containerruntime/lock_windows.go new file mode 100644 index 00000000..666173cd --- /dev/null +++ b/internal/containerruntime/lock_windows.go @@ -0,0 +1,34 @@ +//go:build windows + +/* +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 containerruntime + +import ( + "os" +) + +// leasesSupported is whether runs on this platform take leases. acquireLease +// checks it before touching the lease directory. +const leasesSupported = false + +// lock is unsupported on Windows, where a file that is open cannot be renamed or removed, which the +// lease protocol relies on. Without a lease a run's resources are never reclaimed by another run; the +// run still removes its own when it exits. +func lock(_ *os.File) error { + return errLeaseUnsupported +} diff --git a/internal/containerruntime/run.go b/internal/containerruntime/run.go new file mode 100644 index 00000000..4660c871 --- /dev/null +++ b/internal/containerruntime/run.go @@ -0,0 +1,599 @@ +/* +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 containerruntime + +import ( + "context" + "encoding/hex" + "io" + "os" + "path/filepath" + "strings" + "time" + + "github.com/containerd/errdefs" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/crossplane/crossplane-runtime/v2/pkg/logging" +) + +// A Run ties the Docker resources one process creates (package containers, +// the render container and the render network) to that process, and reclaims +// the ones left behind by runs that died without cleaning up (SIGKILL, OOM, a +// second Ctrl+C, a CI runner tearing down the job). +// +// Ownership is carried by labels. A run's ID is the session ID its resources +// are labelled with: /session= alongside +// /managed-by=crossplane-cli, under either DomainRender or +// DomainSimulate. A run's resources are exactly the ones carrying both labels +// of one domain with its ID, so a container or network that lacks either is +// never touched. +// +// Liveness is a lease: a run holds an exclusive lock on ".lock" in the +// lease directory for as long as it has resources. The operating system drops +// the lock when the process exits however it exits, so a lease file whose lock +// can be taken belongs to a dead run. That needs no process table, cannot be +// fooled by PID reuse, and keeps concurrent runs from touching each other's +// resources. A lease that is not in this directory (another user, another +// container sharing the Docker socket) is never seen, so its resources are +// never touched. +// +// Reclaiming a dead run finishes what it would have done, following each +// container's recorded /cleanup label: Orphan, or a value this +// version does not know, is left alone, Stop is stopped, and Remove, or no +// cleanup label, is force removed. Its networks are removed after its +// containers. Reclaiming talks to the daemon this run connects to, so if +// DOCKER_HOST changed since a dead run, its lease is retired but its +// resources on the other daemon are left behind. +// +// A Run is not safe for concurrent use; its owner serializes access. +type Run struct { + id string + dir string + leased bool + log logging.Logger + newClient func() (RunClient, error) + + // acquired is set once Acquire has run. + acquired bool + // lease is the open, locked lease file while the run holds it; nil when + // the run is unleased, leasing failed, or the lease was released. + lease *os.File +} + +const ( + // runIDLength is the length of a run ID NewSessionID generates: 16 + // lowercase hex characters (64 random bits), so a session label carrying + // one cannot belong to anything but that run. + runIDLength = 16 + + leaseExt = ".lock" + + // tempLeaseExt ends the name of a lease file before acquireLease renames + // it into place. + tempLeaseExt = ".tmp" + + // staleTempLeaseAge is how old a temporary lease file must be before a + // reaper removes it: far longer than acquireLease holds one. + staleTempLeaseAge = time.Minute + + // reapTimeout bounds reclaiming dead runs' resources, and sweeping a run's + // own networks, so a slow Docker daemon cannot hold up a run. It covers all + // the dead runs one Acquire reclaims together, and each Stop container can + // take StopGracePeriod, so a few dead runs can run out of it. A run that is + // not fully reclaimed in time keeps its lease, and the next run finishes it. + reapTimeout = 10 * time.Second +) + +// errLeaseUnsupported is lock's error on a platform without run leases. +var errLeaseUnsupported = errors.New("run leases are not supported on this platform") + +// RunClient is the subset of the Docker client a Run uses to reclaim +// containers and networks. +type RunClient interface { + CleanupClient + ContainerList(ctx context.Context, options client.ContainerListOptions) (client.ContainerListResult, error) + NetworkList(ctx context.Context, options client.NetworkListOptions) (client.NetworkListResult, error) + NetworkRemove(ctx context.Context, networkID string, options client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) +} + +var _ RunClient = (*client.Client)(nil) + +// A RunOption configures a Run. +type RunOption func(*Run) + +// WithRunClient makes the Run talk to Docker through a client from newClient, +// instead of one configured from the environment. The Run closes each client +// newClient returns, if it is an io.Closer, once done with it. +func WithRunClient(newClient func() (RunClient, error)) RunOption { + return func(r *Run) { + r.newClient = newClient + } +} + +// DefaultLeaseDir is where runs keep their leases: a per-user directory, so +// one user's runs never judge another's. It returns "" when the user has no +// cache directory, which disables leasing and reaping. +func DefaultLeaseDir() string { + dir, err := os.UserCacheDir() + if err != nil { + return "" + } + + return filepath.Join(dir, "crossplane", "runs") +} + +// NewRun returns a Run with the given ID that keeps its lease in dir. An +// empty dir disables both leasing and reaping. leased says whether id is one +// NewSessionID generated for this run alone; only such a run takes a lease, +// because two processes sharing a supplied ID would rename over each other's +// lease. NewRun does no I/O. +func NewRun(dir, id string, leased bool, log logging.Logger, opts ...RunOption) *Run { + if log == nil { + log = logging.NewNopLogger() + } + + r := &Run{id: id, dir: dir, leased: leased, log: log, newClient: newRunClient} + for _, o := range opts { + o(r) + } + + return r +} + +// ID is the run's ID: the session label value that makes a container or +// network this run's. +func (r *Run) ID() string { + return r.id +} + +// Acquire takes the run's lease, when it is leased, then reclaims what every +// dead run in the lease directory left behind, bounded by reapTimeout. Call +// it before creating any Docker resource. Acquire is idempotent: only the +// first call does anything. +// +// The returned error is a lease or reap failure, for the caller to warn +// about. It never means the run cannot go on: an unleased run's resources +// only risk leaking if it dies, and a dead run that could not be reclaimed is +// retried by the next run. +func (r *Run) Acquire(ctx context.Context) error { + if r.acquired { + return nil + } + + r.acquired = true + + if r.dir == "" { + return nil + } + + var errs []error + + if r.leased { + f, err := r.acquireLease() + switch { + case errors.Is(err, errLeaseUnsupported): + // Not a failure to warn about on every run: the platform has no + // leases, so no run is ever reclaimed, as before leases existed. + r.log.Debug("Run leases are not supported on this platform; this run's Docker resources will not be reclaimed if it dies") + case err != nil: + errs = append(errs, errors.Wrap(err, "cannot lease this run's Docker resources; they will not be reclaimed if it dies")) + } + + r.lease = f + } + + if err := r.reapDead(ctx); err != nil { + errs = append(errs, errors.Wrap(err, "cannot clean up Docker resources left behind by an earlier run; the next run will retry")) + } + + return errors.Join(errs...) +} + +// Release removes the run's leftover session networks under sweepDomain, +// unless it is empty, then gives up the run's lease. The lease file is kept, +// so the next run reclaims what is left, when clean is false or a network +// could not be removed. clean reports whether the caller removed everything +// else the run created. Release is a no-op on the lease of an unleased run, +// and safe to call more than once. +func (r *Run) Release(ctx context.Context, clean bool, sweepDomain string) error { + var err error + if sweepDomain != "" { + err = r.sweepNetworks(ctx, sweepDomain) + } + + releaseLease(r.lease, leasePath(r.dir, r.id), clean && err == nil) + r.lease = nil + + return err +} + +// acquireLease takes the run's lease, refusing an ID that is not one +// NewSessionID generates: reapers skip such lease files, so it would protect +// nothing and never be reclaimed. +func (r *Run) acquireLease() (*os.File, error) { + if !validRunID(r.id) { + return nil, errors.Errorf("run ID %q is not %d lowercase hex characters", r.id, runIDLength) + } + + return acquireLease(r.dir, r.id) +} + +// sweepNetworks removes every network labelled with the run's session under +// domain. As in reclaim, a network that one of the run's kept containers +// (Orphan, or a cleanup policy this version does not know) is still attached +// to is left, and is not an error. +func (r *Run) sweepNetworks(ctx context.Context, domain string) error { + cli, err := r.newClient() + if err != nil { + return errors.Wrap(err, "cannot remove this run's Docker networks") + } + defer closeClient(cli, r.log) + + ctx, cancel := context.WithTimeout(ctx, reapTimeout) + defer cancel() + + kept := map[string]bool{} + + var errs []error + + res, err := cli.ContainerList(ctx, client.ContainerListOptions{All: true, Filters: sessionFilter(domain, r.id)}) + if err != nil { + // Still try the networks. The list failure is reported either way, + // so it keeps the lease and the next run retries. + errs = append(errs, errors.Wrap(err, "cannot list Docker containers")) + } + + for _, c := range res.Items { + if keeps(Cleanup(c.Labels[domain+labelSuffixCleanup])) { + keep(kept, c) + } + } + + errs = append(errs, r.removeNetworks(ctx, cli, r.id, kept, domain)) + + return errors.Wrap(errors.Join(errs...), "cannot remove this run's Docker networks") +} + +// deadLease is a lease this run took from a dead run. +type deadLease struct { + id string + path string + f *os.File +} + +// reapDead reclaims the resources of every run whose lease is in r.dir but +// held by no process. With no dead lease it makes no Docker call. +func (r *Run) reapDead(ctx context.Context) error { + // Not filepath.Glob: it would read glob syntax in r.dir as a pattern. + entries, err := os.ReadDir(r.dir) + if err != nil { + return nil // Best effort: a missing or unreadable dir has no leases to reap. + } + + var dead []deadLease + + for _, e := range entries { + if !e.Type().IsRegular() { + continue + } + + if isTempLease(e.Name()) { + removeStaleTempLease(filepath.Join(r.dir, e.Name())) + continue + } + + id, ok := strings.CutSuffix(e.Name(), leaseExt) + if !ok || id == r.id || !validRunID(id) { + continue + } + + p := filepath.Join(r.dir, e.Name()) + + if f, ok := tryLease(p); ok { + dead = append(dead, deadLease{id: id, path: p, f: f}) + } + } + + if len(dead) == 0 { + return nil + } + + cli, err := r.newClient() + if err != nil { + // NOTE(jcogilvie): a Docker-less machine that once left a lease retries this + // on every run. Drop such leases after N failures if that shows up. + for _, l := range dead { + releaseLease(l.f, l.path, false) + } + + return err + } + defer closeClient(cli, r.log) + + ctx, cancel := context.WithTimeout(ctx, reapTimeout) + defer cancel() + + var errs []error + + for _, l := range dead { + err := r.reclaim(ctx, cli, l.id) + if err != nil { + errs = append(errs, errors.Wrapf(err, "run %s", l.id)) + } else { + r.log.Debug("Reclaimed Docker resources left by a dead run", "run", l.id) + } + + releaseLease(l.f, l.path, err == nil) + } + + return errors.Join(errs...) +} + +// reclaim finishes what dead run id would have done to its containers, per +// each one's cleanup label, then removes its networks. It reports whether the +// run is fully reclaimed: a network that a kept container is attached to does +// not count against it. +func (r *Run) reclaim(ctx context.Context, cli RunClient, id string) error { + grace := int(StopGracePeriod / time.Second) + + // kept holds the names of the networks kept containers are attached to. + kept := map[string]bool{} + + var errs []error + + for _, d := range []string{DomainRender, DomainSimulate} { + res, err := cli.ContainerList(ctx, client.ContainerListOptions{All: true, Filters: sessionFilter(d, id)}) + if err != nil { + errs = append(errs, errors.Wrap(err, "cannot list Docker containers")) + continue + } + + for _, c := range res.Items { + switch cl := Cleanup(c.Labels[d+labelSuffixCleanup]); { + case keeps(cl): + if cl != CleanupOrphan { + r.log.Debug("Leaving Docker container with an unknown cleanup policy", "container", c.ID, "cleanup", cl, "run", id) + } + keep(kept, c) + case cl == CleanupStop: + if err := stopContainer(ctx, cli, c.ID, client.ContainerStopOptions{Timeout: &grace}); err != nil && !errdefs.IsNotFound(err) { + errs = append(errs, errors.Wrapf(err, "cannot stop Docker container %s", c.ID)) + } + default: + // Remove, or no cleanup label: the render container, which + // the dead run would have removed. Force stops it if running. + if err := removeContainer(ctx, cli, c.ID); err != nil && !errdefs.IsNotFound(err) { + errs = append(errs, errors.Wrapf(err, "cannot remove Docker container %s", c.ID)) + } + } + } + } + + if len(errs) > 0 { + // A container still attached would make removing its network fail, + // so networks wait for every container to be finished. + return errors.Join(errs...) + } + + return r.removeNetworks(ctx, cli, id, kept, DomainRender, DomainSimulate) +} + +// keeps reports whether cleanup policy cl leaves a container running: Orphan, +// or a policy this version does not know, perhaps from a newer one, which may +// have asked to keep it. No policy (no cleanup label) is not kept. +func keeps(cl Cleanup) bool { + return cl != CleanupStop && cl != CleanupRemove && cl != "" +} + +// keep records the networks kept container c is attached to. +func keep(kept map[string]bool, c container.Summary) { + if c.NetworkSettings == nil { + return + } + + for n := range c.NetworkSettings.Networks { + kept[n] = true + } +} + +// removeNetworks removes every network of run id under domains. A run that +// used no Docker render engine, or a network the user supplied, has none. A +// network named in kept is left with the kept container attached to it, even +// if that container is stopped: Docker would remove it then, and the +// container could not start again without it. +func (r *Run) removeNetworks(ctx context.Context, cli RunClient, id string, kept map[string]bool, domains ...string) error { + var errs []error + + for _, d := range domains { + res, err := cli.NetworkList(ctx, client.NetworkListOptions{Filters: sessionFilter(d, id)}) + if err != nil { + errs = append(errs, errors.Wrap(err, "cannot list Docker networks")) + continue + } + + for _, n := range res.Items { + if kept[n.Name] { + r.log.Debug("Leaving Docker network attached to a kept container", "network", n.Name, "run", id) + continue + } + + _, err := cli.NetworkRemove(ctx, n.ID, client.NetworkRemoveOptions{}) + + switch { + case err == nil, errdefs.IsNotFound(err): + default: + errs = append(errs, errors.Wrapf(err, "cannot remove Docker network %s", n.Name)) + } + } + } + + return errors.Join(errs...) +} + +// newRunClient returns a Docker client configured from the environment. +func newRunClient() (RunClient, error) { + cli, err := client.New(client.FromEnv) + if err != nil { + return nil, errors.Wrap(err, "cannot create Docker client using environment variables") + } + + return cli, nil +} + +// closeClient closes cli if it holds a connection. +func closeClient(cli RunClient, log logging.Logger) { + c, ok := cli.(io.Closer) + if !ok { + return + } + + if err := c.Close(); err != nil { + log.Debug("Error closing Docker client", "error", err) + } +} + +// sessionFilter matches what run id created under domain. Docker requires +// every label filter to match, so a resource is matched only if it carries +// both labels. +func sessionFilter(domain, id string) client.Filters { + return make(client.Filters).Add("label", + domain+labelSuffixSession+"="+id, + domain+labelSuffixManagedBy+"="+LabelValueManagedBy) +} + +func validRunID(id string) bool { + if len(id) != runIDLength { + return false + } + + _, err := hex.DecodeString(id) + + return err == nil && strings.ToLower(id) == id +} + +func leasePath(dir, id string) string { + return filepath.Join(dir, id+leaseExt) +} + +// acquireLease creates and locks run id's lease file. The file is locked +// under a temporary name and renamed into place, so a reaper never sees a +// lease file that is not yet locked. +func acquireLease(dir, id string) (*os.File, error) { + if !leasesSupported { + return nil, errLeaseUnsupported + } + + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, errors.Wrap(err, "cannot create lease directory") + } + + f, err := os.CreateTemp(dir, id+"-*"+tempLeaseExt) + if err != nil { + return nil, errors.Wrap(err, "cannot create lease file") + } + + if err := lock(f); err != nil { + _ = f.Close() + _ = os.Remove(f.Name()) + + return nil, errors.Wrap(err, "cannot lock lease file") + } + + if err := os.Rename(f.Name(), leasePath(dir, id)); err != nil { + _ = f.Close() + _ = os.Remove(f.Name()) + + return nil, errors.Wrap(err, "cannot publish lease file") + } + + return f, nil +} + +// isTempLease reports whether name is a lease file acquireLease has not yet +// renamed into place: "-.tmp". +func isTempLease(name string) bool { + rest, ok := strings.CutSuffix(name, tempLeaseExt) + if !ok { + return false + } + + id, _, ok := strings.Cut(rest, "-") + + return ok && validRunID(id) +} + +// removeStaleTempLease removes a temporary lease file that a run which died +// in acquireLease, between creating and renaming it, left behind. A file +// younger than staleTempLeaseAge may be another run's, about to be locked and +// renamed, so it is left; so is one whose lock is held. +func removeStaleTempLease(path string) { + fi, err := os.Stat(path) + if err != nil || time.Since(fi.ModTime()) < staleTempLeaseAge { + return + } + + if f, ok := tryLease(path); ok { + releaseLease(f, path, true) + } +} + +// tryLease takes the lease at path if no process holds it, i.e. if its run is +// dead. A lease that another reaper reclaimed and removed between our open +// and our lock is no longer at path, so it is not taken: each dead run is +// reclaimed by one reaper. +func tryLease(path string) (*os.File, bool) { + f, err := os.OpenFile(path, os.O_RDWR, 0) //nolint:gosec // The path is a lease file in our own lease directory. + if err != nil { + return nil, false + } + + if err := lock(f); err != nil { + _ = f.Close() + return nil, false + } + + held, err := f.Stat() + if err != nil { + _ = f.Close() + return nil, false + } + + if now, err := os.Stat(path); err != nil || !os.SameFile(held, now) { + _ = f.Close() + return nil, false + } + + return f, true +} + +// releaseLease gives up a lease, removing its file first when the run's +// resources are all gone, so no one can take a lease that no longer protects +// anything. A nil lease (unleased, or leasing failed) is a no-op. +func releaseLease(f *os.File, path string, remove bool) { + if f == nil { + return + } + + if remove { + _ = os.Remove(path) + } + + _ = f.Close() +} diff --git a/internal/containerruntime/run_test.go b/internal/containerruntime/run_test.go new file mode 100644 index 00000000..5bedd31f --- /dev/null +++ b/internal/containerruntime/run_test.go @@ -0,0 +1,960 @@ +/* +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 containerruntime_test + +import ( + "context" + "maps" + "os" + "path/filepath" + "regexp" + "runtime" + "slices" + "strings" + "sync" + "testing" + "time" + + "github.com/containerd/errdefs" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" + "github.com/moby/moby/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + + cr "github.com/crossplane/cli/v2/internal/containerruntime" + "github.com/crossplane/cli/v2/internal/containerruntime/fake" +) + +const ( + selfID = "00000000000000ff" + deadID = "dead00000000000a" + dead2ID = "dead00000000000b" + orphanID = "dead00000000000c" + futureID = "dead00000000000d" + liveID = "1111111111111111" +) + +const ( + running = container.StateRunning + exited = container.StateExited +) + +// host is an in-memory Docker daemon behind a fake.Client, holding containers, +// found by ID or name, and networks. It behaves as the daemon +// does wherever the difference would change what is left: create refuses a +// name in use; listing applies label filters (a resource matches only if it +// carries every one) and, without All, lists only running containers; a +// running container is only removed with Force; stopping a container leaves +// it exited; and a network is not removed while a running container is +// attached to it. +type host struct { + containers []container.Summary + networks []network.Summary + + // raced, when set, is created under its name by another run just before + // this run's first create, so that create finds the name taken. + raced *container.Summary + // cannotRemove names the container the daemon fails to remove. + cannotRemove string + // vanished holds IDs that are listed but gone by the time they are + // stopped or removed, as when a concurrent reaper got there first. + vanished map[string]bool +} + +// client is a Docker client of the host. +func (h *host) client() *fake.Client { + return &fake.Client{ + MockContainerInspect: h.containerInspect, + MockContainerCreate: h.containerCreate, + MockContainerStart: h.containerStart, + MockContainerList: h.containerList, + MockContainerStop: h.containerStop, + MockContainerRemove: h.containerRemove, + MockNetworkList: h.networkList, + MockNetworkRemove: h.networkRemove, + } +} + +// newClient is a RunClient factory for the host. +func (h *host) newClient() (cr.RunClient, error) { + return h.client(), nil +} + +// left is what is left on the host: each container's state, and the +// networks. +func (h *host) left() (map[string]container.ContainerState, []string) { + cs := make(map[string]container.ContainerState, len(h.containers)) + for _, c := range h.containers { + cs[c.ID] = c.State + } + + ns := make([]string, 0, len(h.networks)) + for _, n := range h.networks { + ns = append(ns, n.Name) + } + + return cs, ns +} + +// named reports whether container c is named name. +func named(c container.Summary, name string) bool { + return slices.Contains(c.Names, "/"+name) +} + +// container returns the index of the container with ID or name id. +func (h *host) container(id string) (int, error) { + i := slices.IndexFunc(h.containers, func(c container.Summary) bool { return c.ID == id || named(c, id) }) + + switch { + case i < 0: + return i, errors.Wrapf(errdefs.ErrNotFound, "no such container: %s", id) + case h.vanished[id]: + h.containers = slices.Delete(h.containers, i, i+1) + return -1, errors.Wrapf(errdefs.ErrNotFound, "no such container: %s", id) + } + + return i, nil +} + +func (h *host) containerInspect(_ context.Context, id string, _ client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + i, err := h.container(id) + if err != nil { + return client.ContainerInspectResult{}, err + } + + c := h.containers[i] + + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: c.ID, + Name: c.Names[0], + State: &container.State{Running: c.State == running}, + Config: &container.Config{Labels: c.Labels}, + NetworkSettings: &container.NetworkSettings{Networks: c.NetworkSettings.Networks}, + }}, nil +} + +func (h *host) containerCreate(_ context.Context, o client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + if h.raced != nil { + h.containers = append(h.containers, *h.raced) + h.raced = nil + } + + if slices.ContainsFunc(h.containers, func(c container.Summary) bool { return named(c, o.Name) }) { + return client.ContainerCreateResult{}, errors.Wrapf(errdefs.ErrConflict, "container name %q is already in use", o.Name) + } + + var nets []string + if o.NetworkingConfig != nil { + nets = slices.Collect(maps.Keys(o.NetworkingConfig.EndpointsConfig)) + } + + c := ctr("new-"+o.Name, container.StateCreated, o.Config.Labels, nets...) + c.Names = []string{"/" + o.Name} + h.containers = append(h.containers, c) + + return client.ContainerCreateResult{ID: c.ID}, nil +} + +func (h *host) containerStart(_ context.Context, id string, _ client.ContainerStartOptions) (client.ContainerStartResult, error) { + i, err := h.container(id) + if err != nil { + return client.ContainerStartResult{}, err + } + + h.containers[i].State = running + + return client.ContainerStartResult{}, nil +} + +func (h *host) containerList(_ context.Context, opts client.ContainerListOptions) (client.ContainerListResult, error) { + var res client.ContainerListResult + + for _, c := range h.containers { + ok, err := matches(opts.Filters, c.Labels) + if err != nil { + return client.ContainerListResult{}, err + } + + if ok && (opts.All || c.State == running) { + res.Items = append(res.Items, c) + } + } + + return res, nil +} + +func (h *host) containerStop(_ context.Context, id string, _ client.ContainerStopOptions) (client.ContainerStopResult, error) { + i, err := h.container(id) + if err != nil { + return client.ContainerStopResult{}, err + } + + h.containers[i].State = exited + + return client.ContainerStopResult{}, nil +} + +func (h *host) containerRemove(_ context.Context, id string, opts client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + if id == h.cannotRemove { + return client.ContainerRemoveResult{}, errors.New("daemon says no") + } + + i, err := h.container(id) + if err != nil { + return client.ContainerRemoveResult{}, err + } + + if h.containers[i].State == running && !opts.Force { + return client.ContainerRemoveResult{}, errors.Wrapf(errdefs.ErrConflict, "container %s is running: stop it or force the removal", id) + } + + h.containers = slices.Delete(h.containers, i, i+1) + + return client.ContainerRemoveResult{}, nil +} + +func (h *host) networkList(_ context.Context, opts client.NetworkListOptions) (client.NetworkListResult, error) { + var res client.NetworkListResult + + for _, n := range h.networks { + ok, err := matches(opts.Filters, n.Labels) + if err != nil { + return client.NetworkListResult{}, err + } + + if ok { + res.Items = append(res.Items, n) + } + } + + return res, nil +} + +func (h *host) networkRemove(_ context.Context, id string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + i := slices.IndexFunc(h.networks, func(n network.Summary) bool { return n.ID == id || n.Name == id }) + if i < 0 { + return client.NetworkRemoveResult{}, errors.Wrapf(errdefs.ErrNotFound, "network %s not found", id) + } + + n := h.networks[i] + + switch { + case h.vanished[n.ID]: + h.networks = slices.Delete(h.networks, i, i+1) + return client.NetworkRemoveResult{}, errors.Wrapf(errdefs.ErrNotFound, "network %s not found", id) + case slices.ContainsFunc(h.containers, func(c container.Summary) bool { + _, attached := c.NetworkSettings.Networks[n.Name] + return attached && c.State == running + }): + return client.NetworkRemoveResult{}, errors.Wrapf(errdefs.ErrConflict, "error while removing network: network %s has active endpoints", n.Name) + } + + h.networks = slices.Delete(h.networks, i, i+1) + + return client.NetworkRemoveResult{}, nil +} + +// matches reports whether labels satisfy filters as the daemon's label filter +// does. It fails a list that uses any other filter term rather than ignore it. +func matches(filters client.Filters, labels map[string]string) (bool, error) { + for term, values := range filters { + if term != "label" { + return false, errors.Errorf("fake Docker daemon does not support the %q list filter", term) + } + + for v := range values { + k, want, hasValue := strings.Cut(v, "=") + if got, ok := labels[k]; !ok || (hasValue && got != want) { + return false, nil + } + } + } + + return true, nil +} + +// ctr is a container named id. +func ctr(id string, state container.ContainerState, labels map[string]string, networks ...string) container.Summary { + c := container.Summary{ID: id, Names: []string{"/" + id}, State: state, Labels: labels, NetworkSettings: &container.NetworkSettingsSummary{ + Networks: map[string]*network.EndpointSettings{}, + }} + for _, n := range networks { + c.NetworkSettings.Networks[n] = &network.EndpointSettings{} + } + + return c +} + +func net(name string, labels map[string]string) network.Summary { + return network.Summary{Network: network.Network{ID: name, Name: name, Labels: labels}} +} + +// hostContainers is a Docker host shared by three dead runs, a live run, this +// run, and things no run owns. Each run's render-domain containers are +// attached to its network. +func hostContainers() []container.Summary { + return []container.Summary{ + // The dead run: a stopped Remove function, the render container that + // was running when it died (no cleanup label), a running Stop + // function, and a running Remove diff server (simulate domain). + ctr("dead-fn", exited, cr.ContainerLabels(cr.DomainRender, deadID, cr.CleanupRemove), "dead-net"), + ctr("dead-render", running, cr.ManagedLabels(cr.DomainRender, deadID), "dead-net"), + ctr("dead-stop", running, cr.ContainerLabels(cr.DomainRender, deadID, cr.CleanupStop), "dead-net"), + ctr("dead-server", running, cr.ContainerLabels(cr.DomainSimulate, deadID, cr.CleanupRemove)), + + // A second dead run. + ctr("dead2-fn", exited, cr.ContainerLabels(cr.DomainRender, dead2ID, cr.CleanupRemove), "dead2-net"), + + // A dead run whose user asked to keep a function and a diff server. + ctr("orphan-fn", running, cr.ContainerLabels(cr.DomainRender, orphanID, cr.CleanupOrphan), "orphan-net"), + ctr("orphan-server", running, cr.ContainerLabels(cr.DomainSimulate, orphanID, cr.CleanupOrphan)), + + // A live run, and this run. + ctr("live-fn", running, cr.ContainerLabels(cr.DomainRender, liveID, cr.CleanupRemove), "live-net"), + ctr("self-fn", running, cr.ContainerLabels(cr.DomainRender, selfID, cr.CleanupRemove), "self-net"), + + // Not ours to judge: no labels, the dead run's session without + // crossplane's management, its labels split across our two domains, + // and its labels under a domain that isn't ours. + ctr("unlabelled", running, nil), + ctr("session-only", running, map[string]string{cr.DomainRender + "/session": deadID}), + ctr("mixed-domains", running, map[string]string{ + cr.DomainRender + "/managed-by": cr.LabelValueManagedBy, + cr.DomainSimulate + "/session": deadID, + }), + ctr("other-domain", running, cr.ManagedLabels("example.org", deadID)), + } +} + +func hostNetworks() []network.Summary { + return []network.Summary{ + net("dead-net", cr.ManagedLabels(cr.DomainRender, deadID)), + net("dead2-net", cr.ManagedLabels(cr.DomainRender, dead2ID)), + net("orphan-net", cr.ManagedLabels(cr.DomainRender, orphanID)), + net("live-net", cr.ManagedLabels(cr.DomainRender, liveID)), + net("self-net", cr.ManagedLabels(cr.DomainRender, selfID)), + + // Not ours to judge: one the user supplied with + // CROSSPLANE_DOCKER_NETWORK (never labelled), and one with the dead + // run's session but not crossplane's management. + net("user-net", nil), + net("session-only-net", map[string]string{cr.DomainRender + "/session": deadID}), + } +} + +// untouched is what reaping dead runs never changes. +var ( + untouchedContainers = map[string]container.ContainerState{ + "orphan-fn": running, "orphan-server": running, + "live-fn": running, "self-fn": running, + "unlabelled": running, "session-only": running, "mixed-domains": running, "other-domain": running, + } + untouchedNetworks = []string{"live-net", "self-net", "user-net", "session-only-net"} + + // deadLeft and dead2Left are what each dead run leaves while unreaped. + deadLeft = map[string]container.ContainerState{ + "dead-fn": exited, "dead-render": running, "dead-stop": running, "dead-server": running, + } + dead2Left = map[string]container.ContainerState{"dead2-fn": exited} + + // deadReaped is what the dead run leaves once reaped: its Stop function, + // stopped. + deadReaped = map[string]container.ContainerState{"dead-stop": exited} +) + +func union[K comparable, V any](ms ...map[K]V) map[K]V { + out := map[K]V{} + for _, m := range ms { + maps.Copy(out, m) + } + + return out +} + +// startRun acquires a run the way a live process does, holding its lease +// until the test ends. +func startRun(t *testing.T, dir, id string, h *host) *cr.Run { + t.Helper() + + r := cr.NewRun(dir, id, true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + _ = r.Acquire(t.Context()) + + t.Cleanup(func() { _ = r.Release(context.Background(), false, "") }) + + return r +} + +// leaveLease leaves the lease file of a run that died: it exists, but no +// process holds its lock. +func leaveLease(t *testing.T, dir, id string) { + t.Helper() + + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(filepath.Join(dir, id+".lock"), nil, 0o600); err != nil { + t.Fatal(err) + } +} + +func dirEntries(t *testing.T, dir string) []string { + t.Helper() + + entries, err := os.ReadDir(dir) + if err != nil && !os.IsNotExist(err) { + t.Fatalf("ReadDir(%q): %v", dir, err) + } + + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + + return names +} + +var unordered = []cmp.Option{ + cmpopts.SortSlices(func(a, b string) bool { return a < b }), + cmpopts.EquateEmpty(), +} + +func skipWithoutLeases(t *testing.T) { + t.Helper() + + if runtime.GOOS == "windows" { + t.Skip("run leases are not supported on Windows") + } +} + +func TestNewSessionID(t *testing.T) { + format := regexp.MustCompile(`^[0-9a-f]{16}$`) + + a, b := cr.NewSessionID(), cr.NewSessionID() + for _, id := range []string{a, b} { + if !format.MatchString(id) { + t.Errorf("NewSessionID() = %q, want 16 lowercase hex characters: reapers skip any other lease name", id) + } + } + + if a == b { + t.Errorf("NewSessionID() returned %q twice", a) + } +} + +func TestRunAcquire(t *testing.T) { + skipWithoutLeases(t) + + type want struct { + Containers map[string]container.ContainerState + Networks []string + Files []string + Err bool + } + + tests := map[string]struct { + reason string + + dead []string // runs that died, leaving their lease behind + live []string // runs that are still running + otherFiles []string // files in the lease directory that are not leases + host host // resources to add to hostContainers and hostNetworks, and failures + + want want + }{ + "NoDeadRunRemovesNothing": { + reason: "With no dead run to reclaim, acquiring a run changes nothing.", + live: []string{liveID}, + want: want{ + Containers: union(untouchedContainers, deadLeft, dead2Left), + Networks: append(slices.Clone(untouchedNetworks), "dead-net", "dead2-net", "orphan-net"), + Files: []string{liveID + ".lock", selfID + ".lock"}, + }, + }, + "ReapsOnlyTheDeadRunsResourcesPerPolicy": { + reason: "Every dead run's Remove and unlabelled containers, in both domains, are removed, its Stop one " + + "is stopped, then its network and lease go. A live run's, this run's, and anything without both " + + "of one domain's labels for a dead run are left alone.", + dead: []string{deadID, dead2ID}, + live: []string{liveID}, + want: want{ + Containers: union(untouchedContainers, deadReaped), + Networks: append(slices.Clone(untouchedNetworks), "orphan-net"), + Files: []string{liveID + ".lock", selfID + ".lock"}, + }, + }, + "OrphanIsKeptWithItsNetwork": { + reason: "A container the user asked to keep is left running, and its network with it. That counts as " + + "reclaimed, so the lease is dropped.", + dead: []string{orphanID}, + want: want{ + Containers: union(untouchedContainers, deadLeft, dead2Left), + Networks: append(slices.Clone(untouchedNetworks), "dead-net", "dead2-net", "orphan-net"), + Files: []string{selfID + ".lock"}, + }, + }, + "StoppedOrphanKeepsItsNetwork": { + reason: "Docker would remove the network of a stopped Orphan container, which then could not start " + + "again, so the network is left with it, and the lease is dropped.", + dead: []string{futureID}, + host: host{ + containers: []container.Summary{ + ctr("stopped-orphan", exited, cr.ContainerLabels(cr.DomainRender, futureID, cr.CleanupOrphan), "future-net"), + }, + networks: []network.Summary{net("future-net", cr.ManagedLabels(cr.DomainRender, futureID))}, + }, + want: want{ + Containers: union(untouchedContainers, deadLeft, dead2Left, map[string]container.ContainerState{"stopped-orphan": exited}), + Networks: append(slices.Clone(untouchedNetworks), "dead-net", "dead2-net", "orphan-net", "future-net"), + Files: []string{selfID + ".lock"}, + }, + }, + "UnknownCleanupPolicyIsKept": { + reason: "A container whose cleanup label holds a value this version does not know may have been asked " + + "to be kept, so it is left alone like Orphan, with its network, and the lease is dropped.", + dead: []string{futureID}, + host: host{ + containers: []container.Summary{ + ctr("future-fn", running, cr.ContainerLabels(cr.DomainRender, futureID, "orphan"), "future-net"), + }, + networks: []network.Summary{net("future-net", cr.ManagedLabels(cr.DomainRender, futureID))}, + }, + want: want{ + Containers: union(untouchedContainers, deadLeft, dead2Left, map[string]container.ContainerState{"future-fn": running}), + Networks: append(slices.Clone(untouchedNetworks), "dead-net", "dead2-net", "orphan-net", "future-net"), + Files: []string{selfID + ".lock"}, + }, + }, + "ContainerRemovalFailureKeepsNetworkAndLease": { + reason: "A container that could not be removed may still hold its network, and the next run should " + + "retry, so the network and lease are kept and Acquire reports it.", + dead: []string{deadID}, + host: host{cannotRemove: "dead-render"}, + want: want{ + Containers: union(untouchedContainers, deadReaped, map[string]container.ContainerState{"dead-render": running}, dead2Left), + Networks: append(slices.Clone(untouchedNetworks), "dead-net", "dead2-net", "orphan-net"), + Files: []string{deadID + ".lock", selfID + ".lock"}, + Err: true, + }, + }, + "ResourcesAlreadyGoneCountAsReaped": { + reason: "A container or network that no longer exists by the time it is removed, e.g. removed by a " + + "concurrent reaper, is already reclaimed.", + dead: []string{deadID}, + host: host{vanished: map[string]bool{"dead-fn": true, "dead-stop": true, "dead-net": true}}, + want: want{ + Containers: union(untouchedContainers, dead2Left), + Networks: append(slices.Clone(untouchedNetworks), "dead2-net", "orphan-net"), + Files: []string{selfID + ".lock"}, + }, + }, + "IgnoresFilesThatAreNotLeases": { + reason: "Only <16 lowercase hex>.lock files are leases; anything else in the directory is never acted on.", + otherFiles: []string{ + "notes.txt", + "not-a-run-id.lock", + strings.ToUpper(deadID) + ".lock", + deadID + "-123.tmp", + }, + want: want{ + Containers: union(untouchedContainers, deadLeft, dead2Left), + Networks: append(slices.Clone(untouchedNetworks), "dead-net", "dead2-net", "orphan-net"), + Files: []string{ + deadID + "-123.tmp", strings.ToUpper(deadID) + ".lock", "not-a-run-id.lock", "notes.txt", + selfID + ".lock", + }, + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + + h := tt.host + h.containers = append(hostContainers(), tt.host.containers...) + h.networks = append(hostNetworks(), tt.host.networks...) + + // Live runs started before any run died, so had nothing to reclaim. + for _, id := range tt.live { + startRun(t, dir, id, &h) + } + + for _, id := range tt.dead { + leaveLease(t, dir, id) + } + + for _, f := range tt.otherFiles { + if err := os.WriteFile(filepath.Join(dir, f), nil, 0o600); err != nil { + t.Fatal(err) + } + } + + r := cr.NewRun(dir, selfID, true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + err := r.Acquire(t.Context()) + t.Cleanup(func() { _ = r.Release(context.Background(), false, "") }) + + got := want{Files: dirEntries(t, dir), Err: err != nil} + got.Containers, got.Networks = h.left() + + if diff := cmp.Diff(tt.want, got, unordered...); diff != "" { + t.Errorf("\n%s\nAcquire(...): -want, +got:\n%s", tt.reason, diff) + } + }) + } +} + +// TestRunRemovesStaleTempLeases checks that a reaper removes the temporary +// lease file a run that died while taking its lease left, once it is old +// enough that no live run can still be renaming it, and nothing else. +func TestRunRemovesStaleTempLeases(t *testing.T) { + skipWithoutLeases(t) + + dir := t.TempDir() + stale := time.Now().Add(-time.Hour) + + files := map[string]bool{ // name -> stale + deadID + "-123.tmp": true, + dead2ID + "-456.tmp": false, + "notes-123.tmp": true, + } + for f, old := range files { + p := filepath.Join(dir, f) + if err := os.WriteFile(p, nil, 0o600); err != nil { + t.Fatal(err) + } + if old { + if err := os.Chtimes(p, stale, stale); err != nil { + t.Fatal(err) + } + } + } + + h := &host{} + startRun(t, dir, selfID, h) + + want := []string{dead2ID + "-456.tmp", "notes-123.tmp", selfID + ".lock"} + if diff := cmp.Diff(want, dirEntries(t, dir), unordered...); diff != "" { + t.Errorf("Acquire(...): lease directory, -want, +got:\n%s", diff) + } +} + +// TestRunConcurrentReapersOfOneDeadRun checks that two runs reaping one dead +// run at once both succeed, leaving the dead run reclaimed once, by one of +// them, and its lease gone. A flock conflicts per open file, so two goroutines contend for +// a lease as two processes do. Repeated, since the interleaving varies. +func TestRunConcurrentReapersOfOneDeadRun(t *testing.T) { + skipWithoutLeases(t) + + for range 50 { + dir := t.TempDir() + leaveLease(t, dir, deadID) + + h := &host{ + containers: []container.Summary{ + ctr("dead-fn", running, cr.ContainerLabels(cr.DomainRender, deadID, cr.CleanupRemove), "dead-net"), + ctr("dead-stop", running, cr.ContainerLabels(cr.DomainRender, deadID, cr.CleanupStop), "dead-net"), + ctr("dead-server", running, cr.ContainerLabels(cr.DomainSimulate, deadID, cr.CleanupRemove)), + }, + networks: []network.Summary{net("dead-net", cr.ManagedLabels(cr.DomainRender, deadID))}, + } + var mu sync.Mutex + finished := map[string]int{} + newClient := func() (cr.RunClient, error) { return serialized(&mu, h.client(), finished), nil } + + ids := []string{cr.NewSessionID(), cr.NewSessionID()} + errs := make([]error, len(ids)) + var wg sync.WaitGroup + for i, id := range ids { + r := cr.NewRun(dir, id, true, logging.NewNopLogger(), cr.WithRunClient(newClient)) + t.Cleanup(func() { _ = r.Release(context.Background(), false, "") }) + wg.Go(func() { errs[i] = r.Acquire(t.Context()) }) + } + wg.Wait() + + type state struct { + Errs []error + Containers map[string]container.ContainerState + Networks []string + Files []string + Finished map[string]int + } + got := state{Errs: errs, Files: dirEntries(t, dir), Finished: finished} + got.Containers, got.Networks = h.left() + want := state{ + Errs: []error{nil, nil}, + Containers: map[string]container.ContainerState{"dead-stop": exited}, + Files: []string{ids[0] + ".lock", ids[1] + ".lock"}, + // Only one reaper takes the dead run's lease and reclaims it. + Finished: map[string]int{"dead-fn": 1, "dead-stop": 1, "dead-server": 1, "dead-net": 1}, + } + if diff := cmp.Diff(want, got, append(unordered, cmpopts.EquateErrors())...); diff != "" { + t.Fatalf("two concurrent Acquire(...): -want, +got:\n%s", diff) + } + } +} + +// serialized makes a fake client's calls run one at a time under mu, for a +// host shared by concurrent runs, and counts in finished each container or +// network a stop or remove succeeded on. +func serialized(mu *sync.Mutex, c *fake.Client, finished map[string]int) *fake.Client { + count := func(id string, err error) error { + if err == nil { + finished[id]++ + } + return err + } + + return &fake.Client{ + MockContainerList: func(ctx context.Context, o client.ContainerListOptions) (client.ContainerListResult, error) { + mu.Lock() + defer mu.Unlock() + return c.MockContainerList(ctx, o) + }, + MockContainerStop: func(ctx context.Context, id string, o client.ContainerStopOptions) (client.ContainerStopResult, error) { + mu.Lock() + defer mu.Unlock() + res, err := c.MockContainerStop(ctx, id, o) + return res, count(id, err) + }, + MockContainerRemove: func(ctx context.Context, id string, o client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + mu.Lock() + defer mu.Unlock() + res, err := c.MockContainerRemove(ctx, id, o) + return res, count(id, err) + }, + MockNetworkList: func(ctx context.Context, o client.NetworkListOptions) (client.NetworkListResult, error) { + mu.Lock() + defer mu.Unlock() + return c.MockNetworkList(ctx, o) + }, + MockNetworkRemove: func(ctx context.Context, id string, o client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + mu.Lock() + defer mu.Unlock() + res, err := c.MockNetworkRemove(ctx, id, o) + return res, count(id, err) + }, + } +} + +func TestRunWithoutLease(t *testing.T) { + skipWithoutLeases(t) + + t.Run("SuppliedIDIsNeverLeased", func(t *testing.T) { + dir := t.TempDir() + h := &host{containers: hostContainers(), networks: hostNetworks()} + leaveLease(t, dir, deadID) + + // A supplied ID that happens to be valid hex is still not leased. + r := cr.NewRun(dir, selfID, false, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + if err := r.Acquire(t.Context()); err != nil { + t.Fatalf("Acquire(...): unexpected error: %v", err) + } + + if got := dirEntries(t, dir); len(got) != 0 { + t.Errorf("Acquire(...) of a supplied ID: lease directory holds %v, want nothing (no lease of its own, dead lease reaped)", got) + } + + if err := r.Release(t.Context(), true, ""); err != nil { + t.Errorf("Release(...): unexpected error: %v", err) + } + }) + + t.Run("NoLeaseDirDisablesLeaseAndReap", func(t *testing.T) { + // A dead run's lease in the working directory, which an empty lease + // directory would resolve to, is left alone, along with its resources. + dir := t.TempDir() + t.Chdir(dir) + leaveLease(t, dir, deadID) + h := &host{containers: hostContainers(), networks: hostNetworks()} + + r := cr.NewRun("", cr.NewSessionID(), true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + if err := r.Acquire(t.Context()); err != nil { + t.Fatalf("Acquire(...): unexpected error: %v", err) + } + + if err := r.Release(t.Context(), true, ""); err != nil { + t.Errorf("Release(...): unexpected error: %v", err) + } + + if got := dirEntries(t, dir); !slices.Equal(got, []string{deadID + ".lock"}) { + t.Errorf("Acquire/Release(...) without a lease dir: working directory holds %v, want only the dead lease", got) + } + + if got, _ := h.left(); got["dead-fn"] != exited { + t.Errorf("Acquire/Release(...) without a lease dir: reaped a dead run's container, left %v", got) + } + }) +} + +func TestRunRelease(t *testing.T) { + skipWithoutLeases(t) + + type want struct { + Networks []string + Files []string + Err bool + } + + tests := map[string]struct { + reason string + clean bool + sweep string + host host + want want + }{ + "CleanDropsLease": { + reason: "A run that removed everything drops its lease file, and leaves networks alone without a sweep.", + clean: true, + want: want{ + Networks: []string{"live-net", "self-net", "self-net-2", "self-sim-net"}, + }, + }, + "NotCleanKeepsLease": { + reason: "A run that left something behind keeps its lease file, unlocked, so the next run reaps it.", + want: want{ + Networks: []string{"live-net", "self-net", "self-net-2", "self-sim-net"}, + Files: []string{selfID + ".lock"}, + }, + }, + "SweepRemovesOnlyThisRunsNetworksInItsDomain": { + reason: "A sweep removes this run's networks in the domain it is given, and no other run's or domain's.", + clean: true, + sweep: cr.DomainRender, + want: want{ + Networks: []string{"live-net", "self-sim-net"}, + }, + }, + "SweepExcusesNetworkOfKeptContainer": { + reason: "A network that this run's Orphan or unknown-policy container is attached to, running or stopped, is left, and does not keep the lease.", + clean: true, + sweep: cr.DomainRender, + host: host{containers: []container.Summary{ + ctr("orphan", exited, cr.ContainerLabels(cr.DomainRender, selfID, cr.CleanupOrphan), "self-net"), + ctr("future", running, cr.ContainerLabels(cr.DomainRender, selfID, cr.Cleanup("Future")), "self-net-2"), + }}, + want: want{ + Networks: []string{"live-net", "self-net", "self-net-2", "self-sim-net"}, + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + + h := tt.host + h.networks = []network.Summary{ + net("self-net", cr.ManagedLabels(cr.DomainRender, selfID)), + net("self-net-2", cr.ManagedLabels(cr.DomainRender, selfID)), + net("self-sim-net", cr.ManagedLabels(cr.DomainSimulate, selfID)), + net("live-net", cr.ManagedLabels(cr.DomainRender, liveID)), + } + + r := cr.NewRun(dir, selfID, true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + if err := r.Acquire(t.Context()); err != nil { + t.Fatalf("Acquire(...): unexpected error: %v", err) + } + + err := r.Release(t.Context(), tt.clean, tt.sweep) + + _, nets := h.left() + got := want{Networks: nets, Files: dirEntries(t, dir), Err: err != nil} + + if diff := cmp.Diff(tt.want, got, unordered...); diff != "" { + t.Errorf("\n%s\nRelease(...): -want, +got:\n%s", tt.reason, diff) + } + }) + } +} + +// TestRunReclaimsARunThatDied pins reaping end to end, with generated IDs and +// the leases runs really take: a later run leaves a run's resources alone +// while that run is alive, and removes them once its lock is gone. +func TestRunReclaimsARunThatDied(t *testing.T) { + skipWithoutLeases(t) + + type left struct { + Containers map[string]container.ContainerState + Networks []string + } + + dir := t.TempDir() + h := &host{} + + first := cr.NewRun(dir, cr.NewSessionID(), true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + if err := first.Acquire(t.Context()); err != nil { + t.Fatalf("first Acquire(...): unexpected error: %v", err) + } + + h.containers = []container.Summary{ctr("fn", running, cr.ContainerLabels(cr.DomainRender, first.ID(), cr.CleanupRemove), "net")} + h.networks = []network.Summary{net("net", cr.ManagedLabels(cr.DomainRender, first.ID()))} + + startRun(t, dir, cr.NewSessionID(), h) + + got := map[string]left{} + got["WhileAlive"] = func() left { c, n := h.left(); return left{c, n} }() + + // The first run dies: its lock is dropped and its lease file stays. + _ = first.Release(t.Context(), false, "") + + startRun(t, dir, cr.NewSessionID(), h) + + got["AfterItDied"] = func() left { c, n := h.left(); return left{c, n} }() + + want := map[string]left{ + "WhileAlive": {Containers: map[string]container.ContainerState{"fn": running}, Networks: []string{"net"}}, + "AfterItDied": {}, + } + if diff := cmp.Diff(want, got, unordered...); diff != "" { + t.Errorf("what later runs left of a run, -want, +got:\n%s", diff) + } +} + +func TestRunReclaimsInAGlobLikeDir(t *testing.T) { + skipWithoutLeases(t) + + type left struct { + Containers map[string]container.ContainerState + Networks []string + } + + // Glob syntax in the lease dir must not hide its leases. + dir := filepath.Join(t.TempDir(), "[x]", "runs") + h := &host{} + + dead := cr.NewRun(dir, cr.NewSessionID(), true, logging.NewNopLogger(), cr.WithRunClient(h.newClient)) + if err := dead.Acquire(t.Context()); err != nil { + t.Fatalf("Acquire(...): unexpected error: %v", err) + } + + h.containers = []container.Summary{ctr("fn", running, cr.ContainerLabels(cr.DomainRender, dead.ID(), cr.CleanupRemove), "net")} + h.networks = []network.Summary{net("net", cr.ManagedLabels(cr.DomainRender, dead.ID()))} + + _ = dead.Release(t.Context(), false, "") + + startRun(t, dir, cr.NewSessionID(), h) + + c, n := h.left() + if diff := cmp.Diff(left{}, left{c, n}, unordered...); diff != "" { + t.Errorf("what a later run left of a dead run, -want, +got:\n%s", diff) + } +} From e26a27f00075aebe78f577f87a38823ca914abfc Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 3 Nov 2026 17:17:17 -0500 Subject: [PATCH 3/5] feat(render): add Session, a run-scoped render API Session ties a render run to a containerruntime Run: it reclaims what dead runs left, starts function runtimes and the Crossplane engine under the run's session ID, and on Close removes them and releases the lease. A Session does all its Docker work through one client, which callers can supply, and refuses Render after Close. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- internal/docker/docker.go | 19 +- pkg/render/engine_docker.go | 25 +- pkg/render/engine_docker_test.go | 38 +++ pkg/render/session.go | 382 +++++++++++++++++++++ pkg/render/session_test.go | 570 +++++++++++++++++++++++++++++++ 5 files changed, 1025 insertions(+), 9 deletions(-) create mode 100644 pkg/render/session.go diff --git a/internal/docker/docker.go b/internal/docker/docker.go index 9ca70b70..c92df184 100644 --- a/internal/docker/docker.go +++ b/internal/docker/docker.go @@ -336,6 +336,15 @@ type runContainerConfig struct { hostConfig *container.HostConfig networkConfig *network.NetworkingConfig stdin []byte + cli client.APIClient +} + +// RunWithClient makes RunContainer talk to Docker through cli, which it never +// closes, instead of a client built from the environment. +func RunWithClient(cli client.APIClient) RunContainerOption { + return func(cfg *runContainerConfig) { + cfg.cli = cli + } } // RunWithCommand sets the command to run in the container. @@ -433,9 +442,13 @@ func RunContainer(ctx context.Context, img string, opts ...RunContainerOption) ( opt(cfg) } - cli, err := NewClient() - if err != nil { - return nil, nil, err + cli := cfg.cli + if cli == nil { + c, err := NewClient() + if err != nil { + return nil, nil, err + } + cli = c } // Pull the image if it's not already present. diff --git a/pkg/render/engine_docker.go b/pkg/render/engine_docker.go index 02e8d8ba..27f40673 100644 --- a/pkg/render/engine_docker.go +++ b/pkg/render/engine_docker.go @@ -23,6 +23,7 @@ import ( "strings" "time" + "github.com/moby/moby/client" "google.golang.org/protobuf/proto" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" @@ -73,18 +74,25 @@ type dockerRenderEngine struct { 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. + // When nil, Setup builds a client from the environment, only when it + // needs to create a network. A Session sets it from WithDockerClient. networks networkClient + + // dockerClient, when set, is the Docker client the render container is + // run through, and the daemon CheckContextSupport checks, instead of the + // environment's. A Session sets it, and networks, from WithDockerClient. + dockerClient client.APIClient } func (e *dockerRenderEngine) CheckContextSupport() error { if runtime.GOOS == "windows" { return errors.New("context handling via --context-values/--context-files/--include-context is not supported on Windows") } - if host := os.Getenv("DOCKER_HOST"); host != "" && !strings.HasPrefix(host, "unix://") { + host := os.Getenv("DOCKER_HOST") + if e.dockerClient != nil { + host = e.dockerClient.DaemonHost() + } + if host != "" && !strings.HasPrefix(host, "unix://") { return errors.New("context handling via --context-values/--context-files/--include-context requires a local Docker daemon or Crossplane controller binary") } @@ -156,7 +164,9 @@ func (e *dockerRenderEngine) Setup(ctx context.Context, fns []pkgv1.Function) (f // partial response AND a non-nil error containing stderr — letting callers // recover the partial output (e.g. RequiredResources) and iterate. func (e *dockerRenderEngine) Render(ctx context.Context, req *renderv1alpha1.RenderRequest) (*renderv1alpha1.RenderResponse, error) { - // Update any localhost function addresses if needed. + // Update any localhost function addresses if needed, on a copy: the + // caller's request may be rendering concurrently. + req = proto.CloneOf(req) if cinput := req.GetComposite(); cinput != nil { cinput.Functions = RewriteAddressesForDocker(cinput.GetFunctions()) } @@ -180,6 +190,9 @@ func (e *dockerRenderEngine) Render(ctx context.Context, req *renderv1alpha1.Ren if e.network != "" { opts = append(opts, docker.RunWithNetworkName(e.network)) } + if e.dockerClient != nil { + opts = append(opts, docker.RunWithClient(e.dockerClient)) + } e.log.Debug("Running crossplane internal render in Docker", "image", e.image, "network", e.network) diff --git a/pkg/render/engine_docker_test.go b/pkg/render/engine_docker_test.go index 571015c0..bb8e211b 100644 --- a/pkg/render/engine_docker_test.go +++ b/pkg/render/engine_docker_test.go @@ -248,6 +248,44 @@ func TestDockerRenderEngineRender(t *testing.T) { } } +// TestDockerRenderEngineRenderLeavesRequest checks that Render rewrites +// function addresses on its own copy, so concurrent Renders of one request +// don't race on it. +func TestDockerRenderEngineRenderLeavesRequest(t *testing.T) { + fns := func() []*renderv1alpha1.FunctionInput { + return []*renderv1alpha1.FunctionInput{{Name: "fn", Address: "localhost:9443"}} + } + reqs := map[string]func() *renderv1alpha1.RenderRequest{ + "Composite": func() *renderv1alpha1.RenderRequest { + return &renderv1alpha1.RenderRequest{Input: &renderv1alpha1.RenderRequest_Composite{Composite: &renderv1alpha1.CompositeInput{Functions: fns()}}} + }, + "Operation": func() *renderv1alpha1.RenderRequest { + return &renderv1alpha1.RenderRequest{Input: &renderv1alpha1.RenderRequest_Operation{Operation: &renderv1alpha1.OperationInput{Functions: fns()}}} + }, + } + + for name, newReq := range reqs { + t.Run(name, func(t *testing.T) { + e := &dockerRenderEngine{ + image: "test-image", + log: logging.NewNopLogger(), + runner: &mockContainerRunner{MockRun: func(context.Context, string, ...docker.RunContainerOption) ([]byte, []byte, error) { + return nil, nil, nil + }}, + } + + req := newReq() + if _, err := e.Render(t.Context(), req); err != nil { + t.Fatalf("Render(...): unexpected error: %v", err) + } + + if diff := cmp.Diff(newReq(), req, protocmp.Transform()); diff != "" { + t.Errorf("Render(...): want the caller's request unchanged, -want, +got:\n%s", diff) + } + }) + } +} + func TestDockerRenderEngineSetup(t *testing.T) { // These cases all exercise the early-return branch of Setup — where // e.network is already non-empty, either because NewEngineFromFlags diff --git a/pkg/render/session.go b/pkg/render/session.go new file mode 100644 index 00000000..cfadf10c --- /dev/null +++ b/pkg/render/session.go @@ -0,0 +1,382 @@ +/* +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" + "maps" + "slices" + "sync" + "sync/atomic" + + "github.com/moby/moby/client" + + "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" + + "github.com/crossplane/cli/v2/internal/containerruntime" + renderv1alpha1 "github.com/crossplane/cli/v2/proto/render/v1alpha1" +) + +// errSessionClosed is returned by a Session used after Close. +var errSessionClosed = errors.New("render session is closed") + +// A Session owns everything one run of render creates: a render engine, the +// environment its Setup creates (such as a Docker network), every function +// runtime it starts, and the run's lease. It starts each function once and +// renders any number of times against them. +// +// Order: the first Start or Render takes the run's lease, reclaims what dead +// runs left behind, then runs the engine's Setup, all before any Render +// reaches the engine. Close stops every runtime, then runs the Setup +// cleanups, then releases the lease. +// +// Concurrency: Start and Render are safe for concurrent use. Starting is +// serialized, so concurrent Starts that need the same function start it once. +// Rendering is not: once the first Setup has run, Render calls the engine +// without waiting for any Start. Close must not run while a Render is in +// flight; a Start or Render that begins after Close returns an error. +// +// Only a Session with a generated ID is leased. A supplied ID may be shared +// with other processes, so its Session neither leases it nor, at Close, +// removes networks labelled with it. +type Session struct { + flags EngineFlags + log logging.Logger + engine Engine + run *containerruntime.Run + warn func(error) + + // sweep is whether Close removes networks left labelled with this + // session: only when the engine creates its own network and the session + // ID belongs to this run alone. + sweep bool + + leaseDir string + newEngine func(*EngineFlags, logging.Logger) Engine + // docker, if set, is the Docker client for everything the Session does + // on Docker. When nil, each piece builds its own from the environment. + docker client.APIClient + + // mu guards everything below, and serializes Setup and starting runtimes. + mu sync.Mutex + // addrs maps the name of every started function to its address. + addrs map[string]string + // runtimes holds every started runtime, so Close stops each of them. + runtimes []startedRuntime + cleanups []func() + // leaked is whether a failed Start may have left a Docker container + // behind, so Close keeps the lease for a later run to reclaim. + leaked bool + + // closed is set, under mu, by Close. Render reads it without mu. + closed atomic.Bool + + // setUp is set, under mu, once the engine's first Setup succeeded. Render + // reads it without mu, which orders what Setup wrote before the render. + setUp atomic.Bool +} + +// A SessionOption configures a Session. +type SessionOption func(*Session) + +// WithLeaseDir makes the Session lease its run in dir and reclaim what dead +// runs leased there left behind. The default, "", disables both. A Session +// with a supplied ID still reclaims dead runs, but takes no lease. +func WithLeaseDir(dir string) SessionOption { + return func(s *Session) { + s.leaseDir = dir + } +} + +// WithEngineFactory makes the Session build its engine with f. The default +// is NewEngineFromFlags. +func WithEngineFactory(f func(*EngineFlags, logging.Logger) Engine) SessionOption { + return func(s *Session) { + s.newEngine = f + } +} + +// WithWarn makes the Session report lease and reclaim failures to f. They +// never stop the Session. The default logs them at Info. f is called with +// the Session's lock held, so f must not call the Session. +func WithWarn(f func(error)) SessionOption { + return func(s *Session) { + s.warn = f + } +} + +// WithDockerClient makes the Session do everything it does on Docker through +// c, instead of clients built from the environment: create and remove the +// render network, run the render container, start and stop every function +// container, and reclaim what dead runs left behind. The render network and +// container are the Docker engine's, as NewEngineFromFlags builds it; another +// engine from WithEngineFactory keeps its own clients: if it uses Docker, it +// must use c's daemon too, or its network and the function containers end up +// on different daemons. Leases don't record their daemon, so a Session can +// retire the lease of a dead run against another daemon that shares its lease +// directory. The Session never closes c; the caller does, after Close. A nil +// c is the default. +func WithDockerClient(c client.APIClient) SessionOption { + return func(s *Session) { + s.docker = c + } +} + +// NewSession returns a Session that renders with an engine built from a copy +// of f. When f.SessionID is empty, the Session generates an ID, and leases it +// only when WithLeaseDir is set; a supplied ID is used as is and never +// leased, since other processes may share it. NewSession does no I/O. +func NewSession(f EngineFlags, log logging.Logger, opts ...SessionOption) *Session { + if log == nil { + log = logging.NewNopLogger() + } + + s := &Session{ + flags: f, + log: log, + newEngine: NewEngineFromFlags, + addrs: make(map[string]string), + } + s.warn = func(err error) { + s.log.Info("Problem managing this run's Docker resources", "error", err) + } + + for _, o := range opts { + o(s) + } + + leased := s.flags.SessionID == "" + if leased { + s.flags.SessionID = containerruntime.NewSessionID() + } + + var runOpts []containerruntime.RunOption + if s.docker != nil { + runOpts = append(runOpts, containerruntime.WithRunClient(func() (containerruntime.RunClient, error) { + return sharedRunClient{s.docker}, nil + })) + } + s.run = containerruntime.NewRun(s.leaseDir, s.flags.SessionID, leased, log, runOpts...) + s.engine = s.newEngine(&s.flags, log) + + e, isDocker := s.engine.(*dockerRenderEngine) + if isDocker && s.docker != nil { + e.networks = s.docker + e.dockerClient = s.docker + } + s.sweep = isDocker && s.flags.CrossplaneDockerNetwork == "" && leased + + return s +} + +// ID is the session ID the Session labels its Docker resources with. +func (s *Session) ID() string { + return s.flags.SessionID +} + +// CheckContextSupport reports whether the Session's engine supports context +// injection and collection. +func (s *Session) CheckContextSupport() error { + return s.engine.CheckContextSupport() +} + +// Start ensures each of fns is running, starting the ones this Session has +// not started yet, and returns the addresses of fns only. Functions are +// identified by name. The first call, even with no fns, runs the engine's +// Setup. Start copies each function's annotations before Setup adds to them, +// so the caller's are untouched. +func (s *Session) Start(ctx context.Context, fns []pkgv1.Function) (map[string]string, error) { + s.mu.Lock() + defer s.mu.Unlock() + + if s.closed.Load() { + return nil, errSessionClosed + } + + // Functions sharing a name within fns all start, and the last one's + // address is used, as with StartFunctionRuntimes. + batch := make([]pkgv1.Function, 0, len(fns)) + for _, fn := range fns { + if _, ok := s.addrs[fn.GetName()]; ok { + continue + } + + // fn is a copy, but shares its annotation map with the caller's. + fn.SetAnnotations(maps.Clone(fn.GetAnnotations())) + batch = append(batch, fn) + } + + if err := s.setupLocked(ctx, batch); err != nil { + return nil, err + } + + if len(batch) > 0 { + docker := false + fa, err := startFunctionRuntimes(ctx, s.log, batch, func(fn pkgv1.Function, log logging.Logger) (Runtime, error) { + rt, err := s.getRuntime(fn, log) + if _, ok := rt.(*RuntimeDocker); ok { + docker = true + } + return rt, err + }) + if err != nil { + // A Docker start can fail after creating its container, which + // then is in no runtime Close stops. + s.leaked = s.leaked || docker + // The Setup cleanup is kept: Close still runs it. + return nil, errors.Wrap(err, "cannot start function runtimes") + } + + maps.Copy(s.addrs, fa.addrs) + s.runtimes = append(s.runtimes, fa.runtimes...) + } + + // Copied, because the caller reads them after mu is released. + out := make(map[string]string, len(fns)) + for _, fn := range fns { + if a, ok := s.addrs[fn.GetName()]; ok { + out[fn.GetName()] = a + } + } + + return out, nil +} + +// getRuntime gets fn's runtime as GetRuntime does, on the Session's Docker +// client when it has one. +func (s *Session) getRuntime(fn pkgv1.Function, log logging.Logger) (Runtime, error) { + rt, err := GetRuntime(fn, log) + if err != nil { + return nil, err + } + if rd, ok := rt.(*RuntimeDocker); ok && s.docker != nil { + rd.dockerClient = s.docker + } + return rt, nil +} + +// Render runs req through the Session's engine. It waits for the first +// Setup, running it if no Start has, then renders without holding the +// Session's lock. After Close it returns an error without rendering. See +// Engine.Render for the partial response it may return with an error. +func (s *Session) Render(ctx context.Context, req *renderv1alpha1.RenderRequest) (*renderv1alpha1.RenderResponse, error) { + if s.closed.Load() { + return nil, errSessionClosed + } + + if !s.setUp.Load() { + s.mu.Lock() + err := s.setupLocked(ctx, nil) + s.mu.Unlock() + + if err != nil { + return nil, err + } + } + + return s.engine.Render(ctx, req) +} + +// setupLocked takes the run's lease and reclaims dead runs, the first time, +// then integrates fns into the engine's environment. Once the first Setup +// succeeded, it does nothing for no fns. The caller holds mu. +func (s *Session) setupLocked(ctx context.Context, fns []pkgv1.Function) error { + if s.closed.Load() { + return errSessionClosed + } + + if s.setUp.Load() && len(fns) == 0 { + return nil + } + + // Idempotent: only the first call does anything. + if err := s.run.Acquire(ctx); err != nil { + s.warn(err) + } + + cleanup, err := s.engine.Setup(ctx, fns) + if err != nil { + // Returned as is, as before Sessions: the engine's errors say what + // failed. + return err + } + + s.cleanups = append(s.cleanups, cleanup) + s.setUp.Store(true) + + return nil +} + +// Close stops every runtime the Session started, each per its own cleanup +// policy and concurrently, then runs the engine's Setup cleanups in reverse +// order, then releases the run's lease. Teardown is detached from ctx's +// cancellation, so it can follow an interrupted render. The lease is kept, +// so a later run reclaims what is left, when a runtime cannot be stopped or +// a failed Start may have left a container. +// Close is idempotent; Start or Render after Close returns an error. Close +// must not run while a Render is in flight. +func (s *Session) Close(ctx context.Context) error { + s.mu.Lock() + defer s.mu.Unlock() + + if s.closed.Swap(true) { + return nil + } + + // No deadline here: a Docker runtime's Stop bounds each of its requests + // (containerruntime.StopTimeout, then RemoveTimeout), as the network + // cleanup and the sweep bound theirs, so a hung daemon cannot hold Close. + // The other runtimes' Stops do nothing. + ctx = context.WithoutCancel(ctx) + + errs := make([]error, len(s.runtimes)) + + var wg sync.WaitGroup + for i, rt := range s.runtimes { + wg.Go(func() { + if err := rt.Stop(ctx); err != nil { + errs[i] = errors.Wrapf(err, "cannot stop function %q runtime (target %q)", rt.name, rt.Target) + } + }) + } + wg.Wait() + + for _, c := range slices.Backward(s.cleanups) { + c() + } + + stopErr := errors.Join(errs...) + + // Without a Setup, the engine created no network to sweep. + sweep := "" + if s.sweep && s.setUp.Load() { + sweep = labelDomain + } + relErr := s.run.Release(ctx, stopErr == nil && !s.leaked, sweep) + + return errors.Join(stopErr, relErr) +} + +// sharedRunClient is the Session's Docker client as the Run uses it. It hides +// the client's Close, so the Run, which closes the clients it gets, leaves the +// caller's open. +type sharedRunClient struct { + containerruntime.RunClient +} diff --git a/pkg/render/session_test.go b/pkg/render/session_test.go index c766a407..8818286a 100644 --- a/pkg/render/session_test.go +++ b/pkg/render/session_test.go @@ -17,13 +17,32 @@ limitations under the License. package render import ( + "context" + "fmt" + "io" + "maps" + "net" + "os" + "path/filepath" + "runtime" + "slices" + "strings" + "sync" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" + "github.com/moby/moby/client" + "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" + + "github.com/crossplane/cli/v2/internal/containerruntime" + renderv1alpha1 "github.com/crossplane/cli/v2/proto/render/v1alpha1" ) func TestManagedLabelsSession(t *testing.T) { @@ -145,3 +164,554 @@ func TestGetRuntimeDockerSession(t *testing.T) { t.Errorf("GetRuntimeDocker(...): Session: want %q, got %q", "my-session", rd.Session) } } + +// recorder records events from concurrent goroutines. +type recorder struct { + mu sync.Mutex + events []string +} + +func (r *recorder) add(e string) { + r.mu.Lock() + defer r.mu.Unlock() + r.events = append(r.events, e) +} + +func (r *recorder) get() []string { + r.mu.Lock() + defer r.mu.Unlock() + return slices.Clone(r.events) +} + +func namedFn(name string, annotations map[string]string) pkgv1.Function { + fn := pkgv1.Function{} + fn.SetName(name) + fn.SetAnnotations(annotations) + return fn +} + +// devFn returns a Function named name on the Development runtime, which +// starts nothing and listens on name:9443. +func devFn(name string, annotations map[string]string) pkgv1.Function { + a := map[string]string{ + AnnotationKeyRuntime: string(AnnotationValueRuntimeDevelopment), + AnnotationKeyRuntimeDevelopmentTarget: name + ":9443", + } + maps.Copy(a, annotations) + return namedFn(name, a) +} + +// starts counts, by function name, the runtimes s started. +func starts(s *Session) map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + n := make(map[string]int) + for _, rt := range s.runtimes { + n[rt.name]++ + } + return n +} + +// recordStops makes each runtime s started record its stop in rec, then +// return err. +func recordStops(s *Session, rec *recorder, err error) { + s.mu.Lock() + defer s.mu.Unlock() + for i := range s.runtimes { + name := s.runtimes[i].name + s.runtimes[i].Stop = func(context.Context) error { + rec.add("stop:" + name) + return err + } + } +} + +// newTestSession returns a Session on engine, leasing in a temporary +// directory. +func newTestSession(t *testing.T, engine Engine, opts ...SessionOption) (*Session, string) { + t.Helper() + dir := t.TempDir() + opts = append([]SessionOption{ + WithLeaseDir(dir), + WithEngineFactory(func(*EngineFlags, logging.Logger) Engine { return engine }), + WithWarn(func(err error) { t.Errorf("unexpected warning: %v", err) }), + }, opts...) + s := NewSession(EngineFlags{}, logging.NewNopLogger(), opts...) + return s, dir +} + +func TestSessionStartsEachFunctionOnceConcurrently(t *testing.T) { + s, _ := newTestSession(t, &MockEngine{}) + + names := []string{"a", "b", "c", "d", "e"} + + var wg sync.WaitGroup + for i := range 16 { + wg.Go(func() { + fns := []pkgv1.Function{devFn(names[i%5], nil), devFn(names[(i+1)%5], nil)} + addrs, err := s.Start(t.Context(), fns) + if err != nil { + t.Errorf("Start(...): unexpected error: %v", err) + return + } + want := map[string]string{names[i%5]: names[i%5] + ":9443", names[(i+1)%5]: names[(i+1)%5] + ":9443"} + if diff := cmp.Diff(want, addrs); diff != "" { + t.Errorf("Start(...): -want addresses, +got addresses:\n%s", diff) + } + if _, err := s.Render(t.Context(), &renderv1alpha1.RenderRequest{}); err != nil { + t.Errorf("Render(...): unexpected error: %v", err) + } + }) + } + wg.Wait() + + got := starts(s) + for _, n := range names { + if got[n] != 1 { + t.Errorf("function %q: want 1 start, got %d", n, got[n]) + } + } +} + +func TestSessionSetupOnceBeforeRender(t *testing.T) { + cases := map[string]struct { + reason string + start bool + }{ + "StartWithNoFunctions": { + reason: "A Start with no functions should still run Setup, once, before the first Render.", + start: true, + }, + "RenderWithoutStart": { + reason: "A Render with no Start before it should run Setup itself, once.", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + rec := &recorder{} + engine := &MockEngine{ + MockSetup: func(context.Context, []pkgv1.Function) (func(), error) { + rec.add("setup") + return func() {}, nil + }, + MockRender: func(context.Context, *renderv1alpha1.RenderRequest) (*renderv1alpha1.RenderResponse, error) { + rec.add("render") + return &renderv1alpha1.RenderResponse{}, nil + }, + } + s, _ := newTestSession(t, engine) + + if tc.start { + if _, err := s.Start(t.Context(), nil); err != nil { + t.Fatalf("\n%s\nStart(...): unexpected error: %v", tc.reason, err) + } + if _, err := s.Start(t.Context(), nil); err != nil { + t.Fatalf("\n%s\nStart(...): unexpected error: %v", tc.reason, err) + } + } + for range 2 { + if _, err := s.Render(t.Context(), &renderv1alpha1.RenderRequest{}); err != nil { + t.Fatalf("\n%s\nRender(...): unexpected error: %v", tc.reason, err) + } + } + + if diff := cmp.Diff([]string{"setup", "render", "render"}, rec.get()); diff != "" { + t.Errorf("\n%s\n-want events, +got events:\n%s", tc.reason, diff) + } + }) + } +} + +func TestSessionClose(t *testing.T) { + rec := &recorder{} + var dir string + + leaseHeld := func() string { + matches, _ := filepath.Glob(filepath.Join(dir, "*.lock")) + return fmt.Sprintf("lease held: %t", len(matches) == 1) + } + + engine := &MockEngine{ + MockSetup: func(_ context.Context, fns []pkgv1.Function) (func(), error) { + n := fns[0].GetName() + return func() { rec.add(fmt.Sprintf("cleanup %s (%s)", n, leaseHeld())) }, nil + }, + } + s, dir := newTestSession(t, engine) + + if _, err := s.Start(t.Context(), []pkgv1.Function{devFn("a", nil)}); err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + // Two functions named b in one Start both start, and both stop. + if _, err := s.Start(t.Context(), []pkgv1.Function{devFn("a", nil), devFn("b", nil), devFn("b", nil)}); err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + if diff := cmp.Diff(map[string]int{"a": 1, "b": 2}, starts(s)); diff != "" { + t.Errorf("Start(...): want each function started once per Start that names it first: -want starts, +got starts:\n%s", diff) + } + recordStops(s, rec, nil) + + if err := s.Close(t.Context()); err != nil { + t.Fatalf("Close(...): unexpected error: %v", err) + } + rec.add(leaseHeld()) + + got := rec.get() + // Runtimes stop concurrently, so their order is not fixed. + slices.Sort(got[0:3]) + want := []string{ + "stop:a", "stop:b", "stop:b", + "cleanup b (lease held: true)", "cleanup a (lease held: true)", + "lease held: false", + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Close(...): want runtimes stopped, then Setup cleanups LIFO, then the lease released: -want events, +got events:\n%s", diff) + } + + if err := s.Close(t.Context()); err != nil { + t.Errorf("Close(...): a second Close: unexpected error: %v", err) + } + if diff := cmp.Diff(want, rec.get(), cmpopts.SortSlices(func(a, b string) bool { return a < b })); diff != "" { + t.Errorf("Close(...): a second Close should do nothing: -want events, +got events:\n%s", diff) + } + + if _, err := s.Start(t.Context(), nil); !errors.Is(err, errSessionClosed) { + t.Errorf("Start(...) after Close: want %v, got %v", errSessionClosed, err) + } +} + +func TestSessionStartFailureKeepsSetupCleanup(t *testing.T) { + rec := &recorder{} + engine := &MockEngine{ + MockSetup: func(context.Context, []pkgv1.Function) (func(), error) { + return func() { rec.add("cleanup") }, nil + }, + } + s, dir := newTestSession(t, engine) + + // An unknown runtime cannot be started. + fn := namedFn("a", map[string]string{AnnotationKeyRuntime: "Unknown"}) + _, err := s.Start(t.Context(), []pkgv1.Function{fn}) + if err == nil || !strings.Contains(err.Error(), "cannot start function runtimes") || !strings.Contains(err.Error(), "unknown runtime") { + t.Fatalf("Start(...): want a failure to start the unknown runtime, got %v", err) + } + if err := s.Close(t.Context()); err != nil { + t.Fatalf("Close(...): unexpected error: %v", err) + } + + if diff := cmp.Diff([]string{"cleanup"}, rec.get()); diff != "" { + t.Errorf("Close(...): a failed start should keep the Setup cleanup: -want events, +got events:\n%s", diff) + } + if matches, _ := filepath.Glob(filepath.Join(dir, "*.lock")); len(matches) != 0 { + t.Errorf("Close(...): want the lease released, got lease files %v", matches) + } +} + +func TestSessionCloseStopFailureKeepsLease(t *testing.T) { + errBoom := errors.New("boom") + s, dir := newTestSession(t, &MockEngine{}) + + if _, err := s.Start(t.Context(), []pkgv1.Function{devFn("a", nil)}); err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + recordStops(s, &recorder{}, errBoom) + if err := s.Close(t.Context()); !errors.Is(err, errBoom) { + t.Errorf("Close(...): want an error wrapping %v, got %v", errBoom, err) + } + + if matches, _ := filepath.Glob(filepath.Join(dir, "*.lock")); len(matches) != 1 { + t.Errorf("Close(...): a runtime that cannot be stopped should keep the lease for a later run to reclaim, got lease files %v", matches) + } +} + +// TestSessionDockerStartFailureKeepsLease checks that a container created by a +// failed Start, which no runtime stops, keeps the lease for a later run. +func TestSessionDockerStartFailureKeepsLease(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows has no run leases") + } + t.Setenv("DOCKER_CONFIG", t.TempDir()) // No credential helpers. + + errBoom := errors.New("boom") + cli := &fakeDocker{host: "unix:///var/run/docker.sock", startErr: errBoom} + s, dir := newTestSession(t, &MockEngine{}, WithDockerClient(cli)) + + fn := namedFn("fn", nil) + fn.Spec.Package = "fn-image" + if _, err := s.Start(t.Context(), []pkgv1.Function{fn}); !errors.Is(err, errBoom) { + t.Fatalf("Start(...): want an error wrapping %v, got %v", errBoom, err) + } + if err := s.Close(t.Context()); err != nil { + t.Fatalf("Close(...): unexpected error: %v", err) + } + + type state struct { + Containers []string + Leases int + } + matches, _ := filepath.Glob(filepath.Join(dir, "*.lock")) + got := state{Containers: cli.left().Containers, Leases: len(matches)} + want := state{Containers: []string{"id-fn-image"}, Leases: 1} + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Start and Close: want the created container left and its lease kept for a later run: -want, +got:\n%s", diff) + } +} + +// TestSessionSweepsOnlyItsOwnNetworks checks that Close removes the networks +// labelled with its session only when its session ID is its own. The seeded +// network is not the engine's own, so only the sweep can remove it. +func TestSessionSweepsOnlyItsOwnNetworks(t *testing.T) { + cases := map[string]struct { + reason string + flags EngineFlags + want []string + }{ + "OwnNetwork": { + reason: "A Docker engine on a network it created, under a generated ID, should sweep its session's networks.", + }, + "SuppliedID": { + reason: "A supplied ID may be shared with another process, so its networks should not be swept.", + flags: EngineFlags{SessionID: "0123456789abcdef"}, + want: []string{"stray"}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cli := &fakeDocker{} + s := NewSession(tc.flags, logging.NewNopLogger(), WithDockerClient(cli)) + cli.networks = []network.Summary{{Network: network.Network{ID: "stray", Labels: managedLabels(s.ID())}}} + + if _, err := s.Start(t.Context(), nil); err != nil { + t.Fatalf("\n%s\nStart(...): unexpected error: %v", tc.reason, err) + } + if err := s.Close(t.Context()); err != nil { + t.Fatalf("\n%s\nClose(...): unexpected error: %v", tc.reason, err) + } + + if diff := cmp.Diff(tc.want, cli.left().Networks, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("\n%s\nClose(...): -want networks left, +got networks left:\n%s", tc.reason, diff) + } + }) + } +} + +// TestSessionWithDockerClient checks that a Session reclaims a dead run, runs +// a function and the render container, and cleans up after itself through the +// client WithDockerClient supplies, and that it never closes that client. The +// environment points at no daemon, so a path that builds its own client fails. +// CheckContextSupport judges the supplied client's daemon too. +func TestSessionWithDockerClient(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows has no run leases, so no dead run to reclaim") + } + t.Setenv("DOCKER_HOST", "unix://"+filepath.Join(t.TempDir(), "no-docker.sock")) + t.Setenv("DOCKER_CONFIG", t.TempDir()) // No credential helpers. + + dir := t.TempDir() + deadID := containerruntime.NewSessionID() + if err := os.WriteFile(filepath.Join(dir, deadID+".lock"), nil, 0o600); err != nil { + t.Fatal(err) + } + + cli := &fakeDocker{ + host: "tcp://remote:2376", + containers: []container.Summary{{ID: "dead-fn", State: container.StateRunning, Labels: managedLabels(deadID)}}, + networks: []network.Summary{{Network: network.Network{ID: "dead-net", Labels: managedLabels(deadID)}}}, + } + s := NewSession(EngineFlags{CrossplaneImage: "render-image"}, logging.NewNopLogger(), + WithDockerClient(cli), + WithLeaseDir(dir), + WithWarn(func(err error) { t.Errorf("unexpected warning: %v", err) }), + ) + + // The environment's daemon is local, the supplied client's remote. + if err := s.CheckContextSupport(); err == nil { + t.Errorf("CheckContextSupport(): want an error for the supplied client's remote daemon, got nil") + } + + fn := namedFn("fn", nil) + fn.Spec.Package = "fn-image" + + if _, err := s.Start(t.Context(), []pkgv1.Function{fn}); err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + if _, err := s.Render(t.Context(), &renderv1alpha1.RenderRequest{}); err != nil { + t.Fatalf("Render(...): unexpected error: %v", err) + } + if err := s.Close(t.Context()); err != nil { + t.Fatalf("Close(...): unexpected error: %v", err) + } + + if diff := cmp.Diff(leftover{}, cli.left(), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Start, Render and Close: want the dead run and the Session's own resources gone, and the client not closed: -want, +got:\n%s", diff) + } +} + +// fakeDocker is a Docker client that keeps the containers and networks it +// serves, for the Docker calls a Session makes. Any other call panics. +type fakeDocker struct { + client.APIClient + + host string + // startErr, if set, is what ContainerStart returns. + startErr error + + mu sync.Mutex + closed bool + network string + networks []network.Summary + containers []container.Summary +} + +// leftover is what a fakeDocker still holds. +type leftover struct { + Containers []string + Networks []string + Closed bool +} + +func (f *fakeDocker) left() leftover { + f.mu.Lock() + defer f.mu.Unlock() + l := leftover{Closed: f.closed} + for _, c := range f.containers { + l.Containers = append(l.Containers, c.ID) + } + for _, n := range f.networks { + l.Networks = append(l.Networks, n.ID) + } + return l +} + +func (f *fakeDocker) DaemonHost() string { return f.host } + +func (f *fakeDocker) Close() error { + f.mu.Lock() + defer f.mu.Unlock() + f.closed = true + return nil +} + +// hasLabels reports whether labels has every key=value label filters selects +// on. +func hasLabels(filters client.Filters, labels map[string]string) bool { + for l := range filters["label"] { + k, v, _ := strings.Cut(l, "=") + if labels[k] != v { + return false + } + } + return true +} + +// ContainerList lists the containers the fake created and has not removed. +func (f *fakeDocker) ContainerList(_ context.Context, o client.ContainerListOptions) (client.ContainerListResult, error) { + f.mu.Lock() + defer f.mu.Unlock() + var res client.ContainerListResult + for _, c := range f.containers { + if hasLabels(o.Filters, c.Labels) { + res.Items = append(res.Items, c) + } + } + return res, nil +} + +// NetworkList lists the networks the fake created and has not removed. +func (f *fakeDocker) NetworkList(_ context.Context, o client.NetworkListOptions) (client.NetworkListResult, error) { + f.mu.Lock() + defer f.mu.Unlock() + var res client.NetworkListResult + for _, n := range f.networks { + if hasLabels(o.Filters, n.Labels) { + res.Items = append(res.Items, n) + } + } + return res, nil +} + +func (f *fakeDocker) NetworkCreate(_ context.Context, name string, o client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + f.mu.Lock() + f.network = name + f.networks = append(f.networks, network.Summary{Network: network.Network{ID: "net-id", Name: name, Labels: o.Labels}}) + f.mu.Unlock() + return client.NetworkCreateResult{ID: "net-id"}, nil +} + +func (f *fakeDocker) NetworkRemove(_ context.Context, id string, _ client.NetworkRemoveOptions) (client.NetworkRemoveResult, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.networks = slices.DeleteFunc(f.networks, func(n network.Summary) bool { return n.ID == id }) + return client.NetworkRemoveResult{}, nil +} + +func (f *fakeDocker) ImageInspect(_ context.Context, _ string, _ ...client.ImageInspectOption) (client.ImageInspectResult, error) { + return client.ImageInspectResult{}, nil +} + +// ContainerCreate records the container, running and attached to the networks +// it was created on, so ContainerList lists it until it is removed. +func (f *fakeDocker) ContainerCreate(_ context.Context, o client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + id := "id-" + o.Config.Image + c := container.Summary{ID: id, State: container.StateRunning, Labels: o.Config.Labels, NetworkSettings: &container.NetworkSettingsSummary{ + Networks: map[string]*network.EndpointSettings{}, + }} + if o.NetworkingConfig != nil { + for n := range o.NetworkingConfig.EndpointsConfig { + c.NetworkSettings.Networks[n] = &network.EndpointSettings{} + } + } + f.mu.Lock() + f.containers = append(f.containers, c) + f.mu.Unlock() + return client.ContainerCreateResult{ID: id}, nil +} + +func (f *fakeDocker) ContainerStart(_ context.Context, _ string, _ client.ContainerStartOptions) (client.ContainerStartResult, error) { + return client.ContainerStartResult{}, f.startErr +} + +// ContainerInspect reports the container attached to the network the fake +// created last. +func (f *fakeDocker) ContainerInspect(_ context.Context, id string, _ client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + f.mu.Lock() + defer f.mu.Unlock() + return client.ContainerInspectResult{Container: container.InspectResponse{ + Name: "/" + id, + NetworkSettings: &container.NetworkSettings{Networks: map[string]*network.EndpointSettings{f.network: {}}}, + }}, nil +} + +func (f *fakeDocker) ContainerStop(_ context.Context, _ string, _ client.ContainerStopOptions) (client.ContainerStopResult, error) { + return client.ContainerStopResult{}, nil +} + +func (f *fakeDocker) ContainerRemove(_ context.Context, id string, _ client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.containers = slices.DeleteFunc(f.containers, func(c container.Summary) bool { return c.ID == id }) + return client.ContainerRemoveResult{}, nil +} + +// ContainerAttach returns a connection that discards stdin and has no output. +func (f *fakeDocker) ContainerAttach(_ context.Context, _ string, _ client.ContainerAttachOptions) (client.ContainerAttachResult, error) { + return client.ContainerAttachResult{HijackedResponse: client.NewHijackedResponse(discardConn{}, "")}, nil +} + +// ContainerWait reports that the container exited successfully. +func (f *fakeDocker) ContainerWait(_ context.Context, _ string, _ client.ContainerWaitOptions) client.ContainerWaitResult { + res := make(chan container.WaitResponse, 1) + res <- container.WaitResponse{} + return client.ContainerWaitResult{Result: res} +} + +// discardConn is a connection that discards writes and reads nothing. +type discardConn struct { + net.Conn +} + +func (discardConn) Read([]byte) (int, error) { return 0, io.EOF } +func (discardConn) Write(b []byte) (int, error) { return len(b), nil } +func (discardConn) Close() error { return nil } From 6a8f6a0a4d78f005e488b662f66d17332d285579 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 3 Nov 2026 17:17:17 -0500 Subject: [PATCH 4/5] feat(render): reclaim crashed runs in composition and operation render Run crossplane render xr and op on a Session, so containers and networks a crashed render left behind are reclaimed by the next run. A named, reused function container is now taken over only when no live run will finish it, and keeps its creator's cleanup policy. The help text documents crash reclamation, named-container sharing, which containers a reap keeps, and that a supplied --session-id is not leased. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- cmd/crossplane/render/op/cmd.go | 38 ++- cmd/crossplane/render/op/cmd_test.go | 5 + cmd/crossplane/render/op/help/render.md | 19 +- cmd/crossplane/render/xr/cmd.go | 38 ++- cmd/crossplane/render/xr/cmd_test.go | 5 + cmd/crossplane/render/xr/help/render.md | 19 +- internal/containerruntime/container.go | 216 +++++++++++-- internal/containerruntime/container_test.go | 324 ++++++++++++++++++++ internal/containerruntime/run_test.go | 18 +- pkg/render/engine.go | 10 +- pkg/render/render.go | 4 +- pkg/render/runtime_docker.go | 1 + pkg/render/runtime_docker_test.go | 42 +++ 13 files changed, 668 insertions(+), 71 deletions(-) create mode 100644 internal/containerruntime/container_test.go diff --git a/cmd/crossplane/render/op/cmd.go b/cmd/crossplane/render/op/cmd.go index 0cee112c..86d2c5a0 100644 --- a/cmd/crossplane/render/op/cmd.go +++ b/cmd/crossplane/render/op/cmd.go @@ -41,6 +41,7 @@ import ( "github.com/crossplane/cli/v2/cmd/crossplane/render/contextfn" "github.com/crossplane/cli/v2/internal/async" "github.com/crossplane/cli/v2/internal/config" + "github.com/crossplane/cli/v2/internal/containerruntime" "github.com/crossplane/cli/v2/internal/dependency" "github.com/crossplane/cli/v2/internal/project" "github.com/crossplane/cli/v2/internal/project/functions" @@ -172,14 +173,22 @@ func (c *Cmd) Run(baseCtx context.Context, k *kong.Context, log logging.Logger, c.SetDefaultCrossplaneDockerNetwork(fns) - engine := c.newEngine(&c.EngineFlags, log) + sess := render.NewSession(c.EngineFlags, log, + render.WithEngineFactory(c.newEngine), + render.WithLeaseDir(containerruntime.DefaultLeaseDir()), + // Leasing this run and reclaiming dead runs is best effort, and + // must not change this command's output. + render.WithWarn(func(err error) { + log.Debug("Cannot lease this run or reclaim dead runs' Docker resources", "error", err) + }), + ) seedCtx := len(c.ContextValues) > 0 || len(c.ContextFiles) > 0 captureCtx := c.IncludeContext var ctxHandle *contextfn.Handle if seedCtx || captureCtx { - if err := engine.CheckContextSupport(); err != nil { + if err := sess.CheckContextSupport(); err != nil { return err } @@ -208,28 +217,23 @@ func (c *Cmd) Run(baseCtx context.Context, k *kong.Context, log logging.Logger, } } - cleanup, err := engine.Setup(ctx, fns) - if err != nil { - return err - } - defer cleanup() - - // Start function runtimes to get their addresses. - fnAddrs, err := render.StartFunctionRuntimes(ctx, log, fns) - if err != nil { - return errors.Wrap(err, "cannot start function runtimes") - } + // Close stops the function runtimes, then removes what Setup created. defer func() { - if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { - log.Info("Error stopping function runtimes", "error", err) + if err := sess.Close(ctx); err != nil { + log.Info("Error cleaning up render session", "error", err) render.WarnCleanupFailure(k.Stderr, err) } }() + fnAddrs, err := sess.Start(ctx, fns) + if err != nil { + return err + } + // Build and execute the render request. in := render.OperationInputs{ Operation: op, - FunctionAddrs: fnAddrs.Addresses(), + FunctionAddrs: fnAddrs, RequiredResources: rrs, RequiredSchemas: rsc, FunctionCredentials: fcreds, @@ -239,7 +243,7 @@ func (c *Cmd) Run(baseCtx context.Context, k *kong.Context, log logging.Logger, return errors.Wrap(err, "cannot build render request") } - rsp, err := engine.Render(ctx, req) + rsp, err := sess.Render(ctx, req) if err != nil { return errors.Wrap(err, "cannot render operation") } diff --git a/cmd/crossplane/render/op/cmd_test.go b/cmd/crossplane/render/op/cmd_test.go index ed68908e..52493ae4 100644 --- a/cmd/crossplane/render/op/cmd_test.go +++ b/cmd/crossplane/render/op/cmd_test.go @@ -98,6 +98,11 @@ func mustNewStruct(t *testing.T, data map[string]any) *structpb.Struct { } func TestCmdRun(t *testing.T) { + // Run leases this run in the user's cache directory, and reaps the dead + // runs it finds there; keep both off the developer's real one. + t.Setenv("HOME", t.TempDir()) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + type args struct { cmd Cmd } diff --git a/cmd/crossplane/render/op/help/render.md b/cmd/crossplane/render/op/help/render.md index 39ab9e50..21ceb9e4 100644 --- a/cmd/crossplane/render/op/help/render.md +++ b/cmd/crossplane/render/op/help/render.md @@ -20,7 +20,7 @@ they're run: | `render.crossplane.io/runtime: "Development"` | Connect to a Function running locally, instead of using Docker, for example when developing or debugging a new Function. The Function must be listening at `localhost:9443` and running with the `--insecure` flag. | | `render.crossplane.io/runtime-development-target: "dns:///example.org:7443"` | Connect to a Function running somewhere other than `localhost:9443`. The target uses gRPC target syntax (for example, `dns:///example.org:7443` or `example.org:7443`). | | `render.crossplane.io/runtime-docker-cleanup: "Orphan"` | Don't stop the Function's Docker container after rendering. | -| `render.crossplane.io/runtime-docker-name: ""` | Create or reuse a container with the given name. Restart the container if needed. | +| `render.crossplane.io/runtime-docker-name: ""` | Create or reuse a container with the given name. Restart the container if needed. See below for when a running container is reused. | | `render.crossplane.io/runtime-docker-pull-policy: "Always"` | Always pull the Function's package, even if it already exists locally. Other supported values are `Never` or `IfNotPresent`. | | `render.crossplane.io/runtime-docker-publish-address: "0.0.0.0"` | Host address that Docker should publish the Function's container port to. Defaults to `127.0.0.1` (localhost only). Use `0.0.0.0` to publish to all host network interfaces, enabling access from remote machines. | | `render.crossplane.io/runtime-docker-target: "docker-host"` | Address that the render CLI should use to connect to the Function's Docker container. If not specified, uses the publish address. | @@ -31,6 +31,23 @@ to the Docker daemon. See the [Docker environment variables](https://docs.docker.com/engine/reference/commandline/cli/#environment-variables) reference. +## Cleaning up after a crashed run + +If a run is killed before it cleans up (for example by `kill -9`), the next +`composition render` or `operation render` run cleans up what it left behind, +following each container's `runtime-docker-cleanup` policy. This doesn't apply +to runs started with `--session-id`, to runs on Windows, or to a different +`DOCKER_HOST`. + +If a container named by `runtime-docker-name` already exists: + +| Container | What this run does | +|---|---| +| Stopped | Restarts it. On exit it keeps it as long as either run's policy asks (`Orphan` > `Stop` > `Remove`). | +| Running, `Orphan` | Shares it and leaves it running. | +| Running, started by this run | Shares it. | +| Running, `Stop` or `Remove`, started by another run | Fails, because that run will stop or remove it. Use `Orphan` to share a container between runs. | + ## Project support When running `render` in a Crossplane Project (any directory containing a diff --git a/cmd/crossplane/render/xr/cmd.go b/cmd/crossplane/render/xr/cmd.go index 98cabd08..cc9dbfe7 100644 --- a/cmd/crossplane/render/xr/cmd.go +++ b/cmd/crossplane/render/xr/cmd.go @@ -48,6 +48,7 @@ import ( "github.com/crossplane/cli/v2/cmd/crossplane/render/contextfn" "github.com/crossplane/cli/v2/internal/async" "github.com/crossplane/cli/v2/internal/config" + "github.com/crossplane/cli/v2/internal/containerruntime" "github.com/crossplane/cli/v2/internal/dependency" "github.com/crossplane/cli/v2/internal/project" "github.com/crossplane/cli/v2/internal/project/functions" @@ -232,14 +233,22 @@ func (c *Cmd) Run(baseCtx context.Context, k *kong.Context, log logging.Logger, c.SetDefaultCrossplaneDockerNetwork(fns) - engine := c.newEngine(&c.EngineFlags, log) + sess := render.NewSession(c.EngineFlags, log, + render.WithEngineFactory(c.newEngine), + render.WithLeaseDir(containerruntime.DefaultLeaseDir()), + // Leasing this run and reclaiming dead runs is best effort, and + // must not change this command's output. + render.WithWarn(func(err error) { + log.Debug("Cannot lease this run or reclaim dead runs' Docker resources", "error", err) + }), + ) seedCtx := len(c.ContextValues) > 0 || len(c.ContextFiles) > 0 captureCtx := c.IncludeContext var ctxHandle *contextfn.Handle if seedCtx || captureCtx { - if err := engine.CheckContextSupport(); err != nil { + if err := sess.CheckContextSupport(); err != nil { return err } @@ -268,29 +277,24 @@ func (c *Cmd) Run(baseCtx context.Context, k *kong.Context, log logging.Logger, } } - cleanup, err := engine.Setup(ctx, fns) - if err != nil { - return err - } - defer cleanup() - - // Start function runtimes to get their addresses. - fnAddrs, err := render.StartFunctionRuntimes(ctx, log, fns) - if err != nil { - return errors.Wrap(err, "cannot start function runtimes") - } + // Close stops the function runtimes, then removes what Setup created. defer func() { - if err := render.StopFunctionRuntimes(ctx, fnAddrs); err != nil { - log.Info("Error stopping function runtimes", "error", err) + if err := sess.Close(ctx); err != nil { + log.Info("Error cleaning up render session", "error", err) render.WarnCleanupFailure(k.Stderr, err) } }() + fnAddrs, err := sess.Start(ctx, fns) + if err != nil { + return err + } + // Build and execute the render request. in := render.CompositionInputs{ CompositeResource: xr, Composition: comp, - FunctionAddrs: fnAddrs.Addresses(), + FunctionAddrs: fnAddrs, ObservedResources: ors, RequiredResources: rrs, RequiredSchemas: rsc, @@ -302,7 +306,7 @@ func (c *Cmd) Run(baseCtx context.Context, k *kong.Context, log logging.Logger, return errors.Wrap(err, "cannot build render request") } - rsp, err := engine.Render(ctx, req) + rsp, err := sess.Render(ctx, req) if err != nil { return errors.Wrap(err, "cannot render composite resource") } diff --git a/cmd/crossplane/render/xr/cmd_test.go b/cmd/crossplane/render/xr/cmd_test.go index dbd0d3db..d2ed5ae3 100644 --- a/cmd/crossplane/render/xr/cmd_test.go +++ b/cmd/crossplane/render/xr/cmd_test.go @@ -129,6 +129,11 @@ func fillResourceRefs(t *testing.T, xr *structpb.Struct) *structpb.Struct { } func TestCmdRun(t *testing.T) { + // Run leases this run in the user's cache directory, and reaps the dead + // runs it finds there; keep both off the developer's real one. + t.Setenv("HOME", t.TempDir()) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + type args struct { cmd Cmd } diff --git a/cmd/crossplane/render/xr/help/render.md b/cmd/crossplane/render/xr/help/render.md index 7bcce965..322802ee 100644 --- a/cmd/crossplane/render/xr/help/render.md +++ b/cmd/crossplane/render/xr/help/render.md @@ -23,7 +23,7 @@ they're run: | `render.crossplane.io/runtime: "Development"` | Connect to a Function running locally, instead of using Docker, for example when developing or debugging a new Function. The Function must be listening at `localhost:9443` and running with the `--insecure` flag. | | `render.crossplane.io/runtime-development-target: "dns:///example.org:7443"` | Connect to a Function running somewhere other than `localhost:9443`. The target uses gRPC target syntax (for example, `dns:///example.org:7443` or `example.org:7443`). | | `render.crossplane.io/runtime-docker-cleanup: "Orphan"` | Don't stop the Function's Docker container after rendering. | -| `render.crossplane.io/runtime-docker-name: ""` | Create or reuse a container with the given name. Restart the container if needed. | +| `render.crossplane.io/runtime-docker-name: ""` | Create or reuse a container with the given name. Restart the container if needed. See below for when a running container is reused. | | `render.crossplane.io/runtime-docker-pull-policy: "Always"` | Always pull the Function's package, even if it already exists locally. Other supported values are `Never` or `IfNotPresent`. | | `render.crossplane.io/runtime-docker-publish-address: "0.0.0.0"` | Host address that Docker should publish the Function's container port to. Defaults to `127.0.0.1` (localhost only). Use `0.0.0.0` to publish to all host network interfaces, enabling access from remote machines. | | `render.crossplane.io/runtime-docker-target: "docker-host"` | Address that the render CLI should use to connect to the Function's Docker container. If not specified, uses the publish address. | @@ -34,6 +34,23 @@ to the Docker daemon. See the [Docker environment variables](https://docs.docker.com/engine/reference/commandline/cli/#environment-variables) reference. +## Cleaning up after a crashed run + +If a run is killed before it cleans up (for example by `kill -9`), the next +`composition render` or `operation render` run cleans up what it left behind, +following each container's `runtime-docker-cleanup` policy. This doesn't apply +to runs started with `--session-id`, to runs on Windows, or to a different +`DOCKER_HOST`. + +If a container named by `runtime-docker-name` already exists: + +| Container | What this run does | +|---|---| +| Stopped | Restarts it. On exit it keeps it as long as either run's policy asks (`Orphan` > `Stop` > `Remove`). | +| Running, `Orphan` | Shares it and leaves it running. | +| Running, started by this run | Shares it. | +| Running, `Stop` or `Remove`, started by another run | Fails, because that run will stop or remove it. Use `Orphan` to share a container between runs. | + ## Project support When running `render` in a Crossplane Project (any directory containing a diff --git a/internal/containerruntime/container.go b/internal/containerruntime/container.go index 20e8cace..c123dd65 100644 --- a/internal/containerruntime/container.go +++ b/internal/containerruntime/container.go @@ -46,10 +46,19 @@ type Container struct { Image string // Name of the container. When set, an existing container with this name - // is reused (and restarted if needed) instead of creating a new one. - // Empty means Docker assigns a random name and nothing is reused. + // is reused (and restarted if needed) instead of creating a new one, + // subject to Domain. Empty means Docker assigns a random name and nothing + // is reused. Name string + // Domain is the annotation domain of Labels, e.g. DomainRender. When set + // with Name, Start reuses an existing container only when its labels say + // that's safe, and refuses one that another run will stop or remove; a + // run never stops or removes a container an earlier run labelled to keep + // longer. find has the full rules. Empty means any container with Name + // is reused. + Domain string + // Cmd is the command the container runs, e.g. a server subcommand and // its flags. Cmd []string @@ -113,8 +122,8 @@ type Client interface { ContainerStart(ctx context.Context, containerID string, options client.ContainerStartOptions) (client.ContainerStartResult, error) } -// CleanupClient is the subset of the Docker client Running.Stop uses to -// clean up a container. +// CleanupClient is the subset of the Docker client a Run uses to clean up a +// container. type CleanupClient 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) @@ -139,7 +148,18 @@ type Running struct { // rather than creating a new one. Reused bool - cli CleanupClient + // keep is the cleanup policy of the container Start reused, when Stop + // must keep it at least that long: Orphan for a running container another + // run keeps, or the label of a stopped container Start took over. Empty + // means Stop applies the policy it's given. + keep Cleanup + + // startedAt is when the container last started, as Start found it. A + // later start means another run took the container over. + startedAt string + + cli Client + log logging.Logger } // Start ensures the container is running and returns a handle carrying the @@ -164,7 +184,7 @@ func (c *Container) Start(ctx context.Context) (*Running, error) { } // Try to find an existing container with the supplied container name. - containerID, err := c.find(ctx, cli) + containerID, keep, err := c.find(ctx, cli, false) if err != nil { return nil, err } @@ -173,31 +193,70 @@ func (c *Container) Start(ctx context.Context) (*Running, error) { // Create new container if not found. if containerID == "" { containerID, err = c.create(ctx, cli) + if err != nil && c.Domain != "" && c.Name != "" && errdefs.IsConflict(err) { + // Another run created a container with this name since find + // looked. Judge it by the same rules, so a near-simultaneous + // run gets the reason it can't share it, not Docker's conflict. + // It may not have started yet, so judge it as running. + var cerr error + containerID, keep, cerr = c.find(ctx, cli, true) + switch { + case cerr != nil: + return nil, cerr + case containerID != "": + reused, err = true, nil + } + } if err != nil { return nil, err } } - addr, err := c.start(ctx, cli, containerID) + addr, startedAt, err := c.start(ctx, cli, containerID) if err != nil { return nil, err } - return &Running{Addr: addr, ID: containerID, Reused: reused, cli: cli}, nil + return &Running{Addr: addr, ID: containerID, Reused: reused, keep: keep, startedAt: startedAt, cli: cli, log: c.Log}, nil } // Stop finishes the container according to the cleanup policy: Orphan leaves // it running, Stop stops it with the grace period, and Remove stops it and -// then force removes it even if the graceful stop fails. +// then force removes it even if the graceful stop fails. When Start reused a +// container whose cleanup policy keeps it longer than cleanup, that policy +// applies instead. A container that has started again since Start, because +// another run found it stopped and took it over, is left to that run. func (r *Running) Stop(ctx context.Context, cleanup Cleanup) error { grace := int(StopGracePeriod / time.Second) stopOpts := client.ContainerStopOptions{Timeout: &grace} + if r.keep != "" && retention(r.keep) > retention(cleanup) { + r.log.Debug("Applying the reused Docker container's cleanup policy, because it keeps the container longer than this run's", "container", r.ID, "cleanup", r.keep, "run-cleanup", cleanup) + cleanup = r.keep + } + + if cleanup == CleanupStop || cleanup == CleanupRemove { + res, err := inspectContainer(ctx, r.cli, r.ID) + switch { + case errdefs.IsNotFound(err): + return nil + case err != nil: + return errors.Wrap(err, "cannot inspect Docker container") + case startedAt(res.Container.State) != r.startedAt: + r.log.Debug("Leaving Docker container another run started again after this one", "container", r.ID, "cleanup", cleanup) + return nil + } + } + + // Two handles can share one container, e.g. two Functions with one name, + // so the other may have removed it already, or still be removing it: a + // forced removal conflicts only with one in progress. That handle reports + // its own failure. switch cleanup { case CleanupOrphan: return nil case CleanupStop: - if err := stopContainer(ctx, r.cli, r.ID, stopOpts); err != nil { + if err := stopContainer(ctx, r.cli, r.ID, stopOpts); err != nil && !errdefs.IsNotFound(err) { return errors.Wrap(err, "cannot stop Docker container") } case CleanupRemove: @@ -209,7 +268,7 @@ func (r *Running) Stop(ctx context.Context, cleanup Cleanup) error { if err := stopContainer(ctx, r.cli, r.ID, stopOpts); err != nil { stopErr = errors.Wrap(err, "cannot stop Docker container") } - if err := removeContainer(ctx, r.cli, r.ID); err != nil { + if err := removeContainer(ctx, r.cli, r.ID); err != nil && !errdefs.IsNotFound(err) && !errdefs.IsConflict(err) { return errors.Join(errors.Wrap(err, "cannot remove Docker container"), stopErr) } } @@ -217,20 +276,104 @@ func (r *Running) Stop(ctx context.Context, cleanup Cleanup) error { return nil } -func (c *Container) find(ctx context.Context, cli Client) (string, error) { +// retention ranks how long a cleanup policy keeps a container: Remove +// shortest, then Stop, then any other, which includes the kept ones (see +// keeps). +func retention(cl Cleanup) int { + if cl == CleanupRemove { + return 0 + } + + if cl == CleanupStop { + return 1 + } + + return 2 +} + +// find returns the ID of the container named Name that Start may reuse, or +// "" if there is none, and the policy Stop must keep it at least as long as +// (see Running.keep). raced judges a container that isn't running as if it +// were, because another run created it a moment ago. +// +// With a Domain, a container is reused only when it is stopped, it has no +// /cleanup label, it's running with a kept policy (Orphan, or one +// this version doesn't know, and Running.Stop then always leaves it +// running), or it's running with this container's /session. A +// container another session runs under the Stop or Remove policy is +// refused, since that session stops or removes it when it exits. A stopped +// container is restarted and taken over, and Running.Stop then applies +// whichever of its /cleanup label and the policy Stop is given +// keeps it longer (Orphan, then Stop, then Remove), so a run never stops or +// removes a container an earlier run labelled to keep longer. A name +// conflict at create applies the same rules to the container that won the +// name, judging it as running even if it hasn't started yet. +// +// Docker labels can't change, so a taken-over container keeps its creator's +// labels: a later run that's refused it names the creator's session, not +// the run that now owns it, and a container labelled Stop or Remove that a +// taken-over Orphan run left running is refused to other sessions until +// it's removed by hand. A stopped container is taken over even if the run +// that created it is still running, but that run's Running.Stop then finds +// it started again and leaves it. If the run that took it over is killed, +// later runs don't reclaim it, since it carries the creator's session. Two runs that +// take over the same stopped container at once both treat it as theirs. A +// running container with no /cleanup label (an older CLI's) is +// reused, and Running.Stop treats it per the policy Stop is given. +func (c *Container) find(ctx context.Context, cli Client, raced bool) (string, Cleanup, error) { if c.Name == "" { - return "", nil + return "", "", nil } inspect, err := cli.ContainerInspect(ctx, c.Name, client.ContainerInspectOptions{}) if err != nil { if errdefs.IsNotFound(err) { - return "", nil // Container doesn't exist, but that's not an error + return "", "", nil // Container doesn't exist, but that's not an error } - return "", errors.Wrapf(err, "cannot inspect Docker container %q", c.Name) + return "", "", errors.Wrapf(err, "cannot inspect Docker container %q", c.Name) + } + found := inspect.Container + if c.Domain == "" { + return found.ID, "", nil } - return inspect.Container.ID, nil + var labels map[string]string + if found.Config != nil { + labels = found.Config.Labels + } + if !raced && (found.State == nil || !found.State.Running) { + // Stopped: restart it and take it over, keeping it at least as long + // as its label asks. Docker can't relabel it. + return found.ID, Cleanup(labels[c.Domain+labelSuffixCleanup]), nil + } + session := labels[c.Domain+labelSuffixSession] + switch cl := Cleanup(labels[c.Domain+labelSuffixCleanup]); { + case cl == "": + // Created before render recorded cleanup policies: reuse it, as + // before. + return found.ID, "", nil + case keeps(cl): + // Kept by the run that created it, or by a policy this version + // doesn't know: share it, never stop or remove it. + return found.ID, CleanupOrphan, nil + default: + // Stop or Remove. + if session == c.Labels[c.Domain+labelSuffixSession] { + // This run's own, e.g. two Functions with one name. + return found.ID, "", nil + } + // Only render's Functions choose their container's name. Simulate + // names only Orphan diff servers, so it meets this only for a + // container an older CLI or a crashed run left. + hint := "" + if c.Domain == DomainRender { + hint = fmt.Sprintf("To share it between runs, set the %s/runtime-docker-cleanup annotation to %q on the Functions that name it, or drop the %s/runtime-docker-name annotation. ", + c.Domain, CleanupOrphan, c.Domain) + } + return "", "", errors.Errorf("Docker container %q is in use by another run (session %q), which will %s it when it exits (cleanup policy %q). "+ + "%sIf it is left over from a crashed run or an older CLI, remove it with: docker rm -f %s", + c.Name, session, strings.ToLower(string(cl)), cl, hint, c.Name) + } } func (c *Container) create(ctx context.Context, cli Client) (string, error) { @@ -357,11 +500,12 @@ func (c *Container) pullImage(ctx context.Context, cli Client, options client.Im return nil } -// start ensures the container is running and returns its address. -func (c *Container) start(ctx context.Context, cli Client, containerID string) (string, error) { +// start ensures the container is running and returns its address and when +// it last started. +func (c *Container) start(ctx context.Context, cli Client, containerID string) (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") + return "", "", errors.Wrap(err, "cannot start Docker container") } // When using a Docker network, containers are resolvable by name within @@ -371,30 +515,30 @@ func (c *Container) start(ctx context.Context, cli Client, containerID string) ( if c.Network != "" { inspect, err := cli.ContainerInspect(ctx, containerID, client.ContainerInspectOptions{}) if err != nil { - return "", errors.Wrap(err, "cannot inspect Docker container") + return "", "", errors.Wrap(err, "cannot inspect Docker container") } if _, ok := inspect.Container.NetworkSettings.Networks[c.Network]; !ok { - return "", errors.Errorf("container %q is not connected to Docker network %q; verify the network exists and the container is attached to it", strings.TrimPrefix(inspect.Container.Name, "/"), c.Network) + return "", "", errors.Errorf("container %q is not connected to Docker network %q; verify the network exists and the container is attached to it", strings.TrimPrefix(inspect.Container.Name, "/"), c.Network) } hostname := strings.TrimPrefix(inspect.Container.Name, "/") if hostname == "" { - return "", errors.Errorf("cannot determine hostname for container %q on network %q", containerID, c.Network) + return "", "", errors.Errorf("cannot determine hostname for container %q on network %q", containerID, c.Network) } addr := net.JoinHostPort(hostname, fmt.Sprintf("%d", c.Port)) c.Log.Debug("Container reachable on network", "network", c.Network, "address", addr) - return addr, nil + return addr, startedAt(inspect.Container.State), nil } // Inspect the container to get the actual allocated host port. inspect, err := cli.ContainerInspect(ctx, containerID, client.ContainerInspectOptions{}) if err != nil { - return "", errors.Wrap(err, "cannot inspect Docker container") + return "", "", errors.Wrap(err, "cannot inspect Docker container") } // Look up the specific container port instead of taking the first one p := network.MustParsePort(fmt.Sprintf("%d/tcp", c.Port)) if len(inspect.Container.NetworkSettings.Ports[p]) == 0 { - return "", errors.Errorf("container %q has no published binding for port %s", c.Name, p.Port()) + return "", "", errors.Errorf("container %q has no published binding for port %s", c.Name, p.Port()) } binding := inspect.Container.NetworkSettings.Ports[p][0] @@ -403,10 +547,10 @@ func (c *Container) start(ctx context.Context, cli Client, containerID string) ( host = binding.HostIP.String() } if host == "" { - return "", errors.Errorf("container %q has port binding for %s but no host address", c.Name, p.Port()) + return "", "", errors.Errorf("container %q has port binding for %s but no host address", c.Name, p.Port()) } - return net.JoinHostPort(host, binding.HostPort), nil + return net.JoinHostPort(host, binding.HostPort), startedAt(inspect.Container.State), nil } func (c *Container) getPullOptions() (client.ImagePullOptions, error) { @@ -436,6 +580,24 @@ func (c *Container) getPullOptions() (client.ImagePullOptions, error) { }, nil } +// startedAt is when a container last started, or "" if Docker didn't say. +func startedAt(s *container.State) string { + if s == nil { + return "" + } + + return s.StartedAt +} + +// inspectContainer inspects a container, bounding the request by +// RemoveTimeout. +func inspectContainer(ctx context.Context, cli Client, containerID string) (client.ContainerInspectResult, error) { + ctx, cancel := context.WithTimeout(ctx, RemoveTimeout) + defer cancel() + + return cli.ContainerInspect(ctx, containerID, client.ContainerInspectOptions{}) +} + // stopContainer stops a container, bounding the request by StopTimeout. func stopContainer(ctx context.Context, cli CleanupClient, containerID string, opts client.ContainerStopOptions) error { ctx, cancel := context.WithTimeout(ctx, StopTimeout) diff --git a/internal/containerruntime/container_test.go b/internal/containerruntime/container_test.go new file mode 100644 index 00000000..d0414d39 --- /dev/null +++ b/internal/containerruntime/container_test.go @@ -0,0 +1,324 @@ +/* +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 containerruntime_test + +import ( + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/moby/moby/api/types/container" + + cr "github.com/crossplane/cli/v2/internal/containerruntime" +) + +const ( + ctrName = "fn" + ctrNet = "render-net" + thisRun = "this-session" + otherRun = "other-session" +) + +// existing is a render container named ctrName on ctrNet. An empty cleanup +// means it has no cleanup label, as from an older CLI. +type existing struct { + id string + running bool + session string + cleanup cr.Cleanup +} + +func (e existing) summary(domain string) container.Summary { + state, labels := exited, cr.ManagedLabels(domain, e.session) + if e.running { + state = running + } + if e.cleanup != "" { + labels = cr.ContainerLabels(domain, e.session, e.cleanup) + } + + c := ctr(e.id, state, labels, ctrNet) + c.Names = []string{"/" + ctrName} + + return c +} + +// TestContainerNamedReuse covers which container found by name Start reuses, +// and what Stop then leaves of it. A container Start creates has ID new-fn. +func TestContainerNamedReuse(t *testing.T) { + type args struct { + noDomain bool + // domain is the Container's Domain. Empty means DomainRender. + domain string + cleanup cr.Cleanup // this run's policy + existing *existing + // raced is created under the name by another run between Start's + // lookup and its create. + raced *existing + } + type want struct { + // errContains are substrings of Start's error. Empty means no error. + errContains []string + // errExcludes are substrings Start's error must not contain. + errExcludes []string + containers map[string]container.ContainerState + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "NotFoundIsCreated": { + reason: "With no container by the name, Start creates one, and Stop applies this run's policy.", + args: args{cleanup: cr.CleanupStop}, + want: want{containers: map[string]container.ContainerState{"new-fn": exited}}, + }, + "RunningOrphanIsSharedAndLeftRunning": { + reason: "A running Orphan container is shared, and this run never stops or removes it, though its own policy is Remove.", + args: args{cleanup: cr.CleanupRemove, existing: &existing{id: "old", running: true, session: otherRun, cleanup: cr.CleanupOrphan}}, + want: want{containers: map[string]container.ContainerState{"old": running}}, + }, + "RunningUnknownPolicyIsSharedAndLeftRunning": { + reason: "A running container with a policy this version doesn't know counts as Orphan, so it's shared and left running.", + args: args{cleanup: cr.CleanupRemove, existing: &existing{id: "old", running: true, session: otherRun, cleanup: cr.Cleanup("Keep")}}, + want: want{containers: map[string]container.ContainerState{"old": running}}, + }, + "RunningSameSessionIsShared": { + reason: "A running Remove container of this run's session is this run's own, so it's shared and finished under this run's policy.", + args: args{cleanup: cr.CleanupStop, existing: &existing{id: "old", running: true, session: thisRun, cleanup: cr.CleanupRemove}}, + want: want{containers: map[string]container.ContainerState{"old": exited}}, + }, + "RunningOfOtherSessionIsRefused": { + reason: "A running Remove container of another session would be removed under this run, so Start refuses it, saying why and what to do, and leaves it alone.", + args: args{cleanup: cr.CleanupRemove, existing: &existing{id: "old", running: true, session: otherRun, cleanup: cr.CleanupRemove}}, + want: want{ + errContains: []string{`"fn"`, otherRun, "another run", "remove it when it exits", `"Orphan"`, "runtime-docker-name", "docker rm -f fn"}, + containers: map[string]container.ContainerState{"old": running}, + }, + }, + "RefusalOutsideRenderGivesNoFunctionHint": { + reason: "Simulate names only Orphan diff servers, so a refusal there points at removing the leftover, not at render's Function annotations.", + args: args{domain: cr.DomainSimulate, cleanup: cr.CleanupOrphan, existing: &existing{id: "old", running: true, session: otherRun, cleanup: cr.CleanupRemove}}, + want: want{ + errContains: []string{`"fn"`, otherRun, "another run", "older CLI", "docker rm -f fn"}, + errExcludes: []string{"Functions", "runtime-docker-name"}, + containers: map[string]container.ContainerState{"old": running}, + }, + }, + "RunningWithoutCleanupLabelIsReused": { + reason: "A running container from before render recorded cleanup policies is reused under this run's policy, as before.", + args: args{cleanup: cr.CleanupStop, existing: &existing{id: "old", running: true, session: otherRun}}, + want: want{containers: map[string]container.ContainerState{"old": exited}}, + }, + "StoppedIsRestartedAndKeptPerItsLabel": { + reason: "A stopped container is restarted and taken over, and its Orphan label keeps it running past this Remove run.", + args: args{cleanup: cr.CleanupRemove, existing: &existing{id: "old", session: otherRun, cleanup: cr.CleanupOrphan}}, + want: want{containers: map[string]container.ContainerState{"old": running}}, + }, + "StoppedIsRestartedAndKeptPerThisRun": { + reason: "A stopped container is restarted and taken over, and this Stop run keeps it longer than its Remove label asks.", + args: args{cleanup: cr.CleanupStop, existing: &existing{id: "old", session: otherRun, cleanup: cr.CleanupRemove}}, + want: want{containers: map[string]container.ContainerState{"old": exited}}, + }, + "NameConflictIsRefusedByTheRules": { + reason: "When another run creates the name between lookup and create, Start judges that run's container by the same rules, refusing a Remove one rather than returning Docker's conflict.", + args: args{cleanup: cr.CleanupRemove, raced: &existing{id: "raced", running: true, session: otherRun, cleanup: cr.CleanupRemove}}, + want: want{ + errContains: []string{otherRun, "another run", "docker rm -f fn"}, + containers: map[string]container.ContainerState{"raced": running}, + }, + }, + "NoDomainReusesAnything": { + reason: "Without a Domain (the diff server manager), any container with the name is reused under this run's policy, as before.", + args: args{noDomain: true, cleanup: cr.CleanupStop, existing: &existing{id: "old", running: true, session: otherRun, cleanup: cr.CleanupRemove}}, + want: want{containers: map[string]container.ContainerState{"old": exited}}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + domain := tc.args.domain + if domain == "" { + domain = cr.DomainRender + } + + h := &host{} + if tc.args.existing != nil { + h.containers = append(h.containers, tc.args.existing.summary(domain)) + } + if tc.args.raced != nil { + c := tc.args.raced.summary(domain) + h.raced = &c + } + + c := &cr.Container{ + Image: "xpkg.crossplane.io/example/fn:v1", + Name: ctrName, + Domain: domain, + Port: 9443, + Labels: cr.ContainerLabels(domain, thisRun, tc.args.cleanup), + Network: ctrNet, + PullPolicy: cr.PullNever, + Client: h.client(), + } + if tc.args.noDomain { + c.Domain = "" + } + + r, err := c.Start(t.Context()) + if err == nil { + err = r.Stop(t.Context(), tc.args.cleanup) + } + + if len(tc.want.errContains) == 0 && err != nil { + t.Errorf("\n%s\nStart or Stop: unexpected error: %v", tc.reason, err) + } + for _, s := range tc.want.errContains { + if err == nil || !strings.Contains(err.Error(), s) { + t.Errorf("\n%s\nStart(...): want an error containing %q, got %v", tc.reason, s, err) + } + } + for _, s := range tc.want.errExcludes { + if err != nil && strings.Contains(err.Error(), s) { + t.Errorf("\n%s\nStart(...): want an error without %q, got %v", tc.reason, s, err) + } + } + + got, _ := h.left() + if diff := cmp.Diff(tc.want.containers, got, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("\n%s\ncontainers left: -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +// TestRunningStopSharedContainer covers two handles to one container, as when +// two Functions share a name: the second to stop it finds it removed, or its +// removal still in progress, which is not a failure. +func TestRunningStopSharedContainer(t *testing.T) { + cases := map[string]struct { + reason string + cleanup cr.Cleanup + // removing makes the second handle's removal conflict, as Docker + // refuses one while another is in progress. + removing bool + }{ + "RemoveFindsItRemoved": { + reason: "The second Remove handle finds the container gone.", + cleanup: cr.CleanupRemove, + }, + "RemoveFindsItBeingRemoved": { + reason: "The second Remove handle finds the first still removing the container.", + cleanup: cr.CleanupRemove, + removing: true, + }, + "StopFindsItRemoved": { + reason: "A Stop handle finds the container removed by a Remove handle.", + cleanup: cr.CleanupStop, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + h := &host{} + c := &cr.Container{ + Image: "xpkg.crossplane.io/example/fn:v1", + Name: ctrName, + Domain: cr.DomainRender, + Port: 9443, + Labels: cr.ContainerLabels(cr.DomainRender, thisRun, cr.CleanupRemove), + Network: ctrNet, + PullPolicy: cr.PullNever, + Client: h.client(), + } + + first, err := c.Start(t.Context()) + if err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + second, err := c.Start(t.Context()) + if err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + + if tc.removing { + h.removing = first.ID + } else if err := first.Stop(t.Context(), cr.CleanupRemove); err != nil { + t.Fatalf("Stop(...): unexpected error: %v", err) + } + + if err := second.Stop(t.Context(), tc.cleanup); err != nil { + t.Errorf("\n%s\nStop(...): want no error, got %v", tc.reason, err) + } + }) + } +} + +// TestRunningStopTakenOver covers a container that stopped while the run that +// started it was still running, e.g. because its process exited, and that +// another run then took over: the first run's Stop leaves it to that run. +func TestRunningStopTakenOver(t *testing.T) { + for _, cleanup := range []cr.Cleanup{cr.CleanupStop, cr.CleanupRemove} { + t.Run(string(cleanup), func(t *testing.T) { + h := &host{} + newContainer := func(session string) *cr.Container { + return &cr.Container{ + Image: "xpkg.crossplane.io/example/fn:v1", + Name: ctrName, + Domain: cr.DomainRender, + Port: 9443, + Labels: cr.ContainerLabels(cr.DomainRender, session, cleanup), + Network: ctrNet, + PullPolicy: cr.PullNever, + Client: h.client(), + } + } + + creator, err := newContainer(thisRun).Start(t.Context()) + if err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + h.containers[0].State = exited + + taker, err := newContainer(otherRun).Start(t.Context()) + if err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + + if err := creator.Stop(t.Context(), cleanup); err != nil { + t.Fatalf("Stop(...): unexpected error: %v", err) + } + if got, _ := h.left(); got["new-"+ctrName] != running { + t.Errorf("creator's Stop(...): want the taken-over container left running, got %v", got) + } + + if err := taker.Stop(t.Context(), cleanup); err != nil { + t.Fatalf("Stop(...): unexpected error: %v", err) + } + want := map[string]container.ContainerState{"new-" + ctrName: exited} + if cleanup == cr.CleanupRemove { + want = nil + } + got, _ := h.left() + if diff := cmp.Diff(want, got, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("taker's Stop(...): containers left, -want, +got:\n%s", diff) + } + }) + } +} diff --git a/internal/containerruntime/run_test.go b/internal/containerruntime/run_test.go index 5bedd31f..3df7f374 100644 --- a/internal/containerruntime/run_test.go +++ b/internal/containerruntime/run_test.go @@ -24,6 +24,7 @@ import ( "regexp" "runtime" "slices" + "strconv" "strings" "sync" "testing" @@ -74,6 +75,12 @@ type host struct { raced *container.Summary // cannotRemove names the container the daemon fails to remove. cannotRemove string + // removing names the container another client is removing, so a removal + // conflicts. + removing string + // starts counts each container's starts, which inspect reports as when + // it last started. + starts map[string]int // vanished holds IDs that are listed but gone by the time they are // stopped or removed, as when a concurrent reaper got there first. vanished map[string]bool @@ -145,7 +152,7 @@ func (h *host) containerInspect(_ context.Context, id string, _ client.Container return client.ContainerInspectResult{Container: container.InspectResponse{ ID: c.ID, Name: c.Names[0], - State: &container.State{Running: c.State == running}, + State: &container.State{Running: c.State == running, StartedAt: strconv.Itoa(h.starts[c.ID])}, Config: &container.Config{Labels: c.Labels}, NetworkSettings: &container.NetworkSettings{Networks: c.NetworkSettings.Networks}, }}, nil @@ -179,6 +186,12 @@ func (h *host) containerStart(_ context.Context, id string, _ client.ContainerSt return client.ContainerStartResult{}, err } + if h.containers[i].State != running { + if h.starts == nil { + h.starts = map[string]int{} + } + h.starts[h.containers[i].ID]++ + } h.containers[i].State = running return client.ContainerStartResult{}, nil @@ -216,6 +229,9 @@ func (h *host) containerRemove(_ context.Context, id string, opts client.Contain if id == h.cannotRemove { return client.ContainerRemoveResult{}, errors.New("daemon says no") } + if id == h.removing { + return client.ContainerRemoveResult{}, errors.Wrapf(errdefs.ErrConflict, "removal of container %s is already in progress", id) + } i, err := h.container(id) if err != nil { diff --git a/pkg/render/engine.go b/pkg/render/engine.go index cbec0994..0e4da451 100644 --- a/pkg/render/engine.go +++ b/pkg/render/engine.go @@ -99,11 +99,11 @@ type Engine interface { // EngineFlags contains flags for configuring the render engine. It is embedded // by render command structs to provide shared engine configuration. type EngineFlags struct { - CrossplaneVersion string `help:"Version of the Crossplane image to use for rendering. Defaults to the latest stable version." placeholder:"VERSION" xor:"crossplane-selector"` - CrossplaneImage string `help:"Override the full Crossplane Docker image reference for rendering." placeholder:"IMAGE" xor:"crossplane-selector"` - CrossplaneBinary string `help:"Path to a local crossplane binary to use instead of Docker. Ignores --crossplane-docker-network." placeholder:"PATH" type:"existingfile" xor:"crossplane-selector"` - CrossplaneDockerNetwork string `env:"CROSSPLANE_DOCKER_NETWORK" help:"The docker network to start the crossplane container in (also $$${env}). Ignored with --crossplane-binary."` - SessionID string `help:"${session_id_help=Session ID to apply as the render.crossplane.io/session label on Docker containers and networks that render creates. Defaults to a random per-process ID.}" placeholder:"ID"` + CrossplaneVersion string `help:"Version of the Crossplane image to use for rendering. Defaults to the latest stable version." placeholder:"VERSION" xor:"crossplane-selector"` + CrossplaneImage string `help:"Override the full Crossplane Docker image reference for rendering." placeholder:"IMAGE" xor:"crossplane-selector"` + CrossplaneBinary string `help:"Path to a local crossplane binary to use instead of Docker. Ignores --crossplane-docker-network." placeholder:"PATH" type:"existingfile" xor:"crossplane-selector"` + CrossplaneDockerNetwork string `env:"CROSSPLANE_DOCKER_NETWORK" help:"The docker network to start the crossplane container in (also $$${env}). Ignored with --crossplane-binary."` + SessionID string `help:"${session_id_help=Session ID to apply as the render.crossplane.io/session label on Docker containers and networks that render creates. Defaults to a random per-process ID. A supplied ID isn't leased, so its resources aren't reclaimed after a crash.}" placeholder:"ID"` } // AfterApply warns when --crossplane-docker-network is passed on the command diff --git a/pkg/render/render.go b/pkg/render/render.go index 9e9da0e7..48cb91aa 100644 --- a/pkg/render/render.go +++ b/pkg/render/render.go @@ -246,11 +246,11 @@ func OverrideFunctionAnnotations(fns []pkgv1.Function, annotations []string) err } // 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 +// err is nil. Callers use it to surface render session 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) + _, _ = fmt.Fprintf(w, "warning: failed to clean up render session; some resources (e.g. containers or networks) may need to be removed manually: %v\n", err) } diff --git a/pkg/render/runtime_docker.go b/pkg/render/runtime_docker.go index 60270c16..359a17c4 100644 --- a/pkg/render/runtime_docker.go +++ b/pkg/render/runtime_docker.go @@ -319,6 +319,7 @@ func (r *RuntimeDocker) Start(ctx context.Context) (RuntimeContext, error) { Port: FunctionPort, Env: r.Env, Labels: functionContainerLabels(r.Session, r.Cleanup), + Domain: labelDomain, BindAddress: r.BindAddress, Target: r.Target, Network: r.Network, diff --git a/pkg/render/runtime_docker_test.go b/pkg/render/runtime_docker_test.go index 4e3b62e3..06f17fe9 100644 --- a/pkg/render/runtime_docker_test.go +++ b/pkg/render/runtime_docker_test.go @@ -708,3 +708,45 @@ func TestRuntimeDockerStartLabels(t *testing.T) { }) } } + +func TestRuntimeDockerNamedContainer(t *testing.T) { + const ( + name = "shared-fn" + dockerNetwork = "render-net" + ) + + // A running Orphan container found by name is shared, and this run's + // Remove policy never stops or removes it: ContainerStop and + // ContainerRemove are nil, so calling either panics. + cli := &fake.Client{ + MockContainerInspect: func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: "old", + Name: "/" + name, + State: &container.State{Running: true}, + Config: &container.Config{Labels: functionContainerLabels("other-session", AnnotationValueRuntimeDockerCleanupOrphan)}, + NetworkSettings: &container.NetworkSettings{Networks: map[string]*network.EndpointSettings{dockerNetwork: {}}}, + }}, nil + }, + MockContainerStart: startContainer("old"), + } + r := &RuntimeDocker{ + Image: "xpkg.crossplane.io/crossplane-contrib/function-dummy:v0.1.0", + Name: name, + Cleanup: AnnotationValueRuntimeDockerCleanupRemove, + Session: "this-session", + PullPolicy: AnnotationValueRuntimeDockerPullPolicyNever, + Keychain: authn.NewMultiKeychain(), + Network: dockerNetwork, + log: logging.NewNopLogger(), + dockerClient: cli, + } + + rctx, err := r.Start(t.Context()) + if err != nil { + t.Fatalf("Start(...): unexpected error: %v", err) + } + if err := rctx.Stop(t.Context()); err != nil { + t.Errorf("Stop(...): unexpected error: %v", err) + } +} From 4df4ba69be539d62cfdcd54bee696861cab46831 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 3 Nov 2026 17:17:17 -0500 Subject: [PATCH 5/5] test: reclaim a SIGKILLed run and a Session's network on real Docker Add Docker integration tests that SIGKILL a process holding a run lease and check the next run reclaims its containers and networks, and that a Session removes its network on Close. The busybox image is pinned and its pull is retried and bounded so the tests stay reliable on CI. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- .../containerruntime/run_integration_test.go | 311 ++++++++++++++++++ pkg/render/session_integration_test.go | 87 +++++ 2 files changed, 398 insertions(+) create mode 100644 internal/containerruntime/run_integration_test.go create mode 100644 pkg/render/session_integration_test.go diff --git a/internal/containerruntime/run_integration_test.go b/internal/containerruntime/run_integration_test.go new file mode 100644 index 00000000..8a4459ad --- /dev/null +++ b/internal/containerruntime/run_integration_test.go @@ -0,0 +1,311 @@ +//go:build integration && unix + +/* +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 containerruntime_test + +import ( + "context" + "maps" + "os" + "testing" + "time" + + "github.com/containerd/errdefs" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/network" + "github.com/moby/moby/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + + cr "github.com/crossplane/cli/v2/internal/containerruntime" +) + +const ( + // busybox is pinned, on the GCR mirror, which avoids Docker Hub's + // unauthenticated pull limits. + busybox = "mirror.gcr.io/library/busybox:1.37" + + // labelTest marks every resource this test creates, with a value unique + // to the test, so its cleanup removes those and nothing else. If the test + // dies without running its cleanups (a -timeout panic, a SIGKILL), remove + // what it leaked with + // + // docker rm -f $(docker ps -aq --filter label=containerruntime.crossplane.io/integration-test) + // docker network rm $(docker network ls -q --filter label=containerruntime.crossplane.io/integration-test) + labelTest = "containerruntime.crossplane.io/integration-test" +) + +// TestRunReclaimsADeadRunOnDocker pins, against a real Docker daemon, what +// Acquire does to the resources of a run that was SIGKILLed: the containers it +// would have removed are removed, a Stop container is stopped and kept, an +// Orphan container is kept running, a network holding only a stopped +// container is removed, the network of the kept Orphan container is kept, and +// the dead run's lease is dropped. While the run lives, its lease keeps all of +// it. The resources carry the child's session ID but are created here: the +// reaper finds them by label, whichever process created them. +// +// It fails, rather than skips, without a Docker daemon: the integration job +// must have one, and a skip would hide a broken runner. So it fails on a dev +// machine without Docker too. +func TestRunReclaimsADeadRunOnDocker(t *testing.T) { + ctx := t.Context() + + cli, err := client.New(client.FromEnv) + if err != nil { + t.Fatalf("client.New(...): %v", err) + } + t.Cleanup(func() { _ = cli.Close() }) + + if _, err := cli.Ping(ctx, client.PingOptions{}); err != nil { + t.Fatalf("this test needs a Docker daemon: Ping(...): %v", err) + } + + pullIfMissing(t, cli, busybox) + + testID := cr.NewSessionID() + t.Cleanup(func() { removeTestResources(t, cli, testID) }) + + dir, id := t.TempDir(), cr.NewSessionID() + child := startLeaseHelper(t, dir, id) + + render := func(c cr.Cleanup) map[string]string { return cr.ContainerLabels(cr.DomainRender, id, c) } + + net := createNetwork(t, cli, testID, cr.ManagedLabels(cr.DomainRender, id)) + orphanNet := createNetwork(t, cli, testID, cr.ManagedLabels(cr.DomainRender, id)) + + remove := runContainer(t, cli, testID, busybox, render(cr.CleanupRemove), net) + renderer := runContainer(t, cli, testID, busybox, cr.ManagedLabels(cr.DomainRender, id), net) + stop := runContainer(t, cli, testID, busybox, render(cr.CleanupStop), net) + simulate := runContainer(t, cli, testID, busybox, cr.ContainerLabels(cr.DomainSimulate, id, cr.CleanupRemove), "") + orphan := runContainer(t, cli, testID, busybox, render(cr.CleanupOrphan), orphanNet) + + all := []string{remove, renderer, stop, simulate, orphan} + + // While the child lives, its lease keeps everything. + acquire(t, dir) + + for _, c := range all { + if !containerRunning(t, cli, c) { + t.Errorf("container %s of a live run: not running after another run's Acquire, want it untouched", c) + } + } + + for _, n := range []string{net, orphanNet} { + if !networkExists(t, cli, n) { + t.Errorf("network %s of a live run: removed by another run's Acquire, want it untouched", n) + } + } + + if err := child.Process.Kill(); err != nil { + t.Fatalf("Kill(): %v", err) + } + _ = child.Wait() + + acquire(t, dir) + + for _, c := range []string{remove, renderer, simulate} { + if containerExists(t, cli, c) { + t.Errorf("container %s of the dead run: still exists after Acquire, want it removed", c) + } + } + + switch { + case !containerExists(t, cli, stop): + t.Errorf("Stop container %s of the dead run: removed by Acquire, want it stopped and kept", stop) + case containerRunning(t, cli, stop): + t.Errorf("Stop container %s of the dead run: still running after Acquire, want it stopped", stop) + } + + if !containerRunning(t, cli, orphan) { + t.Errorf("Orphan container %s of the dead run: not running after Acquire, want it untouched", orphan) + } + + if networkExists(t, cli, net) { + t.Errorf("network %s of the dead run, holding only a stopped container: still exists after Acquire, want it removed", net) + } + + if !networkExists(t, cli, orphanNet) { + t.Errorf("network %s of the dead run's Orphan container: removed by Acquire, want it kept", orphanNet) + } + + if _, err := os.Stat(cr.LeasePath(dir, id)); !os.IsNotExist(err) { + t.Errorf("lease file of the dead run: Stat(...) = %v, want it removed", err) + } +} + +// acquire runs a new run's Acquire on the lease directory, then releases it. +func acquire(t *testing.T, dir string) { + t.Helper() + + r := cr.NewRun(dir, cr.NewSessionID(), true, logging.NewNopLogger()) + if err := r.Acquire(t.Context()); err != nil { + t.Errorf("Acquire(...): unexpected error: %v", err) + } + + if err := r.Release(context.Background(), true, ""); err != nil { + t.Errorf("Release(...): unexpected error: %v", err) + } +} + +// pullIfMissing pulls image ref unless it is on the daemon, so local runs +// stay offline. +func pullIfMissing(t *testing.T, cli *client.Client, ref string) { + t.Helper() + + _, err := cli.ImageInspect(t.Context(), ref) + if !errdefs.IsNotFound(err) { + if err != nil { + t.Fatalf("ImageInspect(%q): %v", ref, err) + } + return + } + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Minute) + defer cancel() + + if err := cr.PullImage(ctx, cli, ref, client.ImagePullOptions{}); err != nil { + t.Fatalf("PullImage(%q): %v", ref, err) + } +} + +// createNetwork creates a bridge network with labels, marked as the test's. +func createNetwork(t *testing.T, cli *client.Client, testID string, labels map[string]string) string { + t.Helper() + + name := "crossplane-cli-test-" + cr.NewSessionID() + + l := maps.Clone(labels) + l[labelTest] = testID + + if _, err := cli.NetworkCreate(t.Context(), name, client.NetworkCreateOptions{Driver: "bridge", Labels: l}); err != nil { + t.Fatalf("NetworkCreate(%q): %v", name, err) + } + + return name +} + +// runContainer starts a container of image with labels, marked as the test's, +// on network net when it is not empty, and returns its ID. +func runContainer(t *testing.T, cli *client.Client, testID, image string, labels map[string]string, net string) string { + t.Helper() + + l := maps.Clone(labels) + l[labelTest] = testID + + init := true + opts := client.ContainerCreateOptions{ + Config: &container.Config{Image: image, Cmd: []string{"sleep", "300"}, Labels: l}, + HostConfig: &container.HostConfig{Init: &init}, + } + if net != "" { + opts.NetworkingConfig = &network.NetworkingConfig{EndpointsConfig: map[string]*network.EndpointSettings{net: {}}} + } + + rsp, err := cli.ContainerCreate(t.Context(), opts) + if err != nil { + t.Fatalf("ContainerCreate(...): %v", err) + } + + if _, err := cli.ContainerStart(t.Context(), rsp.ID, client.ContainerStartOptions{}); err != nil { + t.Fatalf("ContainerStart(%s): %v", rsp.ID, err) + } + + return rsp.ID +} + +// containerExists reports whether container id exists, in any state. +func containerExists(t *testing.T, cli *client.Client, id string) bool { + t.Helper() + + _, err := cli.ContainerInspect(t.Context(), id, client.ContainerInspectOptions{}) + switch { + case err == nil: + return true + case errdefs.IsNotFound(err): + return false + } + + t.Fatalf("ContainerInspect(%s): %v", id, err) + + return false +} + +// containerRunning reports whether container id exists and is running. +func containerRunning(t *testing.T, cli *client.Client, id string) bool { + t.Helper() + + res, err := cli.ContainerInspect(t.Context(), id, client.ContainerInspectOptions{}) + switch { + case err == nil: + return res.Container.State != nil && res.Container.State.Running + case errdefs.IsNotFound(err): + return false + } + + t.Fatalf("ContainerInspect(%s): %v", id, err) + + return false +} + +// networkExists reports whether network name exists. +func networkExists(t *testing.T, cli *client.Client, name string) bool { + t.Helper() + + _, err := cli.NetworkInspect(t.Context(), name, client.NetworkInspectOptions{}) + switch { + case err == nil: + return true + case errdefs.IsNotFound(err): + return false + } + + t.Fatalf("NetworkInspect(%q): %v", name, err) + + return false +} + +// removeTestResources force removes every container, then every network, +// marked as test testID's. +func removeTestResources(t *testing.T, cli *client.Client, testID string) { + t.Helper() + + ctx := context.Background() + f := make(client.Filters).Add("label", labelTest+"="+testID) + + cs, err := cli.ContainerList(ctx, client.ContainerListOptions{All: true, Filters: f}) + if err != nil { + t.Errorf("cleanup: ContainerList(...): %v", err) + } + + for _, c := range cs.Items { + if _, err := cli.ContainerRemove(ctx, c.ID, client.ContainerRemoveOptions{Force: true}); err != nil && !errdefs.IsNotFound(err) { + t.Errorf("cleanup: ContainerRemove(%s): %v", c.ID, err) + } + } + + ns, err := cli.NetworkList(ctx, client.NetworkListOptions{Filters: f}) + if err != nil { + t.Errorf("cleanup: NetworkList(...): %v", err) + } + + for _, n := range ns.Items { + if _, err := cli.NetworkRemove(ctx, n.ID, client.NetworkRemoveOptions{}); err != nil && !errdefs.IsNotFound(err) { + t.Errorf("cleanup: NetworkRemove(%s): %v", n.Name, err) + } + } +} diff --git a/pkg/render/session_integration_test.go b/pkg/render/session_integration_test.go new file mode 100644 index 00000000..4ff5424e --- /dev/null +++ b/pkg/render/session_integration_test.go @@ -0,0 +1,87 @@ +//go:build integration + +/* +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 ( + "testing" + + "github.com/moby/moby/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/logging" + + "github.com/crossplane/cli/v2/internal/containerruntime" +) + +// TestSessionNetworkOnDocker pins, against a real Docker daemon, that a +// Session's Start with no functions creates its network on the daemon and its +// Close removes it. Both the engine's cleanup and Close's sweep remove it, so +// this does not tell which one did. It needs no image. Like the +// containerruntime integration test, it fails rather than skips without a +// Docker daemon. +// +// If the test dies without its cleanups (a -timeout panic, a SIGKILL), remove +// the network it leaked, using the session ID it logged, with +// +// docker network rm $(docker network ls -q --filter name=crossplane-render- --filter label=render.crossplane.io/session=) +func TestSessionNetworkOnDocker(t *testing.T) { + ctx := t.Context() + + cli, err := client.New(client.FromEnv) + if err != nil { + t.Fatalf("client.New(...): %v", err) + } + t.Cleanup(func() { _ = cli.Close() }) + + if _, err := cli.Ping(ctx, client.PingOptions{}); err != nil { + t.Fatalf("this test needs a Docker daemon: Ping(...): %v", err) + } + + s := NewSession(EngineFlags{}, logging.NewNopLogger(), WithLeaseDir(t.TempDir()), WithDockerClient(cli)) + t.Cleanup(func() { _ = s.Close(ctx) }) + t.Logf("session %s", s.ID()) + + if _, err := s.Start(ctx, nil); err != nil { + t.Fatalf("Start(ctx, nil): %v", err) + } + + f := make(client.Filters) + for k, v := range containerruntime.ManagedLabels(containerruntime.DomainRender, s.ID()) { + f = f.Add("label", k+"="+v) + } + + ns, err := cli.NetworkList(ctx, client.NetworkListOptions{Filters: f}) + if err != nil { + t.Fatalf("NetworkList(...): %v", err) + } + if len(ns.Items) != 1 { + t.Fatalf("networks labelled with the session's ID after Start: got %d, want 1", len(ns.Items)) + } + + if err := s.Close(ctx); err != nil { + t.Errorf("Close(...): %v", err) + } + + ns, err = cli.NetworkList(ctx, client.NetworkListOptions{Filters: f}) + if err != nil { + t.Fatalf("NetworkList(...): %v", err) + } + if len(ns.Items) != 0 { + t.Errorf("networks labelled with the session's ID after Close: got %d, want 0", len(ns.Items)) + } +}