fix(dgraph): correct latent StatefulSet bugs (mount, datadir volume, exec, serviceAccountName) - #148
fix(dgraph): correct latent StatefulSet bugs (mount, datadir volume, exec, serviceAccountName)#148mlwelles wants to merge 1 commit into
Conversation
…exec, serviceAccountName)
| {{- if .Values.serviceAccount.create }} | ||
| serviceAccountName: {{ .Values.serviceAccount.name }} | ||
| automountServiceAccountToken: {{ .Values.zero.automountServiceAccountToken }} | ||
| {{- if or .Values.serviceAccount.create .Values.serviceAccount.name }} |
There was a problem hiding this comment.
Anchoring here because it's the only zero hunk in the diff, but the point is about two blocks further down this file that the PR doesn't touch.
The description reads as four general StatefulSet fixes, but the mount gate and the inert datadir volume land on alpha only. Zero still has both. With zero.persistence.enabled=false:
volumeMounts:
terminationGracePeriodSeconds: 60
Null volumeMounts — the emptyDir gets declared at line 207 and never mounted, so zero writes to the container layer exactly like alpha did. And with persistence on, lines 208-210 still carry the same inert persistentVolumeClaim: claimName: datadir.
exec was correctly scoped, to be clear: zero already has it on both branches at 148 and 150, so alpha really was the only gap there. But the mount gate and the volume block are verbatim duplicates across the two files, and leaving zero on the old shape is how the asymmetry got here in the first place. I'd rather fix both now than have zero's copy rediscovered in six months.
| {{- end }} | ||
| terminationGracePeriodSeconds: {{ .Values.alpha.terminationGracePeriodSeconds }} | ||
| volumes: | ||
| {{- if not .Values.alpha.persistence.enabled }} |
There was a problem hiding this comment.
With the inert entry gone, a stock install (persistence on, no config file, no TLS, no backups) leaves this list empty:
terminationGracePeriodSeconds: 600
volumes:
volumeClaimTemplates:
volumes: null, which Kubernetes accepts, so nothing breaks. But it's the same empty-key artifact you're cleaning up on the volumeMounts side, and it reads as a template bug to whoever next looks at a rendered manifest. Worth guarding the key so it only appears when something populates it.
Separately, worth confirming I reached the same conclusion you did on the removal itself: updateStorage in the StatefulSet controller drops any pod-template volume whose name matches a volumeClaimTemplates entry and substitutes the per-pod PVC, so that block genuinely never took effect. And mounting the emptyDir over /dgraph is safe — /dgraph in dgraph/dgraph:v25.3.1 is an empty directory, so nothing gets hidden.
| {{- if .Values.serviceAccount.create }} | ||
| serviceAccountName: {{ .Values.serviceAccount.name }} | ||
| automountServiceAccountToken: {{ .Values.alpha.automountServiceAccountToken }} | ||
| {{- if or .Values.serviceAccount.create .Values.serviceAccount.name }} |
There was a problem hiding this comment.
This fix is bigger than the description lets on, and it wants a release note. On main, with serviceAccount.create=false and serviceAccount.name=my-sa, neither field renders at all:
--- main ---
(nothing)
--- pr148 ---
serviceAccountName: my-sa
automountServiceAccountToken: true
So anyone using a pre-existing ServiceAccount has silently been running alpha and zero under default. After this they move to the named SA.
RBAC isn't the risk — these pods don't talk to the API server. Workload identity is. Under a mesh the SA is the pod's identity: Istio derives the SPIFFE ID from it, so any AuthorizationPolicy or PeerAuthentication keyed to .../sa/default stops matching after the upgrade. Given #145 in this same series is specifically about making backups work under strict mTLS, that's a combination operators should read about in the notes rather than find live.
While we're on upgrade impact: the stock render diff against main is exactly two changes, both in alpha (this exec prefix and the removed volume), so zero doesn't roll but alpha does. The exec fix earns it — before this, bash swallowed SIGTERM and every alpha termination was a SIGKILL after the full 600s grace period.
Base of the stacked chain. Correct four latent StatefulSet bugs: mount
/dgrapheven when persistence is disabled; remove an inert explicitdatadirvolume that duplicated the volumeClaimTemplates entry;execthe alpha command so SIGTERM reaches Dgraph; routeserviceAccountNamethrough the helper so it never renders empty.volumeClaimTemplatesandspec.selectorare untouched (immutable on update).Part of splitting #140 into per-area PRs. The merge of all split PRs reproduces #140's tree byte-for-byte. #140 is being closed as superseded.