Skip to content
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

Distributor autoscaling #3378

Merged
merged 11 commits into from Nov 4, 2022
Merged

Conversation

jhesketh
Copy link
Contributor

@jhesketh jhesketh commented Nov 3, 2022

What this PR does

Add support for autoscaling distributors. Lay some groundwork to help making other components autoscale.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@jhesketh
Copy link
Contributor Author

jhesketh commented Nov 3, 2022

This is still WIP, but looking for advise on how to implement more flexible alerts for more autoscaled components.

Specifically in operations/mimir-mixin/alerts/autoscaling.libsonnet I want to loop over $._config.autoscaling but jsonnet is expecting an array and $._config.autoscaling is apparently an object. Not sure why or what I could do?

@pracucci
Copy link
Collaborator

pracucci commented Nov 3, 2022

This is still WIP, but looking for advise on how to implement more flexible alerts for more autoscaled components.

Specifically in operations/mimir-mixin/alerts/autoscaling.libsonnet I want to loop over $._config.autoscaling but jsonnet is expecting an array and $._config.autoscaling is apparently an object. Not sure why or what I could do?

Not a direct answer, but I think alerts shouldn't be based on the configured $._config.autoscaling.*enabled if possible. The reason is that if you deploy multiple Mimir clusters, alerts are global but config is per cluster (at least that's how it works at Grafana Labs) so if you autoscaling enabled only in some clusters then the alert will not work as expected for all clusters.

