-
Notifications
You must be signed in to change notification settings - Fork 61
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: Scaling issues with web due to low CPU limits #15955
Conversation
WalkthroughThe changes involve updates to resource allocation settings for a web service across multiple configuration files. Specifically, CPU limits and requests have been increased from '400m' to '1000m' for limits and from '200m' to '300m' for requests. Memory settings remain unchanged. These adjustments are reflected in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
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
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
charts/islandis/values.dev.yaml (1)
Line range hint
13-25
: Consider adjusting the service fee or discount structure to better align with customer incentives.The current implementation of a flat $20 service fee on discounted bills could negate the benefit of the discount, especially for smaller purchases or the lower loyalty tier (3-4 years at 10% discount).
For example, a $100 purchase with a 10% discount would result in a final price of $110 after adding the service fee, which is more than the undiscounted price. This might lead to customer dissatisfaction, as the intent to reward loyalty could paradoxically increase their bill.
Consider one of the following:
- Make the service fee a percentage of the discounted subtotal instead of a flat fee.
- Increase the discount percentages to account for the service fee.
- Only apply the service fee for discounts above a certain threshold.
- Waive the service fee for loyalty tiers below a certain threshold.
This will help ensure that the loyalty discount program aligns with customer expectations and incentivizes them appropriately.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- apps/web/infra/web.ts (1 hunks)
- charts/islandis/values.dev.yaml (1 hunks)
- charts/islandis/values.prod.yaml (1 hunks)
- charts/islandis/values.staging.yaml (1 hunks)
Additional context used
Path-based instructions (1)
apps/web/infra/web.ts (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
Additional comments not posted (5)
apps/web/infra/web.ts (1)
57-58
: Approve the CPU limit and request changes, but consider the impact and monitor usage.The increased CPU limits and requests align with the PR objective of addressing scaling issues. However, please consider the following:
- Monitor the web service's CPU usage to ensure the new limits are not consistently reached, which could indicate the need for further optimization or scaling.
- Assess the impact of these changes on the resources available for other services in the cluster.
- Ensure that the changes adhere to NextJS best practices for resource utilization and performance optimization.
charts/islandis/values.staging.yaml (2)
3073-3074
: Verify reasoning for CPU limit increase.The CPU limit for the
web
service has been significantly increased from400m
to1000m
. While this may help handle higher loads, it's important to:
- Verify the rationale behind this 150% increase. Was it based on observed CPU usage metrics or anticipated traffic growth?
- Ensure there is sufficient headroom in the cluster to accommodate this increase without adversely impacting other services.
- Monitor CPU utilization and latency after applying this change to validate if it's having the intended effect.
3076-3077
: LGTM!The increase in CPU request from
200m
to300m
aligns with the corresponding increase in CPU limits for theweb
service. This will allow the pods to start with a higher guaranteed CPU allocation.charts/islandis/values.prod.yaml (2)
3210-3210
: Verify if the increased CPU limit is necessary and optimize it based on actual utilization.The CPU limit for the
web
service has been increased significantly from400m
to1000m
. While the change is approved, it's recommended to:
- Verify if this much additional CPU capacity is truly needed to avoid overallocation of resources.
- Monitor the CPU utilization over time and optimize the limit if the additional capacity remains underutilized.
3213-3213
: LGTM!Increasing the CPU request from
200m
to300m
helps ensure theweb
service pods have a guaranteed minimum of 300 millicores of CPU. This reduces the risk of CPU starvation.The request is 30% of the 1000m limit, which enables good resource utilization.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15955 +/- ##
=======================================
Coverage 36.84% 36.84%
=======================================
Files 6709 6709
Lines 137553 137553
Branches 39102 39102
=======================================
Hits 50676 50676
Misses 86877 86877
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
* fix: Scaling issues with web due to low CPU limits * chore: charts update dirty files --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Change CPU limits for WEB
Why
On startup web uses a lot of CPU, causing HPA's to spawn more and more pods
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit