change he main functions name and add more detailed comments#894
change he main functions name and add more detailed comments#894LiZhenCheng9527 wants to merge 1 commit intovolcano-sh:mainfrom
Conversation
|
[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.
Code Review
This pull request refactors the ModelServingController by renaming several key synchronization methods—such as syncServingGroupReplicas, syncRoleWithinServingGroups, and manageRollingUpdate—to improve clarity and consistency. It also adds detailed documentation for these methods and introduces logic in DeleteRole to ensure roles are removed from the internal store only after their resources are fully deleted, triggering a re-reconciliation. Review feedback suggests updating an error message to align with the new method names and refining the wording of a comment regarding the role deletion process for better clarity.
| // Note: This measure is taken to prevent the Role’s resources from being deleted before the current function execution has completed, | ||
| // which would prevent them from being queued for re-coordination. |
There was a problem hiding this comment.
The note is confusingly worded. It seems to imply that it prevents the deletion of resources, but the logic is actually about when to remove the role from the internal store. It should clarify that the role is kept in the store with a Deleting status until its associated resources are fully removed from the cluster to prevent inconsistent state during reconciliation.
| // Note: This measure is taken to prevent the Role’s resources from being deleted before the current function execution has completed, | |
| // which would prevent them from being queued for re-coordination. | |
| // Note: This measure ensures the Role remains in the store with a 'Deleting' status until its resources are fully removed from the cluster, | |
| // preventing inconsistent state or premature re-creation during reconciliation. |
There was a problem hiding this comment.
Pull request overview
This PR updates the ModelServing controller’s internal function naming (to reflect “sync” semantics) and adds more detailed step-by-step comments to clarify the reconciliation flow.
Changes:
- Renamed key reconciliation helpers (e.g., ServingGroup/Role/HeadlessService syncing) and updated call sites.
- Added/expanded GoDoc comments describing the main reconciliation steps and rolling update behavior.
- Added a small fast-path cleanup in
DeleteRoleto remove a role from the store when deletion is already complete.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/model-serving-controller/controller/model_serving_controller.go | Renames internal reconciliation helpers and adds detailed GoDoc/comments; includes a DeleteRole cleanup fast-path and expanded rolling update documentation. |
| pkg/model-serving-controller/controller/model_serving_controller_test.go | Updates tests to call the renamed controller methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: LiZhenCheng9527 <lizhencheng6@huawei.com>
238bca6 to
bfb9c61
Compare
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Following several revisions, the names of the main functions in the previous
syncModelServinghave become obsolete. Consequently, these have been updated and more detailed comments have been added.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: