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

Loki Helm Chart: Add missing server config ingester_client #12054

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

andersondario
Copy link

@andersondario andersondario commented Feb 24, 2024

What this PR does / why we need it: It adds a missing server configuration option in Helm chart, as described here in the docs: Supported contents and default values of loki.yaml

Which issue(s) this PR fixes:
#6182

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • 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

@andersondario andersondario requested a review from a team as a code owner February 24, 2024 19:32
@CLAassistant
Copy link

CLAassistant commented Feb 24, 2024

CLA assistant check
All committers have signed the CLA.

@MichelHollands
Copy link
Contributor

@andersondario What is your use case here? This setting doesn't seem needed in SSD mode so I'm not sure how this would fix anything. Also you don't specify a default value.

@niklas-gullberg-ing
Copy link

This would help #6182 where the suggested workaround is to increase ingester_client.remote_timeout, since the config is currently unavailable through helm.

@andersondario
Copy link
Author

andersondario commented Feb 29, 2024

@andersondario What is your use case here? This setting doesn't seem needed in SSD mode so I'm not sure how this would fix anything. Also you don't specify a default value.

I had to do a workaround a few months ago when I installed Loki in a Kubernetes cluster on GCP. I don't remember in detail what the problem was, but I think it was related to the issue #6182 as @niklas-gullberg-ing mentioned

#6182 (comment)

@ivanfavi
Copy link

@coro Is there anything we can do to get this PR merged?

@coro
Copy link
Contributor

coro commented Jun 11, 2024

I'm afraid I'm not a maintainer of this repo, but @DylanGuedes & @JStickler may be able to help! I believe there is also a Community Call weekly where you may be able to ask for help from the community.

@ivanfavi
Copy link

@MichelHollands Could you please assist in merging this PR? Is there anything missing?

@DylanGuedes
Copy link
Contributor

In case this is blocking any of you: you can use the other Helm config sections to accomplish the same. You can add a ingester_client to your own structuredConfig section and it will work similar to this one.

@ivanfavi
Copy link

@DylanGuedes Using structuredConfig would completely override the templated value of loki.config. In our case, we prefer to continue using the templated approach for Loki configuration.

This pull request appears to be a straightforward addition to the Helm chart. Could you let us know if there's anything incorrect or missing in our proposed changes? We're keen to understand why it might not be suitable for merging.

@alex-berger
Copy link

Having the same need, so would love to see this PR merged.

@CmdrSharp
Copy link

I too would appreciate this to be merged. It seems like a very minor, non-breaking change. What's holding it up?

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.

sorry this has been sitting for so long. I'm good to get it merged. Can you remove the manual update of the version, and add a changleog entry?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants