Add standard device attribute 'resource.kubernetes.io/pcieRoot' to GPU and MIG Devices. #401
Conversation
…o GPU and MIG devices Signed-off-by: Shingo Omura <[email protected]>
everpeace
force-pushed
the
add-pcieRoot-device-attribute
branch
from
ba062f0 to
bb72e60
Compare
|
@klueska Sorry, I just deleted my comment because I also thought https://github.com/NVIDIA/go-nvml is the right place where we should place the pcie root resolution logic as you pointed out. I will update the repo soon🙇 |
| func resolvePCIeRootFromBusID(pciBusID string) (string, error) { | ||
| devicePath, err := os.Readlink(filepath.Join("/sys/bus/pci/devices", pciBusID)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error resolving PCIe root from Bus ID '%s': %v", pciBusID, err) | ||
| } | ||
|
|
||
| regex := regexp.MustCompile(`^/sys/devices/pci[0-9a-f]{4}:[0-9a-f]{2}/.*$`) | ||
| if !regex.MatchString(devicePath) { | ||
| return "", fmt.Errorf("unexpected device path for PCIe device '%s': %s", pciBusID, devicePath) | ||
| } | ||
|
|
||
| return strings.Split(devicePath, "/")[2], nil | ||
| } |
There was a problem hiding this comment.
We need to make sure that everyone agrees on the same way to represent the value of this attributes in every driver (not just on the name of the attribute).
Seems like we should add a new pkg in our library code to build these standard atrributes for all drivers to use:
https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/dynamic-resource-allocation
There was a problem hiding this comment.
We need to make sure that everyone agrees on the same way to represent the value of this attributes in every driver (not just on the name of the attribute).
I think the KEP suggests DRA drivers scan /sys/devices/:
DRA drivers MAY determine this value by inspecting the hierarchical path of the device's entry in sysfs (e.g.,
/sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0), where thepci<domain>:<bus>segment at the beginning of the real path identifies the Root Complex (e.g.,pci0000:00).
This matches the official Linux Kernel Doc: https://docs.kernel.org/PCI/sysfs-pci.html. So, I think it's quite a standard way to resolve PCI Root(Domain and Bus).
Seems like we should add a new pkg in our library code to build these standard atrributes for all drivers to use:
https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/dynamic-resource-allocation
Yeah, agreed. However, I think these API will be bumped to v1 soon. I expect the v1 API will include standardized device attribute constant. So, I will catch up to it later.
There was a problem hiding this comment.
There is no API, as such, associated with that library pkg. It is free to change from release-to-release (even with breaking changes).
As such, I do think it's better to implement this logic in one place rather than to start with fragmented implementations in different drivers. You can even define the constant for the attribute there.
I can see it as a new standard-attributes package with a set of library calls to get specific standard attributes.
Happy to review such a change there.
There was a problem hiding this comment.
As such, I do think it's better to implement this logic in one place rather than to start with fragmented implementations in different drivers. You can even define the constant for the attribute there.
Ah! I got it, completely agreed!!!
I can see it as a new standard-attributes package with a set of library calls to get specific standard attributes.
Happy to review such a change there.
I'm interested in it. But, would you know any good OSS libraries to handle PCI devices?? Is Kubernetes ok to depend https://github.com/NVIDIA/go-nvml. I'm not sure. OR, implementing a thin helper method which takes a BusID in BDF format and scan /sys/devices dir like my implementation here??
WDYT??
There was a problem hiding this comment.
You definitely wont be able to pull in https://github.com/NVIDIA/go-nvml directly. It will need to be generically implemented against standard linux interfaces.
I think its pretty clear how you would get the root complex given a BudID in BDF format, so lets start there.
What I'm less clear on is how to get it for CPUs (which a DRA driver for will be coming soon).
There was a problem hiding this comment.
I think its pretty clear how you would get the root complex given a BudID in BDF format, so lets start there.
Cool. Thanks. Let me try 👍
There was a problem hiding this comment.
We need to make sure that everyone agrees on the same way to represent the value of this attributes in every driver (not just on the name of the attribute).
I was just precisely talked about this in google/dranet#114 (comment) that @michaelasp is implementing , we need to ensure we all expose the same value
As such, I do think it's better to implement this logic in one place rather than to start with fragmented implementations in different drivers. You can even define the constant for the attribute there.
+1000 I ended doing my own parsing https://github.com/google/dranet/blob/main/pkg/inventory/sysfs.go#L112-L195 but I prefer if we standardize the logic too
There was a problem hiding this comment.
+1 on this, having a standard library to get this value would make it less prone to parsing issues and misinterpretation of the kep.
There was a problem hiding this comment.
go-nvml is just a thin wrapper around the underlying C library for NVML -- we can't just add custom methods in there |
github-project-automation
bot
moved this to Backlog
in Planning Board: k8s-dra-driver-gpu
|
@klueska |
github-project-automation
bot
moved this from Backlog
to Closed
in Planning Board: k8s-dra-driver-gpu
fixes #400
ref: #213
This PR puts the standardized device attribute
resource.kubernetes.io/pcieRootto GPU and MIG devices.