-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 - Loki: Introduce runtimeConfig #8179
Conversation
./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% |
b5be135
to
16b2b65
Compare
./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% |
16b2b65
to
a460467
Compare
./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% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @nervo, thanks for putting this together. It's very useful!
I've left a couple of comments and questions.
production/helm/loki/CHANGELOG.md
Outdated
@@ -21,6 +21,9 @@ Entries should include a reference to the pull request that introduced the chang | |||
- [CHANGE] **BREKAING** Rename and change format of `enterprise.provisioner.tenants`. Property has been renamed to `enterprise.provisioner.additionalTenants`, and is now an array of objects rather than string. Each object must contain a `name` and a `secretNamespace` field, where `name` is the name of the tenant and `secretNamespace` is the namespace to create the secret with the tenant's read and write token. | |||
- [CHANGE] **BREAKING** Change the structure of `monitoring.selfMonitoring.tenant` from a string to an object. The new object must have a `name` and a `secretNamespace` field, where `name` is the name of the self-monitoring tenant and `secretNamespace` is the namespace to create an additional secret with the tenant's token. A secret will still also be created in the release namespace as it's needed by the Loki canary. | |||
- [CHANGE] **BREAKING** Remove ability to create self-monitoring resources in different namespaces (with the exception of dashboard configmaps). | |||
## 3.10.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be 4.0.1 (as the change on Chart.yaml) and be added before 4.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, looking at the proposed changes in production/helm/loki/Chart.yaml
and production/helm/loki/README.md
, I'm not sure if those align with the release process for the Loki HELM charts.
@trevorwhitney may have more context on this, but, shouldn't we have a Next section here with the changes to be released with the next version of the charts? Otherwise, any new PR to these charts will result in a new chart version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops :) Nice catch, sorry.
Fyi, in my company, we use https://keepachangelog.com/en/1.0.0/ . It recommends a ## [Unreleased]
section at the top of the changelog, comparable to the Next
section you suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your great work. Please address our comments out or should be good to go.
a460467
to
b6eab91
Compare
./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% |
b6eab91
to
1385533
Compare
./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% |
5b4443b
to
ad707b4
Compare
./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% |
ad707b4
to
077ad8f
Compare
./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% |
comments addressed, pr rebased, docs fixed, tests ok ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for the contribution. You'll need to rebase and bump the version, and I think it's better to bump the minor version instead of the patch as this is new functionality. Thanks, LGTM!
production/helm/loki/Chart.yaml
Outdated
@@ -4,7 +4,7 @@ name: loki | |||
description: Helm chart for Grafana Loki in simple, scalable mode | |||
type: application | |||
appVersion: 2.7.0 | |||
version: 4.1.0 | |||
version: 4.1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just to a minor version for this. New functionality, so not really a patch IMO
@@ -0,0 +1,9 @@ | |||
apiVersion: v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jeschkies, keep a ConfigMap
for now, and I'd like to see us implement a configuration as talked about in #7358 to specify ConfigMap
vs Secret
for both this and the regular config at a later point.
@@ -206,6 +209,9 @@ loki: | |||
max_cache_freshness_per_query: 10m | |||
split_queries_by_interval: 15m | |||
|
|||
# -- Provides a reloadable runtime configuration file for some specific configuration | |||
runtimeConfig: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this decision to make this loki.runtimeConfig
rather than top level 👍
077ad8f
to
fc645ff
Compare
I took the liberty of rebasing and bumping the version, hope you don't mind 😄 |
./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 this looks good to go, just needs an approval from you. |
Why is the changelog pointing to 4.1.1 while chart version is 4.4.0? |
@trallnag good catch, looks likes a bit of cleanup is needed. |
This is completely inspired by similar value available in loki-distributed via grafana/helm-charts#1827, which is itself largely inspired by similar value in mimir-distributed helm chart.
The idea is to have a dedicated configmap to handle runtime_config, alway present even if empty. This way, having it seperately from main config, components pods are guaranteed not to restart for any changes.
Fixes #7918
Some questionable details:
loki-distributed
/mimir-distributed
,runtimeConfig
is available at values root (.Values.runtimeConfig
). I feel a bit uncomfortable with this, i would have preferred.Values.loki.runtimeConfig
, but having kind-of compatibility withloki-distributed
/mimir-distributed
may be more desirable.mimir-distributed
uses/data
for mounting data and/var/mimir
for mounting runtime config. Asloki
uses/var/loki
for mounting its data, i've decided to stay in/var
directory and uses/var/loki-runtime
for its runtime config.Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updateddocs/sources/upgrading/_index.md