Helm chart: Add enableServiceLinks #7129

TAYTS · 2022-09-10T13:07:55Z

What this PR does / why we need it:
This is to allow disable enableServiceLinks so that the service environments are not injected into the pods where in my scenario Loki is deployed in the same namespace as the Knative services resulted the Loki pod unable to start up due to too many service environment being injected.

Reference issue for pod unable to startup due to too many services: kubernetes/kubernetes#84539 (comment)

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

CLAassistant · 2022-09-10T13:07:59Z

All committers have signed the CLA.

grafanabot · 2022-09-10T13:13:31Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

jeschkies · 2022-09-12T16:18:43Z

Could you raise the Helm Chart version so it's released?

grafanabot · 2022-09-12T21:12:46Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

TAYTS · 2022-09-14T20:51:27Z

@jeschkies I have updated the Helm chart version, please help to review again, thank you

trevorwhitney

Can you describe the problem a bit more? Is there some technical limit on the number of service environment variables that can be injected into a POD? What was the technical issue this created for you?

trevorwhitney · 2022-09-20T22:47:11Z

production/helm/loki/templates/gateway/deployment-gateway.yaml

@@ -32,6 +32,13 @@ spec:
        {{- include "loki.gatewaySelectorLabels" . | nindent 8 }}
    spec:
      serviceAccountName: {{ include "loki.serviceAccountName" . -}}
+      {{- if semverCompare ">=1.13-0" .Capabilities.KubeVersion.GitVersion }}
+      {{- if or (.Values.loki.enableServiceLinks) (eq (.Values.loki.enableServiceLinks | toString) "<nil>") }}


why doe we need the toString compared to "<nil>"? shouldn't helm treat a nil value as falsey? Also, would you consider moving this to a single if statement, potentially even extracting a template function if it's messy?

The toString is to set the enableServiceLinks to true (default value) when enableServiceLinks is not provided in the values.yaml.

We will need to have 2 if because in k8s version < v1.13.0, there is no enableServiceLinks and I have moved this chunk into template to make it dry.

We were deploying the loki-stack in the same namespace as knative and as the service grow, we were not able to start the pod due to too many services environment variables being injected. We are moving loki-stack to different namespace at the moment to fix the issue, but thought this feature could help those that has constraint with the namespace(ie. only allow to deploy to certain namespace).

You can refer to the link I have attached about the pod unable to startup due to too many service environments injected into the pods.
Here is the link for an interesting experiment did for the knative: knative/serving#8498

grafanabot · 2022-09-24T07:03:41Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-10-01T13:02:33Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0.1%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

TAYTS · 2022-10-16T01:34:38Z

@trevorwhitney Is it possible to help review this MR? Thank you so much

trevorwhitney

After reading a bit more into those Knative issue you linked (thank you) I'm open to this change. I have one small tweak to your template function I'd like to see, then LGTM!

production/helm/loki/templates/_helpers.tpl

trevorwhitney · 2022-10-18T22:17:32Z

production/helm/loki/templates/_helpers.tpl

+{{- if or (.Values.loki.enableServiceLinks) (eq (.Values.loki.enableServiceLinks | toString) "<nil>") -}}
+enableServiceLinks: true
+{{- else -}}
+enableServiceLinks: false
+{{- end -}}


I still don't love the to string and comparison to the string "<nil>", as I feel like that's too tightly coupled to go template implementation details. How would you feel about switching the logic to avoid that?

Suggested change

{{- if or (.Values.loki.enableServiceLinks) (eq (.Values.loki.enableServiceLinks | toString) "<nil>") -}}

enableServiceLinks: true

{{- else -}}

enableServiceLinks: false

{{- end -}}

{{- if not .Values.loki.enableServiceLinks -}}

enableServiceLinks: false

{{- else -}}

enableServiceLinks: true

{{- end -}}

this result in a true if it's explicitly true or omitted

This won't work because if the value is omitted, it is treated as false

I made the change to {{- if or (.Values.loki.enableServiceLinks) (ne .Values.loki.enableServiceLinks false) -}} so instead of using toString will check if it is false, this way will decouple with go templating implementation, hope this is ok for you

I'm not following. My understanding was we want enableServiceLinks to be true if it's either explicitly set to true or if it's omitted. The above should work since we set the default to true, so if omitted it should use the default value. I checked out your branch, and made only the change above, and these are my results.

helm template . --set loki.enableServiceLinks=false --show-only templates/gateway/deployment-gateway.yaml:

--- # Source: loki/templates/gateway/deployment-gateway.yaml ... spec: serviceAccountName: loki enableServiceLinks: false ...

helm template . --set loki.enableServiceLinks=true --show-only templates/gateway/deployment-gateway.yaml:

--- # Source: loki/templates/gateway/deployment-gateway.yaml ... spec: serviceAccountName: loki enableServiceLinks: true ...

helm template . --show-only templates/gateway/deployment-gateway.yaml

--- # Source: loki/templates/gateway/deployment-gateway.yaml ... spec: serviceAccountName: loki enableServiceLinks: true ...

Sorry for the confusion, but is this not the correct behavior?

This second condition is to cover the case where the enableServiceLinks is missing in values.yaml or it is set to empty string

If it's missing in values.yaml I don't understand why it wouldn't get the default value?

grafanabot · 2022-10-22T22:42:55Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0.1%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

production/helm/loki/Chart.yaml

grafanabot · 2022-10-25T20:42:36Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-10-25T22:21:30Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-11-08T22:54:48Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

jeschkies

Thanks. Could you generate the documentation reference?

grafanabot · 2023-01-17T22:49:34Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

jeschkies · 2023-01-18T11:49:45Z

@TAYTS could you merge the current main branch? It will fix the build error.

grafanabot · 2023-01-18T17:36:26Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-18T18:37:02Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-18T20:21:33Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

**What this PR does / why we need it**: This is to allow disable `enableServiceLinks` so that the service environments are not injected into the pods where in my scenario Loki is deployed in the same namespace as the Knative services resulted the Loki pod unable to start up due to too many service environment being injected. Reference issue for pod unable to startup due to too many services: kubernetes/kubernetes#84539 (comment) Co-authored-by: Trevor Whitney <trevorjwhitney@gmail.com>

TAYTS requested a review from a team as a code owner September 10, 2022 13:07

pull-request-size bot added the size/M label Sep 10, 2022

github-actions bot added the area/helm label Sep 10, 2022

jeschkies requested a review from trevorwhitney September 12, 2022 16:18

trevorwhitney reviewed Sep 20, 2022

View reviewed changes

pull-request-size bot added size/S and removed size/M labels Sep 24, 2022

TAYTS force-pushed the feat/add-enable-service-links branch from d33f3ee to e8eccf2 Compare September 24, 2022 06:58

TAYTS requested a review from trevorwhitney September 24, 2022 21:28

TAYTS force-pushed the feat/add-enable-service-links branch from 7717b00 to adcf94f Compare October 1, 2022 12:55

trevorwhitney reviewed Oct 18, 2022

View reviewed changes

TAYTS force-pushed the feat/add-enable-service-links branch from adcf94f to e3a1ef5 Compare October 22, 2022 22:35

trevorwhitney reviewed Oct 25, 2022

View reviewed changes

production/helm/loki/Chart.yaml Outdated Show resolved Hide resolved

TAYTS force-pushed the feat/add-enable-service-links branch from e3a1ef5 to b0bd555 Compare October 25, 2022 20:36

jeschkies approved these changes Jan 12, 2023

View reviewed changes

trevorwhitney force-pushed the feat/add-enable-service-links branch from 43b0f52 to 0a34c27 Compare January 17, 2023 22:44

pull-request-size bot added size/M and removed size/S labels Jan 17, 2023

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jan 17, 2023

trevorwhitney force-pushed the feat/add-enable-service-links branch from 0a34c27 to e295046 Compare January 18, 2023 17:32

TAYTS and others added 6 commits January 18, 2023 13:16

Add enableServiceLinks to values

06a9f09

Create template for enableServiceLinks

6bfd777

Bump chart version

d4b8f84

Replace toString and nil comparison with ne

632aa42

Change .KubeVersion.GitVersion to .KubeVersion.Version

db1dbd3

update reference docs

12cbd84

trevorwhitney force-pushed the feat/add-enable-service-links branch from e295046 to 12cbd84 Compare January 18, 2023 20:16

trevorwhitney mentioned this pull request Jan 18, 2023

Helm: Add ingester config in helm values #7733

Merged

5 tasks

trevorwhitney merged commit 94ee5a2 into grafana:main Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm chart: Add enableServiceLinks #7129

Helm chart: Add enableServiceLinks #7129

TAYTS commented Sep 10, 2022

CLAassistant commented Sep 10, 2022 •

edited

Loading

grafanabot commented Sep 10, 2022

jeschkies commented Sep 12, 2022

grafanabot commented Sep 12, 2022

TAYTS commented Sep 14, 2022

trevorwhitney left a comment

trevorwhitney Sep 20, 2022

TAYTS Sep 24, 2022

TAYTS Sep 24, 2022

grafanabot commented Sep 24, 2022

grafanabot commented Oct 1, 2022

TAYTS commented Oct 16, 2022

trevorwhitney left a comment

trevorwhitney Oct 18, 2022 •

edited

Loading

trevorwhitney Oct 18, 2022

TAYTS Oct 22, 2022

TAYTS Oct 22, 2022

trevorwhitney Oct 25, 2022

TAYTS Oct 25, 2022 •

edited

Loading

trevorwhitney Oct 31, 2022

grafanabot commented Oct 22, 2022

grafanabot commented Oct 25, 2022

grafanabot commented Oct 25, 2022

grafanabot commented Nov 8, 2022

jeschkies left a comment

grafanabot commented Jan 17, 2023

jeschkies commented Jan 18, 2023

grafanabot commented Jan 18, 2023

grafanabot commented Jan 18, 2023

grafanabot commented Jan 18, 2023

Helm chart: Add enableServiceLinks #7129

Helm chart: Add enableServiceLinks #7129

Conversation

TAYTS commented Sep 10, 2022

CLAassistant commented Sep 10, 2022 • edited Loading

grafanabot commented Sep 10, 2022

jeschkies commented Sep 12, 2022

grafanabot commented Sep 12, 2022

TAYTS commented Sep 14, 2022

trevorwhitney 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

grafanabot commented Sep 24, 2022

grafanabot commented Oct 1, 2022

TAYTS commented Oct 16, 2022

trevorwhitney left a comment

Choose a reason for hiding this comment

trevorwhitney Oct 18, 2022 • edited Loading

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

TAYTS Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented Oct 22, 2022

grafanabot commented Oct 25, 2022

grafanabot commented Oct 25, 2022

grafanabot commented Nov 8, 2022

jeschkies left a comment

Choose a reason for hiding this comment

grafanabot commented Jan 17, 2023

jeschkies commented Jan 18, 2023

grafanabot commented Jan 18, 2023

grafanabot commented Jan 18, 2023

grafanabot commented Jan 18, 2023

CLAassistant commented Sep 10, 2022 •

edited

Loading

trevorwhitney Oct 18, 2022 •

edited

Loading

TAYTS Oct 25, 2022 •

edited

Loading