treewide: preliminary b300 support - #2566
Conversation
d6453e5 to
aa3bec1
Compare
| "nvidia.com/GB110_B300_SXM6_AC", | ||
| "nvidia.com/GH100_H100_PCIE", | ||
| // TODO(msanft): remove when the GPU operator is updated in all clusters. | ||
| "nvidia.com/pgpu", |
There was a problem hiding this comment.
I don't remember what the reason was in Contrast to use pgpu instead of specific names. The user docs also recommend this. Was that related to the gpu-operator and will this change with the new version?
There was a problem hiding this comment.
Was that related to the gpu-operator and will this change with the new version?
Exactly. Hence the comment
There was a problem hiding this comment.
I vaguely remember some hacky patch inside Contrast to make specific names work esp. for Privatemode, which code path we didn't want to be taken for the general audience. But I might remember that wrong.
There was a problem hiding this comment.
I think we (i.e. @charludo, iirc) removed that in Kata at some point when we didn't need it anymore?
There was a problem hiding this comment.
I'm not sure exactly what patch you mean, do you remember when it was dropped?
In any case, iiuc, specific names should work: https://github.com/edgelesssys/contrast/blob/main/packages/by-name/kata/source/0018-shim-guess-CDI-devices-without-direct-match.patch
There was a problem hiding this comment.
#2180 says that that patch "is brittle and should not be relied upon". I don't know why this is or whether this has changed.
charludo
left a comment
There was a problem hiding this comment.
Thanks for your efforts! This generally reads good to me, I have a couple nits in the comments + the below:
- there's a leftover comment in genpolicy/package.nix:58 that should probably also go in the "fix outdated comments" commit.
aa3bec1 to
a49b754
Compare
charludo
left a comment
There was a problem hiding this comment.
Thanks for the changes! LGTM, also since nothing existing seems to break.
d977ddc to
bda5598
Compare
Do the documentation changes need to be backported?Changes to
|
|
| --set sandboxDevicePlugin.version=8e76fe81 \ | ||
| --set 'sandboxDevicePlugin.env[0].name=P_GPU_ALIAS' \ | ||
| --set 'sandboxDevicePlugin.env[0].value=pgpu' \ | ||
| --set 'sandboxDevicePlugin.env[0].value=' \ |
There was a problem hiding this comment.
I think setting this to empty is wrong for this version of the gpu-operator
There was a problem hiding this comment.
This script now mixes stuff for the old gpu-operator and the new gpu-operator. The result will probably not work for either.
|
|
||
| # TODO: Remove this workaround once the sandbox device plugin includes PCI ID 0x3182. | ||
| # https://github.com/NVIDIA/gpu-operator/pull/2625 | ||
| patch_b300_pci_ids() { |
There was a problem hiding this comment.
The patch seems overly complicated. Can't we just put an up-to-date pci.ids in a ConfigMap and mount that into the main container to override the file?
There was a problem hiding this comment.
Yes. I haven't got to review the patch yet, but I've verified it works for the new version of the operator.
bda5598 to
5f2c877
Compare
d40eeba to
f9b48d6
Compare
8b09038 to
4cdbfcb
Compare
d484143 to
4794cb8
Compare
4794cb8 to
f91f9cb
Compare
In anticipation of Privatemode's B300 use, we can already prepare some (but not all) places in Contrast for B300 GPU support. For now, this PR uses the
GB110_B300_SXM6_ACdevice ID from this table and the name normalization logic in the sandbox-device-plugin.Closes CON-290