OCPBUGS-55050: Remove exception for Gateway API Proxy resources limits#31145
OCPBUGS-55050: Remove exception for Gateway API Proxy resources limits#31145rikatz wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/severity-important
|
@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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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 (1)
WalkthroughThis PR updates a test file that validates operator resource handling. It removes unused import statements, simplifies ReplicaSet-to-Deployment owner reference resolution by removing error-path handling, and removes two Istio gateway resource-limit entries from the known-broken test data. ChangesTest Logic and Data Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 |
|
Skipping CI for Draft Pull Request. |
Previously, there was a need for exception on Gateway API Proxy pods to allow resources limits, as this wasn't properly configurable. The situation is now fixed on cluster ingress operator, and the exception can be removed.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rikatz 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 |
rikatz
force-pushed
the
remove-sail-exception
branch
from
d1b0eef to
275387f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/operators/resources.go (1)
123-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSilent swallowing of non-NotFound Deployment lookup errors may cause spurious test failures.
The previous code explicitly handled
apierrors.IsNotFound(orphaned RS → keep RS ref) versus other errors (some alternate path). The new code collapses all error cases into a no-op: any error—transient 5xx, RBAC denial, rate-limiting—leaves therefaskind=ReplicaSet, so the constructedcontrollerkey will beapps/v1/ReplicaSet/<ns>/<rs-name>instead ofapps/v1/Deployment/<ns>/<deploy-name>.Consequence: if such an error occurs at test runtime, any matching entry in
exceptionGrantedorknownBrokenPods(which are Deployment-keyed) will not match, and the violation is promoted fromwaitingForFix/excepted →notAllowed, causing a hard test failure unrelated to the actual resource-limit compliance.Consider at minimum logging the unexpected error so it's visible in test output:
🛡️ Proposed fix
if deploy, err := oc.KubeFramework().ClientSet.AppsV1().Deployments(pod.Namespace).Get(context.Background(), name, metav1.GetOptions{}); err == nil { ref.Name = deploy.Name ref.Kind = "Deployment" ref.APIVersion = "apps/v1" +} else if !apierrors.IsNotFound(err) { + e2e.Logf("WARNING: unexpected error resolving Deployment for ReplicaSet %s/%s: %v", pod.Namespace, ref.Name, err) }And restore the
apierrorsimport:+ "k8s.io/apimachinery/pkg/api/errors"🤖 Prompt for 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. In `@test/extended/operators/resources.go` around lines 123 - 131, When resolving ReplicaSet refs in the switch case (the block that calls oc.KubeFramework().ClientSet.AppsV1().Deployments(...).Get and then sets ref.Name/ref.Kind/ref.APIVersion), do not silently ignore all errors: import k8s.io/apimachinery/pkg/api/errors (apierrors), check apierrors.IsNotFound(err) and keep the ReplicaSet ref in that case, but for any other non-nil error log the unexpected error (with context: namespace, rs name and the error) and avoid treating it as a benign NotFound — this preserves the original behavior for orphaned RS while surfacing transient/permission errors so they don't produce spurious test failures.
🤖 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.
Outside diff comments:
In `@test/extended/operators/resources.go`:
- Around line 123-131: When resolving ReplicaSet refs in the switch case (the
block that calls oc.KubeFramework().ClientSet.AppsV1().Deployments(...).Get and
then sets ref.Name/ref.Kind/ref.APIVersion), do not silently ignore all errors:
import k8s.io/apimachinery/pkg/api/errors (apierrors), check
apierrors.IsNotFound(err) and keep the ReplicaSet ref in that case, but for any
other non-nil error log the unexpected error (with context: namespace, rs name
and the error) and avoid treating it as a benign NotFound — this preserves the
original behavior for orphaned RS while surfacing transient/permission errors so
they don't produce spurious test failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f7326b7f-5fca-45c9-8e2c-f06703cc9d14
📒 Files selected for processing (1)
test/extended/operators/resources.go
openshift-merge-bot
Bot
added
the
ready-for-human-review
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rikatz: This pull request references Jira Issue OCPBUGS-55050, which is valid. The bug has been moved to the POST state. 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. |
|
I am leaving this as a draft just while openshift/cluster-ingress-operator#1440 is not merged yet |
2 participants
Previously, there was a need for exception on Gateway API Proxy pods to allow resources limits, as this wasn't properly configurable.
The situation is now fixed on cluster ingress operator, and the exception can be removed. The fix is implemented on openshift/cluster-ingress-operator#1440
Summary by CodeRabbit