fix(opensearch-operator): raise vm.max_map_count on every node from a DaemonSet - #4152
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe OpenSearch operator installation is now privileged. The system chart adds a node-critical DaemonSet that raises ChangesOpenSearch sysctl setup
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change deploys a node-level OpenSearch sysctl setup that raises vm.max_map_count only when needed while preserving higher values. The privileged operation is limited to initialization and the rendered configuration is covered by chart tests, leaving no concrete current-head merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant DaemonSet
participant InitSysctl
participant NodeKernel
participant Idle
DaemonSet->>InitSysctl: Start privileged init container
InitSysctl->>NodeKernel: Read vm.max_map_count
InitSysctl->>NodeKernel: Set value to 262144 when below the floor
InitSysctl->>Idle: Complete initialization
Idle->>DaemonSet: Keep the pod running
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
OpenSearch refuses to start when vm.max_map_count is below 262144 and the store may use mmap. The kernel default is 65530 and nothing in the platform's node configuration raises it: the Talos profiles declare no sysctls, and neither does any documented node-preparation path. The value cannot come from the OpenSearch pod. The kernel does not namespace vm.*, so kubelet refuses it in a pod's securityContext.sysctls and in --allowed-unsafe-sysctls alike, both of which admit namespaced sysctls only. The operator's own answer, spec.general.setVMMaxMapCount, appends a privileged init container to every OpenSearch pod. Tenant namespaces carry no PodSecurity label of their own and take the cluster default, which Talos ships as baseline through the apiserver's PodSecurityConfiguration; that default rejects the container, so the StatefulSet is created and never produces a pod. Set the value once per node instead, from a DaemonSet in the operator's own namespace. The init container raises the limit to a floor rather than assigning it: the tunable is node-global, so a node already tuned higher for another mmap-heavy workload keeps its own value. The pod runs at system-node-critical: it is a precondition for every OpenSearch pod on the node, so it must not lose a scheduling race to the workload it enables, though priority is not start ordering: a pod that starts first just fails its own bootstrap check and retries. The idle container runs the sleep under a shell with a TERM handler, because PID 1 of a namespace receives only the signals it installed a handler for and a bare sleep would be SIGKILLed after the full grace period on every rollout and drain. The write goes to /proc/sys only, with no hostPath and no edit to the node's on-disk configuration; the pod restarts with the node and reapplies the value. The component is declared privileged so the platform labels that namespace enforce=privileged, which is what this DaemonSet needs and what tenant namespaces deliberately do not get. The file returns to a path bb660b5 emptied. That earlier revision bind-mounted the host's /etc to append to sysctl.conf and ran an unpinned image; this one writes no host file and pins the image by digest. This removes the reason for an OpenSearch pod to ask the operator for the sysctl, but the app chart still asks. Dropping that request is a separate change and is what stops the pods being rejected. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
5a64149 to
a3ddba4
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
install.privileged stamps pod-security.kubernetes.io/enforce=privileged (internal/operator/package_reconciler.go:824) at reconcile line 176, ahead of the HelmRelease at 333; six mutations of the new tests went red.
Findings
- [MINOR]
packages/system/opensearch-operator/templates/sysctl-daemonset.yaml:57, the floor guard fails open on a read it cannot parse - [MINOR]
packages/system/opensearch-operator/templates/sysctl-daemonset.yaml:67, 13 of 71 rendered lines are#prose shipped into the live object - [MINOR]
packages/system/opensearch-operator/values.yaml:21, the new digest pin has no producer - [MINOR]
packages/core/platform/sources/opensearch-operator.yaml:21, merged alone this pays the cost of the fix without delivering it
Claim mismatches
[PARTIAL] Release note, "so OpenSearch no longer depends on a privileged init container that PodSecurity baseline refuses". It still does: packages/apps/opensearch/templates/opensearch.yaml:16 keeps setVMMaxMapCount: true. Your next sentence says so, but the lead clause reads as the effect of this merge.
Caveats
- Not executed here, no cluster in a static review: the privileged
/proc/syswrite and PSS admission on Talos. The install lane covers both indirectly, sincevalues-isp-full.yamlenables paas andcozy_wait_all_helmreleases_readynow gates on a release that cannot go Ready until the DaemonSet is Ready on every node. Nothing asserts the resulting node value. - Recorded, not disputed: every paas cluster gets the DaemonSet and
cozy-opensearch-operatordrops to PSS privileged (the manager Deployment with it), including clusters that never create an OpenSearchCluster. No chart-level opt-out; the escape is disabling the component. bb660b57cdeleted an earlier sysctl DaemonSet here on review feedback. Both objections to it, the hostPath write into/etcand the unconditional assignment, are gone in this shape.
| command: | ||
| - sh | ||
| - -c | ||
| - 'set -e; current=$(sysctl -n vm.max_map_count); if [ "$current" -lt 262144 ]; then sysctl -w vm.max_map_count=262144; fi' |
There was a problem hiding this comment.
[MINOR] the floor guard fails open on a read it cannot parse
[ "$current" -lt 262144 ] is the safety check, and its unhappy input is a value that is not a number. set -e does not fire on a failing if condition, so the container skips the write and exits 0: the DaemonSet goes Ready, the node keeps 65530. Against the pinned digest:
$ docker run --rm docker.io/library/busybox:1.37.0@sha256:1487d0af5f52... sh -c \
'sh -c "set -e; current=\"\"; if [ \"\$current\" -lt 262144 ]; then echo WRITE; fi; echo end"; echo rc=$?'
sh: bad number
end
rc=0
Same for vm.max_map_count = 65530 as the value. Missing-key is fine (sysctl -n exits 1, set -e catches it, checked separately); what stays open is an unparseable read, and sysctl.image makes it reachable, since the key added for air-gapped registries un-pins the sysctl this comparison rests on. The fix also gives an operator a reason instead of a green pod on an unpatched node:
set -e
current=$(sysctl -n vm.max_map_count)
case "$current" in ''|*[!0-9]*) echo "unparseable vm.max_map_count: '$current'" >&2; exit 1;; esac
[ "$current" -ge 262144 ] || sysctl -w vm.max_map_count=262144What would change my mind: a ruling that sysctl.image may only ever point at a mirror of this busybox.
| limits: | ||
| memory: 16Mi | ||
| containers: | ||
| # Carries no work: it exists so the pod stays scheduled, because a |
There was a problem hiding this comment.
[MINOR] 13 of 71 rendered lines are # prose shipped into the live object
The header block is {{- /* */}} and is stripped correctly, but the two inline blocks are # and land verbatim in the DaemonSet object on every cluster: helm template opensearch-operator packages/system/opensearch-operator --show-only templates/sysctl-daemonset.yaml | grep -c '^\s*#' gives 13, at render lines 25-28 and 42-50. The nine-line "do not simplify this back to a bare sleep" block is worth moving; {{/* */}} keeps it where the next reader needs it and out of the applied object.
| # Third-party pass-through pinned by digest, the same reference the kubernetes | ||
| # package vendors; docs/agents/image-refs.md covers why such a ref is neither | ||
| # retagged nor mirrored. | ||
| image: docker.io/library/busybox:1.37.0@sha256:1487d0af5f52b4ba31c7e465126ee2123fe3f2305d638e7827681e7cf6c83d5e |
There was a problem hiding this comment.
[MINOR] the new digest pin has no producer
.github/renovate.json limits enabledManagers to gomod, dockerfile, github-actions and custom.regex; helm-values is off repo-wide and neither custom manager's managerFilePatterns (packages/extra/etcd/templates/etcd-cluster.yaml, packages/.+/templates/hooks/.+\.yaml) reaches a packages/system/*/values.yaml, so the digest ages silently. It is also a second, unlinked copy of the ref in packages/apps/kubernetes/images/busybox.tag, so bumping one diverges them. That copy is equally unmanaged today, which is why this is MINOR. Adding a custom regex manager for this file closes it.
| - name: opensearch-operator | ||
| path: system/opensearch-operator | ||
| install: | ||
| privileged: true |
There was a problem hiding this comment.
[MINOR] merged alone this pays the cost of the fix without delivering it
privileged: true here stamps pod-security.kubernetes.io/enforce=privileged on the whole cozy-opensearch-operator namespace (internal/operator/package_reconciler.go:819-825), the manager Deployment included, and packages/core/platform/templates/bundles/paas.yaml:22 makes this a default paas package, so the DaemonSet lands on every node of every paas cluster.
What that buys on its own is nothing a user can see. #4131's failure is on the tenant side: tenant namespaces run at the cluster-default PodSecurity level baseline, which refuses the operator's init-sysctl container, so the StatefulSet never creates a Pod. That container is still requested here, because packages/apps/opensearch/templates/opensearch.yaml:16 keeps setVMMaxMapCount: true, and the tenant namespace never gets the privileged label: the map at package_reconciler.go:823-825 covers platform package namespaces, and a tenant- namespace is explicitly not one of them.
So the node value is raised, the privileged surface is added, and #4131 reproduces exactly as before until the app-chart flip lands in #2682. That ordering is deliberate and the header comment at packages/system/opensearch-operator/templates/sysctl-daemonset.yaml:17-18 says so, which is why this is MINOR rather than a defect. Worth pinning it in the description, though, so that if the follow-up stalls it is visible that every customer node is carrying a privileged DaemonSet for a fix that has not shipped.
…anager (#2682) ## What this PR does The OpenSearch HTTP API (9200) and Dashboards (5601) already served TLS, from the operator's own private CA and with a SAN built from `cluster.local` while the credentials Secret advertised the release on the platform domain, so nothing a client was handed could verify. Both certificates now come from a per-release cert-manager CA in the tenant namespace, and the CA reaches tenants as a key-free trust anchor. - `tls.issuer` selects who issues the HTTP server certificate, `cert-manager` or `operator`, and is named for the issuer because TLS is served under both. Unset resolves from `external` (cert-manager for externally published services, the operator for cluster-internal); an explicit value always wins. - Renders a self-contained cert-manager chain in the tenant namespace: a self-signed Issuer, a CA Certificate, a CA Issuer, and two leaf Certificates, one for the HTTP API and one for Dashboards, each with the SANs of its own Service and the external hostname when `external: true`. Leaf keys are written as PKCS8, since the JVM loader rejects the PKCS1/SEC1 encoding cert-manager emits by default, and the CA subject is pinned so the operator's signature check accepts the chain. - The CA Secret is `<release>.http-ca`. The dot separates the chart-chosen suffix from release names, so no other release can produce that name; the two leaf Secrets stay hyphenated because none of the chart's suffixes is a tail of another. The operator's own transport CA remains at `<release>-ca` and is not touched. - Transport mTLS (9300) stays operator-managed (`transport.generate: true`, `perNode: true`). The security plugin requires it and the chart cannot turn it off. - `hack/e2e-chainsaw/opensearch/` runs the move on a cluster: it installs the app on the operator's HTTP listener, removes `tls.issuer` so it resolves from `external`, and asserts the cert-manager chain, the CR dropping `http.generate`, the admin certificate reissued under the chart CA, the StatefulSet rolling onto the three subPath mounts, the tenant anchor, and the certificate the endpoint actually presents to a client holding that anchor. ### Sysctl carrier and single-node rolls The E2E suite for this PR was red because the chart set `spec.general.setVMMaxMapCount: true`, which makes the operator prepend a privileged `init-sysctl` container to every OpenSearch pod. Tenant namespaces run at the cluster's default PodSecurity level, baseline on Talos, so admission refused the container and the StatefulSet never produced a pod (#4131). The carrier for the sysctl is now on main: #4152 added a DaemonSet to the `opensearch-operator` package that raises `vm.max_map_count` to a floor of 262144 on every node, from a namespace the platform labels privileged. This branch is rebased onto that main and sets the flag to an explicit `false`, with a template comment saying why and a unit test pinning it, so a release no longer asks the operator for a container admission refuses. The value stays with the DaemonSet until the node contract carries it (#4144); the DaemonSet's own template comment, which said the chart still asks, now says it does not. Three more things came out of the same pass. The chart asked the operator to drain data nodes before every rolling restart, and with a single data node the drain never finishes: at v2.8.0 `PreparePodForDelete` excludes the node from allocation and then waits for it to hold no shards, which the only data node can never satisfy, so a one-replica release could not roll for a version bump or for the certificate move this PR relies on. The chart now drains only when a second data node exists, pinned both ways in the unit tests. The chart maps an empty `storageClass` to `replicated` at one replica, and the PR lane runs LINSTOR without DRBD and creates only `local`, so the E2E fixture takes its class from the `storageClass` value the harness passes with `--set-string` from `COZY_E2E_STORAGE_CLASS`, the way the vminstance fixtures do; on the nightly QEMU lane that still resolves to `replicated`. And the external Dashboards Service selected labels no pod carries. `pkg/builders/dashboards.go` at v2.8.0 stamps `opensearch.cluster.dashboards: <cluster name>` on the Deployment, its selector and the pod template, and nothing else; the chart asked for `opster.io/opensearch-cluster` together with `app.kubernetes.io/component: dashboards`, so the LoadBalancer came up with no endpoints and no error to show for it. It selects the label the operator sets now. ### securityadmin cross-trust, still blocked on the DNS base Chart-managed HTTP TLS previously broke the operator's securityadmin reconciliation: the admin certificate was signed by the transport CA while the HTTP listener trusted the cert-manager CA, so the two never verified against each other and users and roles silently never applied. This chart puts one CA under both sides, handing the HTTP CA to the operator through `spec.security.tls.http.caSecret` so that it signs the admin certificate with the CA the listener trusts. That closes the CA mismatch. The other half, the job resolving a name the operator built from `cluster.local` on a cluster that does not use it, was closed separately and is on main, so the security configuration applies once this lands. No operator bump is required; the API exists in the pinned version. That path is version-gated upstream at OpenSearch 2.0.0: below it the operator falls back to the transport CA and the mismatch returns with no error anywhere. The chart therefore refuses chart-managed HTTP TLS on `version: v1` with an explicit render-time failure that names the mechanism and offers the alternatives, rather than letting it fail silently at runtime. ### Trust anchor for tenants The HTTP server's private key is not exposed to tenants. The chart renders a `TenantProjection` sentinel that names `<release>.http-ca` as its source; the CA-extraction controller publishes a `ca.crt`-only copy as `<release>.tenant-ca`, and the ResourceDefinition selects it with the engine-agnostic `internal.cozystack.io/tenant-ca` label. The sentinel is gated on the same condition as the chain, so a plaintext release renders no sentinel instead of one stuck at `SourceNotFound`. Tenants get what they need to verify the server and nothing more. ## Known limitations These are stated rather than left to be discovered: 1. **securityadmin needed both trust and reachability, and both are closed now.** This branch put one CA under the admin certificate and the HTTP listener. The other half was the operator resolving `<svc>.<ns>.svc.<DNS_BASE>` with `DNS_BASE` left at the vendored `cluster.local`, so the job waited on a name that never resolved whichever CA had signed; the platform now renders that value from `networking.clusterDomain` and it is on main, which is what unblocks this branch. 2. **The e2e suite reaches the move, not the chart bump that triggers it.** The suite above arrives at the chart-managed chain by removing `tls.issuer` from a running release, which leaves exactly the values an upgraded release arrives with. The one step it cannot run is the one where the previous chart version wrote the CR, because there is no second chart version to install from in this tree. Everything downstream of that write is exercised on a cluster. 3. **A reissue does not reach a running pod.** Neither service reloads its certificate, and cert-manager reissues on any change to `external`, the tenant host or the cluster domain, so an ordinary edit leaves the pods serving material that no longer matches their own names with no error, event or condition to show for it. Completing such a change is manual. So is removing the CA private key cert-manager leaves behind when TLS is turned off again, since the platform runs with `enableCertificateOwnerRef: false` and deleting a `Certificate` does not delete its `Secret`. `packages/apps/opensearch/README.md` carries both, with the commands. ### Downstream repositories `packages/apps/opensearch/values.schema.json` gains the `tls` object, so `cozystack/terraform-provider-cozystack` needs the field modelled for `cozystack_opensearch`; tracked in cozystack/terraform-provider-cozystack#41. ### Upgrade impact An existing `external: true` release with `tls.issuer` unset serves operator-issued HTTP TLS today and moves to chart-managed TLS on its first reconcile after this lands: the cluster rolls once and is re-anchored on a new per-release CA, so any client pinned to the operator's CA has to be repointed at the key-free `<release>.tenant-ca`. Setting `tls.issuer: operator` before the upgrade keeps that half as it is. Releases that are not external are untouched. Every existing release rolls once regardless of `tls.issuer` and `external`, because the pod template loses the operator's `init-sysctl` container; on a single-replica release that roll is the first one the operator can complete, since draining the only data node used to block every rolling restart. The published external-dns name changes on the same upgrade whatever `tls.issuer` says, because the Services are gated on `external` alone: they move from `<release>.<namespace>.<cluster-domain>`, an in-cluster name that never resolved outside the cluster, to `<release>.<tenant-host>` and `<release>-dashboards.<tenant-host>`. external-dns is configured upsert-only here, so the old record is not withdrawn and keeps pointing at the LoadBalancer until it is removed from the zone by hand. That cleanup is per zone rather than per release and nothing enumerates the records, so the README shows how to derive the list from the cluster. ### Release note ```release-note feat(opensearch)!: the OpenSearch HTTP API and Dashboards already served TLS, from the operator's own private CA and with a SAN built from cluster.local while the credentials Secret advertised the release on the platform domain; both certificates now come from a per-release cert-manager CA whose anchor the platform publishes into the tenant as <release>.tenant-ca. Any client that worked against the old endpoint was skipping verification, because no hostname it was handed could have matched that SAN. Existing external releases move from the operator's CA to the chart's on their first reconcile, and their published external-dns name moves from the in-cluster domain to <release>.<tenant-host> with the old record left in place; set tls.issuer: operator beforehand to opt out of the CA move. With dashboards.enabled the application name is limited to 41 characters, and to 32 when external is on as well; past either bound the chart fails the render and withholds every object in the release. A release already above a bound is not broken by the guard, it is told why it has not been upgrading: the Dashboards Service the operator builds from that name exceeds the 63-character limit for a DNS label, the operator returns that failure on every pass, and the reconcilers queued behind Dashboards, which are upgrade and restart and snapshot repository, have not run since the release was created. Set dashboards.enabled to false to drop the bound. The external Dashboards Service also selected labels the operator does not put on Dashboards pods, so it stood up with no endpoints; it selects opensearch.cluster.dashboards now. The drain of data nodes is now requested only with the data role on and more than two replicas, since at two the operator gives up on a full drain itself. The chart stops asking the operator for its privileged init-sysctl container, since the opensearch-operator package now sets vm.max_map_count from a DaemonSet, so every existing release rolls once on upgrade; single-replica releases no longer ask the operator to drain their only data node, which used to block every rolling restart ```
What this PR does
OpenSearch will not start below
vm.max_map_count=262144when the store may use mmap, and nothing in the platform sets it. The Talos profiles declare no sysctls and no documented node-preparation path sets one, so nodes sit at the kernel default of 65530. The app chart asks the operator to cover that withspec.general.setVMMaxMapCount, which appends a privileged init container to every OpenSearch pod. Tenant namespaces carry no PodSecurity label of their own, so they take the cluster default, which Talos ships as baseline through the apiserver'sPodSecurityConfiguration. That default refuses the container and the StatefulSet never produces a pod. On vanilla kubeadm, kind or k3s the default isprivilegedand the same release comes up, which is why this only bites on a Talos install.The value cannot come from the pod at all. The kernel does not namespace
vm.*, so kubelet rejects it insecurityContext.sysctls, and refuses to start if it appears in--allowed-unsafe-sysctls. That leaves node configuration or a privileged pod, and a privileged pod belongs in a system namespace rather than in every tenant.This adds a DaemonSet to the
opensearch-operatorpackage that sets the value once per node from a privileged init container, and marks the component privileged so the platform labelscozy-opensearch-operatorwithenforce=privileged. The init container writes to/proc/sysand nothing else, with no hostPath. The pod restarts with the node and reapplies the value, so there is nothing to persist.It raises the limit to a floor rather than assigning it, which matters more than it looks.
vm.max_map_countis node-global, and a stock Ubuntu 24.04 node already reads 1048576 from asysctl.ddrop-in. An unconditional write would drop that to 262144 for every workload on the node, on every boot, and undo whatever the operator set it for. So the init container reads the current value first and writes only when it is below the threshold.The idle main container runs its sleep under a shell that traps SIGTERM, so a pod exits on its own when the DaemonSet rolls or a node drains, instead of ignoring the signal and being killed after the full grace period.
This does not fix OpenSearch on its own. The app chart still requests the operator's init container through
setVMMaxMapCount: true, so a release stays blocked until the flip tofalselands with #2682, after this merges. The DaemonSet has to be in place first: flipping it without one would trade the admission failure for a failed bootstrap check. Carrying the sysctl in the node contract instead, across the Talos profiles and the downstream node-prep paths, is tracked in #4144; that is what would let this DaemonSet be retired.Two things change on upgrade, and neither is visible from the diff. One small DaemonSet pod appears per node in
cozy-opensearch-operator, and that namespace gainspod-security.kubernetes.io/enforce=privileged. That label has no finer granularity than the namespace, so the operator Deployment sitting there leaves the cluster default too, not just the DaemonSet. Keeping the manager at baseline would mean a namespace of its own and another component entry, which did not look worth it, but it is a real cost rather than an accident. The operator is in thepaasbundle's always-on list, so this lands on every node of every paas installation and raises a node-global kernel tunable there, whether or not anyone ever creates an OpenSearchCluster. That is the price of a node-level sysctl having no cheaper carrier, and it is worth an explicit decision rather than being noticed later.The second is
cozypkg add, which gates privileged components behind a confirmation or--allow-privileged. The gate runs over the resolved dependency closure, andopensearch-applicationdependsOn the operator, so installing the app that way now asks one more question. This is not a new break for scripts:addalready prompts for a variant on every package it resolves and fails on EOF, so it was never usable non-interactively in the first place. Platform-bundle installs go through the reconciler and are unaffected.Screenshots
Not a UI change.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff. It adds a template, a test and a values key underpackages/system/opensearch-operator/, plus one field inpackages/core/platform/sources/opensearch-operator.yaml. No new app package, no app schema or version enum, noApplicationDefinition, no installer value, no namespace rename, no variant or bundle, nohack/path or make target, and no node prerequisite inhack/e2e-prepare-cluster.bats. Nothing in the map matches.Release note
Summary by CodeRabbit
Bug Fixes
vm.max_map_countsetting, improving startup reliability.Tests