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

[DOC] Updates from working session on Helm docs #2590

Merged
merged 6 commits into from
Jul 17, 2023
Merged

[DOC] Updates from working session on Helm docs #2590

merged 6 commits into from
Jul 17, 2023

Conversation

knylander-grafana
Copy link
Contributor

What this PR does:

Updates Helm chart doc based on feed back from @danstadler-pdx.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]


### Tempo helm chart values
### Tempo Helm chart values

This sample file contains example values for installing Tempo using Helm.

Copy link
Contributor Author

@knylander-grafana knylander-grafana Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zalegrala Question about helm configuration for MinIO. Tempo Helm chart has values that aren't configured for GET Helm chart values. Do we need to copy the MinIO section from the Tempo chart values sample to the GET Helm chart value sample?

The Tempo Helm chart has these values for MinIO (lines 116-130):

minio:
  enabled: true
  mode: standalone
  rootUser: grafana-tempo
  rootPassword: supersecret
  buckets:
    # Default Tempo storage bucket.
    - name: tempo-traces
      policy: none
      purge: false

GET Helm chart values (lines 160-1):

minio:
  enabled: true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link where you see?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I see now you mean in the docs not in the chart. Yes the same logic applies. If users want to use minio, with non-default credentials, they will need to set them in the minio block as well as configured for the s3 storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add this to the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zalegrala updated. Review?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, sorry to barge into the convo like that. I hope it is okay for me to ask a short question regarding the docs here...

I ran helm install with the suggested custom values from the doc. However, multiple pods like the tempo-ingester fail to start up with the logs:

level=info ts=2023-07-13T20:05:32.435682845Z caller=main.go:215 msg="initialising OpenTracing tracer"
level=info ts=2023-07-13T20:05:32.450606561Z caller=main.go:102 msg="Starting Tempo" version="(version=2.1.1, branch=HEAD, revision=4157d7620)"
level=error ts=2023-07-13T20:05:32.452641937Z caller=main.go:105 msg="error running Tempo" err="failed to init module services error initialising module: store: failed to create store unexpected error from ListObjects on tempo-traces: The Access Key Id you provided does not exist in our records."

Am I misunderstanding the docs? From what I see on my cluster, the actual access key and secret key which I can use to log in to MinIO are base64 encoded in the secret tempo-minio and are not equal to the strings grafana-tempo and supersecret specified in storage.s3.access_key and storage.s3.secret_key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ViviLearns2Code I think there was a mistake in the chart dependency version. grafana/helm-charts#2509 should correct the dependency version. I'll test with @danstadler-pdx this afternoon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knylander-grafana For the two minio examples, one includes the root credentials and one does not. If we don't include the root credentials as an example, folks might be confused how to change those credentials, but since the values there are default then we don't need to include them. The only required key is the minio.enabled: true and then default values will be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zalegrala The example was updated to include the credentials (see section below

Copy link
Contributor

@zalegrala zalegrala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure we've got everything working for Dan, but these updates look good to me.

Co-authored-by: danstadler-pdx <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants