Skip to content

Commit a76eb69

Browse files
samuelkarpk8s-infra-cherrypick-robot
samuelkarp
authored and
k8s-infra-cherrypick-robot
committed
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 <[email protected]>
1 parent 683ccda commit a76eb69

3 files changed

Lines changed: 46 additions & 13 deletions

File tree

‎internal/cri/server/container_create.go‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,16 @@ import (
2525
"strings"
2626
"time"
2727

28+
"github.com/containerd/log"
29+
"github.com/containerd/platforms"
30+
"github.com/containerd/typeurl/v2"
31+
"github.com/davecgh/go-spew/spew"
32+
imagespec "github.com/opencontainers/image-spec/specs-go/v1"
33+
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
34+
"github.com/opencontainers/selinux/go-selinux"
35+
"github.com/opencontainers/selinux/go-selinux/label"
36+
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
37+
2838
containerd "github.com/containerd/containerd/v2/client"
2939
"github.com/containerd/containerd/v2/core/containers"
3040
"github.com/containerd/containerd/v2/internal/cri/annotations"
@@ -35,18 +45,10 @@ import (
3545
containerstore "github.com/containerd/containerd/v2/internal/cri/store/container"
3646
"github.com/containerd/containerd/v2/internal/cri/store/sandbox"
3747
"github.com/containerd/containerd/v2/internal/cri/util"
48+
"github.com/containerd/containerd/v2/internal/registrar"
3849
"github.com/containerd/containerd/v2/pkg/blockio"
3950
"github.com/containerd/containerd/v2/pkg/oci"
4051
"github.com/containerd/containerd/v2/pkg/tracing"
41-
"github.com/containerd/log"
42-
"github.com/containerd/platforms"
43-
"github.com/containerd/typeurl/v2"
44-
"github.com/davecgh/go-spew/spew"
45-
imagespec "github.com/opencontainers/image-spec/specs-go/v1"
46-
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
47-
"github.com/opencontainers/selinux/go-selinux"
48-
"github.com/opencontainers/selinux/go-selinux/label"
49-
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
5052
)
5153

5254
func init() {
@@ -94,6 +96,11 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
9496
name := makeContainerName(metadata, sandboxMetadata)
9597
log.G(ctx).Debugf("Generated id %q for container %q", id, name)
9698
if err = c.containerNameIndex.Reserve(name, id); err != nil {
99+
var resErr *registrar.ReservedErr
100+
if errors.As(err, &resErr) {
101+
log.G(ctx).WithError(err).Warn("possible concurrent CreateContainer request")
102+
return nil, fmt.Errorf("failed to reserve container name %q; check if another CreateContainer request is in progress: %w", name, err)
103+
}
97104
return nil, fmt.Errorf("failed to reserve container name %q: %w", name, err)
98105
}
99106
span.SetAttributes(

‎internal/registrar/registrar.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,24 @@ func NewRegistrar() *Registrar {
3838
}
3939
}
4040

41+
type ReservedErr struct {
42+
Field string
43+
Name string
44+
Key string
45+
}
46+
47+
func (e *ReservedErr) Error() string {
48+
switch e.Field {
49+
case "name":
50+
return fmt.Sprintf("name %q is reserved for %q", e.Name, e.Key)
51+
case "key":
52+
return fmt.Sprintf("key %q is reserved for %q", e.Key, e.Name)
53+
}
54+
return fmt.Sprintf("name %q is reserved for %q", e.Name, e.Key)
55+
}
56+
57+
func (e *ReservedErr) Conflict() {}
58+
4159
// Reserve registers a name<->key mapping, name or key must not
4260
// be empty.
4361
// Reserve is idempotent.
@@ -54,14 +72,14 @@ func (r *Registrar) Reserve(name, key string) error {
5472

5573
if k, exists := r.nameToKey[name]; exists {
5674
if k != key {
57-
return fmt.Errorf("name %q is reserved for %q", name, k)
75+
return &ReservedErr{Field: "name", Name: name, Key: k}
5876
}
5977
return nil
6078
}
6179

6280
if n, exists := r.keyToName[key]; exists {
6381
if n != name {
64-
return fmt.Errorf("key %q is reserved for %q", key, n)
82+
return &ReservedErr{Field: "key", Name: n, Key: key}
6583
}
6684
return nil
6785
}

‎internal/registrar/registrar_test.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import (
2525
func TestRegistrar(t *testing.T) {
2626
r := NewRegistrar()
2727
assert := assertlib.New(t)
28+
var err error
29+
var resErr *ReservedErr
2830

2931
t.Logf("should be able to reserve a name<->key mapping")
3032
assert.NoError(r.Reserve("test-name-1", "test-id-1"))
@@ -36,8 +38,14 @@ func TestRegistrar(t *testing.T) {
3638
assert.NoError(r.Reserve("test-name-1", "test-id-1"))
3739

3840
t.Logf("should not be able to reserve conflict name<->key mapping")
39-
assert.Error(r.Reserve("test-name-1", "test-id-conflict"))
40-
assert.Error(r.Reserve("test-name-conflict", "test-id-2"))
41+
err = r.Reserve("test-name-1", "test-id-conflict")
42+
assert.Error(err)
43+
assert.ErrorAs(err, &resErr)
44+
assert.ErrorContains(resErr, `name "test-name-1" is reserved for "test-id-1"`)
45+
err = r.Reserve("test-name-conflict", "test-id-2")
46+
assert.Error(err)
47+
assert.ErrorAs(err, &resErr)
48+
assert.ErrorContains(resErr, `key "test-id-2" is reserved for "test-name-2"`)
4149

4250
t.Logf("should be able to release name<->key mapping by key")
4351
r.ReleaseByKey("test-id-1")

0 commit comments

Comments
 (0)