[2/3] Add 'Devlink Params configuration' implementation#1036
[2/3] Add 'Devlink Params configuration' implementation#1036e0ne wants to merge 8 commits intok8snetworkplumbingwg:masterfrom
Conversation
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com> fd
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
fix: Don't modify multiport param by default
When SriovNetworkNodePolicy specifies groupingPolicy: all, the operator should create a single OVS bridge with all matching PFs as uplinks. Previously, it was incorrectly creating separate bridges for each PF.
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Summary of ChangesHello @e0ne, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the SR-IOV Network Operator's capabilities by introducing comprehensive support for configuring devlink parameters on network interface cards. It provides a new API for users to specify these parameters, implements the underlying logic for their discovery and application on host machines, and integrates this functionality into the existing configuration and reconciliation loops. Additionally, it refines OVS bridge management to support grouping multiple uplinks into a single bridge, enhancing flexibility for complex network setups. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for configuring devlink parameters and adds a grouping policy for OVS bridges. The changes span across the API types, CRDs, helper functions, and vendor-specific logic. The implementation looks mostly correct, but I've found a few issues related to efficiency, error handling, and robustness. Specifically, there's an inefficient loop that can be optimized, an ignored error that could lead to silent failures, and a case-sensitive comparison that could cause issues with user-provided configurations. I've also pointed out a minor typo and some confusing text in the new design document. My suggestions aim to address these points to improve the quality and reliability of the new features.
| for _, v := range p.Values { | ||
| // only CLI-visible cmodes | ||
| strVal, _ := n.devlinkValueToString(p.Type, v.Data) | ||
| switch v.CMODE { | ||
| case nl.DEVLINK_PARAM_CMODE_RUNTIME, | ||
| nl.DEVLINK_PARAM_CMODE_DRIVERINIT, | ||
| nl.DEVLINK_PARAM_CMODE_PERMANENT: | ||
| merged[p.Name][v.CMODE] = strVal | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The error returned by n.devlinkValueToString is being ignored. This could lead to silent failures if the value conversion fails, potentially causing incorrect devlink parameters to be processed or stored. The error should be checked and handled appropriately.
for _, v := range p.Values {
// only CLI-visible cmodes
strVal, err := n.devlinkValueToString(p.Type, v.Data)
if err != nil {
log.Log.Error(err, "Failed to decode devlink param value", "device", pciAddr, "param", p.Name)
return nil, err
}
switch v.CMODE {
case nl.DEVLINK_PARAM_CMODE_RUNTIME,
nl.DEVLINK_PARAM_CMODE_DRIVERINIT,
nl.DEVLINK_PARAM_CMODE_PERMANENT:
merged[p.Name][v.CMODE] = strVal
}
}| } | ||
| for _, param := range iface.DevlinkParams.Params { | ||
| var err error | ||
| switch param.ApplyOn { |
There was a problem hiding this comment.
The CRD for DevlinkParam allows applyOn to be specified in both uppercase and lowercase (e.g., PF, pf). However, this switch statement performs a case-sensitive comparison against uppercase constants. This will cause the logic to fail for lowercase values. To make the implementation robust, you should convert the param.ApplyOn value to uppercase before the comparison.
| switch param.ApplyOn { | |
| switch strings.ToUpper(param.ApplyOn) { |
| for _, uplink := range uplinks { | ||
| state.Spec.Bridges.OVS = slices.DeleteFunc(state.Spec.Bridges.OVS, func(br OVSConfigExt) bool { | ||
| return slices.ContainsFunc(br.Uplinks, func(u OVSUplinkConfigExt) bool { | ||
| return u.PciAddress == uplink.PciAddress | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The current implementation for removing existing individual per-PF bridges is inefficient. It iterates through all uplinks and, for each, re-scans and re-slices state.Spec.Bridges.OVS. This results in a complexity of roughly O(U * B), where U is the number of uplinks and B is the number of bridges, and involves multiple memory allocations. A more efficient approach would be to first collect all PCI addresses of the uplinks into a set for quick lookups, and then iterate through the bridges once to remove the ones that contain any of these uplinks.
| for _, uplink := range uplinks { | |
| state.Spec.Bridges.OVS = slices.DeleteFunc(state.Spec.Bridges.OVS, func(br OVSConfigExt) bool { | |
| return slices.ContainsFunc(br.Uplinks, func(u OVSUplinkConfigExt) bool { | |
| return u.PciAddress == uplink.PciAddress | |
| }) | |
| }) | |
| } | |
| pciAddressesToRemove := make(map[string]struct{}) | |
| for _, uplink := range uplinks { | |
| pciAddressesToRemove[uplink.PciAddress] = struct{}{} | |
| } | |
| state.Spec.Bridges.OVS = slices.DeleteFunc(state.Spec.Bridges.OVS, func(br OVSConfigExt) bool { | |
| for _, u := range br.Uplinks { | |
| if _, found := pciAddressesToRemove[u.PciAddress]; found { | |
| return true | |
| } | |
| } | |
| return false | |
| }) |
| SriovNetworkPoolConfig is used only for OpenShift to provide configuration for | ||
| OVS Hardware Offloading. We can extend it to add configuration for the drain | ||
| pool. E.g.: |
There was a problem hiding this comment.
This paragraph seems to be a copy-paste error from another document. It discusses SriovNetworkPoolConfig and drain pools, which is unrelated to the devlink parameters being added to SriovNetworkNodePolicy in this design. This should be removed to avoid confusion.
| requsted, found := false, false | ||
| for _, devlinkParam := range iface.DevlinkParams.Params { | ||
| if devlinkParam.Name != devlinkMultiportParamName { | ||
| continue | ||
| } | ||
| requsted = true | ||
|
|
||
| for _, devlinkParamStatus := range ifaceStatus.DevlinkParams.Params { | ||
| if devlinkParamStatus.Name == devlinkMultiportParamName { | ||
| found = true | ||
| if iface.Name == ifaceStatus.Name && devlinkParam.Value != devlinkParamStatus.Value { | ||
| return true, &devlinkParam | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if requsted && !found { |
There was a problem hiding this comment.
There is a typo in the variable name requsted. It should be requested for better readability and correctness. This should be fixed in all its occurrences within this function.
| requsted, found := false, false | |
| for _, devlinkParam := range iface.DevlinkParams.Params { | |
| if devlinkParam.Name != devlinkMultiportParamName { | |
| continue | |
| } | |
| requsted = true | |
| for _, devlinkParamStatus := range ifaceStatus.DevlinkParams.Params { | |
| if devlinkParamStatus.Name == devlinkMultiportParamName { | |
| found = true | |
| if iface.Name == ifaceStatus.Name && devlinkParam.Value != devlinkParamStatus.Value { | |
| return true, &devlinkParam | |
| } | |
| } | |
| } | |
| } | |
| if requsted && !found { | |
| requested, found := false, false | |
| for _, devlinkParam := range iface.DevlinkParams.Params { | |
| if devlinkParam.Name != devlinkMultiportParamName { | |
| continue | |
| } | |
| requested = true | |
| for _, devlinkParamStatus := range ifaceStatus.DevlinkParams.Params { | |
| if devlinkParamStatus.Name == devlinkMultiportParamName { | |
| found = true | |
| if iface.Name == ifaceStatus.Name && devlinkParam.Value != devlinkParamStatus.Value { | |
| return true, &devlinkParam | |
| } | |
| } | |
| } | |
| } | |
| if requested && !found { |
2 participants
No description provided.