Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add nvidia MIG #258

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

piyush-jena
Copy link
Contributor

@piyush-jena piyush-jena commented Nov 13, 2024

Issue number:

Related:

Description of changes:
Adding nvidia-migmanager service and binary that configures the instance with nvidia mig.

Testing done:

  1. Instance joined the cluster
NAME                                           STATUS   ROLES    AGE   VERSION
ip-XXXX.us-west-2.compute.internal   Ready    <none>   15h   v1.29.5-eks-1109419
  1. Model Default:
bash-5.1# apiclient get settings.kubelet-device-plugin
{
  "settings": {
    "kubelet-device-plugins": {
      "nvidia": {
        "device-id-strategy": "index",
        "device-list-strategy": "volume-mounts",
        "device-partitioning-strategy": "none",
        "device-sharing-strategy": "none",
        "pass-device-specs": true
      }
    }
  }
}
  1. Model Updates:
bash-5.1#: apiclient set settings.kubelet-device-plugins.nvidia.device-partitioning-strategy="mig"
bash-5.1#: apiclient set settings.kubelet-device-plugins.nvidia.mig.profile."a100-40gb"="1g.5gb"
bash-5.1# apiclient get settings.kubelet-device-plugin
{
  "settings": {
    "kubelet-device-plugins": {
      "nvidia": {
        "device-id-strategy": "index",
        "device-list-strategy": "volume-mounts",
        "device-partitioning-strategy": "mig",
        "device-sharing-strategy": "none",
        "mig": {
          "profile": {
            "a100-40gb": "1g.5gb"
          }
        },
        "pass-device-specs": true
      }
    }
  }
}

kubectl describe node shows 56 gpus post instance reboot.

  1. Bounded check:
bash-5.1# apiclient apply <<EOF
> [settings.kubelet-device-plugins.nvidia.mig.profile]
> "hello"="1g.5gb"
> EOF
Failed to apply settings: Failed to PATCH settings from '-' to '/settings?tx=apiclient-apply-7NsnlaurtHEacSYL': Status 400 when PATCHing /settings?tx=apiclient-apply-7NsnlaurtHEacSYL: Json deserialize error: Unable to deserialize into NvidiaGPUModel: NVIDIA GPU Model must match '^([a-z])(\d+)\.(\d+)gb$', given: hello at line 1 column 62
bash-5.1# apiclient apply <<EOF
> [settings.kubelet-device-plugins.nvidia.mig.profile]
> "a100.40gb"="2"
> EOF
bash-5.1# apiclient apply <<EOF
> [settings.kubelet-device-plugins.nvidia.mig.profile]
> "a100.40gb"="5"
> EOF
Failed to apply settings: Failed to PATCH settings from '-' to '/settings?tx=apiclient-apply-GzUHB0axGlWNPzGw': Status 400 when PATCHing /settings?tx=apiclient-apply-GzUHB0axGlWNPzGw: Json deserialize error: Unable to deserialize into MIGProfile: MIG Profile must match '^[0-9]g\.\d+gb$', given: 5 at line 1 column 71
  1. Files generated:

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.

@piyush-jena piyush-jena changed the title feat: add nvidia mig feat: add nvidia MIG Nov 13, 2024
@piyush-jena piyush-jena marked this pull request as draft November 13, 2024 20:26
@piyush-jena piyush-jena force-pushed the nvidia-mig-feature branch 3 times, most recently from 6b025dc to d72bc52 Compare November 18, 2024 14:14
@piyush-jena piyush-jena force-pushed the nvidia-mig-feature branch 6 times, most recently from 4cb9254 to 385d4fe Compare January 24, 2025 23:54
@piyush-jena piyush-jena requested a review from bcressey January 24, 2025 23:56
@@ -1,5 +1,6 @@
d /run/cache 0755 root root -
d /run/lock 0755 root root -
d /run/prairiedog 0755 root root -
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The systemd package's tmpfiles snippet isn't the right place for this.

I'm also not convinced migmanager should be using prairiedog's temp directory, or that we want to route it all through prairiedog.

We could just add another program to reboot-if-required.service (like this, or via drop-in):

[Service]
Type=oneshot
ExecStart=/usr/bin/prairiedog reboot-if-required
ExecStart=-/usr/bin/nvidia-migmanager reboot-if-required

sources/nvidia-migmanager/src/main.rs Outdated Show resolved Hide resolved
sources/api/prairiedog/src/bootconfig.rs Outdated Show resolved Hide resolved
sources/api/prairiedog/src/bootconfig.rs Outdated Show resolved Hide resolved
sources/api/prairiedog/src/bootconfig.rs Outdated Show resolved Hide resolved
sources/nvidia-migmanager/src/main.rs Outdated Show resolved Hide resolved
Comment on lines +190 to +355
// If there a multiple devices with the same ID, dedup them to minimize iterations
let unique_ids = present_devices
.iter()
.map(|x| format!("0x{}", x.device().to_uppercase()).clone())
.collect::<HashSet<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the system has multiple GPUs, can each GPU be in a different state with respect to MIG?

sources/nvidia-migmanager/src/main.rs Outdated Show resolved Hide resolved
Comment on lines +190 to +352
// If there a multiple devices with the same ID, dedup them to minimize iterations
let unique_ids = present_devices
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code assumes that all GPUs will be the same, but it might improve it to abstract over the possibility of heterogenous GPUs, and to be precise about which GPU is being operated on.

In other words - return a Result<Vec<NvidiaGpu>>, ensure each NvidiaGpu entry has enough metadata to identify it in calls to nvidia-smi to query the device state for that GPU, and so on.

sources/nvidia-migmanager/src/main.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants