-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix node count reconcile #4473
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
fix node count reconcile #4473
Conversation
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 @Monokaix, 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 addresses a bug in the HyperNode controller's status reconciliation logic. Previously, the NodeCount in a HyperNode's status only reflected the number of members defined in its specification, not the actual number of Kubernetes nodes matching those member selectors. The changes introduce a robust mechanism to accurately determine and update the NodeCount by listing and filtering Kubernetes nodes based on exact, label, and regex matches defined in the HyperNode's members, ensuring the status is always consistent with the cluster's actual state.
Highlights
- RBAC Permissions: Added necessary
listandwatchpermissions for thenodesresource to the controller's ClusterRole in both the Helm chart and development manifest, enabling the controller to retrieve Kubernetes node information. - HyperNode Controller Enhancement: Integrated a
nodeListerinto thehyperNodeControllerto allow it to access and list Kubernetes nodes, which is crucial for accurate node counting. - Accurate Node Count Reconciliation: Implemented a new
actualNodeCntfunction within thehyperNodeControllerto calculate the true number of nodes associated with aHyperNode. This function now correctly processesHyperNodemember selectors (exact, label, and regex matches) to identify unique nodes, replacing the previous simple count of members. - Refactored Member Selection Logic: The
getMembersutility function in thepkg/scheduler/apipackage has been made public and renamed toGetMembers. It was also refined to correctly aggregate nodes identified by different selector types (exact, regex, label) into a unique set, ensuring no duplicates are counted. - Comprehensive Unit Testing: Updated and expanded unit tests for
HyperNodestatus reconciliation to include scenarios involving combined exact, label, and regex matching, ensuring the new node counting logic functions as expected under various conditions.
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 is currently in preview and 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 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.
Pull Request Overview
This PR fixes the hyperNode node count reconciliation logic by changing from a static member count to calculating actual node counts based on selectors (label, regex, and exact match). The main fix involves making the GetMembers function public and using it to determine the real count of nodes that match the hyperNode's member selectors.
- Modified node count calculation to use actual matching nodes instead of static member spec count
- Made
GetMembersfunction public and refactored exact match logic for consistency - Added RBAC permissions for the controller to access node resources
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/scheduler/api/hyper_node_info.go | Made GetMembers function public and refactored exact match logic |
| pkg/scheduler/api/hyper_node_info_test.go | Updated test to use public GetMembers function |
| pkg/controllers/hypernode/hypernode_handler.go | Added actualNodeCnt method that uses GetMembers to count matching nodes |
| pkg/controllers/hypernode/hypernode_controller.go | Added nodeLister field to controller struct |
| pkg/controllers/hypernode/hypernode_handler_test.go | Enhanced tests with new test cases for different selector types |
| installer/volcano-development.yaml | Added RBAC permissions for nodes access |
| installer/helm/chart/volcano/templates/controllers.yaml | Added RBAC permissions for nodes access |
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 necessary changes to reconcile the NodeCount in HyperNodeStatus by evaluating member selectors. However, the actualNodeCnt function has a logical flaw that could lead to incorrect status reporting when hypernodes are nested. Please address the comments to ensure correct behavior.
c2c8c72 to
5f3e248
Compare
Signed-off-by: Monokaix <[email protected]>
JesseStutler
left a comment
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.
/lgtm
lowang-bh
left a comment
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.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lowang-bh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…-upstream-release-1.12 Automated cherry pick of #4473: fix node count reconcile
What type of PR is this?
/kind bug
What this PR does / why we need it:
Reconcile hyperNode status when use label®ex&exact match.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?