6964 mimir istio fixes #6965

Paul424 · 2023-12-19T16:06:38Z

What this PR does

Solves networking issues when running mimir injected (istio). Following issue's are addressed:

When query_scheduler is enabled the helm chart does not render the headless service for the query-frontend, but istio needs it to register the (headless) svc route that the querier is using to report back query results.
When memcached is enabled the svc cannot connect to the caches because the port naming prevent istio to do proper protocol selection.
grpc ports do need appProtocol setting to make sure istio selects the correct protocol.

Which issue(s) this PR fixes or relates to

Fixes #6964

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

…ing in the sidecar when querier tries to send query results back to the frontend (using the podip)

…oper protocol selection in istio; allow to optionally set appProtocol to tcp

…nts to istio for protocol-selection

CLAassistant · 2023-12-19T16:06:44Z

All committers have signed the CLA.

…s enabled for istio to register the podip's which is required for querier to report back the results

dimitarvdimitrov

Thank you for the contribution! We don't have much experience with istio, so this contribution is very valuable.

I left a couple of comments and questions, but the core of the change makes sense to me

dimitarvdimitrov · 2023-12-20T19:36:03Z

CHANGELOG.md

@@ -68,6 +68,7 @@
  * `store_gateway_zone_c_node_affinity_matchers`
 * [FEATURE] Ingester: Allow automated zone-by-zone downscaling, that can be enabled via the `ingester_automated_downscale_enabled` flag. It is disabled by default. #6850
 * [BUGFIX] Update memcached-exporter to 0.14.1 due to CVE-2023-39325. #6861
+* [BUGFIX] Always generate query-frontend headless service (otherwise istio doesn't register the pod ip's for querier to report back the results) #6964


Suggested change

* [BUGFIX] Always generate query-frontend headless service (otherwise istio doesn't register the pod ip's for querier to report back the results) #6964

