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

fix(helm): only default bucket names when using minio #12548

Merged
merged 8 commits into from
Apr 9, 2024

Conversation

trevorwhitney
Copy link
Collaborator

@trevorwhitney trevorwhitney commented Apr 9, 2024

What this PR does / why we need it:

Only default bucket names in helm when using minio.

@trevorwhitney trevorwhitney requested a review from a team as a code owner April 9, 2024 21:08
Comment on lines -314 to -317
bucketNames:
chunks: chunks
ruler: ruler
admin: admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of deleting this, maybe comment it out with FIXME for the names?

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Apr 9, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 9, 2024
@trevorwhitney trevorwhitney enabled auto-merge (squash) April 9, 2024 22:33
@trevorwhitney trevorwhitney merged commit 2e32ec5 into main Apr 9, 2024
14 checks passed
@trevorwhitney trevorwhitney deleted the mo-buckets-mo-problems branch April 9, 2024 22:35
@luong-komorebi
Copy link

@trevorwhitney upgrading from 6.0 to 6.1 and the error below happens.

There is a high chance this PR breaks current installations where the defaults for storage are not defined
We should have some notes in the changelog to warn operators about this

│ Error: template: loki/templates/ruler/statefulset-ruler.yaml:24:12: executing "loki/templates/ruler/statefulset-ruler.yaml" at <include "loki.config.checksum" .>: error calling include: template: loki/templates/_helpers.tpl:978:20: executing "loki.config.checksum" at <include (print .Template.BasePath "/config.yaml") .>: error calling include: template: loki/templates/config.yaml:19:7: executing "loki/templates/config.yaml" at <include "loki.calculatedConfig" .>: error calling include: template: loki/templates/_helpers.tpl:461:24: executing "loki.calculatedConfig" at <tpl .Values.loki.config .>: error calling tpl: error during tpl function execution for "{{- if .Values.enterprise.enabled}}\n{{- tpl .Values.enterprise.config . }}\n{{- else }}\nauth_enabled: {{ .Values.loki.auth_enabled }}\n{{- end }}\n\n{{- with .Values.loki.server }}\nserver:\n  {{- toYaml . | nindent 2}}\n{{- end}}\n\nmemberlist:\n{{- if .Values.loki.memberlistConfig }}\n  {{- toYaml .Values.loki.memberlistConfig | nindent 2 }}\n{{- else }}\n{{- if .Values.loki.extraMemberlistConfig}}\n{{- toYaml .Values.loki.extraMemberlistConfig | nindent 2}}\n{{- end }}\n  join_members:\n    - {{ include \"loki.memberlist\" . }}\n    {{- with .Values.migrate.fromDistributed }}\n    {{- if .enabled }}\n    - {{ .memberlistService }}\n    {{- end }}\n    {{- end }}\n{{- end }}\n\n{{- with .Values.loki.ingester }}\ningester:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- if .Values.loki.commonConfig}}\ncommon:\n{{- toYaml .Values.loki.commonConfig | nindent 2}}\n  storage:\n  {{- include \"loki.commonStorageConfig\" . | nindent 4}}\n{{- end}}\n\n{{- with .Values.loki.limits_config }}\nlimits_config:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\nruntime_config:\n  file: /etc/loki/runtime-config/runtime-config.yaml\n\n{{- with .Values.chunksCache }}\n{{- if .enabled }}\nchunk_store_config:\n  chunk_cache_config:\n    default_validity: {{ .defaultValidity }}\n    background:\n      writeback_goroutines: {{ .writebackParallelism }}\n      writeback_buffer: {{ .writebackBuffer }}\n      writeback_size_limit: {{ .writebackSizeLimit }}\n    memcached:\n      batch_size: {{ .batchSize }}\n      parallelism: {{ .parallelism }}\n    memcached_client:\n      addresses: dnssrvnoa+_memcached-client._tcp.{{ template \"loki.fullname\" $ }}-chunks-cache.{{ $.Release.Namespace }}.svc\n      consistent_hash: true\n      timeout: {{ .timeout }}\n      max_idle_conns: 72\n{{- end }}\n{{- end }}\n\n{{- if .Values.loki.schemaConfig }}\nschema_config:\n{{- toYaml .Values.loki.schemaConfig | nindent 2}}\n{{- end }}\n\n{{- if .Values.loki.useTestSchema }}\nschema_config:\n{{- toYaml .Values.loki.testSchemaConfig | nindent 2}}\n{{- end }}\n\n{{ include \"loki.rulerConfig\" . }}\n\n{{- if or .Values.tableManager.retention_deletes_enabled .Values.tableManager.retention_period }}\ntable_manager:\n  retention_deletes_enabled: {{ .Values.tableManager.retention_deletes_enabled }}\n  retention_period: {{ .Values.tableManager.retention_period }}\n{{- end }}\n\nquery_range:\n  align_queries_with_step: true\n  {{- with .Values.loki.query_range }}\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n  {{- end }}\n  {{- if .Values.resultsCache.enabled }}\n  {{- with .Values.resultsCache }}\n  cache_results: true\n  results_cache:\n    cache:\n      default_validity: {{ .defaultValidity }}\n      background:\n        writeback_goroutines: {{ .writebackParallelism }}\n        writeback_buffer: {{ .writebackBuffer }}\n        writeback_size_limit: {{ .writebackSizeLimit }}\n      memcached_client:\n        consistent_hash: true\n        addresses: dnssrvnoa+_memcached-client._tcp.{{ template \"loki.fullname\" $ }}-results-cache.{{ $.Release.Namespace }}.svc\n        timeout: {{ .timeout }}\n        update_interval: 1m\n  {{- end }}\n  {{- end }}\n\n{{- with .Values.loki.storage_config }}\nstorage_config:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.query_scheduler }}\nquery_scheduler:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.compactor }}\ncompactor:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.analytics }}\nanalytics:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.querier }}\nquerier:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.index_gateway }}\nindex_gateway:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.frontend }}\nfrontend:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.frontend_worker }}\nfrontend_worker:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.distributor }}\ndistributor:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\ntracing:\n  enabled: {{ .Values.loki.tracing.enabled }}\n": template: loki/templates/ruler/statefulset-ruler.yaml:37:6: executing "loki/templates/ruler/statefulset-ruler.yaml" at <include "loki.commonStorageConfig" .>: error calling include: template: loki/templates/_helpers.tpl:228:19: executing "loki.commonStorageConfig" at <$.Values.loki.storage.bucketNames.chunks>: nil pointer evaluating interface {}.chunks

