Apply validator resources to init containers#2557
Conversation
73f0040 to
98cfccd
Compare
|
Thanks @Shivkumar13 , the changes look fine to me. We should be doing this for all driver daemonsets managed by gpu-operator. For example, we are still missing requests/limits for k8s-driver-manager init container in driver daemonset. There are few other init containers as well which are also missing requests/limits. |
|
@rahulait should we do that in this PR or separate ones? |
|
Lets do them in single PR if possible. Its easier to review together and keep things consistent and tied to single commit. |
|
@tariq1890 any objections to the change being done here? |
Agreed, we should be making this change in this PR. This behavioural change should applied consistently to all components managed by the operator |
98cfccd to
0af5a73
Compare
|
|
||
| // apply plugin configuration through ConfigMap if one is provided | ||
| err = handleDevicePluginConfig(obj, config) | ||
| err = handleDevicePluginConfig(obj, config, config.DevicePlugin.Resources) |
There was a problem hiding this comment.
If Resources is nested within config, why add one more function argument? Can't we just access .DevicePlugin.Resources from the existing config function arg?
|
|
||
| // apply spec changes to make custom configurations provided via a ConfigMap available to all containers | ||
| func handleDevicePluginConfig(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec) error { | ||
| func handleDevicePluginConfig(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec, resources *gpuv1.ResourceRequirements) error { |
There was a problem hiding this comment.
| func handleDevicePluginConfig(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec, resources *gpuv1.ResourceRequirements) error { | |
| func handleDevicePluginConfig(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec) error { |
| } | ||
|
|
||
| func transformConfigManagerInitContainer(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec) error { | ||
| func transformConfigManagerInitContainer(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec, resources *gpuv1.ResourceRequirements) error { |
There was a problem hiding this comment.
| func transformConfigManagerInitContainer(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec, resources *gpuv1.ResourceRequirements) error { | |
| func transformConfigManagerInitContainer(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec) error { |
| if config.DevicePlugin.ImagePullPolicy != "" { | ||
| initContainer.ImagePullPolicy = gpuv1.ImagePullPolicy(config.DevicePlugin.ImagePullPolicy) | ||
| } | ||
| applyResourceRequirements(initContainer, resources) |
There was a problem hiding this comment.
| applyResourceRequirements(initContainer, resources) | |
| applyResourceRequirements(initContainer, config.DevicePlugin.Resources) |
|
This PR does not update the DRA driver DaemonSet at manifests/state-dra-driver/0500_daemonset.yaml. Its driver-validation init container still has no resources. Since both the regular containers can have their own requests/limits, we should use the max of them for init container. |
Signed-off-by: Shivkumar Ople <sople@nvidia.com>
0af5a73 to
bd73cd3
Compare
3 participants
Fixes #702.
This change applies the existing
spec.validator.resourcessettings to validator init containers as well as the main validator container. The affected init containers run the validator image for driver, toolkit, CUDA, and plugin checks, so using the existing validator resource field keeps the API surface unchanged while making those init containers configurable.Validation:
go test ./controllers -run TestTransformValidatorComponentWithResources -count=1go test ./controllers -run TestTransformValidator -count=1go test ./controllers -run TestTransformSandboxValidator -count=1