DLPX-96312 Add InfluxDB/Telegraf infrastructure for Engine Performance Analytics

[dbshah12]

Design Doc

• https://perforce.atlassian.net/wiki/spaces/DLXEN/pages/2740027450/Design+of+Delphix+Engine+Performance+Analytics+in+DCT

Problem

Telegraf is already collecting engine performance metrics and writing them to local JSON files on the appliance. However, there is no local time-series database to store and serve these metrics, making it difficult for tools like DCT Smart Proxy to query historical performance data from the engine directly.

Additionally, several valuable metrics — per-connection TCP statistics, and storage I/O (NFS, iSCSI, backend disk) — were either not collected or only available when the performance playbook was explicitly enabled.

Storing all metrics in a single bucket would also mix Grafana-dashboard data with low-level diagnostics (aggregates, process counters, TCP internals), inflating storage costs for data that serves no dashboard purpose.

Solution

InfluxDB 2.x infrastructure

Add InfluxDB 2.x to the appliance as the single metrics store, mirroring the existing Telegraf setup pattern:

• influxdb/influxdb.toml — InfluxDB daemon config: bound to 127.0.0.1:8086, with bolt/engine paths matching the installed package (/var/lib/influxdb/). Named .toml (not .conf) because InfluxDB uses the Viper config library, which determines the file format from the extension — .conf is not recognized and is silently ignored, causing influxd to fall back to defaults (~/.influxdbv2/).
• influxdb/influxdb-init.conf — Tunable init config (org, bucket names, retention period, readiness wait parameters) sourced by the init script. Change values here without touching the script. Both default and support_metrics buckets use 30-day retention (INFLUXDB_RETENTION_SECONDS and INFLUXDB_SUPPORT_RETENTION_SECONDS = 2592000 respectively).
• influxdb/delphix-influxdb-init — One-time init script that:
  • Exits immediately if both /etc/influxdb/influxdb_meta and /etc/telegraf/telegraf.outputs.influxdb exist (safe on upgrades and reboots). If meta exists but the output file is missing (e.g. deleted by operator), falls through to a recovery path that reads stored tokens from meta and regenerates the output stanza without repeating any InfluxDB API calls.
  • Waits for InfluxDB to be ready via the /health endpoint.
  • Calls /api/v2/setup to create the org, default bucket, and admin credentials (one-shot; uses curl directly, no influx CLI dependency).
  • Is crash-safe: persists a setup state file immediately after /api/v2/setup; each subsequent step (bucket creation, token creation) atomically rewrites the full state file via write_state_file() (mktemp+mv) — a crash mid-write leaves the previous complete state rather than a torn line. Checks state on re-run; the entire script is idempotent end-to-end.
  • Creates the support_metrics bucket (30-day retention) for diagnostic and aggregate data that is not displayed in Grafana dashboards.
  • Creates three scoped tokens: a write-only token for Telegraf → default, a read-only token for DCT Smart Proxy → default, and a write-only token for Telegraf → support_metrics.
  • Writes two [[outputs.influxdb_v2]] stanzas to /etc/telegraf/telegraf.outputs.influxdb (chmod 640) — see Dual-bucket routing below.
  • Touches /etc/telegraf/INFLUXDB_ENABLED to enable InfluxDB output by default.
  • Atomically writes /etc/influxdb/influxdb_meta (chmod 600) containing: INFLUXDB_ORG, INFLUXDB_BUCKET, INFLUXDB_BUCKET_ID, INFLUXDB_SUPPORT_BUCKET, INFLUXDB_SUPPORT_BUCKET_ID, INFLUXDB_ADMIN_USER, INFLUXDB_ADMIN_PASSWORD, INFLUXDB_WRITE_TOKEN, INFLUXDB_READ_TOKEN, INFLUXDB_SUPPORT_WRITE_TOKEN. The support write token is stored so the recovery path can regenerate both [[outputs.influxdb_v2]] stanzas from stored credentials when telegraf.outputs.influxdb is missing.
• influxdb/delphix-influxdb-service — Wrapper that starts influxd with INFLUXD_CONFIG_PATH=/etc/influxdb/influxdb.toml in the background, runs the init script, then waits on the daemon PID.
• influxdb/delphix-influxdb.service — Systemd unit following the same structure as delphix-telegraf.service (PartOf=delphix.target, PartOf=delphix-platform.service for stop propagation on platform maintenance, Restart=always, runs as root).
• influxdb/perf_influxdb — Toggle script (mirrors perf_playbook) to enable/disable InfluxDB metric output from Telegraf without stopping InfluxDB itself. Exits with error if systemctl restart fails so the operator is not left silently broken.
• influxdb/influxdb-nginx.conf — nginx reverse proxy config that exposes InfluxDB externally at /influxdb/.
• debian/rules — Installs all influxdb files.
• debian/control — Added influxdb2, curl, and openssl to Depends.

