Merge branch 'refactor-csv-discovery' into 'main'

Refactor device discovery

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

internal/discover/char_devices.go

@@ -0,0 +1,64 @@
+/**
+# Copyright (c) 2022, NVIDIA CORPORATION.  All rights reserved.
+#
+# 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 discover
+
+import (
+	"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup"
+	"github.com/sirupsen/logrus"
+)
+
+// charDevices is a discover for a list of character devices
+type charDevices mounts
+
+var _ Discover = (*charDevices)(nil)
+
+// NewCharDeviceDiscoverer creates a discoverer which locates the specified set of device nodes.
+func NewCharDeviceDiscoverer(logger *logrus.Logger, devices []string, root string) Discover {
+	locator := lookup.NewCharDeviceLocator(logger, root)
+
+	return NewDeviceDiscoverer(logger, locator, devices)
+}
+
+// NewDeviceDiscoverer creates a discoverer which locates the specified set of device nodes using the specified locator.
+func NewDeviceDiscoverer(logger *logrus.Logger, locator lookup.Locator, devices []string) Discover {
+	return &charDevices{
+		logger:   logger,
+		lookup:   locator,
+		required: devices,
+	}
+}
+
+// Mounts returns the discovered mounts for the charDevices.
+// Since this explicitly specifies a device list, the mounts are nil.
+func (d *charDevices) Mounts() ([]Mount, error) {
+	return nil, nil
+}
+
+// Devices returns the discovered devices for the charDevices.
+// Here the device nodes are first discovered as mounts and these are converted to devices.
+func (d *charDevices) Devices() ([]Device, error) {
+	devicesAsMounts, err := (*mounts)(d).Mounts()
+	if err != nil {
+		return nil, err
+	}
+	var devices []Device
+	for _, mount := range devicesAsMounts {
+		devices = append(devices, Device(mount))
+	}
+
+	return devices, nil
+}

internal/discover/char_devices_test.go

@@ -0,0 +1,83 @@
+/**
+# Copyright (c) 2021, NVIDIA CORPORATION.  All rights reserved.
+#
+# 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 discover
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup"
+	testlog "github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/require"
+)
+
+func TestCharDevices(t *testing.T) {
+	logger, logHook := testlog.NewNullLogger()
+
+	testCases := []struct {
+		description          string
+		input                *charDevices
+		expectedMounts       []Mount
+		expectedMountsError  error
+		expectedDevicesError error
+		expectedDevices      []Device
+	}{
+		{
+			description: "dev mounts are empty",
+			input: (*charDevices)(
+				&mounts{
+					lookup: &lookup.LocatorMock{
+						LocateFunc: func(string) ([]string, error) {
+							return []string{"located"}, nil
+						},
+					},
+					required: []string{"required"},
+				},
+			),
+			expectedDevices: []Device{{Path: "located", HostPath: "located"}},
+		},
+		{
+			description:          "dev devices returns error for nil lookup",
+			input:                &charDevices{},
+			expectedDevicesError: fmt.Errorf("no lookup defined"),
+		},
+	}
+
+	for _, tc := range testCases {
+		logHook.Reset()
+
+		t.Run(tc.description, func(t *testing.T) {
+			tc.input.logger = logger
+
+			mounts, err := tc.input.Mounts()
+			if tc.expectedMountsError != nil {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+			require.ElementsMatch(t, tc.expectedMounts, mounts)
+
+			devices, err := tc.input.Devices()
+			if tc.expectedDevicesError != nil {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+			require.ElementsMatch(t, tc.expectedDevices, devices)
+		})
+	}
+}

internal/discover/csv.go

@@ -24,11 +24,6 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// charDevices is a discover for a list of character devices
-type charDevices mounts
-
-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
 // single CSV files.
@@ -47,23 +42,21 @@ func NewFromCSVFiles(logger *logrus.Logger, files []string, root string) (Discov
 		csv.MountSpecSym: symlinkLocator,
 	}
 
-	var discoverers []Discover
+	var mountSpecs []*csv.MountSpec
 	for _, filename := range files {
-		d, err := NewFromCSVFile(logger, locators, filename)
+		targets, err := loadCSVFile(logger, filename)
 		if err != nil {
 			logger.Warnf("Skipping CSV file %v: %v", filename, err)
 			continue
 		}
-		discoverers = append(discoverers, d)
+		mountSpecs = append(mountSpecs, targets...)
 	}
 
-	return &list{discoverers: discoverers}, nil
+	return newFromMountSpecs(logger, locators, mountSpecs)
 }
 
-// NewFromCSVFile creates a discoverer for the specified CSV file. A logger is also supplied.
+// loadCSVFile loads the specified CSV file and returns the list of mount specs
-// The constructed discoverer is comprised of a list, with each element in the list being associated with a particular
+func loadCSVFile(logger *logrus.Logger, filename string) ([]*csv.MountSpec, error) {
-// MountSpecType.
-func NewFromCSVFile(logger *logrus.Logger, locators map[csv.MountSpecType]lookup.Locator, filename string) (Discover, error) {
 	// Create a discoverer for each file-kind combination
 	targets, err := csv.NewCSVFileParser(logger, filename).Parse()
 	if err != nil {
@@ -73,7 +66,7 @@ func NewFromCSVFile(logger *logrus.Logger, locators map[csv.MountSpecType]lookup
 		return nil, fmt.Errorf("CSV file is empty")
 	}
 
-	return newFromMountSpecs(logger, locators, targets)
+	return targets, nil
 }
 
 // newFromMountSpecs creates a discoverer for the CSV file. A logger is also supplied.
@@ -99,41 +92,20 @@ func newFromMountSpecs(logger *logrus.Logger, locators map[csv.MountSpecType]loo
 			return nil, fmt.Errorf("no locator defined for '%v'", t)
 		}
 
-		m := &mounts{
+		var m Discover
-			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.
+			m = NewDeviceDiscoverer(logger, locator, candidatesByType[t])
-			discoverers = append(discoverers, (*charDevices)(m))
 		default:
-			discoverers = append(discoverers, m)
+			m = &mounts{
+				logger:   logger,
+				lookup:   locator,
+				required: candidatesByType[t],
+			}
 		}
+		discoverers = append(discoverers, m)
+
 	}
 
 	return &list{discoverers: discoverers}, nil
 }
-
-// Mounts returns the discovered mounts for the charDevices. Since this explicitly specifies a
-// device list, the mounts are nil.
-func (d *charDevices) Mounts() ([]Mount, error) {
-	return nil, nil
-}
-
-// Devices returns the discovered devices for the charDevices. Here the device nodes are first
-// discovered as mounts and these are converted to devices.
-func (d *charDevices) Devices() ([]Device, error) {
-	devicesAsMounts, err := (*mounts)(d).Mounts()
-	if err != nil {
-		return nil, err
-	}
-	var devices []Device
-	for _, mount := range devicesAsMounts {
-		devices = append(devices, Device(mount))
-	}
-
-	return devices, nil
-}

internal/discover/csv_test.go

@@ -26,63 +26,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestCharDevices(t *testing.T) {
-	logger, logHook := testlog.NewNullLogger()
-
-	testCases := []struct {
-		description          string
-		input                *charDevices
-		expectedMounts       []Mount
-		expectedMountsError  error
-		expectedDevicesError error
-		expectedDevices      []Device
-	}{
-		{
-			description: "dev mounts are empty",
-			input: (*charDevices)(
-				&mounts{
-					lookup: &lookup.LocatorMock{
-						LocateFunc: func(string) ([]string, error) {
-							return []string{"located"}, nil
-						},
-					},
-					required: []string{"required"},
-				},
-			),
-			expectedDevices: []Device{{Path: "located", HostPath: "located"}},
-		},
-		{
-			description:          "dev devices returns error for nil lookup",
-			input:                &charDevices{},
-			expectedDevicesError: fmt.Errorf("no lookup defined"),
-		},
-	}
-
-	for _, tc := range testCases {
-		logHook.Reset()
-
-		t.Run(tc.description, func(t *testing.T) {
-			tc.input.logger = logger
-
-			mounts, err := tc.input.Mounts()
-			if tc.expectedMountsError != nil {
-				require.Error(t, err)
-			} else {
-				require.NoError(t, err)
-			}
-			require.ElementsMatch(t, tc.expectedMounts, mounts)
-
-			devices, err := tc.input.Devices()
-			if tc.expectedDevicesError != nil {
-				require.Error(t, err)
-			} else {
-				require.NoError(t, err)
-			}
-			require.ElementsMatch(t, tc.expectedDevices, devices)
-		})
-	}
-}
-
 func TestNewFromMountSpec(t *testing.T) {
 	logger, _ := testlog.NewNullLogger()
 

internal/discover/gds.go

@@ -30,11 +30,11 @@ type gdsDeviceDiscoverer struct {
 
 // NewGDSDiscoverer creates a discoverer for GPUDirect Storage devices and mounts.
 func NewGDSDiscoverer(logger *logrus.Logger, root string) (Discover, error) {
-	devices := &mounts{
+	devices := NewCharDeviceDiscoverer(
-		logger:   logger,
+		logger,
-		lookup:   lookup.NewCharDeviceLocator(logger, root),
+		[]string{"/dev/nvidia-fs*"},
-		required: []string{"/dev/nvidia-fs*"},
+		root,
-	}
+	)
 
 	udev := &mounts{
 		logger:   logger,
@@ -59,17 +59,7 @@ func NewGDSDiscoverer(logger *logrus.Logger, root string) (Discover, error) {
 
 // Devices discovers the nvidia-fs device nodes for use with GPUDirect Storage
 func (d *gdsDeviceDiscoverer) Devices() ([]Device, error) {
-	devicesAsMounts, err := d.devices.Mounts()
+	return d.devices.Devices()
-	if err != nil {
-		d.logger.Debugf("Could not locate GDS devices: %v", err)
-		return nil, nil
-	}
-	var devices []Device
-	for _, mount := range devicesAsMounts {
-		devices = append(devices, Device(mount))
-	}
-
-	return devices, nil
 }
 
 // Mounts discovers the required mounts for GPUDirect Storage.

internal/discover/mofed.go

@@ -17,37 +17,19 @@
 package discover
 
 import (
-	"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup"
 	"github.com/sirupsen/logrus"
 )
 
-type mofedDeviceDiscoverer mounts
-
 // NewMOFEDDiscoverer creates a discoverer for MOFED devices.
 func NewMOFEDDiscoverer(logger *logrus.Logger, root string) (Discover, error) {
-	devices := &mofedDeviceDiscoverer{
+	devices := NewCharDeviceDiscoverer(
-		logger: logger,
+		logger,
-		lookup: lookup.NewCharDeviceLocator(logger, root),
+		[]string{
-		required: []string{
 			"/dev/infiniband/uverbs*",
 			"/dev/infiniband/rdma_cm",
 		},
-	}
+		root,
-
+	)
-	return devices, nil
-}
-
-// Devices discovers the uverbs* and rdma_cm device nodes for use with GPUDirect Storage and the MOFED stack.
-func (d *mofedDeviceDiscoverer) Devices() ([]Device, error) {
-	devicesAsMounts, err := (*mounts)(d).Mounts()
-	if err != nil {
-		d.logger.Debugf("Could not locate MOFED devices: %v", err)
-		return nil, nil
-	}
-	var devices []Device
-	for _, mount := range devicesAsMounts {
-		devices = append(devices, Device(mount))
-	}
 
 	return devices, nil
 }