Skip to content

feat: roll HyperDX pods on config changes - #273

Open
bsosnader wants to merge 6 commits into
ClickHouse:mainfrom
bsosnader:brsosnad/investigate-values-only-checksums
Open

bsosnader wants to merge 6 commits into
ClickHouse:mainfrom
bsosnader:brsosnad/investigate-values-only-checksums

Conversation

@bsosnader

@bsosnader bsosnader commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • add chart-owned pod-template checksums for the rendered HyperDX ConfigMap and optional Secret
  • preserve existing user annotation merge precedence while reserving scoped checksum keys
  • document automatic rollout behavior and external Secret limitations
  • add focused Helm unit coverage for deterministic, content-sensitive checksums

Motivation

HyperDX consumes clickstack-config and clickstack-secret through envFrom. Kubernetes resolves those environment variables when a pod starts, so changing only the ConfigMap or Secret during helm upgrade leaves existing HyperDX pods with stale startup configuration unless the Deployment pod template also changes.

This follows Helm's documented include ... | sha256sum rollout pattern. The ConfigMap and Secret manifests now share canonical named renderers with the Deployment checksums, ensuring the hashed content cannot drift from the resources Helm applies. Git enforces LF for ClickStack Helm template source, so the standard include ... | sha256sum pattern produces identical annotations across checkout platforms without runtime normalization.

No new values API is needed: these are chart-managed resources with deterministic rollout behavior. Arbitrary template paths or tpl evaluation in user values would be less safe and would not improve this owned-resource case.

Backward compatibility

Existing hyperdx.deployment.annotations and hyperdx.deployment.podAnnotations values remain merged with the same precedence. The generated checksum/clickstack-config and checksum/clickstack-secret keys are chart-reserved and override caller values so stale hashes cannot disable rollouts.

checksum/clickstack-secret is omitted when hyperdx.secrets: null, matching Secret rendering and envFrom behavior. Externally managed Secret changes still require an explicit rollout because Helm cannot hash resources it does not render.

The first upgrade containing this change intentionally performs a one-time HyperDX rollout because the generated annotations are added to the pod template. The checksum covers the full rendered chart-managed resource, including standard chart labels, so a chart-version-only upgrade also intentionally rolls HyperDX; this matches Helm's documented rendered-template checksum pattern.

Tests

Verified with the repository-pinned helm-unittest plugin v1.0.3:

  • helm unittest --strict -f tests/hyperdx-rollout-checksums_test.yaml charts/clickstack — 6 tests passed
  • helm unittest charts/clickstack — 31 suites, 257 tests passed
  • repeated identical helm template renders produce identical checksum annotations
  • config-only and secret-only changes alter only their corresponding checksum; unrelated Deployment values leave both stable
  • helm lint --strict charts/clickstack
  • default chart render
  • ALB ingress, API-only, and OTEL custom-config example renders
  • git diff --check

@bsosnader
bsosnader requested a review from a team as a code owner August 31, 2026 18:08
@changeset-bot

changeset-bot Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1bb7efe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
helm-charts Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added the external Opened by an external contributor label Aug 31, 2026
@CLAassistant

CLAassistant commented Aug 31, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bsosnader
bsosnader force-pushed the brsosnad/investigate-values-only-checksums branch from f7cf6b3 to fa92f63 Compare August 31, 2026 18:13
@wrn14897

wrn14897 commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Please fix the P1 checksum-test failure before merging. The CI unit-test job reports 5 failed, 252 passed, with all five failures in hyperdx-rollout-checksums_test.yaml.

The hard-coded expected hashes differ from CI's rendered hashes (e.g. baseline config expects 0587572216112d61fd7d298001d71ed852194038be79415116a447e260753058 but renders b76d0e7c9c3bc42636d77be4768074ac0627ebf8af4c5f75caa5ce697e7e04fe). Please investigate the mismatch using CI's pinned helm-unittest v1.0.3, make the fixtures reproducible by pinning the relevant rendering context, and rerun the full suite. Please also update the PR's test summary to reflect the verified result.

