From e8843c38f234f9faa83efabecb4a3092fb9449eb Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 15 Jun 2022 14:12:17 +0200 Subject: [PATCH] Move cmd/nvidia-container-runtime/modifier package to internal/modifier Signed-off-by: Evan Lezar --- cmd/nvidia-container-runtime/main_test.go | 2 +- .../runtime_factory.go | 2 +- .../modifier/csv.go | 13 +- .../modifier/csv_test.go | 138 +---------------- internal/modifier/discover_test.go | 145 ++++++++++++++++++ .../modifier/hook_remover.go | 0 internal/modifier/hook_remover_test.go | 111 ++++++++++++++ .../modifier/stable.go | 0 .../modifier/stable_test.go | 0 9 files changed, 267 insertions(+), 144 deletions(-) rename {cmd/nvidia-container-runtime => internal}/modifier/csv.go (89%) rename {cmd/nvidia-container-runtime => internal}/modifier/csv_test.go (53%) create mode 100644 internal/modifier/discover_test.go rename {cmd/nvidia-container-runtime => internal}/modifier/hook_remover.go (100%) create mode 100644 internal/modifier/hook_remover_test.go rename {cmd/nvidia-container-runtime => internal}/modifier/stable.go (100%) rename {cmd/nvidia-container-runtime => internal}/modifier/stable_test.go (100%) diff --git a/cmd/nvidia-container-runtime/main_test.go b/cmd/nvidia-container-runtime/main_test.go index 69cab81f..218c6d40 100644 --- a/cmd/nvidia-container-runtime/main_test.go +++ b/cmd/nvidia-container-runtime/main_test.go @@ -10,7 +10,7 @@ import ( "strings" "testing" - "github.com/NVIDIA/nvidia-container-toolkit/cmd/nvidia-container-runtime/modifier" + "github.com/NVIDIA/nvidia-container-toolkit/internal/modifier" "github.com/NVIDIA/nvidia-container-toolkit/internal/test" "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/require" diff --git a/cmd/nvidia-container-runtime/runtime_factory.go b/cmd/nvidia-container-runtime/runtime_factory.go index 9023d419..26925ced 100644 --- a/cmd/nvidia-container-runtime/runtime_factory.go +++ b/cmd/nvidia-container-runtime/runtime_factory.go @@ -19,9 +19,9 @@ package main import ( "fmt" - "github.com/NVIDIA/nvidia-container-toolkit/cmd/nvidia-container-runtime/modifier" "github.com/NVIDIA/nvidia-container-toolkit/internal/config" "github.com/NVIDIA/nvidia-container-toolkit/internal/info" + "github.com/NVIDIA/nvidia-container-toolkit/internal/modifier" "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" "github.com/NVIDIA/nvidia-container-toolkit/internal/runtime" "github.com/sirupsen/logrus" diff --git a/cmd/nvidia-container-runtime/modifier/csv.go b/internal/modifier/csv.go similarity index 89% rename from cmd/nvidia-container-runtime/modifier/csv.go rename to internal/modifier/csv.go index 06a1306d..fb5c3d07 100644 --- a/cmd/nvidia-container-runtime/modifier/csv.go +++ b/internal/modifier/csv.go @@ -24,7 +24,6 @@ import ( "github.com/NVIDIA/nvidia-container-toolkit/internal/cuda" "github.com/NVIDIA/nvidia-container-toolkit/internal/discover" "github.com/NVIDIA/nvidia-container-toolkit/internal/discover/csv" - "github.com/NVIDIA/nvidia-container-toolkit/internal/modifier" "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" "github.com/NVIDIA/nvidia-container-toolkit/internal/requirements" "github.com/sirupsen/logrus" @@ -51,8 +50,7 @@ func NewCSVModifier(logger *logrus.Logger, cfg *config.Config, ociSpec oci.Spec) return nil, fmt.Errorf("failed to load OCI spec: %v", err) } - // In experimental mode, we check whether a modification is required at all and return the lowlevelRuntime directly - // if no modification is required. + // We check whether a modification is required and return a nil modifier if this is not the case. visibleDevices, exists := ociSpec.LookupEnv(visibleDevicesEnvvar) if !exists || visibleDevices == "" || visibleDevices == visibleDevicesVoid { logger.Infof("No modification required: %v=%v (exists=%v)", visibleDevicesEnvvar, visibleDevices, exists) @@ -103,17 +101,12 @@ func NewCSVModifier(logger *logrus.Logger, cfg *config.Config, ociSpec oci.Spec) d := discover.NewList(csvDiscoverer, ldcacheUpdateHook, createSymlinksHook) - return newCSVModifierFromDiscoverer(logger, d) -} - -// newCSVModifierFromDiscoverer is used to test with dependency injection -func newCSVModifierFromDiscoverer(logger *logrus.Logger, d discover.Discover) (oci.SpecModifier, error) { - discoverModifier, err := modifier.NewModifierFromDiscoverer(logger, d) + discoverModifier, err := NewModifierFromDiscoverer(logger, d) if err != nil { return nil, fmt.Errorf("failed to construct modifier: %v", err) } - modifiers := modifier.Merge( + modifiers := Merge( nvidiaContainerRuntimeHookRemover{logger}, discoverModifier, ) diff --git a/cmd/nvidia-container-runtime/modifier/csv_test.go b/internal/modifier/csv_test.go similarity index 53% rename from cmd/nvidia-container-runtime/modifier/csv_test.go rename to internal/modifier/csv_test.go index ac3a0dee..b02180f2 100644 --- a/cmd/nvidia-container-runtime/modifier/csv_test.go +++ b/internal/modifier/csv_test.go @@ -21,7 +21,6 @@ import ( "testing" "github.com/NVIDIA/nvidia-container-toolkit/internal/config" - "github.com/NVIDIA/nvidia-container-toolkit/internal/discover" "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" "github.com/opencontainers/runtime-spec/specs-go" testlog "github.com/sirupsen/logrus/hooks/test" @@ -95,105 +94,15 @@ func TestNewCSVModifier(t *testing.T) { } } -func TestExperimentalModifier(t *testing.T) { +func TestCSVModifierRemovesHook(t *testing.T) { logger, _ := testlog.NewNullLogger() testCases := []struct { description string - discover *discover.DiscoverMock spec *specs.Spec expectedError error expectedSpec *specs.Spec }{ - { - description: "empty discoverer does not modify spec", - discover: &discover.DiscoverMock{}, - }, - { - description: "failed hooks discoverer returns error", - discover: &discover.DiscoverMock{ - HooksFunc: func() ([]discover.Hook, error) { - return nil, fmt.Errorf("discover.Hooks error") - }, - }, - expectedError: fmt.Errorf("discover.Hooks error"), - }, - { - description: "discovered hooks are injected into spec", - spec: &specs.Spec{}, - discover: &discover.DiscoverMock{ - HooksFunc: func() ([]discover.Hook, error) { - hooks := []discover.Hook{ - { - Lifecycle: "prestart", - Path: "/hook/a", - Args: []string{"/hook/a", "arga"}, - }, - { - Lifecycle: "createContainer", - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - } - return hooks, nil - }, - }, - expectedSpec: &specs.Spec{ - Hooks: &specs.Hooks{ - Prestart: []specs.Hook{ - { - Path: "/hook/a", - Args: []string{"/hook/a", "arga"}, - }, - }, - CreateContainer: []specs.Hook{ - { - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - }, - }, - }, - }, - { - description: "existing hooks are maintained", - spec: &specs.Spec{ - Hooks: &specs.Hooks{ - Prestart: []specs.Hook{ - { - Path: "/hook/a", - Args: []string{"/hook/a", "arga"}, - }, - }, - }, - }, - discover: &discover.DiscoverMock{ - HooksFunc: func() ([]discover.Hook, error) { - hooks := []discover.Hook{ - { - Lifecycle: "prestart", - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - } - return hooks, nil - }, - }, - expectedSpec: &specs.Spec{ - Hooks: &specs.Hooks{ - Prestart: []specs.Hook{ - { - Path: "/hook/a", - Args: []string{"/hook/a", "arga"}, - }, - { - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - }, - }, - }, - }, { description: "modification removes existing nvidia-container-runtime-hook", spec: &specs.Spec{ @@ -206,26 +115,9 @@ func TestExperimentalModifier(t *testing.T) { }, }, }, - discover: &discover.DiscoverMock{ - HooksFunc: func() ([]discover.Hook, error) { - hooks := []discover.Hook{ - { - Lifecycle: "prestart", - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - } - return hooks, nil - }, - }, expectedSpec: &specs.Spec{ Hooks: &specs.Hooks{ - Prestart: []specs.Hook{ - { - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - }, + Prestart: []specs.Hook{}, }, }, }, @@ -241,26 +133,9 @@ func TestExperimentalModifier(t *testing.T) { }, }, }, - discover: &discover.DiscoverMock{ - HooksFunc: func() ([]discover.Hook, error) { - hooks := []discover.Hook{ - { - Lifecycle: "prestart", - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - } - return hooks, nil - }, - }, expectedSpec: &specs.Spec{ Hooks: &specs.Hooks{ - Prestart: []specs.Hook{ - { - Path: "/hook/b", - Args: []string{"/hook/b", "argb"}, - }, - }, + Prestart: []specs.Hook{}, }, }, }, @@ -268,17 +143,16 @@ func TestExperimentalModifier(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - m, err := newCSVModifierFromDiscoverer(logger, tc.discover) - require.NoError(t, err) + m := nvidiaContainerRuntimeHookRemover{logger: logger} - err = m.Modify(tc.spec) + err := m.Modify(tc.spec) if tc.expectedError != nil { require.Error(t, err) } else { require.NoError(t, err) } - require.EqualValues(t, tc.expectedSpec, tc.spec) + require.Empty(t, tc.spec.Hooks.Prestart) }) } } diff --git a/internal/modifier/discover_test.go b/internal/modifier/discover_test.go new file mode 100644 index 00000000..f3523e3f --- /dev/null +++ b/internal/modifier/discover_test.go @@ -0,0 +1,145 @@ +/** +# 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 modifier + +import ( + "fmt" + "testing" + + "github.com/NVIDIA/nvidia-container-toolkit/internal/discover" + "github.com/opencontainers/runtime-spec/specs-go" + testlog "github.com/sirupsen/logrus/hooks/test" + "github.com/stretchr/testify/require" +) + +func TestDiscoverModifier(t *testing.T) { + logger, _ := testlog.NewNullLogger() + + testCases := []struct { + description string + discover *discover.DiscoverMock + spec *specs.Spec + expectedError error + expectedSpec *specs.Spec + }{ + { + description: "empty discoverer does not modify spec", + discover: &discover.DiscoverMock{}, + }, + { + description: "failed hooks discoverer returns error", + discover: &discover.DiscoverMock{ + HooksFunc: func() ([]discover.Hook, error) { + return nil, fmt.Errorf("discover.Hooks error") + }, + }, + expectedError: fmt.Errorf("discover.Hooks error"), + }, + { + description: "discovered hooks are injected into spec", + spec: &specs.Spec{}, + discover: &discover.DiscoverMock{ + HooksFunc: func() ([]discover.Hook, error) { + hooks := []discover.Hook{ + { + Lifecycle: "prestart", + Path: "/hook/a", + Args: []string{"/hook/a", "arga"}, + }, + { + Lifecycle: "createContainer", + Path: "/hook/b", + Args: []string{"/hook/b", "argb"}, + }, + } + return hooks, nil + }, + }, + expectedSpec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ + { + Path: "/hook/a", + Args: []string{"/hook/a", "arga"}, + }, + }, + CreateContainer: []specs.Hook{ + { + Path: "/hook/b", + Args: []string{"/hook/b", "argb"}, + }, + }, + }, + }, + }, + { + description: "existing hooks are maintained", + spec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ + { + Path: "/hook/a", + Args: []string{"/hook/a", "arga"}, + }, + }, + }, + }, + discover: &discover.DiscoverMock{ + HooksFunc: func() ([]discover.Hook, error) { + hooks := []discover.Hook{ + { + Lifecycle: "prestart", + Path: "/hook/b", + Args: []string{"/hook/b", "argb"}, + }, + } + return hooks, nil + }, + }, + expectedSpec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ + { + Path: "/hook/a", + Args: []string{"/hook/a", "arga"}, + }, + { + Path: "/hook/b", + Args: []string{"/hook/b", "argb"}, + }, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + m, err := NewModifierFromDiscoverer(logger, tc.discover) + require.NoError(t, err) + + err = m.Modify(tc.spec) + if tc.expectedError != nil { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + require.EqualValues(t, tc.expectedSpec, tc.spec) + }) + } +} diff --git a/cmd/nvidia-container-runtime/modifier/hook_remover.go b/internal/modifier/hook_remover.go similarity index 100% rename from cmd/nvidia-container-runtime/modifier/hook_remover.go rename to internal/modifier/hook_remover.go diff --git a/internal/modifier/hook_remover_test.go b/internal/modifier/hook_remover_test.go new file mode 100644 index 00000000..61c960b9 --- /dev/null +++ b/internal/modifier/hook_remover_test.go @@ -0,0 +1,111 @@ +/** +# 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 modifier + +import ( + "testing" + + "github.com/opencontainers/runtime-spec/specs-go" + testlog "github.com/sirupsen/logrus/hooks/test" + "github.com/stretchr/testify/require" +) + +func TestHookRemover(t *testing.T) { + logger, _ := testlog.NewNullLogger() + + testCases := []struct { + description string + spec *specs.Spec + expectedError error + expectedSpec *specs.Spec + }{ + { + description: "existing hooks are maintained", + spec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ + { + Path: "/hook/a", + Args: []string{"/hook/a", "arga"}, + }, + }, + }, + }, + expectedSpec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ + { + Path: "/hook/a", + Args: []string{"/hook/a", "arga"}, + }, + }, + }, + }, + }, + { + description: "modification removes existing nvidia-container-runtime-hook", + spec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ + { + Path: "/path/to/nvidia-container-runtime-hook", + Args: []string{"/path/to/nvidia-container-runtime-hook", "prestart"}, + }, + }, + }, + }, + expectedSpec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: nil, + }, + }, + }, + { + description: "modification removes existing nvidia-container-toolkit", + spec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ + { + Path: "/path/to/nvidia-container-toolkit", + Args: []string{"/path/to/nvidia-container-toolkit", "prestart"}, + }, + }, + }, + }, + expectedSpec: &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: nil, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + m := nvidiaContainerRuntimeHookRemover{logger: logger} + + err := m.Modify(tc.spec) + if tc.expectedError != nil { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + require.EqualValues(t, tc.expectedSpec, tc.spec) + }) + } +} diff --git a/cmd/nvidia-container-runtime/modifier/stable.go b/internal/modifier/stable.go similarity index 100% rename from cmd/nvidia-container-runtime/modifier/stable.go rename to internal/modifier/stable.go diff --git a/cmd/nvidia-container-runtime/modifier/stable_test.go b/internal/modifier/stable_test.go similarity index 100% rename from cmd/nvidia-container-runtime/modifier/stable_test.go rename to internal/modifier/stable_test.go