-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[elasticsearch] set cpu request = cpu limit #458
Conversation
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.
LGTM!
9d3a952
to
c566822
Compare
jenkins test this please |
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.
LGTM!
Upgrade test is failing when changing the cpu request: https://devops-ci.elastic.co/job/elastic+helm-charts+pull-request+integration-elasticsearch/254/ES_SUITE=upgrade,KUBERNETES_VERSION=1.13,label=docker&&virtual/ I'll investigate next week. |
jenkins test this please |
1 similar comment
jenkins test this please |
By increasing elasticsearch cpu request in c566822, n1-standard-8 don't provide enough cpu for the tests
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.
LGTM!
@@ -39,7 +39,7 @@ variable "additional_zones" { | |||
|
|||
variable "machine_type" { | |||
description = "Machine type for the kubernetes nodes" | |||
default = "n1-standard-8" | |||
default = "custom-10-30720" |
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.
The fact that this increase is causing issues for our testing environment might be a sign that this should be announced as a breaking chance in the release notes. At least breaking for anybody using the current default resource 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.
In our testing environment we are deploying 21 Elasticsearch pods (7 scenarios x 3 nodes) per clusters and we can assume that the pod are never requiring more resources than set in requests resources during the tests.
This change cause resource needed for Elasticsearch pods grow from 2.1 CPU to 21 CPU in our testing environment.
IMHO standard usage is closer to 3 long running Elasticsearch pods per cluster which are consuming CPU and memory limits, so I'm not sure that the impact should be considered as a breaking change. WDYT?
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.
The impact was big enough to cause us issues in our testing environment because of resource capacity. A 10x increase in the amount of CPUs requested by default could also cause issues for others too. For clusters with autoscaling enabled they might suddenly end up with 10 times the amount of Kubernetes node running after a version bump.
I don't think the change is big enough to avoid doing it, but I do think it would be nice to give all users a heads up including documentation for how to keep the existing configuration.
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.
That sounds good to me, I'll merge the PR and will add a note in next release's changelog about this change and how to keep existing config.
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.
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.
Perfect thank you!
${CHART}/tests/*.py
${CHART}/examples/*/test/goss.yaml
Related to #409 (comment)
I think we can let cpu request to 100m in "workstation" examples like docker-for-mac example.
What about default values for Filebeat, Metricbeat, Kibana and Logstash should we also stay with the request=limit rule?