Fix nvidia fallback services failing to start#475
Conversation
ginglis13
left a comment
ginglis13
left a comment
There was a problem hiding this comment.
LGTM - nit on missing space after : in commit message kmod-nvidia:fallback.service to install on all variants
| select(.devid != "0x1DB1" | ||
| and .devid != "0x1DB5" | ||
| and .devid != "0x1DEB8" | ||
| and .devid != "0x1EB8" |
There was a problem hiding this comment.
In your PR description, can you add the testing you did here to validate that g4dn.* are using the correct driver. And add in the commit message that we are following the steps of the EKS Optimized AMI.
| @@ -11,4 +11,4 @@ ExecStart=/usr/bin/systemctl stop grid-license-check.timer --no-block | |||
| RemainAfterExit=true | |||
There was a problem hiding this comment.
The commit message is not well formatted and confusing. Could you please expand on how your change fixes the problem with non-k8s variants?
Also, should the service run After=nvidia-gridd.service? WantedBy doesn't guarantee ordering.
There was a problem hiding this comment.
Updated the commit message with more details.
The services can start at the same time as nvidia-gridd.service. They no-op if they are on an unsupported instance and will exit without failing the boot.
| StandardOutput=append:/tmp/.nvidia-gridd-license | ||
|
|
||
| [Install] | ||
| WantedBy=nvidia-k8s-device-plugin.service |
There was a problem hiding this comment.
In the commit message, or even in this file, explain that this unit doesn't need an Install section because the service will be started by a timer.
But also, you must guarantee that the service will start as soon as we reach multi-user.target. If you validated this, call it out in the PR description. And in the commit message, call out why the wanted ordering is preserved (e.g. timers.target run before, in parallel, after multi-user.target whichever is the case we want).
There was a problem hiding this comment.
Updated the commit message with more info about this.
We don't need to wait for multiuser.target for this service. It was already setup to run after nvidia-gridd.service which is installed in preconfigured.target. This change shouldn't affect the ordering or install since timers.target was always going to install this service.
Signed-off-by: Vighnesh Maheshwari <[email protected]>
This aligns with the EKS optimized AMI and their decision to use the tesla driver with g4dn instance types. The original PR had a typo and this change fixes that. Signed-off-by: Vighnesh Maheshwari <[email protected]>
The different nvidia driver services (tesla, open-gpu, and grid) were setup to run only for the nvidia-k8s-device-plugin.service. With this change, the driver services are now installed with the nvidia-gridd.service which runs on all nvidia instances The dependency flow is as below: timers.target (base systemd target) | -> Installs grid-license-check.timer | | | -> Installs grid-license-check.service | preconfigured.target (base systemd target) | -> Installs nvidia-gridd.service | -> Triggers grid-license-check.service (Existing behaviour) -> Triggers open-gpu-license-fallback.service (New) -> Triggers tesla-license-fallback.service (New) Signed-off-by: Vighnesh Maheshwari <[email protected]>
|
⬆️ Update following comments |
arnaldo2792
left a comment
arnaldo2792
left a comment
There was a problem hiding this comment.
One more nit in the PR description:
I understand what's changing with your code changes, but what's the behavior your change fixed? I understand the nvidia-gridd.service now starts in all variants, but what problem was this causing? Failed boots? Excessive logging? A service running that shouldn't? Or a service not running that should?
3 participants
Issue number:
Closes #464
Description of changes:
%buildsections to kmod-nvidia specfilesg4dninstances causing the open driver to be loaded instead of the tesla driver. With this change, we are in-line with what the EKS optimized AMI doesopen-gpu-license-fallback.serviceand thetesla-license-fallback.serviceto depend on thenvidia-gridd.servicewhich itself depends onpreconfigured.targetensuring all are started on all variants.Testing done:
Build and test the a combination of ecs and k8s variants on the 6.1, 6.12, and 6.18 kernels with the 580 driver
Testing for
g4dn.*instances:Before the change:
After the change
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.