Skip to content

DLPX-98733 Wire up config directory for drop-ins to delphix-telegraf.service - #127

Open
willguyette wants to merge 1 commit into
developfrom
dlpx/pr/willguyette/9489f912-b5f9-41c4-9c7f-a2c4b19ce32f
Open

DLPX-98733 Wire up config directory for drop-ins to delphix-telegraf.service#127
willguyette wants to merge 1 commit into
developfrom
dlpx/pr/willguyette/9489f912-b5f9-41c4-9c7f-a2c4b19ce32f

Conversation

@willguyette

@willguyette willguyette commented Aug 28, 2026

Copy link
Copy Markdown

Problem Statement

There's no supported way for a consumer outside this package to add Telegraf inputs/outputs on a Delphix Engine without editing product-owned files directly. This surfaced when the dlpx-qa-gate performance regression automation (which sed-patches /etc/telegraf/telegraf.base to add an external [[outputs.influxdb]] block forwarding metrics to influx1-prod-usw2.ops.delphix.com) silently broke after the InfluxDB/Telegraf feature restructured telegraf.base. The comment line the performance team's sed anchored on no longer exists, so the insert became a no-op with no error. Full root-cause writeup in DP-2736; this ticket (DLPX-98733) is the proposed engine-side fix.

Solution

Rather than sed patch the product config files, wire -config-directory /etc/telegraf/telegraf.d into the Telegraf invocation in delphix-telegraf-service. That directory already exists — currently empty and unreferenced. Consumers can now drop-in a self-contained *.conf file (e.g. an additional [[outputs.influxdb]] block) with zero edits to telegraf.base or any other product-owned file, and it survives telegraf.conf regeneration indefinitely since delphix-telegraf-service's rebuild logic never touches that directory.

Since Telegraf runs as root (delphix-telegraf.service has User=root) and Telegraf configs support plugins (inputs.exec, outputs.exec, etc.) that execute arbitrary commands, delphix-telegraf-service now explicitly locks /etc/telegraf/telegraf.d to root:root 700 on every start rather than relying on umask or upstream package defaults. Added a pointer comment in telegraf.base's output section directing future readers to the new directory instead of editing the file.

GitHub Copilot's automated review flagged that the initial version of this lockdown didn't check whether mkdir/chown/chmod actually succeeded — if they failed, the script fell through and started Telegraf with -config-directory pointed at a directory whose permissions weren't verified. Implemented a fix for this to instead disable the -config-directory flag and log an error if lockdown fails, so base metric collection keeps running even in that edge case — only the drop-in extension point is unavailable until the underlying cause is fixed and the service restarts.

Testing Done

  1. Confirmed the installed Telegraf binary (1.39.3) supports --config-directory before writing the change.

  2. Deployed and validated on two engines:

Fresh engine (wg-telegraf.dlpxdc.co, cloned off develop, no prior manual patches) — clean-room validation of the fix in isolation:

  • Captured baseline: original invocation was telegraf -config /etc/telegraf/telegraf.conf (no -config-directory), telegraf.d was root:root 755 and empty, only the 2 local outputs.influxdb_v2 blocks existed.
  • Deployed the updated delphix-telegraf-service and telegraf.base, restarted delphix-telegraf.
  • Confirmed the running process line now includes -config-directory /etc/telegraf/telegraf.d.
  • Confirmed telegraf.d permissions are now root:root 700 — verified by attempting ls as the non-root delphix user and getting Permission denied.
  • Confirmed local InfluxDB collection is unaffected: cpu measurement still flowing into the local default bucket every 10s as before.
  • Tested the extension point itself: dropped a self-contained qa-external-influx.conf (chmod 640, root-owned) into telegraf.d defining an additional [[outputs.influxdb]] pointing at influx1-prod-usw2.ops.delphix.com (InfluxDB 1.7.7) — no edits to telegraf.base or delphix-telegraf-service needed.
  • Restarted once more; journalctl -u delphix-telegraf showed zero errors (excluding pre-existing, unrelated estat/BCC kernel-compatibility noise on process exit, confirmed unrelated to this change).
  • Queried influx1-prod-usw2 directly via its HTTP API and confirmed this engine's cpu data (host=ip-10-110-246-90) landed in the telegraf_metrics database, concurrently with the continued local writes — proving both outputs run side by side with no conflict.

