Merge pull request #5494 from nalind/cdi-dirs

Builder.cdiSetupDevicesInSpecdefConfig(): use configured CDI dirs

go.mod

@@ -10,7 +10,7 @@ go 1.20 // *****  ATTENTION  WARNING  CAUTION  DANGER  ******
 //         different distros and distro-versions build from
 //         this code, golang version consistency is
 //         desireable.  After manually updating to 1.21, a
-//         `toolchain` specificication should be added to pin
+//         `toolchain` specification should be added to pin
 //         the version and block auto-updates.  This does not
 //         block any future changes to the `go` value.
 //         Ref: Upstream discussion:
@@ -55,7 +55,7 @@ require (
 	golang.org/x/sys v0.19.0
 	golang.org/x/term v0.19.0
 	sigs.k8s.io/yaml v1.4.0
-	tags.cncf.io/container-device-interface v0.7.1
+	tags.cncf.io/container-device-interface v0.7.2
 )
 
 require (

go.sum

@@ -656,7 +656,7 @@ k8s.io/klog v1.0.0 h1:Pt+yjF5aB1xDSVbau4VsWe+dQNzA0qv1LlXdC2dF6Q8=
 k8s.io/klog v1.0.0/go.mod h1:4Bi6QPql/J/LkTDqv7R/cd3hPo4k2DG6Ptcz060Ez5I=
 sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E=
 sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY=
-tags.cncf.io/container-device-interface v0.7.1 h1:MATNCbAD1su9U6zwQe5BrQ2vGGp1GBayD70bYaxYCNE=
-tags.cncf.io/container-device-interface v0.7.1/go.mod h1:h1JVuOqTQVORp8DziaWKUCDNzAmN+zeCbqbqD30D0ZQ=
+tags.cncf.io/container-device-interface v0.7.2 h1:MLqGnWfOr1wB7m08ieI4YJ3IoLKKozEnnNYBtacDPQU=
+tags.cncf.io/container-device-interface v0.7.2/go.mod h1:Xb1PvXv2BhfNb3tla4r9JL129ck1Lxv9KuU6eVOfKto=
 tags.cncf.io/container-device-interface/specs-go v0.7.0 h1:w/maMGVeLP6TIQJVYT5pbqTi8SCw/iHZ+n4ignuGHqg=
 tags.cncf.io/container-device-interface/specs-go v0.7.0/go.mod h1:hMAwAbMZyBLdmYqWgYcKH0F/yctNpV3P35f+/088A80=

run_linux.go

@@ -47,6 +47,7 @@ import (
 	"golang.org/x/exp/slices"
 	"golang.org/x/sys/unix"
 	"tags.cncf.io/container-device-interface/pkg/cdi"
+	"tags.cncf.io/container-device-interface/pkg/parser"
 )
 
 var (
@@ -69,40 +70,55 @@ func setChildProcess() error {
 }
 
 func (b *Builder) cdiSetupDevicesInSpec(deviceSpecs []string, configDir string, spec *specs.Spec) ([]string, error) {
-	leftoverDevices := deviceSpecs
-	registry, err := cdi.NewCache()
+	var configDirs []string
+	defConfig, err := config.Default()
 	if err != nil {
-		return nil, fmt.Errorf("creating CDI registry: %w", err)
+		return nil, fmt.Errorf("failed to get container config: %w", err)
 	}
-	var configDirs []string
+	// The CDI cache prioritizes entries from directories that are later in
+	// the list of ones it scans, so start with our general config, then
+	// append values passed to us through API layers.
+	configDirs = slices.Clone(defConfig.Engine.CdiSpecDirs.Get())
 	if b.CDIConfigDir != "" {
 		configDirs = append(configDirs, b.CDIConfigDir)
 	}
 	if configDir != "" {
 		configDirs = append(configDirs, configDir)
 	}
-	// TODO: CdiSpecDirs will be in containers/common v0.59.0 or later?
-	// defConfig, err := config.Default()
-	// if err != nil {
-	//	return nil, fmt.Errorf("failed to get container config: %w", err)
-	// }
-	// configDirs = append(configDirs, defConfig.Engine.CdiSpecDirs.Get()...)
-	if len(configDirs) > 0 {
-		if err := registry.Configure(cdi.WithSpecDirs(configDirs...)); err != nil {
-			return nil, fmt.Errorf("CDI registry ignored configured directories %v: %w", configDirs, err)
+	if len(configDirs) == 0 {
+		// No directories to scan for CDI configuration means that CDI
+		// won't have any details for setting up any devices, so we
+		// don't need to be doing anything here.
+		return deviceSpecs, nil
+	}
+	var qualifiedDeviceSpecs, unqualifiedDeviceSpecs []string
+	for _, deviceSpec := range deviceSpecs {
+		if parser.IsQualifiedName(deviceSpec) {
+			qualifiedDeviceSpecs = append(qualifiedDeviceSpecs, deviceSpec)
+		} else {
+			unqualifiedDeviceSpecs = append(unqualifiedDeviceSpecs, deviceSpec)
 		}
 	}
-	if err := registry.Refresh(); err != nil {
-		logrus.Warnf("CDI registry refresh: %v", err)
+	if len(qualifiedDeviceSpecs) == 0 {
+		// None of the specified devices were in the form that would be
+		// handled by CDI, so we don't need to do anything here.
+		return deviceSpecs, nil
+	}
+	if err := cdi.Configure(cdi.WithSpecDirs(configDirs...)); err != nil {
+		return nil, fmt.Errorf("CDI default registry ignored configured directories %v: %w", configDirs, err)
+	}
+	leftoverDevices := slices.Clone(deviceSpecs)
+	if err := cdi.Refresh(); err != nil {
+		logrus.Warnf("CDI default registry refresh: %v", err)
 	} else {
-		leftoverDevices, err = registry.InjectDevices(spec, deviceSpecs...)
+		leftoverDevices, err = cdi.InjectDevices(spec, qualifiedDeviceSpecs...)
 		if err != nil {
-			logrus.Debugf("CDI device injection: %v, unresolved list %v", err, leftoverDevices)
+			return nil, fmt.Errorf("CDI device injection (leftover devices: %v): %w", leftoverDevices, err)
 		}
 	}
 	removed := slices.DeleteFunc(slices.Clone(deviceSpecs), func(t string) bool { return slices.Contains(leftoverDevices, t) })
-	logrus.Debugf("CDI taking care of devices %v, leaving devices %v", removed, leftoverDevices)
-	return leftoverDevices, nil
+	logrus.Debugf("CDI taking care of devices %v, leaving devices %v, skipped %v", removed, leftoverDevices, unqualifiedDeviceSpecs)
+	return append(leftoverDevices, unqualifiedDeviceSpecs...), nil
 }
 
 // Extract the device list so that we can still try to make it work if

tests/bud/cdi/Dockerfile

@@ -1,3 +1,4 @@
 FROM busybox
 RUN mkdir /etc/cdi
 RUN cp /dev/containers-cdi.yaml /etc/cdi/containers-cdi.yaml
+RUN wc /dev/outsidenull

tests/bud/cdi/containers-cdi.yaml

@@ -1,5 +1,5 @@
 ---
-cdiVersion: 0.5.0
+cdiVersion: 0.7.0
 devices:
   - name: all
     containerEdits:

tests/cdi.bats

@@ -12,7 +12,7 @@ load helpers
   echo === Begin CDI configuration in $cdidir/containers-cdi.yaml ===
   cat $cdidir/containers-cdi.yaml
   echo === End CDI configuration ===
-  run_buildah build $WITH_POLICY_JSON --cdi-config-dir=$cdidir --security-opt label=disable --device=containers.github.io/sample=all $BUDFILES/cdi
+  run_buildah build $WITH_POLICY_JSON --cdi-config-dir=$cdidir --security-opt label=disable --device=containers.github.io/sample=all --device=/dev/null:/dev/outsidenull:rwm $BUDFILES/cdi
 }
 
 @test "from with CDI" {
@@ -25,9 +25,9 @@ load helpers
   echo === Begin CDI configuration in $cdidir/containers-cdi.yaml ===
   cat $cdidir/containers-cdi.yaml
   echo === End CDI configuration ===
-  run_buildah from $WITH_POLICY_JSON --security-opt label=disable --cdi-config-dir=$cdidir --device=containers.github.io/sample=all busybox
+  run_buildah from $WITH_POLICY_JSON --security-opt label=disable --cdi-config-dir=$cdidir --device=containers.github.io/sample=all --device=/dev/null:/dev/outsidenull:rwm busybox
   cid="$output"
-  run_buildah run "$cid" cat /dev/containers-cdi.yaml
+  run_buildah run "$cid" cat /dev/containers-cdi.yaml /dev/outsidenull
 }
 
 @test "run with CDI" {

vendor/modules.txt

@@ -998,7 +998,7 @@ k8s.io/klog
 ## explicit; go 1.12
 sigs.k8s.io/yaml
 sigs.k8s.io/yaml/goyaml.v2
-# tags.cncf.io/container-device-interface v0.7.1
+# tags.cncf.io/container-device-interface v0.7.2
 ## explicit; go 1.20
 tags.cncf.io/container-device-interface/internal/validation
 tags.cncf.io/container-device-interface/internal/validation/k8s