From 7ec3cd0b5b303afd0b1b0cbf4ee58493183eab05 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Fri, 8 Apr 2022 11:36:48 +0200 Subject: [PATCH 1/3] Fix creation of CSV parser in create-symlinks Signed-off-by: Evan Lezar --- cmd/nvidia-ctk/hook/create-symlinks/create-symlinks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/nvidia-ctk/hook/create-symlinks/create-symlinks.go b/cmd/nvidia-ctk/hook/create-symlinks/create-symlinks.go index f8e6e835..aaf45e76 100644 --- a/cmd/nvidia-ctk/hook/create-symlinks/create-symlinks.go +++ b/cmd/nvidia-ctk/hook/create-symlinks/create-symlinks.go @@ -103,7 +103,7 @@ func (m command) run(c *cli.Context, cfg *config) error { var candidates []string for _, file := range csvFiles { - mountSpecs, err := csv.ParseFile(m.logger, file) + mountSpecs, err := csv.NewCSVFileParser(m.logger, file).Parse() if err != nil { m.logger.Debugf("Skipping CSV file %v: %v", file, err) continue From 2c1e35637072c831e6fdab68b8ccc1c18241d8b9 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Fri, 8 Apr 2022 08:52:07 +0200 Subject: [PATCH 2/3] Refactor CSV discovery to make char device discovery clearer Signed-off-by: Evan Lezar --- internal/discover/csv.go | 81 +++++++++++++++-------------------- internal/discover/csv_test.go | 76 +++++++++++--------------------- 2 files changed, 59 insertions(+), 98 deletions(-) diff --git a/internal/discover/csv.go b/internal/discover/csv.go index 436fbcbd..f3761b9f 100644 --- a/internal/discover/csv.go +++ b/internal/discover/csv.go @@ -24,13 +24,10 @@ import ( "github.com/sirupsen/logrus" ) -type csvDiscoverer struct { - mounts - filename string - mountType csv.MountSpecType -} +// charDevices is a discover for a list of character devices +type charDevices mounts -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,23 +73,17 @@ func NewFromCSVFile(logger *logrus.Logger, locators map[csv.MountSpecType]lookup return nil, fmt.Errorf("CSV file is empty") } - csvDiscoverers, err := newFromMountSpecs(logger, locators, targets) - if err != nil { - return nil, err - } - var discoverers []Discover - for _, d := range csvDiscoverers { - d.filename = filename - discoverers = append(discoverers, d) - } - - return &list{discoverers: discoverers}, nil + return newFromMountSpecs(logger, locators, targets) } // 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) ([]*csvDiscoverer, error) { - var discoverers []*csvDiscoverer +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 candidatesByType := make(map[csv.MountSpecType][]string) for _, t := range targets { candidatesByType[t.Type] = append(candidatesByType[t.Type], t.Path) @@ -103,45 +94,41 @@ func newFromMountSpecs(logger *logrus.Logger, locators map[csv.MountSpecType]loo if !exists { return nil, fmt.Errorf("no locator defined for '%v'", t) } - d := csvDiscoverer{ - mounts: mounts{ - logger: logger, - lookup: locator, - required: candidates, - }, - mountType: t, + + m := &mounts{ + logger: logger, + lookup: locator, + required: candidates, + } + + 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) } - discoverers = append(discoverers, &d) } - return discoverers, nil + return &list{discoverers: discoverers}, nil } -// Mounts returns the discovered mounts for the csvDiscoverer. -// Note that if the discoverer is for the device MountSpecType, the list of mounts is empty. -func (d *csvDiscoverer) Mounts() ([]Mount, error) { - if d.mountType == csv.MountSpecDev { - return d.None.Mounts() - } - - return d.mounts.Mounts() +// 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 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() +// 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 mounts { - device := Device(mount) - devices = append(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 ac0ef482..28b1e021 100644 --- a/internal/discover/csv_test.go +++ b/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{ - mounts: mounts{ + input: (*charDevices)( + &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{ - mountType: "dev", - }, + description: "dev devices returns error for nil lookup", + input: &charDevices{}, 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 { @@ -117,13 +92,14 @@ func TestNewFromMountSpec(t *testing.T) { } testCases := []struct { - description string - targets []*csv.MountSpec - expectedError error - expectedCSVDiscoverers []*csvDiscoverer + description string + targets []*csv.MountSpec + expectedError error + 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{ - { - mountType: "dev", - mounts: mounts{ - logger: logger, - lookup: locators["dev"], - required: []string{"dev0", "dev1"}, - }, - }, - { - mountType: "lib", - mounts: mounts{ + expectedDiscoverer: &list{ + discoverers: []Discover{ + (*charDevices)( + &mounts{ + logger: logger, + lookup: locators["dev"], + required: []string{"dev0", "dev1"}, + }, + ), + &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) }) } } From 62f608a3fe7d45726da48ac176a76f53e6594820 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Fri, 8 Apr 2022 11:59:26 +0200 Subject: [PATCH 3/3] Make order of discoverers deterministic Signed-off-by: Evan Lezar --- internal/discover/csv.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/discover/csv.go b/internal/discover/csv.go index f3761b9f..9d9fda64 100644 --- a/internal/discover/csv.go +++ b/internal/discover/csv.go @@ -84,12 +84,16 @@ func newFromMountSpecs(logger *logrus.Logger, locators map[csv.MountSpecType]loo } var discoverers []Discover + var mountSpecTypes []csv.MountSpecType candidatesByType := make(map[csv.MountSpecType][]string) 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, candidates := range candidatesByType { + for _, t := range mountSpecTypes { locator, exists := locators[t] if !exists { return nil, fmt.Errorf("no locator defined for '%v'", t) @@ -98,7 +102,7 @@ func newFromMountSpecs(logger *logrus.Logger, locators map[csv.MountSpecType]loo m := &mounts{ logger: logger, lookup: locator, - required: candidates, + required: candidatesByType[t], } switch t {