Merge branch 'refactor-csv-mount-spec-discovery' into 'master'

Refactor CSV discovery to make char device discovery clearer

See merge request nvidia/container-toolkit/container-toolkit!129

internal/discover/csv.go

@@ -24,13 +24,10 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-type csvDiscoverer struct {
+// charDevices is a discover for a list of character devices
-	mounts
+type charDevices mounts
-	filename  string
-	mountType csv.MountSpecType
-}
 
-var _ Discover = (*csvDiscoverer)(nil)
+var _ Discover = (*charDevices)(nil)
 
 // NewFromCSVFiles creates a discoverer for the specified CSV files. A logger is also supplied.
 // The constructed discoverer is comprised of a list, with each element in the list being associated with a
@@ -76,72 +73,66 @@ func NewFromCSVFile(logger *logrus.Logger, locators map[csv.MountSpecType]lookup
 		return nil, fmt.Errorf("CSV file is empty")
 	}
 
-	csvDiscoverers, err := newFromMountSpecs(logger, locators, targets)
+	return newFromMountSpecs(logger, locators, targets)
-	if err != nil {
+}
-		return nil, err
+
+// newFromMountSpecs creates a discoverer for the CSV file. A logger is also supplied.
+// A list of csvDiscoverers is returned, with each being associated with a single MountSpecType.
+func newFromMountSpecs(logger *logrus.Logger, locators map[csv.MountSpecType]lookup.Locator, targets []*csv.MountSpec) (Discover, error) {
+	if len(targets) == 0 {
+		return &None{}, nil
 	}
+
 	var discoverers []Discover
-	for _, d := range csvDiscoverers {
+	var mountSpecTypes []csv.MountSpecType
-		d.filename = filename
+	candidatesByType := make(map[csv.MountSpecType][]string)
-		discoverers = append(discoverers, d)
+	for _, t := range targets {
+		if _, exists := candidatesByType[t.Type]; !exists {
+			mountSpecTypes = append(mountSpecTypes, t.Type)
+		}
+		candidatesByType[t.Type] = append(candidatesByType[t.Type], t.Path)
+	}
+
+	for _, t := range mountSpecTypes {
+		locator, exists := locators[t]
+		if !exists {
+			return nil, fmt.Errorf("no locator defined for '%v'", t)
+		}
+
+		m := &mounts{
+			logger:   logger,
+			lookup:   locator,
+			required: candidatesByType[t],
+		}
+
+		switch t {
+		case csv.MountSpecDev:
+			// For device mount specs, we insert a charDevices into the list of discoverers.
+			discoverers = append(discoverers, (*charDevices)(m))
+		default:
+			discoverers = append(discoverers, m)
+		}
 	}
 
 	return &list{discoverers: discoverers}, nil
 }
 
-// newFromMountSpecs creates a discoverer for the CSV file. A logger is also supplied.
+// Mounts returns the discovered mounts for the charDevices. Since this explicitly specifies a
-// A list of csvDiscoverers is returned, with each being associated with a single MountSpecType.
+// device list, the mounts are nil.
-func newFromMountSpecs(logger *logrus.Logger, locators map[csv.MountSpecType]lookup.Locator, targets []*csv.MountSpec) ([]*csvDiscoverer, error) {
+func (d *charDevices) Mounts() ([]Mount, error) {
-	var discoverers []*csvDiscoverer
+	return nil, nil
-	candidatesByType := make(map[csv.MountSpecType][]string)
-	for _, t := range targets {
-		candidatesByType[t.Type] = append(candidatesByType[t.Type], t.Path)
-	}
-
-	for t, candidates := range candidatesByType {
-		locator, exists := locators[t]
-		if !exists {
-			return nil, fmt.Errorf("no locator defined for '%v'", t)
-		}
-		d := csvDiscoverer{
-			mounts: mounts{
-				logger:   logger,
-				lookup:   locator,
-				required: candidates,
-			},
-			mountType: t,
-		}
-		discoverers = append(discoverers, &d)
-	}
-
-	return discoverers, nil
 }
 
-// Mounts returns the discovered mounts for the csvDiscoverer.
+// Devices returns the discovered devices for the charDevices. Here the device nodes are first
-// Note that if the discoverer is for the device MountSpecType, the list of mounts is empty.
+// discovered as mounts and these are converted to devices.
-func (d *csvDiscoverer) Mounts() ([]Mount, error) {
+func (d *charDevices) Devices() ([]Device, error) {
-	if d.mountType == csv.MountSpecDev {
+	devicesAsMounts, err := (*mounts)(d).Mounts()
-		return d.None.Mounts()
-	}
-
-	return d.mounts.Mounts()
-}
-
-// Devices returns the discovered devices for the csvDiscoverer.
-// Note that if the discoverer is not for the device MountSpecType, the list of devices is empty.
-func (d *csvDiscoverer) Devices() ([]Device, error) {
-	if d.mountType != csv.MountSpecDev {
-		return d.None.Devices()
-	}
-
-	mounts, err := d.mounts.Mounts()
 	if err != nil {
 		return nil, err
 	}
 	var devices []Device
-	for _, mount := range mounts {
+	for _, mount := range devicesAsMounts {
-		device := Device(mount)
+		devices = append(devices, Device(mount))
-		devices = append(devices, device)
 	}
 
 	return devices, nil

internal/discover/csv_test.go

@@ -26,12 +26,12 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestCSVDiscoverer(t *testing.T) {
+func TestCharDevices(t *testing.T) {
 	logger, logHook := testlog.NewNullLogger()
 
 	testCases := []struct {
 		description          string
-		input                *csvDiscoverer
+		input                *charDevices
 		expectedMounts       []Mount
 		expectedMountsError  error
 		expectedDevicesError error
@@ -39,8 +39,8 @@ func TestCSVDiscoverer(t *testing.T) {
 	}{
 		{
 			description: "dev mounts are empty",
-			input: &csvDiscoverer{
+			input: (*charDevices)(
-				mounts: mounts{
+				&mounts{
 					lookup: &lookup.LocatorMock{
 						LocateFunc: func(string) ([]string, error) {
 							return []string{"located"}, nil
@@ -48,39 +48,14 @@ func TestCSVDiscoverer(t *testing.T) {
 					},
 					required: []string{"required"},
 				},
-				mountType: "dev",
+			),
-			},
 			expectedDevices: []Device{{Path: "located"}},
 		},
 		{
 			description:          "dev devices returns error for nil lookup",
-			input: &csvDiscoverer{
+			input:                &charDevices{},
-				mountType: "dev",
-			},
 			expectedDevicesError: fmt.Errorf("no lookup defined"),
 		},
-		{
-			description: "lib devices are empty",
-			input: &csvDiscoverer{
-				mounts: mounts{
-					lookup: &lookup.LocatorMock{
-						LocateFunc: func(string) ([]string, error) {
-							return []string{"located"}, nil
-						},
-					},
-					required: []string{"required"},
-				},
-				mountType: "lib",
-			},
-			expectedMounts: []Mount{{Path: "located"}},
-		},
-		{
-			description: "lib mounts returns error for nil lookup",
-			input: &csvDiscoverer{
-				mountType: "lib",
-			},
-			expectedMountsError: fmt.Errorf("no lookup defined"),
-		},
 	}
 
 	for _, tc := range testCases {
@@ -120,10 +95,11 @@ func TestNewFromMountSpec(t *testing.T) {
 		description        string
 		targets            []*csv.MountSpec
 		expectedError      error
-		expectedCSVDiscoverers []*csvDiscoverer
+		expectedDiscoverer Discover
 	}{
 		{
-			description: "empty targets returns empyt list",
+			description:        "empty targets returns None discoverer list",
+			expectedDiscoverer: &None{},
 		},
 		{
 			description: "unexpected locator returns error",
@@ -151,18 +127,16 @@ func TestNewFromMountSpec(t *testing.T) {
 					Path: "dev1",
 				},
 			},
-			expectedCSVDiscoverers: []*csvDiscoverer{
+			expectedDiscoverer: &list{
-				{
+				discoverers: []Discover{
-					mountType: "dev",
+					(*charDevices)(
-					mounts: mounts{
+						&mounts{
 							logger:   logger,
 							lookup:   locators["dev"],
 							required: []string{"dev0", "dev1"},
 						},
-				},
+					),
-				{
+					&mounts{
-					mountType: "lib",
-					mounts: mounts{
 						logger:   logger,
 						lookup:   locators["lib"],
 						required: []string{"lib0"},
@@ -174,13 +148,13 @@ func TestNewFromMountSpec(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			discoverers, err := newFromMountSpecs(logger, locators, tc.targets)
+			discoverer, err := newFromMountSpecs(logger, locators, tc.targets)
 			if tc.expectedError != nil {
 				require.Error(t, err)
 				return
 			}
 			require.NoError(t, err)
-			require.ElementsMatch(t, tc.expectedCSVDiscoverers, discoverers)
+			require.EqualValues(t, tc.expectedDiscoverer, discoverer)
 		})
 	}
 }