USHIFT-6912: Add DNS deployment resource configuration support#6621
USHIFT-6912: Add DNS deployment resource configuration support#6621Neilhamza wants to merge 5 commits intoopenshift:mainfrom
Conversation
Allow users to configure CPU and memory resources for the dns container in the dns-default DaemonSet via dns.resources in config.yaml. Defaults to the current hardcoded values (cpu=50m, memory=70Mi requests, no limits) when not configured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Neilhamza: This pull request references USHIFT-6912 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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-robot
added
the
jira/valid-reference
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdd DNS container resource configuration: new config types and defaults; merge and validate user requests/limits; controller exposes template params; DaemonSet templates request values and optional limits; update schema, packaging, docs, and tests. ChangesDNS Resource Configuration
Sequence DiagramsequenceDiagram
participant User
participant ConfigService
participant Controller
participant Kubernetes
participant Pod
User->>ConfigService: write/edit `dns.resources` drop-in
ConfigService->>Controller: incorporateUserSettings (merge requests/limits)
Controller->>Kubernetes: render DaemonSet template with DNS requests/limits params
Kubernetes->>Pod: create/update CoreDNS Pod with resources set
Pod->>Kubernetes: report Pod status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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.12.1)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." ... [truncated 29740 characters] ... elet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/howto_config.md`:
- Around line 207-209: The "Default Settings" example shows an empty requests
object ("requests: {}") which is misleading because the system applies defaults;
update the example in the Default Settings section so the effective DNS request
defaults are explicit (e.g., set requests to include cpu: 50m and memory: 70Mi)
and add a short note next to the example clarifying that these values reflect
the system's applied defaults; locate the block containing "resources:" /
"limits:" / "requests:" in the docs and replace the empty requests with the
explicit default key/value pairs and the clarifying note.
In `@pkg/config/dns.go`:
- Around line 131-153: The validateResources method on DNS should reject any
resource keys other than the allowed set ("cpu" and "memory"); update
DNS.validateResources to iterate Requests and Limits and return an error if a
key is not "cpu" or "memory" (before parsing), ensuring both
t.Resources.Requests and t.Resources.Limits are checked, and keep the existing
quantity parsing and comparison logic (use the same error formatting style) so
downstream templating in components/controllers.go and
assets/components/openshift-dns/dns/daemonset.yaml only sees cpu/memory keys.
🪄 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: 67ea07fa-e704-476d-83a9-5d9cae789b63
📒 Files selected for processing (8)
assets/components/openshift-dns/dns/daemonset.yamlcmd/generate-config/config/config-openapi-spec.jsondocs/user/howto_config.mdpackaging/microshift/config.yamlpkg/components/controllers.gopkg/config/config.gopkg/config/config_test.gopkg/config/dns.go
Reject unsupported resource keys during config validation to prevent typos and ensure only cpu/memory are accepted, matching what the DaemonSet template renders. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
|
hi @Neilhamza thank you very much for creating the PR early in the dev cycle. |
|
Thanks for the feedback @agullon !! i was thinking about a seperate PR so i wont bloat this one, but if adding the tests here is the accepted approach i can do it, what do you think? :) |
|
/test e2e-aws-tests |
I don't have a strong opinion about when/where add the RF, it's up to you. As I said I think it's better to add them along with product change. This PR is not that big, no problem. |
RobotFramework tests covering: default resources, custom requests and limits, requests only, partial requests, invalid quantity rejection, and DNS resolution after resource change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai perform full review |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
|
@agullon added RF testscoverage - : default resources, custom requests+limits, requests only, partial requests, invalid quantity rejection, and DNS resolution after change |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/suites/configuration/dns-resource-configuration.robot (2)
141-141: ⚡ Quick winMake restart-failure assertion less brittle.
Line [141] hardcodes
0 != 1, which tightly couples the test to exact assertion text/order. A wildcard pattern is more resilient to harmless keyword-message changes.Proposed change
- Run Keyword And Expect Error 0 != 1 Restart MicroShift + Run Keyword And Expect Error *!=* Restart MicroShift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/configuration/dns-resource-configuration.robot` at line 141, Replace the brittle exact-error match in the test's Run Keyword And Expect Error call: instead of hardcoding the literal '0 != 1' use a wildcard pattern (e.g. '*') so the assertion matches any error message from the Restart MicroShift keyword; update the Run Keyword And Expect Error invocation that currently contains '0 != 1' and 'Restart MicroShift' to use a wildcard error pattern.
42-115: ⚡ Quick winAdd a negative case for
limit < request.The suite validates bad quantity parsing, but it does not yet exercise the
limits >= requestsvalidation path inpkg/config/dns.go. Adding this guards an important contract from regressions.Suggested test addition
+${DNS_INVALID_LIMIT_LT_REQUEST} SEPARATOR=\n +... --- +... dns: +... \ \ resources: +... \ \ \ requests: +... \ \ \ \ cpu: "200m" +... \ \ \ limits: +... \ \ \ \ cpu: "100m" + +Limit Lower Than Request Prevents Start + [Documentation] Verify MicroShift fails when dns limit is lower than request + [Setup] Apply Invalid DNS Resource Config ${DNS_INVALID_LIMIT_LT_REQUEST} + Pattern Should Appear In Log Output ${CURSOR} must be greater than or equal to request + [Teardown] Restore Valid DNS Config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/configuration/dns-resource-configuration.robot` around lines 42 - 115, Add a new negative Robot test case that verifies the config validation rejects entries where a resource limit is less than the corresponding request (exercising the limits >= requests path in pkg/config/dns.go); create a test named like "Invalid Limits Less Than Requests" that uses the existing helper keywords (Apply DNS Resource Config or a new Apply Invalid DNS Resource Config with a YAML payload where cpu limit < cpu request and/or memory limit < memory request), assert the controller/log contains the appropriate error (e.g., "invalid dns resource request" or similar via Pattern Should Appear In Log Output ${CURSOR}) and clean up with the existing teardown keyword (Restore Valid DNS Config or Remove DNS Resource Config) so the suite remains isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/configuration/dns-resource-configuration.robot`:
- Line 141: Replace the brittle exact-error match in the test's Run Keyword And
Expect Error call: instead of hardcoding the literal '0 != 1' use a wildcard
pattern (e.g. '*') so the assertion matches any error message from the Restart
MicroShift keyword; update the Run Keyword And Expect Error invocation that
currently contains '0 != 1' and 'Restart MicroShift' to use a wildcard error
pattern.
- Around line 42-115: Add a new negative Robot test case that verifies the
config validation rejects entries where a resource limit is less than the
corresponding request (exercising the limits >= requests path in
pkg/config/dns.go); create a test named like "Invalid Limits Less Than Requests"
that uses the existing helper keywords (Apply DNS Resource Config or a new Apply
Invalid DNS Resource Config with a YAML payload where cpu limit < cpu request
and/or memory limit < memory request), assert the controller/log contains the
appropriate error (e.g., "invalid dns resource request" or similar via Pattern
Should Appear In Log Output ${CURSOR}) and clean up with the existing teardown
keyword (Restore Valid DNS Config or Remove DNS Resource Config) so the suite
remains isolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e441d468-d579-4a05-8012-d779622e04f5
📒 Files selected for processing (1)
test/suites/configuration/dns-resource-configuration.robot
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| {{- if .DNSHasLimits }} | ||
| limits: | ||
| {{- if .DNSCPULimit }} | ||
| cpu: {{ .DNSCPULimit }} | ||
| {{- end }} | ||
| {{- if .DNSMemoryLimit }} | ||
| memory: {{ .DNSMemoryLimit }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Why not just give customers whole resources: {}? If they define anything, we take it whatever they provide. Would simplify templating greatly
There was a problem hiding this comment.
I agree . since resource is something generic we might be able to reuse it for different components besides dns. (ie ingress)
There was a problem hiding this comment.
Nice! simplified the template to pass resource map directly with range loops of 2 params instead of 7 and is reusable for other components
|
@Neilhamza: 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. |
|
/assign |
|
/assign @eslutsky |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Pass resource maps directly instead of individual values, use range loops in the template. Reduces params from 7 to 2 and makes the pattern reusable for other components. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Neilhamza 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 |
6 participants
Allow users to configure CPU and memory resources for the dns container in the dns-default DaemonSet via dns.resources in config.yaml. Defaults to the current hardcoded values (cpu=50m, memory=70Mi requests, no limits) when not configured.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests