From f51a2fbfdeaa7495583248709cd46c11dcaef814 Mon Sep 17 00:00:00 2001 From: Eric Mountain Date: Sun, 29 Jun 2025 10:33:50 +0200 Subject: [PATCH 1/3] Test showing RuntimeHandlers in Status() are unordered Signed-off-by: Eric Mountain --- internal/cri/server/status_test.go | 90 ++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/internal/cri/server/status_test.go b/internal/cri/server/status_test.go index efd380b9b1..2db728ec81 100644 --- a/internal/cri/server/status_test.go +++ b/internal/cri/server/status_test.go @@ -17,9 +17,15 @@ package server import ( + "context" + "reflect" "testing" + "unsafe" "github.com/containerd/containerd/api/services/introspection/v1" + containerd "github.com/containerd/containerd/v2/client" + coreintrospection "github.com/containerd/containerd/v2/core/introspection" + "github.com/google/uuid" "github.com/stretchr/testify/assert" runtime "k8s.io/cri-api/pkg/apis/runtime/v1" ) @@ -48,3 +54,87 @@ func TestRuntimeConditionContainerdHasNoDeprecationWarnings(t *testing.T) { Status: true, }, cond) } + +// fakeIntrospectionService is a minimal stub that implements the +// coreintrospection.Service. We need this because criService.Status() +// invokes the client.IntrospectionService() method. +type fakeIntrospectionService struct{} + +var _ coreintrospection.Service = fakeIntrospectionService{} + +func (fakeIntrospectionService) Plugins(ctx context.Context, _ ...string) (*introspection.PluginsResponse, error) { + return &introspection.PluginsResponse{}, nil +} + +func (fakeIntrospectionService) Server(ctx context.Context) (*introspection.ServerResponse, error) { + return &introspection.ServerResponse{}, nil +} + +func (fakeIntrospectionService) PluginInfo(ctx context.Context, _ string, _ string, _ any) (*introspection.PluginInfoResponse, error) { + return &introspection.PluginInfoResponse{}, nil +} + +// newFakeContainerdClient returns a *containerd.Client with a stub +// IntrospectionService injected via reflection. This avoids needing a real +// gRPC connection while satisfying criService.Status(). +func newFakeContainerdClient() *containerd.Client { + c := &containerd.Client{} + sv := reflect.ValueOf(c).Elem().FieldByName("services") + if !sv.IsValid() { + return c + } + f := sv.FieldByName("introspectionService") + if !f.IsValid() { + return c + } + // Make the unexported/private field introspectionService settable + f = reflect.NewAt(f.Type(), unsafe.Pointer(f.UnsafeAddr())).Elem() + f.Set(reflect.ValueOf(fakeIntrospectionService{}).Convert(f.Type())) + return c +} + +// newStatusTestCRIService creates a minimal CRI service for testing +func newStatusTestCRIService() *criService { + return &criService{ + client: newFakeContainerdClient(), + runtimeHandlers: make(map[string]*runtime.RuntimeHandler), + } +} + +// TestStatusRuntimeHandlersOrdering checks that the runtime handlers +// returned by Status() are in the same order every time +func TestStatusRuntimeHandlersOrdering(t *testing.T) { + c := newStatusTestCRIService() + + // Forge many runtime handlers to lower risk of accidental stable + // ordering on consecutive Status() calls + const numHandlers = 100 + handlers := make(map[string]*runtime.RuntimeHandler, numHandlers) + for range numHandlers { + h := &runtime.RuntimeHandler{Name: "random-" + uuid.New().String()} + handlers[h.Name] = h + } + c.runtimeHandlers = handlers + + // Call Status() twice + resp1, err := c.Status(context.Background(), &runtime.StatusRequest{}) + assert.NoError(t, err) + assert.Len(t, resp1.RuntimeHandlers, len(handlers), "Unexpected number of runtime handlers") + + resp2, err := c.Status(context.Background(), &runtime.StatusRequest{}) + assert.NoError(t, err) + assert.Len(t, resp2.RuntimeHandlers, len(handlers), "Unexpected number of runtime handlers") + + // Check runtime handlers are in the same order + sameOrder := true + for i := 0; i < len(resp1.RuntimeHandlers); i++ { + if resp1.RuntimeHandlers[i].Name != resp2.RuntimeHandlers[i].Name { + sameOrder = false + break + } + } + + // The test will fail if the order is the same, showing that the ordering is stable + // In the current implementation, order is not stable + assert.False(t, sameOrder, "RuntimeHandlers order is stable across Status() calls - unexpected") +} From c6ae0819348e7389900ad32c99a843fc17b387df Mon Sep 17 00:00:00 2001 From: Eric Mountain Date: Fri, 27 Jun 2025 16:05:28 +0200 Subject: [PATCH 2/3] CRI: Stable sort for RuntimeHandlers The runtimeHandlers list in the response to `crictl info` has unstable ordering since commit 97eb1cd (underlying switch from list to map) that was shipped in v2.1.0. On Kubernetes nodes this causes the kubelet to update node status subresources every time the order of runtime handlers changes in the status response from containerd. The lieklihood increases with the number of runtime handlers present on nodes. In some clusters this leads to every single node sending a status update every few seconds leading to excessive Kube API server load. This change enforces stable ordering on runtime handler names. Signed-off-by: Eric Mountain --- internal/cri/server/status.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/cri/server/status.go b/internal/cri/server/status.go index a888724458..4e9722dfae 100644 --- a/internal/cri/server/status.go +++ b/internal/cri/server/status.go @@ -23,6 +23,7 @@ import ( "maps" goruntime "runtime" "slices" + "sort" "github.com/containerd/containerd/api/services/introspection/v1" "github.com/containerd/log" @@ -59,9 +60,15 @@ func (c *criService) Status(ctx context.Context, r *runtime.StatusRequest) (*run runtimeCondition, networkCondition, }}, - RuntimeHandlers: slices.Collect(maps.Values(c.runtimeHandlers)), - Features: c.runtimeFeatures, + Features: c.runtimeFeatures, } + + // Ensure stable ordering of runtime handlers in response + resp.RuntimeHandlers = slices.Collect(maps.Values(c.runtimeHandlers)) + sort.SliceStable(resp.RuntimeHandlers, func(i, j int) bool { + return resp.RuntimeHandlers[i].Name < resp.RuntimeHandlers[j].Name + }) + if r.Verbose { configByt, err := json.Marshal(c.config) if err != nil { From eb63b5b4dfadaab5b43080ffb5529209459548db Mon Sep 17 00:00:00 2001 From: Eric Mountain Date: Sun, 29 Jun 2025 19:31:57 +0200 Subject: [PATCH 3/3] Amend runtime handler test for stable order Signed-off-by: Eric Mountain --- internal/cri/server/status_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/cri/server/status_test.go b/internal/cri/server/status_test.go index 2db728ec81..6ab9ffc77f 100644 --- a/internal/cri/server/status_test.go +++ b/internal/cri/server/status_test.go @@ -134,7 +134,6 @@ func TestStatusRuntimeHandlersOrdering(t *testing.T) { } } - // The test will fail if the order is the same, showing that the ordering is stable - // In the current implementation, order is not stable - assert.False(t, sameOrder, "RuntimeHandlers order is stable across Status() calls - unexpected") + // Fail if runtime handlers order varies across calls to Status() + assert.True(t, sameOrder, "RuntimeHandlers order is unstable across Status() calls") }