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

[Helm] Allow node port on single binary service #8166

Closed
wants to merge 1 commit into from

Conversation

jkroepke
Copy link
Contributor

Signed-off-by: Jan-Otto Kröpke jok@cloudeteer.de

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@jkroepke jkroepke requested a review from a team as a code owner January 16, 2023 13:47
@github-actions github-actions bot added area/helm type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories labels Jan 16, 2023
@grafanabot
Copy link
Collaborator

./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
Copy link
Collaborator

./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
Copy link
Collaborator

./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%

@sapslaj
Copy link
Contributor

sapslaj commented Jan 17, 2023

Hey @jkroepke looks like we had the same idea. #8146 #8147

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.

Can you please provide a description and explain why you need this functionality? We are intentionally limiting what can be configured on the "internal" services.

@jkroepke
Copy link
Contributor Author

We have an central grafana instance (from loki perspective outside of the cluster) and we do not have flexibility of using ingress (we have only public ingress) and we do not want to expose loki to public internet.

The central grafana outside of the cluster needs direct access. node port is the easiest available option for us. In that case, the single binary loki "internal" service needs to be exposed to an other cluster.

@grafanabot
Copy link
Collaborator

./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%

@jkroepke
Copy link
Contributor Author

@trevorwhitney how to generate the docs?

make -C docs sources/installation/helm/reference.md

generates no change

@MasslessParticle
Copy link
Contributor

@jkroepke This was an error on our end. We fixed it so a rebase should get this to pass. Sorry about that.

Signed-off-by: Jan-Otto Kröpke <jok@cloudeteer.de>
@grafanabot
Copy link
Collaborator

./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%

@trevorwhitney
Copy link
Collaborator

As discussed in #8147, enabling the gateway for single binary mode #8269 will solve a few issues at once, while keeping the config smaller and different deployment modes more consistent. Closing in favor of that approach.

@jkroepke
Copy link
Contributor Author

@trevorwhitney I'm bit confused, keeping the configuration small while integrate an automatic way to identity customer need feels like a conflict (#8185)

@jkroepke jkroepke deleted the node-port branch January 25, 2023 08:53
@trevorwhitney
Copy link
Collaborator

@jkroepke does my implementation clarify at all? #8284. This requires no additional config (keeping it small), and only required the change of a single default value (which I believe was done in a non-breaking way). It does make it so the gateway is always enabled, but I feel like that's a good thing from a consistency perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm size/M type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants