Skip to content

helm: update chart for v2.0 breaking changes#5076

Merged
simonswine merged 8 commits intografana:mainfrom
simonswine:20260417_pyroscope-v2-breaking-fixes
Apr 17, 2026
Merged

helm: update chart for v2.0 breaking changes#5076
simonswine merged 8 commits intografana:mainfrom
simonswine:20260417_pyroscope-v2-breaking-fixes

Conversation

@simonswine
Copy link
Copy Markdown
Contributor

@simonswine simonswine commented Apr 17, 2026

Summary

Aligns the Helm chart with the binary breaking changes from 3a68604 (Pyroscope OSS v2.0).

  • Fix write-path for v1 deployments: Binary default changed from ingester to segment-writer; without explicit -write-path=ingester all v1 write traffic would be routed to the segment-writer and fail. Applied to all components (not just distributor) since the validation runs at startup for every component.
  • Replace removed flags with -architecture.storage: all.enable-v1-write-path and all.enable-v1-read-path were deleted; replaced by -architecture.storage=v1|v2|v1-v2-dual.
  • Remove PYROSCOPE_V2_EXPERIMENT env var: No longer read by the binary.
  • Remove explicit -storage.backend=filesystem: Now the binary default.
  • Remove explicit -enable-query-backend=true: Now defaults to true.
  • Remove /data-shared volume mount: The new default filesystem dir ./data/v2/shared falls naturally under the existing /data PVC mount.
  • Clean up values.yaml: Remove migration.queryBackend (flag no longer exists) and persistence.shared stanza (mount no longer used).

Test plan

  • make helm/check passes (all 6 rendered variants validate with kubeconform)
  • v1 single-binary: verify -architecture.storage=v1 and -write-path=ingester are set
  • v2 single-binary: verify -architecture.storage=v2 is set, no v1 flags
  • v1-v2-dual: verify -architecture.storage=v1-v2-dual and -write-path=combined are set

Note

Medium Risk
Changes runtime flags and storage/write-path behavior across all deployed components; misconfiguration could break ingestion/querying during upgrades despite being mostly Helm-level changes.

Overview
Updates the Pyroscope Helm chart to match v2.0 binary flag/behavior changes by switching components to the new -architecture.storage flag and explicitly setting -write-path (including ingester for v1-only installs and combined for dual-mode).

Bumps chart appVersion/rendered manifests from 1.21.0 to main-171db75, removes deprecated v2 experiment/env and legacy shared data mount expectations, and adjusts documented values (eg. architecture.storage.migration.queryBackendFrom) to reflect the new migration semantics.

Reviewed by Cursor Bugbot for commit 1243659. Bugbot is set up for automated code reviews on this repo. Configure here.

@simonswine simonswine requested a review from a team as a code owner April 17, 2026 13:19
@simonswine simonswine force-pushed the 20260417_pyroscope-v2-breaking-fixes branch from 0ebacd2 to fd4d206 Compare April 17, 2026 13:22
Comment thread operations/pyroscope/helm/pyroscope/values.yaml
Copy link
Copy Markdown
Contributor

@aleks-p aleks-p left a comment

Choose a reason for hiding this comment

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

Left one nit, also the Cursor find seems relevant. Looks good otherwise, I haven't actually tried it.

Comment thread operations/pyroscope/helm/pyroscope/templates/deployments-statefulsets.yaml Outdated
Copy link
Copy Markdown
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

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

LGTM

- "-metastore.address=kubernetes:///pyroscope-dev-metastore-headless.$(NAMESPACE_FQDN):9095"
- "-enable-query-backend=true"
- "-architecture.storage=v2"
- "-enable-query-backend-from=auto"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this only here for dual mode, isn't it?

Copy link
Copy Markdown
Contributor Author

@simonswine simonswine Apr 17, 2026

Choose a reason for hiding this comment

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

Yeah trued and it is now also the default, so I can remove it

@simonswine
Copy link
Copy Markdown
Contributor Author

Helm ci will fail until there is a tagged v2.0.0

Comment thread operations/pyroscope/helm/pyroscope/Chart.yaml Outdated
@simonswine simonswine force-pushed the 20260417_pyroscope-v2-breaking-fixes branch from ea23a44 to e56c350 Compare April 17, 2026 15:24
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.

Reviewed by Cursor Bugbot for commit e56c350. Configure here.

@simonswine simonswine force-pushed the 20260417_pyroscope-v2-breaking-fixes branch from e56c350 to 3dc8ba5 Compare April 17, 2026 16:23
Align the Helm chart with the binary changes from 3a68604:

- Replace removed `all.enable-v1-write-path`/`all.enable-v1-read-path`
  flags with the new `-architecture.storage=v1|v2|v1-v2-dual` flag
- Add explicit `-write-path=ingester` for v1-only mode; the binary
  default changed from `ingester` to `segment-writer` so without this
  all v1 writes would be routed to the segment-writer and fail
- Remove `PYROSCOPE_V2_EXPERIMENT` env var, which was removed from the
  binary and no longer has any effect
- Remove explicit `-storage.backend=filesystem`; filesystem is now the
  default storage backend
- Remove explicit `-enable-query-backend=true`; it now defaults to true
- Remove `/data-shared` volume mount; the new default filesystem dir
  `./data/v2/shared` falls naturally under the existing `/data` PVC mount
- Remove `migration.queryBackend` value (rendered flag no longer exists)
- Remove `persistence.shared` value stanza (mount no longer used)
The write-path/architecture.storage validation runs at startup for every
component, not only the distributor. Without an explicit flag all non-v2
pods would fail to start because the binary default changed to
segment-writer.
…uto values

The flag is only meaningful during migration when both v1 and v2 storage
are active. Skip it for pure v2 deployments, and skip it when the value
is "auto" since that is already the binary default.
The queryBackend boolean was removed from values.yaml; the query backend
is always enabled when v2 storage is active, so remove the dead branch.
The write-path.segment-writer-weight flag was hardcoded to 1, silently
ignoring the architecture.storage.migration.segmentWriterWeight value
from values.yaml.
Same issue as segmentWriterWeight: the ingesterWeight value from
values.yaml was never forwarded to the binary in combined mode.
Point CI chart installation at the snapshot image built from commit
3a68604 which introduced the -architecture.storage flag.
The v prefix was hardcoded, producing vmain-171db75 for snapshot
appVersions. Only prepend v when appVersion starts with a digit
(i.e. is a semver tag).
@simonswine simonswine force-pushed the 20260417_pyroscope-v2-breaking-fixes branch from 3dc8ba5 to 1243659 Compare April 17, 2026 16:32
@simonswine simonswine merged commit b2127ab into grafana:main Apr 17, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants