-
Notifications
You must be signed in to change notification settings - Fork 101
implemented the Preemptable field for podgroup api #190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
implemented the Preemptable field for podgroup api #190
Conversation
Signed-off-by: SuyashParmar <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @SuyashParmar, 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 primarily focuses on enhancing the PodGroupSpec API by introducing a dedicated Preemptable field. This change aims to provide a more explicit and native mechanism for managing PodGroup preemption, improving clarity and compatibility over previous annotation-based methods. The core of the work involves schema modifications and the necessary regeneration of associated client and conversion code to support this new API field.
Highlights
- API Definition Enhancement: I've introduced a new
Preemptable *boolfield to thePodGroupSpecin both the core API (pkg/apis/scheduling/types.go) and itsv1beta1version (pkg/apis/scheduling/v1beta1/types.go). This field provides a native way to explicitly control whether a PodGroup can be preempted, moving away from annotation-based behavior. The field's logic is designed for compatibility:nilortruemeans it can be preempted, whilefalsemeans it cannot. - Client and Conversion Code Regeneration: I've updated the auto-generated deepcopy, conversion, and client apply configuration files (
zz_generated.deepcopy.go,zz_generated.conversion.go,podgroupspec.go) to correctly handle the newPreemptablefield. This ensures proper serialization, deserialization, deep copying, and API version conversions, maintaining compatibility and enabling client-side declarative configuration for the new field.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new Preemptable field to the PodGroupSpec API to provide a native way of controlling preemption, which is a good improvement over using annotations. The changes to the API definitions and the generated client code look mostly correct.
I've identified a significant issue with the protobuf serialization tag for the new field, which could cause interoperability problems. I've left a specific comment with a suggestion to fix it.
Additionally, the pull request description mentions that the scheduler logic was updated to use this new field, but the corresponding code changes are not included in the provided diffs. Please ensure those changes are part of this PR or a subsequent one to complete the feature.
| // When nil or true, the PodGroup can be preempted for compatibility. | ||
| // When false, the PodGroup cannot be preempted. | ||
| // +optional | ||
| Preemptable *bool `json:"preemptable,omitempty" protobuf:"bytes,6,opt,name=preemptable"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protobuf tag for Preemptable uses bytes, which is incorrect for a boolean type. For protobuf encoding, boolean values should be encoded as varint.
While this might appear to work in some contexts, it's semantically incorrect and can lead to compatibility and serialization issues with other standard Kubernetes tooling or future versions of protobuf.
For reference, standard Kubernetes API types use varint for *bool fields (e.g., core/v1.SecurityContext.AllowPrivilegeEscalation). Using the correct type ensures proper serialization and interoperability.
Please change bytes to varint.
| Preemptable *bool `json:"preemptable,omitempty" protobuf:"bytes,6,opt,name=preemptable"` | |
| Preemptable *bool `json:"preemptable,omitempty" protobuf:"varint,6,opt,name=preemptable"` |
|
@SuyashParmar: PR needs rebase. DetailsInstructions 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/test-infra repository. |
Problem
Current preemptable behavior uses annotations/labels with default
false, limiting compatibility.Solution
Add native
Preemptable *boolfield toPodGroupSpec:nilortrue→ can be preempted (for compatibility)false→ cannot be preemptedChanges
apis/,volcano-apis/)