-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update loki distributed for v1.32 #181
base: feat/release-v5.0.0
Are you sure you want to change the base?
Update loki distributed for v1.32 #181
Conversation
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.
GG overall, it looks neat! I left just a couple of comments
# admin: admin | ||
type: s3 | ||
s3: | ||
s3: http://minio:[email protected]:9000 |
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.
Is there a way to specify a secret for the credentials to be used for s3 on this new chart? Maybe we could use the same secret we create with the logging-operated
package (or create a dedicated one, to maintain this package as atomic as possible) mounted as env somewhere.
I know we are currently doing as committed here in the current release, I'm just hoping we could change it
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 it is a good idea, I'm exploring this possibility since, according to the documentation, it seems possible: https://grafana.com/docs/loki/latest/configure/#use-environment-variables-in-the-configuration
We can inject the secret contents as environment variables and then reference them from the Loki config.
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.
It would be great to have it in a secret. Having said that, pointing to the internal minio could work as a default value but it is important that we can override it later from the distribution, like we do now:
https://docs.kubernetesfury.com/docs/reference/OnPremises#specdistributionmoduleslogginglokiexternalendpoint
# S3 configuration (when type is "s3") | ||
s3: | ||
# S3 endpoint URL | ||
endpoint: http://minio:[email protected]:9000 |
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.
Same as above
``` | ||
|
||
With the `loki-stack-built.yaml` file, check differences with the current `deploy.yml` file and change accordingly. | ||
With the `loki-distributed-built.yaml` file and `loki-built.yaml` file, check differences with the current `deploy.yml` file and change accordingly. |
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.
Does this mean that the two outputs of the helm template
commands should be identical? I'm not sure I get why we should template twice with the two different charts
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.
The two outputs from the helm command won't be the same. We need to take resources that are not created with the loki-distributed
chart but are needed for the new loki version (e.g. the index-gateway
). Since the version 3.0 Grafana has implemented new Loki components that need to be deployed using the distributed mode. At the same time some components are no more generated by the helm-loki
chart (e.g. ServiceMonitors
). Hence we need to cherry pick the resources generated. For future work I think we can do a refactor to completely align with the new chart, moving the ServiceMonitors to a custom manifest to avoid to lose them.
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.
let's add this comment to the maintenance guide.
I'm not a fan of mixing the two charts. I'd rather use the new one and add via kustomize what is missing, like we do in the other modules.
Is it too much effort to do this now @lukeskywlkr?
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'm working on it, I'll be back with more info ASAP
Starting with the vTBD of the Logging Core Module Loki version has been bumped to 3.4.2. Please refer to [`loki documentation`](https://grafana.com/docs/loki/v3.4.x/setup/upgrade/) | ||
for the complete release notes. |
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 is nice to know, but it does not tell me anything about breaking changes. Is this the right place or should it be under the "New features" section?
``` | ||
|
||
With the `loki-stack-built.yaml` file, check differences with the current `deploy.yml` file and change accordingly. | ||
With the `loki-distributed-built.yaml` file and `loki-built.yaml` file, check differences with the current `deploy.yml` file and change accordingly. |
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.
let's add this comment to the maintenance guide.
I'm not a fan of mixing the two charts. I'd rather use the new one and add via kustomize what is missing, like we do in the other modules.
Is it too much effort to do this now @lukeskywlkr?
# admin: admin | ||
type: s3 | ||
s3: | ||
s3: http://minio:[email protected]:9000 |
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.
It would be great to have it in a secret. Having said that, pointing to the internal minio could work as a default value but it is important that we can override it later from the distribution, like we do now:
https://docs.kubernetesfury.com/docs/reference/OnPremises#specdistributionmoduleslogginglokiexternalendpoint
persistence: | ||
# -- Enable creating PVCs which is required when using boltdb-shipper | ||
enabled: true |
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.
do we need to set this to true? we are moving away from boltdb aren't we?
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.
we need to remember to align the configuration file also on the distribution side https://github.com/sighupio/fury-distribution/blob/main/templates/distribution/manifests/logging/patches/loki-config.yaml.tpl
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.
We should also change the patchesJson6902
field in every Kustomize project inside loki-configs
to use the newer patches
syntax (you can use kustomize edit fix
to automatically do that)
And don't forget to test that the build output is the same with kustomize v5.6.0 as it was with v3.10.0 |
Summary 💡
Update
loki-distributed
to the latest loki version (3.4.2)Closes: https://github.com/sighupio/product-management/issues/567
Description 📝
loki-distributed
chart to0.80.2
helm-loki
chart version6.28.0
loki-distributed
chart as base (using image tag3.4.2
)to introduce new components from thehelm-loki
chartloki-distributed
nomenclaturemax_label_names_per_series
to 30 since the new default is 15Breaking Changes 💔
shared_store
,shared_store_key_prefix
,max_transfer_retries
Tests performed 🧪
Future work 🔧
Complete the chart migration, if possible, in order to keep the alignment with the mainstream development.