Return annotation devices from VisibleDevices

This change includes annotation devices in CUDA.VisibleDevices
with the highest priority. This allows for the CDI device
request extraction to be consistent across all request mechanisms.

Note that this does change behaviour in the following ways:
1. Annotations are considered when resolving the runtime mode.
2. Incorrectly formed device names in annotations are no longer treated as an error.

Signed-off-by: Evan Lezar <elezar@nvidia.com>

internal/modifier/cdi.go

@@ -89,15 +89,6 @@ func (c *cdiDeviceRequestor) DeviceRequests() []string {
 	if c == nil {
 		return nil
 	}
-	annotationDevices, err := getAnnotationDevices(c.image)
-	if err != nil {
-		c.logger.Warningf("failed to get device requests from container annotations: %v; ignoring.", err)
-		annotationDevices = nil
-	}
-	if len(annotationDevices) > 0 {
-		return annotationDevices
-	}
-
 	var devices []string
 	for _, name := range c.image.VisibleDevices() {
 		if !parser.IsQualifiedName(name) {
@@ -109,21 +100,6 @@ func (c *cdiDeviceRequestor) DeviceRequests() []string {
 	return devices
 }
 
-// getAnnotationDevices returns a list of devices specified in the annotations.
-// Keys starting with the specified prefixes are considered and expected to contain a comma-separated list of
-// fully-qualified CDI devices names. If any device name is not fully-quality an error is returned.
-// The list of returned devices is deduplicated.
-func getAnnotationDevices(image image.CUDA) ([]string, error) {
-	var annotationDevices []string
-	for _, device := range image.CDIDeviceRequestsFromAnnotations() {
-		if !parser.IsQualifiedName(device) {
-			return nil, fmt.Errorf("invalid device name %q in annotations", device)
-		}
-		annotationDevices = append(annotationDevices, device)
-	}
-	return annotationDevices, nil
-}
-
 // filterAutomaticDevices searches for "automatic" device names in the input slice.
 // "Automatic" devices are a well-defined list of CDI device names which, when requested,
 // trigger the generation of a CDI spec at runtime. This removes the need to generate a
@@ -192,21 +168,6 @@ func generateAutomaticCDISpec(logger logger.Interface, cfg *config.Config, devic
 	)
 }
 
-func deviceRequestorFromImage(image image.CUDA) deviceRequestor {
-	return &fromImage{image}
-}
-
-type fromImage struct {
-	image.CUDA
-}
-
-func (f *fromImage) DeviceRequests() []string {
-	if f == nil {
-		return nil
-	}
-	return f.CUDA.VisibleDevices()
-}
-
 type deduplicatedDeviceRequestor struct {
 	deviceRequestor
 }

internal/modifier/cdi_test.go

@@ -17,7 +17,6 @@
 package modifier
 
 import (
-	"fmt"
 	"testing"
 
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -27,80 +26,6 @@ import (
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/config/image"
 )
 
-func TestGetAnnotationDevices(t *testing.T) {
-	testCases := []struct {
-		description     string
-		prefixes        []string
-		annotations     map[string]string
-		expectedDevices []string
-		expectedError   error
-	}{
-		{
-			description: "no annotations",
-		},
-		{
-			description: "no matching annotations",
-			prefixes:    []string{"not-prefix/"},
-			annotations: map[string]string{
-				"prefix/foo": "example.com/device=bar",
-			},
-		},
-		{
-			description: "single matching annotation",
-			prefixes:    []string{"prefix/"},
-			annotations: map[string]string{
-				"prefix/foo": "example.com/device=bar",
-			},
-			expectedDevices: []string{"example.com/device=bar"},
-		},
-		{
-			description: "multiple matching annotations",
-			prefixes:    []string{"prefix/", "another-prefix/"},
-			annotations: map[string]string{
-				"prefix/foo":         "example.com/device=bar",
-				"another-prefix/bar": "example.com/device=baz",
-			},
-			expectedDevices: []string{"example.com/device=bar", "example.com/device=baz"},
-		},
-		{
-			description: "multiple matching annotations with duplicate devices",
-			prefixes:    []string{"prefix/", "another-prefix/"},
-			annotations: map[string]string{
-				"prefix/foo":         "example.com/device=bar",
-				"another-prefix/bar": "example.com/device=bar",
-			},
-			expectedDevices: []string{"example.com/device=bar", "example.com/device=bar"},
-		},
-		{
-			description: "invalid devices",
-			prefixes:    []string{"prefix/"},
-			annotations: map[string]string{
-				"prefix/foo": "example.com/device",
-			},
-			expectedError: fmt.Errorf("invalid device %q", "example.com/device"),
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
-			image, err := image.New(
-				image.WithAnnotations(tc.annotations),
-				image.WithAnnotationsPrefixes(tc.prefixes),
-			)
-			require.NoError(t, err)
-
-			devices, err := getAnnotationDevices(image)
-			if tc.expectedError != nil {
-				require.Error(t, err)
-				return
-			}
-
-			require.NoError(t, err)
-			require.ElementsMatch(t, tc.expectedDevices, devices)
-		})
-	}
-}
-
 func TestDeviceRequests(t *testing.T) {
 	logger, _ := testlog.NewNullLogger()
 
@@ -108,6 +33,7 @@ func TestDeviceRequests(t *testing.T) {
 		description     string
 		input           cdiDeviceRequestor
 		spec            *specs.Spec
+		prefixes        []string
 		expectedDevices []string
 	}{
 		{
@@ -144,6 +70,73 @@ func TestDeviceRequests(t *testing.T) {
 			},
 			expectedDevices: []string{"nvidia.com/gpu=0", "example.com/class=device"},
 		},
+		{
+			description: "no matching annotations",
+			prefixes:    []string{"not-prefix/"},
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					"prefix/foo": "example.com/device=bar",
+				},
+			},
+		},
+		{
+			description: "single matching annotation",
+			prefixes:    []string{"prefix/"},
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					"prefix/foo": "example.com/device=bar",
+				},
+			},
+			expectedDevices: []string{"example.com/device=bar"},
+		},
+		{
+			description: "multiple matching annotations",
+			prefixes:    []string{"prefix/", "another-prefix/"},
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					"prefix/foo":         "example.com/device=bar",
+					"another-prefix/bar": "example.com/device=baz",
+				},
+			},
+			expectedDevices: []string{"example.com/device=bar", "example.com/device=baz"},
+		},
+		{
+			description: "multiple matching annotations with duplicate devices",
+			prefixes:    []string{"prefix/", "another-prefix/"},
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					"prefix/foo":         "example.com/device=bar",
+					"another-prefix/bar": "example.com/device=bar",
+				},
+			},
+			expectedDevices: []string{"example.com/device=bar", "example.com/device=bar"},
+		},
+		{
+			description: "devices in annotations are expanded",
+			input: cdiDeviceRequestor{
+				defaultKind: "nvidia.com/gpu",
+			},
+			prefixes: []string{"prefix/"},
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					"prefix/foo": "device",
+				},
+			},
+			expectedDevices: []string{"nvidia.com/gpu=device"},
+		},
+		{
+			description: "invalid devices in annotations are treated as strings",
+			input: cdiDeviceRequestor{
+				defaultKind: "nvidia.com/gpu",
+			},
+			prefixes: []string{"prefix/"},
+			spec: &specs.Spec{
+				Annotations: map[string]string{
+					"prefix/foo": "example.com/device",
+				},
+			},
+			expectedDevices: []string{"nvidia.com/gpu=example.com/device"},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -153,6 +146,7 @@ func TestDeviceRequests(t *testing.T) {
 			tc.spec,
 			image.WithAcceptDeviceListAsVolumeMounts(true),
 			image.WithAcceptEnvvarUnprivileged(true),
+			image.WithAnnotationsPrefixes(tc.prefixes),
 		)
 		require.NoError(t, err)
 		tc.input.image = image