From 91b3d0c2912681969ccf92aabeb5cabb6af89cd9 Mon Sep 17 00:00:00 2001 From: Amp Date: Fri, 30 Oct 2026 20:22:56 +0000 Subject: [PATCH 1/2] redis: keep config checksum stable across chart versions The redis Deployments' checksum/redis-config annotation hashed the whole rendered ConfigMap, including its helm.sh/chart and app.kubernetes.io/version labels. With config.enabled, every Sourcegraph upgrade changed the hash and restarted redis-cache and redis-store even when redis.conf was unchanged. Move the redis.conf rendering into a sourcegraph.redis.conf helper that both the ConfigMaps and the checksum use, so the checksum covers only redis.conf. Amp-Thread-ID: https://ampcode.com/threads/T-01a18a84-419f-703b-a77d-635921732235 Co-authored-by: Dinesh Kumar --- charts/sourcegraph/CHANGELOG.md | 1 + charts/sourcegraph/templates/_helpers.tpl | 30 +++++++++++++ .../redis/redis-cache.ConfigMap.yaml | 16 +------ .../redis/redis-cache.Deployment.yaml | 2 +- .../redis/redis-store.ConfigMap.yaml | 16 +------ .../redis/redis-store.Deployment.yaml | 2 +- .../tests/redisConfigMount_test.yaml | 42 +++++++++++++++++++ 7 files changed, 77 insertions(+), 32 deletions(-) diff --git a/charts/sourcegraph/CHANGELOG.md b/charts/sourcegraph/CHANGELOG.md index 4eaa8b46..d9180714 100644 --- a/charts/sourcegraph/CHANGELOG.md +++ b/charts/sourcegraph/CHANGELOG.md @@ -13,6 +13,7 @@ Use `**BREAKING**:` to denote a breaking change - Added `indexedSearch.readinessProbe` (`failureThreshold`, `periodSeconds`, `timeoutSeconds`) to tune the `zoekt-webserver` readiness probe, which was previously hardcoded. Defaults are unchanged (`3`/`5`/`5`). Raise `failureThreshold` on instances with large monorepos, where loading a many-shard repo can stall `/healthz` past `timeoutSeconds` and drop a healthy pod out of the `indexed-search` service. - Added `indexedSearch.livenessProbe` (`failureThreshold`, `initialDelaySeconds`, `periodSeconds`, `timeoutSeconds`) to tune the `zoekt-webserver` liveness probe, which was previously hardcoded. Defaults are unchanged (`10`/`120`/`60`/`5`). Raise `failureThreshold` or `timeoutSeconds` where the index greatly exceeds the container memory limit: page-cache reclaim stalls `/healthz` past `timeoutSeconds`, and consecutive failures restart a container that is still serving. Keep the resulting window wider than zoekt's in-process watchdog (`ZOEKT_WATCHDOG_ERRORS` x `ZOEKT_WATCHDOG_TICK`) so the watchdog remains the first line of defence. - `searcher.storageSize` now accepts any Kubernetes quantity (e.g. `150G`, `1Ti`, `1.5Ti`, `500000M`) when deriving `SEARCHER_CACHE_SIZE_MB` and `SYMBOLS_CACHE_SIZE_MB`. Previously anything other than `Gi` silently rendered both as `0`, which disables cache eviction and lets the cache volume fill up. Unparseable values, or values below `3M`, now fail the render unless `searcher.autoCacheSize` is enabled. The derived size now matches searcher's own auto-sizing arithmetic (45% of the volume in decimal megabytes), so existing `Gi` deployments see a slightly higher value on upgrade, e.g. the default `26Gi` renders `12562` instead of `11700`. +- Fixed `redis-cache` and `redis-store` restarting on every upgrade when `config.enabled` is true. The `checksum/redis-config` pod annotation hashed the whole rendered ConfigMap, including its chart-version labels, so it changed with every release even when `redis.conf` did not. It now hashes only `redis.conf`, so redis restarts only when its configuration changes. The annotation value changes once, so redis restarts one last time on the upgrade that picks up this fix. ## 8.1.0 diff --git a/charts/sourcegraph/templates/_helpers.tpl b/charts/sourcegraph/templates/_helpers.tpl index cf4ca4af..58b7afa5 100644 --- a/charts/sourcegraph/templates/_helpers.tpl +++ b/charts/sourcegraph/templates/_helpers.tpl @@ -368,6 +368,36 @@ Resolution order: {{- end -}} {{- end -}} +{{/* +Render the effective redis.conf for a service: the vendored file plus the +chart-managed directives. +Usage: include "sourcegraph.redis.conf" (list . "redisCache") + +The redis Deployments hash this output for their checksum/redis-config +annotation, so it must depend only on the config, never on release metadata +such as the chart version; otherwise every upgrade restarts redis. +*/}} +{{- define "sourcegraph.redis.conf" -}} +{{- $top := index . 0 -}} +{{- $service := index . 1 -}} +{{- $config := (index $top.Values $service).config -}} +{{- $body := $top.Files.Get (printf "files/%s/redis.conf" (kebabcase $service)) | trimSuffix "\n" -}} +{{- $overrides := list -}} +{{- with include "sourcegraph.redis.maxmemory" (list $top $service) -}} +{{- $overrides = append $overrides (printf "maxmemory %s" .) -}} +{{- end -}} +{{- with $config.maxmemoryPolicy -}} +{{- $overrides = append $overrides (printf "maxmemory-policy %s" .) -}} +{{- end -}} +{{- with $config.additionalConfig -}} +{{- $overrides = append $overrides (tpl . $top | trim) -}} +{{- end -}} +{{- if $overrides -}} +{{- $body = printf "%s\n\n# Chart-managed directives. Redis applies the last occurrence of a directive.\n%s" $body (join "\n" $overrides) -}} +{{- end -}} +{{- $body -}} +{{- end -}} + {{/* Fail the render when a service's extra volumes or volume mounts collide with the chart-managed redis config mount. Without this the collision only surfaces as an diff --git a/charts/sourcegraph/templates/redis/redis-cache.ConfigMap.yaml b/charts/sourcegraph/templates/redis/redis-cache.ConfigMap.yaml index bfecbc7e..fc2b350e 100644 --- a/charts/sourcegraph/templates/redis/redis-cache.ConfigMap.yaml +++ b/charts/sourcegraph/templates/redis/redis-cache.ConfigMap.yaml @@ -3,20 +3,6 @@ {{- fail "You can only define one of 'redisCache.config.existingConfig' and 'redisCache.config.additionalConfig' at a time" }} {{- end }} {{- if not .Values.redisCache.config.existingConfig -}} -{{- $body := .Files.Get "files/redis-cache/redis.conf" | trimSuffix "\n" -}} -{{- $overrides := list -}} -{{- with include "sourcegraph.redis.maxmemory" (list . "redisCache") -}} -{{- $overrides = append $overrides (printf "maxmemory %s" .) -}} -{{- end -}} -{{- with .Values.redisCache.config.maxmemoryPolicy -}} -{{- $overrides = append $overrides (printf "maxmemory-policy %s" .) -}} -{{- end -}} -{{- with .Values.redisCache.config.additionalConfig -}} -{{- $overrides = append $overrides (tpl . $ | trim) -}} -{{- end -}} -{{- if $overrides -}} -{{- $body = printf "%s\n\n# Chart-managed directives. Redis applies the last occurrence of a directive.\n%s" $body (join "\n" $overrides) -}} -{{- end -}} apiVersion: v1 kind: ConfigMap metadata: @@ -32,6 +18,6 @@ metadata: name: {{ .Values.redisCache.name }}-conf data: redis.conf: | - {{- $body | nindent 4 }} + {{- include "sourcegraph.redis.conf" (list . "redisCache") | nindent 4 }} {{- end }} {{- end }} diff --git a/charts/sourcegraph/templates/redis/redis-cache.Deployment.yaml b/charts/sourcegraph/templates/redis/redis-cache.Deployment.yaml index 1ea57e11..8492a962 100644 --- a/charts/sourcegraph/templates/redis/redis-cache.Deployment.yaml +++ b/charts/sourcegraph/templates/redis/redis-cache.Deployment.yaml @@ -30,7 +30,7 @@ spec: annotations: kubectl.kubernetes.io/default-container: redis-cache {{- if and .Values.redisCache.config.enabled (not .Values.redisCache.config.existingConfig) }} - checksum/redis-config: {{ include (print $.Template.BasePath "/redis/redis-cache.ConfigMap.yaml") . | sha256sum }} + checksum/redis-config: {{ include "sourcegraph.redis.conf" (list . "redisCache") | sha256sum }} {{- end }} {{- if .Values.sourcegraph.podAnnotations }} {{- toYaml .Values.sourcegraph.podAnnotations | nindent 8 }} diff --git a/charts/sourcegraph/templates/redis/redis-store.ConfigMap.yaml b/charts/sourcegraph/templates/redis/redis-store.ConfigMap.yaml index 6cf4246c..dca95755 100644 --- a/charts/sourcegraph/templates/redis/redis-store.ConfigMap.yaml +++ b/charts/sourcegraph/templates/redis/redis-store.ConfigMap.yaml @@ -3,20 +3,6 @@ {{- fail "You can only define one of 'redisStore.config.existingConfig' and 'redisStore.config.additionalConfig' at a time" }} {{- end }} {{- if not .Values.redisStore.config.existingConfig -}} -{{- $body := .Files.Get "files/redis-store/redis.conf" | trimSuffix "\n" -}} -{{- $overrides := list -}} -{{- with include "sourcegraph.redis.maxmemory" (list . "redisStore") -}} -{{- $overrides = append $overrides (printf "maxmemory %s" .) -}} -{{- end -}} -{{- with .Values.redisStore.config.maxmemoryPolicy -}} -{{- $overrides = append $overrides (printf "maxmemory-policy %s" .) -}} -{{- end -}} -{{- with .Values.redisStore.config.additionalConfig -}} -{{- $overrides = append $overrides (tpl . $ | trim) -}} -{{- end -}} -{{- if $overrides -}} -{{- $body = printf "%s\n\n# Chart-managed directives. Redis applies the last occurrence of a directive.\n%s" $body (join "\n" $overrides) -}} -{{- end -}} apiVersion: v1 kind: ConfigMap metadata: @@ -32,6 +18,6 @@ metadata: name: {{ .Values.redisStore.name }}-conf data: redis.conf: | - {{- $body | nindent 4 }} + {{- include "sourcegraph.redis.conf" (list . "redisStore") | nindent 4 }} {{- end }} {{- end }} diff --git a/charts/sourcegraph/templates/redis/redis-store.Deployment.yaml b/charts/sourcegraph/templates/redis/redis-store.Deployment.yaml index e557c4eb..c1a726a0 100644 --- a/charts/sourcegraph/templates/redis/redis-store.Deployment.yaml +++ b/charts/sourcegraph/templates/redis/redis-store.Deployment.yaml @@ -30,7 +30,7 @@ spec: annotations: kubectl.kubernetes.io/default-container: redis-store {{- if and .Values.redisStore.config.enabled (not .Values.redisStore.config.existingConfig) }} - checksum/redis-config: {{ include (print $.Template.BasePath "/redis/redis-store.ConfigMap.yaml") . | sha256sum }} + checksum/redis-config: {{ include "sourcegraph.redis.conf" (list . "redisStore") | sha256sum }} {{- end }} {{- if .Values.sourcegraph.podAnnotations }} {{- toYaml .Values.sourcegraph.podAnnotations | nindent 8 }} diff --git a/charts/sourcegraph/tests/redisConfigMount_test.yaml b/charts/sourcegraph/tests/redisConfigMount_test.yaml index 78dab75c..e9b421ee 100644 --- a/charts/sourcegraph/tests/redisConfigMount_test.yaml +++ b/charts/sourcegraph/tests/redisConfigMount_test.yaml @@ -253,3 +253,45 @@ tests: path: spec.template.spec.volumes count: 2 template: redis/redis-store.Deployment.yaml +# The checksum must hash only redis.conf. Hashing the rendered ConfigMap pulls in +# its chart-version labels and restarts redis on every upgrade. +- it: should not change the config checksum when only the chart version changes + template: redis/redis-cache.Deployment.yaml + chart: + version: 8.0.0 + appVersion: 8.0.0 + set: + redisCache.config: + enabled: true + maxmemory: 1gb + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + # sha256 of the vendored redis-cache redis.conf plus "maxmemory 1gb" + value: &redisCacheChecksum 57f261f5131adbefd08fe4181b50b898b68f386949f8bbc10f747da37e178f74 +- it: should keep the same config checksum on another chart version + template: redis/redis-cache.Deployment.yaml + chart: + version: 9.9.9 + appVersion: 9.9.9 + set: + redisCache.config: + enabled: true + maxmemory: 1gb + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *redisCacheChecksum +- it: should change the config checksum when redis.conf changes + template: redis/redis-cache.Deployment.yaml + chart: + version: 9.9.9 + appVersion: 9.9.9 + set: + redisCache.config: + enabled: true + maxmemory: 2gb + asserts: + - notEqual: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *redisCacheChecksum From 047f3b00ea46f3716738ecd78a509356d6237f4f Mon Sep 17 00:00:00 2001 From: Dinesh Kumar <5300055+devdinu@users.noreply.github.com> Date: Fri, 30 Oct 2026 13:37:49 -0700 Subject: [PATCH 2/2] redis: cover both services in the config checksum tests The checksum tests pinned a hash for redis-cache only, so a regression confined to redis-store would not have failed them. The cases also sat in redisConfigMount_test.yaml, which covers the volume mount rather than the annotation. Move them into a redisConfigChecksum_test.yaml suite, pin a hash for both services, and assert the hash ignores unrelated input (global labels, image tag) while still changing for maxmemory, maxmemoryPolicy and additionalConfig. The helper now fails the render when a vendored redis.conf is missing instead of silently hashing an empty body. --- charts/sourcegraph/templates/_helpers.tpl | 6 +- .../tests/redisConfigChecksum_test.yaml | 165 ++++++++++++++++++ .../tests/redisConfigMount_test.yaml | 42 ----- 3 files changed, 170 insertions(+), 43 deletions(-) create mode 100644 charts/sourcegraph/tests/redisConfigChecksum_test.yaml diff --git a/charts/sourcegraph/templates/_helpers.tpl b/charts/sourcegraph/templates/_helpers.tpl index 58b7afa5..d12f414a 100644 --- a/charts/sourcegraph/templates/_helpers.tpl +++ b/charts/sourcegraph/templates/_helpers.tpl @@ -381,7 +381,11 @@ such as the chart version; otherwise every upgrade restarts redis. {{- $top := index . 0 -}} {{- $service := index . 1 -}} {{- $config := (index $top.Values $service).config -}} -{{- $body := $top.Files.Get (printf "files/%s/redis.conf" (kebabcase $service)) | trimSuffix "\n" -}} +{{- $dir := kebabcase $service -}} +{{- $body := $top.Files.Get (printf "files/%s/redis.conf" $dir) | trimSuffix "\n" -}} +{{- if not $body -}} +{{- fail (printf "no vendored redis.conf found at files/%s/redis.conf for service %s" $dir $service) -}} +{{- end -}} {{- $overrides := list -}} {{- with include "sourcegraph.redis.maxmemory" (list $top $service) -}} {{- $overrides = append $overrides (printf "maxmemory %s" .) -}} diff --git a/charts/sourcegraph/tests/redisConfigChecksum_test.yaml b/charts/sourcegraph/tests/redisConfigChecksum_test.yaml new file mode 100644 index 00000000..a0b13152 --- /dev/null +++ b/charts/sourcegraph/tests/redisConfigChecksum_test.yaml @@ -0,0 +1,165 @@ +suite: redis config checksum +templates: + - templates/redis/redis-store.Deployment.yaml + - templates/redis/redis-cache.Deployment.yaml +tests: + # The checksum must cover the rendered redis.conf body and nothing else. + # Hashing the whole ConfigMap manifest pulls in the helm.sh/chart label, so + # every chart build changes it and restarts redis even when the config is + # byte-identical. These pin the hash for a fixed config, so any change that + # lets unrelated input back in fails here. + # + # Editing a vendored files/redis-*/redis.conf changes these hashes on purpose. + # Re-pin by rendering the ConfigMap for the same values and hashing its + # redis.conf body, e.g. + # helm template charts/sourcegraph --set redisStore.config.enabled=true \ + # --set redisStore.resources.limits.memory=7Gi \ + # -s templates/redis/redis-store.ConfigMap.yaml + - it: pins the redis-store checksum for a fixed config + templates: + - templates/redis/redis-store.Deployment.yaml + set: + redisStore.config.enabled: true + redisStore.resources.limits.memory: 7Gi + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: &pinnedStoreChecksum f65e9a885e0fd9df5e9f4c3dc6dbcf5b05f11b997d375f5272f7a3c9397f65e5 + + - it: pins the redis-cache checksum for a fixed config + templates: + - templates/redis/redis-cache.Deployment.yaml + set: + redisCache.config.enabled: true + redisCache.resources.limits.memory: 10Gi + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: 0a0f0bb343c5d37ed52bcebf58fd3dbe10f54423c9d920aa90ac8504ee108877 + + # The chart version is the specific input that caused the fleet-wide restarts: + # it is bumped on every release and reached the hash via the ConfigMap labels. + - it: does not change when only the chart version changes + templates: + - templates/redis/redis-cache.Deployment.yaml + chart: + version: 8.0.0 + appVersion: 8.0.0 + set: + redisCache.config: + enabled: true + maxmemory: 1gb + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: &pinnedCacheChecksum 57f261f5131adbefd08fe4181b50b898b68f386949f8bbc10f747da37e178f74 + + - it: keeps the same checksum on a later chart version + templates: + - templates/redis/redis-cache.Deployment.yaml + chart: + version: 9.9.9 + appVersion: 9.9.9 + set: + redisCache.config: + enabled: true + maxmemory: 1gb + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *pinnedCacheChecksum + + - it: changes when an explicit maxmemory changes + templates: + - templates/redis/redis-cache.Deployment.yaml + chart: + version: 9.9.9 + appVersion: 9.9.9 + set: + redisCache.config: + enabled: true + maxmemory: 2gb + asserts: + - notEqual: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *pinnedCacheChecksum + + # Unrelated input must not reach the hash. These are the regressions that + # caused redis to roll on every chart build. + - it: does not change when an unrelated global label changes + templates: + - templates/redis/redis-store.Deployment.yaml + set: + redisStore.config.enabled: true + redisStore.resources.limits.memory: 7Gi + sourcegraph.labels.team: platform + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *pinnedStoreChecksum + + - it: does not change when the sourcegraph image tag changes + templates: + - templates/redis/redis-store.Deployment.yaml + set: + redisStore.config.enabled: true + redisStore.resources.limits.memory: 7Gi + sourcegraph.image.tag: some-new-build + asserts: + - equal: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *pinnedStoreChecksum + + # Real config changes must still roll redis, otherwise the annotation is useless. + - it: changes when the memory limit changes the derived maxmemory + templates: + - templates/redis/redis-store.Deployment.yaml + set: + redisStore.config.enabled: true + redisStore.resources.limits.memory: 14Gi + asserts: + - notEqual: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *pinnedStoreChecksum + + - it: changes when maxmemoryPolicy is set + templates: + - templates/redis/redis-store.Deployment.yaml + set: + redisStore.config.enabled: true + redisStore.resources.limits.memory: 7Gi + redisStore.config.maxmemoryPolicy: allkeys-lru + asserts: + - notEqual: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *pinnedStoreChecksum + + - it: changes when additionalConfig is set + templates: + - templates/redis/redis-store.Deployment.yaml + set: + redisStore.config.enabled: true + redisStore.resources.limits.memory: 7Gi + redisStore.config.additionalConfig: io-threads 4 + asserts: + - notEqual: + path: spec.template.metadata.annotations["checksum/redis-config"] + value: *pinnedStoreChecksum + + - it: is absent when the chart-managed config is disabled + set: + redisStore.config.enabled: false + redisCache.config.enabled: false + asserts: + - notExists: + path: spec.template.metadata.annotations["checksum/redis-config"] + + - it: is absent when an existingConfig is supplied + set: + redisStore.config.enabled: true + redisCache.config.enabled: true + redisStore.config.existingConfig: my-own-redis-conf + redisCache.config.existingConfig: my-own-redis-conf + asserts: + - notExists: + path: spec.template.metadata.annotations["checksum/redis-config"] diff --git a/charts/sourcegraph/tests/redisConfigMount_test.yaml b/charts/sourcegraph/tests/redisConfigMount_test.yaml index e9b421ee..78dab75c 100644 --- a/charts/sourcegraph/tests/redisConfigMount_test.yaml +++ b/charts/sourcegraph/tests/redisConfigMount_test.yaml @@ -253,45 +253,3 @@ tests: path: spec.template.spec.volumes count: 2 template: redis/redis-store.Deployment.yaml -# The checksum must hash only redis.conf. Hashing the rendered ConfigMap pulls in -# its chart-version labels and restarts redis on every upgrade. -- it: should not change the config checksum when only the chart version changes - template: redis/redis-cache.Deployment.yaml - chart: - version: 8.0.0 - appVersion: 8.0.0 - set: - redisCache.config: - enabled: true - maxmemory: 1gb - asserts: - - equal: - path: spec.template.metadata.annotations["checksum/redis-config"] - # sha256 of the vendored redis-cache redis.conf plus "maxmemory 1gb" - value: &redisCacheChecksum 57f261f5131adbefd08fe4181b50b898b68f386949f8bbc10f747da37e178f74 -- it: should keep the same config checksum on another chart version - template: redis/redis-cache.Deployment.yaml - chart: - version: 9.9.9 - appVersion: 9.9.9 - set: - redisCache.config: - enabled: true - maxmemory: 1gb - asserts: - - equal: - path: spec.template.metadata.annotations["checksum/redis-config"] - value: *redisCacheChecksum -- it: should change the config checksum when redis.conf changes - template: redis/redis-cache.Deployment.yaml - chart: - version: 9.9.9 - appVersion: 9.9.9 - set: - redisCache.config: - enabled: true - maxmemory: 2gb - asserts: - - notEqual: - path: spec.template.metadata.annotations["checksum/redis-config"] - value: *redisCacheChecksum