USHIFT-6913: Add DNS resource configuration enhancement for MicroShift#1989
USHIFT-6913: Add DNS resource configuration enhancement for MicroShift#1989Neilhamza wants to merge 4 commits intoopenshift:masterfrom
Conversation
|
@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. |
a296905 to
fda3b3f
Compare
|
@Neilhamza: This pull request references USHIFT-6913 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. |
|
/jira refresh |
|
@Neilhamza: This pull request references USHIFT-6913 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. |
fda3b3f to
89258a5
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
89258a5 to
23f187e
Compare
|
/assign |
| Requests map[string]string `json:"requests,omitempty"` | ||
| Limits map[string]string `json:"limits,omitempty"` |
There was a problem hiding this comment.
Should we use specific fields here instead of arbitrary keys? With current types a user could input gpus: 1 for example.
Wondering if corev1.ResourceRequirements type would bring any benefits too.
There was a problem hiding this comment.
restrict keys to only "cpu" and "memory" during validation, unsupported keys like "gpu" are rejected with a clear error. in the implementation we used map[string]string to stay consistent with the existing config patterns (simple Go types, not Kubernetes API types). Using corev1.ResourceRequirements would be the only Kubernetes type in the config package and would add coupling to the API types for serialization.
There was a problem hiding this comment.
Added to the Document that only cpu and memory keys are accepted, unsupported keys rejected during validation
| memory: <resource.Quantity> # optional, default: not set | ||
| ``` | ||
|
|
||
| By default, the `dns.resources` section is not set, and the current hardcoded defaults are used. When provided, individual fields within `requests` and `limits` are optional - users can set only the values they want to override. |
There was a problem hiding this comment.
If a user includes any of those values, does that invalidate the default setting for the others? For example, setting cpu to 500m still keeps the 70Mi for memory, or does it become unset?
There was a problem hiding this comment.
setting only cpu: 500m preserves the default memory: 70Mi. The implementation uses key-by-key merge, not full map replacement. and we have a test for it
There was a problem hiding this comment.
Added to the Document explicit explanation of key-by-key merge behavior and that defaults are preserved
| "DNSCPULimit": dnsResourceValue(cfg.DNS.Resources, "limits", "cpu", ""), | ||
| "DNSMemoryLimit": dnsResourceValue(cfg.DNS.Resources, "limits", "memory", ""), |
There was a problem hiding this comment.
Unset limits would render empty strings, which renders to false in the template. This is now a requirement for the function, right?
There was a problem hiding this comment.
Yes, unset limits return empty strings.
The template uses {{- if .DNSHasLimits }} (checks len(cfg.DNS.Resources.Limits) > 0) to conditionally render the entire limits block. Individual limit values are also guarded with {{- if .DNSCPULimit }}. So empty strings never appear in the rendered YAML.
There was a problem hiding this comment.
Updated template to use DNSHasLimits boolean (matching implementation), added explanation of nil map behavior
| ### Risks and Mitigations | ||
|
|
||
| **Risk:** Users configure resources too low, causing CoreDNS to be OOM-killed or throttled. | ||
| **Mitigation:** Document minimum recommended values. MicroShift does not enforce minimum thresholds to allow flexibility on constrained devices. |
There was a problem hiding this comment.
We might need to include a minimum setting or else the cluster is not even able to start properly (without dns there is no readiness from all pods).
There was a problem hiding this comment.
Good point. We could add minimum thresholds in validation (e.g., reject cpu < 10m or memory < 20Mi), but could this block legitimate use cases on extremely constrained edge devices?
There was a problem hiding this comment.
updated mitigation text, acknowledged the risk, noted minimum validation can be added later
There was a problem hiding this comment.
We have minimum requirements to run MicroShift (without including the app). Even if there is an extreme resource contraint the minimum requirements must hold, and having too low resources for dns might render unstable clusters (dns not responding, throttling in other pods, etc.).
What do you think about having the minimum the same as the default value we ship, as that has been the case so far since first version?
There was a problem hiding this comment.
sure! updated the minimum requirements same as the default we ship
| **Risk:** Users configure resources too low, causing CoreDNS to be OOM-killed or throttled. | ||
| **Mitigation:** Document minimum recommended values. MicroShift does not enforce minimum thresholds to allow flexibility on constrained devices. | ||
|
|
||
| **Risk:** Users configure resource limits without requests, leading to unexpected Kubernetes scheduling behavior. |
There was a problem hiding this comment.
I'm afraid that if u only set the limit value without setting the request value, the request value should be equal to the limit value by default
There was a problem hiding this comment.
In the implementation, default requests (cpu=50m, memory=70Mi) are always populated by dnsDefaults(). If a user sets only limits, the defaults remain as requests. The rendered DaemonSet YAML always contains explicit requests and limits values
adding this to the Document
There was a problem hiding this comment.
Added to the Document detailed explanation that default requests are always populated, and validation catches limit default request
There was a problem hiding this comment.
Should we add a test for this in the test plan? This is a corner case for the config.
There was a problem hiding this comment.
yes! added the test in the e2e tests and in the test plan
Update enhancement document to match implementation: - Value type DNSResources instead of pointer - Key-by-key merge preserving defaults for partial config - Key validation restricting to cpu and memory - DNSHasLimits boolean for template rendering - Clarify limit-without-request behavior - Expand test plan to cover all 7 e2e test cases 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: 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 |
Minimum requests (cpu: 50m, memory: 70Mi) match shipped defaults. Requests below these thresholds are rejected at startup to prevent unstable clusters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Neilhamza: all tests passed! 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. |
Summary
dns.resourcesconfig option to override CPU/memory requests and limits for thednscontainer in thedns-defaultDaemonSetdns.hostsfeature