Add DRA NUMA list attribute - #283
Conversation
✅ Deploy Preview for dranet canceled.
|
| // WatchHealthStatus implements the Kubernetes 1.37 DRAPlugin contract. DRANET | ||
| // disables the optional health service. | ||
| func (np *NetworkDriver) WatchHealthStatus(context.Context, chan<- kubeletplugin.DeviceHealthReport) error { | ||
| return kubeletplugin.ErrHealthNotSupported |
There was a problem hiding this comment.
This is required in 1.37 - but not implemented as part of this change. I imagine it needs a good amount more conversation.
There was a problem hiding this comment.
Is have a stub implementation required or optional?
There was a problem hiding this comment.
It's a required part of the interface in 1.37 - it was added to the DRAPlugin interface as part of this change.
There was a problem hiding this comment.
@killianmuldoon please add a TODO, we already report health so implementing this sounds reasonable if helps reliability ... although I want to understand better this feature , how it works and what problems solve
There was a problem hiding this comment.
WatchHealthStatus is about device health that can be propagated all the way to the workload pod's status containerStatus.AllocatedResourcesStatus if the device claimed by the the pod becomes unhealthy. It is sort of alternative (if we can say so) to the events which is general but this is specifically for the DRA drivers.
There was a problem hiding this comment.
we definitively should have this , but also put some user stories on how we want to implement it , opened #293 for tracking, just add TODO and reference that issue please
There was a problem hiding this comment.
I added a TODO with a link to the issue - let me know if you'd prefer a different format or wording.
gauravkghildiyal
left a comment
There was a problem hiding this comment.
Thanks @killianmuldoon
| // WatchHealthStatus implements the Kubernetes 1.37 DRAPlugin contract. DRANET | ||
| // disables the optional health service. | ||
| func (np *NetworkDriver) WatchHealthStatus(context.Context, chan<- kubeletplugin.DeviceHealthReport) error { | ||
| return kubeletplugin.ErrHealthNotSupported |
There was a problem hiding this comment.
Is have a stub implementation required or optional?
gauravkghildiyal
left a comment
There was a problem hiding this comment.
Thanks @killianmuldoon. Took a closer look, mostly looking good.
One request I had was if we could be a slightly more detailed with the release notes. For example, let's include the full attribute name of the attribute being added. Also, there is this bit of nuance with the standard NUMA attribute that I remember discussing which we should be linking to https://www.kubernetes.dev/resources/keps/6072/#consumer-expectations.
| // WatchHealthStatus implements the Kubernetes 1.37 DRAPlugin contract. DRANET | ||
| // disables the optional health service. | ||
| func (np *NetworkDriver) WatchHealthStatus(context.Context, chan<- kubeletplugin.DeviceHealthReport) error { | ||
| return kubeletplugin.ErrHealthNotSupported |
| db.deviceAttributeMachineModifiers..., | ||
| ) | ||
| if err != nil { | ||
| klog.V(4).Infof("Not publishing standardized NUMA attribute for PCI device %s: %v", normalizedAddr, err) |
There was a problem hiding this comment.
Should we log this as Error instead?
(Matches the other standard attribute GetPCIeRootAttributeByPCIBusID handling)
There was a problem hiding this comment.
I'm not so sure here - the error could be very noisy if there's devices without NUMA information. That could potentially happen on some VM setups. That said I'm happy either way here.
There was a problem hiding this comment.
You're right. Thinking about this more, GetPCIeRootAttributeByPCIBusID has a less chance of failing compared to NUMA (just because a PCI Device is more than likely to have a PCIe root, but not necessary have NUMA information populated). Sorry for asking you to change it back to the original.
There was a problem hiding this comment.
No issue changing it back - set it to Info now!
| _ = db.addPCIAttributes(devices, nil) | ||
| }) | ||
|
|
||
| t.Run("publishes standardized scalar NUMA attribute by default", func(t *testing.T) { |
There was a problem hiding this comment.
The tests are helpful. Thanks for adding them!
@gauravkghildiyal any special reason to feature gate this option? is a read only attribute from a kubernetes well defined value so it feels we should expose it once we move to 1.37 |
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
f126de3 to
ea393a7
Compare
This is slightly confusing and non-ideal from upstream itself, but the idea is that the attribute Defining a "list" type attribute is still alpha in DRA. So what this change is feature gating is NOT whether to publish the attribute, but whether to publish it as a scalar or a list. If you think when we change this from scalar to list then it would be a breaking change, you are right, but that's the recommendation https://www.kubernetes.dev/resources/keps/6072/#consumer-expectations without an alternative option. |
|
@killianmuldoon One last thing and we should be good: #283 (comment) |
Signed-off-by: Killian Muldoon <kmuldoon@nvidia.com>
|
/label tide/merge-method-squash |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauravkghildiyal, killianmuldoon 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR uses the upstream DRA NUMA helpers to enable reporting of the upstream standard NUMA List attribute in DRANet where available. The list attribute is described in a KEP. It enables matching by closest NUMA on more complex hardware where the network device NUMA node may not directly match the node of other hardware.
A bump to Kubernetes 1.37-rc.0 libraries is included as a dependency. This feature is enabled with a feature gate on Kubernetes 1.37, and the upstream helper libraries are only available in 1.37 tags.
Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
Does this PR introduce a user-facing change?