Dual-bucket routing

Metrics are split across two buckets to keep Grafana-facing data separate from diagnostic data:

Bucket	Purpose	Measurements
default	Grafana dashboards	cpu, disk, diskio, net, zfs, tcp_stats, estat_nfs, estat_iscsi, hist_estat_nfs, hist_estat_iscsi, hist_estat_backend-io
support_metrics	Everything else — diagnostics, aggregates, uncategorised	mem, processes, system, procstat, agg_*, nfs_threads, estat_backend-io, estat_zpl/zvol/zio/zio-queue/metaslab-alloc, hist_estat_zpl/zvol/zio/…, docker_container_*

Routing uses namepass on the default bucket stanza (explicit allowlist of the 11 Grafana-facing measurements) and namedrop on the support_metrics stanza (mirrors the allowlist). Any new measurement added in future automatically lands in support_metrics — it must be explicitly added to the namepass list to reach the Grafana-facing bucket. This prevents accidental default-bucket bloat from new collections.

Telegraf metric collection changes

• telegraf/telegraf.base — Removed all [[outputs.file]] stanzas; InfluxDB is the sole output. percpu=false, tagexclude=["fstype","mode"] on disk, tagdrop for partition devices and tagexclude=["wwid"] on diskio, tagexclude=["cgroup_full"] on procstat. Added [[inputs.execd]] for tcp_stats via connstat-stats.sh. Added arcstats_demand_metadata_hits and arcstats_demand_metadata_misses to ZFS fieldpass.
• telegraf/connstat-stats.sh — Aggregates per-connection TCP stats by (laddr, raddr, service). Uses -c 2 so awk exits after each 10-second interval, triggering C runtime flush — required because mawk does not flush via fflush() to a Telegraf execd pipe. sleep 5 guard prevents tight CPU spin if connstat fails immediately.
• telegraf/telegraf.inputs.storage_io — Always-on estat_nfs, estat_iscsi, estat_backend-io when InfluxDB is enabled. Processor pipeline: starlark drops name=total rows → clone (order=1) creates hist_estat_* → starlark (order=2) pops microseconds from original estat_* rows → starlark (order=3) expands hist_estat_* into per-bucket le=/count rows. Uses processors.starlark (not processors.strings) to drop microseconds because fieldexclude in processors.strings only controls which fields string transforms operate on — it does not remove fields from the metric.
• telegraf/telegraf.inputs.playbook — Removed estat_nfs/iscsi/backend-io (moved to storage_io). Updated estat_metaslab-alloc to use wrapper script.
• telegraf/delphix-telegraf-service — Appends storage_io and InfluxDB output stanzas when InfluxDB is enabled; falls back to [[outputs.discard]] otherwise. chmod 640 on telegraf.conf applied unconditionally as hardening.

BPF/estat kernel compatibility fixes (DLPX-96701)

Removed conflicting forward declarations and struct typedefs from bpf/estat/nfs.c, bpf/stbtrace/nfs.st, bpf/estat/zvol.c, bpf/stbtrace/iscsi.st. Added -D__KERNEL__/-D_KERNEL BPF compiler flags. Fixed estat zil (zil_commit_waiter_skip kprobe removed, --coll default added).

Complete list of measurements in InfluxDB

Measurement	Source	Bucket	Availability
cpu	[[inputs.cpu]] (cpu-total only)	default	Always
disk	[[inputs.disk]] (fstype/mode tags excluded)	default	Always
diskio	[[inputs.diskio]] (partitions + wwid excluded)	default	Always
net	[[inputs.net]]	default	Always
zfs	[[inputs.zfs]]	default	Always
tcp_stats	connstat	default	Always
mem	[[inputs.mem]]	support_metrics	Always
processes	[[inputs.processes]]	support_metrics	Always
system	[[inputs.system]]	support_metrics	Always
procstat	[[inputs.procstat]] (cgroup_full excluded)	support_metrics	Always
agg_cpu/disk/diskio/mem/net/processes/system	Hourly aggregates	support_metrics	Always
estat_nfs	estat nfs	default	When InfluxDB enabled
estat_iscsi	estat iscsi	default	When InfluxDB enabled
estat_backend-io	estat backend-io scalars	support_metrics	When InfluxDB enabled
hist_estat_nfs, hist_estat_iscsi, hist_estat_backend-io	Histogram clones (le=/count rows)	default	When InfluxDB enabled
estat_zpl/zio/zvol/zio-queue/metaslab-alloc	ZFS stats	support_metrics	Playbook only
hist_estat_zpl/zvol/zio/…	Histogram clones of playbook estat_*	support_metrics	Playbook only
nfs_threads	NFS thread utilization	support_metrics	Playbook only
docker_container_*	Docker/DCT metrics	support_metrics	DCT engines only

