From b0052d94a643c6da60058b262c4dc61e92c4dfa7 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 May 2025 00:39:39 +0200 Subject: [PATCH] pkg/oci: prevent panic for some platform-specific options Some of these options are designed to be a no-op when used on a Spec that doesn't match the platform for the option. However, if the given plaform was not present, they would panic. This patch: - Adds an early-return for options that are only applied on a specific platform. - Update the GoDoc for these functions to describe they're a no-op on other platforms. - Adds some rudimentary unit-tests to verify their behavior. Signed-off-by: Sebastiaan van Stijn --- pkg/oci/spec_opts.go | 60 ++++++++++--- pkg/oci/spec_opts_test.go | 176 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 225 insertions(+), 11 deletions(-) diff --git a/pkg/oci/spec_opts.go b/pkg/oci/spec_opts.go index 7e0388882c..636ed43029 100644 --- a/pkg/oci/spec_opts.go +++ b/pkg/oci/spec_opts.go @@ -1483,9 +1483,12 @@ func WithWindowsDevice(idType, id string) SpecOpts { } } -// WithMemorySwap sets the container's swap in bytes +// WithMemorySwap sets the container's swap in bytes. It is a no-op on non-Linux specs. func WithMemorySwap(swap int64) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setResources(s) if s.Linux.Resources.Memory == nil { s.Linux.Resources.Memory = &specs.LinuxMemory{} @@ -1495,9 +1498,12 @@ func WithMemorySwap(swap int64) SpecOpts { } } -// WithPidsLimit sets the container's pid limit or maximum +// WithPidsLimit sets the container's pid limit or maximum. It is a no-op on non-Linux specs. func WithPidsLimit(limit int64) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setResources(s) if s.Linux.Resources.Pids == nil { s.Linux.Resources.Pids = &specs.LinuxPids{} @@ -1507,45 +1513,61 @@ func WithPidsLimit(limit int64) SpecOpts { } } -// WithBlockIO sets the container's blkio parameters +// WithBlockIO sets the container's blkio parameters. It is a no-op on non-Linux specs. func WithBlockIO(blockio *specs.LinuxBlockIO) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setResources(s) s.Linux.Resources.BlockIO = blockio return nil } } -// WithCPUShares sets the container's cpu shares +// WithCPUShares sets the container's cpu shares. It is a no-op on non-Linux specs. func WithCPUShares(shares uint64) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setCPU(s) s.Linux.Resources.CPU.Shares = &shares return nil } } -// WithCPUs sets the container's cpus/cores for use by the container +// WithCPUs sets the container's cpus/cores for use by the container. It is a no-op on non-Linux specs. func WithCPUs(cpus string) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setCPU(s) s.Linux.Resources.CPU.Cpus = cpus return nil } } -// WithCPUsMems sets the container's cpu mems for use by the container +// WithCPUsMems sets the container's cpu mems for use by the container. It is a no-op on non-Linux specs. func WithCPUsMems(mems string) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setCPU(s) s.Linux.Resources.CPU.Mems = mems return nil } } -// WithCPUCFS sets the container's Completely fair scheduling (CFS) quota and period +// WithCPUCFS sets the container's Completely fair scheduling (CFS) quota and period. +// It is a no-op on non-Linux specs. func WithCPUCFS(quota int64, period uint64) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setCPU(s) s.Linux.Resources.CPU.Quota = "a s.Linux.Resources.CPU.Period = &period @@ -1553,9 +1575,12 @@ func WithCPUCFS(quota int64, period uint64) SpecOpts { } } -// WithCPUBurst sets the container's cpu burst +// WithCPUBurst sets the container's cpu burst. It is a no-op on non-Linux specs. func WithCPUBurst(burst uint64) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setCPU(s) s.Linux.Resources.CPU.Burst = &burst return nil @@ -1563,8 +1588,12 @@ func WithCPUBurst(burst uint64) SpecOpts { } // WithCPURT sets the container's realtime scheduling (RT) runtime and period. +// It is a no-op on non-Linux specs. func WithCPURT(runtime int64, period uint64) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { + if s.Linux == nil { + return nil + } setCPU(s) s.Linux.Resources.CPU.RealtimeRuntime = &runtime s.Linux.Resources.CPU.RealtimePeriod = &period @@ -1590,9 +1619,12 @@ func WithRdt(closID, l3CacheSchema, memBwSchema string) SpecOpts { } // WithWindowsCPUCount sets the `Windows.Resources.CPU.Count` section to the -// `count` specified. +// `count` specified. It is a no-op for non-Windows specs. func WithWindowsCPUCount(count uint64) SpecOpts { return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { + if s.Windows == nil { + return nil + } setCPUWindows(s) s.Windows.Resources.CPU.Count = &count return nil @@ -1600,9 +1632,12 @@ func WithWindowsCPUCount(count uint64) SpecOpts { } // WithWindowsCPUShares sets the `Windows.Resources.CPU.Shares` section to the -// `shares` specified. +// `shares` specified. It is a no-op for non-Windows specs. func WithWindowsCPUShares(shares uint16) SpecOpts { return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { + if s.Windows == nil { + return nil + } setCPUWindows(s) s.Windows.Resources.CPU.Shares = &shares return nil @@ -1610,9 +1645,12 @@ func WithWindowsCPUShares(shares uint16) SpecOpts { } // WithWindowsCPUMaximum sets the `Windows.Resources.CPU.Maximum` section to the -// `max` specified. +// `max` specified. It is a no-op for non-Windows specs. func WithWindowsCPUMaximum(max uint16) SpecOpts { return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { + if s.Windows == nil { + return nil + } setCPUWindows(s) s.Windows.Resources.CPU.Maximum = &max return nil diff --git a/pkg/oci/spec_opts_test.go b/pkg/oci/spec_opts_test.go index 15a253221a..876d90cf01 100644 --- a/pkg/oci/spec_opts_test.go +++ b/pkg/oci/spec_opts_test.go @@ -42,6 +42,12 @@ import ( "github.com/containerd/errdefs" ) +var emptySpecs = map[string]Spec{ + "empty": {}, + "linux": {Linux: &specs.Linux{}}, + "windows": {Windows: &specs.Windows{}}, +} + type blob []byte func (b blob) ReadAt(p []byte, off int64) (int, error) { @@ -416,6 +422,131 @@ func TestWithMemoryLimit(t *testing.T) { } } +func TestWithMemorySwap(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := int64(123) + err := WithMemorySwap(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expected, *spec.Linux.Resources.Memory.Swap) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + +func TestWithPidsLimit(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := int64(123) + err := WithPidsLimit(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expected, spec.Linux.Resources.Pids.Limit) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + +func TestWithBlockIO(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + v := uint16(123) + expected := &specs.LinuxBlockIO{ + Weight: &v, + LeafWeight: &v, + } + err := WithBlockIO(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expected, spec.Linux.Resources.BlockIO) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + +func TestWithCPUShares(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := uint64(123) + err := WithCPUShares(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expected, *spec.Linux.Resources.CPU.Shares) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + +func TestWithCPUs(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := "0,1" + err := WithCPUs(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expected, spec.Linux.Resources.CPU.Cpus) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + +func TestWithCPUsMems(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := "0,1" + err := WithCPUsMems(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expected, spec.Linux.Resources.CPU.Mems) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + +func TestWithCPUBurst(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := uint64(123) + err := WithCPUBurst(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expected, *spec.Linux.Resources.CPU.Burst) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + +func TestWithCPURT(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expectedRT, expectedPeriod := int64(123), uint64(456) + err := WithCPURT(expectedRT, expectedPeriod)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "linux" { + assert.Equal(t, expectedRT, *spec.Linux.Resources.CPU.RealtimeRuntime) + assert.Equal(t, expectedPeriod, *spec.Linux.Resources.CPU.RealtimePeriod) + } else { + assert.Empty(t, spec.Linux, "should not have modified spec") + } + }) + } +} + func isEqualStringArrays(values, expected []string) bool { if len(values) != len(expected) { return false @@ -770,3 +901,48 @@ func TestWithWindowsDevice(t *testing.T) { }) } } + +func TestWithWindowsCPUCount(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := uint64(123) + err := WithWindowsCPUCount(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "windows" { + assert.Equal(t, expected, *spec.Windows.Resources.CPU.Count) + } else { + assert.Empty(t, spec.Windows, "should not have modified spec") + } + }) + } +} + +func TestWithWindowsCPUShares(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := uint16(123) + err := WithWindowsCPUShares(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "windows" { + assert.Equal(t, expected, *spec.Windows.Resources.CPU.Shares) + } else { + assert.Empty(t, spec.Windows, "should not have modified spec") + } + }) + } +} + +func TestWithWindowsCPUMaximum(t *testing.T) { + for name, spec := range emptySpecs { + t.Run(name, func(t *testing.T) { + expected := uint16(123) + err := WithWindowsCPUMaximum(expected)(nil, nil, nil, &spec) + assert.NoError(t, err) + if name == "windows" { + assert.Equal(t, expected, *spec.Windows.Resources.CPU.Maximum) + } else { + assert.Empty(t, spec.Windows, "should not have modified spec") + } + }) + } +}