false
)
),
groups+: if !anyEnabled($._config.autoscaling) then [] else [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep it simple and always configure the alert, regardless whether autoscaling is enabled or not. If autoscaling is not enabled, then the metric kube_horizontalpodautoscaler_status_condition won't exist and alerting rule will not evaluate. That's an approach we take for other alerts too, which are based on features you may or may not have enabled.

@pracucci
Copy link
Collaborator

pracucci commented Nov 3, 2022

Specifically in operations/mimir-mixin/alerts/autoscaling.libsonnet I want to loop over $._config.autoscaling but jsonnet is expecting an array and $._config.autoscaling is apparently an object.

To answer this specific question, you can use one of the functions from the stdlib to get an array from an object, for example std.objectFields(). See here the ones that better fit to your use case: https://jsonnet.org/ref/stdlib.html

},
autoscaling: [
{
name: 'querier',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two questions:

  1. Any specific reason why we're moving away from the previous config structure?
  2. Given you shouldn't configured it twice for the same component, have you considered having a config structure like this?
autoscaling: {
  querier: {
    enabled: true,
    hpa_name: 'keda-hpa-querier',
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Moving away so that I can loop over the components.
  2. Not sure what you mean by configuring it twice? But yes, your suggested structure would work well. It's really just moving the name field to a key, but it's much neater so will try to implement that.

@jhesketh
Copy link
Contributor Author

jhesketh commented Nov 3, 2022

Not a direct answer, but I think alerts shouldn't be based on the configured $._config.autoscaling.*enabled if possible. The reason is that if you deploy multiple Mimir clusters, alerts are global but config is per cluster (at least that's how it works at Grafana Labs) so if you autoscaling enabled only in some clusters then the alert will not work as expected for all clusters.

Okay. I was following the existing convention and expanding it for more components. In this case enabled means that the alert is enabled, not necessarily that autoscaling itself is actually configured.

However from your next comment..

I would keep it simple and always configure the alert, regardless whether autoscaling is enabled or not. If autoscaling is not enabled, then the metric kube_horizontalpodautoscaler_status_condition won't exist and alerting rule will not evaluate. That's an approach we take for other alerts too, which are based on features you may or may not have enabled.

In that case I should drop the .enabled field. I still need $._config.autoscaling though to describe the other components.

I'll give this a go tomorrow with std.objectFields(). I did play around with some of the stdlib stuff, but thought there might be a better way to do it.

@jhesketh
Copy link
Contributor Author

jhesketh commented Nov 4, 2022

FYI, the problem I was experiencing was that I missed that operations/mimir-mixin/mixin-compiled.libsonnet was overwriting the autoscaling value and thus modifying the object(s). Now that I see that path, I should be able to resolve this.

jhesketh and others added 3 commits November 4, 2022 18:01
This is WIP. Because the distributor scales off multiple metrics this
needs some adjusting.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! We pushed a couple of commits while doing pair code review, and left few comments. Apart from the minor comments, everything else LGTM 👏

@jhesketh jhesketh marked this pull request as ready for review November 4, 2022 10:44
@jhesketh jhesketh requested review from osg-grafana and a team as code owners November 4, 2022 10:44
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM!

@pracucci pracucci enabled auto-merge (squash) November 4, 2022 10:51
@osg-grafana osg-grafana added the type/docs Improvements or additions to documentation label Nov 4, 2022
@@ -28,12 +28,15 @@

* [CHANGE] Alerts: Change `MimirSchedulerQueriesStuck` `for` time to 7 minutes to account for the time it takes for HPA to scale up. #3223
* [CHANGE] Dashboards: Removed the `Querier > Stages` panel from the `Mimir / Queries` dashboard. #3311
* [CHANGE] Configuration: The format of the `autoscaling` section of the configuration has changed to support more components. #3378
* Instead of specific config variables for each component, they are listed in a dictionary. For example, `autoscaling.querier_enabled` becomes `autoscaling.querier.enabled`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Instead of specific config variables for each component, they are listed in a dictionary. For example, `autoscaling.querier_enabled` becomes `autoscaling.querier.enabled`.
* Instead of specific configuration variables for each component, they are listed in a dictionary. For example, `autoscaling.querier_enabled` becomes `autoscaling.querier.enabled`.

@@ -58,6 +61,7 @@
* Renaming the alertmanager's bucket name configuration from provider-specific to the new `alertmanager_storage_bucket_name` key.
* [ENHANCEMENT] Added `$._config.usageStatsConfig` to track the installation mode via the anonymous usage statistics. #3294
* [ENHANCEMENT] The query-tee node port (`$._config.query_tee_node_port`) is now optional. #3272
* [ENHANCEMENT] Add support for autoscaling distributors. #3378
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [ENHANCEMENT] Add support for autoscaling distributors. #3378
* [ENHANCEMENT] Added support for autoscaling distributors. #3378

@@ -58,6 +61,7 @@
* Renaming the alertmanager's bucket name configuration from provider-specific to the new `alertmanager_storage_bucket_name` key.
* [ENHANCEMENT] Added `$._config.usageStatsConfig` to track the installation mode via the anonymous usage statistics. #3294
* [ENHANCEMENT] The query-tee node port (`$._config.query_tee_node_port`) is now optional. #3272
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [ENHANCEMENT] The query-tee node port (`$._config.query_tee_node_port`) is now optional. #3272
* [ENHANCEMENT] Made the query-tee node port (`$._config.query_tee_node_port`) optional. #3272

Copy link
Contributor

Choose a reason for hiding this comment

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

Out-of-scope, so feel free to throw out feedback

@@ -45,7 +47,7 @@ However, if KEDA is not running successfully, there are consequences for Mimir a
- `keda-operator` is down (not critical): changes to `ScaledObject` CRD will not be reflected to the HPA until the operator will get back online. HPA functionality is not affected.
- `keda-operator-metrics-apiserver` is down (critical): HPA is not able to fetch updated metrics and it will stop scaling the deployment until metrics will be back. The deployment (e.g. queriers) will keep working but, in case of any surge of traffic, HPA will not be able to detect it (because of a lack of metrics) and so will not scale up.

The [alert `MimirQuerierAutoscalerNotActive`]({{< relref "../../monitor-grafana-mimir/_index.md" >}}) fires if HPA is unable to scale the deployment for any reason (e.g. unable to scrape metrics from KEDA metrics API server).
The [alert `MimirAutoscalerNotActive`]({{< relref "../../monitor-grafana-mimir/_index.md" >}}) fires if HPA is unable to scale the deployment for any reason (e.g. unable to scrape metrics from KEDA metrics API server).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The [alert `MimirAutoscalerNotActive`]({{< relref "../../monitor-grafana-mimir/_index.md" >}}) fires if HPA is unable to scale the deployment for any reason (e.g. unable to scrape metrics from KEDA metrics API server).
The [alert `MimirAutoscalerNotActive`]({{< relref "../../monitor-grafana-mimir/_index.md" >}}) fires if HPA is unable to scale the deployment for any reason, for example it is unable to scrape metrics from the KEDA metrics API server.

@pracucci pracucci merged commit 9090af5 into grafana:main Nov 4, 2022
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
* Operations: Add support for autoscaling distributors

* Update documentation regarding autoscaling

* Update MimirAutoscalerNotActive alert to support more components

* Fix compiling MimirAutoscalerNotActive

* Add dashboard row for distributor autoscaling metrics

This is WIP. Because the distributor scales off multiple metrics this
needs some adjusting.

* Split the distributor autoscaling panels into two: CPU and memory

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Simplied the autoscaling alert

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Apply suggestions from code review

Co-authored-by: Marco Pracucci <marco@pracucci.com>

* Update runbook documentation for MimirAutoscalerNotActive

* Update CHANGELOG

* Add distributor autoscaling to jsonnet tests

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
@mattmendick
Copy link
Contributor

Hey @jhesketh - autoscaling distributors - that's great! I think I see that it's in a few environments, but not production quite yet (via this). If that's not true, LMK what the reality is, and if it is true, can you let me know what your thoughts are on the rollout to more environments? Thanks!

@jhesketh
Copy link
Contributor Author

Hey @jhesketh - autoscaling distributors - that's great! I think I see that it's in a few environments, but not production quite yet (via this). If that's not true, LMK what the reality is, and if it is true, can you let me know what your thoughts are on the rollout to more environments? Thanks!

Hey Matt. That is correct. We have rolled it out in dev clusters, but not prod. (More detail on slack).

@mattmendick
Copy link
Contributor

Ah, my apologies for the private Grafana Labs repo link!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants