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.