From c94b42332b5a1e830e5b198d57895c9ca4c52afb Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Tue, 16 Dec 2025 10:54:56 -0800 Subject: [PATCH] cri: emit warning for concurrent CreateContainer We have existing detection for concurrent CreateContainer requests, but the error message is unclear and there is no warning in containerd logs. This change adds a warning and clarifies the error message. Signed-off-by: Samuel Karp --- internal/cri/server/container_create.go | 25 ++++++++++++++++--------- internal/registrar/registrar.go | 22 ++++++++++++++++++++-- internal/registrar/registrar_test.go | 12 ++++++++++-- 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/internal/cri/server/container_create.go b/internal/cri/server/container_create.go index 1c1516b5d5..303f60372b 100644 --- a/internal/cri/server/container_create.go +++ b/internal/cri/server/container_create.go @@ -25,6 +25,16 @@ import ( "strings" "time" + "github.com/containerd/log" + "github.com/containerd/platforms" + "github.com/containerd/typeurl/v2" + "github.com/davecgh/go-spew/spew" + imagespec "github.com/opencontainers/image-spec/specs-go/v1" + runtimespec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/opencontainers/selinux/go-selinux" + "github.com/opencontainers/selinux/go-selinux/label" + runtime "k8s.io/cri-api/pkg/apis/runtime/v1" + containerd "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/core/containers" "github.com/containerd/containerd/v2/internal/cri/annotations" @@ -35,18 +45,10 @@ import ( containerstore "github.com/containerd/containerd/v2/internal/cri/store/container" "github.com/containerd/containerd/v2/internal/cri/store/sandbox" "github.com/containerd/containerd/v2/internal/cri/util" + "github.com/containerd/containerd/v2/internal/registrar" "github.com/containerd/containerd/v2/pkg/blockio" "github.com/containerd/containerd/v2/pkg/oci" "github.com/containerd/containerd/v2/pkg/tracing" - "github.com/containerd/log" - "github.com/containerd/platforms" - "github.com/containerd/typeurl/v2" - "github.com/davecgh/go-spew/spew" - imagespec "github.com/opencontainers/image-spec/specs-go/v1" - runtimespec "github.com/opencontainers/runtime-spec/specs-go" - "github.com/opencontainers/selinux/go-selinux" - "github.com/opencontainers/selinux/go-selinux/label" - runtime "k8s.io/cri-api/pkg/apis/runtime/v1" ) func init() { @@ -94,6 +96,11 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta name := makeContainerName(metadata, sandboxMetadata) log.G(ctx).Debugf("Generated id %q for container %q", id, name) if err = c.containerNameIndex.Reserve(name, id); err != nil { + var resErr *registrar.ReservedErr + if errors.As(err, &resErr) { + log.G(ctx).WithError(err).Warn("possible concurrent CreateContainer request") + return nil, fmt.Errorf("failed to reserve container name %q; check if another CreateContainer request is in progress: %w", name, err) + } return nil, fmt.Errorf("failed to reserve container name %q: %w", name, err) } span.SetAttributes( diff --git a/internal/registrar/registrar.go b/internal/registrar/registrar.go index d365a57307..bf2e6767c6 100644 --- a/internal/registrar/registrar.go +++ b/internal/registrar/registrar.go @@ -38,6 +38,24 @@ func NewRegistrar() *Registrar { } } +type ReservedErr struct { + Field string + Name string + Key string +} + +func (e *ReservedErr) Error() string { + switch e.Field { + case "name": + return fmt.Sprintf("name %q is reserved for %q", e.Name, e.Key) + case "key": + return fmt.Sprintf("key %q is reserved for %q", e.Key, e.Name) + } + return fmt.Sprintf("name %q is reserved for %q", e.Name, e.Key) +} + +func (e *ReservedErr) Conflict() {} + // Reserve registers a name<->key mapping, name or key must not // be empty. // Reserve is idempotent. @@ -54,14 +72,14 @@ func (r *Registrar) Reserve(name, key string) error { if k, exists := r.nameToKey[name]; exists { if k != key { - return fmt.Errorf("name %q is reserved for %q", name, k) + return &ReservedErr{Field: "name", Name: name, Key: k} } return nil } if n, exists := r.keyToName[key]; exists { if n != name { - return fmt.Errorf("key %q is reserved for %q", key, n) + return &ReservedErr{Field: "key", Name: n, Key: key} } return nil } diff --git a/internal/registrar/registrar_test.go b/internal/registrar/registrar_test.go index 318bfa2fd9..c1a008183b 100644 --- a/internal/registrar/registrar_test.go +++ b/internal/registrar/registrar_test.go @@ -25,6 +25,8 @@ import ( func TestRegistrar(t *testing.T) { r := NewRegistrar() assert := assertlib.New(t) + var err error + var resErr *ReservedErr t.Logf("should be able to reserve a name<->key mapping") assert.NoError(r.Reserve("test-name-1", "test-id-1")) @@ -36,8 +38,14 @@ func TestRegistrar(t *testing.T) { assert.NoError(r.Reserve("test-name-1", "test-id-1")) t.Logf("should not be able to reserve conflict name<->key mapping") - assert.Error(r.Reserve("test-name-1", "test-id-conflict")) - assert.Error(r.Reserve("test-name-conflict", "test-id-2")) + err = r.Reserve("test-name-1", "test-id-conflict") + assert.Error(err) + assert.ErrorAs(err, &resErr) + assert.ErrorContains(resErr, `name "test-name-1" is reserved for "test-id-1"`) + err = r.Reserve("test-name-conflict", "test-id-2") + assert.Error(err) + assert.ErrorAs(err, &resErr) + assert.ErrorContains(resErr, `key "test-id-2" is reserved for "test-name-2"`) t.Logf("should be able to release name<->key mapping by key") r.ReleaseByKey("test-id-1")