Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

[elasticsearch] set cpu request = cpu limit #458

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

jmlrt
Copy link
Member

@jmlrt jmlrt commented Jan 23, 2020

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${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?

@jmlrt jmlrt added the enhancement New feature or request label Jan 23, 2020
@jmlrt jmlrt requested review from Crazybus and a team January 23, 2020 10:05
@jmlrt jmlrt self-assigned this Jan 23, 2020
Crazybus
Crazybus previously approved these changes Jan 24, 2020
Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jmlrt
Copy link
Member Author

jmlrt commented Jan 31, 2020

jenkins test this please

Crazybus
Crazybus previously approved these changes Jan 31, 2020
Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jmlrt
Copy link
Member Author

jmlrt commented Jan 31, 2020

@jmlrt
Copy link
Member Author

jmlrt commented Feb 3, 2020

jenkins test this please

1 similar comment
@jmlrt
Copy link
Member Author

jmlrt commented Feb 3, 2020

jenkins test this please

By increasing elasticsearch cpu request in c566822, n1-standard-8 don't provide enough cpu for the tests
Copy link
Contributor

@Crazybus Crazybus left a 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"
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Crazybus notice added in 293f0ec

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect thank you!

@jmlrt jmlrt merged commit ebdae67 into elastic:master Feb 5, 2020
@jmlrt jmlrt deleted the request-equals-limits branch February 5, 2020 08:29
@jmlrt jmlrt mentioned this pull request Mar 24, 2020
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants