Rename SriovResourceFilter to SriovResourcePolicy#69
Rename SriovResourceFilter to SriovResourcePolicy#69rollandf wants to merge 1 commit intok8snetworkplumbingwg:mainfrom
Conversation
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 introduces a fundamental shift in how SR-IOV devices are exposed within Kubernetes by transitioning from an implicit advertisement model to an explicit opt-in policy-based approach. This change enhances security, improves resource management by preventing the advertisement of unintended devices, and aligns the driver's operational model with the SR-IOV Network Operator ecosystem. The core Highlights
Changelog
Activity
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
|
Pull Request Test Coverage Report for Build 22070645004Details
💛 - Coveralls |
There was a problem hiding this comment.
Code Review
This pull request primarily renames SriovResourceFilter to SriovResourcePolicy throughout the codebase to better align with the new opt-in device advertisement model. However, a security audit identified several critical issues that need immediate attention: a data race on the shared device state map that can lead to driver crashes (DoS), a logic error in the reconciler that leaves stale attributes on devices when multiple policies conflict, and an incomplete implementation of the opt-in model where the driver still advertises all devices regardless of policy matching. Additionally, some old naming conventions persist in directory and file names, there's a potential tooling version downgrade in the CRD definition, and a copyright header was removed in a generated file. Addressing these issues is crucial for the driver's stability and to meet security objectives.
...m/dra-driver-sriov/templates/sriovnetwork.k8snetworkplumbingwg.io_sriovresourcepolicies.yaml
Outdated
Show resolved
Hide resolved
55d7a5d to
4c8edca
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable renaming of SriovResourceFilter to SriovResourcePolicy, which more accurately reflects the opt-in nature of the device advertisement policy. The changes are comprehensive, touching API types, the controller, RBAC, documentation, and tests. The renaming has been executed consistently throughout the codebase. I've made a couple of suggestions to rename a demo directory and a file to align with these changes, which will improve overall consistency and user experience. Great work on improving the clarity of the API.
Changes API naming to better reflect the opt-in policy-based approach where devices are only advertised when explicitly defined in a policy. - Rename API types and CRD - Update controller: resourcefiltercontroller.go → resourcepolicycontroller.go - Update RBAC, demo examples, and tests - Update documentation with opt-in model terminology Part of k8snetworkplumbingwg#23 Signed-off-by: Fred Rolland <frolland@nvidia.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on renaming SriovResourceFilter to SriovResourcePolicy across the codebase to better align with the opt-in policy model. The changes are extensive, touching documentation, API definitions, controller logic, CRDs, Helm charts, and example manifests. The renaming has been applied consistently and correctly. Additionally, the demo example for resource policies has been significantly expanded to provide a more comprehensive usage scenario.
Changes API naming to better reflect the opt-in policy-based approach
where devices are only advertised when explicitly defined in a policy.
Part of #23