* [BUGFIX] Always generate query-frontend headless service (otherwise, istio doesn't register the pod IPs for querier to report back the results). #6965

we usually put the PR number in the changelog entry, so it's easier to trace back the change

Let me make a fix there...

dimitarvdimitrov · 2023-12-20T19:38:05Z

operations/helm/charts/mimir-distributed/CHANGELOG.md

@@ -40,6 +40,7 @@ Entries should include a reference to the Pull Request that introduced the chang
 * [BUGFIX] Let the unified gatway/nginx config listen on IPv6 as well. Followup to #5948. #6204
 * [BUGFIX] Quote `checksum/config` when using external config. This allows setting `externalConfigVersion` to numeric values. #6407
 * [BUGFIX] Update memcached-exporter to 0.14.1 due to CVE-2023-39325. #6861
+* [BUGFIX] Allow Mimir to run injected (istio); optionally add appProtocol to memcached svc's and grpc ports and always generate query-frontend headless service (otherwise istio doesn't register the pod ip's for querier to report back the results) #6964


Suggested change

* [BUGFIX] Allow Mimir to run injected (istio); optionally add appProtocol to memcached svc's and grpc ports and always generate query-frontend headless service (otherwise istio doesn't register the pod ip's for querier to report back the results) #6964

* [BUGFIX] Allow Mimir to run injected (istio); optionally add appProtocol to memcached services and gRPC ports and always generate query-frontend headless service (otherwise, istio doesn't register the pod IPs for querier to report back the results). #6965

dimitarvdimitrov · 2023-12-20T19:45:24Z

operations/helm/charts/mimir-distributed/values.yaml

+  appProtocol:
+    # -- Set the optional service protocol. Ex: "tcp"
+    client: null


it took me a bit to understand the format of this parameter. Is client meant to represent the name of the port on the service? If so let's call it memcached-client, like it actually is on the Service manifest.

Perhaps adding a more explicit doc on the field would also help; something like

Suggested change

appProtocol:

# -- Set the optional service protocol. Ex: "tcp"

client: null

appProtocol:

# -- Set the optional service appProtocol for the memcached-client port. Ex: "tcp"

memcached-client: null

Also, can you apply this to all the ports in this PR?

yes, i had to somehow refer to the port, and indeed client isn't the best choice there, i will rework.

aha, that makes more sense now. Sorry for making you change this twice, but can you add a comment explaining what the null value means too?

also, can you make it explicit that the keys in appProtocol refer to individual ports of the service? A common on each key or on appProtocol itself should be sufficient

dimitarvdimitrov · 2023-12-20T20:10:12Z

operations/helm/charts/mimir-distributed/values.yaml

+  appProtocol:
+    # -- Set the optional service protocol. Ex: "tcp"
+    client: null


Adding this as a separate comment because it's a different topic: can you help me understand why we wouldn't want to set the appProtocol by default? I tried setting it to grpc in a cluster without istio and nothing broke 😄 but that's not a very rigorous test

Well actually I took this approach from the tempo charts, both the values structure and the choice of not adding it be default. I guess this is the most flexible way of doing it, if a deployment has multiple ports you can just extend the structure and when you run a different mesh requiring a different app-protocol you can easily configure it.

fair enough - consistency with tempo and flexibility with other meshes. Would have been time to be compatible by default with istio, but maybe we're not there.

… for istio

…vent sidecar returning 503

dimitarvdimitrov · 2024-01-02T17:50:59Z

The CI seems to be failing. The lint-jsonnet step is failing because the rendered kubernetes manifests for jsonnet haven't been updated. You can do that with make build-jsonnet-tests

However, lint-helm seems to be failing with some helm templating issue. I can reproduce the failure locally with running make build-helm-tests. After fixing them you should rerun that command and commit the diff.

duricanikolic · 2024-03-12T09:59:04Z

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

nbjohnson · 2024-03-26T13:26:50Z

@Paul424 Are you able to clean up this MR according to the requests so this is able to get merged? Having better istio support we be a great feature to have and I and I'm sure many others look forward to getting this merged in and having it available

Paul424 · 2024-03-26T19:12:19Z

@nbjohnson I would love to, but I no longer have access to the dev/env I used to work on this PR. Give me some time to setup a new environment... or if someone else wants to fork and complete the PR? fine for me.
Paul

github-actions · 2025-01-08T02:03:54Z

Thank you for your contribution. This pull request has been marked as stale because it has had no activity in the last 150 days. It will be closed in 30 days if there is no further activity. If you need more time, you can add a comment to the PR.

Paul424 added 3 commits December 19, 2023 16:27

fix: 6964: Istio needs the query-frontend-headless svc to enable rout…

145d7d8

…ing in the sidecar when querier tries to send query results back to the frontend (using the podip)

fix: 6964: memcached uses portname memcached-client which prevents pr…

422a3f4

…oper protocol selection in istio; allow to optionally set appProtocol to tcp

fix: 6964: optionally add appProtocol to mimir services to provide hi…

83dcea3

…nts to istio for protocol-selection

Paul424 requested a review from a team as a code owner December 19, 2023 16:06

56quarters added the helm label Dec 19, 2023

Paul424 added 2 commits December 20, 2023 09:41

fix: 6964: changelog and fix linting errors

8fde9ab

fix: 6964: Generate query-frontend headless svc even when scheduler i…

892e675

…s enabled for istio to register the podip's which is required for querier to report back the results

dimitarvdimitrov reviewed Dec 20, 2023

View reviewed changes

Paul424 added 3 commits December 27, 2023 11:36

chore: 6964: rework after review

07f2a67

fix: 9694: Add gRPC port to ruler service + generate headless service…

f83a04c

… for istio

fix: 6964: optionally set appProtocol for AlertManager service to pre…

399a031

…vent sidecar returning 503

duricanikolic added the release/notified-changelog-cut label Mar 12, 2024

github-actions bot added the stale label Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6964 mimir istio fixes #6965

6964 mimir istio fixes #6965

Paul424 commented Dec 19, 2023 •

edited

Loading

CLAassistant commented Dec 19, 2023 •

edited

Loading

dimitarvdimitrov left a comment

dimitarvdimitrov Dec 20, 2023

paul-bormans Dec 27, 2023

dimitarvdimitrov Dec 20, 2023

dimitarvdimitrov Dec 20, 2023

paul-bormans Dec 27, 2023

dimitarvdimitrov Jan 2, 2024

dimitarvdimitrov Jan 2, 2024

dimitarvdimitrov Dec 20, 2023

paul-bormans Dec 27, 2023

dimitarvdimitrov Jan 2, 2024

dimitarvdimitrov commented Jan 2, 2024

duricanikolic commented Mar 12, 2024

nbjohnson commented Mar 26, 2024

Paul424 commented Mar 26, 2024

github-actions bot commented Jan 8, 2025

	* [BUGFIX] Always generate query-frontend headless service (otherwise istio doesn't register the pod ip's for querier to report back the results) #6964
	* [BUGFIX] Always generate query-frontend headless service (otherwise, istio doesn't register the pod IPs for querier to report back the results). #6965

	* [BUGFIX] Allow Mimir to run injected (istio); optionally add appProtocol to memcached svc's and grpc ports and always generate query-frontend headless service (otherwise istio doesn't register the pod ip's for querier to report back the results) #6964
	* [BUGFIX] Allow Mimir to run injected (istio); optionally add appProtocol to memcached services and gRPC ports and always generate query-frontend headless service (otherwise, istio doesn't register the pod IPs for querier to report back the results). #6965

6964 mimir istio fixes #6965

Are you sure you want to change the base?

6964 mimir istio fixes #6965

Conversation

Paul424 commented Dec 19, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

CLAassistant commented Dec 19, 2023 • edited Loading

dimitarvdimitrov 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

dimitarvdimitrov commented Jan 2, 2024

duricanikolic commented Mar 12, 2024

nbjohnson commented Mar 26, 2024

Paul424 commented Mar 26, 2024

github-actions bot commented Jan 8, 2025

Paul424 commented Dec 19, 2023 •

edited

Loading

CLAassistant commented Dec 19, 2023 •

edited

Loading