Scheduler should detect change in replicas for multiple component workloads #7066
Conversation
Summary of ChangesHello @mszacillo, 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 critical bug that prevented the scheduler from accurately recognizing changes in replica counts for workloads that consist of multiple components. By introducing refined replica calculation methods and integrating them into the existing change detection logic, the system can now correctly identify when replica adjustments are needed, thereby ensuring robust cluster failover and consistent resource management for complex applications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
karmada-bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where the scheduler failed to detect replica changes for multi-component workloads. The fix introduces GetTotalBindingReplicas to correctly calculate total replicas by summing component replicas when available, and updates IsBindingReplicasChanged to use this new logic. The changes are logical and well-tested with new test cases for multi-component scenarios. My feedback is minor and focuses on improving the documentation of the new exported functions to better align with the project's style guide.
| return false | ||
| } | ||
|
|
||
| // GetTotalBindingReplicas will get the total replicas for a given resourcebinding |
There was a problem hiding this comment.
The comment for this exported function could be more descriptive to better align with the repository style guide (lines 7 and 21), which encourages clear descriptions of purpose and behavior over simple restatements of the function name.
| // GetTotalBindingReplicas will get the total replicas for a given resourcebinding | |
| // GetTotalBindingReplicas returns the total replicas for a resource binding, which is the sum of component replicas or the value of spec.Replicas. |
References
- All exported functions, methods, structs, and interfaces must be documented with clear and concise comments describing their purpose and behavior. The example provided shows that a good comment explains what the function does, including its behavior in different cases (e.g., what it returns if a key doesn't exist). (link)
pkg/util/binding.go
Outdated
| return bindingSpec.Replicas | ||
| } | ||
|
|
||
| // GetSumofReplicasForComponents will get the sum of replicas for multi-component resources |
There was a problem hiding this comment.
The comment for this exported function could be more descriptive as per the repository style guide (lines 7 and 21). Additionally, there is a typo in the function name within the comment (GetSumof... should be GetSumOf...).
| // GetSumofReplicasForComponents will get the sum of replicas for multi-component resources | |
| // GetSumOfReplicasForComponents calculates the sum of replicas for a slice of components. |
References
- All exported functions, methods, structs, and interfaces must be documented with clear and concise comments describing their purpose and behavior. The current comment is tautological and could be improved to be more descriptive. (link)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7066 +/- ##
==========================================
+ Coverage 46.55% 46.57% +0.02%
==========================================
Files 700 700
Lines 48084 48099 +15
==========================================
+ Hits 22384 22401 +17
Misses 24016 24016
+ Partials 1684 1682 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RainbowMango
left a comment
RainbowMango
left a comment
There was a problem hiding this comment.
/assign
Will take a look at it next week once I get back from New Year's Day.
|
/assign I recall encountering a similar issue in scaling scenarios before. I’ll look into it further to see if it’s a common pattern. |
RainbowMango
left a comment
RainbowMango
left a comment
There was a problem hiding this comment.
I thought this was a simple issue, but it might involve how to record the scheduling results for multi-template workloads and how to detect changes and trigger re-scheduling.
I need a bit more time to look into it.
Thanks @mszacillo for bringing this up, this is indeed a bug.
| func GetSumOfReplicasForComponents(components []workv1alpha2.Component) int32 { | ||
| replicasSum := int32(0) | ||
| for _, component := range components { | ||
| replicasSum += component.Replicas | ||
| } | ||
| return replicasSum | ||
| } |
There was a problem hiding this comment.
A corner case would break down this:
Let's say a FlinkDeployment with two components:
- jobManager, replicas 1
- taskManager, replicas 3
If we swap the replicas between the two components, the total replicas doesn't change.
There was a problem hiding this comment.
Ah, good catch. Perhaps we can consider extending the ClusterInfo that is currently in the ResourceBinding?
// TargetCluster represents the identifier of a member cluster.
type TargetCluster struct {
// Name of target cluster.
Name string `json:"name"`
// Replicas in target cluster
// +optional
Replicas int32 `json:"replicas,omitempty"`
}This could be extended to include components.
There was a problem hiding this comment.
Yeah, that's exactly what I'm thinking too. In addition, I'm also thinking of deprecating .spec.replicaRequirements and .spec.replicas and replacing them with the .spec.components, as part of the multi-components workload scheduling feature(#6998).
Given that they are widely used across the whole codebase, it's a little bit challenging to do that in a compatible and smooth way.
…kloads Signed-off-by: mszacillo <[email protected]>
mszacillo
force-pushed
the
component-scheduler-fix
branch
from
63d042c to
a40a850
Compare
|
[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 |
|
The previous behavior of multi-components scheduling was: among multiple candidate clusters, select the one with relatively sufficient resources for scheduling. Due to the absence of a rescheduling (re-entrant scheduling) path, the initial scheduling decision was never revised once made. This fix aims to add a re-entrant scheduling path for multi-components scheduling, but the following points need further confirmation:
WDYT? @mszacillo @RainbowMango |
At least in our use-case, if someone is scaling up their workload, then Karmada should keep the application in the cluster that it is currently scheduled to. This is the current behavior when you use an aggregated replicaDivisionPreference. If for instance I scale up the parallelism of a FlinkDeployment currently comprising of {1 JM, 5 TMs} -> {1 JM, 6 TMs}, only the delta of 1 replica will be taken into account as part of dynamicScaleUp. Ideally we should keep the behavior similar in the component case.
Yeah this is the more complicated issue. :/ Thinking out loud, I wonder if we can update what the component estimator computes depending on mode: If
For currently scheduled replicas, we'd need to reference the ClusterInfo as mentioned above. That said this would require a good amount of changes to how we call |
Yeah, I agree with this strategy, as it best ensures business continuity. However, if scaling operations (scale-up or scale-down) also retrigger the scheduling process, the current implementation cannot guarantee this behavior. I think this is also worth noting. |
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes bug which prevents cluster failover for multiple component workloads when MultiplePodTemplatesScheduling is enabled.
Which issue(s) this PR fixes:
Fixes #7065
Does this PR introduce a user-facing change?: