Add resource requirements for ingress router pods#2803
Add resource requirements for ingress router pods#2803joseorpa wants to merge 1 commit intoopenshift:masterfrom
Conversation
This commit introduces the `Resources` field in the `IngressControllerSpec` to allow configuration of resource limits for router pods. It also adds the `RouterResourceRequirements` struct to define resource requirements for the router, metrics, and logs containers. Additionally, a new feature gate, `FeatureGateIngressRouterResourceLimits`, is registered to enable this functionality. Enhancement: openshift/enhancements#1877 Signed-off-by: Jose Ortiz jose.orpa@gmail.com
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
size/XXL
|
Hello @joseorpa! Some important instructions when contributing to openshift/api: |
|
Hi @joseorpa. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
bot
added
the
needs-ok-to-test
|
[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 |
📝 WalkthroughWalkthroughThis change introduces a new feature gate 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/v1/types_ingress.go`:
- Around line 194-195: The comment that says "When this field is set, it takes
precedence over spec.nodePlacement.resources for configuring router pod
resources." references a stale path; update the docstring above the ingress
router resource field in types_ingress.go to point to the correct current field
path (or remove the nonexistent path) instead of spec.nodePlacement.resources —
locate the comment that begins "When this field is set…" which documents the
router resource configuration field and replace the stale reference with the
correct API field path used by the current spec (or reword to a generic
statement like "takes precedence over node placement resources in the spec") so
docs no longer reference a non‑existent field.
🪄 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: Pro Plus
Run ID: a4ab075b-60b0-4bee-8c7e-0239e1d9f031
⛔ Files ignored due to path filters (1)
openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (3)
features.mdfeatures/features.gooperator/v1/types_ingress.go
| // When this field is set, it takes precedence over spec.nodePlacement.resources | ||
| // for configuring router pod resources. |
There was a problem hiding this comment.
Fix stale API docs reference to spec.nodePlacement.resources.
Line 194 references a field path that does not exist in this API type, which can mislead users reading generated docs.
✏️ Proposed doc fix
- // When this field is set, it takes precedence over spec.nodePlacement.resources
- // for configuring router pod resources.
+ // When this field is set, it overrides the operator's default router pod
+ // resource configuration for router pods.📝 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.
| // When this field is set, it takes precedence over spec.nodePlacement.resources | |
| // for configuring router pod resources. | |
| // When this field is set, it overrides the operator's default router pod | |
| // resource configuration for router pods. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/v1/types_ingress.go` around lines 194 - 195, The comment that says
"When this field is set, it takes precedence over spec.nodePlacement.resources
for configuring router pod resources." references a stale path; update the
docstring above the ingress router resource field in types_ingress.go to point
to the correct current field path (or remove the nonexistent path) instead of
spec.nodePlacement.resources — locate the comment that begins "When this field
is set…" which documents the router resource configuration field and replace the
stale reference with the correct API field path used by the current spec (or
reword to a generic statement like "takes precedence over node placement
resources in the spec") so docs no longer reference a non‑existent field.
There was a problem hiding this comment.
This seems reasonable to address.
There was a problem hiding this comment.
@everettraven, glad you agree! Here's the proposed fix to remove the stale spec.nodePlacement.resources reference (which doesn't exist in the NodePlacement struct):
- Update the doc comment on the
Resourcesfield inIngressControllerSpecto remove the nonexistentspec.nodePlacement.resourcesreference
Failed to handle agent chat message. Please try again.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Thanks for the PR @joseorpa . It looks like you've already been working with @Miciah and @alebedev87 on the enhancement here.
@Miciah @alebedev87 is this something that is on your radar to continue moving forward with?
Additionally, it looks like you may need to re-generate the generated files related to the CRD. Running PROTO_OPTIONAL=true make update should update everything correctly.
| reportProblemsToJiraComponent("Networking/router"). | ||
| contactPerson("miciah"). | ||
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/1877"). |
There was a problem hiding this comment.
Note: This enhancement should be merged prior to this feature gate being enabled in TechPreviewNoUpgrade.
| // When this field is set, it takes precedence over spec.nodePlacement.resources | ||
| // for configuring router pod resources. |
There was a problem hiding this comment.
This seems reasonable to address.
| // | ||
| // +openshift:enable:FeatureGate=IngressRouterResourceLimits | ||
| // +optional | ||
| Resources *RouterResourceRequirements `json:"resources,omitempty"` |
There was a problem hiding this comment.
Is there a semantic difference between omitting the field and setting resources: {}?
| // router container. | ||
| // | ||
| // +optional | ||
| RouterContainer *corev1.ResourceRequirements `json:"routerContainer,omitempty"` |
There was a problem hiding this comment.
In general, we prefer to not re-use the corev1 types here because re-use of them doesn't result in validation constraints being applied here.
Instead, we recommend creating a local type that replicates the corev1 type but implements the necessary validation constraints that are evaluated at admission time, preventing creates/updates with invalid inputs.
2 participants
This commit introduces the
Resourcesfield in theIngressControllerSpecto allow configuration of resource limits for router pods. It also adds theRouterResourceRequirementsstruct to define resource requirements for the router, metrics, and logs containers. Additionally, a new feature gate,FeatureGateIngressRouterResourceLimits, is registered to enable this functionality.Enhancement: openshift/enhancements#1877
Signed-off-by: Jose Ortiz jose.orpa@gmail.com