Existing engine (scale-dlpx-qa-release-DOSEREL-1.dlpxdc.co), which had DP-2736's manual stopgap patch (external output hand-appended directly to telegraf.base) — validated the migration path:

  • Deployed the same two files, which removed the hand-patched block (as expected, since it's no longer needed).
  • Replaced it with the same drop-in .conf approach in telegraf.d.
  • Confirmed data continued flowing to both the local and external InfluxDB instances after the switch, with no gap.
  1. Addressed the Copilot review comment on delphix-telegraf-service, all re-tested on wg-telegraf.dlpxdc.co:
  • Reproduced the gap: forced mkdir -p $TELEGRAF_CONFIG_DIR to fail (by placing a regular file where a path component should be a directory) against an isolated copy of the script pointed at throwaway paths, confirming the original code silently continued to start Telegraf regardless.
  • First fix attempt (fail closed): confirmed the script now exits 1 and logs an error via logger without ever invoking Telegraf, in isolation.
  • Verified the actual availability impact of failing closed by deliberately breaking the real /etc/telegraf/telegraf.d (swapped for a file) and restarting the real delphix-telegraf service. Confirmed via journalctl/systemctl status that systemd's default StartLimitBurst=5/StartLimitIntervalUSec=10s bounds this to 5 rapid restart attempts (under 1 second total, not an infinite crash loop), after which the unit is left in failed (Result: exit-code) with StartLimitAction=none — meaning the entire Telegraf service (not just the drop-in feature) stays down until a manual systemctl reset-failed && systemctl restart delphix-telegraf once the underlying cause is fixed. Restored the real directory and service to healthy afterward.
  • Given that blast radius, switched to the graceful-degrade design (disable -config-directory on lockdown failure, keep base collection running) and re-verified in isolation: forced the same mkdir failure, confirmed the error is still logged via logger, and confirmed — by inspecting the process table mid-run — that Telegraf actually started successfully and loaded all inputs/outputs, with the invoked command line correctly omitting -config-directory entirely. The real production Telegraf process was undisturbed throughout this isolated test.
  • Deployed the final version for real and re-confirmed the happy path is unaffected: delphix-telegraf active, correct -config-directory command line, telegraf.d still root:root 700, existing drop-in intact, zero errors.

@willguyette
willguyette force-pushed the dlpx/pr/willguyette/9489f912-b5f9-41c4-9c7f-a2c4b19ce32f branch from 1e9fd93 to e66e3bc Compare August 28, 2026 18:20
@willguyette
willguyette requested review from dbshah12 and sebroy and a lite review from Copilot August 28, 2026 18:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Wires Telegraf “drop-in” configuration support into the Delphix Engine Telegraf startup path, enabling external consumers to add inputs/outputs without editing product-owned Telegraf config files.

Changes:

  • Add guidance in telegraf.base directing external consumers to use /etc/telegraf/telegraf.d drop-ins.
  • Update delphix-telegraf-service to invoke Telegraf with -config-directory /etc/telegraf/telegraf.d and enforce restrictive permissions on that directory at startup.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
telegraf/telegraf.base Adds a pointer comment directing external configuration to /etc/telegraf/telegraf.d.
telegraf/delphix-telegraf-service Adds -config-directory support and startup-time permission lockdown for the drop-in directory.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread telegraf/delphix-telegraf-service Outdated
@willguyette
willguyette force-pushed the dlpx/pr/willguyette/9489f912-b5f9-41c4-9c7f-a2c4b19ce32f branch from e66e3bc to 10d4eb1 Compare August 28, 2026 19:10
@willguyette
willguyette force-pushed the dlpx/pr/willguyette/9489f912-b5f9-41c4-9c7f-a2c4b19ce32f branch from 10d4eb1 to 7a416a3 Compare August 28, 2026 19:16
@willguyette
willguyette marked this pull request as ready for review August 28, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants