Conversation
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Summary of ChangesHello @rollandf, 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 adds a new design document detailing the integration of Kubernetes Dynamic Resource Allocation (DRA) into the SR-IOV Network Operator. The primary goal is to modernize SR-IOV resource management by replacing the existing device plugin framework with DRA, offering more flexible and advanced capabilities for allocating Virtual Functions. The document describes the new workflow, API extensions, and a phased approach to implementation, ensuring a smooth transition and enhanced resource control for SR-IOV devices. 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
|
There was a problem hiding this comment.
Code Review
This is a very comprehensive and well-structured design document for integrating DRA into the SR-IOV Network Operator. The proposal is detailed, covering motivation, API changes, implementation phases, and testing. The phased approach is sensible, and the synchronization mechanism to prevent race conditions is well-thought-out. I've added a few comments to point out some minor inconsistencies and potential typos in the document that should be addressed for clarity.
| expression: | | ||
| device.driver == "sriov.k8snetworkplumbingwg.io" && | ||
| device.attributes["sriovnetwork.k8snetworkplumbingwg.io"].resourceName == "intel_nic" |
There was a problem hiding this comment.
The CEL expression for the DeviceClass selector uses device.driver == "sriov.k8snetworkplumbingwg.io". This is inconsistent with the driver name sriovnetwork.k8snetworkplumbingwg.io used in other parts of the document (e.g., lines 89 and 566). Please ensure the driver name is consistent throughout the design to avoid implementation errors.
| expression: | | |
| device.driver == "sriov.k8snetworkplumbingwg.io" && | |
| device.attributes["sriovnetwork.k8snetworkplumbingwg.io"].resourceName == "intel_nic" | |
| expression: | | |
| device.driver == "sriovnetwork.k8snetworkplumbingwg.io" && | |
| device.attributes["sriovnetwork.k8snetworkplumbingwg.io"].resourceName == "intel_nic" |
| `device.driver == "sriov.k8snetworkplumbingwg.io" && ` + | ||
| `device.attributes["sriovnetwork.k8snetworkplumbingwg.io"].resourceName == "%s"`, |
There was a problem hiding this comment.
The CEL expression in generateDeviceClassSelector uses device.driver == "sriov.k8snetworkplumbingwg.io". This is inconsistent with the driver name sriovnetwork.k8snetworkplumbingwg.io defined for the basic DeviceClass on lines 89 and 566. Please ensure the driver name is used consistently throughout the design to avoid implementation errors.
| `device.driver == "sriov.k8snetworkplumbingwg.io" && ` + | |
| `device.attributes["sriovnetwork.k8snetworkplumbingwg.io"].resourceName == "%s"`, | |
| `device.driver == "sriovnetwork.k8snetworkplumbingwg.io" && ` + | |
| `device.attributes["sriovnetwork.k8snetworkplumbingwg.io"].resourceName == "%s"`, |
| creation-date: 11-02-2026 | ||
| last-updated: 11-02-2026 |
| 11. Create ResourceClaim referencing the basic DeviceClass | ||
| 12. Create pod with ResourceClaim and verify VF allocation | ||
| 11. Test CNI integration with DRA-allocated VFs | ||
| 12. Test both kernel driver and VFIO modes |
There was a problem hiding this comment.
The list of integration tests has duplicate numbering. Items 11. and 12. appear twice. Please renumber the list correctly.
| 11. Create ResourceClaim referencing the basic DeviceClass | |
| 12. Create pod with ResourceClaim and verify VF allocation | |
| 11. Test CNI integration with DRA-allocated VFs | |
| 12. Test both kernel driver and VFIO modes | |
| 11. Create ResourceClaim referencing the basic DeviceClass | |
| 12. Create pod with ResourceClaim and verify VF allocation | |
| 13. Test CNI integration with DRA-allocated VFs | |
| 14. Test both kernel driver and VFIO modes |
| 3. **Advanced Filtering** | ||
| - Create user-managed `SriovResourceFilter` CRs with advanced criteria | ||
| - Verify they coexist with auto-generated filters | ||
| - Deploy pods requesting specific resource types | ||
| - Verify correct VF allocation based on filters | ||
|
|
||
| 3. **Migration Scenario** |
There was a problem hiding this comment.
The E2E Tests section has two items numbered as 3.. The "Migration Scenario" should likely be numbered 4., and subsequent items renumbered accordingly.
| 3. **Advanced Filtering** | |
| - Create user-managed `SriovResourceFilter` CRs with advanced criteria | |
| - Verify they coexist with auto-generated filters | |
| - Deploy pods requesting specific resource types | |
| - Verify correct VF allocation based on filters | |
| 3. **Migration Scenario** | |
| 3. **Advanced Filtering** | |
| - Create user-managed `SriovResourceFilter` CRs with advanced criteria | |
| - Verify they coexist with auto-generated filters | |
| - Deploy pods requesting specific resource types | |
| - Verify correct VF allocation based on filters | |
| 4. **Migration Scenario** |
Pull Request Test Coverage Report for Build 22132182544Details
💛 - Coveralls |
Signed-off-by: Fred Rolland <frolland@nvidia.com>
No description provided.