Notes to Reviewers

Bucket routing: namepass (explicit allowlist), not namedrop

The default bucket stanza uses namepass with an explicit list of the 11 Grafana-facing measurements. Any new measurement added in future automatically lands in support_metrics without any config change — it must be explicitly promoted to reach Grafana. This prevents accidental default-bucket bloat.

Retention policy: both buckets are 30 days

Both default and support_metrics use 30-day retention (INFLUXDB_RETENTION_SECONDS = INFLUXDB_SUPPORT_RETENTION_SECONDS = 2592000), configured in influxdb/influxdb-init.conf. This matches the existing Telegraf JSON file retention and provides sufficient history for trend analysis and support investigation.

hist_estat_* — why processors.starlark, not processors.strings

The microseconds field is dropped from original estat_* rows via a [[processors.starlark]] processor calling metric.fields.pop("microseconds", None). processors.strings with fieldexclude was not used because fieldexclude in that plugin only controls which fields string transforms (replace, trim, etc.) operate on — it does not remove fields from the metric. With no transforms defined, the block is a complete no-op and microseconds would be stored as a raw string in every estat_* row in InfluxDB (verified: leaked before the Starlark fix was applied).

PartOf=delphix-platform.service in both service units

Both delphix-influxdb.service and delphix-telegraf.service carry two PartOf= directives: PartOf=delphix.target (unit group membership) and PartOf=delphix-platform.service (stop propagation). After= alone establishes only boot ordering — it does not propagate stop/restart events. The second PartOf= ensures both services stop cleanly when the Delphix platform stops during maintenance.

Runtime dependency decisions (debian/control)

Dependency	Decision	Reason
openssl	Added	Used by delphix-influxdb-init for password generation; only in Build-Depends in delphix-platform
python3	Not added	Already present via python3-minimal
curl	Added	Only in delphix-platform's Build-Depends

Why influxdb.toml instead of influxdb.conf

InfluxDB 2.x uses Viper for config parsing — only .json, .toml, .yaml, .yml are recognized. .conf is silently ignored; influxd falls back to ~/.influxdbv2/. Verified on v2.8.0.

metaslab-alloc-stats.sh — DLPX-88427 garbage name filter

Moved to a dedicated PR (#120 / DLPX-88427). Not part of this PR.

Testing Done

ab-pre-push

• Original run (initial commit 1000aa9): https://selfservice-jenkins.eng-tools-prd.aws.delphixcloud.com/job/appliance-build-orchestrator-pre-push/13811/ - ✅
• Pending: new ab-pre-push required for follow-up commits b6e6d4a / d4f491e / 7bccceb (review fixes + Starlark le tag correction)
• On the ab-pre-push engine, below files are created:

/etc/influxdb# ls -l
total 4
-rw-r--r-- 1 root root  86 Mar 31 09:56 config.toml
-rw-r--r-- 1 root root 357 Mar 31 09:19 influxdb-init.conf
-rw-r--r-- 1 root root 274 Mar 31 09:19 influxdb.toml
-rw------- 1 root root 347 Mar 31 12:24 influxdb_meta

/etc/influxdb# ls -l /var/lib/influxdb
total 23
drwxr-x--- 5 influxdb influxdb      5 Mar 31 12:46 engine
-rw------- 1 influxdb influxdb  65536 Mar 31 12:46 influxd.bolt
-rw-r----- 1 influxdb influxdb      4 Mar 31 12:22 influxd.pid
-rw-r----- 1 influxdb influxdb 122880 Mar 31 12:23 influxd.sqlite

• InfluxDB setup is also completed, and I can see data there in the UI:

Measurements verified in InfluxDB

default bucket (Grafana-facing — dashboard measurements only):

cpu              disk             diskio           estat_iscsi
estat_nfs        hist_estat_backend-io             hist_estat_iscsi
hist_estat_nfs   net              tcp_stats        zfs

support_metrics bucket (everything else):

agg_cpu          agg_disk         agg_diskio       agg_mem
agg_net          agg_processes    agg_system       estat_backend-io
mem              nfs_threads      processes        procstat
system

Change-specific verifications

Change	Verification
diskio partition exclusion	nvme0n1p* and sda[0-9]* absent; only whole-disk entries present
diskio wwid tag removal	wwid absent from diskio
disk fstype/mode tag removal	Tags absent
procstat cgroup_full tag removal	Tag absent
hist_estat_* histogram expansion	Present in default with string le=<upper_bound_us> tags and integer count field (Telegraf Starlark requires string tags); raw microseconds string absent
M2 recovery — output file regeneration	Deleted telegraf.outputs.influxdb on dhruv3.dlpxdc.co, re-ran init; file regenerated from influxdb_meta tokens, Telegraf restarted cleanly, histogram data resumed within one collection cycle
microseconds dropped from estat_* originals	Verified via Flux query on dhruv3.dlpxdc.co: estat_iscsi fields are only avg latency(us), iops(/s), stddev(us), throughput(k/s) — microseconds does not exist (confirmed after processors.starlark fix)
tcp_stats in default — all fields	connections, inbytes, outbytes, retranssegs, rtt, cwnd, swnd, rwnd, suna, unsent all present; absent from support_metrics
agg_* in support_metrics only	Confirmed; absent from default
mem/processes/system/procstat in support_metrics	Confirmed; absent from default
estat_backend-io scalars in support_metrics	Confirmed; hist_estat_backend-io in default
tcp_stats service tag	Present (e.g. nfs, https, dlpx-sp)
tcp_stats rport absent	Confirmed; key is (laddr, raddr, service)
influxdb_meta completeness	All ten keys present: INFLUXDB_ORG, INFLUXDB_BUCKET, INFLUXDB_BUCKET_ID, INFLUXDB_SUPPORT_BUCKET, INFLUXDB_SUPPORT_BUCKET_ID, INFLUXDB_ADMIN_USER, INFLUXDB_ADMIN_PASSWORD, INFLUXDB_WRITE_TOKEN, INFLUXDB_READ_TOKEN, INFLUXDB_SUPPORT_WRITE_TOKEN (DLPX-96313)
H1 — already-onboarded probe	Deleted influxdb_setup_state and influxdb_meta on dhruv3 with InfluxDB already running; re-ran init; script probed GET /api/v2/setup, saw {"allowed":false}, exited with clear manual-recovery message rather than looping on a failing POST
M4 — postinst nginx path	nginx -t -c /opt/delphix/server/etc/nginx/nginx.conf succeeds on appliance (the Delphix-managed config includes conf.d/influxdb.conf); nginx -s reload follows; confirmed on ab-pre-push engine
L1 — nginx proxy_read_timeout	influxdb-nginx.conf contains proxy_read_timeout 300s; confirmed via grep on deployed config
L3 — Restart=always on clean exit	Sent SIGTERM to influxd PID directly (clean exit 0); systemd restarted the unit within 100ms; systemctl status shows one restart in the journal
connstat mawk flush fix	10-second batches flushed on schedule; sleep 5 guard confirmed
perf_influxdb error propagation	systemctl restart failure surfaces via `
30-day retention both buckets	INFLUXDB_RETENTION_SECONDS=2592000 and INFLUXDB_SUPPORT_RETENTION_SECONDS=2592000 confirmed in influxdb-init.conf
BPF compilation	estat nfs/iscsi/backend-io compile without errors
estat zil default collection	Runs without -c flag

perf_influxdb enable/disable testing

Test	Result
INFLUXDB_ENABLED flag exists on fresh boot	✅
telegraf.outputs.influxdb with correct perms (-rw-r-----)	✅
Telegraf loaded influxdb_v2 output (2x) on boot	✅
perf_influxdb disable → [[outputs.discard]] config	✅
perf_influxdb enable → both influxdb_v2 stanzas reload	✅
Data flows to both default and support_metrics	✅
Non-root blocked with clear error	✅
No errors in journalctl	✅

[dbshah12]

All Copilot review comments addressed — stale threads resolved, valid issues fixed or explained inline.

[ShibasishDelphix]

minimal test probe

[ShibasishDelphix]

Found 1 blocker, 6 warnings, and 2 nits across 8 files — please address before merging.

❌ Testing — Missing

Please add a Jenkins run link to the PR description before merging.

Reviewed with generic rules — no repo-specific config found for delphix/performance-diagnostics.

[dbshah12]

• appliance-build-orchestrator

Attached ab-pre-push and also appliance-build-orchestrator is running by default as well.

[delphix-devops-bot]

Code Review — performance-diagnostics PR #119

PR: DLPX-96312 Add InfluxDB/Telegraf infrastructure for Engine Performance Analytics
Author: dbshah12 · Base: develop · Head: 77aae7f
Scope: 17 files, +968 / −140
Review date: 2026-07-09
Review mode: no-spec (PR references a Confluence design doc, not a repo spec file — Acceptance Auditor layer skipped)
Layers run: Blind Hunter (adversarial), Edge Case Hunter — both completed, no failed layers.

---

Summary

Code review complete. 0 decision-needed, 11 findings to address (2 verify-first), 0 pre-existing deferrals, 7 dismissed as noise/false-positive.

Severity mix: 1 High · 6 Medium · 4 Low. No finding is a hard merge-blocker on its own, but the High finding and two of the Mediums defeat crash-safety / recovery guarantees the PR explicitly claims, and should be resolved before this ships to appliances.

Because this is a no-spec run, no story file was written and no sprint status was synced. Findings are recorded in this report only.

---

Findings (triaged)

🔴 High

H1. delphix-influxdb-init cannot recover when InfluxDB is already onboarded but local state is absent

• Source: blind + edge (merged)
• File: influxdb/delphix-influxdb-init (setup guard ~L96, /api/v2/setup call ~L120-145)
• Failure scenario: InfluxDB has completed onboarding (bolt/engine exist under /var/lib/influxdb) but /etc/influxdb/influxdb_meta and influxdb_setup_state are both gone. This happens two ways:
  1. Crash window: the process is killed after /api/v2/setup returns 200 but before the mktemp+mv that persists ADMIN_TOKEN. The admin token is now lost forever and no state file exists.
  2. Config-only reset: a package reinstall / config-volume reset wipes /etc/influxdb while the data volume persists.
     In both cases the next run sees no meta and no state, so it re-issues POST /api/v2/setup. InfluxDB rejects a second onboarding (HTTP 4xx), curl -sf fails, influx_post returns 1, and the script exit 1s. delphix-influxdb-service then kills influxd and exits non-zero; systemd Restart=on-failure retries until the default StartLimitBurst is hit, after which the unit stays failed — InfluxDB never comes up and Telegraf output is never configured.
• Why it matters: directly contradicts the "crash-safe end-to-end / idempotent" guarantee in the script header and PR description. Consequence is a permanent InfluxDB outage in a state that looks recoverable.
• Recommended fix: before POST /api/v2/setup, probe onboarding state (GET /api/v2/setup → {"allowed": false} means already onboarded) and branch instead of blindly POSTing. Note the crash-window variant (1) is only fully closed by making the admin-token persist atomic with the setup call — surface that limitation explicitly if it can't be fully closed.
• Bucket: patch

---

🟠 Medium

M1. openssl is used by the init script but is not a package dependency (and the PR description says it was added)

• Source: blind
• File: influxdb/delphix-influxdb-init (INFLUXDB_ADMIN_PASSWORD="$(openssl rand -hex 16)"), debian/control
• Failure scenario: debian/control Depends was extended with influxdb2, curl only — not openssl (verified against develop: openssl was already absent, and this PR does not add it). Under #!/bin/bash -eu, if openssl is missing the command substitution fails and aborts init → InfluxDB never initializes. The PR's own "Runtime dependency decisions" table claims openssl was Added, so code and description disagree.
• Why it matters: openssl is very likely present on the base appliance, so real-world breakage is unlikely — but the dependency is undeclared and the PR misrepresents the change. Cheap to make correct.
• Recommended fix: add openssl to Depends (matching the PR's stated intent), or correct the PR description if openssl is guaranteed by another dependency.
• Bucket: patch

M2. Documented recovery path is broken — lost telegraf.outputs.influxdb is never regenerated

• Source: blind
• File: influxdb/delphix-influxdb-init (early-exit guard ~L96 vs. meta-file comment ~L260-264)
• Failure scenario: /etc/influxdb/influxdb_meta exists but /etc/telegraf/telegraf.outputs.influxdb is deleted/corrupted. perf_influxdb enable dies with "…not found. Run delphix-influxdb-init first.", but re-running delphix-influxdb-init hits if [[ -f "$INFLUXDB_META_FILE" ]]; then … exit 0 and returns without recreating the stanza. The meta-file comment explicitly promises "If telegraf.outputs.influxdb is lost, re-running delphix-influxdb-init will recreate all tokens from scratch" — which is false given the guard.
• Why it matters: a stated recovery procedure is a dead end; the operator has no supported way to restore Telegraf→InfluxDB output short of manual surgery.
• Recommended fix: include the output-file (and/or flag) presence in the early-exit condition so a missing output stanza triggers regeneration, or remove the false recovery claim from the comment.
• Bucket: patch

M3. State file is appended non-atomically — a torn line yields a truncated token that resume then trusts

• Source: blind + edge (merged)
• File: influxdb/delphix-influxdb-init (SUPPORT_BUCKET_ID, WRITE_TOKEN, READ_TOKEN, SUPPORT_WRITE_TOKEN written via >>)
• Failure scenario: the first four keys are persisted atomically (mktemp+chmod+mv), but the four later values are >>-appended one line at a time. If the process is killed mid-append (e.g. WRITE_TOKEN=<half>), the resume parser reads a non-empty truncated token. The guard if [[ -z "$WRITE_TOKEN" ]] treats non-empty as valid, skips regeneration, and writes the broken token into telegraf.outputs.influxdb. Telegraf then authenticates with a bad token and all default-bucket writes are silently rejected.
• Why it matters: undermines the crash-safe resume design; produces a silent, hard-to-diagnose data-loss state (Telegraf "running", no data landing).
• Recommended fix: rebuild the whole state file atomically (accumulate in memory / temp file, mv into place) rather than incremental >> appends, or validate token shape on resume.
• Bucket: patch

M4. postinst validates the wrong nginx config tree, so the InfluxDB proxy may never activate

• Source: blind + edge (merged)
• File: debian/postinst (if nginx -t -c /etc/nginx/nginx.conf &>/dev/null && systemctl is-active --quiet nginx; then nginx -s reload; fi), debian/rules
• Failure scenario: the proxy snippet installs to /opt/delphix/server/etc/nginx/conf.d/influxdb.conf (Delphix-managed nginx prefix — the appliance has no stock /etc/nginx). The guard tests -c /etc/nginx/nginx.conf, which is a different (or non-existent) config tree: it can't see influxdb.conf, so (a) a syntax error in the new snippet passes the test, and (b) if the stock config path is absent/invalid, nginx -t fails, &>/dev/null hides it, the && short-circuits, and nginx -s reload is skipped silently while postinst still exit 0s. Result: the /influxdb/ location isn't loaded until the next full nginx restart/reboot.
• Why it matters: external DCT/Grafana clients get 404 against /influxdb/ after an upgrade until nginx is independently restarted; the install reports success.
• Recommended fix: point nginx -t at the Delphix nginx config actually in use (the one that includes /opt/delphix/server/etc/nginx/conf.d), and make a failed test visible rather than swallowed.
• Bucket: patch

M5. Removing all Telegraf file outputs discards all metrics whenever InfluxDB is disabled or pre-init — verify downstream consumers

• Source: blind
• File: telegraf/telegraf.base (four [[outputs.file]] stanzas + filestat input removed), telegraf/telegraf.inputs.dct (docker file output removed), telegraf/delphix-telegraf-service (echo "[[outputs.discard]]" fallback)
• Failure scenario: metrics previously always landed in /var/log/telegraf/metrics*.json (and metrics_docker.json). Those outputs are gone. Now, whenever INFLUXDB_ENABLED is absent or telegraf.outputs.influxdb is missing — including the window before first-boot init completes, and after any perf_influxdb disable — delphix-telegraf-service appends [[outputs.discard]] and every metric is dropped. Any support-bundle collector or tooling that reads those JSON files gets nothing.
• Why it matters: this is an intentional design shift (InfluxDB as sole sink, enabled by default), but it is a broad behavioral change. It needs confirmation that support-bundle / diagnostics collection has been migrated off the JSON files, otherwise field diagnostics silently regress.
• Recommended action: confirm (with the design doc / platform team) that no consumer depends on /var/log/telegraf/*.json; if any do, migrate or retain a minimal file output. Verify before merge.
• Bucket: patch (verify-first)

M6. connstat two-sample aggregation double-counts unless connstat emits a ^= interval separator — verify

• Source: blind + edge (merged)
• File: telegraf/connstat-stats.sh (connstat … -i 10 -c 2 … piped to awk; /^=/ reset vs. NF==13 accumulate vs. END flush)
• Failure scenario: -c 2 emits two per-connection snapshots per invocation. The awk resets/flushes accumulators only on a line matching /^=/ (and at END). If connstat's parsable output does not print a =-prefixed separator between the two intervals, both snapshots match NF==13 and accumulate into the same key: cumulative fields (inbytes, outbytes, retranssegs, suna, unsent) are summed to ≈2× their true value, and window/RTT fields are averaged over 2n connections. tcp_stats would report inflated counters and skewed averages every cycle.
• Why it matters: silent data-correctness bug in a newly-exposed measurement. The presence of the hand-written /^=/ branch suggests the author observed such separators in connstat output (and the PR's testing table reports plausible values), so this may already be correct — but it rests on an unasserted output-format assumption that neither reviewer could confirm from the diff.
• Recommended action: confirm the exact connstat -PLe -i 10 -c 2 -T u output format on a live engine (does it delimit intervals with a ^= line?). If not, the flush logic must key on something deterministic. Verify before relying on tcp_stats values.
• Bucket: patch (verify-first)

---

🟡 Low

L1. nginx /influxdb/ proxy: proxy_read_timeout 999d and no engine-layer auth

• Source: blind
• File: influxdb/influxdb-nginx.conf
• Detail: the location is a bare proxy_pass with no auth_request/allow/session-auth include — all InfluxDB v2 API endpoints are reachable by anyone who can hit the engine's HTTPS. Practical risk is limited because every InfluxDB API call still requires a valid token (so this is token-gated, not open), but /health leaks version and the posture is weaker than the rest of the appliance API. Separately, proxy_read_timeout 999d (~2.7 years) means hung/slow query connections are never reaped, risking worker-connection exhaustion.
• Recommended fix: set a sane proxy_read_timeout; consider whether /influxdb/ should sit behind the engine's normal auth layer.
• Bucket: patch

L2. influxdb-init.conf is sourced under set -u with no key validation

• Source: edge
• File: influxdb/delphix-influxdb-init (source "$INFLUXDB_INIT_CONF")
• Detail: the conf is an admin-editable file. Under bash -eu, deleting any expected key (e.g. INFLUXDB_SUPPORT_RETENTION_SECONDS) leaves it unbound, and the first reference aborts init with a terse "unbound variable" and no diagnostic — InfluxDB output never gets configured.
• Recommended fix: validate required keys after sourcing (or provide in-script defaults) and fail with a clear message.
• Bucket: patch

L3. delphix-influxdb-service doesn't restart on a clean influxd exit and forwards no signals

• Source: blind + edge (merged)
• File: influxdb/delphix-influxdb-service, influxdb/delphix-influxdb.service
• Detail: influxd runs as a backgrounded child; the wrapper waits on it. If influxd exits 0 (clean/handled), wait returns 0, the wrapper exits 0, and Restart=on-failure does not restart — InfluxDB stays down while Telegraf keeps writing to a dead endpoint. The wrapper also installs no trap, relying entirely on KillMode=control-group for shutdown.
• Recommended fix: consider exec influxd after init (so the daemon is the main PID with direct signal delivery), or Restart=always, or a trap that forwards SIGTERM.
• Bucket: patch

L4. Histogram-expansion Starlark: unguarded int() and string-typed le tag

• Source: edge + blind (merged)
• File: telegraf/telegraf.inputs.storage_io (order=3 Starlark processor)
• Detail: m.fields["count"] = int(parts[1]) has no numeric guard — a malformed microseconds pair (e.g. {20000,x}) passes the len==2 and parts[1] presence check but raises in int(), erroring that metric batch. parts[0] is written verbatim as the le tag, so a garbage upper bound becomes a garbage tag, and numeric bucket bounds are stored as strings (lexical ordering, e.g. "100000" < "20000"). Low likelihood since estat generates the histogram strings, but ungracefully handled.
• Recommended fix: guard int() conversion (skip non-numeric pairs) and confirm Grafana heatmap consumption tolerates string le values.
• Bucket: patch

---

Dismissed (7) — false positives / handled elsewhere

#	Finding (as raised)	Why dismissed
D1	chmod 640 on telegraf.conf / telegraf.outputs.influxdb breaks Telegraf if it runs as user telegraf	Verified: delphix-telegraf.service runs User=root (and so does delphix-influxdb.service). Root reads 640 root-owned files fine.
D2	postinst never enables/starts delphix-influxdb.service	#DEBHELPER# is present → dh_installsystemd enables and starts WantedBy=delphix.target units. Handled by packaging.
D3	Histogram schema change (merged JSON object → per-bucket le/count) breaks existing Grafana dashboards on upgrade	InfluxDB is new in this PR — there were no prior InfluxDB-backed dashboards to break. The old JSON-object format only ever fed the removed file outputs.
D4	order=2 Starlark namepass=["estat_*"] also strips microseconds from the hist_estat_* clones (breaking order=3)	Telegraf glob estat_* is prefix-anchored and does not match hist_estat_*; order=3 still sees microseconds. Edge Hunter self-cleared this on walk-through.
D5	INFLUXD_CONFIG_PATH treated as a directory → influxdb.toml ignored → influxd binds all interfaces	PR testing evidence shows the bolt/engine paths from influxdb.toml in effect (/var/lib/influxdb/…, not ~/.influxdbv2), i.e. the file is loaded, so http-bind-address = 127.0.0.1:8086 is honored.
D6	Duplicate PartOf= directives in delphix-influxdb.service	Intentional and documented in PR notes (PartOf=delphix.target for grouping + PartOf=delphix-platform.service for stop propagation), mirroring delphix-telegraf.service.
D7	Playbook estat histogram processing now depends on InfluxDB state (wasted collection when playbook on / InfluxDB off)	Moot: when InfluxDB is disabled the output is [[outputs.discard]], so nothing is stored regardless. Wasted CPU only, no data defect.

---

Verification performed during triage

• Cloned delphix/performance-diagnostics@develop and inspected files outside the diff:
  • telegraf/delphix-telegraf.service → User=root (dismisses D1).
  • debian/control on develop → confirms openssl was already absent and this PR does not add it (confirms M1).
  • debian/postinst → #DEBHELPER# present (dismisses D2); appliance has no /etc/nginx tree (supports M4).
• Cross-checked both reviewer layers against each other; merged 4 overlapping findings (H1, M3, M4, M6, L3).

Notes / residual verification for the author (not counted as findings)

• M5 and M6 are marked verify-first — they may already be correct by design/tooling, but neither could be confirmed from the diff alone and both carry silent-failure consequences.
• Systemd StartLimit is not tuned on delphix-influxdb.service; combined with H1, a repeatedly-failing init will land the unit in a permanent failed state (not an infinite spin). Consider whether init failure should really kill influxd.

[dbshah12]

Thanks for the thorough review. Here's a status update on each finding:

Addressed in b6e6d4a (Address review findings: crash safety, idempotency, packaging, nginx):

• H1 ✅ — delphix-influxdb-init now probes GET /api/v2/setup before POSTing. If InfluxDB is already onboarded but local state is absent, it logs a clear manual-recovery message and exits rather than silently looping into failure.

• M1 ✅ — openssl added to Depends in debian/control.

• M2 ✅ — Early-exit guard now requires both influxdb_meta and telegraf.outputs.influxdb to be present. If the output file is missing, the script falls through to a recovery path that reads stored tokens from influxdb_meta and regenerates the stanza without repeating any InfluxDB API calls. Smoke-tested on a live engine: deleted telegraf.outputs.influxdb, re-ran init, file restored correctly, Telegraf restarted cleanly.

• M3 ✅ — All >> appends replaced with a write_state_file() helper that rewrites the full state file atomically via mktemp+mv on every step. A crash mid-write now leaves the previous complete state rather than a torn line.

• M4 ✅ — postinst now uses -c /opt/delphix/server/etc/nginx/nginx.conf (the Delphix-managed config that includes conf.d/influxdb.conf). Addressed in d4f491e.

• M5 (verify-first) — The file outputs were removed intentionally: InfluxDB is the new sole sink for all time-series data, and support bundle collection has been confirmed to not depend on /var/log/telegraf/*.json. The [[outputs.discard]] fallback during pre-init is the designed behavior (metrics during the narrow boot window are not worth persisting).

• M6 (verify-first) — Confirmed on a live engine: connstat -PLe -i 10 -c 2 -T u does emit a =-prefixed separator line between the two 10s intervals. The awk /^=/ reset fires correctly; per-service tcp_stats values show no double-counting (e.g. dlpx-connector reports 6 connections, not 12).

• L1 ✅ — proxy_read_timeout reduced to 300s in influxdb-nginx.conf. On engine-layer auth: InfluxDB v2 requires a valid token on every API call, so the proxy effectively is token-gated. We're not adding a second auth layer on top of that.

• L2 ✅ — Validation loop added immediately after source "$INFLUXDB_INIT_CONF" to fail with a clear ERROR: Required key '...' is missing message.

• L3 ✅ — delphix-influxdb.service changed to Restart=always so a clean influxd exit also triggers a restart.

• L4 ✅ (completed in 7bccceb) — b6e6d4a added the isdigit() malformed-pair guard correctly, but the int() cast on m.tags["le"] was wrong: Telegraf Starlark requires all tag values to be strings, and the int() caused every hist_estat_* per-bucket row to be dropped with tag value must be of type 'str'. 7bccceb removes the int() cast, keeping le as the string it already is from split(). hist_estat_* histogram data confirmed flowing on the live engine with correct le tag values ("300000", "800000", etc.).