Rest of the change looks good: annotation merge precedence is preserved, the Secret checksum is correctly guarded on hyperdx.secrets: null, and both integration suites plus all three example renders pass. Two non-blocking notes:

  • The checksums include chart labels, so a chart-version-only upgrade will also roll HyperDX even when the app image and env config are unchanged. Worth confirming that is intended.
  • The deep-review job failure is unrelated to this change — actions/checkout@v6 refuses to check out fork code under pull_request_target.

bsosnader and others added 3 commits September 9, 2026 11:50
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bsosnader

Copy link
Copy Markdown
Contributor Author

@wrn14897 thanks for the review. pushed fixes and ran full validation, updated PR description.

Chart-version changes intentionally trigger a rollout, following Helm’s checksum convention

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This change is a well-scoped, low-risk refactor plus additive rollout behavior. The HyperDX ConfigMap and Secret rendering are extracted into shared named templates (clickstack.hyperdx.configmap, clickstack.hyperdx.secret) and the Deployment hashes those same templates into pod-template annotations. Static analysis confirms the refactor is drift-free: the checksum is a deterministic function of the rendered content (Helm sorts map keys), the manifest output is byte-identical to the pre-refactor templates modulo a trailing newline that does not affect content-sensitivity, and the Secret checksum is correctly guarded by ne .Values.hyperdx.secrets nil consistent with secret.yaml and the envFrom block. The reserved-key override (set $podAnnotations) and annotation merge precedence are preserved and covered by tests.

No secrets are exposed by the diff, and no template path fails to render under static inspection.

🟡 P2 -- recommended

  • charts/clickstack/tests/hyperdx-rollout-checksums_test.yaml:20 -- The suite asserts exact sha256 digests, so any future change to a default entry in hyperdx.config/hyperdx.secrets or to clickstack.labels silently breaks these assertions, and there is no in-repo note on how to regenerate the digests; this is the same failure class that previously broke CI.
    • Fix: Add a short comment in the test header documenting the exact command used to regenerate the pinned digests (pinned helm-unittest version, chart-metadata overrides, release name/namespace) so maintainers can refresh them deterministically.
  • charts/clickstack/tests/hyperdx-rollout-checksums_test.yaml:1 -- The pinned digests could not be verified in this environment because helm/helm-unittest are unavailable; given the prior CI failure was an exact-hash mismatch, the digests' correctness rests entirely on the CI unit-test job under pinned helm-unittest v1.0.3.
    • Fix: Confirm the CI Helm Chart Tests job is green on the latest commit before merge; treat that run as the source of truth for the pinned hashes.
🔵 P3 nitpicks (2)
  • charts/clickstack/tests/hyperdx-rollout-checksums_test.yaml:8 -- The suite pins appVersion: 2.36.0 while the shipped Chart.yaml declares appVersion: 2.38.0, so the suite validates label rendering for a version the chart no longer ships; intentional for digest stability but potentially confusing to future readers.
    • Fix: Add a one-line comment clarifying that the pinned appVersion is arbitrary-but-fixed purely to keep the content-change assertions stable across real version bumps.
  • charts/clickstack/tests/hyperdx-rollout-checksums_test.yaml:16 -- No assertion verifies that the rendered configmap.yaml/secret.yaml manifest content matches the checksummed named-template output; the drift-free property is currently guaranteed only structurally by the shared template.
    • Fix: Optionally add a lightweight assertion (or snapshot) tying the emitted ConfigMap/Secret content to the checksum inputs so the drift-free guarantee is regression-protected, not just structural.

Reviewers (5): correctness, testing, maintainability, project-standards, previous-comments.

Testing gaps:

  • Pinned sha256 digests could not be executed locally (helm/helm-unittest unavailable); rely on the CI unit-test job as the authoritative pass signal.
  • No coverage for the hyperdx.secrets: {} (empty-map) case, which still renders a Secret and a checksum/clickstack-secret annotation.
  • The documented "chart-version-only upgrade also rolls HyperDX" behavior (driven by clickstack.labels embedding helm.sh/chart and app.kubernetes.io/version) is intended and documented in values.yaml, but is not asserted by a test.

No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external Opened by an external contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants