From 3131e3905b0fc006072238b5b4426ef57ed5718b Mon Sep 17 00:00:00 2001 From: eren23 Date: Sun, 18 Oct 2026 13:50:02 +0200 Subject: [PATCH] fix(orchestrator): refuse a create that runs out of memory so placement retries it Delete stops the sandbox in a goroutine and returns before Firecracker exits. The killed sandbox keeps its hugepages for up to about 0.5 s. A create in that window fails to mmap the guest memfd with ENOMEM. Create returned codes.Internal, so placement excluded the node and the API answered 500 on a single-node cluster. Return codes.ResourceExhausted when the create error is ENOMEM. Placement then retries the node with backoff, and the create lands when the pages come back. Run the start rollback before that refusal and release the sandbox ID at once. The rollback holds only the template pin at that point. Placement retries within a millisecond with the same sandbox ID, and an ID that is still reserved gives AlreadyExists, which excludes the node. The wait for uffd ready now wraps context.Cause instead of ctx.Err, so the ENOMEM also reaches Create when the uffd handler exits after Firecracker loaded the snapshot. Fixes #3689 --- .../orchestrator/pkg/sandbox/fc/client.go | 2 +- .../pkg/sandbox/fc/client_test.go | 55 +++++++++++++++++++ .../pkg/server/memory_refusal_test.go | 35 ++++++++++++ packages/orchestrator/pkg/server/sandboxes.go | 26 +++++++++ 4 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 packages/orchestrator/pkg/sandbox/fc/client_test.go create mode 100644 packages/orchestrator/pkg/server/memory_refusal_test.go diff --git a/packages/orchestrator/pkg/sandbox/fc/client.go b/packages/orchestrator/pkg/sandbox/fc/client.go index 89691ea8c2..dc69575eae 100644 --- a/packages/orchestrator/pkg/sandbox/fc/client.go +++ b/packages/orchestrator/pkg/sandbox/fc/client.go @@ -109,7 +109,7 @@ func (c *apiClient) loadSnapshot( select { case <-ctx.Done(): - return fmt.Errorf("context canceled while waiting for uffd ready: %w", ctx.Err()) + return fmt.Errorf("context canceled while waiting for uffd ready: %w", context.Cause(ctx)) case <-uffdReady: // Wait for the uffd to be ready to serve requests } diff --git a/packages/orchestrator/pkg/sandbox/fc/client_test.go b/packages/orchestrator/pkg/sandbox/fc/client_test.go new file mode 100644 index 0000000000..b4e199afbe --- /dev/null +++ b/packages/orchestrator/pkg/sandbox/fc/client_test.go @@ -0,0 +1,55 @@ +//go:build linux + +package fc + +import ( + "context" + "fmt" + "net" + "net/http" + "os" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// A uffd handler that exits after Firecracker loaded the snapshot ends the +// wait for it with its exit error as the cause. The cause must reach the +// caller: Create tells a node out of memory (ENOMEM from the memfd mmap) from +// a failed create by it. +func TestLoadSnapshotKeepsUffdExitCause(t *testing.T) { + t.Parallel() + + dir, err := os.MkdirTemp("", "fc") //nolint:usetesting // t.TempDir embeds the test name and overflows the unix sun_path limit + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + + socket := dir + "/fc.sock" + listener, err := new(net.ListenConfig).Listen(t.Context(), "unix", socket) + require.NoError(t, err) + + loaded := make(chan struct{}) + mux := http.NewServeMux() + mux.HandleFunc("PUT /snapshot/load", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + close(loaded) + }) + server := &http.Server{Handler: mux} //nolint:gosec // no timeouts needed for a stub on a private socket + go func() { _ = server.Serve(listener) }() + t.Cleanup(func() { _ = server.Close() }) + + ctx, cancel := context.WithCancelCause(t.Context()) + defer cancel(nil) + go func() { + <-loaded + // Give the client time to read the answer, so the exit lands in the wait for uffd ready. + time.Sleep(100 * time.Millisecond) + cancel(fmt.Errorf("uffd process exited: failed to wrap memfd: mmap memfd: %w", syscall.ENOMEM)) + }() + + // uffdReady is never closed: the handler exits before it is ready. + err = newApiClient(socket).loadSnapshot(ctx, dir+"/uffd.sock", make(chan struct{}), stubSnapfile(dir+"/snapfile"), false, false, "", "team-1") + require.ErrorIs(t, err, syscall.ENOMEM) +} diff --git a/packages/orchestrator/pkg/server/memory_refusal_test.go b/packages/orchestrator/pkg/server/memory_refusal_test.go new file mode 100644 index 0000000000..0725624ba0 --- /dev/null +++ b/packages/orchestrator/pkg/server/memory_refusal_test.go @@ -0,0 +1,35 @@ +package server + +import ( + "errors" + "fmt" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// A memfd mmap out of hugepages is capacity that frees up, so it is refused +// for placement to retry: a killed sandbox keeps its pages until its +// Firecracker exits, after Delete returned. Any other error is left to the caller. +func TestMemoryRefusal(t *testing.T) { + t.Parallel() + + // The chain a create right after a kill returns. + uffdExit := fmt.Errorf("uffd process exited: %w", errors.Join(fmt.Errorf("failed to wrap memfd: %w", fmt.Errorf("mmap memfd: %w", unix.ENOMEM)))) + load := &url.Error{Op: "Put", URL: "http://localhost/snapshot/load", Err: uffdExit} + err := memoryRefusal(fmt.Errorf("failed to start FC: %w", fmt.Errorf("error loading snapshot: %w", load))) + require.Error(t, err) + + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.ResourceExhausted, st.Code()) + assert.Contains(t, st.Message(), "mmap memfd: cannot allocate memory") + + assert.NoError(t, memoryRefusal(errors.New("mmap memfd: cannot allocate memory")), "only the errno counts, not the text") + assert.NoError(t, memoryRefusal(errors.New("envd did not start"))) +} diff --git a/packages/orchestrator/pkg/server/sandboxes.go b/packages/orchestrator/pkg/server/sandboxes.go index bd2e5cfeee..59d0b65c8a 100644 --- a/packages/orchestrator/pkg/server/sandboxes.go +++ b/packages/orchestrator/pkg/server/sandboxes.go @@ -11,6 +11,7 @@ import ( "net/http" "slices" "sync" + "syscall" "time" "github.com/google/uuid" @@ -452,6 +453,18 @@ func (s *Server) Create(ctx context.Context, req *orchestrator.SandboxCreateRequ return nil, refusal } + if refusal := memoryRefusal(err); refusal != nil { + logger.L().Warn(ctx, "create refused: node out of memory", zap.Error(err), logger.WithSandboxID(runtime.SandboxID)) + // Placement retries this node with the same sandbox ID within a + // millisecond, so free the ID before answering. The rollback holds + // only the template pin here, so it is quick. + if cleanupErr := rollback.Run(ctx); cleanupErr != nil { + telemetry.ReportCriticalError(ctx, "failed to clean up sandbox start", cleanupErr) + } + rollback = nil + + return nil, refusal + } err = errors.Join(err, context.Cause(ctx)) telemetry.ReportCriticalError(ctx, "failed to create sandbox", err) @@ -3063,6 +3076,19 @@ func volumeRefusal(ctx context.Context, err error) error { return createRefused(ctx, codes.FailedPrecondition, "%s", volumeErr.Error()) } +// memoryRefusal is the create refusal for a node with no memory left to map +// the guest memory, or nil when err is not one. Delete returns before the +// killed sandbox's Firecracker exits, so a create right after a kill can find +// the hugepage pool still held for a few hundred milliseconds. A refusal makes +// placement retry the node with backoff instead of failing the create. +func memoryRefusal(err error) error { + if !errors.Is(err, syscall.ENOMEM) { + return nil + } + + return status.Errorf(codes.ResourceExhausted, "not enough memory on the node to start the sandbox, please retry: %s", err) +} + // filesystemOnlyDisabledStatus is the flag refusal with a typed detail, so the // API can answer it as its own pre-flight does instead of a generic failure // when the two flag evaluations disagree.