Skip to content

fix(dgraph): correct latent StatefulSet bugs (mount, datadir volume, exec, serviceAccountName) - #148

Open
mlwelles wants to merge 1 commit into
mainfrom
fix-statefulset-latent-bugs
Open

fix(dgraph): correct latent StatefulSet bugs (mount, datadir volume, exec, serviceAccountName)#148
mlwelles wants to merge 1 commit into
mainfrom
fix-statefulset-latent-bugs

Conversation

@mlwelles

Copy link
Copy Markdown
Contributor

Base of the stacked chain. Correct four latent StatefulSet bugs: mount /dgraph even when persistence is disabled; remove an inert explicit datadir volume that duplicated the volumeClaimTemplates entry; exec the alpha command so SIGTERM reaches Dgraph; route serviceAccountName through the helper so it never renders empty. volumeClaimTemplates and spec.selector are 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.

{{- if .Values.serviceAccount.create }}
serviceAccountName: {{ .Values.serviceAccount.name }}
automountServiceAccountToken: {{ .Values.zero.automountServiceAccountToken }}
{{- if or .Values.serviceAccount.create .Values.serviceAccount.name }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants