feat: add CPU limit multiplier to simple limit strategy #446
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SimpleLimitStrategy
participant MetricsPodData
participant K8sObjectData
User->>SimpleLimitStrategy: Configure with cpu_limit_multiplier
SimpleLimitStrategy->>SimpleLimitStrategy: description (shows multiplier in text)
User->>SimpleLimitStrategy: Request CPU proposal
SimpleLimitStrategy->>MetricsPodData: Fetch CPU history data
SimpleLimitStrategy->>K8sObjectData: Fetch object data
SimpleLimitStrategy->>SimpleLimitStrategy: Calculate base percentile value
SimpleLimitStrategy->>SimpleLimitStrategy: Multiply by cpu_limit_multiplier
SimpleLimitStrategy-->>User: Return CPU limit recommendation
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
robusta_krr/strategies/simple_limit.py (1)
30-32: Consider adding an upper bound constraint for the multiplier.While the
gt=0constraint prevents negative and zero values, there's no upper limit on the multiplier. Consider adding a reasonable upper bound to prevent unrealistic CPU limit recommendations.For example, you could add an upper bound:
cpu_limit_multiplier: float = pd.Field( - 1.0, gt=0, description="Multiplier for CPU limit based on percentile. For example, 2.0 means 200% of percentile, 1.5 means 150%." + 1.0, gt=0, le=10.0, description="Multiplier for CPU limit based on percentile. For example, 2.0 means 200% of percentile, 1.5 means 150%. Maximum value is 10.0." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
robusta_krr/strategies/simple_limit.py(3 hunks)
🔇 Additional comments (2)
robusta_krr/strategies/simple_limit.py (2)
30-32: LGTM! Well-designed field with proper validation.The new
cpu_limit_multiplierfield is well-implemented with appropriate type annotation, sensible default value (1.0 for backward compatibility), proper validation constraint (gt=0), and clear description with examples.
143-145: LGTM! Correct implementation of the multiplier logic.The CPU limit calculation correctly applies the multiplier to the base percentile value. The two-step approach (calculate base, then multiply) is clear and maintainable.
Comment on lines
+98
to
106
| cpu_limit_percentage = int(self.settings.cpu_limit_multiplier * 100) | ||
| s = textwrap.dedent(f"""\ | ||
| CPU request: {self.settings.cpu_request}% percentile, limit: {self.settings.cpu_limit}% percentile | ||
| CPU request: {self.settings.cpu_request}% percentile, limit: {cpu_limit_percentage}% of {self.settings.cpu_limit}% percentile | ||
| Memory request: max + {self.settings.memory_buffer_percentage}%, limit: max + {self.settings.memory_buffer_percentage}% | ||
| History: {self.settings.history_duration} hours | ||
| Step: {self.settings.timeframe_duration} minutes | ||
| All parameters can be customized. For example: `krr simple_limit --cpu_request=66 --cpu_limit=96 --memory_buffer_percentage=15 --history_duration=24 --timeframe_duration=0.5` | ||
| All parameters can be customized. For example: `krr simple_limit --cpu_request=66 --cpu_limit=96 --cpu_limit_multiplier=2.0 --memory_buffer_percentage=15 --history_duration=24 --timeframe_duration=0.5` | ||
| """) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve description clarity and handle decimal multipliers properly.
The description formatting has some clarity issues:
- Converting the multiplier to an integer percentage (
int(self.settings.cpu_limit_multiplier * 100)) truncates decimal values, which could be misleading for multipliers like 1.5. - The description format "200% of 96% percentile" might be confusing - consider a clearer format.
Apply this diff to improve clarity:
- cpu_limit_percentage = int(self.settings.cpu_limit_multiplier * 100)
+ cpu_limit_multiplier = self.settings.cpu_limit_multiplier
s = textwrap.dedent(f"""\
- CPU request: {self.settings.cpu_request}% percentile, limit: {cpu_limit_percentage}% of {self.settings.cpu_limit}% percentile
+ CPU request: {self.settings.cpu_request}% percentile, limit: {self.settings.cpu_limit}% percentile × {cpu_limit_multiplier}
Memory request: max + {self.settings.memory_buffer_percentage}%, limit: max + {self.settings.memory_buffer_percentage}%
History: {self.settings.history_duration} hours
Step: {self.settings.timeframe_duration} minutes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cpu_limit_percentage = int(self.settings.cpu_limit_multiplier * 100) | |
| s = textwrap.dedent(f"""\ | |
| CPU request: {self.settings.cpu_request}% percentile, limit: {self.settings.cpu_limit}% percentile | |
| CPU request: {self.settings.cpu_request}% percentile, limit: {cpu_limit_percentage}% of {self.settings.cpu_limit}% percentile | |
| Memory request: max + {self.settings.memory_buffer_percentage}%, limit: max + {self.settings.memory_buffer_percentage}% | |
| History: {self.settings.history_duration} hours | |
| Step: {self.settings.timeframe_duration} minutes | |
| All parameters can be customized. For example: `krr simple_limit --cpu_request=66 --cpu_limit=96 --memory_buffer_percentage=15 --history_duration=24 --timeframe_duration=0.5` | |
| All parameters can be customized. For example: `krr simple_limit --cpu_request=66 --cpu_limit=96 --cpu_limit_multiplier=2.0 --memory_buffer_percentage=15 --history_duration=24 --timeframe_duration=0.5` | |
| """) | |
| cpu_limit_multiplier = self.settings.cpu_limit_multiplier | |
| s = textwrap.dedent(f"""\ | |
| CPU request: {self.settings.cpu_request}% percentile, limit: {self.settings.cpu_limit}% percentile × {cpu_limit_multiplier} | |
| Memory request: max + {self.settings.memory_buffer_percentage}%, limit: max + {self.settings.memory_buffer_percentage}% | |
| History: {self.settings.history_duration} hours | |
| Step: {self.settings.timeframe_duration} minutes | |
| All parameters can be customized. For example: `krr simple_limit --cpu_request=66 --cpu_limit=96 --cpu_limit_multiplier=2.0 --memory_buffer_percentage=15 --history_duration=24 --timeframe_duration=0.5` | |
| """) |
🤖 Prompt for AI Agents
In robusta_krr/strategies/simple_limit.py around lines 98 to 106, the current
code converts cpu_limit_multiplier to an integer percentage, truncating decimals
and causing potential confusion in the description format. To fix this, keep the
multiplier as a float and format it with one or two decimal places to preserve
precision. Also, rephrase the description to clearly explain the relationship,
for example by stating "CPU limit: cpu_limit_multiplier times the cpu_limit
percentile" instead of using percentage multiplication wording.
|
@patsevanton could you achieve something similar by modifying the existing limit field so it can take values above 100%? (e.g. 120%) |
|
@aantn using percentages greater than 100 may not make practical sense in the context of statistics, since a percentage of 100% already represents the maximum value in the dataset. |
|
Isn't that what you're trying to achieve though?
…On Tue, Jul 22, 2025, 16:35 Anton Patsev ***@***.***> wrote:
*patsevanton* left a comment (robusta-dev/krr#446)
<#446 (comment)>
@aantn <https://github.com/aantn> using percentages greater than 100 may
not make practical sense in the context of statistics, since a percentage
of 100% already represents the maximum value in the dataset.
—
Reply to this email directly, view it on GitHub
<#446 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADYUB25BQLX3IH23L4JXQL3JY42PAVCNFSM6AAAAACBLHPEF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCMBSG42TANJXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I will try build and run. later |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a new configuration parameter
cpu_limit_multiplierto the SimpleLimitStrategy, providing more flexible control over CPU limit recommendations.Changes:
cpu_limit_multiplierfield toSimpleLimitStrategySettingswith default value of 1.0Benefits:
Example Usage: