diff --git a/helm/pinot/README.md b/helm/pinot/README.md index 9da061323052..dc600ab39c27 100644 --- a/helm/pinot/README.md +++ b/helm/pinot/README.md @@ -265,6 +265,29 @@ The chart can be customized using the following configurable parameters: | `image.pullPolicy` | Pinot Container image pull policy | `IfNotPresent` | | `cluster.name` | Pinot Cluster name | `pinot-quickstart` | |------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------| +| `jfr.configuration` | JFR event settings: `default`, `profile`, or a path to a `.jfc` inside the container | `default` | +| `jfr.recordingName` | Name of the recording, as used by `jcmd ... JFR.dump name=...` | `pinot` | +| `jfr.maxSize` | Recording data kept for the current JVM run (Kubernetes quantity) | `2Gi` | +| `jfr.maxAge` | Wall-clock history kept for the current run; empty means bound by size alone | `""` | +| `jfr.maxChunkSize` | Size of an individual chunk file; the unit of eviction and of loss on SIGKILL | `12Mi` | +| `jfr.mountPath` | Where the JFR repository is mounted in the container | `/var/pinot/jfr` | +| `jfr.persistence.enabled` | Keep recordings on a PVC (one per pod) rather than an emptyDir; ignored by `minionStateless` | `false` | +| `jfr.persistence.accessMode` | Access mode for the JFR PVC | `ReadWriteOnce` | +| `jfr.persistence.size` | Size of the JFR PVC, per pod | `10Gi` | +| `jfr.persistence.restartHeadroom` | In-place container restarts to reserve room for in the sizing check | `1` | +| `jfr.persistence.storageClass` | StorageClass for the JFR PVC; `-` means the empty class | `""` | +| `jfr.persistence.emptyDirSizeLimit` | `sizeLimit` for the emptyDir; empty derives `maxSize + 2 * maxChunkSize` | `""` | +| `jfr.janitor.enabled` | Run the init container that reclaims repositories left by previous JVM runs | `true` | +| `jfr.janitor.maxAge` | Drop leftover repositories older than this (`m`/`h`/`d`); empty skips the pass | `7d` | +| `jfr.janitor.maxTotalSize` | Trim oldest-first until the repository fits this; empty skips the pass | `4Gi` | +| `jfr.janitor.minIdleMinutes` | Never delete a repository written to this recently | `15` | +| `jfr.janitor.image.repository` | Image for the cleanup init container; defaults to the Pinot image | `""` | +| `jfr.janitor.image.tag` | Tag for the cleanup init container image | `""` | +| `jfr.janitor.image.pullPolicy` | Pull policy for the cleanup init container image | `""` | +| `jfr.janitor.securityContext` | Security context for the cleanup init container | `{}` | +| `jfr.janitor.resources` | Resources for the cleanup init container | `{}` | +| `controller.jfr.enabled` / `broker.jfr.enabled` / `server.jfr.enabled` / `minion.jfr.enabled` / `minionStateless.jfr.enabled` | Enable continuous JFR for that role | `false` | +|------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------| | `controller.name` | Name of Pinot Controller | `controller` | | `controller.port` | Pinot controller port | `9000` | | `controller.replicaCount` | Pinot controller replicas | `1` | @@ -402,6 +425,176 @@ or If you want to use pd-standard storageClass: kubectl apply -f gke-pd.yaml ``` +## Continuous profiling with Java Flight Recorder + +Each Pinot role can run a continuous JFR recording, so that when something goes wrong the +profile of the minutes leading up to it is already on disk. Enable it per role and tune the +shared `jfr` block: + +```yaml +jfr: + configuration: default # or `profile` for much more detail, at a higher cost + maxSize: 2Gi # recording data kept for the current JVM run + persistence: + enabled: false # true keeps recordings on a PVC across rescheduling + size: 10Gi + janitor: # only used when persistence.enabled is true + maxAge: 7d # drop repositories left by runs older than this + maxTotalSize: 4Gi # ...and trim the oldest until the volume fits + +server: + jfr: + enabled: true +broker: + jfr: + enabled: true +``` + +The recording is started by the JVM itself: the chart appends `-XX:StartFlightRecording` and +`-XX:FlightRecorderOptions` to that role's `JAVA_OPTS`. It is therefore live from the first +instruction, and it does not depend on ZooKeeper, Helix or any Pinot config being reachable — +which matters, because those are not safe assumptions during the incidents you would most want a +profile for. + +### Sizing + +JFR's `maxSize` makes the recording roll like a log file: once the repository exceeds it, the +oldest chunks are evicted. Nothing has to rotate files by hand. Steady-state disk use for a +running JVM is about `maxSize + 2 * maxChunkSize`. + +Prefer to bound the recording by **size** rather than by age. The event rate depends almost +entirely on the workload — GC frequency above all — so picking `maxAge` up front means guessing a +number you do not know. Set the disk budget you can afford, leave `jfr.maxAge` empty, and read the +window you actually got back off the recording: + +```bash +jfr summary recording.jfr # prints Start and Duration +``` + +### Choosing where recordings are stored + +By default (`jfr.persistence.enabled: false`) recordings go to an `emptyDir`. They survive a +container restart but not the pod being rescheduled, the chart caps the volume with a `sizeLimit`, +and no cleanup init container is needed. This applies in place with a normal rolling restart, which +makes it the right choice for "turn on profiling now". + +Set `jfr.persistence.enabled: true` to keep recordings on a PersistentVolume, so a profile survives +the node loss or eviction that destroyed the pod. Two costs come with it: + +- It provisions one volume of `jfr.persistence.size` **per pod** — 50 servers at the default `10Gi` + is 500Gi — and pods stay `Pending` on a cluster with no default StorageClass unless you set + `jfr.persistence.storageClass`. +- It adds an entry to the StatefulSet's `volumeClaimTemplates`, a field Kubernetes **forbids + changing in place**, so it cannot be switched on with a plain `helm upgrade`. See + [UPGRADING.md](UPGRADING.md) for the one-time `kubectl delete statefulset --cascade=orphan` + procedure. + +### The stateless minion is different + +`minionStateless` is a Deployment, not a StatefulSet, so it has no `volumeClaimTemplates` and its +replicas would have to share a single claim. Its recordings therefore always go to an `emptyDir`, +whatever `jfr.persistence.enabled` says. + +This is a correctness constraint, not a limitation we could lift by trying harder. The janitor +below is safe because when it runs, every repository on the volume belongs to a JVM that has +already exited. On a shared claim that stops being true: with `replicaCount > 1`, or during any +rolling update, a starting pod would see a running pod's live repository. (A single +`ReadWriteOnce` claim would also stall the rollout with a Multi-Attach error.) + +### Why there is an init container + +`maxSize` bounds the repository of the JVM that is *running*. Nothing inside the JVM ever reclaims +the repository of a JVM that has already exited, and `preserve-repository=true` — which is what +keeps recordings across a restart in the first place — means those directories survive on the +volume forever. Left alone they accumulate one `maxSize` per restart until the volume is full. + +The `jfr-janitor` init container reclaims them. Running it as an init container is what makes it +safe: init containers finish before the Pinot process starts, so every directory it sees belongs to +a run that is already over and there is no "is this one still in use?" question to get wrong. + +As a backstop, the janitor also refuses to delete any repository written to within +`jfr.janitor.minIdleMinutes` (default 15). A live JFR repository is flushed at least once a second, +so anything idle that long belongs to a JVM that is gone. Nothing the janitor does is required for +Pinot to run, so it tolerates every failure and always exits 0 — a failed cleanup must never keep a +role from starting. + +Note that Kubernetes does **not** re-run init containers when it restarts a container in place (an +OOMKilled process, for example) — only when the pod itself is recreated. Each such restart strands +another repository until the next pod-level restart. The chart refuses to render if +`jfr.janitor.maxTotalSize + jfr.maxSize + 2 * jfr.maxChunkSize` exceeds `jfr.persistence.size`, +which guarantees room for the run that follows a cleanup; raise `jfr.persistence.size` beyond that +if your workload restarts in place often. + +### If the volume fills up + +Worth knowing before you enable this. If the JFR volume runs out of space, the JVM cannot create its +repository and **fails to start** — and because Kubernetes restarts the container rather than the +pod, the janitor init container does not re-run to clear it. The role stays down until you delete +the pod: + +```bash +kubectl delete pod # recreates the pod, which re-runs the janitor +``` + +The chart's sizing check exists to keep you out of that state: it refuses to render unless +`jfr.janitor.maxTotalSize + (1 + jfr.persistence.restartHeadroom) * (jfr.maxSize + 2 * jfr.maxChunkSize)` +fits in `jfr.persistence.size`. Raise `jfr.persistence.restartHeadroom` if your workload restarts in +place (OOMKills, liveness failures) more than occasionally. + +### Getting a recording out + +```bash +# Snapshot a running JVM without interrupting the recording +kubectl exec -- jcmd 1 JFR.dump name=pinot filename=/tmp/snap.jfr +kubectl cp :/tmp/snap.jfr ./snap.jfr + +# After a crash: rebuild a recording from the repository left behind, including the +# chunk that was still open when the JVM died +kubectl exec -- ls /var/pinot/jfr +kubectl exec -- jfr assemble /var/pinot/jfr/ /tmp/crash.jfr +``` + +You never need to copy the whole repository. Every `*.jfr` chunk in it is a valid recording on its +own and is named with its start timestamp, so you can pull only the chunks covering the window you +care about; concatenating chunks is a valid merge. To split a large file after the fact, use +`jfr disassemble --max-size 100M recording.jfr`. + +### A note on units + +Every size in the `jfr` block is a Kubernetes quantity, the same as everywhere else in this chart: +`2Gi` is 2^30 and `2G` is 10^9. The chart converts to the byte counts the JVM wants, so JFR's own +unit table never leaks into your values file. + +One trap the chart rejects outright: a lowercase `m` means *milli* in Kubernetes, so `500m` is half +a byte rather than 500 MB. Use `M` or `Mi`. + +### Migrating tuned `pinot.jfr.*` values + +| Cluster config | Chart value | +|---|---| +| `pinot.jfr.enabled` | `.jfr.enabled` | +| `pinot.jfr.configuration` | `jfr.configuration` | +| `pinot.jfr.name` | `jfr.recordingName` | +| `pinot.jfr.directory` | `jfr.mountPath` | +| `pinot.jfr.maxSize` | `jfr.maxSize` | +| `pinot.jfr.maxAge` | `jfr.maxAge` | +| `pinot.jfr.preserveRepository` | set automatically from `jfr.persistence.enabled` | +| `pinot.jfr.repositoryMaxTotalSize` | `jfr.janitor.maxTotalSize` | +| `pinot.jfr.toDisk`, `pinot.jfr.dumpOnExit`, `pinot.jfr.dumpPath` | no equivalent; see above | + +**The value formats differ**, so do not copy values across verbatim: + +| Old format | New format | +|---|---| +| `P7D`, `PT12H` (ISO-8601) | `7d`, `12h` | +| `2GB`, `20GB` | `2Gi`, `20Gi` (Kubernetes quantities) | + +### Relationship to `pinot.jfr.*` cluster configs + +This replaces the `pinot.jfr.*` cluster configs, which are deprecated. Those started the recording +from inside the JVM after it had connected to Helix, which meant startup was never captured and any +change to a `pinot.jfr.*` key restarted the recording — discarding all recorded history. + ## How to clean up Pinot deployment ```bash diff --git a/helm/pinot/UPGRADING.md b/helm/pinot/UPGRADING.md index 23e656841b54..b0638361c2af 100644 --- a/helm/pinot/UPGRADING.md +++ b/helm/pinot/UPGRADING.md @@ -21,6 +21,59 @@ # Upgrading the Pinot Helm Chart +## From 1.0.x to 1.0.1 + +### Continuous JFR profiling moved from cluster config to JVM arguments + +The chart can now run a continuous Java Flight Recorder recording per role, started by +`-XX:StartFlightRecording` in `JAVA_OPTS`. Enable it with `.jfr.enabled` and tune the shared +`jfr` block; see the "Continuous profiling with Java Flight Recorder" section of the README. + +JFR is off by default, so upgrading changes nothing until you opt in. + +#### Enabling it is in-place; keeping recordings on a volume is not + +With the defaults, `.jfr.enabled: true` only changes `spec.template`, so it applies with a +normal rolling restart. Recordings go to an `emptyDir` and are lost when the pod is rescheduled. + +Setting `jfr.persistence.enabled: true` is the part that cannot be done in place: it adds an entry +to that StatefulSet's `volumeClaimTemplates`, and **Kubernetes forbids changing that field**, so +`helm upgrade` fails with: + +``` +updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', +'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden +``` + +Turning it back off fails the same way. If you need the PersistentVolume, delete the StatefulSet +without touching the pods, then upgrade: + +```bash +kubectl delete statefulset -pinot-server --cascade=orphan +helm upgrade ./helm/pinot \ + --set server.jfr.enabled=true --set jfr.persistence.enabled=true +``` + +`--cascade=orphan` leaves the running pods alone; the recreated StatefulSet adopts them, and the new +`jfr` volume is attached as each pod is rolled. Do this per role. A role's existing `data` PVCs are +untouched either way. + +#### If you use the `pinot.jfr.*` cluster configs + +Those are deprecated and will be removed in a future Pinot release. They still work, but they start +the recording only after the component has connected to Helix — so JVM startup is never captured — +and changing any `pinot.jfr.*` key restarts the recording, which discards all history recorded so +far. + +**Remove the `pinot.jfr.*` cluster configs before enabling `.jfr.enabled`.** This is a +prerequisite, not a preference. Running both means the old code path issues +`JFR.configure repositorypath=...`, which is JVM-global: it relocates the recording the JVM started, +off the volume the chart provisioned for it. + +Pinot 1.6.0 and later detect this and ignore the deprecated configs with a warning. **Earlier images +do not** — and the chart happily renders the JVM arguments for whatever `image.tag` you have pinned, +so on a pre-1.6.0 image the collision above is exactly what you get. + ## From 0.x to 1.0.0 Version 1.0.0 replaces the Bitnami ZooKeeper subchart with native Helm diff --git a/helm/pinot/scripts/jfr-janitor-test.sh b/helm/pinot/scripts/jfr-janitor-test.sh new file mode 100755 index 000000000000..62559fdbc24c --- /dev/null +++ b/helm/pinot/scripts/jfr-janitor-test.sh @@ -0,0 +1,145 @@ +#!/bin/sh +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +# Fixture tests for jfr-janitor.sh. Run: sh helm/pinot/scripts/jfr-janitor-test.sh + +set -u + +script_dir=$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd) +janitor="$script_dir/jfr-janitor.sh" +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +failures=0 +checks=0 + +check() { + checks=$(( checks + 1 )) + if [ "$2" = "$3" ]; then + echo "ok - $1" + else + echo "FAIL - $1" + echo " expected: $3" + echo " actual: $2" + failures=$(( failures + 1 )) + fi +} + +# `date` takes a different flag for relative times on BSD (macOS) and GNU (the container). +if date -v-1M +%Y >/dev/null 2>&1; then + minutes_ago() { date -v"-$1M" +%Y%m%d%H%M; } +else + minutes_ago() { date -d "-$1 minutes" +%Y%m%d%H%M; } +fi + +# make_repo [minutes_in_the_past] +make_repo() { + _dir="$1/$2" + mkdir -p "$_dir" + dd if=/dev/zero of="$_dir/chunk.jfr" bs=1024 count="$3" 2>/dev/null + if [ "${4:-0}" -gt 0 ]; then + _stamp=$(minutes_ago "$4") + touch -t "$_stamp" "$_dir/chunk.jfr" "$_dir" + fi +} + +# Names of surviving entries, sorted, space separated. The fixtures use plain names, so `ls` is +# fine here and is the portable option. +# shellcheck disable=SC2012 +survivors() { + ls -1 "$1" 2>/dev/null | sort | tr '\n' ' ' | sed 's/ $//' +} + +run() { + _repo="$1"; shift + env PINOT_JFR_REPOSITORY="$_repo" \ + PINOT_JFR_JANITOR_MAX_AGE_MINUTES="${AGE:-}" \ + PINOT_JFR_JANITOR_MAX_TOTAL_KIB="${BUDGET:-}" \ + PINOT_JFR_JANITOR_MIN_IDLE_MINUTES="${IDLE:-15}" \ + sh "$janitor" "$@" > "$work/out.txt" 2>&1 + echo "$?" > "$work/rc.txt" +} + +# --- age pass ----------------------------------------------------------------------------------- +r="$work/age"; mkdir -p "$r" +make_repo "$r" 2026_08_01_10_00_00_1 512 20000 +make_repo "$r" 2026_08_20_10_00_00_1 512 60 +AGE=10080 BUDGET='' IDLE=15 run "$r" +check "age pass drops only the aged-out repository" "$(survivors "$r")" "2026_08_20_10_00_00_1" +check "age pass exits 0" "$(cat "$work/rc.txt")" "0" + +# --- size pass ---------------------------------------------------------------------------------- +r="$work/size"; mkdir -p "$r" +make_repo "$r" 2026_08_01_10_00_00_1 1024 20000 +make_repo "$r" 2026_08_02_10_00_00_1 1024 20000 +make_repo "$r" 2026_08_03_10_00_00_1 1024 20000 +AGE='' BUDGET=2048 IDLE=15 run "$r" +check "size pass trims oldest-first down to the budget" \ + "$(survivors "$r")" "2026_08_02_10_00_00_1 2026_08_03_10_00_00_1" + +# --- non-repository entries are never touched --------------------------------------------------- +r="$work/foreign"; mkdir -p "$r/lost+found" "$r/operator-scratch" +: > "$r/lost+found/keep"; : > "$r/notes.txt" +make_repo "$r" 2026_08_01_10_00_00_1 1024 20000 +AGE=1 BUDGET=1 IDLE=15 run "$r" +check "foreign files and directories survive" \ + "$(survivors "$r")" "lost+found notes.txt operator-scratch" + +# --- a recently written repository is never deleted --------------------------------------------- +r="$work/live"; mkdir -p "$r" +make_repo "$r" 2026_08_01_10_00_00_1 1024 0 +AGE=1 BUDGET=1 IDLE=15 run "$r" +check "a live repository survives both passes" "$(survivors "$r")" "2026_08_01_10_00_00_1" +check "and the overage is reported" \ + "$(grep -c 'still .* against' "$work/out.txt")" "1" + +# --- a malformed budget must never mean 'delete everything' ------------------------------------- +r="$work/badbudget"; mkdir -p "$r" +make_repo "$r" 2026_08_01_10_00_00_1 1024 20000 +make_repo "$r" 2026_08_02_10_00_00_1 1024 20000 +AGE='' BUDGET='4GiB' IDLE=15 run "$r" +check "unparseable budget skips the size pass instead of deleting" \ + "$(survivors "$r")" "2026_08_01_10_00_00_1 2026_08_02_10_00_00_1" +check "unparseable budget still exits 0" "$(cat "$work/rc.txt")" "0" +check "unparseable budget is reported" "$(grep -c 'unusable size budget' "$work/out.txt")" "1" + +# --- empty budget simply skips the pass --------------------------------------------------------- +r="$work/nobudget"; mkdir -p "$r" +make_repo "$r" 2026_08_01_10_00_00_1 1024 20000 +AGE='' BUDGET='' IDLE=15 run "$r" +check "empty budget skips the size pass" "$(survivors "$r")" "2026_08_01_10_00_00_1" + +# --- degraded environments must not fail the init container ------------------------------------- +AGE=1 BUDGET=1 IDLE=15 run "$work/does-not-exist" +check "missing repository directory exits 0" "$(cat "$work/rc.txt")" "0" + +r="$work/readonly"; mkdir -p "$r" +make_repo "$r" 2026_08_01_10_00_00_1 1024 20000 +chmod 500 "$r" +AGE=1 BUDGET=1 IDLE=15 run "$r" +rc=$(cat "$work/rc.txt") +chmod 700 "$r" +check "an unremovable repository still exits 0" "$rc" "0" + +env PINOT_JFR_REPOSITORY= sh "$janitor" > "$work/out.txt" 2>&1 +check "unset repository exits 0" "$?" "0" + +echo +echo "$checks checks, $failures failure(s)" +[ "$failures" -eq 0 ] diff --git a/helm/pinot/scripts/jfr-janitor.sh b/helm/pinot/scripts/jfr-janitor.sh new file mode 100755 index 000000000000..04317cde7d1e --- /dev/null +++ b/helm/pinot/scripts/jfr-janitor.sh @@ -0,0 +1,168 @@ +#!/bin/sh +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +# Reclaims JFR repositories left behind by previous JVM runs. +# +# JFR's own `maxsize` bounds the repository of the JVM that is running. Nothing inside the JVM ever +# reclaims the repository of a JVM that has already exited, so with `preserve-repository=true` those +# directories accumulate on the volume until it is full. This script deletes them. +# +# It runs as an init container, which is what makes it safe: init containers finish before the Pinot +# container starts, so the repositories it sees belong to runs that are already over. It still +# refuses to touch anything written to recently, so that it stays safe if the volume is ever shared. +# +# Nothing here is required for Pinot to run. Every failure is tolerated and the script always exits +# 0: blocking a Pinot role from starting because a cleanup failed would be far worse than leaving a +# stale recording on disk. +# +# All sizes and durations arrive already converted to plain integers by the Helm chart, so this +# script parses no units. That keeps a single source of truth for the unit table and means a +# malformed value can never turn the size pass into "delete everything". +# +# Inputs (environment): +# PINOT_JFR_REPOSITORY directory holding the per-run repositories (required) +# PINOT_JFR_JANITOR_MAX_AGE_MINUTES drop repositories older than this many minutes; empty skips +# PINOT_JFR_JANITOR_MAX_TOTAL_KIB trim oldest-first until under this many KiB; empty skips +# PINOT_JFR_JANITOR_MIN_IDLE_MINUTES never touch a repository written to this recently + +repo="${PINOT_JFR_REPOSITORY:-}" +max_age_minutes="${PINOT_JFR_JANITOR_MAX_AGE_MINUTES:-}" +max_total_kib="${PINOT_JFR_JANITOR_MAX_TOTAL_KIB:-}" +min_idle="${PINOT_JFR_JANITOR_MIN_IDLE_MINUTES:-15}" + +# A JFR repository directory is named `_`. Matching that pattern keeps the +# janitor away from anything else sharing the volume (`lost+found`, an operator's scratch file), and +# makes a lexicographic sort a chronological one. +pattern='[0-9][0-9][0-9][0-9]_[0-9][0-9]_[0-9][0-9]_*' + +log() { + echo "jfr-janitor: $*" +} + +# A whole number, and nothing else. Anything the chart failed to convert is treated as "not set" +# rather than as zero: a zero budget would mean "delete everything". +is_positive_int() { + case "${1:-}" in + '' | *[!0-9]*) return 1 ;; + *) return 0 ;; + esac +} + +# Size of a path in KiB; 0 if it cannot be read. +kib() { + size=$(du -sk "$1" 2>/dev/null | awk 'NR == 1 { print $1 }') || size="" + is_positive_int "$size" || size=0 + echo "$size" +} + +# True when a repository may still belong to a live JVM. +# +# JFR flushes at least once a second, so a live repository has a recently modified chunk file. The +# directory's own mtime is checked too, which covers the window between a JVM creating its +# repository and writing the first chunk into it. +in_use() { + if [ -n "$(find "$1" -type f -mmin "-$min_idle" 2>/dev/null | head -1)" ]; then + return 0 + fi + [ -n "$(find "$1" -maxdepth 0 -mmin "-$min_idle" 2>/dev/null)" ] +} + +reclaim_by_age() { + is_positive_int "$max_age_minutes" || return 0 + log "dropping repositories older than $max_age_minutes minutes" + find "$repo" -mindepth 1 -maxdepth 1 -type d -name "$pattern" -mmin "+$max_age_minutes" \ + 2>/dev/null | sort > "$candidates" || return 0 + while IFS= read -r dir; do + [ -d "$dir" ] || continue + if in_use "$dir"; then + log "WARN skipping $dir: written to within the last $min_idle minutes" + continue + fi + log "removing $dir (aged out)" + rm -rf "$dir" || log "WARN could not remove $dir" + done < "$candidates" +} + +reclaim_by_size() { + if ! is_positive_int "$max_total_kib"; then + if [ -n "$max_total_kib" ]; then + log "WARN ignoring unusable size budget '$max_total_kib'; skipping the size pass" + fi + return 0 + fi + used=$(kib "$repo") + log "budget is $max_total_kib KiB, $used KiB in use" + find "$repo" -mindepth 1 -maxdepth 1 -type d -name "$pattern" 2>/dev/null | sort \ + > "$candidates" || return 0 + matched=0 + skipped=0 + while IFS= read -r dir; do + if [ "$used" -le "$max_total_kib" ]; then + break + fi + [ -d "$dir" ] || continue + matched=$(( matched + 1 )) + if in_use "$dir"; then + log "WARN skipping $dir: written to within the last $min_idle minutes" + skipped=$(( skipped + 1 )) + continue + fi + size=$(kib "$dir") + log "removing $dir ($size KiB, over budget)" + if rm -rf "$dir"; then + used=$(( used - size )) + else + log "WARN could not remove $dir" + fi + done < "$candidates" + if [ "$used" -gt "$max_total_kib" ]; then + if [ "$matched" -eq 0 ]; then + log "WARN still $used KiB over a $max_total_kib KiB budget and nothing matched '$pattern';" \ + "the JFR repository naming may have changed, or $repo holds data this script does not own" + else + log "WARN still $used KiB against a $max_total_kib KiB budget after cleanup" \ + "($skipped repositories skipped as recently written); the volume may fill" + fi + fi +} + +main() { + if [ -z "$repo" ]; then + log "WARN PINOT_JFR_REPOSITORY is not set; nothing to do" + return 0 + fi + mkdir -p "$repo" 2>/dev/null || true + if [ ! -d "$repo" ]; then + log "WARN $repo does not exist and could not be created; nothing to do" + return 0 + fi + + # Sorted candidates go to a file rather than a pipeline so the loops run in this shell and can + # keep a running total. + candidates="${TMPDIR:-/tmp}/jfr-janitor-candidates.$$" + trap 'rm -f "$candidates"' EXIT + + log "$repo holds $(kib "$repo") KiB before cleanup" + reclaim_by_age + reclaim_by_size + log "$repo holds $(kib "$repo") KiB after cleanup" +} + +main || log "WARN cleanup did not complete; continuing so that Pinot can start" +exit 0 diff --git a/helm/pinot/templates/_helpers.tpl b/helm/pinot/templates/_helpers.tpl index 9c3089440990..83bab9c07e43 100644 --- a/helm/pinot/templates/_helpers.tpl +++ b/helm/pinot/templates/_helpers.tpl @@ -370,3 +370,324 @@ Return pinot namespace to use {{- .Release.Namespace -}} {{- end -}} {{- end -}} + +{{/* +A Kubernetes quantity in bytes. All four JFR size values use this one convention, so `4Gi` means +the same thing everywhere in the chart. + +Deliberately rejects the lowercase `m` suffix: in Kubernetes that means *milli*, so `500m` would be +half a byte rather than 500 MB. pinot.jfr.validate rejects it up front with a message that says so. +*/}} +{{- define "pinot.jfr.sizeToBytes" -}} +{{- $value := . | toString -}} +{{- $number := regexFind "^[0-9]+" $value | int64 -}} +{{- $unit := regexFind "[A-Za-z]*$" $value -}} +{{- if eq $unit "Ki" -}}{{ mul $number 1024 }} +{{- else if eq $unit "Mi" -}}{{ mul $number 1048576 }} +{{- else if eq $unit "Gi" -}}{{ mul $number 1073741824 }} +{{- else if eq $unit "Ti" -}}{{ mul $number 1099511627776 }} +{{- else if eq $unit "k" -}}{{ mul $number 1000 }} +{{- else if eq $unit "M" -}}{{ mul $number 1000000 }} +{{- else if eq $unit "G" -}}{{ mul $number 1000000000 }} +{{- else if eq $unit "T" -}}{{ mul $number 1000000000000 }} +{{- else -}}{{ $number }} +{{- end -}} +{{- end -}} + +{{/* +A duration in whole minutes, from `m`, `h` or `d`. +*/}} +{{- define "pinot.jfr.durationToMinutes" -}} +{{- $value := . | toString -}} +{{- $number := regexFind "^[0-9]+" $value | int64 -}} +{{- $unit := regexFind "[A-Za-z]*$" $value -}} +{{- if eq $unit "d" -}}{{ mul $number 1440 }} +{{- else if eq $unit "h" -}}{{ mul $number 60 }} +{{- else -}}{{ $number }} +{{- end -}} +{{- end -}} + +{{/* +Whether the janitor init container should be rendered for a role. + +Call as: include "pinot.jfr.janitorEnabled" (dict "ctx" . "role" .Values.server) + +Pass `forceEmptyDir true` for a role that never uses a PersistentVolume (the stateless minion). + +More than the two obvious flags. The janitor is only useful on a PersistentVolume: an emptyDir is +created empty with the pod, and volumes are set up before init containers run, so on the emptyDir +path there is provably nothing for it to reclaim. Rendering it there would only add a container to +the serial pod-startup chain. + +This lives in a helper rather than inline at each of the five call sites because getting it wrong is +destructive rather than merely wrong: the janitor deletes recordings, so a user who sets +jfr.janitor.enabled=false must actually get no janitor. +*/}} +{{- define "pinot.jfr.janitorEnabled" -}} +{{- if and .role.jfr.enabled .ctx.Values.jfr.janitor.enabled (include "pinot.jfr.usesPersistentVolume" .) -}} +true +{{- end -}} +{{- end -}} + +{{/* +Whether a role's recordings actually land on a PersistentVolume. + +Not the same as jfr.persistence.enabled: the stateless minion passes `forceEmptyDir true` because a +Deployment cannot give each replica its own volume. Several decisions key off this rather than off +the global flag - whether preserve-repository is worth setting, and whether the janitor has anything +to reclaim. + +Call as: include "pinot.jfr.usesPersistentVolume" (dict "ctx" . "role" .Values.server) +*/}} +{{- define "pinot.jfr.usesPersistentVolume" -}} +{{- if and .ctx.Values.jfr.persistence.enabled (not (.forceEmptyDir | default false)) -}}true{{- end -}} +{{- end -}} + +{{/* +Validate the shared JFR settings. + +JFR options are JVM arguments, so a bad value does not degrade into a warning: the JVM refuses to +start and the pod goes into CrashLoopBackOff with the reason buried in the container log. Catching +it here turns that into a `helm install` error instead. +*/}} +{{- define "pinot.jfr.validate" -}} +{{- $jfr := .Values.jfr -}} +{{- if not $jfr.mountPath -}} +{{- fail "jfr.mountPath must be set when JFR is enabled for any role" -}} +{{- end -}} +{{- if not $jfr.recordingName -}} +{{- fail "jfr.recordingName must be set when JFR is enabled for any role" -}} +{{- end -}} +{{- if not $jfr.configuration -}} +{{- fail "jfr.configuration must be set when JFR is enabled for any role (e.g. 'default' or 'profile')" -}} +{{- end -}} +{{- $sizes := list "maxSize" "maxChunkSize" -}} +{{- range $key := $sizes -}} +{{- $value := index $jfr $key | toString -}} +{{- include "pinot.jfr.validateSize" (dict "key" (printf "jfr.%s" $key) "value" $value) -}} +{{- end -}} +{{- include "pinot.jfr.validateSize" (dict "key" "jfr.persistence.size" "value" ($jfr.persistence.size | toString)) -}} +{{- if $jfr.persistence.emptyDirSizeLimit -}} +{{- include "pinot.jfr.validateSize" (dict "key" "jfr.persistence.emptyDirSizeLimit" "value" ($jfr.persistence.emptyDirSizeLimit | toString)) -}} +{{- end -}} +{{- if $jfr.maxAge -}} +{{- if not (regexMatch "^[0-9]+[smhd]$" ($jfr.maxAge | toString)) -}} +{{- fail (printf "jfr.maxAge must be a duration such as '12h' or '7d', got %q" ($jfr.maxAge | toString)) -}} +{{- end -}} +{{- end -}} +{{- if not (regexMatch "^[0-9]+$" ($jfr.janitor.minIdleMinutes | toString)) -}} +{{- fail (printf "jfr.janitor.minIdleMinutes must be a whole number of minutes, got %q" ($jfr.janitor.minIdleMinutes | toString)) -}} +{{- end -}} +{{- if $jfr.janitor.enabled -}} +{{- if not (or $jfr.janitor.maxAge $jfr.janitor.maxTotalSize) -}} +{{- fail "jfr.janitor is enabled but neither jfr.janitor.maxAge nor jfr.janitor.maxTotalSize is set, so it would never reclaim anything" -}} +{{- end -}} +{{- if $jfr.janitor.maxAge -}} +{{- if not (regexMatch "^[0-9]+[mhd]$" ($jfr.janitor.maxAge | toString)) -}} +{{- fail (printf "jfr.janitor.maxAge must be 'm', 'h' or 'd', got %q" ($jfr.janitor.maxAge | toString)) -}} +{{- end -}} +{{- end -}} +{{- if $jfr.janitor.maxTotalSize -}} +{{- include "pinot.jfr.validateSize" (dict "key" "jfr.janitor.maxTotalSize" "value" ($jfr.janitor.maxTotalSize | toString)) -}} +{{- end -}} +{{- end -}} +{{- include "pinot.jfr.validateBudget" . -}} +{{- end -}} + +{{/* +One Kubernetes quantity. Call as: (dict "key" "jfr.maxSize" "value" "2Gi") +*/}} +{{- define "pinot.jfr.validateSize" -}} +{{- if regexMatch "^[0-9]+m$" .value -}} +{{- fail (printf "%s is %q, but a lowercase 'm' means *milli* in Kubernetes units, so that is a fraction of a byte. Use 'M' (10^6) or 'Mi' (2^20)." .key .value) -}} +{{- end -}} +{{- if not (regexMatch "^[0-9]+(k|Ki|M|Mi|G|Gi|T|Ti)?$" .value) -}} +{{- fail (printf "%s must be a Kubernetes quantity such as '512Mi', '2Gi' or '2G', got %q" .key .value) -}} +{{- end -}} +{{- end -}} + +{{/* +The JFR volume has to hold what the janitor leaves behind plus the run that starts next. Kubernetes +does not re-run init containers when it restarts a container in place, so each in-place restart +strands another repository; jfr.persistence.restartHeadroom says how many of those to budget for. + +Without this check the shipped defaults silently over-commit, and the volume fills during exactly +the incident the recording was enabled to capture. Worse, a full volume stops the JVM from starting +at all, and a container-level restart does not re-run the janitor to recover. +*/}} +{{- define "pinot.jfr.validateBudget" -}} +{{- $jfr := .Values.jfr -}} +{{- if and $jfr.persistence.enabled $jfr.janitor.enabled $jfr.janitor.maxTotalSize -}} +{{- $volume := include "pinot.jfr.sizeToBytes" $jfr.persistence.size | int64 -}} +{{- $budget := include "pinot.jfr.sizeToBytes" $jfr.janitor.maxTotalSize | int64 -}} +{{- $run := include "pinot.jfr.sizeToBytes" $jfr.maxSize | int64 -}} +{{- $chunk := include "pinot.jfr.sizeToBytes" $jfr.maxChunkSize | int64 -}} +{{- $runs := add1 ($jfr.persistence.restartHeadroom | int64) -}} +{{- $needed := add $budget (mul $runs (add $run (mul $chunk 2))) -}} +{{- if gt $needed $volume -}} +{{- fail (printf "JFR volume is over-committed. The janitor trims down to jfr.janitor.maxTotalSize (%s), then each JVM run writes up to jfr.maxSize (%s) plus two chunks of jfr.maxChunkSize (%s). Budgeting %d run(s) (1 + jfr.persistence.restartHeadroom) needs %d bytes, but jfr.persistence.size (%s) provides only %d. Raise jfr.persistence.size, or lower jfr.janitor.maxTotalSize or jfr.persistence.restartHeadroom." ($jfr.janitor.maxTotalSize | toString) ($jfr.maxSize | toString) ($jfr.maxChunkSize | toString) $runs $needed ($jfr.persistence.size | toString) $volume) -}} +{{- end -}} +{{- end -}} +{{- end -}} + +{{/* +JVM arguments that start the continuous recording. Appended to JAVA_OPTS. + +Sizes are rendered as byte counts, which JFR accepts, so that the chart can expose Kubernetes +quantities everywhere and never make the user think about JFR's own unit table. + +Call as: include "pinot.jfr.javaOpts" (dict "ctx" . "role" .Values.server) + +`preserve-repository` tracks whether this role's recordings land on a PersistentVolume. There it is +the whole point: +without it JFR deletes the repository on a clean shutdown, and a clean shutdown is what Kubernetes +asks for first. On an emptyDir it would be actively harmful — the volume outlives an in-place +container restart but the janitor does not run then, so preserved repositories would pile up with +nothing able to reclaim them. + +`dumponexit=false` is deliberate. The repository is already the durable copy, and a dump on exit +only produces a second one; after a SIGKILL there is no exit hook to run anyway. Use +`jfr assemble out.jfr` to turn a leftover repository into a single recording, +including the chunk that was open when the JVM died. +*/}} +{{- define "pinot.jfr.javaOpts" -}} +{{- include "pinot.jfr.validate" .ctx -}} +{{- $jfr := .ctx.Values.jfr -}} +{{- $preserve := include "pinot.jfr.usesPersistentVolume" . -}} +{{- $recording := list + (printf "name=%s" $jfr.recordingName) + (printf "settings=%s" $jfr.configuration) + "disk=true" + (printf "maxsize=%s" (include "pinot.jfr.sizeToBytes" $jfr.maxSize)) + "dumponexit=false" -}} +{{- if $jfr.maxAge -}} +{{- $recording = append $recording (printf "maxage=%s" ($jfr.maxAge | toString)) -}} +{{- end -}} +{{- $options := list + (printf "repository=%s" $jfr.mountPath) + (printf "preserve-repository=%t" (eq $preserve "true")) + (printf "maxchunksize=%s" (include "pinot.jfr.sizeToBytes" $jfr.maxChunkSize)) -}} +{{- printf "-XX:FlightRecorderOptions=%s -XX:StartFlightRecording=%s" (join "," $options) (join "," $recording) -}} +{{- end -}} + +{{/* +Mount for the JFR repository. A volume of its own, never a subdirectory of the role's data volume, +so a recording can never compete with segments for space. +*/}} +{{- define "pinot.jfr.volumeMount" -}} +- name: jfr + mountPath: {{ .Values.jfr.mountPath | quote }} +{{- end -}} + +{{/* +Pod-level JFR volume, used when the recording is not kept on a PersistentVolume. With persistence +enabled the StatefulSet roles get theirs from volumeClaimTemplates instead. + +The emptyDir is always given a sizeLimit so that a runaway recording is the kubelet's problem rather +than the node's: without one it draws on shared node ephemeral storage and can trigger DiskPressure +evictions of unrelated pods. +*/}} +{{- define "pinot.jfr.emptyDirVolume" -}} +{{- $jfr := .Values.jfr -}} +{{- /* takes the root context: the sizeLimit does not depend on the role */ -}} +{{- $limit := $jfr.persistence.emptyDirSizeLimit -}} +{{- if not $limit -}} +{{- $run := include "pinot.jfr.sizeToBytes" $jfr.maxSize | int64 -}} +{{- $chunk := include "pinot.jfr.sizeToBytes" $jfr.maxChunkSize | int64 -}} +{{- $limit = printf "%d" (add $run (mul $chunk 2)) -}} +{{- end -}} +- name: jfr + emptyDir: + sizeLimit: {{ $limit | quote }} +{{- end -}} + +{{/* +volumeClaimTemplates entry for the JFR repository. +*/}} +{{- define "pinot.jfr.volumeClaimTemplate" -}} +- metadata: + name: jfr + spec: + accessModes: + - {{ .Values.jfr.persistence.accessMode | quote }} + {{- if .Values.jfr.persistence.storageClass }} + {{- if (eq "-" .Values.jfr.persistence.storageClass) }} + storageClassName: "" + {{- else }} + storageClassName: {{ .Values.jfr.persistence.storageClass }} + {{- end }} + {{- end }} + resources: + requests: + storage: {{ .Values.jfr.persistence.size }} +{{- end -}} + +{{/* +The role's initContainers list: the JFR janitor (when it applies) followed by whatever the user +configured in .initContainers. + +Call as: include "pinot.jfr.initContainers" (dict "ctx" . "role" .Values.server) + +Renders nothing at all when there is neither, so that roles without init containers keep producing +exactly the manifest they did before - including no stray blank line, which is why the pod-spec +indentation is baked in here rather than applied with `nindent` at the call site. +*/}} +{{- define "pinot.jfr.initContainers" -}} +{{- $extra := .role.initContainers | default list -}} +{{- $janitor := include "pinot.jfr.janitorEnabled" . -}} +{{- if or $janitor $extra }} + initContainers: + {{- if $janitor }} + {{- include "pinot.jfr.janitorInitContainer" .ctx | nindent 8 }} + {{- end }} + {{- if $extra }} + {{- toYaml $extra | nindent 8 }} + {{- end }} +{{- end }} +{{- end -}} + +{{/* +Init container that reclaims JFR repositories left behind by previous JVM runs. + +JFR's own `maxsize` bounds the repository of the JVM that is running; nothing in the JVM ever +reclaims the repository of a JVM that has already exited. On a PersistentVolume those directories +survive every restart and accumulate without limit, so the reclaiming has to happen outside the JVM. + +Running it as an init container is what makes it safe: init containers finish before the Pinot +container starts, so on a volume owned by a single pod every directory belongs to a run that is +already over. The script does not rely on that alone — it also refuses to delete a repository that +was written to within jfr.janitor.minIdleMinutes. + +Sizes and durations are converted here and passed as plain integers, so the script parses no units +and there is one source of truth for the unit table. +*/}} +{{- define "pinot.jfr.janitorInitContainer" -}} +{{- $jfr := .Values.jfr -}} +- name: jfr-janitor + image: "{{ $jfr.janitor.image.repository | default .Values.image.repository }}:{{ $jfr.janitor.image.tag | default .Values.image.tag }}" + imagePullPolicy: {{ $jfr.janitor.image.pullPolicy | default .Values.image.pullPolicy }} + {{- with $jfr.janitor.securityContext }} + securityContext: + {{- toYaml . | nindent 4 }} + {{- end }} + env: + - name: PINOT_JFR_REPOSITORY + value: {{ $jfr.mountPath | quote }} + - name: PINOT_JFR_JANITOR_MAX_AGE_MINUTES + value: {{ if $jfr.janitor.maxAge }}{{ include "pinot.jfr.durationToMinutes" $jfr.janitor.maxAge | quote }}{{ else }}""{{ end }} + - name: PINOT_JFR_JANITOR_MAX_TOTAL_KIB + value: {{ if $jfr.janitor.maxTotalSize }}{{ div (include "pinot.jfr.sizeToBytes" $jfr.janitor.maxTotalSize | int64) 1024 | quote }}{{ else }}""{{ end }} + - name: PINOT_JFR_JANITOR_MIN_IDLE_MINUTES + value: {{ $jfr.janitor.minIdleMinutes | toString | quote }} + command: + - /bin/sh + - -c + - | + {{- .Files.Get "scripts/jfr-janitor.sh" | nindent 6 }} + volumeMounts: + {{- include "pinot.jfr.volumeMount" . | nindent 4 }} + {{- with $jfr.janitor.resources }} + resources: + {{- toYaml . | nindent 4 }} + {{- end }} +{{- end -}} diff --git a/helm/pinot/templates/broker/statefulset.yaml b/helm/pinot/templates/broker/statefulset.yaml index 22ea8e49a0d5..7feda29976c0 100644 --- a/helm/pinot/templates/broker/statefulset.yaml +++ b/helm/pinot/templates/broker/statefulset.yaml @@ -59,8 +59,7 @@ spec: {{ toYaml .Values.broker.affinity | indent 8 }} tolerations: {{ toYaml .Values.broker.tolerations | indent 8 }} - initContainers: -{{ toYaml .Values.broker.initContainers | indent 8 }} +{{- include "pinot.jfr.initContainers" (dict "ctx" . "role" .Values.broker) }} containers: - name: broker securityContext: @@ -75,7 +74,7 @@ spec: ] env: - name: JAVA_OPTS - value: "{{ .Values.broker.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.broker.log4j2ConfFile }} -Dplugins.dir={{ .Values.broker.pluginsDir }}" + value: "{{ .Values.broker.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.broker.log4j2ConfFile }} -Dplugins.dir={{ .Values.broker.pluginsDir }}{{ if .Values.broker.jfr.enabled }} {{ include "pinot.jfr.javaOpts" (dict "ctx" . "role" .Values.broker) }}{{ end }}" {{- if .Values.broker.extraEnv }} {{ toYaml .Values.broker.extraEnv | indent 10 }} {{- end }} @@ -91,6 +90,9 @@ spec: volumeMounts: - name: config mountPath: /var/pinot/broker/config + {{- if .Values.broker.jfr.enabled }} + {{- include "pinot.jfr.volumeMount" . | nindent 10 }} + {{- end }} {{- if ne (len .Values.broker.persistence.extraVolumeMounts) 0 }} {{ toYaml .Values.broker.persistence.extraVolumeMounts | indent 10 }} {{- end }} @@ -134,6 +136,13 @@ spec: - name: config configMap: name: {{ include "pinot.broker.config" . }} + {{- if and .Values.broker.jfr.enabled (not .Values.jfr.persistence.enabled) }} + {{- include "pinot.jfr.emptyDirVolume" . | nindent 8 }} + {{- end }} {{- if ne (len .Values.broker.persistence.extraVolumes) 0 }} {{ toYaml .Values.broker.persistence.extraVolumes | indent 8 }} {{- end }} + {{- if and .Values.broker.jfr.enabled .Values.jfr.persistence.enabled }} + volumeClaimTemplates: + {{- include "pinot.jfr.volumeClaimTemplate" . | nindent 4 }} + {{- end }} diff --git a/helm/pinot/templates/controller/statefulset.yaml b/helm/pinot/templates/controller/statefulset.yaml index b2403767f066..7cede72dead9 100644 --- a/helm/pinot/templates/controller/statefulset.yaml +++ b/helm/pinot/templates/controller/statefulset.yaml @@ -59,8 +59,7 @@ spec: {{ toYaml .Values.controller.affinity | indent 8 }} tolerations: {{ toYaml .Values.controller.tolerations | indent 8 }} - initContainers: -{{ toYaml .Values.controller.initContainers | indent 8 }} +{{- include "pinot.jfr.initContainers" (dict "ctx" . "role" .Values.controller) }} containers: - name: controller securityContext: @@ -70,7 +69,7 @@ spec: args: [ "{{ .Values.controller.startCommand }}", "-configFileName", "/var/pinot/controller/config/pinot-controller.conf" ] env: - name: JAVA_OPTS - value: "{{ .Values.controller.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.controller.log4j2ConfFile }} -Dplugins.dir={{ .Values.controller.pluginsDir }}" + value: "{{ .Values.controller.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.controller.log4j2ConfFile }} -Dplugins.dir={{ .Values.controller.pluginsDir }}{{ if .Values.controller.jfr.enabled }} {{ include "pinot.jfr.javaOpts" (dict "ctx" . "role" .Values.controller) }}{{ end }}" {{- if .Values.controller.extraEnv }} {{ toYaml .Values.controller.extraEnv | indent 10 }} {{- end }} @@ -121,6 +120,9 @@ spec: mountPath: /var/pinot/controller/config - name: data mountPath: "{{ .Values.controller.persistence.mountPath }}" + {{- if .Values.controller.jfr.enabled }} + {{- include "pinot.jfr.volumeMount" . | nindent 10 }} + {{- end }} {{- if ne (len .Values.controller.persistence.extraVolumeMounts) 0 }} {{ toYaml .Values.controller.persistence.extraVolumeMounts | indent 10 }} {{- end }} @@ -135,11 +137,16 @@ spec: - name: data emptyDir: {} {{- end }} + {{- if and .Values.controller.jfr.enabled (not .Values.jfr.persistence.enabled) }} + {{- include "pinot.jfr.emptyDirVolume" . | nindent 6 }} + {{- end }} {{- if ne (len .Values.controller.persistence.extraVolumes) 0 }} {{ toYaml .Values.controller.persistence.extraVolumes | indent 6 }} {{- end }} -{{- if .Values.controller.persistence.enabled }} +{{- $jfrClaim := and .Values.controller.jfr.enabled .Values.jfr.persistence.enabled }} +{{- if or .Values.controller.persistence.enabled $jfrClaim }} volumeClaimTemplates: + {{- if .Values.controller.persistence.enabled }} - metadata: name: data spec: @@ -155,4 +162,8 @@ spec: resources: requests: storage: {{ .Values.controller.persistence.size | quote}} -{{ end }} + {{- end }} + {{- if $jfrClaim }} + {{- include "pinot.jfr.volumeClaimTemplate" . | nindent 4 }} + {{- end }} +{{- end }} diff --git a/helm/pinot/templates/minion-stateless/deployment.yaml b/helm/pinot/templates/minion-stateless/deployment.yaml index 9d59bd918f7b..b91da9418f4a 100644 --- a/helm/pinot/templates/minion-stateless/deployment.yaml +++ b/helm/pinot/templates/minion-stateless/deployment.yaml @@ -51,8 +51,7 @@ spec: {{ toYaml .Values.minionStateless.affinity | indent 8 }} tolerations: {{ toYaml .Values.minionStateless.tolerations | indent 8 }} - initContainers: -{{ toYaml .Values.minionStateless.initContainers | indent 8 }} +{{- include "pinot.jfr.initContainers" (dict "ctx" . "role" .Values.minionStateless "forceEmptyDir" true) }} containers: - name: minion-stateless securityContext: @@ -67,7 +66,7 @@ spec: ] env: - name: JAVA_OPTS - value: "{{ .Values.minionStateless.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.minionStateless.log4j2ConfFile }} -Dplugins.dir={{ .Values.minionStateless.pluginsDir }}" + value: "{{ .Values.minionStateless.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.minionStateless.log4j2ConfFile }} -Dplugins.dir={{ .Values.minionStateless.pluginsDir }}{{ if .Values.minionStateless.jfr.enabled }} {{ include "pinot.jfr.javaOpts" (dict "ctx" . "role" .Values.minionStateless "forceEmptyDir" true) }}{{ end }}" {{- if .Values.minionStateless.extraEnv }} {{ toYaml .Values.minionStateless.extraEnv | indent 10 }} {{- end }} @@ -120,6 +119,9 @@ spec: - name: data mountPath: "{{ .Values.minionStateless.persistence.mountPath }}" {{- end }} + {{- if .Values.minionStateless.jfr.enabled }} + {{- include "pinot.jfr.volumeMount" . | nindent 10 }} + {{- end }} {{- if ne (len .Values.minionStateless.persistence.extraVolumeMounts) 0 }} {{ toYaml .Values.minionStateless.persistence.extraVolumeMounts | indent 10 }} {{- end }} @@ -139,6 +141,20 @@ spec: persistentVolumeClaim: claimName: {{ .Values.minionStateless.persistence.pvcName }} {{- end }} + {{- if .Values.minionStateless.jfr.enabled }} + {{- /* + Always an emptyDir, even when jfr.persistence.enabled is true. + + The stateless minion is a Deployment, so it has no volumeClaimTemplates and every replica + would have to share one claim. That breaks the property the janitor depends on: it assumes + that when it runs, every repository on the volume belongs to a JVM that has already exited. + With a shared claim, a starting pod's janitor would see the repository of a *running* pod — + and during a rolling update (default maxSurge) or with replicaCount > 1 that is the normal + case, not an edge case. A single ReadWriteOnce claim would also stall the rollout with a + Multi-Attach error. See helm/pinot/README.md. + */}} + {{- include "pinot.jfr.emptyDirVolume" . | nindent 8 }} + {{- end }} {{- if ne (len .Values.minionStateless.persistence.extraVolumes) 0 }} {{ toYaml .Values.minionStateless.persistence.extraVolumes | indent 8 }} {{- end }} diff --git a/helm/pinot/templates/minion/statefulset.yaml b/helm/pinot/templates/minion/statefulset.yaml index e8d2a786edd0..cc9a5942df25 100644 --- a/helm/pinot/templates/minion/statefulset.yaml +++ b/helm/pinot/templates/minion/statefulset.yaml @@ -60,8 +60,7 @@ spec: {{ toYaml .Values.minion.affinity | indent 8 }} tolerations: {{ toYaml .Values.minion.tolerations | indent 8 }} - initContainers: -{{ toYaml .Values.minion.initContainers | indent 8 }} +{{- include "pinot.jfr.initContainers" (dict "ctx" . "role" .Values.minion) }} containers: - name: minion securityContext: @@ -76,7 +75,7 @@ spec: ] env: - name: JAVA_OPTS - value: "{{ .Values.minion.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.minion.log4j2ConfFile }} -Dplugins.dir={{ .Values.minion.pluginsDir }}" + value: "{{ .Values.minion.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.minion.log4j2ConfFile }} -Dplugins.dir={{ .Values.minion.pluginsDir }}{{ if .Values.minion.jfr.enabled }} {{ include "pinot.jfr.javaOpts" (dict "ctx" . "role" .Values.minion) }}{{ end }}" {{- if .Values.minion.extraEnv }} {{ toYaml .Values.minion.extraEnv | indent 10 }} {{- end }} @@ -129,6 +128,9 @@ spec: - name: data mountPath: "{{ .Values.minion.persistence.mountPath }}" {{- end }} + {{- if .Values.minion.jfr.enabled }} + {{- include "pinot.jfr.volumeMount" . | nindent 10 }} + {{- end }} {{- if ne (len .Values.minion.persistence.extraVolumeMounts) 0 }} {{ toYaml .Values.minion.persistence.extraVolumeMounts | indent 10 }} {{- end }} @@ -143,11 +145,16 @@ spec: - name: data emptyDir: {} {{- end }} + {{- if and .Values.minion.jfr.enabled (not .Values.jfr.persistence.enabled) }} + {{- include "pinot.jfr.emptyDirVolume" . | nindent 8 }} + {{- end }} {{- if ne (len .Values.minion.persistence.extraVolumes) 0 }} {{ toYaml .Values.minion.persistence.extraVolumes | indent 8 }} {{- end }} - {{- if .Values.minion.persistence.enabled }} + {{- $jfrClaim := and .Values.minion.jfr.enabled .Values.jfr.persistence.enabled }} + {{- if or .Values.minion.persistence.enabled $jfrClaim }} volumeClaimTemplates: + {{- if .Values.minion.persistence.enabled }} - metadata: name: data spec: @@ -163,5 +170,9 @@ spec: resources: requests: storage: {{ .Values.minion.persistence.size }} - {{ end }} + {{- end }} + {{- if $jfrClaim }} + {{- include "pinot.jfr.volumeClaimTemplate" . | nindent 4 }} + {{- end }} + {{- end }} {{- end }} diff --git a/helm/pinot/templates/server/statefulset.yaml b/helm/pinot/templates/server/statefulset.yaml index 724d0a4a20cc..673ee536690d 100644 --- a/helm/pinot/templates/server/statefulset.yaml +++ b/helm/pinot/templates/server/statefulset.yaml @@ -59,8 +59,7 @@ spec: {{ toYaml .Values.server.affinity | indent 8 }} tolerations: {{ toYaml .Values.server.tolerations | indent 8 }} - initContainers: -{{ toYaml .Values.server.initContainers | indent 8 }} +{{- include "pinot.jfr.initContainers" (dict "ctx" . "role" .Values.server) }} containers: - name: server securityContext: @@ -75,7 +74,7 @@ spec: ] env: - name: JAVA_OPTS - value: "{{ .Values.server.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.server.log4j2ConfFile }} -Dplugins.dir={{ .Values.server.pluginsDir }}" + value: "{{ .Values.server.jvmOpts }} -Dlog4j2.configurationFile={{ .Values.server.log4j2ConfFile }} -Dplugins.dir={{ .Values.server.pluginsDir }}{{ if .Values.server.jfr.enabled }} {{ include "pinot.jfr.javaOpts" (dict "ctx" . "role" .Values.server) }}{{ end }}" {{- if .Values.server.extraEnv }} {{ toYaml .Values.server.extraEnv | indent 10 }} {{- end}} @@ -129,6 +128,9 @@ spec: mountPath: /var/pinot/server/config - name: data mountPath: "{{ .Values.server.persistence.mountPath }}" + {{- if .Values.server.jfr.enabled }} + {{- include "pinot.jfr.volumeMount" . | nindent 10 }} + {{- end }} {{- if ne (len .Values.server.persistence.extraVolumeMounts) 0 }} {{ toYaml .Values.server.persistence.extraVolumeMounts | indent 10 }} {{- end }} @@ -143,11 +145,16 @@ spec: - name: data emptyDir: {} {{- end }} + {{- if and .Values.server.jfr.enabled (not .Values.jfr.persistence.enabled) }} + {{- include "pinot.jfr.emptyDirVolume" . | nindent 8 }} + {{- end }} {{- if ne (len .Values.server.persistence.extraVolumes) 0 }} {{ toYaml .Values.server.persistence.extraVolumes | indent 8 }} {{- end }} - {{- if .Values.server.persistence.enabled }} + {{- $jfrClaim := and .Values.server.jfr.enabled .Values.jfr.persistence.enabled }} + {{- if or .Values.server.persistence.enabled $jfrClaim }} volumeClaimTemplates: + {{- if .Values.server.persistence.enabled }} - metadata: name: data spec: @@ -163,4 +170,8 @@ spec: resources: requests: storage: {{ .Values.server.persistence.size }} - {{ end }} + {{- end }} + {{- if $jfrClaim }} + {{- include "pinot.jfr.volumeClaimTemplate" . | nindent 4 }} + {{- end }} + {{- end }} diff --git a/helm/pinot/values.yaml b/helm/pinot/values.yaml index 735698522eee..40cc21079667 100644 --- a/helm/pinot/values.yaml +++ b/helm/pinot/values.yaml @@ -71,6 +71,126 @@ serviceAccount: additionalMatchLabels: {} +# ------------------------------------------------------------------------------ +# Java Flight Recorder (JFR): +# ------------------------------------------------------------------------------ +# Runs a continuous JFR recording in each Pinot JVM, so that when something goes wrong the +# profile of the minutes leading up to it is already on disk. The recording rolls like a log +# file: once it reaches `maxSize` the oldest data is dropped. +# +# Started by the JVM through -XX:StartFlightRecording in JAVA_OPTS, so it covers startup too. +# +# Enable it per role with `.jfr.enabled`. Everything under this block is shared by every +# role that enables it. +jfr: + # Event settings profile. `default` targets ~1% overhead and is the right choice for + # always-on recording; `profile` collects considerably more (and costs more). A path to + # a custom .jfc file also works, as long as that file exists inside the container. + configuration: default + + # Name of the recording. Used by `jcmd JFR.dump name= filename=...`. + recordingName: pinot + + # How much recording data to keep for the current JVM run. Steady-state disk use is about + # `maxSize + 2 * maxChunkSize`. + # + # Recommended range: 1Gi to 4Gi with `configuration: default`. How far back that reaches + # depends on the workload — GC frequency above all — so rather than guessing an event rate, + # pick the disk you can afford and read the window you got from `jfr summary` (it prints + # Start and Duration). Below ~512Mi a busy server keeps only minutes; above ~8Gi you are + # usually better off with `configuration: profile` at a smaller size. + # + # Kubernetes units, like every size in this chart: `2Gi` is 2^30, `2G` is 10^9. + maxSize: 2Gi + + # How much wall-clock history to keep for the current run, e.g. `12h` or `7d`. + # Leave empty to bound the recording by size alone, which is usually what you want: you + # pick the disk budget you can afford and let the time window fall out of the event rate + # rather than guessing the event rate up front. + maxAge: "" + + # Size of an individual chunk file in the repository. Chunks are the unit of eviction and + # the unit of loss: a SIGKILL leaves the open chunk incomplete (`jfr assemble` recovers + # it). Smaller chunks mean finer-grained eviction, larger chunks mean slightly less + # overhead. Kubernetes units. + maxChunkSize: 12Mi + + # Where the JFR repository is mounted inside the container. + # + # This is deliberately a volume of its own rather than a subdirectory of the role's data + # volume, so that a runaway recording can never eat the space Pinot needs for segments. + mountPath: /var/pinot/jfr + + persistence: + # Keep recordings on a PersistentVolume so they survive the pod being rescheduled. + # + # Off by default because it cannot be turned on in place: it adds an entry to the + # StatefulSet's volumeClaimTemplates, which Kubernetes forbids changing on a deployed + # workload. See helm/pinot/UPGRADING.md for the one-time procedure. It also provisions one + # volume of `size` per pod, so 50 servers at 10Gi is 500Gi. + # + # With this off, recordings go to an emptyDir: they survive a container restart but not the + # pod being rescheduled, and no cleanup init container is needed. That is enough for "turn + # on profiling now and look at it soon"; turn it on for post-mortem after a node loss. + # + # Ignored by minionStateless, which is a Deployment and always uses an emptyDir. + enabled: false + accessMode: ReadWriteOnce + size: 10Gi + + # How many in-place container restarts to leave room for. Kubernetes re-runs init containers + # only when the *pod* is recreated, so a process that restarts in place (an OOMKill, say) + # strands its repository until the next pod-level restart. The chart refuses to render unless + # `janitor.maxTotalSize + (1 + restartHeadroom) * (maxSize + 2 * maxChunkSize)` fits in `size`. + # + # Raise it if a role restarts in place often. If the volume does fill, the JVM cannot create + # its repository and fails to start; recovery is `kubectl delete pod`. + restartHeadroom: 1 + # storageClass: "-" + storageClass: "" + # sizeLimit for the emptyDir used when persistence.enabled is false. Left empty the chart derives + # `maxSize + 2 * maxChunkSize`, so a runaway recording is capped by the kubelet rather than + # eating shared node ephemeral storage and evicting unrelated pods. + emptyDirSizeLimit: "" + + # Init container that reclaims the repositories previous JVM runs left behind. JFR bounds the + # repository of the JVM that is running but never reclaims one belonging to a JVM that exited, + # so on a PersistentVolume they accumulate on every restart until the volume is full. + # + # Only applies when persistence.enabled is true; an emptyDir starts empty, so there is nothing + # to reclaim. + janitor: + enabled: true + + # Delete repositories left by runs older than this. Accepts `d`, `h` or `m`. + # Leave empty to skip the age-based pass. + maxAge: 7d + + # After the age pass, if the repository directory is still larger than this, delete the + # oldest leftover repositories until it fits. Leave empty to skip the size-based pass. + # + # Kubernetes units, like every other size in this chart. + # + # This has to leave room for the runs that follow the janitor; see persistence.restartHeadroom. + maxTotalSize: 4Gi + + # Never delete a repository written to this recently, even if it is over budget or aged out. + # JFR flushes at least once a second, so anything idle this long belongs to a JVM that is gone. + minIdleMinutes: 15 + + # Image used to run the cleanup. Defaults to the Pinot image, which is already present + # on the node; override only if you want a smaller one. + image: + repository: "" + tag: "" + pullPolicy: "" + + # Defaults to the Pinot image's own user. Set this if the role runs with a non-default UID and + # the cleanup needs to match it to write to the volume. + securityContext: {} + + resources: {} + pinotAuth: enabled: false @@ -144,6 +264,11 @@ controller: jvmOpts: "-XX:ActiveProcessorCount=2 -Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -Xlog:gc*:file=/opt/pinot/gc-pinot-controller.log -Djute.maxbuffer=4000000" + # Continuous Java Flight Recorder recording for this role. + # Settings live under the top-level `jfr` block and are shared by all roles. + jfr: + enabled: false + log4j2ConfFile: /opt/pinot/etc/conf/pinot-controller-log4j2.xml pluginsDir: /opt/pinot/plugins @@ -253,6 +378,11 @@ broker: jvmOpts: "-XX:ActiveProcessorCount=2 -Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -Xlog:gc*:file=/opt/pinot/gc-pinot-broker.log -Djute.maxbuffer=4000000" + # Continuous Java Flight Recorder recording for this role. + # Settings live under the top-level `jfr` block and are shared by all roles. + jfr: + enabled: false + log4j2ConfFile: /opt/pinot/etc/conf/pinot-broker-log4j2.xml pluginsDir: /opt/pinot/plugins @@ -439,6 +569,11 @@ server: jvmOpts: "-Xms512M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -Xlog:gc*:file=/opt/pinot/gc-pinot-server.log -Djute.maxbuffer=4000000" + # Continuous Java Flight Recorder recording for this role. + # Settings live under the top-level `jfr` block and are shared by all roles. + jfr: + enabled: false + log4j2ConfFile: /opt/pinot/etc/conf/pinot-server-log4j2.xml pluginsDir: /opt/pinot/plugins @@ -558,6 +693,11 @@ minion: dataDir: /var/pinot/minion/data jvmOpts: "-XX:ActiveProcessorCount=2 -Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -Xlog:gc*:file=/opt/pinot/gc-pinot-minion.log -Djute.maxbuffer=4000000" + # Continuous Java Flight Recorder recording for this role. + # Settings live under the top-level `jfr` block and are shared by all roles. + jfr: + enabled: false + log4j2ConfFile: /opt/pinot/etc/conf/pinot-minion-log4j2.xml pluginsDir: /opt/pinot/plugins @@ -680,6 +820,15 @@ minionStateless: dataDir: /var/pinot/minion/data jvmOpts: "-XX:ActiveProcessorCount=2 -Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -Xlog:gc*:file=/opt/pinot/gc-pinot-minion.log -Djute.maxbuffer=4000000" + # Continuous Java Flight Recorder recording for this role. + # Settings live under the top-level `jfr` block and are shared by all roles. + # + # Being a Deployment, this role always uses an emptyDir regardless of jfr.persistence.enabled: + # replicas cannot each have their own volume, and sharing one would let a starting pod's + # cleanup delete a running pod's recording. + jfr: + enabled: false + log4j2ConfFile: /opt/pinot/etc/conf/pinot-minion-log4j2.xml pluginsDir: /opt/pinot/plugins diff --git a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java index 5ed947953e35..f091927aaa3d 100644 --- a/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java +++ b/pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java @@ -676,6 +676,8 @@ public void start() _brokerMetrics.addTimedValue(BrokerTimer.STARTUP_SUCCESS_DURATION_MS, System.currentTimeMillis() - startTimeMs, TimeUnit.MILLISECONDS); + // Deprecated: kept so existing `pinot.jfr.*` cluster configs keep working. New deployments should + // start JFR with JVM arguments instead; see ContinuousJfrStarter for the replacement. _clusterConfigChangeHandler.registerClusterConfigChangeListener(ContinuousJfrStarter.INSTANCE); _clusterConfigChangeHandler.registerClusterConfigChangeListener(_serverRoutingStatsManager); diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java index 9bf65e9489d7..c649a66cb26d 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java @@ -816,6 +816,8 @@ protected void configure() { _serviceStatusCallbackList.add(generateResourceUtilizationCheckerStatusCallback()); } + // Deprecated: kept so existing `pinot.jfr.*` cluster configs keep working. New deployments should + // start JFR with JVM arguments instead; see ContinuousJfrStarter for the replacement. _clusterConfigChangeHandler.registerClusterConfigChangeListener(ContinuousJfrStarter.INSTANCE); _clusterConfigChangeHandler.registerClusterConfigChangeListener( ConsumingSegmentConsistencyModeListener.getInstance()); diff --git a/pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java b/pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java index 09df1b8c6ce1..6a489dc8cfcd 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java @@ -39,6 +39,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import javax.management.MBeanServer; @@ -52,8 +53,52 @@ import org.slf4j.LoggerFactory; +/// Starts and manages a continuous JFR recording driven by the `pinot.jfr.*` cluster configs. +/// +/// @deprecated Configure JFR with JVM arguments instead, and let the JVM own the recording: +/// +/// ``` +/// -XX:FlightRecorderOptions=repository=/var/pinot/jfr,preserve-repository=true,maxchunksize=12M +/// -XX:StartFlightRecording=name=pinot,settings=default,disk=true,maxsize=2147483648,dumponexit=false +/// ``` +/// +/// The Helm chart renders exactly these flags: set `.jfr.enabled` and tune the shared +/// `jfr` block. Starting the recording from outside the JVM is strictly better than starting it +/// from here: +/// +/// - It records from the first instruction. This class cannot start before the component has +/// connected to Helix and read cluster config, so everything up to that point — class +/// loading, plugin init, segment preload, the ZooKeeper connect itself — is never captured. +/// - It does not depend on ZooKeeper being reachable, which is not a safe assumption in exactly +/// the incidents a profile would help with. +/// - It cannot lose history. `JFR.stop` without a filename deletes the whole repository, so any +/// change to a `pinot.jfr.*` key here silently discards every recorded chunk. +/// +/// Note that JFR bounds the repository of the *running* JVM but never reclaims the repository of a +/// JVM that already exited, so on a persistent volume something outside the JVM has to delete +/// them. The Helm chart runs an init container for that; it can do so safely because init +/// containers finish before the Pinot process starts, which means every directory it sees belongs +/// to a run that is already over. +/// +/// A recording started by `-XX:StartFlightRecording` remains fully controllable at runtime through +/// `jcmd` (`JFR.dump`, `JFR.check`) and through the `jdk.jfr` API, so nothing is given up by +/// moving the flags out of cluster config. +/// +/// Thread safety: every mutation goes through `GLOBAL_RECORDING_LOCK`, but the state is split. +/// `GLOBAL_RECORDINGS` is static, because JFR recordings belong to the JVM rather than to any one +/// instance; `_running`, `_recordingName` and `_currentConfig` are per-instance, which is what makes +/// the reference counting in `GLOBAL_RECORDINGS` meaningful. The one exception to the lock is the +/// `_currentConfig` short-circuit at the top of [#onChange], which is an intentionally racy fast +/// path - it is re-checked under the lock before anything is acted on. In production [#INSTANCE] is +/// the only instance: every component registers that same object as a config listener. +@Deprecated(since = "1.6.0", forRemoval = true) +// Suppresses warnings for this class's own use of the deprecated CommonConstants.JFR prefix. Call sites outside +// this deprecated subsystem are deliberately left warning, so they surface when the removal happens. +@SuppressWarnings("removal") public class ContinuousJfrStarter implements PinotClusterConfigChangeListener { private static final Logger LOGGER = LoggerFactory.getLogger(ContinuousJfrStarter.class); + private static final AtomicBoolean DEPRECATION_WARNING_LOGGED = new AtomicBoolean(); + private static final AtomicBoolean JVM_ARGUMENT_WARNING_LOGGED = new AtomicBoolean(); private static final String JFR_CONFIGURE_COMMAND = "jfrConfigure"; private static final String JFR_START_COMMAND = "jfrStart"; private static final String JFR_STOP_COMMAND = "jfrStop"; @@ -217,12 +262,40 @@ public boolean isRunning() { protected static void resetGlobalStateForTesting() { synchronized (GLOBAL_RECORDING_LOCK) { GLOBAL_RECORDINGS.clear(); + DEPRECATION_WARNING_LOGGED.set(false); + JVM_ARGUMENT_WARNING_LOGGED.set(false); } } @GuardedBy("GLOBAL_RECORDING_LOCK") private boolean applyConfig(PinotConfiguration subset, Map newSubsetMap) { boolean enabled = subset.getProperty(ENABLED, DEFAULT_ENABLED); + if (enabled && DEPRECATION_WARNING_LOGGED.compareAndSet(false, true)) { + LOGGER.warn("The '{}.*' cluster configs are deprecated and will be removed. Start JFR with JVM arguments " + + "instead, e.g. -XX:FlightRecorderOptions=repository=,preserve-repository=true and " + + "-XX:StartFlightRecording=name={},settings=default,disk=true,maxsize=,dumponexit=false. " + + "The Helm chart renders these from the 'jfr' values block. Note that JFR does not reclaim " + + "repositories left by previous JVM runs, so schedule cleanup outside the JVM", + CommonConstants.JFR, subset.getProperty(NAME, DEFAULT_NAME)); + } + if (enabled && isRecordingConfiguredByJvmArgument()) { + // Both mechanisms drive the same per-JVM recorder, and this one would win destructively: + // applyRuntimeOptions issues `JFR.configure repositorypath=...`, which is JVM-global and + // relocates the recording the JVM already started - typically off the volume it was meant to + // be written to. Stand down and leave the JVM's own recording alone. + if (JVM_ARGUMENT_WARNING_LOGGED.compareAndSet(false, true)) { + LOGGER.warn("Ignoring the deprecated '{}.*' cluster configs because this JVM was started with " + + "-XX:StartFlightRecording or -XX:FlightRecorderOptions, which already own the flight recorder. " + + "Remove the '{}.*' cluster configs to silence this message", + CommonConstants.JFR, CommonConstants.JFR); + } else { + // The warning is latched, so say something per ignored change rather than going silent. + LOGGER.info("Ignoring a change to the deprecated '{}.*' cluster configs; the flight recorder is owned by " + + "JVM arguments", CommonConstants.JFR); + } + _currentConfig = newSubsetMap; + return true; + } if (!enabled) { if (!releaseRecordingReference()) { return false; @@ -563,6 +636,32 @@ private static void deletePathRecursively(Path path) { } } + /// Whether the flight recorder is configured by JVM arguments, in which case the JVM owns it and + /// this class must not touch it. + /// + /// Deliberately reads the JVM's input arguments rather than asking [jdk.jfr.FlightRecorder] which + /// recordings exist: recordings this class started itself would also show up there, and the two + /// cases need different handling. + @VisibleForTesting + protected boolean isRecordingConfiguredByJvmArgument() { + return isRecordingConfiguredByJvmArgument(ManagementFactory.getRuntimeMXBean().getInputArguments()); + } + + /// Both flags matter, not just the one that starts a recording. `-XX:FlightRecorderOptions` is + /// what sets `repositorypath`, so a JVM carrying only that flag is still one whose repository + /// would be relocated by [#applyRuntimeOptions] - the very thing the caller is guarding against. + /// + /// Matched by prefix because both flags accept `=` and `:` separators. + @VisibleForTesting + static boolean isRecordingConfiguredByJvmArgument(List jvmArguments) { + for (String argument : jvmArguments) { + if (argument.startsWith("-XX:StartFlightRecording") || argument.startsWith("-XX:FlightRecorderOptions")) { + return true; + } + } + return false; + } + @VisibleForTesting protected boolean isDiagnosticCommandAvailable() { return _mBeanServer != null && _diagnosticCommandObjectName != null diff --git a/pinot-core/src/test/java/org/apache/pinot/core/util/trace/ContinuousJfrStarterTest.java b/pinot-core/src/test/java/org/apache/pinot/core/util/trace/ContinuousJfrStarterTest.java index 7c16533cf74c..5fef643e63a4 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/util/trace/ContinuousJfrStarterTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/util/trace/ContinuousJfrStarterTest.java @@ -33,9 +33,12 @@ import org.assertj.core.api.Assertions; import org.testng.SkipException; import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; +/// Covers the deprecated `pinot.jfr.*` cluster config path, which is kept working until it is removed. +@SuppressWarnings("removal") public class ContinuousJfrStarterTest { private static final long DEFAULT_MAX_SIZE_BYTES = DataSizeUtils.toBytes(ContinuousJfrStarter.DEFAULT_MAX_SIZE); private static final long DEFAULT_MAX_AGE_MILLIS = 7L * 24 * 60 * 60 * 1000; @@ -420,6 +423,65 @@ private static boolean waitForRecordingPresence(String recordingName, boolean ex return false; } + /// When the JVM was started with `-XX:StartFlightRecording` the JVM owns the recorder, and this + /// class must not touch it. Issuing `JFR.configure repositorypath=...` here would relocate the + /// JVM's recording, typically off the volume it was meant to be written to. + @Test + public void standsDownWhenTheJvmAlreadyStartedARecording() { + _continuousJfrStarter.setStartedByJvmArgument(true); + + _continuousJfrStarter.onChange(Set.of(), Map.of("pinot.jfr.enabled", "true", + "pinot.jfr.directory", "/some/where/else")); + + Assertions.assertThat(_continuousJfrStarter.getExecutedCommands()) + .describedAs("No JFR command may be issued when the JVM already owns the recording") + .isEmpty(); + Assertions.assertThat(_continuousJfrStarter.isRunning()) + .describedAs("This class is not managing the JVM's recording") + .isFalse(); + } + + @DataProvider(name = "jvmArguments") + public Object[][] jvmArguments() { + return new Object[][]{ + {List.of(), false}, + {List.of("-Xmx1g", "-XX:+UseG1GC"), false}, + // Both forms of both flags. -XX:FlightRecorderOptions matters even on its own: it is what + // sets repositorypath, which is what the stand-down exists to protect. + {List.of("-XX:StartFlightRecording=name=pinot-continuous,disk=true"), true}, + {List.of("-XX:StartFlightRecording:name=pinot-continuous"), true}, + {List.of("-XX:StartFlightRecording"), true}, + {List.of("-XX:FlightRecorderOptions=repository=/var/pinot/jfr"), true}, + {List.of("-XX:FlightRecorderOptions:repository=/var/pinot/jfr"), true}, + // What the Helm chart actually renders. + {List.of("-Xmx1g", "-XX:FlightRecorderOptions=repository=/var/pinot/jfr,preserve-repository=true", + "-XX:StartFlightRecording=name=pinot-continuous,settings=default,disk=true"), true}, + // A JFR flag quoted inside an unrelated property must not count. + {List.of("-Dsomething=-XX:StartFlightRecording"), false}, + {List.of("-Dpinot.jfr.enabled=true"), false}, + }; + } + + @Test(dataProvider = "jvmArguments") + public void detectsRecordingConfiguredByJvmArgument(List arguments, boolean expected) { + Assertions.assertThat(ContinuousJfrStarter.isRecordingConfiguredByJvmArgument(arguments)) + .describedAs("JVM arguments %s", arguments) + .isEqualTo(expected); + } + + /// The stand-down must not leak into the normal path. + @Test + public void startsNormallyWhenTheJvmDidNotConfigureARecording() { + _continuousJfrStarter.setStartedByJvmArgument(false); + + _continuousJfrStarter.onChange(Set.of(), Map.of("pinot.jfr.enabled", "true")); + + Assertions.assertThat(_continuousJfrStarter.isRunning()).isTrue(); + Assertions.assertThat(_continuousJfrStarter.getExecutedCommands()) + .containsExactly("jfrStart name=pinot-continuous settings=default dumponexit=false disk=true maxsize=" + + DEFAULT_MAX_SIZE_BYTES + " maxage=" + DEFAULT_MAX_AGE_MILLIS + "ms"); + } + private static boolean isRecordingPresent(String recordingName) { try { MBeanServer mBeanServer = ManagementFactory.getPlatformMBeanServer(); @@ -436,6 +498,7 @@ private static class TestContinuousJfrStarter extends ContinuousJfrStarter { private final List _executedCommands = new ArrayList<>(); private final Set _failingCommands = new HashSet<>(); private boolean _mBeanAvailable = true; + private boolean _startedByJvmArgument; @Override protected boolean executeDiagnosticCommand(String operationName, String... arguments) { @@ -457,6 +520,15 @@ protected boolean isRepositoryCleanupEnabled() { return false; } + @Override + protected boolean isRecordingConfiguredByJvmArgument() { + return _startedByJvmArgument; + } + + private void setStartedByJvmArgument(boolean startedByJvmArgument) { + _startedByJvmArgument = startedByJvmArgument; + } + private void setMBeanAvailable(boolean mBeanAvailable) { _mBeanAvailable = mBeanAvailable; } diff --git a/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java b/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java index 05ccbd4f6eb4..64f3c151632c 100644 --- a/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java +++ b/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java @@ -141,6 +141,8 @@ public void init(PinotConfiguration config) Executors.newCachedThreadPool(new ThreadFactoryBuilder().setNameFormat("async-task-thread-%d").build()); MinionEventObservers.init(_config, _executorService); + // Deprecated: kept so existing `pinot.jfr.*` cluster configs keep working. New deployments should + // start JFR with JVM arguments instead; see ContinuousJfrStarter for the replacement. _clusterConfigChangeHandler.registerClusterConfigChangeListener(ContinuousJfrStarter.INSTANCE); } diff --git a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java index c156d3a20777..fcf4fcc7d684 100644 --- a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java +++ b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java @@ -290,6 +290,8 @@ public void init(PinotConfiguration serverConf) _helixManager = HelixManagerFactory.getZKHelixManager(_helixClusterName, _instanceId, InstanceType.PARTICIPANT, _zkAddress); + // Deprecated: kept so existing `pinot.jfr.*` cluster configs keep working. New deployments should + // start JFR with JVM arguments instead; see ContinuousJfrStarter for the replacement. _clusterConfigChangeHandler.registerClusterConfigChangeListener(ContinuousJfrStarter.INSTANCE); initTransitionThreadPoolManager(); diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java index 7cdf8a56398d..ad0a90a5fd87 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java @@ -77,6 +77,11 @@ public static class Lucene { public static final String CONFIG_OF_LUCENE_MAX_CLAUSE_COUNT = "pinot.lucene.max.clause.count"; public static final int DEFAULT_LUCENE_MAX_CLAUSE_COUNT = 1024; } + /// Prefix of the cluster configs that drive the in-JVM continuous JFR recording. + /// + /// @deprecated Start JFR with JVM arguments (`-XX:StartFlightRecording` and + /// `-XX:FlightRecorderOptions`) instead. The Helm chart renders them from its `jfr` values block. + @Deprecated(since = "1.6.0", forRemoval = true) public static final String JFR = "pinot.jfr"; public static final String RLS_FILTERS = "rlsFilters"; diff --git a/pinot-tools/src/main/java/org/apache/pinot/tools/JfrQuickstart.java b/pinot-tools/src/main/java/org/apache/pinot/tools/JfrQuickstart.java index e161d4ee2c28..4de500be283d 100644 --- a/pinot-tools/src/main/java/org/apache/pinot/tools/JfrQuickstart.java +++ b/pinot-tools/src/main/java/org/apache/pinot/tools/JfrQuickstart.java @@ -26,6 +26,12 @@ import org.apache.pinot.spi.utils.CommonConstants; +/// Quickstart that turns on a continuous JFR recording. +/// +/// This deliberately uses the deprecated `pinot.jfr.*` cluster configs: a quickstart runs every +/// Pinot role inside one JVM, so it cannot give each role its own `-XX:StartFlightRecording`. Real +/// deployments should set the JVM arguments instead — see [ContinuousJfrStarter]. +@SuppressWarnings("removal") public class JfrQuickstart extends Quickstart { @Override public List types() {