hubcio commented on code in PR #2976:
URL: https://github.com/apache/iggy/pull/2976#discussion_r2965247195
##########
helm/charts/iggy/templates/deployment.yaml:
##########
@@ -44,7 +44,7 @@ spec:
{{- end }}
serviceAccountName: {{ include "iggy.serviceAccountName" . }}
securityContext:
- {{ toYaml .Values.server.podSecurityContext | nindent 8 }}
+ {{ toYaml .Values.podSecurityContext | nindent 8 }}
Review Comment:
this fixes a real bug - `.Values.server.podSecurityContext` never existed,
so the server was silently getting no seccomp profile. the fix is correct.
two things worth considering:
1. any user who discovered the old template and worked around it by setting
`server.podSecurityContext` in their custom values will have their override
silently ignored after upgrading. a template guard would catch this:
```yaml
{{- if .Values.server.podSecurityContext }}
{{- fail "server.podSecurityContext has been moved to podSecurityContext
(root level). Please update your values." }}
{{- end }}
```
2. both the server (here) and UI (line 158) deployments now share the same
root-level `podSecurityContext` which sets `seccompProfile.type: Unconfined`.
the UI is a SvelteKit frontend - it doesn't need io_uring privileges. same
applies to `.Values.securityContext` with `IPC_LOCK` at line 51 vs 162. worth
splitting into `server.*` / `ui.*` namespaces in a follow-up.
##########
helm/charts/iggy/values.yaml:
##########
@@ -36,7 +36,7 @@ server:
serviceMonitor:
# -- Enable this if you're using [Prometheus
Operator](https://github.com/coreos/prometheus-operator)
- enabled: true
+ enabled: false
Review Comment:
the new default is correct - defaulting to `true` created a hard dependency
on Prometheus Operator CRDs at install time, which broke fresh installs.
however, this is a breaking change for `helm upgrade`: existing deployments
that relied on the default `true` (without pinning it in their values override)
will silently lose their `ServiceMonitor` resource on the next upgrade.
prometheus stops scraping with no error or warning.
this should be reflected in a chart version bump (currently `0.4.0` in
`Chart.yaml`) - at least to `0.5.0` to signal the behavioral change.
##########
helm/charts/iggy/templates/deployment.yaml:
##########
@@ -98,11 +98,11 @@ spec:
livenessProbe:
httpGet:
- path: /
+ path: /ping
Review Comment:
`/ping` is the right endpoint for health checks - returns a static `"pong"`
with no auth or db access.
however, the probes use K8s defaults: `initialDelaySeconds: 0`,
`periodSeconds: 10`, `failureThreshold: 3` - that gives the server 30 seconds
to respond before the liveness probe kills it. for an io_uring server that
needs to initialize ring buffers, restore state from persistent storage, and
set up memory-locked regions, 30 seconds can be tight on constrained nodes or
when recovering large `local_data` directories.
a `startupProbe` would decouple "is it still starting?" from "is it alive?"
- e.g. `startupProbe` with `failureThreshold: 30` and `periodSeconds: 10` gives
5 min startup budget while keeping the liveness probe aggressive once running.
##########
helm/charts/iggy/README.md:
##########
@@ -175,13 +196,14 @@ kubectl port-forward svc/iggy-ui 3050:3050
### Using Ingress
-Enable ingress in values:
+Enable ingress in values. Set `className` and any controller-specific
annotations to match your
+ingress implementation:
```yaml
server:
ingress:
enabled: true
- className: nginx
+ className: ""
Review Comment:
the surrounding text says "Set `className` and any controller-specific
annotations to match your ingress implementation" but the example shows
`className: ""`. an empty string causes the `ingressClassName` field to be
omitted entirely from the rendered manifest (per `ingress.yaml:44-46`), which
only works if the cluster has a default IngressClass configured.
either use a placeholder like `className: "<your-ingress-class>"` or add a
note that leaving it empty falls back to the cluster's default ingress class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
