Skip to content

Commit 4a134a2

Browse files
filiphaftekampagent
filiphaftek
and
ampagent
authored
Allow syntactic-code-intel-worker /tmp scratch on a per-pod PVC (#987)
The syntactic-code-intel-worker's `/tmp` scratch volume was always an unbounded `emptyDir`, which places its writes on the node boot disk. This brings it to parity with the precise-code-intel-worker options added in #956 and #958. ## Changes - Add `syntacticCodeIntel.storageType` to select the backing store for `/tmp`: `emptyDir` (default) or `pvc` - `pvc` mounts a generic ephemeral volume, so each pod gets its own claim that is created and deleted with the pod - Add `syntacticCodeIntel.storageSize`, required for `pvc` and applied as `sizeLimit` when left on `emptyDir` - Add `syntacticCodeIntel.storageClassName` for the scratch claim. Empty uses the cluster default class (`reclaimPolicy: Delete`); `storageClass.name` is `Retain` and is deliberately not used - On `pvc`, the pod `fsGroup` defaults to the container `runAsGroup` (with `fsGroupChangePolicy: OnRootMismatch`) so the non-root worker can write to the freshly provisioned volume; a user-set `podSecurityContext.fsGroup` wins The default renders `emptyDir: {}` exactly as before, so existing installs are unaffected. The mount path stays `/tmp`. Example: ```yaml syntacticCodeIntel: enabled: true storageType: pvc storageSize: 50Gi ``` ### Checklist - [x] Follow the [manual testing process](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/TEST.md) - [x] Update [changelog](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/charts/sourcegraph/CHANGELOG.md) - [ ] Update [Kubernetes update doc](https://docs.sourcegraph.com/admin/updates/kubernetes) ### Test plan - New `tests/syntacticCodeIntelStorage_test.yaml` (mirrors `preciseCodeIntelStorage_test.yaml`); its 7 behavior tests fail against the old template and pass with this change - `helm unittest` on all four charts: 237 passed - `helm lint charts/sourcegraph` with `storageType=pvc` passes - `helm template` with `syntacticCodeIntel.enabled=true` and default storage values is byte-identical before and after - `./scripts/helm-docs.sh` leaves no diff - CI Co-authored-by: Amp <amp@ampcode.com>
1 parent a6d7ab7 commit 4a134a2

5 files changed

Lines changed: 201 additions & 1 deletion

File tree

‎charts/sourcegraph/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ Use `**BREAKING**:` to denote a breaking change
88

99
## Unreleased
1010

11+
- Added `syntacticCodeIntel.storageType`, `syntacticCodeIntel.storageSize`, and `syntacticCodeIntel.storageClassName`, so the `syntactic-code-intel-worker` `/tmp` scratch volume can be a per-pod PVC (generic ephemeral volume) instead of an `emptyDir` on the node boot disk, matching `preciseCodeIntel`. The default remains an unbounded `emptyDir`.
1112
- Added an `outbound-http-proxy` example, which routes outbound traffic from frontend, worker, and migrator through an HTTP proxy with `HTTP_PROXY`, `HTTPS_PROXY`, and `NO_PROXY`
1213
- Fixed the `codeintel-db` PersistentVolumeClaim being renamed by `codeIntelDB.name`, which left the StatefulSet pointing at a `codeintel-db` claim that no longer existed and stuck the pod in `Pending`. The PVC is now always named `codeintel-db`, matching the other components and the documented behaviour of `*.name`. If you worked around this by creating a `codeintel-db` PVC by hand, label it `app.kubernetes.io/managed-by: Helm` and annotate it with `meta.helm.sh/release-name` and `meta.helm.sh/release-namespace` before upgrading so Helm can adopt it. [#612](https://github.com/sourcegraph/deploy-sourcegraph-helm/issues/612)
1314
- 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.

‎charts/sourcegraph/README.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,9 @@ In addition to the documented values, all services also support the following va
460460
| syntacticCodeIntel.resources | object | `{"limits":{"cpu":"2","memory":"4G"},"requests":{"cpu":"500m","memory":"2G"}}` | Resource requests & limits for the `syntactic-code-intel-worker` container, learn more from the [Kubernetes documentation](https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/) |
461461
| syntacticCodeIntel.serviceAccount.create | bool | `false` | Enable creation of ServiceAccount for `syntactic-code-intel-worker` |
462462
| syntacticCodeIntel.serviceAccount.name | string | `""` | Name of the ServiceAccount to be created or an existing ServiceAccount |
463+
| syntacticCodeIntel.storageClassName | string | `""` | StorageClass for the `/tmp` scratch PVC when storageType is `pvc`. Empty uses the cluster default class. The claim is per-pod and discarded with the pod, so this class must use `reclaimPolicy: Delete`; `storageClass` is `Retain` for stateful data and is not used here. |
464+
| syntacticCodeIntel.storageSize | string | `""` | Size of the `/tmp` scratch volume. Required when storageType is `pvc`. When storageType is `emptyDir` this sets `sizeLimit` (optional, leave empty for unlimited). |
465+
| syntacticCodeIntel.storageType | string | `"emptyDir"` | Backing store for the `/tmp` scratch volume, used for indexing. One of: emptyDir, pvc. emptyDir: node ephemeral storage, i.e. the node boot disk. pvc: per-pod PVC, discarded with the pod. The pod `fsGroup` defaults to the container `runAsGroup` so the non-root worker can write to the volume; set `podSecurityContext.fsGroup` to override it. |
463466
| syntectServer.containerSecurityContext | object | `{"allowPrivilegeEscalation":false,"readOnlyRootFilesystem":true,"runAsGroup":101,"runAsUser":100}` | Security context for the `syntect-server` container, learn more from the [Kubernetes documentation](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-container) |
464467
| syntectServer.env | object | the chart will add some default environment values | Environment variables for the `syntect-server` container |
465468
| syntectServer.image.defaultTag | string | `<appVersion>@sha256:<digest>`, pinned by each release | Docker image tag for the `syntect-server` image |

‎charts/sourcegraph/templates/syntactic-code-intel/worker.Deployment.yaml‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@ spec:
4646
deploy: sourcegraph
4747
app: syntactic-code-intel-worker
4848
spec:
49+
{{- $storageType := .Values.syntacticCodeIntel.storageType | default "emptyDir" }}
50+
{{- if not (has $storageType (list "emptyDir" "pvc")) }}
51+
{{- fail (printf "syntacticCodeIntel.storageType must be one of emptyDir, pvc; got %q" $storageType) }}
52+
{{- end }}
4953
{{- include "sourcegraph.terminationGracePeriodSeconds" (list . "syntacticCodeIntel") | nindent 6 }}
5054
containers:
5155
- name: syntactic-code-intel-worker
@@ -105,8 +109,13 @@ spec:
105109
{{- if .Values.syntacticCodeIntel.extraContainers }}
106110
{{- toYaml .Values.syntacticCodeIntel.extraContainers | nindent 6 }}
107111
{{- end }}
112+
{{- $podSecurityContext := deepCopy .Values.syntacticCodeIntel.podSecurityContext }}
113+
{{- if eq $storageType "pvc" }}
114+
{{- /* A freshly provisioned volume is root-owned, so the non-root worker needs fsGroup to write /tmp. */}}
115+
{{- $podSecurityContext = merge $podSecurityContext (dict "fsGroup" (.Values.syntacticCodeIntel.containerSecurityContext.runAsGroup | default 101) "fsGroupChangePolicy" "OnRootMismatch") }}
116+
{{- end }}
108117
securityContext:
109-
{{- toYaml .Values.syntacticCodeIntel.podSecurityContext | nindent 8 }}
118+
{{- toYaml $podSecurityContext | nindent 8 }}
110119
{{- include "sourcegraph.nodeSelector" (list . "syntacticCodeIntel" ) | trim | nindent 6 }}
111120
{{- include "sourcegraph.affinity" (list . "syntacticCodeIntel" ) | trim | nindent 6 }}
112121
{{- with include "sourcegraph.priorityClassName" (list . "syntacticCodeIntel" ) | trim }}{{ . | nindent 6 }}{{- end }}
@@ -118,7 +127,24 @@ spec:
118127
{{- include "sourcegraph.renderServiceAccountName" (list . "syntacticCodeIntel") | trim | nindent 6 }}
119128
volumes:
120129
- name: tmpdir
130+
{{- if eq $storageType "pvc" }}
131+
ephemeral:
132+
volumeClaimTemplate:
133+
spec:
134+
accessModes:
135+
- ReadWriteOnce
136+
resources:
137+
requests:
138+
storage: {{ required "syntacticCodeIntel.storageSize is required when syntacticCodeIntel.storageType is pvc" .Values.syntacticCodeIntel.storageSize }}
139+
{{- with .Values.syntacticCodeIntel.storageClassName }}
140+
storageClassName: {{ . }}
141+
{{- end }}
142+
{{- else if .Values.syntacticCodeIntel.storageSize }}
143+
emptyDir:
144+
sizeLimit: {{ .Values.syntacticCodeIntel.storageSize }}
145+
{{- else }}
121146
emptyDir: {}
147+
{{- end }}
122148
{{- if .Values.syntacticCodeIntel.extraVolumes }}
123149
{{- toYaml .Values.syntacticCodeIntel.extraVolumes | nindent 6 }}
124150
{{- end }}

‎charts/sourcegraph/tests/syntacticCodeIntelStorage_test.yaml‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
---
2+
suite: syntacticCodeIntelStorage
3+
templates:
4+
- syntactic-code-intel/worker.Deployment.yaml
5+
set:
6+
syntacticCodeIntel:
7+
enabled: true
8+
tests:
9+
- it: should back tmpdir with a plain emptyDir by default
10+
asserts:
11+
- contains:
12+
path: spec.template.spec.volumes
13+
content:
14+
name: tmpdir
15+
emptyDir: {}
16+
- contains:
17+
path: spec.template.spec.containers[0].volumeMounts
18+
content:
19+
mountPath: /tmp
20+
name: tmpdir
21+
22+
- it: should set sizeLimit when storageSize is set on the emptyDir branch
23+
set:
24+
syntacticCodeIntel:
25+
storageType: emptyDir
26+
storageSize: 20Gi
27+
asserts:
28+
- contains:
29+
path: spec.template.spec.volumes
30+
content:
31+
name: tmpdir
32+
emptyDir:
33+
sizeLimit: 20Gi
34+
35+
- it: should back tmpdir with a per-pod PVC when storageType=pvc
36+
set:
37+
syntacticCodeIntel:
38+
storageType: pvc
39+
storageSize: 50Gi
40+
asserts:
41+
- contains:
42+
path: spec.template.spec.volumes
43+
content:
44+
name: tmpdir
45+
ephemeral:
46+
volumeClaimTemplate:
47+
spec:
48+
accessModes:
49+
- ReadWriteOnce
50+
resources:
51+
requests:
52+
storage: 50Gi
53+
# /tmp mount is unchanged, so no TMPDIR redirect is needed
54+
- contains:
55+
path: spec.template.spec.containers[0].volumeMounts
56+
content:
57+
mountPath: /tmp
58+
name: tmpdir
59+
60+
# The per-pod claim needs a Delete-policy class; storageClass is Retain.
61+
- it: should omit storageClassName by default so the cluster default class is used
62+
set:
63+
syntacticCodeIntel:
64+
storageType: pvc
65+
storageSize: 50Gi
66+
asserts:
67+
- notExists:
68+
path: spec.template.spec.volumes[0].ephemeral.volumeClaimTemplate.spec.storageClassName
69+
70+
- it: should not inherit storageClass.name
71+
set:
72+
storageClass:
73+
name: custom-ssd
74+
syntacticCodeIntel:
75+
storageType: pvc
76+
storageSize: 50Gi
77+
asserts:
78+
- notExists:
79+
path: spec.template.spec.volumes[0].ephemeral.volumeClaimTemplate.spec.storageClassName
80+
81+
- it: should use syntacticCodeIntel.storageClassName when set
82+
set:
83+
storageClass:
84+
name: custom-ssd
85+
syntacticCodeIntel:
86+
storageType: pvc
87+
storageSize: 50Gi
88+
storageClassName: premium-rwo
89+
asserts:
90+
- equal:
91+
path: spec.template.spec.volumes[0].ephemeral.volumeClaimTemplate.spec.storageClassName
92+
value: premium-rwo
93+
94+
- it: should fail when storageSize is omitted on the pvc branch
95+
set:
96+
syntacticCodeIntel:
97+
storageType: pvc
98+
asserts:
99+
- failedTemplate:
100+
errorMessage: syntacticCodeIntel.storageSize is required when syntacticCodeIntel.storageType is pvc
101+
102+
- it: should fall back to emptyDir when storageType is absent
103+
set:
104+
syntacticCodeIntel:
105+
storageType: null
106+
asserts:
107+
- contains:
108+
path: spec.template.spec.volumes
109+
content:
110+
name: tmpdir
111+
emptyDir: {}
112+
113+
- it: should fail on an unknown storageType
114+
set:
115+
syntacticCodeIntel:
116+
storageType: PVC
117+
storageSize: 50Gi
118+
asserts:
119+
- failedTemplate:
120+
errorMessage: 'syntacticCodeIntel.storageType must be one of emptyDir, pvc; got "PVC"'
121+
122+
- it: should leave the pod securityContext untouched on the emptyDir branch
123+
asserts:
124+
- equal:
125+
path: spec.template.spec.securityContext
126+
value: {}
127+
128+
- it: should set fsGroup to the container runAsGroup on the pvc branch so /tmp is writable
129+
set:
130+
syntacticCodeIntel:
131+
storageType: pvc
132+
storageSize: 50Gi
133+
asserts:
134+
- equal:
135+
path: spec.template.spec.securityContext
136+
value:
137+
fsGroup: 101
138+
fsGroupChangePolicy: OnRootMismatch
139+
140+
- it: should keep a user-set fsGroup and other podSecurityContext fields on the pvc branch
141+
set:
142+
syntacticCodeIntel:
143+
storageType: pvc
144+
storageSize: 50Gi
145+
podSecurityContext:
146+
fsGroup: 2000
147+
runAsNonRoot: true
148+
asserts:
149+
- equal:
150+
path: spec.template.spec.securityContext
151+
value:
152+
fsGroup: 2000
153+
fsGroupChangePolicy: OnRootMismatch
154+
runAsNonRoot: true

‎charts/sourcegraph/values.yaml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -965,6 +965,22 @@ syntacticCodeIntel:
965965
create: false
966966
# -- Name of the ServiceAccount to be created or an existing ServiceAccount
967967
name: ""
968+
# -- Backing store for the `/tmp` scratch volume, used for indexing.
969+
# One of: emptyDir, pvc.
970+
# emptyDir: node ephemeral storage, i.e. the node boot disk.
971+
# pvc: per-pod PVC, discarded with the pod. The pod `fsGroup` defaults to the
972+
# container `runAsGroup` so the non-root worker can write to the volume; set
973+
# `podSecurityContext.fsGroup` to override it.
974+
storageType: emptyDir
975+
# -- Size of the `/tmp` scratch volume. Required when storageType is `pvc`.
976+
# When storageType is `emptyDir` this sets `sizeLimit` (optional, leave empty
977+
# for unlimited).
978+
storageSize: ""
979+
# -- StorageClass for the `/tmp` scratch PVC when storageType is `pvc`.
980+
# Empty uses the cluster default class. The claim is per-pod and discarded
981+
# with the pod, so this class must use `reclaimPolicy: Delete`; `storageClass`
982+
# is `Retain` for stateful data and is not used here.
983+
storageClassName: ""
968984

969985
preciseCodeIntel:
970986
# -- Environment variables for the `precise-code-intel-worker` container

0 commit comments

Comments
 (0)