OCPBUGS-55050: Remove resources limits from Gateway API proxy containers#1440
OCPBUGS-55050: Remove resources limits from Gateway API proxy containers#1440rikatz wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
@rikatz: This pull request references Jira Issue OCPBUGS-55050, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request replaces the horizontal pod autoscaler-focused gateway class configuration approach with a new gateway class-specific configuration builder. Changes include introducing a new constant for the Istio proxy container name, updating the Istio CR spec construction in both OLM and Sail installer controllers to use 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
[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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/util_gatewayapi_test.go`:
- Around line 835-850: Replace the loose length check with an explicit nil check
for container resources limits (use c.Resources.Limits != nil rather than
len(c.Resources.Limits) > 0) in the retry loop that inspects the 'istio-proxy'
container (the block referencing variable c and foundIstioProxy), and update the
final error message that references deploymentName to correctly state the
failing field (e.g., "Failed to verify resources.limits on deployment %s: %v"
instead of mentioning terminationMessagePolicy).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2d2d7df3-74bd-4dda-b240-cf79fe5c4920
📒 Files selected for processing (6)
pkg/operator/controller/gatewayclass/controller.gopkg/operator/controller/gatewayclass/controller_test.gopkg/operator/controller/gatewayclass/istio_olm.gopkg/operator/controller/gatewayclass/istio_sail_installer.gotest/e2e/gateway_api_test.gotest/e2e/util_gatewayapi_test.go
When a Gateway is deployed, Istio will create a deployment containing resources limits set, which is not expected on Openshift components. These resource limits are added as part of the default helm template used by Istio controller when creating the deployments and Pods. An attempt to make Istio/Sail Library to have a default configuration on resource limits is not possible, given the Kubernetes resources struct has omitempty, which means when something is null omit, and when it ommits it considers that a user passing null has 'no opinion' as opposed to 'I want to set it null'. This way, the only way of fixing it is by telling the Gateway customization, via the Gateway Class that we want to enforce the resource to be null, and then when Istio deploys this Gateway instance it will remove the resources.limits field from the proxy pods
3cf04dd to
0dafbd5
Compare
|
/jira refresh |
|
@rikatz: This pull request references Jira Issue OCPBUGS-55050, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-vsphere-static-metallb-operator-gwapi |
|
@rikatz: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
When a Gateway is deployed, Istio will create a deployment containing resources limits set, which is not expected on Openshift components. These resource limits are added as part of the default helm template used by Istio controller when creating the deployments and Pods.
An attempt to make Istio/Sail Library to have a default configuration on resource limits is not possible, given the Kubernetes resources struct has omitempty, which means when something is null omit, and when it ommits it considers that a user passing null has 'no opinion' as opposed to 'I want to set it null'.
This way, the only way of fixing it is by telling the Gateway customization, via the Gateway Class that we want to enforce the resource to be null, and then when Istio deploys this Gateway instance it will remove the resources.limits field from the proxy pods
The majority of this PR was originally copied from #1428