Skip to content

Conversation

@Kellen275
Copy link
Contributor

@Kellen275 Kellen275 commented Oct 21, 2024

What this PR does / why we need it:
When running a distributed Loki with zone aware replication. The ingester HPA is not configured to properly locate the 3 zoned ingester StatefulSets. This MR adds zoned HPAs similar to how the existing ingester headless service and statefulsets are handled.

Which issue(s) this PR fixes:
Fixes #14021

Special notes for your reviewer:
This MR simply copies the existing hpa.yaml into 3 zoned yamls, and updates 3 lines

  1. Line 2, to verify zoneAwareReplication is enabled
  2. metadata.name to append the -zone-{a|b|c} suffix
  3. spec.scaleTargetRef.name to append the -zone-{a|b|c} suffix

We also update the hpa.yaml to not deploy when zoneAwareReplication is enabled (following the same pattern seen in service-ingester-headless.yaml

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@Kellen275 Kellen275 requested a review from a team as a code owner October 21, 2024 22:12
@CLAassistant
Copy link

CLAassistant commented Oct 21, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@github-actions

This comment has been minimized.

4 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@trevorwhitney
Copy link
Collaborator

Looks like helm-ci check is failing? This is good to go as soon as that is passing.

@github-actions

This comment has been minimized.

@Kellen275
Copy link
Contributor Author

@trevorwhitney From what I can tell, the CI fails once the branch is out of date. But when I rebase I'm not seeing any failures. I sense I'm missing something simple here, sorry 😅

@Kellen275
Copy link
Contributor Author

Kellen275 commented Nov 2, 2024

Error: INSTALLATION FAILED: template: kube-prometheus-stack/templates/prometheus/prometheus.yaml:262:11: 
executing "kube-prometheus-stack/templates/prometheus/prometheus.yaml" at <ne .Values.prometheus.
prometheusSpec.scrapeConfigNamespaceSelector nil>: error calling ne: uncomparable type map[string]interface
 {}: map[]

Line in question: https://github.com/prometheus-community/helm-charts/blob/kube-prometheus-stack-65.5.1/charts/kube-prometheus-stack/templates/prometheus/prometheus.yaml#L262

Looking at other open PRs, it appears that ~all helm chart changes are failing with the same error

@trevorwhitney
Copy link
Collaborator

@Kellen275 yeah, it's breaking everything helm related at the moment, including releases

@trevorwhitney
Copy link
Collaborator

I've created an issue, this is affecting multiple PRs and all helm releases #14799

@github-actions

This comment has been minimized.

3 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@trevorwhitney
Copy link
Collaborator

The helm linting failure seems to be related to the submodule in the operator folder, and submitting a PR from a fork. I've asked for some help looking into it.

@trevorwhitney
Copy link
Collaborator

the helm linting here has been fixed if you rebase with main

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Kubernetes Manifest Diff Summary

Scenario: default-single-binary-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: default-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: ingress-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: legacy-monitoring-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

Scenario: simple-scalable-aws-kube-irsa-values (Added: 0, Modified: 0, Removed: 0)

Summary:

  • Added: 0

  • Modified: 0

  • Removed: 0

Added Files

No added files

Modified Files

No modified files

Removed Files

No removed files

@trevorwhitney trevorwhitney merged commit 80e46f7 into grafana:main Nov 27, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants