-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MON-3304: Add option to specify resource limits for all components #2067
Conversation
rexagod
commented
Aug 7, 2023
- I added CHANGELOG entry for this change.
- No user facing changes, so no entry in CHANGELOG was needed.
@rexagod: This pull request references MON-3168 which is a valid jira issue. In 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 kubernetes/test-infra repository. |
Skipping CI for Draft Pull Request. |
b4bb772
to
3bcce4e
Compare
@rexagod: This pull request references MON-3304 which is a valid jira issue. In 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 kubernetes/test-infra repository. |
be6cdd8
to
187b71f
Compare
@rexagod: This pull request references MON-3304 which is a valid jira issue. In 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 kubernetes/test-infra repository. |
type spec struct { | ||
replicas int32 | ||
affinity *v1.Affinity | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused.
ddfcec4
to
2ef0c67
Compare
Documentation/openshiftdocs/modules/prometheusoperatoradmissionwebhookconfig.adoc
Outdated
Show resolved
Hide resolved
Documentation/openshiftdocs/modules/clustermonitoringconfiguration.adoc
Outdated
Show resolved
Hide resolved
Documentation/openshiftdocs/modules/prometheusoperatoradmissionwebhookconfig.adoc
Outdated
Show resolved
Hide resolved
Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
@@ -7,6 +7,7 @@ | |||
- [#1950](https://github.com/openshift/cluster-monitoring-operator/pull/1950) Disable CORS headers on Thanos querier by default and add a flag to enable them back. | |||
- [#1963](https://github.com/openshift/cluster-monitoring-operator/pull/1963) Add nodeExporter settings for network devices list. | |||
- [#2049](https://github.com/openshift/cluster-monitoring-operator/pull/2049) Remove Kube*QuotaOvercommit alerts. | |||
- [#2067](https://github.com/openshift/cluster-monitoring-operator/pull/2067) Add options to specify resource requests and limits for all components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, finally :)
but IIUC, these new fields would override the default requests we already set.
Is it really what we want to do? Force users to define both requests and limits even when they only want to set limits?
IIRC, when only limits are defined, Kubernetes will set requests to the same values than limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if we don't set requests for the components, it will use the default requests as defined in each deployment/daemonset
- https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
Note: If you specify a limit for a resource, but do not specify any request, and no admission-time mechanism has applied a default request for that resource, then Kubernetes copies the limit you specified and uses it as the requested value for the resource.
- specify request/limit, the component should use the specified settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @juzhao for confirming that "if only limits are set, requests are set equal to limits".
Here are my concerns in two scenarios:
- User only sets the limits via this feature field without knowing that the default requests are not taken into account anymore and that the requests will be equal to the limits they provided (they may have some capacity issues) (unexpected behaviour)
- User took the care to copy the default requests in this feature field, but I a change was done that makes a component need less or more requests, we take care of changing the default requests, but the requests in the feature field are not up to date anymore. (maintenance burden)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @juzhao for confirming that "if only limits are set, requests are set equal to limits".
Here are my concerns in two scenarios:
* User only sets the limits via this feature field without knowing that the default requests are not taken into account anymore and that the requests will be equal to the limits they provided (they may have some capacity issues) (unexpected behaviour)
I think this is expected behavior, by design of Kubernetes, user should know it
* User took the care to copy the default requests in this feature field, but I a change was done that makes a component need less or more requests, we take care of changing the default requests, but the requests in the feature field are not up to date anymore. (maintenance burden)
example: prometheus-adapter default resources.requests
https://github.com/openshift/cluster-monitoring-operator/blob/release-4.14/assets/prometheus-adapter/deployment.yaml#L73-L76
- if use does not set resources.requests in cluster-monitoring-config configmap, prometheus-adapter will sue the default resources.requests(if the default value can not let prometheus-adapter pod start up, then it's a bug, should be fixed in CMO)
- if user only set resources.limits, resources.requests use the same value as resources.limits,
apiVersion: v1
kind: ConfigMap
metadata:
name: cluster-monitoring-config
namespace: openshift-monitoring
data:
config.yaml: |
k8sPrometheusAdapter:
resources:
limits:
cpu: 200m
memory: 500Mi
- set resources.limits and resources.requests, pod should use the specified value
apiVersion: v1
kind: ConfigMap
metadata:
name: cluster-monitoring-config
namespace: openshift-monitoring
data:
config.yaml: |
k8sPrometheusAdapter:
resources:
requests:
cpu: 20m
memory: 200Mi
limits:
cpu: 200m
memory: 500Mi
when set resources.limits and resources.requests, user should know clearly of the prometheus-adapter cpu/memory usage, then set resources.limits and resources.requests to a proper value, otherwise, pod would not start up
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f, rexagod The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
@rexagod: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
ebaa7e3
into
openshift:master
/jira refresh |