Disable chmod-hook by default #1221
Conversation
Pull Request Test Coverage Report for Build 17094949023Details
💛 - Coveralls |
ArangoGutierrez
force-pushed
the
i-1218
branch
2 times, most recently
from
f18a08a to
b26ad2b
Compare
| &cli.StringSliceFlag{ | ||
| Name: "enable-hook", | ||
| Aliases: []string{"enable-hooks"}, | ||
| Usage: "Explicitly enable a hook in the generated CDI specification. This can be specified multiple times.", |
There was a problem hiding this comment.
| Usage: "Explicitly enable a hook in the generated CDI specification. This can be specified multiple times.", | |
| Usage: "Explicitly enable a hook in the generated CDI specification. This overrides disabled hooks. This can be specified multiple times.", |
| if m.config != nil { | ||
| if len(opts.disabledHooks) == 0 { | ||
| if disabledHooks := m.config.Get("nvidia-container-runtime-hook.disabled"); disabledHooks != nil { | ||
| if hooks, ok := disabledHooks.([]interface{}); ok { | ||
| for _, hook := range hooks { | ||
| if h, ok := hook.(string); ok { | ||
| opts.disabledHooks = append(opts.disabledHooks, h) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this required? I thought we added support to read settings from a config file using altsrc.
internal/config/hook.go
Outdated
| // DisabledHooks specifies the hooks that are disabled for the NVIDIA | ||
| // Container Runtime hook. | ||
| DisabledHooks []string `toml:"disabled,omitempty"` | ||
| // EnabledHooks specifies the hooks that are enabled for the NVIDIA | ||
| // Container Runtime hook. | ||
| // Specify a list of explicitly enabled hooks. If a hook is specified as | ||
| // both enabled and disabled, it will be enabled. | ||
| EnabledHooks []string `toml:"enabled,omitempty"` |
There was a problem hiding this comment.
I don't think we should add these config options, and IF we decide to add them we should not add them in the nvidia-container-runtime-hook section. This section is specific for the nvidia-container-runtime-hook that is used to call out to the nvidia-container-cli.
There was a problem hiding this comment.
removed for now, maybe a discuss-todo item
There was a problem hiding this comment.
Sorry, now pushed
internal/discover/hooks.go
Outdated
| return func(c *cdiHookCreator) { | ||
| for _, hook := range hooks { | ||
| c.disabledHooks[hook] = true | ||
| c.hookStates[hook] = false |
There was a problem hiding this comment.
This implies ordering between the calls to WithDisabledHooks and WithEnabledHooks. I would rather have the order or precedence well-defined such that hooks that are specifically ENABLED take precedence over hooks that are specifically DISABLED.
There was a problem hiding this comment.
Maybe this can be simplified by keeping the cdiHookCreator struct as is, and introducing an options struct that we can use to set up the correct values of disabledHooks.
There was a problem hiding this comment.
you are right, let me think on a proper solution for this
ArangoGutierrez
force-pushed
the
i-1218
branch
2 times, most recently
from
815dae7 to
97df210
Compare
internal/config/hook.go
Outdated
| // DisabledHooks specifies the hooks that are disabled for the NVIDIA | ||
| // Container Runtime hook. | ||
| DisabledHooks []string `toml:"disabled,omitempty"` |
There was a problem hiding this comment.
I thought we said we're going to remove this?
internal/discover/hooks.go
Outdated
|
|
||
| // defaultDisabledHooks defines hooks that are disabled by default. | ||
| // These hooks can be explicitly enabled using the WithEnabledHooks option. | ||
| var defaultDisabledHooks = map[HookName]bool{ |
There was a problem hiding this comment.
This need not be a map. Also does it make sense to just set this inline when constructing the hook creator?
There was a problem hiding this comment.
turned into a list.
I do think it is better to have it here, so the list is easier to maintain. Over time, we might want to add other hooks to the list.
ArangoGutierrez
force-pushed
the
i-1218
branch
from
97df210 to
2334a53
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR disables the chmod createContainer hook by default, addressing issue #1218. The chmod hook was originally added as a workaround for a specific crun issue with device nodes in subdirectories of /dev, but this has since been fixed in crun version 1.7.
- Adds
defaultDisabledHooksslice containing theChmodHookto disable it by default - Introduces
WithEnabledHooksoption to explicitly enable hooks that are disabled by default - Updates the hook creation logic to handle both disabled and enabled hooks with proper precedence
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/discover/hooks.go | Adds default disabled hooks list and enabled hooks functionality |
| pkg/nvcdi/options.go | Adds WithEnabledHook option function |
| pkg/nvcdi/lib.go | Adds enabledHooks field and passes it to hook creator |
| cmd/nvidia-ctk/cdi/generate/generate.go | Adds --enable-hook CLI flag and option handling |
| cmd/nvidia-ctk/cdi/generate/generate_test.go | Adds test case for enabling chmod hook |
| internal/runtime/runtime_factory.go | Refactors hook creator initialization to use options slice |
| cmd/nvidia-ctk-installer/toolkit/toolkit_test.go | Updates expected test output |
internal/discover/hooks.go
Outdated
|
|
||
| // Correct the disabledHooks map to ensure that explicitly enabled hooks | ||
| // are not disabled. | ||
| for hook := range enabledHooks { |
There was a problem hiding this comment.
The loop iterates over enabledHooks variable instead of cdiHookCreator.enabledHooks. This will cause enabled hooks to not be properly processed since the local enabledHooks variable is always empty at this point.
| for hook := range enabledHooks { | |
| for hook := range cdiHookCreator.enabledHooks { |
There was a problem hiding this comment.
Does this mean that we don't have a test for the case where enabled overrides disabled?
internal/runtime/runtime_factory.go
Outdated
| } | ||
|
|
||
| hookCreator := discover.NewHookCreator(discover.WithNVIDIACDIHookPath(cfg.NVIDIACTKConfig.Path)) | ||
| opts := []discover.Option{ |
internal/discover/hooks.go
Outdated
| type cdiHookCreator struct { | ||
| nvidiaCDIHookPath string | ||
| disabledHooks map[HookName]bool | ||
| enabledHooks map[HookName]bool |
There was a problem hiding this comment.
Since this is only actually used in the construction of the cdiHookCreator and doesn't affect behaviour, does it make sense to split out separate options instead of updating the struct directly? Something like:
diff --git a/internal/discover/hooks.go b/internal/discover/hooks.go
index acfaef86..ada69fb8 100644
--- a/internal/discover/hooks.go
+++ b/internal/discover/hooks.go
@@ -85,12 +85,17 @@ func (h *Hook) Hooks() ([]Hook, error) {
return []Hook{*h}, nil
}
-type Option func(*cdiHookCreator)
+type hookCreatorOptions struct {
+ nvidiaCDIHookPath string
+ disabledHooks []HookName
+ enabledHooks []HookName
+}
+
+type Option func(*hookCreatorOptions)
type cdiHookCreator struct {
nvidiaCDIHookPath string
disabledHooks map[HookName]bool
- enabledHooks map[HookName]bool
fixedArgs []string
debugLogging bool
@@ -111,60 +116,61 @@ type HookCreator interface {
// WithDisabledHooks explicitly disables the specified hooks.
// This can be specified multiple times.
-func WithDisabledHooks(hooks ...HookName) Option {
- return func(c *cdiHookCreator) {
- for _, hook := range hooks {
- c.disabledHooks[hook] = true
+func WithDisabledHooks[T string | HookName](hooks ...T) Option {
+ return func(c *hookCreatorOptions) {
+ for _, h := range hooks {
+ c.disabledHooks = append(c.disabledHooks, HookName(h))
}
}
}
// WithEnabledHooks explicitly enables the specified hooks.
// This is useful for enabling hooks that are disabled by default.
-func WithEnabledHooks(hooks ...HookName) Option {
- return func(c *cdiHookCreator) {
- for _, hook := range hooks {
- c.enabledHooks[hook] = true
+func WithEnabledHooks[T string | HookName](hooks ...T) Option {
+ return func(c *hookCreatorOptions) {
+ for _, h := range hooks {
+ c.enabledHooks = append(c.disabledHooks, HookName(h))
}
}
}
// WithNVIDIACDIHookPath sets the path to the nvidia-cdi-hook binary.
func WithNVIDIACDIHookPath(nvidiaCDIHookPath string) Option {
- return func(c *cdiHookCreator) {
+ return func(c *hookCreatorOptions) {
c.nvidiaCDIHookPath = nvidiaCDIHookPath
}
}
func NewHookCreator(opts ...Option) HookCreator {
- disabledHooks := make(map[HookName]bool)
- enabledHooks := make(map[HookName]bool)
- for _, hook := range defaultDisabledHooks {
- disabledHooks[hook] = true
- }
-
- cdiHookCreator := &cdiHookCreator{
+ o := &hookCreatorOptions{
nvidiaCDIHookPath: defaultNvidiaCDIHookPath,
- disabledHooks: disabledHooks,
- enabledHooks: enabledHooks,
}
for _, opt := range opts {
- opt(cdiHookCreator)
+ opt(o)
}
- // Correct the disabledHooks map to ensure that explicitly enabled hooks
- // are not disabled.
- for hook := range enabledHooks {
- cdiHookCreator.disabledHooks[hook] = false
+ o.disabledHooks = append(o.disabledHooks, defaultDisabledHooks...)
+
+ disabledHooks := make(map[HookName]bool)
+ for _, h := range o.disabledHooks {
+ disabledHooks[h] = true
}
- if cdiHookCreator.disabledHooks[AllHooks] {
+ if disabledHooks[AllHooks] && len(o.enabledHooks) == 0 {
return &allDisabledHookCreator{}
}
- cdiHookCreator.fixedArgs = getFixedArgsForCDIHookCLI(cdiHookCreator.nvidiaCDIHookPath)
+ for _, h := range o.enabledHooks {
+ disabledHooks[h] = false
+ }
+
+ c := &cdiHookCreator{
+ nvidiaCDIHookPath: o.nvidiaCDIHookPath,
+ disabledHooks: make(map[HookName]bool),
+ fixedArgs: getFixedArgsForCDIHookCLI(o.nvidiaCDIHookPath),
+ }
- return cdiHookCreator
+ return c
}
// Create creates a new hook with the given name and arguments.
@@ -183,7 +189,11 @@ func (c cdiHookCreator) Create(name HookName, args ...string) *Hook {
}
func (c cdiHookCreator) isDisabled(name HookName, args ...string) bool {
- if c.disabledHooks[name] {
+ disabled, ok := c.disabledHooks[name]
+ if ok {
+ return disabled
+ }
+ if c.disabledHooks[AllHooks] {
return true
}
It would also be good to add a unit test to test cdiHookCreator.isDisabled behaviour.
There was a problem hiding this comment.
Unit test added
ArangoGutierrez
force-pushed
the
i-1218
branch
from
2334a53 to
2b74695
Compare
internal/discover/hooks_test.go
Outdated
| name: "nvidia-ctk in /usr/sbin (RPM-based systems)", | ||
| opts: []Option{ | ||
| WithNVIDIACDIHookPath("/usr/sbin/nvidia-ctk"), | ||
| }, | ||
| expectedType: "cdiHookCreator", | ||
| expectedPath: "/usr/sbin/nvidia-ctk", | ||
| expectedDisabled: map[HookName]bool{ | ||
| ChmodHook: true, | ||
| }, |
There was a problem hiding this comment.
While I see the point of this test in principle there is nothing in the constructor that depends on the bin vs sbin on various platforms. From the point of view of the contructor the path is just a string, and only the BASENAME of the executable affects the behaviour.
internal/discover/hooks_test.go
Outdated
| for hook, disabled := range tc.expectedDisabled { | ||
| require.Equal(t, disabled, creator.disabledHooks[hook], "Hook %s disabled state mismatch", hook) | ||
| } |
There was a problem hiding this comment.
Can one use require.ElementsMatch or something similar here?
| testCases := []struct { | ||
| name string | ||
| opts []Option | ||
| expectedType string // "cdiHookCreator" or "allDisabledHookCreator" | ||
| expectedPath string | ||
| expectedDisabled map[HookName]bool | ||
| }{ |
There was a problem hiding this comment.
I have pushed a commit with a proposal for testing the constructor. We can define testCases as:
testCases := []struct {
name string
opts []Option
expected HookCreator
}{
and clean up the implementation significantly.
internal/discover/hooks_test.go
Outdated
| o := &hookCreatorOptions{ | ||
| nvidiaCDIHookPath: defaultNvidiaCDIHookPath, | ||
| } | ||
| opts := []Option{ | ||
| WithDisabledHooks(tt.disabledHooks...), | ||
| WithEnabledHooks(tt.enabledHooks...), | ||
| } | ||
| for _, opt := range opts { | ||
| opt(o) | ||
| } | ||
| o.disabledHooks = append(o.disabledHooks, defaultDisabledHooks...) | ||
|
|
||
| disabledHooks := make(map[HookName]bool) | ||
| for _, h := range o.disabledHooks { | ||
| disabledHooks[h] = true | ||
| } | ||
|
|
||
| for _, h := range o.enabledHooks { | ||
| disabledHooks[h] = false | ||
| } | ||
|
|
||
| testCreator := &cdiHookCreator{ | ||
| nvidiaCDIHookPath: o.nvidiaCDIHookPath, | ||
| disabledHooks: disabledHooks, | ||
| fixedArgs: getFixedArgsForCDIHookCLI(o.nvidiaCDIHookPath), | ||
| } | ||
|
|
||
| require.Equal(t, testCreator.isDisabled(tt.hookName, tt.args...), tt.expectedResult) |
There was a problem hiding this comment.
Why are we reimplementing the contructor here? What exactly are we testing?
There was a problem hiding this comment.
The rationale behind the way this test is built is that we want to test the CDIHookCreator.isDisabled directly without testing the NewHookCreator func.
I'll simplify this. I realized that we can trim down to focus on creating the cdiHookCreator
There was a problem hiding this comment.
As a note. If we wanted to test isDisabled directly then just have the testCases struct define a cdiHookCreator member directly. We can set that the way we want it WITHOUT having to reimplement the constructor.
ArangoGutierrez
force-pushed
the
i-1218
branch
from
d5f640d to
d30175a
Compare
ArangoGutierrez
force-pushed
the
i-1218
branch
from
d30175a to
19a786e
Compare
| }, | ||
| }, | ||
| { | ||
| name: "multiple hooks disabled and enabled", |
There was a problem hiding this comment.
nit: This doesn't test multiple enabled hooks.
| }, | ||
| }, | ||
| { | ||
| name: "WithDisabledHooks can be called multiple times", |
There was a problem hiding this comment.
Do we need a similar test for WithEnabledHooks?
There was a problem hiding this comment.
Not really, as we are calling the NewHookCreator during the create test
| { | ||
| name: "nvidia-ctk in custom NVIDIA toolkit path", | ||
| opts: []Option{ | ||
| WithNVIDIACDIHookPath("/usr/local/nvidia/toolkit/nvidia-ctk"), |
There was a problem hiding this comment.
Nit: Let's use the same custom path as for the nvidia-cdi-hook tests.
internal/discover/hooks_test.go
Outdated
| { | ||
| name: "nvidia-cdi-hook in /usr/local/bin", | ||
| opts: []Option{ | ||
| WithNVIDIACDIHookPath("/usr/local/bin/nvidia-cdi-hook"), | ||
| }, | ||
| expected: &cdiHookCreator{ | ||
| nvidiaCDIHookPath: "/usr/local/bin/nvidia-cdi-hook", | ||
| fixedArgs: []string{"nvidia-cdi-hook"}, | ||
| disabledHooks: map[HookName]bool{ | ||
| ChmodHook: true, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This test doesn't test anything that isn't already covered by the custom path test already added.
internal/discover/hooks_test.go
Outdated
| disabledHooks := make(map[HookName]bool) | ||
| for _, h := range tt.disabledHooks { | ||
| disabledHooks[h] = true | ||
| } | ||
| for _, h := range tt.enabledHooks { | ||
| disabledHooks[h] = false | ||
| } | ||
|
|
||
| testCreator := &cdiHookCreator{ | ||
| nvidiaCDIHookPath: defaultNvidiaCDIHookPath, | ||
| disabledHooks: disabledHooks, | ||
| fixedArgs: getFixedArgsForCDIHookCLI(defaultNvidiaCDIHookPath), | ||
| } | ||
|
|
||
| require.Equal(t, testCreator.isDisabled(tt.hookName, tt.args...), tt.expectedResult) |
There was a problem hiding this comment.
I already proposed this change once:
| disabledHooks := make(map[HookName]bool) | |
| for _, h := range tt.disabledHooks { | |
| disabledHooks[h] = true | |
| } | |
| for _, h := range tt.enabledHooks { | |
| disabledHooks[h] = false | |
| } | |
| testCreator := &cdiHookCreator{ | |
| nvidiaCDIHookPath: defaultNvidiaCDIHookPath, | |
| disabledHooks: disabledHooks, | |
| fixedArgs: getFixedArgsForCDIHookCLI(defaultNvidiaCDIHookPath), | |
| } | |
| require.Equal(t, testCreator.isDisabled(tt.hookName, tt.args...), tt.expectedResult) | |
| testCreator := NewHookCreator( | |
| WithDisabledHooks(tt.disabledHooks...), | |
| WithEnabledHooks(tt.enabledHooks...), | |
| ) | |
| require.Equal(t, testCreator.(*cdiHookCreator).isDisabled(tt.hookName, tt.args...), tt.expectedResult) | |
| }) |
If you left it this way to test the "AllDisabled" cases, then let's rather add those to the Create tests above:
+ {
+ name: "allDisabled with unknown hook",
+ hookCreator: &allDisabledHookCreator{},
+ hookName: HookName("unknown-hook"),
+ args: []string{},
+ expectedHook: nil,
+ },
+ {
+ name: "allDisabled with defined",
+ hookCreator: &allDisabledHookCreator{},
+ hookName: UpdateLDCacheHook,
+ args: []string{},
+ expectedHook: nil,
+ },
There was a problem hiding this comment.
I acknowledge you proposed the change, but I ended up deciding for the current implementation, as the testCreator during the AllDisabled returns a allDisabledHookCreator which doesn't have an isDisabled method.
This is the main reason I decided on the current implementation.
With that said, let me add a change that might work.
There was a problem hiding this comment.
Ok, I adopted your suggestion and added the AllHooks test to the Create section
ArangoGutierrez
force-pushed
the
i-1218
branch
from
19a786e to
f7a710b
Compare
ArangoGutierrez
force-pushed
the
i-1218
branch
from
f7a710b to
bf77d1e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR disables the chmod container hook by default, addressing a workaround that is no longer needed since crun v1.7 fixed the underlying issue. The chmod hook was originally added to handle device node permissions in subdirectories of /dev but is now redundant.
Key changes:
- Modified hook creator to disable chmod hook by default
- Added explicit hook enabling/disabling functionality
- Updated CDI generation to support explicit hook enabling
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/nvcdi/options.go |
Adds WithEnabledHooks option and deprecates singular WithDisabledHook |
pkg/nvcdi/lib.go |
Adds enabledHooks field and passes it to hook creator |
internal/runtime/runtime_factory.go |
Minor formatting change (blank line addition) |
internal/discover/hooks_test.go |
Comprehensive test suite for hook creation and disabling functionality |
internal/discover/hooks.go |
Core implementation to disable chmod hook by default and support enabled hooks |
cmd/nvidia-ctk/cdi/generate/generate_test.go |
Test case validating chmod hook can be explicitly enabled |
cmd/nvidia-ctk/cdi/generate/generate.go |
CLI flag for enabling hooks and validation logic |
cmd/nvidia-ctk-installer/toolkit/toolkit_test.go |
Updates expected output to include additional device node |
ArangoGutierrez
force-pushed
the
i-1218
branch
from
bf77d1e to
568194d
Compare
|
@elezar commits squashed |
ArangoGutierrez
force-pushed
the
i-1218
branch
from
568194d to
a9727db
Compare
This change disables the chmod hook by default. This hook was added as a workaround for a specific crun bug and should no longer be necessary in the most cases. In addition the ability to explicitly enable disabled hooks was added to both the nvcdi API and nvidia-ctk cdi generate command so that users can opt-in to the chmod hook using cdi if required. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]> Signed-off-by: Evan Lezar <[email protected]>
3 participants
Fixes #1218
The chmod createContainer hook was added as a workaround to a specific
crunissue for device nodes that are found in subdirectories of/dev. Since this has since been addressed incrun, this PR disables the chmod hook by default to reduce the number of supported hooks.For users that require this hook, it is recommended that
cdibe used and that the--enable-hook=chmodis passed to thenvidia-ctk cdi generatecommand.See: