From 55cb82c6c8740235512432d96faf22a5746aab6f Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Thu, 16 Jun 2022 14:17:53 +0200 Subject: [PATCH 1/2] Create single discoverer per mount type for CSV Instead of creating a set of discoverers per file, this change creates a discoverer per type by first concatenating the mount specifications from all files. This will allow all device nodes, for example, to be treated as a single device. Signed-off-by: Evan Lezar --- internal/discover/csv.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/internal/discover/csv.go b/internal/discover/csv.go index 9d9fda64..3e87cd0b 100644 --- a/internal/discover/csv.go +++ b/internal/discover/csv.go @@ -47,23 +47,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. -// The constructed discoverer is comprised of a list, with each element in the list being associated with a particular -// MountSpecType. -func NewFromCSVFile(logger *logrus.Logger, locators map[csv.MountSpecType]lookup.Locator, filename string) (Discover, error) { +// loadCSVFile +func loadCSVFile(logger *logrus.Logger, filename string) ([]*csv.MountSpec, error) { // Create a discoverer for each file-kind combination targets, err := csv.NewCSVFileParser(logger, filename).Parse() if err != nil { @@ -73,7 +71,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. From beff276a527f27073fa717afa031366b0c056916 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 6 Jul 2022 11:37:14 +0200 Subject: [PATCH 2/2] Add charDevices discoverer for devices This change adds a charDevices discoverer and using this for CSV, GDS, and MOFED discovery. Internally the discoverer is a "mounts" discoverer with a charDevice locator. Signed-off-by: Evan Lezar --- internal/discover/char_devices.go | 64 ++++++++++++++++++++ internal/discover/char_devices_test.go | 83 ++++++++++++++++++++++++++ internal/discover/csv.go | 46 ++++---------- internal/discover/csv_test.go | 57 ------------------ internal/discover/gds.go | 22 ++----- internal/discover/mofed.go | 28 ++------- 6 files changed, 168 insertions(+), 132 deletions(-) create mode 100644 internal/discover/char_devices.go create mode 100644 internal/discover/char_devices_test.go diff --git a/internal/discover/char_devices.go b/internal/discover/char_devices.go new file mode 100644 index 00000000..b7503fcc --- /dev/null +++ b/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 +} diff --git a/internal/discover/char_devices_test.go b/internal/discover/char_devices_test.go new file mode 100644 index 00000000..af820bb4 --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/discover/csv.go b/internal/discover/csv.go index 3e87cd0b..64e5d25c 100644 --- a/internal/discover/csv.go +++ b/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. @@ -60,7 +55,7 @@ func NewFromCSVFiles(logger *logrus.Logger, files []string, root string) (Discov return newFromMountSpecs(logger, locators, mountSpecs) } -// loadCSVFile +// loadCSVFile loads the specified CSV file and returns the list of mount specs func loadCSVFile(logger *logrus.Logger, filename string) ([]*csv.MountSpec, error) { // Create a discoverer for each file-kind combination targets, err := csv.NewCSVFileParser(logger, filename).Parse() @@ -97,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{ - logger: logger, - lookup: locator, - required: candidatesByType[t], - } - + var m Discover switch t { case csv.MountSpecDev: - // For device mount specs, we insert a charDevices into the list of discoverers. - discoverers = append(discoverers, (*charDevices)(m)) + m = NewDeviceDiscoverer(logger, locator, candidatesByType[t]) 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 -} diff --git a/internal/discover/csv_test.go b/internal/discover/csv_test.go index d1b37d0e..b46f9509 100644 --- a/internal/discover/csv_test.go +++ b/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() diff --git a/internal/discover/gds.go b/internal/discover/gds.go index f771aed7..fe11aa6d 100644 --- a/internal/discover/gds.go +++ b/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{ - logger: logger, - lookup: lookup.NewCharDeviceLocator(logger, root), - required: []string{"/dev/nvidia-fs*"}, - } + devices := NewCharDeviceDiscoverer( + logger, + []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() - 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 + return d.devices.Devices() } // Mounts discovers the required mounts for GPUDirect Storage. diff --git a/internal/discover/mofed.go b/internal/discover/mofed.go index 4e81ed2a..c48725e2 100644 --- a/internal/discover/mofed.go +++ b/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{ - logger: logger, - lookup: lookup.NewCharDeviceLocator(logger, root), - required: []string{ + devices := NewCharDeviceDiscoverer( + logger, + []string{ "/dev/infiniband/uverbs*", "/dev/infiniband/rdma_cm", }, - } - - 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)) - } + root, + ) return devices, nil }