Skip to content

Hash only the ConfigMap data in the checksum/* pod annotations - #2079

Open
DrFaust92 wants to merge 1 commit into
Altinity:masterfrom
DrFaust92:fix/checksum-chart-churn
Open

DrFaust92 wants to merge 1 commit into
Altinity:masterfrom
DrFaust92:fix/checksum-chart-churn

Conversation

@DrFaust92

Copy link
Copy Markdown

The checksum/* pod annotations on the operator Deployment hash the entire rendered ConfigMap, and the manifest's metadata.labels include helm.sh/chart. That value changes on every chart version bump, so the operator pod is restarted on every helm upgrade even when none of the config files changed.

This adds an altinity-clickhouse-operator.configMapContentHash helper that hashes only the data/binaryData sections, emits it from dev/generate_helm_chart.sh, and regenerates Deployment-clickhouse-operator.yaml — all nine annotations (files, confd-files, configd-files, templatesd-files, usersd-files and the four keeper-* ones).

Running the generator before and after the change produces a diff limited to those nine lines. Rendered against a bumped chart version the checksums are unchanged, and adding a file to configs.configdFiles still changes the matching checksum. helm lint passes.

No chart version bump here, since the chart version tracks the operator release.

The same fix was recently made in the argo-cd chart (argoproj/argo-helm#4044); the loki chart uses the same data-only hashing.

The annotations hashed the entire rendered ConfigMap, whose metadata.labels
include helm.sh/chart. That value changes on every chart version bump, so the
operator was restarted on every upgrade even when no configuration had changed.

Add an altinity-clickhouse-operator.configMapContentHash helper that hashes
only the data, emit it from dev/generate_helm_chart.sh, and regenerate the
deployment template.

Signed-off-by: Ilia Lazebnik <ilia.lazebnik@gmail.com>
@sunsingerus

Copy link
Copy Markdown
Collaborator

Thanks for this — the diagnosis is right, the helper is well built, and updating dev/generate_helm_chart.sh alongside the generated template is exactly the convention here. I reproduced your result: at chart 0.27.4 → 0.27.5 with identical config, all 9 checksums move today and none move with your patch. Regenerating with your generator change reproduces the committed Deployment-clickhouse-operator.yaml byte-for-byte, and helm lint / helm template / dev/test_helm_chart.sh (21/21) all pass. I also checked the failure mode I was most worried about — a silent fromYaml failure freezing the hash — and it cannot ship: malformed YAML aborts the whole render before any checksum is computed, and a bad template path hard-fails on include.

Two things before this can land.

1. The pod still restarts — the chart version is also in the pod template labels

templates/generated/Deployment-clickhouse-operator.yaml:25 renders the pod template labels through the full labels helper, which emits helm.sh/chart and app.kubernetes.io/version:

  template:
    metadata:
      labels:
        helm.sh/chart: altinity-clickhouse-operator-0.27.4     # changes on every chart bump
        app.kubernetes.io/name: altinity-clickhouse-operator
        app.kubernetes.io/instance: t
        app.kubernetes.io/version: "0.27.4"                    # changes on every appVersion bump
        app.kubernetes.io/managed-by: Helm

Those live inside spec.template.metadata.labels, so they feed pod-template-hash and roll the Deployment on their own. Rendering 0.27.4 vs 0.27.5 with your patch applied and the image tag pinned, the 9 checksums are now identical — but the render still differs by 4 lines (helm.sh/chart and app.kubernetes.io/version, once in metadata.labels and once in the pod template), and the operator still restarts. So the PR as it stands is necessary but not sufficient for its stated goal.

Note dev/generate_helm_chart.sh:53-57 sets .version and .appVersion from the same release file, so they always move together. With an unpinned tag the image tag changes too and a restart is intended — the churn only ever mattered for users pinning image.tag, and the labels defeat the fix for them as well.

Could you include the label fix here? The line is generator-owned, so it needs the same two-place treatment you already used for the checksums: dev/generate_helm_chart.sh:208 and the regenerated template.

Two shapes, and I'd suggest the second:

  • selectorLabels + podLabels — simplest, but it also drops app.kubernetes.io/managed-by and any user-supplied commonLabels from the pods. commonLabels is a documented values knob that users reasonably expect on pods, so this trades one regression for another.
  • A dedicated helper — say altinity-clickhouse-operator.podTemplateLabels — that is the labels helper minus helm.sh/chart and app.kubernetes.io/version. That removes exactly the two churning labels and keeps managed-by and commonLabels.

Either is safe with respect to the selector: spec.selector.matchLabels (line 22) uses selectorLabels only, and both shapes keep those.

2. Please add a regression guard to dev/test_helm_chart.sh

That script has no checksum coverage at all today, so it passes equally before and after your change — and it would not catch a revert to whole-manifest hashing, nor the nastier failure where the hash freezes and the operator silently stops restarting on real config changes. Since the operator reads its config once at startup and has no file watcher, a missed restart strands stale configd/usersd/templatesd config, so this behaviour is worth pinning.

The assertion is the contract itself: render at two chart versions, require the 9 checksum/* annotations to be equal, and require that a real data change still moves the matching one. Chart version is not settable via --set, so it needs a temp copy of the chart with Chart.yaml edited — roughly:

# checksum/* must be stable across a chart version bump ...
tmp=$(mktemp -d); cp -r "${chart_dir}" "${tmp}/c"
a=$(helm template t "${tmp}/c" | grep -E '^\s+checksum/')
yq e -i '.version = "99.99.99"' "${tmp}/c/Chart.yaml"
b=$(helm template t "${tmp}/c" | grep -E '^\s+checksum/')
[ "${a}" = "${b}" ] || fail "checksum/* changed on a chart version bump"

# ... but must still move when the mounted data changes
c=$(helm template t "${chart_dir}" --set 'watchNamespaces={ns1,ns2}' | grep -E '^\s+checksum/files:')
[ "${c}" != "$(printf '%s' "${a}" | grep 'checksum/files:')" ] || fail "checksum/files did not track a data change"

Smaller points, no action needed unless you want to

  • Three annotations now collapse to the same value (4ca13a59…) because the confd, keeper-confd and keeper-usersd ConfigMaps render with no data keys. Correct, but it looks odd at a glance.
  • fromYaml silently decodes only the first document of a multi-document file and does not report an error. None of the nine generated ConfigMaps is multi-document, so this is latent rather than a live bug, but the raw-include form did cover it — worth a line in the helper comment.
  • Worth a release-note line once the label fix is in: adopting this recomputes all nine annotations once, so the operator pod restarts a single time on the upgrade that lands it. ClickHouse clusters are unaffected — only reconciliation pauses.

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.

2 participants