fix: mixin generation when cluster label is changed #12613

QuentinBisson · 2024-04-15T10:02:45Z

What this PR does / why we need it:

This PR fixes mixin generation when the per_cluster_label value is changed

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
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

QuentinBisson · 2024-04-15T11:52:34Z

The diff after generation can be seen here https://github.com/QuentinBisson/loki-upstream/pull/1/files#diff-7691b03fc1df22118e5c4440a69f2f0420ae833be78dbfd8fee564ce1df510e0

QuentinBisson · 2024-04-15T11:53:44Z

@MichelHollands could you take a look at this?

Signed-off-by: QuentinBisson <quentin@giantswarm.io>

QuentinBisson · 2024-04-15T12:25:12Z

@JStickler Do you know who could help review this PR?

cstyan

there's enough changes here that we would want to test deploying these dashboards ourselves with your changes to check that nothing is broken, I don't have time to do that myself this week but I know @shantanualsi has been doing some dashboard work so maybe he can test them out?

cstyan · 2024-04-16T18:30:02Z

production/loki-mixin/alerts.libsonnet

+              message: std.strReplace(|||
                {{ $labels.cluster }} {{ $labels.namespace }} has had {{ printf "%.0f" $value }} compactors running for more than 5m. Only one compactor should run at a time.
-              |||,
+              |||, 'cluster', $._config.per_cluster_label),


I'm not sure this will work as written, it will look specifically for cluster in the string when in reality the values for the labels would be something like prod-<region> and loki-prod-#, so we would have prod-<region> loki-prod-# has had <some value> compactors running for more than 5m as the message.

From my tests here, it actually replaces the label correctly in the rule https://github.com/QuentinBisson/loki-upstream/pull/1/files#diff-147d57f875c8b29ffd1bfb7026b4d16da82db9df6b9a2d7dfb1cb82d18fa50a4R36.

I tried to use the % syntax with %s but it did not work due to the printf "%.0f" $value

ah right, the jsonnet strReplace is happening earlier than I thought

is that still not incorrect though? in your code within config.libsonnet the cluster_label is still cluster.

I only regenerated the ssd mixins so I changed this file https://github.com/QuentinBisson/loki-upstream/pull/1/files#diff-58c1308d64904785b992854f36800a299e22dbdab03edc9181729b506ad8a3c3R9 :D

ah okay! looks correct then

cstyan · 2024-04-16T18:33:07Z

production/loki-mixin/dashboards/loki-operational.libsonnet

+                                 // Replace the recording rules cluster label with the per-cluster label
+                                 then std.strReplace(
+                                   // Replace the cluster label for equality matchers with the per-cluster label
+                                   std.strReplace(
+                                     // Replace the cluster label for regex matchers with the per-cluster label
+                                     std.strReplace(
+                                       expr,
+                                       'cluster=~"$cluster"',
+                                       $._config.per_cluster_label + '=~"$cluster"'
+                                     ),
+                                     'cluster="$cluster"',
+                                     $._config.per_cluster_label + '="$cluster"'
+                                   ),
+                                   'cluster_',
+                                   $._config.per_cluster_label + '_'
+                                 )


just noting that having to do something like this is yet another reason for us to try and get rid of these giant copy/paste json dashboards at some point

cstyan · 2024-04-16T18:33:41Z

production/loki-mixin/dashboards/loki-writes.libsonnet

@@ -9,10 +9,9 @@ local utils = import 'mixin-utils/utils.libsonnet';
                          local cfg = self,

                          showMultiCluster:: true,
-                          clusterLabel:: $._config.per_cluster_label,


why are we removing this?

I removed it because it is unused

cstyan · 2024-04-16T18:33:55Z

production/loki-mixin/dashboards/loki-reads.libsonnet

@@ -31,10 +31,9 @@ local utils = import 'mixin-utils/utils.libsonnet';
                         local cfg = self,

                         showMultiCluster:: true,
-                         clusterLabel:: $._config.per_cluster_label,


same here and others

Same as above, I did not see it being used

QuentinBisson · 2024-04-16T19:31:36Z

Also @cstyan and @shantanualsi this #12567 might be easier to review as it only fixes the compactor pod matcher which is currently broken

QuentinBisson · 2024-04-16T19:31:49Z

And thanks for the review :)

shantanualsi · 2024-04-19T08:40:29Z

production/loki-mixin-compiled-ssd/alerts.yaml

-    for: 5m
-    labels:
-      severity: warning
+    - name: loki_alerts


are these indentation changes intentional or from your editor? can you please keep the original indentation so as to keep this diff clean? thank you!

Those happen when we generate the alerts from the mixins so I can realign them sure but they probably hsould not have been indented at all :D

What do you want me to do?

