source/pci: Group identical PCI entries #2031
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ozhuraki The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @ozhuraki. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Thanks @ozhuraki for working on this.
We should only group devices where all attributes are identical. So we need to contain all attributes in the lookup key (e.g. with json.Marshal or something).
Also, we'd probably need a config option and/or feature gate for this so that it can be turned off. I've seen usages where people create per-device labels (like dev-0, dev-1, ...) by looping over the devices so we don't want to completely break those.
|
@marquiz: GitHub didn't allow me to request PR reviews from the following users: ivelichkovich, yavinash007. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
ping @ozhuraki, any updates on this? I think this would be an important optimization for the next release |
Signed-off-by: Oleg Zhurakivskyy <[email protected]>
k8s-ci-robot
added
size/M
|
@marquiz |
| for _, device := range devices { | ||
| info, err := readPciDevInfo(filepath.Join(sysfsBasePath, device.Name())) | ||
| if err != nil { | ||
| klog.ErrorS(err, "failed to read PCI device info") | ||
| continue | ||
| } | ||
| devInfo = append(devInfo, *info) | ||
|
|
||
| b, err := json.Marshal(info.Attributes) |
There was a problem hiding this comment.
Wondering is there any benefit of having the vendor-device map[string]map[string]? What if just using b (or a hash of it) as a key to map[string]DevGroupedEntry (where DevGroupedEntry would contain the Count plus the nfdv1alpha1.InstanceFeature)?
|
|
||
| for _, dev := range devInfo { | ||
| entry := devGrouped[dev.Attributes["vendor"]][dev.Attributes["device"]] | ||
| dev.Attributes["count"] = string(entry.Count) |
There was a problem hiding this comment.
Need to use strconv for this
No description provided.