From 3d53430fe14eb76849a6c997d60b21a9f95c19ed Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Mon, 13 Jan 2025 21:55:52 -0800 Subject: [PATCH] Move CDI device spec out of the OCI package The CDI device injection spec opt was mistakenly added to the OCI package which brought in an unintended dependency on CDI and its transitive dependencies. Signed-off-by: Derek McGowan (cherry picked from commit e20f7f4a2425c005d85855abfd4556d7b4ccbf87) Signed-off-by: Derek McGowan --- cmd/ctr/commands/run/run_unix.go | 8 +++-- internal/cri/opts/spec_linux.go | 8 +++-- pkg/cdi/oci_opt.go | 56 ++++++++++++++++++++++++++++++++ pkg/oci/spec_opts.go | 42 +++++++----------------- 4 files changed, 78 insertions(+), 36 deletions(-) create mode 100644 pkg/cdi/oci_opt.go diff --git a/cmd/ctr/commands/run/run_unix.go b/cmd/ctr/commands/run/run_unix.go index af58ed1c3..396bdfec3 100644 --- a/cmd/ctr/commands/run/run_unix.go +++ b/cmd/ctr/commands/run/run_unix.go @@ -27,6 +27,9 @@ import ( "strconv" "strings" + "github.com/containerd/log" + "github.com/containerd/platforms" + containerd "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/cmd/ctr/commands" "github.com/containerd/containerd/v2/contrib/apparmor" @@ -34,9 +37,8 @@ import ( "github.com/containerd/containerd/v2/contrib/seccomp" "github.com/containerd/containerd/v2/core/containers" "github.com/containerd/containerd/v2/core/snapshots" + cdispec "github.com/containerd/containerd/v2/pkg/cdi" "github.com/containerd/containerd/v2/pkg/oci" - "github.com/containerd/log" - "github.com/containerd/platforms" "github.com/intel/goresctrl/pkg/blockio" "github.com/opencontainers/runtime-spec/specs-go" @@ -358,7 +360,7 @@ func NewContainer(ctx context.Context, client *containerd.Client, cliContext *cl if len(cdiDeviceIDs) > 0 { opts = append(opts, withStaticCDIRegistry()) } - opts = append(opts, oci.WithCDIDevices(cdiDeviceIDs...)) + opts = append(opts, cdispec.WithCDIDevices(cdiDeviceIDs...)) rootfsPropagation := cliContext.String("rootfs-propagation") if rootfsPropagation != "" { diff --git a/internal/cri/opts/spec_linux.go b/internal/cri/opts/spec_linux.go index d78c99c30..712c080cf 100644 --- a/internal/cri/opts/spec_linux.go +++ b/internal/cri/opts/spec_linux.go @@ -31,9 +31,11 @@ import ( runtime "k8s.io/cri-api/pkg/apis/runtime/v1" "tags.cncf.io/container-device-interface/pkg/cdi" - "github.com/containerd/containerd/v2/core/containers" - "github.com/containerd/containerd/v2/pkg/oci" "github.com/containerd/log" + + "github.com/containerd/containerd/v2/core/containers" + cdispec "github.com/containerd/containerd/v2/pkg/cdi" + "github.com/containerd/containerd/v2/pkg/oci" ) // Linux dependent OCI spec opts. @@ -172,6 +174,6 @@ func WithCDI(annotations map[string]string, CDIDevices []*runtime.CDIDevice) oci log.G(ctx).Debug("Passing CDI devices as annotations will be deprecated soon, please use CRI CDIDevices instead") } - return oci.WithCDIDevices(devices...)(ctx, client, c, s) + return cdispec.WithCDIDevices(devices...)(ctx, client, c, s) } } diff --git a/pkg/cdi/oci_opt.go b/pkg/cdi/oci_opt.go new file mode 100644 index 000000000..0fd6f28e9 --- /dev/null +++ b/pkg/cdi/oci_opt.go @@ -0,0 +1,56 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package cdi + +import ( + "context" + "fmt" + + "github.com/containerd/log" + "tags.cncf.io/container-device-interface/pkg/cdi" + + "github.com/containerd/containerd/v2/core/containers" + "github.com/containerd/containerd/v2/pkg/oci" +) + +// WithCDIDevices injects the requested CDI devices into the OCI specification. +func WithCDIDevices(devices ...string) oci.SpecOpts { + return func(ctx context.Context, _ oci.Client, c *containers.Container, s *oci.Spec) error { + if len(devices) == 0 { + return nil + } + + if err := cdi.Refresh(); err != nil { + // We don't consider registry refresh failure a fatal error. + // For instance, a dynamically generated invalid CDI Spec file for + // any particular vendor shouldn't prevent injection of devices of + // different vendors. CDI itself knows better and it will fail the + // injection if necessary. + log.G(ctx).Warnf("CDI registry refresh failed: %v", err) + } + + if _, err := cdi.InjectDevices(s, devices...); err != nil { + return fmt.Errorf("CDI device injection failed: %w", err) + } + + // One crucial thing to keep in mind is that CDI device injection + // might add OCI Spec environment variables, hooks, and mounts as + // well. Therefore it is important that none of the corresponding + // OCI Spec fields are reset up in the call stack once we return. + return nil + } +} diff --git a/pkg/oci/spec_opts.go b/pkg/oci/spec_opts.go index 5101c63bb..7ea08b71f 100644 --- a/pkg/oci/spec_opts.go +++ b/pkg/oci/spec_opts.go @@ -28,18 +28,17 @@ import ( "strconv" "strings" + "github.com/containerd/continuity/fs" + "github.com/containerd/platforms" + "github.com/moby/sys/user" + v1 "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/opencontainers/runtime-spec/specs-go" + "github.com/containerd/containerd/v2/core/containers" "github.com/containerd/containerd/v2/core/content" "github.com/containerd/containerd/v2/core/images" "github.com/containerd/containerd/v2/core/mount" "github.com/containerd/containerd/v2/pkg/namespaces" - "github.com/containerd/continuity/fs" - "github.com/containerd/log" - "github.com/containerd/platforms" - "github.com/moby/sys/user" - v1 "github.com/opencontainers/image-spec/specs-go/v1" - "github.com/opencontainers/runtime-spec/specs-go" - "tags.cncf.io/container-device-interface/pkg/cdi" ) // SpecOpts sets spec specific information to a newly generated OCI spec @@ -1644,30 +1643,13 @@ func WithWindowsNetworkNamespace(ns string) SpecOpts { } } -// WithCDIDevices injects the requested CDI devices into the OCI specification. +// WithCDIDevices should be used from the cdi package. This version is used for +// compatibility to point to the non-deprecated version but will return an error if used. +// This function will be removed in 2.1. +// +// Deprecated: Use [github.com/containerd/containerd/v2/pkg/cdi.WithCDIDevices] instead. func WithCDIDevices(devices ...string) SpecOpts { return func(ctx context.Context, _ Client, c *containers.Container, s *Spec) error { - if len(devices) == 0 { - return nil - } - - if err := cdi.Refresh(); err != nil { - // We don't consider registry refresh failure a fatal error. - // For instance, a dynamically generated invalid CDI Spec file for - // any particular vendor shouldn't prevent injection of devices of - // different vendors. CDI itself knows better and it will fail the - // injection if necessary. - log.G(ctx).Warnf("CDI registry refresh failed: %v", err) - } - - if _, err := cdi.InjectDevices(s, devices...); err != nil { - return fmt.Errorf("CDI device injection failed: %w", err) - } - - // One crucial thing to keep in mind is that CDI device injection - // might add OCI Spec environment variables, hooks, and mounts as - // well. Therefore it is important that none of the corresponding - // OCI Spec fields are reset up in the call stack once we return. - return nil + return errors.New("must use cdi package for CDI device injection") } }