Add explicit envvar to control if we mask /proc/driver/nvidia/params #198
Conversation
| @@ -35,6 +35,7 @@ selectorLabelsOverride: {} | |||
| allowDefaultNamespace: false | |||
|
|
|||
| deviceClasses: ["gpu", "mig", "imex"] | |||
| maskNvidiaDriverParams: false | |||
There was a problem hiding this comment.
do we want to add a description to the value to call out that this is for testing under kind with multiple nodes?
There was a problem hiding this comment.
Also, should this be a top-level option?
There was a problem hiding this comment.
Happy to add some comments around this in the values file. Should I remove the comments in the template file then or have them in both places?
Regarding top-level option vs. not -- I thought about that and don't really have a strong preference. It's only relevant on nodes that have GPUs (i.e. the kubeletplugin in this component), but all of the other options there are native k8s primitives, so it felt weird to add this one off one that was not. What is your preference?
There was a problem hiding this comment.
I would assume that a user would first inspect the values file to determine what can be set and not necessarily go as deep as the template file.
I would summarise it as follows: the comment in the template file is for us, and the comment in the values file is for users.
What is your preference?
I prefer grouping options for readability, but I can also see that doing this early is not ideal -- especially if we don't have something that makes sense as a logical unit. Let's leave it top-level for now.
There was a problem hiding this comment.
Updated the comment in the template to be more geared towards us rather than the user.
klueska
force-pushed
the
explicit-envvar-for-mask
branch
2 times, most recently
from
0ebb992
to
8f9ea13
Compare
| - name: MASK_NVIDIA_DRIVER_PARAMS | ||
| value: "{{ .Values.maskNvidiaDriverParams }}" |
There was a problem hiding this comment.
My one final question would be whether we want to inject this conditionally -- i.e. only if the value is specified and have nil as a default? Not a blocker though.
The one disadvantage of this is that the type of the setting is not apparent from reading the default.
There was a problem hiding this comment.
I don't think its necessary. If / when we do that we should do it for all options to be symmetrical.
klueska
force-pushed
the
explicit-envvar-for-mask
branch
from
8f9ea13
to
9b26828
Compare
Signed-off-by: Kevin Klues <[email protected]>
klueska
force-pushed
the
explicit-envvar-for-mask
branch
from
9b26828
to
6f2c2aa
Compare
No description provided.