I think the confusion here was caused by differences in jsonnet versions that were generating the compiled mixin, that's my fault. I added the github action to check the jsonnet linting and compiled mixins, which is using a different version of jsonnet within the build image. So unfortunately make loki-mixin and make BUILD_IN_CONTAINER=false loki-mixin will get you different results at the moment. I'm working on resolving that today.

shantanualsi · 2024-04-19T08:44:06Z

@QuentinBisson I'll try and spend some time testing these changes today on our dashboards. Meanwhile, when you get some time, can you resolve the conflicts on this PR?
Thank you!

QuentinBisson · 2024-04-19T08:53:01Z

@shantanualsi thanks for reviewing those :) I fixed the conflicts and the list of changes is a lot less :D

cstyan · 2024-04-19T19:58:04Z

Had a chance to test this out locally, no obvious breakages so lets merge and we can deal with any issues via follow up PRs

QuentinBisson · 2024-04-19T20:40:08Z

Thank you for the merge :)

QuentinBisson requested a review from a team as a code owner April 15, 2024 10:02

pull-request-size bot added the size/L label Apr 15, 2024

QuentinBisson changed the title ~~Fix mixin generation when cluster label is changed~~ fix: mixin generation when cluster label is changed Apr 15, 2024

QuentinBisson force-pushed the fix-cluster-label branch 3 times, most recently from c92564a to eb1a6e4 Compare April 15, 2024 11:46

Fix mixin generation when cluster label is changed

e858975

Signed-off-by: QuentinBisson <quentin@giantswarm.io>

QuentinBisson force-pushed the fix-cluster-label branch from f4ca44b to e858975 Compare April 15, 2024 11:59

Merge branch 'main' into fix-cluster-label

61451fc

Merge branch 'main' into fix-cluster-label

152d533

cstyan reviewed Apr 16, 2024

View reviewed changes

Merge branch 'main' into fix-cluster-label

7f413b7

shantanualsi reviewed Apr 19, 2024

View reviewed changes

Merge branch 'main' into fix-cluster-label

dd95b2e

pull-request-size bot added size/M and removed size/L labels Apr 19, 2024

cstyan approved these changes Apr 19, 2024

View reviewed changes

cstyan merged commit 1ba7a30 into grafana:main Apr 19, 2024
15 checks passed

This was referenced Apr 22, 2024

chore(k199): release 3.1.0 #12717

Open

chore(k200): release 3.1.0 #12812

Open

loki-gh-app bot mentioned this pull request May 6, 2024

chore(k201): release 3.1.0 #12894

Open

This was referenced May 13, 2024

chore(k202): release 3.1.0 #12945

Open

chore(k203): release 3.1.0 #12988

Open

This was referenced May 27, 2024

chore(k204): release 3.1.0 #13037

Open

chore(k205): release 3.1.0 #13102

Open

This was referenced Jun 10, 2024

chore(k206): release 3.1.0 #13184

Open

chore(k207): release 3.1.0 #13225

Open

loki-gh-app bot mentioned this pull request Jun 24, 2024

chore(k208): release 3.1.0 #13291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: mixin generation when cluster label is changed #12613

fix: mixin generation when cluster label is changed #12613

QuentinBisson commented Apr 15, 2024

QuentinBisson commented Apr 15, 2024

QuentinBisson commented Apr 15, 2024

QuentinBisson commented Apr 15, 2024

cstyan left a comment

cstyan Apr 16, 2024

QuentinBisson Apr 16, 2024

cstyan Apr 16, 2024

QuentinBisson Apr 16, 2024

cstyan Apr 16, 2024

cstyan Apr 16, 2024

cstyan Apr 16, 2024

QuentinBisson Apr 16, 2024

cstyan Apr 16, 2024

QuentinBisson Apr 16, 2024

QuentinBisson commented Apr 16, 2024

QuentinBisson commented Apr 16, 2024

shantanualsi Apr 19, 2024

QuentinBisson Apr 19, 2024

QuentinBisson Apr 19, 2024

cstyan Apr 19, 2024

shantanualsi commented Apr 19, 2024 •

edited

Loading

QuentinBisson commented Apr 19, 2024

cstyan commented Apr 19, 2024

QuentinBisson commented Apr 19, 2024

fix: mixin generation when cluster label is changed #12613

fix: mixin generation when cluster label is changed #12613

Conversation

QuentinBisson commented Apr 15, 2024

QuentinBisson commented Apr 15, 2024

QuentinBisson commented Apr 15, 2024

QuentinBisson commented Apr 15, 2024

cstyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuentinBisson commented Apr 16, 2024

QuentinBisson commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shantanualsi commented Apr 19, 2024 • edited Loading

QuentinBisson commented Apr 19, 2024

cstyan commented Apr 19, 2024

QuentinBisson commented Apr 19, 2024

shantanualsi commented Apr 19, 2024 •

edited

Loading