dra: attributes: compatibility with dra-driver-sriov#65
dra: attributes: compatibility with dra-driver-sriov#65ffromani wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
Add compatibility with dra-driver-sriov to enable NUMA alignment. Reviewing https://github.com/k8snetworkplumbingwg/dra-driver-sriov/blob/main/pkg/consts/consts.go this seems the only attribute which makes sense to expose. Signed-off-by: Francesco Romani <fromani@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
/cc @pravk03 not urgent but this is a one-liner (actually a two-liner!) that I think we should add in 0.1.0. |
|
Thanks. The change looks good, but this previous discussion highlighted some concerns with exposing NUMA as a standard attribute. An alternative would be to use list attributes and align based on PCIE root complex after kubernetes/enhancements#5491 is implemented, but that might take some time. Given that the SRIOV driver already exposes this attribute, it seems reasonable to include the same attribute here as a temporary solution (similar to the DRANet attribute). Maybe we should also re-initiate the discussion on standardizing NUMA and socket attributes for the long term? /lgtm |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
I share your concerns. And I raised the point about attributes in the last wg-device-management meeting. I think is worth exploring if and how we can expose the pcieRoot attribute instead. It's worth exploring if we can somehow bind core groups to this physical property in a safe manner, and I think it's worth sketching out this approach before to move forward with this PR,. |
|
Exploring sysfs a bit, I think there could be a good way to group cpu cores by pcieRoot affinity. This will call for a new groupBy mode, and this will allow us to expose the standard pcieRoot attribute avoiding the complexity entirely. I'll explore this option next week and hopefully push a PR. |
| // TODO(pravk03): Remove. Hack to align with NIC (DRANet). We need some standard attribute to align other resources with CPU. | ||
| "dra.net/numaNode": {IntValue: &numaID}, | ||
| // dra-driver-sriov (https://github.com/k8snetworkplumbingwg/dra-driver-sriov) compatibility | ||
| deviceattribute.StandardDeviceAttributePrefix + "numaNode": {IntValue: &numaID}, |
There was a problem hiding this comment.
A big 👎 on this. A driver should never use the reserved namespace for attributes that haven't been defined by Kubernetes.
At this point, numaNode is something that has to live inside a driver's own namespace.
|
thanks @pohly , the feedback is clear. Compatibility needs to be achieved using a different mean, and (ab)using the kubernetes namespace for extra attributes is a bad practice we should try to rectify rather than follow. I think there's a promising avenue to reach compatibility for the CPU (and memory) using the pcieRoot attribute, we will pursue in a upcoming PR. |
Drivers should not use the reserved Kubernetes namespace (resource.kubernetes.io/) for attributes that have not been officially defined by Kubernetes. The numaNode attribute was never standardized and using it under the standard prefix is a bad practice that could create compatibility issues as more drivers adopt it without upstream consensus. This commit removes the numaNode attribute entirely from: - Device attributes exposed on SR-IOV VFs - SriovResourceFilter CRD (numaNodes filter field) - Resource filter controller logic - Host interface (GetNumaNode) - All examples and documentation Users who need topology-aware resource alignment should use the standard pcieRoot attribute (resource.kubernetes.io/pcieRoot) instead, which is officially defined by Kubernetes and already exposed by this driver. The pcieRoot attribute provides PCIe Root Complex affinity which achieves similar locality constraints without abusing the reserved attribute namespace. Example migration for DeviceConstraints: Before: matchAttribute: "resource.kubernetes.io/numaNode" After: matchAttribute: "resource.kubernetes.io/pcieRoot" Ref: kubernetes-sigs/dra-driver-cpu#65 (comment) Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Drivers should not use the reserved Kubernetes namespace (resource.kubernetes.io/) for attributes that have not been officially defined by Kubernetes. The numaNode attribute was never standardized and using it under the standard prefix is a bad practice that could create compatibility issues as more drivers adopt it without upstream consensus. This commit removes the numaNode attribute entirely from: - Device attributes exposed on SR-IOV VFs - SriovResourceFilter CRD (numaNodes filter field) - Resource filter controller logic - Host interface (GetNumaNode) - All examples and documentation Users who need topology-aware resource alignment should use the standard pcieRoot attribute (resource.kubernetes.io/pcieRoot) instead, which is officially defined by Kubernetes and already exposed by this driver. The pcieRoot attribute provides PCIe Root Complex affinity which achieves similar locality constraints without abusing the reserved attribute namespace. Example migration for DeviceConstraints: Before: matchAttribute: "resource.kubernetes.io/numaNode" After: matchAttribute: "resource.kubernetes.io/pcieRoot" Ref: kubernetes-sigs/dra-driver-cpu#65 (comment) Signed-off-by: Sebastian Sch <sebassch@gmail.com>
Drivers should not use the reserved Kubernetes namespace (resource.kubernetes.io/) for attributes that have not been officially defined by Kubernetes. The numaNode attribute was never standardized and using it under the standard prefix is a bad practice that could create compatibility issues as more drivers adopt it without upstream consensus. This commit removes the numaNode attribute entirely from: - Device attributes exposed on SR-IOV VFs - SriovResourceFilter CRD (numaNodes filter field) - Resource filter controller logic - Host interface (GetNumaNode) - All examples and documentation Users who need topology-aware resource alignment should use the standard pcieRoot attribute (resource.kubernetes.io/pcieRoot) instead, which is officially defined by Kubernetes and already exposed by this driver. The pcieRoot attribute provides PCIe Root Complex affinity which achieves similar locality constraints without abusing the reserved attribute namespace. Example migration for DeviceConstraints: Before: matchAttribute: "resource.kubernetes.io/numaNode" After: matchAttribute: "resource.kubernetes.io/pcieRoot" Ref: kubernetes-sigs/dra-driver-cpu#65 (comment) Signed-off-by: Sebastian Sch <sebassch@gmail.com>
DRA drivers should not use the resource.kubernetes.io namespace for non-official attributes. They should use their own namespace. xref: kubernetes-sigs/dra-driver-cpu#65 (comment) Signed-off-by: Francesco Romani <fromani@redhat.com>
Drivers should not use the reserved Kubernetes namespace (resource.kubernetes.io/) for attributes that have not been officially defined by Kubernetes. The numaNode attribute was never standardized and using it under the standard prefix is a bad practice that could create compatibility issues as more drivers adopt it without upstream consensus. This commit removes the numaNode attribute entirely from: - Device attributes exposed on SR-IOV VFs - SriovResourceFilter CRD (numaNodes filter field) - Resource filter controller logic - Host interface (GetNumaNode) - All examples and documentation Users who need topology-aware resource alignment should use the standard pcieRoot attribute (resource.kubernetes.io/pcieRoot) instead, which is officially defined by Kubernetes and already exposed by this driver. The pcieRoot attribute provides PCIe Root Complex affinity which achieves similar locality constraints without abusing the reserved attribute namespace. Example migration for DeviceConstraints: Before: matchAttribute: "resource.kubernetes.io/numaNode" After: matchAttribute: "resource.kubernetes.io/pcieRoot" Ref: kubernetes-sigs/dra-driver-cpu#65 (comment) Signed-off-by: Sebastian Sch <sebassch@gmail.com>
|
we merged this one k8snetworkplumbingwg/dra-driver-sriov#70 so we are not using this anymore |
5 participants
Add compatibility with dra-driver-sriov to enable NUMA alignment. Reviewing https://github.com/k8snetworkplumbingwg/dra-driver-sriov/blob/main/pkg/consts/consts.go this seems the only attribute which makes sense to expose.