From d5b2e3c41acd708477164ef7abf64ce998d37ac6 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 12 Apr 2024 10:31:51 -0400 Subject: [PATCH] Builder.cdiSetupDevicesInSpecdefConfig(): use configured CDI dirs Use the directories configured in containers.conf, now that containers/common can tell us what they are, and now that it provides a place to configure defaults for container tools, always override the library's default set, even if it's empty, which means we do nothing. Switch to the default CDI registry instead of the recenty-deprecated non-global one. Signed-off-by: Nalin Dahyabhai --- go.mod | 4 +-- go.sum | 4 +-- run_linux.go | 54 ++++++++++++++++++++----------- tests/bud/cdi/Dockerfile | 1 + tests/bud/cdi/containers-cdi.yaml | 2 +- tests/cdi.bats | 6 ++-- vendor/modules.txt | 2 +- 7 files changed, 45 insertions(+), 28 deletions(-) diff --git a/go.mod b/go.mod index e5e430d08b3..e227d394570 100644 --- a/go.mod +++ b/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 ( diff --git a/go.sum b/go.sum index bc3dedbc5e3..0780b4f174c 100644 --- a/go.sum +++ b/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= diff --git a/run_linux.go b/run_linux.go index 300f6590bfc..14c74af00cb 100644 --- a/run_linux.go +++ b/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 diff --git a/tests/bud/cdi/Dockerfile b/tests/bud/cdi/Dockerfile index be71d538ea3..f303e1a414b 100644 --- a/tests/bud/cdi/Dockerfile +++ b/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 diff --git a/tests/bud/cdi/containers-cdi.yaml b/tests/bud/cdi/containers-cdi.yaml index 131bd6a97aa..f4959dacae9 100644 --- a/tests/bud/cdi/containers-cdi.yaml +++ b/tests/bud/cdi/containers-cdi.yaml @@ -1,5 +1,5 @@ --- -cdiVersion: 0.5.0 +cdiVersion: 0.7.0 devices: - name: all containerEdits: diff --git a/tests/cdi.bats b/tests/cdi.bats index d6c8de54d78..f3ee5b6d8e4 100644 --- a/tests/cdi.bats +++ b/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" { diff --git a/vendor/modules.txt b/vendor/modules.txt index 16b61ae358e..7d190035bef 100644 --- a/vendor/modules.txt +++ b/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