@grafanabot
Copy link
Collaborator

The backport to helm-5.47.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-12548-to-helm-5.47.3 origin/helm-5.47.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 2e32ec52d8766c0a5a75be30585402f1dce52cc5

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-12548-to-helm-5.47.3
# Create the PR body template
PR_BODY=$(gh pr view 12548 --json body --template 'Backport 2e32ec52d8766c0a5a75be30585402f1dce52cc5 from #12548{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "chore: [helm-5.47.3] fix(helm): only default bucket names when using minio" --body-file - --label "size/M" --label "area/helm" --label "type/docs" --label "type/bug" --label "product-approved" --label "backport" --base helm-5.47.3 --milestone helm-5.47.3 --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-12548-to-helm-5.47.3

# Create a pull request where the `base` branch is `helm-5.47.3` and the `compare`/`head` branch is `backport-12548-to-helm-5.47.3`.

# Remove the local backport branch
git switch main
git branch -D backport-12548-to-helm-5.47.3

@trevorwhitney
Copy link
Collaborator Author

@luong-komorebi would you mind sharing a bit of information about your install? I think the goal here is to intentionally break any installation using the default buckets with a cloud provider as you likely don't own those buckets and don't mean to send data there? Was your installation using the defaults, and if so, were you only using it to query recent data?

@Farfaday
Copy link

Hi @trevorwhitney

Commenting out loki.storage.bucketNames in values.yaml currently breaks helm lint, as the keys are not defined anymore but are called 9 times in the _helpers.tpl file.

Helm lint create errors of type nil pointer evaluating interface.

I would propose adding a default value, something like

-  bucket_name: {{ $.Values.loki.storage.bucketNames.ruler }}
+  bucket_name: {{ $.Values.loki.storage.bucketNames.ruler | default "ruler" }}

What do you think? I can open an MR if you think that makes sense.

Thank you!

@luong-komorebi
Copy link

@trevorwhitney sorry Trevor, I was late to this due to being busy.

Yes my installation was pretty much default. However, I did have storage config inside which I define an s3 bucket name

# something like this
  storage_config:
    aws:
      bucketnames: abc-prod-logs
      region: us-west-2
      s3forcepathstyle: true

and I dont configure other things like storage.type or storage.bucketNames

this breaks after I upgraded

Later I do have to configure other things so the app can work again ( cant recall what I configured, but it seems like I needed to add :

        storage = {
          type = "s3"
          bucketNames = {
            chunks = "chunks"
            ruler  = "ruler"
            admin  = "admin"
          }
        }
# and 

      minio = { enabled = false }

) because these are what caused the app to break. I think there are some defaults that the upgrade fails to handle or notice maintainers

@thiagowfx
Copy link

@Farfaday Do you plan to open a PR? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm backport helm-5.47.3 backport-failed product-approved size/M type/bug Somehing is not working as expected 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.

6 participants