docs(dns-ha): keepalived analysis for DNS high availability#39
docs(dns-ha): keepalived analysis for DNS high availability#39NickJLange wants to merge 9 commits intomasterfrom
Conversation
- Update podmanSetupRaspberryPi submodule to enforce overlay driver in /etc/containers/storage.conf. - Add fuse-overlayfs to stage 5 package install list. - Enforce overlay driver configuration in stage 5 playbook. - Add openspec/changes/podman-overlayfs.md.
- Add Telegraf DNS monitoring config (pihole API + dig latency checks) - Add Datadog monitor JSON definitions (warning/critical alerts) - Add DNS restart playbook with diagnostic collection - Add Podman healthcheck for pihole containers - Add Unbound access-control for monitoring host Implements: - OpenTelemetry-compatible DNS health metrics per server - Datadog → Pagerduty alerting - Restart playbook for operational response - Container healthcheck fix for Podman
Analysis document covering: - Keepalived/VRRP vs round-robin vs client-side options - VIP address plan (.2 per subnet) - Network requirements (Ubiquity + commodity switches) - Risks: ARP cache, split-brain, firewall - Implementation roadmap: network check -> keepalived -> integration -> client migration - Decision: gradual client migration, no cross-region failover, monitor keepalived state
📝 WalkthroughWalkthroughAdds Datadog monitor JSONs, Telegraf DNS monitoring defaults and template, an Ansible dns-restart playbook and Podman overlayfs changes, two role submodule pointer updates, and multiple openspec design/spec/task documents for DNS HA and monitoring. Changes
Sequence Diagram(s)sequenceDiagram
participant PiHole as Pi-hole
participant Telegraf as Telegraf Agent
participant Datadog as Datadog Ingest
participant Monitor as Datadog Monitor
participant PagerDuty as PagerDuty
participant Operator as Operator (Ansible)
PiHole->>Telegraf: Expose metrics (HTTP API, exec dig)
Telegraf->>Datadog: Send metrics/events
Datadog->>Monitor: Evaluate monitor rules (5m/15m windows)
Monitor-->>PagerDuty: CRITICAL → trigger incident
PagerDuty->>Operator: Alert on-call
Operator->>PiHole: Run playbook (dns-restart.yml) to restart and collect diagnostics
PiHole->>Datadog: Recovery metrics/events → Monitor resolves incident
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 17 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dns-monitoring/datadog-monitor-dns-down-warning.json">
<violation number="1" location="dns-monitoring/datadog-monitor-dns-down-warning.json:17">
P2: `warning` and `critical` thresholds are identical, so this monitor will always go critical and never emit a warning state. Either remove the warning threshold or adjust the monitor to reflect critical severity to avoid misleading alert severity.</violation>
</file>
<file name="dns-monitoring/datadog-monitor-dns-latency-warning.json">
<violation number="1" location="dns-monitoring/datadog-monitor-dns-latency-warning.json:4">
P2: The query threshold should match the critical threshold (critical is defined in the query for metric alerts). With `> 100` in the query but `critical: 500`, the monitor will alert at 100 or fail validation. Update the query to use the critical threshold and keep warning in `thresholds`.</violation>
</file>
<file name="playbooks/dns-restart.yml">
<violation number="1" location="playbooks/dns-restart.yml:32">
P2: The restart task fails the play immediately, so the diagnostics tasks never run when a restart fails. Move failure handling after diagnostics (e.g., block/rescue or defer fail) so diagnostics are collected before stopping the play.</violation>
</file>
<file name="roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2">
<violation number="1" location="roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2:19">
P2: This `dig +short` command outputs an IP string, but the input is configured for integer values, so Telegraf will fail to parse the metric. Output a numeric latency (e.g., parse `Query time` from `+stats`) instead.</violation>
<violation number="2" location="roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2:35">
P2: This command returns IP strings, but the metric expects an integer value, so parsing will fail. Emit numeric latency (e.g., parse `Query time` from `+stats`).</violation>
</file>
<file name="dns-monitoring/datadog-monitor-dns-external-latency.json">
<violation number="1" location="dns-monitoring/datadog-monitor-dns-external-latency.json:4">
P2: Query threshold (200) conflicts with critical threshold (500). Datadog monitors require the query threshold to match the critical threshold; otherwise the monitor definition is invalid or behaves unexpectedly. Align the query threshold with the critical value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "new_group_delay": 30, | ||
| "include_tags": false, | ||
| "thresholds": { | ||
| "critical": 1, |
There was a problem hiding this comment.
P2: warning and critical thresholds are identical, so this monitor will always go critical and never emit a warning state. Either remove the warning threshold or adjust the monitor to reflect critical severity to avoid misleading alert severity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dns-monitoring/datadog-monitor-dns-down-warning.json, line 17:
<comment>`warning` and `critical` thresholds are identical, so this monitor will always go critical and never emit a warning state. Either remove the warning threshold or adjust the monitor to reflect critical severity to avoid misleading alert severity.</comment>
<file context>
@@ -0,0 +1,21 @@
+ "new_group_delay": 30,
+ "include_tags": false,
+ "thresholds": {
+ "critical": 1,
+ "warning": 1
+ }
</file context>
| { | ||
| "name": "DNS Latency High - WARNING", | ||
| "type": "query alert", | ||
| "query": "max(last_5m):max:dns_internal_latency{*} > 100", |
There was a problem hiding this comment.
P2: The query threshold should match the critical threshold (critical is defined in the query for metric alerts). With > 100 in the query but critical: 500, the monitor will alert at 100 or fail validation. Update the query to use the critical threshold and keep warning in thresholds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dns-monitoring/datadog-monitor-dns-latency-warning.json, line 4:
<comment>The query threshold should match the critical threshold (critical is defined in the query for metric alerts). With `> 100` in the query but `critical: 500`, the monitor will alert at 100 or fail validation. Update the query to use the critical threshold and keep warning in `thresholds`.</comment>
<file context>
@@ -0,0 +1,21 @@
+{
+ "name": "DNS Latency High - WARNING",
+ "type": "query alert",
+ "query": "max(last_5m):max:dns_internal_latency{*} > 100",
+ "message": "DNS latency on {{server.name}} is above 100ms for 5 minutes.\n\n@slack-ops",
+ "tags": ["dns", "latency", "warning"],
</file context>
playbooks/dns-restart.yml
Outdated
| - name: Restart pihole service | ||
| command: sudo -u {{ pihole_user }} pihole restartdns | ||
| register: restart_result | ||
| failed_when: restart_result.rc not in [0, 3] |
There was a problem hiding this comment.
P2: The restart task fails the play immediately, so the diagnostics tasks never run when a restart fails. Move failure handling after diagnostics (e.g., block/rescue or defer fail) so diagnostics are collected before stopping the play.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At playbooks/dns-restart.yml, line 32:
<comment>The restart task fails the play immediately, so the diagnostics tasks never run when a restart fails. Move failure handling after diagnostics (e.g., block/rescue or defer fail) so diagnostics are collected before stopping the play.</comment>
<file context>
@@ -0,0 +1,62 @@
+ - name: Restart pihole service
+ command: sudo -u {{ pihole_user }} pihole restartdns
+ register: restart_result
+ failed_when: restart_result.rc not in [0, 3]
+
+ - name: Verify restart succeeded
</file context>
| # Tests that upstream DNS resolution works | ||
| {% for domain in telegraf_dns_external_domains | default(['google.com', 'cloudflare.com', 'github.com']) %} | ||
| [[inputs.exec]] | ||
| commands = ["dig +short {{ domain }} @8.8.8.8 +time=3 +tries=2"] |
There was a problem hiding this comment.
P2: This command returns IP strings, but the metric expects an integer value, so parsing will fail. Emit numeric latency (e.g., parse Query time from +stats).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2, line 35:
<comment>This command returns IP strings, but the metric expects an integer value, so parsing will fail. Emit numeric latency (e.g., parse `Query time` from `+stats`).</comment>
<file context>
@@ -0,0 +1,56 @@
+# Tests that upstream DNS resolution works
+{% for domain in telegraf_dns_external_domains | default(['google.com', 'cloudflare.com', 'github.com']) %}
+[[inputs.exec]]
+ commands = ["dig +short {{ domain }} @8.8.8.8 +time=3 +tries=2"]
+ name_override = "dns_external_latency"
+ interval = "60s"
</file context>
| # Tests that internal DNS resolution works across the fleet | ||
| {% for host in groups['pihole'] %} | ||
| [[inputs.exec]] | ||
| commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }}#5335 +time=3 +tries=2"] |
There was a problem hiding this comment.
P2: This dig +short command outputs an IP string, but the input is configured for integer values, so Telegraf will fail to parse the metric. Output a numeric latency (e.g., parse Query time from +stats) instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2, line 19:
<comment>This `dig +short` command outputs an IP string, but the input is configured for integer values, so Telegraf will fail to parse the metric. Output a numeric latency (e.g., parse `Query time` from `+stats`) instead.</comment>
<file context>
@@ -0,0 +1,56 @@
+# Tests that internal DNS resolution works across the fleet
+{% for host in groups['pihole'] %}
+[[inputs.exec]]
+ commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }}#5335 +time=3 +tries=2"]
+ name_override = "dns_internal_latency"
+ interval = "60s"
</file context>
| { | ||
| "name": "DNS External Latency High", | ||
| "type": "query alert", | ||
| "query": "max(last_5m):max:dns_external_latency{*} > 200", |
There was a problem hiding this comment.
P2: Query threshold (200) conflicts with critical threshold (500). Datadog monitors require the query threshold to match the critical threshold; otherwise the monitor definition is invalid or behaves unexpectedly. Align the query threshold with the critical value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dns-monitoring/datadog-monitor-dns-external-latency.json, line 4:
<comment>Query threshold (200) conflicts with critical threshold (500). Datadog monitors require the query threshold to match the critical threshold; otherwise the monitor definition is invalid or behaves unexpectedly. Align the query threshold with the critical value.</comment>
<file context>
@@ -0,0 +1,21 @@
+{
+ "name": "DNS External Latency High",
+ "type": "query alert",
+ "query": "max(last_5m):max:dns_external_latency{*} > 200",
+ "message": "External DNS latency is above 200ms. Check internet connectivity or upstream DNS.\n\n@slack-ops",
+ "tags": ["dns", "external", "latency"],
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (4)
roles/telegraf-container-service/tasks/config.yml (1)
36-36:| default(true)is redundant —telegraf_dns_monitoring_enabledis alreadytrueindefaults/main.yml. The filter suggests the value might be undefined, which could confuse a future reader about whether the default is authoritative.♻️ Proposed simplification
- when: "telegraf_dns_monitoring_enabled | default(true)" + when: telegraf_dns_monitoring_enabled🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/telegraf-container-service/tasks/config.yml` at line 36, The when condition in tasks/config.yml uses a redundant default filter: remove the "| default(true)" from the expression "telegraf_dns_monitoring_enabled | default(true)" so it simply checks "telegraf_dns_monitoring_enabled" (the default for that variable is already set in defaults/main.yml); update the when clause accordingly in the task that references telegraf_dns_monitoring_enabled to avoid confusing readers about undefined defaults.dns-monitoring/datadog-monitor-dns-down-critical.json (1)
11-11:notify_no_data: falseis a monitoring blind spot.If the Telegraf agent crashes or the Datadog pipeline fails,
pihole.dns_upstops reporting — and this monitor stays silent. Consider enablingnotify_no_data: truewith a suitableno_data_timeframe(e.g.30minutes) so a dead metrics pipeline doesn't mask a real outage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dns-monitoring/datadog-monitor-dns-down-critical.json` at line 11, The monitor currently sets "notify_no_data": false which leaves a blind spot if the metric pipeline stops; update the monitor JSON to set "notify_no_data": true and add a "no_data_timeframe" (e.g. 30) so the Datadog monitor for the metric (pihole.dns_up) alerts when no data is received; locate and modify the "notify_no_data" field in datadog-monitor-dns-down-critical.json and add the "no_data_timeframe" property alongside it.roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2 (2)
24-26:servertag has inconsistent semantics between internal and external sections.In the internal section the
servertag is set to the loop variablehost(the queried DNS server), whereas in the external section (lines 41, 52)serverisinventory_hostname(the Telegraf agent). In monitoring conventionservertypically identifies the reporting agent, not the target endpoint. Aligning both sections — usinginventory_hostnameforserverandhostfortarget— makes dashboards and alert queries consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2` around lines 24 - 26, The internal inputs.exec.tags block currently sets server = "{{ host }}.{{ dns_subdomain_base }}" causing inconsistent semantics with external sections; change it so server uses the reporting agent (inventory_hostname) and target uses the queried DNS server (host), i.e. set server = "{{ inventory_hostname }}" (or "{{ inventory_hostname }}.{{ dns_subdomain_base }}" if domain suffix is required by convention) and keep target = "{{ host }}.{{ dns_subdomain_base }}" to align with external sections and monitoring conventions; update the inputs.exec.tags block in the template accordingly (references: inputs.exec.tags, server, target, host, inventory_hostname).
3-3: Guard against a missingpiholegroup in inventory.
groups['pihole']raises a KeyError if the group is absent. Use the Jinja2 default filter to fail gracefully.✏️ Proposed fix (applies to both `for` loops at lines 3 and 17)
-{% for host in groups['pihole'] %} +{% for host in groups.get('pihole', []) %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2` at line 3, The template iterates over groups['pihole'] which raises KeyError when that inventory group is missing; update both for-loops that reference groups['pihole'] to use a Jinja2 fallback so the loop iterates over an empty list when the group is absent (e.g., apply the default filter or get-style fallback to provide []), ensuring safe iteration and graceful failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dns-monitoring/datadog-monitor-dns-down-critical.json`:
- Line 2: Fix the typo in the Datadog monitor name by changing the "name" field
value that currently reads "DNS Server Down - CRITICAL - PageDuty" to "DNS
Server Down - CRITICAL - PagerDuty"; locate the JSON "name" key in
datadog-monitor-dns-down-critical.json and update the string so the UI and alert
titles use the correct "PagerDuty" spelling.
In `@dns-monitoring/datadog-monitor-dns-down-warning.json`:
- Around line 16-19: The monitor's thresholds object currently sets "critical"
and "warning" both to 1, making the WARNING state unreachable; remove the
"warning" key from the "thresholds" object so the monitor relies solely on the
critical threshold defined in the query (leave "critical": 1 if needed) — update
the JSON where the "thresholds" object appears to delete the "warning" entry and
keep only the "critical" entry to allow the shorter-window Slack warning monitor
to behave as intended.
In `@dns-monitoring/datadog-monitor-dns-external-latency.json`:
- Line 4: The monitor query's threshold (> 200) does not match the configured
critical threshold (options.thresholds.critical = 500), causing Datadog API
validation to fail; update the "query" field in
datadog-monitor-dns-external-latency.json to use > 500 so it exactly matches
options.thresholds.critical, and make the analogous change in
datadog-monitor-dns-latency-warning.json (change > 100 to > 500) so each "query"
string matches its respective options.thresholds.critical value.
In `@dns-monitoring/datadog-monitor-dns-latency-warning.json`:
- Around line 4-5: The monitor query uses a template variable {{server.name}}
but the query "max(last_5m):max:dns_internal_latency{*} > 100" lacks the
required grouping, so add the grouping to the query (e.g., include "by {server}"
in the aggregation) so the multi-alert will evaluate per-server and the
{{server.name}} template in the message can resolve; update the "query" string
accordingly to match the pattern used in other monitors.
- Line 4: The monitor's query uses a 100ms threshold ("query":
"max(last_5m):max:dns_internal_latency{*} > 100") but the monitor payload's
thresholds.critical is set to 500; update thresholds.critical to 100 so the
query and thresholds.critical match for datadog-monitor-dns-latency-warning.json
(and likewise change thresholds.critical to 200 in
datadog-monitor-dns-external-latency.json to match its query > 200); edit the
JSON key "thresholds" -> "critical" in the monitor objects to the matching
numeric values.
In `@openspec/changes/dns-keepalived-analysis/.openspec.yaml`:
- Line 2: The YAML `created` value is unquoted and may be parsed as a date by
some YAML loaders; update the `.openspec.yaml` entry so the `created` field is a
quoted string (e.g., change created: 2026-02-21 to created: "2026-02-21") to
ensure consistent string parsing across YAML parsers and consumers referencing
the `created` key.
In `@openspec/changes/dns-keepalived-analysis/design.md`:
- Line 107: Add a language specifier to the fenced code block that contains the
line "Wisconsin (192.168.20.0/24):" to silence markdownlint MD040; replace the
opening ``` with ```text (or ```plain) so the block starts as ```text and
preserves the existing content, ensuring the fence around "Wisconsin
(192.168.20.0/24):" is updated accordingly.
- Line 134: Replace the incorrect keepalived option name `advertisement_intvl`
with the correct `advert_int` in the document text and any examples referencing
the vrrp_instance advertisement interval; update the sentence "Set lower
`advertisement_intvl` and use `garp_master_delay`" to "Set lower `advert_int`
and use `garp_master_delay`" and scan the file for any other occurrences of
`advertisement_intvl` to correct to `advert_int`.
In `@openspec/changes/dns-keepalived-analysis/tasks.md`:
- Line 4: The task text contains a brand-name typo: replace the word "Ubiquity"
with the correct "Ubiquiti" in the task checkbox line that currently reads
"Verify Ubiquity switch allows VRRP multicast (224.0.0.18)"; update that exact
string so it becomes "Verify Ubiquiti switch allows VRRP multicast
(224.0.0.18)".
In `@openspec/changes/podman-overlayfs.md`:
- Around line 7-11: The spec references tasks in the empty role
podmanSetupRaspberryPi (roles/podmanSetupRaspberryPi/tasks/install.yml and
roles/podmanSetupRaspberryPi/tasks/configure.yml) but the actual ini_file
configuration lives in playbooks/initial-playbook-stage-5.yml; either remove the
role-file references and state that the Podman overlayfs configuration is
implemented only in playbooks/initial-playbook-stage-5.yml, or move the ini_file
tasks into the podmanSetupRaspberryPi role by adding the mandatory task files
(validation, install, configure, systemd management) and updating the spec to
point to roles/podmanSetupRaspberryPi/tasks/configure.yml for setting
/etc/containers/storage.conf, driver = "overlay", and graphroot =
/var/lib/containers/storage.
In `@playbooks/dns-restart.yml`:
- Line 53: Update the `when` guards on the `copy` and `fetch` tasks so they not
only check that `pihole_diag` and `pihole_diag.stdout` are defined but also that
`pihole_diag.stdout != ""`; locate the tasks labeled `copy` and `fetch` in the
play and change the condition from `when: pihole_diag is defined and
pihole_diag.stdout is defined` to include `and pihole_diag.stdout != ""` so an
empty stdout will skip the copy/fetch steps.
- Line 30: The task currently runs "command: sudo -u {{ pihole_user }} pihole
restartdns" while the play uses become: true; remove the inline sudo and instead
set become_user: "{{ pihole_user }}" on the same task (or use a task-level
become_user) so Ansible handles privilege escalation properly; update the other
similar task that also uses "sudo -u {{ pihole_user }}" to the same pattern and
keep the command as "pihole restartdns" (refer to the task that calls pihole
restartdns and the pihole_user variable to locate the changes).
- Around line 29-47: The restart task and subsequent diagnostics are never
reached because the `Restart pihole service` task uses `failed_when` and is not
inside a `block`/`rescue`; wrap the restart and verification tasks in an Ansible
`block` and move the `failed_when` logic into the block so failures fall into a
`rescue` section where you run the `Collect summary diagnostics on failure`
command (`pihole -d -a` registered to `pihole_diag`) and then explicitly fail
(or re-raise) using the `fail` module with context from `restart_result`; keep
the `Display restart result` in the `block`'s `always` or after successful
execution so it only shows on success and ensure task names `Restart pihole
service`, `Verify restart succeeded`, and `Collect summary diagnostics on
failure` remain identifiable.
In `@playbooks/initial-playbook-stage-5.yml`:
- Around line 48-64: The two tasks "Ensure Podman storage driver is overlay" and
"Ensure Podman graphroot is set" use the bare ini_file module; change them to
the FQCN community.general.ini_file, add backup: true to each task, and ensure
the repository declares or the environment has the community.general collection
(add it to requirements.yml or install the collection) before applying the
change to avoid missing-module failures; keep the same
path/section/option/value/mode/create settings when updating the tasks.
In `@roles/pihole.container`:
- Line 1: The submodule pointer in roles/pihole.container moved from 162604c →
2a32007; clone or fetch the upstream repo
git@github.com:NickJLange/pihole.container.git and inspect the commits between
SHA 162604c and 2a32007 to verify the changes, specifically confirm they
implement the intended Podman overlay storage enforcement and do not introduce
breaking changes; if they are intentional, update the PR description to note the
submodule bump and its impact, otherwise revert the submodule pointer in
roles/pihole.container back to 162604c or apply a corrective commit before
merging.
In
`@roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2`:
- Around line 34-54: The two [[inputs.exec]] blocks that run dig (identified by
commands = ["dig +short {{ domain }} `@8.8.8.8` +time=3 +tries=2"] and commands =
["dig +short {{ domain }} `@1.1.1.1` +time=3 +tries=2"] and name_override =
"dns_external_latency") are missing an explicit timeout; add a timeout = "10s"
setting to each of those [[inputs.exec]] blocks (same value/pattern as the
Pi-hole HTTP input) so the exec plugin cannot hang the collection cycle if dig
stalls.
- Line 19: The dig command in the Jinja template uses an incorrect nameserver
port syntax ("@{{ hostvars[host].ansible_host }}#5335") which leaves dig
querying port 53; update the commands entry that builds the dig invocation so it
passes the port via the -p flag (e.g. remove the "#5335" suffix from "{{
hostvars[host].ansible_host }}" and add "-p 5335" before or after the nameserver
argument) while keeping the hostname expansion ({{ host }}.{{ dns_subdomain_base
}}) and the timeout/tries flags intact.
- Around line 18-27: Replace the failing [[inputs.exec]] + dig blocks with
Telegraf's native [[inputs.dns_query]] plugin so you collect query_time_ms
instead of IP text; for the internal measurement create a [[inputs.dns_query]]
block (measurement name_override "dns_internal_latency") using query = ["{{ host
}}.{{ dns_subdomain_base }}"] and set port = 5335 and timeout = "3s", remove
data_format/data_type lines, and add the same tags server and target with "{{
host }}.{{ dns_subdomain_base }}"; do the analogous replacement for the external
block (measurement name_override "dns_external_latency") and point
query/port/timeout appropriately so the metric collected is query_time_ms (a
numeric latency) instead of dig output.
---
Nitpick comments:
In `@dns-monitoring/datadog-monitor-dns-down-critical.json`:
- Line 11: The monitor currently sets "notify_no_data": false which leaves a
blind spot if the metric pipeline stops; update the monitor JSON to set
"notify_no_data": true and add a "no_data_timeframe" (e.g. 30) so the Datadog
monitor for the metric (pihole.dns_up) alerts when no data is received; locate
and modify the "notify_no_data" field in datadog-monitor-dns-down-critical.json
and add the "no_data_timeframe" property alongside it.
In `@roles/telegraf-container-service/tasks/config.yml`:
- Line 36: The when condition in tasks/config.yml uses a redundant default
filter: remove the "| default(true)" from the expression
"telegraf_dns_monitoring_enabled | default(true)" so it simply checks
"telegraf_dns_monitoring_enabled" (the default for that variable is already set
in defaults/main.yml); update the when clause accordingly in the task that
references telegraf_dns_monitoring_enabled to avoid confusing readers about
undefined defaults.
In
`@roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2`:
- Around line 24-26: The internal inputs.exec.tags block currently sets server =
"{{ host }}.{{ dns_subdomain_base }}" causing inconsistent semantics with
external sections; change it so server uses the reporting agent
(inventory_hostname) and target uses the queried DNS server (host), i.e. set
server = "{{ inventory_hostname }}" (or "{{ inventory_hostname }}.{{
dns_subdomain_base }}" if domain suffix is required by convention) and keep
target = "{{ host }}.{{ dns_subdomain_base }}" to align with external sections
and monitoring conventions; update the inputs.exec.tags block in the template
accordingly (references: inputs.exec.tags, server, target, host,
inventory_hostname).
- Line 3: The template iterates over groups['pihole'] which raises KeyError when
that inventory group is missing; update both for-loops that reference
groups['pihole'] to use a Jinja2 fallback so the loop iterates over an empty
list when the group is absent (e.g., apply the default filter or get-style
fallback to provide []), ensuring safe iteration and graceful failure.
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "name": "DNS Server Down - CRITICAL - PageDuty", | |||
There was a problem hiding this comment.
Typo in monitor name: "PageDuty" → "PagerDuty".
This appears in the Datadog UI and in PagerDuty alert titles.
🐛 Proposed fix
- "name": "DNS Server Down - CRITICAL - PageDuty",
+ "name": "DNS Server Down - CRITICAL - PagerDuty",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "DNS Server Down - CRITICAL - PageDuty", | |
| "name": "DNS Server Down - CRITICAL - PagerDuty", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dns-monitoring/datadog-monitor-dns-down-critical.json` at line 2, Fix the
typo in the Datadog monitor name by changing the "name" field value that
currently reads "DNS Server Down - CRITICAL - PageDuty" to "DNS Server Down -
CRITICAL - PagerDuty"; locate the JSON "name" key in
datadog-monitor-dns-down-critical.json and update the string so the UI and alert
titles use the correct "PagerDuty" spelling.
| "thresholds": { | ||
| "critical": 1, | ||
| "warning": 1 | ||
| } |
There was a problem hiding this comment.
Warning and critical thresholds are identical — the WARNING state is unreachable.
There are two threshold types for metric alerts: critical and warning. Critical is defined in the query, but can also be specified in this option. Warning threshold can only be specified using the thresholds option. When both are 1 on a < 1 query, the monitor jumps straight from OK to CRITICAL; the WARNING bucket is never entered. If the intent is to fire a Slack warning on a shorter window (5 min here vs. 15 min on the critical monitor), remove the warning key from thresholds and let the monitor use only the critical threshold:
🐛 Proposed fix
"thresholds": {
- "critical": 1,
- "warning": 1
+ "critical": 1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "thresholds": { | |
| "critical": 1, | |
| "warning": 1 | |
| } | |
| "thresholds": { | |
| "critical": 1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dns-monitoring/datadog-monitor-dns-down-warning.json` around lines 16 - 19,
The monitor's thresholds object currently sets "critical" and "warning" both to
1, making the WARNING state unreachable; remove the "warning" key from the
"thresholds" object so the monitor relies solely on the critical threshold
defined in the query (leave "critical": 1 if needed) — update the JSON where the
"thresholds" object appears to delete the "warning" entry and keep only the
"critical" entry to allow the shorter-window Slack warning monitor to behave as
intended.
| { | ||
| "name": "DNS External Latency High", | ||
| "type": "query alert", | ||
| "query": "max(last_5m):max:dns_external_latency{*} > 200", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "datadog-monitor-dns-external-latency.json" -type fRepository: NickJLange/mkrasberry
Length of output: 123
🏁 Script executed:
# Once file is located, read its full content to check thresholds
cat -n dns-monitoring/datadog-monitor-dns-external-latency.jsonRepository: NickJLange/mkrasberry
Length of output: 804
🏁 Script executed:
# Also check the internal latency monitor mentioned for comparison
find . -name "datadog-monitor-dns-latency*.json" -type f | head -5Repository: NickJLange/mkrasberry
Length of output: 122
🏁 Script executed:
cat -n dns-monitoring/datadog-monitor-dns-latency-warning.jsonRepository: NickJLange/mkrasberry
Length of output: 783
🌐 Web query:
Datadog monitor API query threshold critical threshold validation requirement
💡 Result:
When creating/updating a Datadog query-based monitor via the Monitor API, you must define the threshold twice:
- In the
querystring, as part of the comparison, e.g.... > 200(the query format is... operator #). [1] - In
options.thresholds.critical, and it must match the numeric value used in the query, otherwise/api/v1/monitor/validate(and monitor create/update) can fail with a validation error like “Alert threshold … does not match that used in the query …”. [1][4]
Notes:
warning(and recovery thresholds likecritical_recovery,warning_recovery) are optional extras, butcriticalis the key threshold used for the main alert condition. [1][2][3]- For anomaly monitors, Datadog explicitly documents that the
criticalthreshold inoptions.thresholdsshould match the threshold used in the query. [2]
Sources: Datadog Monitor API docs (query format + validate examples + thresholds object) [1], Datadog anomaly monitor threshold requirement [2], Datadog recovery thresholds via API [3], community-reported validation error when query/thresholds differ [4].
Query threshold must match the critical threshold for Datadog API validation.
The Datadog Monitor API requires the threshold value in the query string to exactly match options.thresholds.critical, or the monitor creation/update will fail with a validation error. Currently, the query uses > 200 (matching the warning threshold) while thresholds.critical is set to 500.
🐛 Proposed fix
- "query": "max(last_5m):max:dns_external_latency{*} > 200",
+ "query": "max(last_5m):max:dns_external_latency{*} > 500",The same issue exists at lines 16–19 in datadog-monitor-dns-latency-warning.json where the query uses > 100 but the critical threshold is 500.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "query": "max(last_5m):max:dns_external_latency{*} > 200", | |
| "query": "max(last_5m):max:dns_external_latency{*} > 500", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dns-monitoring/datadog-monitor-dns-external-latency.json` at line 4, The
monitor query's threshold (> 200) does not match the configured critical
threshold (options.thresholds.critical = 500), causing Datadog API validation to
fail; update the "query" field in datadog-monitor-dns-external-latency.json to
use > 500 so it exactly matches options.thresholds.critical, and make the
analogous change in datadog-monitor-dns-latency-warning.json (change > 100 to >
500) so each "query" string matches its respective options.thresholds.critical
value.
| "query": "max(last_5m):max:dns_internal_latency{*} > 100", | ||
| "message": "DNS latency on {{server.name}} is above 100ms for 5 minutes.\n\n@slack-ops", |
There was a problem hiding this comment.
{{server.name}} won't resolve — query has no by {server} grouping.
If you configure tags or dimensions in your query, these values are available for every group evaluated in the multi alert to dynamically fill in notifications with useful context. Without by {server} in the query the template variable renders empty. Add the grouping to match the pattern used in the other monitors:
🐛 Proposed fix
- "query": "max(last_5m):max:dns_internal_latency{*} > 500",
+ "query": "max(last_5m):max:dns_internal_latency{*} by {server} > 500",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "query": "max(last_5m):max:dns_internal_latency{*} > 100", | |
| "message": "DNS latency on {{server.name}} is above 100ms for 5 minutes.\n\n@slack-ops", | |
| "query": "max(last_5m):max:dns_internal_latency{*} by {server} > 100", | |
| "message": "DNS latency on {{server.name}} is above 100ms for 5 minutes.\n\n@slack-ops", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dns-monitoring/datadog-monitor-dns-latency-warning.json` around lines 4 - 5,
The monitor query uses a template variable {{server.name}} but the query
"max(last_5m):max:dns_internal_latency{*} > 100" lacks the required grouping, so
add the grouping to the query (e.g., include "by {server}" in the aggregation)
so the multi-alert will evaluate per-server and the {{server.name}} template in
the message can resolve; update the "query" string accordingly to match the
pattern used in other monitors.
| { | ||
| "name": "DNS Latency High - WARNING", | ||
| "type": "query alert", | ||
| "query": "max(last_5m):max:dns_internal_latency{*} > 100", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n dns-monitoring/datadog-monitor-dns-latency-warning.jsonRepository: NickJLange/mkrasberry
Length of output: 783
🌐 Web query:
Datadog API alert threshold query parameter validation HTTP 400
💡 Result:
An HTTP 400 from the Datadog Monitors API during “alert threshold … validation” is most commonly caused by a mismatch between the threshold embedded in the query string and the thresholds you send in options.thresholds.
What to check / fix
-
Your
querymust includeoperator #
Datadog monitor queries are expected to end with an operator and a numeric threshold (for example... > 5). The API docs explicitly describe this query format. [1] -
For anomaly monitors, the threshold in the query must match
options.thresholds.critical
Datadog documents that for anomaly monitors the critical threshold should match the threshold used in the query (and related window fields must match too—see#3). If they differ, validation can fail with 400. [2]
A commonly seen error is: “Alert threshold (n) does not match that used in the query (n2).” [3] -
For anomaly monitors, also match the alert window fields
If you’re creating an anomaly monitor:- the query’s
alert_window='...'must matchoptions.threshold_windows.trigger_window - you must set
options.threshold_windowscorrectly (trigger + recovery)
Datadog calls this out in the anomaly monitor API docs. [2]
- the query’s
-
Use the “validate monitor” endpoint to pinpoint the exact validation failure
Before creating/updating, call Datadog’s validate monitor endpoint with the same body; it’s designed to return validation errors for the query/options you’re sending. [1]
Sources: Datadog Monitors API reference (query format + validate endpoint) [1], Datadog anomaly monitor API requirements (threshold + window matching) [2], example of the threshold-mismatch 400 error message in the wild [3].
🏁 Script executed:
fd -t f "datadog-monitor.*\.json" dns-monitoring/ | head -20Repository: NickJLange/mkrasberry
Length of output: 284
🏁 Script executed:
git ls-files dns-monitoring/ | grep -i monitorRepository: NickJLange/mkrasberry
Length of output: 284
🏁 Script executed:
cat -n dns-monitoring/datadog-monitor-dns-down-critical.jsonRepository: NickJLange/mkrasberry
Length of output: 795
🏁 Script executed:
cat -n dns-monitoring/datadog-monitor-dns-down-warning.jsonRepository: NickJLange/mkrasberry
Length of output: 776
🏁 Script executed:
cat -n dns-monitoring/datadog-monitor-dns-external-latency.jsonRepository: NickJLange/mkrasberry
Length of output: 804
Query threshold must match thresholds.critical or the Datadog API will reject this monitor with HTTP 400.
The critical threshold value must match the threshold in the query string; a mismatch causes the Datadog API to return HTTP 400 with "The value provided for parameter 'query' is invalid".
Here the query fires at > 100 but thresholds.critical is 500. Since this is a WARNING alert and the intent is to alert at 100 ms, change thresholds.critical to 100:
🐛 Proposed fix
"thresholds": {
"critical": 100,Note: The same threshold mismatch exists in datadog-monitor-dns-external-latency.json (query > 200, critical 500).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dns-monitoring/datadog-monitor-dns-latency-warning.json` at line 4, The
monitor's query uses a 100ms threshold ("query":
"max(last_5m):max:dns_internal_latency{*} > 100") but the monitor payload's
thresholds.critical is set to 500; update thresholds.critical to 100 so the
query and thresholds.critical match for datadog-monitor-dns-latency-warning.json
(and likewise change thresholds.critical to 200 in
datadog-monitor-dns-external-latency.json to match its query > 200); edit the
JSON key "thresholds" -> "critical" in the monitor objects to the matching
numeric values.
| - name: Ensure Podman storage driver is overlay | ||
| ini_file: | ||
| path: /etc/containers/storage.conf | ||
| section: storage | ||
| option: driver | ||
| value: '"overlay"' | ||
| mode: '0644' | ||
| create: yes | ||
|
|
||
| - name: Ensure Podman graphroot is set | ||
| ini_file: | ||
| path: /etc/containers/storage.conf | ||
| section: storage | ||
| option: graphroot | ||
| value: '"/var/lib/containers/storage"' | ||
| mode: '0644' | ||
| create: yes |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find dependency declaration files
fd -e yml -e yaml "requirements\|galaxy" | head -20Repository: NickJLange/mkrasberry
Length of output: 47
🏁 Script executed:
# List repository structure to understand context
git ls-files | grep -E "(playbooks|roles|requirements|galaxy)" | head -20Repository: NickJLange/mkrasberry
Length of output: 725
🏁 Script executed:
# Check for requirements and galaxy files at repo root and common locations
find . -maxdepth 2 -type f \( -name "requirements*.yml" -o -name "requirements*.yaml" -o -name "galaxy.yml" \)Repository: NickJLange/mkrasberry
Length of output: 47
🏁 Script executed:
# Check if initial-playbook-stage-5.yml exists and read it
cat -n playbooks/initial-playbook-stage-5.yml | head -80Repository: NickJLange/mkrasberry
Length of output: 2607
🏁 Script executed:
# Check for ansible.cfg and other config files
find . -maxdepth 2 -type f -name "ansible.cfg" -o -name ".ansible.cfg"Repository: NickJLange/mkrasberry
Length of output: 47
🏁 Script executed:
# Search for other uses of ini_file and community.general in the repo
rg "ini_file|community\.general" --type yamlRepository: NickJLange/mkrasberry
Length of output: 174
🏁 Script executed:
# Check if there's a requirements file at root
ls -la | grep -i requireRepository: NickJLange/mkrasberry
Length of output: 47
Inconsistency: Use FQCN for ini_file to match the rest of the file.
The ini_file tasks (lines 48-64) use a bare module name while the existing tasks in the same file already use the fully-qualified collection name ansible.builtin.lineinfile (lines 34, 41). Per Ansible 2.10+ best practices, the explicit FQCN should be used.
However, ini_file is provided by the community.general collection, which is not declared as a dependency in this repository. Before applying the proposed fix, verify that community.general is available in your Ansible environment. If it is not configured, either:
- Add
community.generalas a dependency in your requirements.yml or ansible.cfg, or - Ensure the collection is pre-installed in your Ansible environment.
Additionally, consider adding backup: true to both tasks as a safety measure, since an unintended write to /etc/containers/storage.conf would have no rollback path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/initial-playbook-stage-5.yml` around lines 48 - 64, The two tasks
"Ensure Podman storage driver is overlay" and "Ensure Podman graphroot is set"
use the bare ini_file module; change them to the FQCN
community.general.ini_file, add backup: true to each task, and ensure the
repository declares or the environment has the community.general collection (add
it to requirements.yml or install the collection) before applying the change to
avoid missing-module failures; keep the same
path/section/option/value/mode/create settings when updating the tasks.
| @@ -1 +1 @@ | |||
| Subproject commit 162604cb955e7f8e18b4be37da67ac6bdf8cca9a | |||
| Subproject commit 2a320077f7d59d06362dd047083f572c70a8e4a4 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the repository structure and understand the submodule setup
git ls-files --stage | grep roles/pihole.containerRepository: NickJLange/mkrasberry
Length of output: 137
🏁 Script executed:
# Check the .gitmodules file to understand the submodule configuration
if [ -f .gitmodules ]; then
cat .gitmodules
else
echo ".gitmodules not found"
fiRepository: NickJLange/mkrasberry
Length of output: 2575
🏁 Script executed:
# Get the current state of the submodule pointer
if [ -f roles/pihole.container ]; then
cat roles/pihole.container
else
echo "File not found"
fiRepository: NickJLange/mkrasberry
Length of output: 79
🏁 Script executed:
# Check git log to find references to the submodule commits mentioned in the review
git log --oneline --all | grep -E "(162604c|2a32007|pihole|Podman|overlay)" | head -20Repository: NickJLange/mkrasberry
Length of output: 143
🏁 Script executed:
# Check the git log for the submodule pointer changes to see what the previous commit was
git log -p --all -- roles/pihole.container | head -100Repository: NickJLange/mkrasberry
Length of output: 47
Verify the upstream submodule changes are intentional.
The submodule pointer advances from 162604c → 2a32007. Since the submodule is not cloned in this repository, to inspect the upstream commits between these SHAs, clone the submodule or check the upstream repository directly at git@github.com:NickJLange/pihole.container.git. Confirm the changes align with the PR's intent (Podman overlay storage enforcement) and don't introduce breaking changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@roles/pihole.container` at line 1, The submodule pointer in
roles/pihole.container moved from 162604c → 2a32007; clone or fetch the upstream
repo git@github.com:NickJLange/pihole.container.git and inspect the commits
between SHA 162604c and 2a32007 to verify the changes, specifically confirm they
implement the intended Podman overlay storage enforcement and do not introduce
breaking changes; if they are intentional, update the PR description to note the
submodule bump and its impact, otherwise revert the submodule pointer in
roles/pihole.container back to 162604c or apply a corrective commit before
merging.
| [[inputs.exec]] | ||
| commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }}#5335 +time=3 +tries=2"] | ||
| name_override = "dns_internal_latency" | ||
| interval = "60s" | ||
| data_format = "value" | ||
| data_type = "integer" | ||
| [inputs.exec.tags] | ||
| server = "{{ host }}.{{ dns_subdomain_base }}" | ||
| target = "{{ host }}.{{ dns_subdomain_base }}" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Telegraf value parser data_type integer expected format
💡 Result:
For Telegraf’s data_format = "value" with data_type = "integer", the input must be a base-10 integer string that Go’s strconv.Atoi() can parse (for example: 0, 42, -17). Telegraf trims whitespace (and NUL bytes), and if the payload contains multiple whitespace-separated tokens, it uses only the last token as the value. It will error on non-integer formats like 42.0 or 1e3. [1] [2]
Example valid payloads
42\n-123entropy: 987(parses987) [2]
Config snippet
data_format = "value"
data_type = "integer"[1]
🌐 Web query:
Telegraf inputs.dns_query plugin documentation
💡 Result:
Official Telegraf inputs.dns_query (DNS Query Input Plugin) documentation is here: (docs.influxdata.com)
Key config options (from the doc): servers, network (e.g., udp), domains, record_type (A/AAAA/CNAME/MX/NS/PTR/TXT/SOA/SPF/SRV), port, timeout, and include_fields (first_ip / all_ips). (docs.influxdata.com)
Emitted measurement and fields/tags:
- Measurement:
dns_query - Tags:
server,domain,record_type,result,rcode - Fields:
query_time_ms(float),result_code(int: success=0/timeout=1/error=2),rcode_value(int) (docs.influxdata.com)
Citations:
- 1: https://docs.influxdata.com/telegraf/v1/input-plugins/dns_query/
- 2: https://docs.influxdata.com/telegraf/v1/input-plugins/dns_query/
- 3: https://docs.influxdata.com/telegraf/v1/input-plugins/dns_query/
🌐 Web query:
dig +short command output format what does it return
💡 Result:
dig +short switches dig to a terse (“short”) answer format instead of the normal verbose output. (bind9.readthedocs.io)
What it returns (format-wise):
- Only the answer data (RDATA), one result per line, with no header/question/section formatting.
- Example for
A/AAAA: just IP addresses (one per line). (unix.stackexchange.com) - Example for
CNAME: the target name (and if you query the CNAME owner, you may see the CNAME target plus the resolved A/AAAA lines). - Example for
MX: typicallypreferenceandexchangeon one line (e.g.,10 mail.example.com.). - Example for
TXT: the quoted text strings.
- Example for
Related: with +short, +identify can additionally show the server IP and port that supplied the answer (normally omitted in short mode). (bind9.readthedocs.io)
Citations:
- 1: https://bind9.readthedocs.io/en/v9.18.30/manpages.html
- 2: https://unix.stackexchange.com/questions/88938/how-can-i-get-the-output-and-test-the-dig-command?utm_source=openai
- 3: https://bind9.readthedocs.io/en/v9.18.30/manpages.html
dig +short outputs IP addresses, not latency metrics — all DNS measurements will silently fail to collect.
dig +short hostname @server`` returns only the resolved answer data (IP addresses like "192.168.20.250"), not query latency. However, the config specifies `data_type = "integer"`, which expects a base-10 integer parseable by Go's `strconv.Atoi()`. Telegraf's value parser will reject every sample containing an IP address, causing both `dns_internal_latency` and `dns_external_latency` measurements to silently fail to write.
The same applies to the external blocks at lines 34–54.
Replace all [[inputs.exec]] + dig blocks with Telegraf's native [[inputs.dns_query]] plugin, which directly measures DNS query round-trip time via query_time_ms, supports custom port and timeout settings, and requires no shell parsing:
Suggested replacement (internal latency; external is analogous)
-{% for host in groups['pihole'] %}
-[[inputs.exec]]
- commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }}#5335 +time=3 +tries=2"]
- name_override = "dns_internal_latency"
- interval = "60s"
- data_format = "value"
- data_type = "integer"
- [inputs.exec.tags]
- server = "{{ host }}.{{ dns_subdomain_base }}"
- target = "{{ host }}.{{ dns_subdomain_base }}"
-
-{% endfor %}
+{% for host in groups['pihole'] %}
+[[inputs.dns_query]]
+ servers = ["{{ hostvars[host].ansible_host }}"]
+ domains = ["{{ host }}.{{ dns_subdomain_base }}"]
+ port = 5335
+ record_type = "A"
+ timeout = "3s"
+ interval = "60s"
+ name_override = "dns_internal_latency"
+ [inputs.dns_query.tags]
+ server = "{{ inventory_hostname }}.{{ dns_subdomain_base }}"
+ target = "{{ host }}.{{ dns_subdomain_base }}"
+
+{% endfor %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2`
around lines 18 - 27, Replace the failing [[inputs.exec]] + dig blocks with
Telegraf's native [[inputs.dns_query]] plugin so you collect query_time_ms
instead of IP text; for the internal measurement create a [[inputs.dns_query]]
block (measurement name_override "dns_internal_latency") using query = ["{{ host
}}.{{ dns_subdomain_base }}"] and set port = 5335 and timeout = "3s", remove
data_format/data_type lines, and add the same tags server and target with "{{
host }}.{{ dns_subdomain_base }}"; do the analogous replacement for the external
block (measurement name_override "dns_external_latency") and point
query/port/timeout appropriately so the metric collected is query_time_ms (a
numeric latency) instead of dig output.
| # Tests that internal DNS resolution works across the fleet | ||
| {% for host in groups['pihole'] %} | ||
| [[inputs.exec]] | ||
| commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }}#5335 +time=3 +tries=2"] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
dig command @server#port syntax documentation nameserver port
💡 Result:
In BIND 9 dig, the nameserver is specified with @server, and the destination (nameserver) port is specified separately with -p port#:
dig `@SERVER` -p PORT NAME [TYPE]Example:
dig `@192.0.2.53` -p 5353 example.com AThe #port syntax is not for the destination nameserver; it’s used with -b address[#port] to set the source IP (and optional source port) of the query. [1]
-p port# is explicitly documented as “queries a non-standard port number” (destination port). [1]
Sources: BIND 9 dig(1) man page (Oracle) [1].
[1] https://docs.oracle.com/cd/E88353_01/html/E37839/dig-1.html
🌐 Web query:
dig -p port option syntax dig manual
💡 Result:
In dig, the option to set the destination DNS server port is:
- Syntax (from the dig manual):
-p port#(or-p portdepending on the manpage formatting) — tellsdigto send the query to that port instead of 53. [1][8] - Example:
dig@127.0.0.1-p 5353 example.com[1]
Note: -b address[#port] is different—it sets the source address (and optionally source port) for the outgoing query. [1][9]
Sources: NSRC BIND dig manpage [1], Oracle dig(1) manpage [8], Oracle dig(1M) manpage [9].
Fix dig port syntax: use -p flag instead of @host#port.
The dig syntax for specifying a non-standard destination port is -p port# as a separate flag, not as a suffix to the nameserver address. The #port syntax applies only to the -b address[#port] source-address option. Using @host#5335 will query port 53 (default) on the target, not port 5335.
Proposed fix
- commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }}#5335 +time=3 +tries=2"]
+ commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }} -p 5335 +time=3 +tries=2"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }}#5335 +time=3 +tries=2"] | |
| commands = ["dig +short {{ host }}.{{ dns_subdomain_base }} @{{ hostvars[host].ansible_host }} -p 5335 +time=3 +tries=2"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2`
at line 19, The dig command in the Jinja template uses an incorrect nameserver
port syntax ("@{{ hostvars[host].ansible_host }}#5335") which leaves dig
querying port 53; update the commands entry that builds the dig invocation so it
passes the port via the -p flag (e.g. remove the "#5335" suffix from "{{
hostvars[host].ansible_host }}" and add "-p 5335" before or after the nameserver
argument) while keeping the hostname expansion ({{ host }}.{{ dns_subdomain_base
}}) and the timeout/tries flags intact.
| [[inputs.exec]] | ||
| commands = ["dig +short {{ domain }} @8.8.8.8 +time=3 +tries=2"] | ||
| name_override = "dns_external_latency" | ||
| interval = "60s" | ||
| data_format = "value" | ||
| data_type = "integer" | ||
| [inputs.exec.tags] | ||
| server = "{{ inventory_hostname }}.{{ dns_subdomain_base }}" | ||
| upstream = "google-dns" | ||
| domain = "{{ domain }}" | ||
|
|
||
| [[inputs.exec]] | ||
| commands = ["dig +short {{ domain }} @1.1.1.1 +time=3 +tries=2"] | ||
| name_override = "dns_external_latency" | ||
| interval = "60s" | ||
| data_format = "value" | ||
| data_type = "integer" | ||
| [inputs.exec.tags] | ||
| server = "{{ inventory_hostname }}.{{ dns_subdomain_base }}" | ||
| upstream = "cloudflare-dns" | ||
| domain = "{{ domain }}" |
There was a problem hiding this comment.
Missing timeout on [[inputs.exec]] blocks.
The Pi-hole HTTP block (line 8) has timeout = "10s", but none of the [[inputs.exec]] blocks do. If a dig query hangs (e.g., firewall silently drops the packet), the exec plugin uses Telegraf's global default timeout, which could stall the collection cycle. Explicit timeout is safer and matches the pattern already set for the http input.
✏️ Proposed fix (apply to both external blocks)
[[inputs.exec]]
commands = ["dig +short {{ domain }} `@8.8.8.8` +time=3 +tries=2"]
name_override = "dns_external_latency"
interval = "60s"
+ timeout = "10s"
data_format = "value"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[inputs.exec]] | |
| commands = ["dig +short {{ domain }} @8.8.8.8 +time=3 +tries=2"] | |
| name_override = "dns_external_latency" | |
| interval = "60s" | |
| data_format = "value" | |
| data_type = "integer" | |
| [inputs.exec.tags] | |
| server = "{{ inventory_hostname }}.{{ dns_subdomain_base }}" | |
| upstream = "google-dns" | |
| domain = "{{ domain }}" | |
| [[inputs.exec]] | |
| commands = ["dig +short {{ domain }} @1.1.1.1 +time=3 +tries=2"] | |
| name_override = "dns_external_latency" | |
| interval = "60s" | |
| data_format = "value" | |
| data_type = "integer" | |
| [inputs.exec.tags] | |
| server = "{{ inventory_hostname }}.{{ dns_subdomain_base }}" | |
| upstream = "cloudflare-dns" | |
| domain = "{{ domain }}" | |
| [[inputs.exec]] | |
| commands = ["dig +short {{ domain }} `@8.8.8.8` +time=3 +tries=2"] | |
| name_override = "dns_external_latency" | |
| interval = "60s" | |
| timeout = "10s" | |
| data_format = "value" | |
| data_type = "integer" | |
| [inputs.exec.tags] | |
| server = "{{ inventory_hostname }}.{{ dns_subdomain_base }}" | |
| upstream = "google-dns" | |
| domain = "{{ domain }}" | |
| [[inputs.exec]] | |
| commands = ["dig +short {{ domain }} `@1.1.1.1` +time=3 +tries=2"] | |
| name_override = "dns_external_latency" | |
| interval = "60s" | |
| timeout = "10s" | |
| data_format = "value" | |
| data_type = "integer" | |
| [inputs.exec.tags] | |
| server = "{{ inventory_hostname }}.{{ dns_subdomain_base }}" | |
| upstream = "cloudflare-dns" | |
| domain = "{{ domain }}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@roles/telegraf-container-service/templates/etc/telegraf/telegraf.d/dns-monitoring.conf.j2`
around lines 34 - 54, The two [[inputs.exec]] blocks that run dig (identified by
commands = ["dig +short {{ domain }} `@8.8.8.8` +time=3 +tries=2"] and commands =
["dig +short {{ domain }} `@1.1.1.1` +time=3 +tries=2"] and name_override =
"dns_external_latency") are missing an explicit timeout; add a timeout = "10s"
setting to each of those [[inputs.exec]] blocks (same value/pattern as the
Pi-hole HTTP input) so the exec plugin cannot hang the collection cycle if dig
stalls.
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md">
<violation number="1" location="openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md:15">
P2: Podman healthchecks are command-based only; there is no separate `httpGet` healthcheck method. Specifying `httpGet` here is misleading and may result in an unsupported configuration. Consider requiring an exec command that performs the HTTP check instead.</violation>
</file>
<file name="openspec/changes/dns-server-monitoring-tool/specs/dns-health-monitoring/spec.md">
<violation number="1" location="openspec/changes/dns-server-monitoring-tool/specs/dns-health-monitoring/spec.md:4">
P3: The server identifier list uses "new york" with a space, but later you require `server` label values "new_york". Standardize the identifier naming to avoid inconsistent label expectations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| - **THEN** `podman inspect` shows `Health` status as "unhealthy" and triggers any configured container restart policy | ||
|
|
||
| ### Requirement: Healthcheck uses appropriate check method | ||
| The healthcheck SHALL use either exec or httpGet method that works reliably with Podman. |
There was a problem hiding this comment.
P2: Podman healthchecks are command-based only; there is no separate httpGet healthcheck method. Specifying httpGet here is misleading and may result in an unsupported configuration. Consider requiring an exec command that performs the HTTP check instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md, line 15:
<comment>Podman healthchecks are command-based only; there is no separate `httpGet` healthcheck method. Specifying `httpGet` here is misleading and may result in an unsupported configuration. Consider requiring an exec command that performs the HTTP check instead.</comment>
<file context>
@@ -0,0 +1,34 @@
+- **THEN** `podman inspect` shows `Health` status as "unhealthy" and triggers any configured container restart policy
+
+### Requirement: Healthcheck uses appropriate check method
+The healthcheck SHALL use either exec or httpGet method that works reliably with Podman.
+
+#### Scenario: Exec method works
</file context>
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: DNS health metrics collection | ||
| The system SHALL collect DNS health metrics from each Pi-hole server (all, new york, miyami) using OpenTelemetry-compatible format. |
There was a problem hiding this comment.
P3: The server identifier list uses "new york" with a space, but later you require server label values "new_york". Standardize the identifier naming to avoid inconsistent label expectations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At openspec/changes/dns-server-monitoring-tool/specs/dns-health-monitoring/spec.md, line 4:
<comment>The server identifier list uses "new york" with a space, but later you require `server` label values "new_york". Standardize the identifier naming to avoid inconsistent label expectations.</comment>
<file context>
@@ -0,0 +1,38 @@
+## ADDED Requirements
+
+### Requirement: DNS health metrics collection
+The system SHALL collect DNS health metrics from each Pi-hole server (all, new york, miyami) using OpenTelemetry-compatible format.
+
+#### Scenario: Metrics endpoint accessible
</file context>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
openspec/config.yaml (1)
3-10: Consider populating thecontextfield with project-specific information.The
contextsection drives AI-assisted artifact generation in the spec-driven workflow. Since this PR already introduces several spec artifacts (DNS HA design, monitoring specs, tasks), leaving it empty means future artifact creation gets no project-specific guidance. Based on the PR, the relevant context would include the tech stack and domain:✏️ Suggested context content
schema: spec-driven -# Project context (optional) -# This is shown to AI when creating artifacts. -# Add your tech stack, conventions, style guides, domain knowledge, etc. -# Example: -# context: | -# Tech stack: TypeScript, React, Node.js -# We use conventional commits -# Domain: e-commerce platform +context: | + Tech stack: Ansible, Podman, keepalived/VRRP, Telegraf, Datadog, Pi-hole/AdGuard + Infrastructure: Raspberry Pi cluster (home lab), Ubiquiti + commodity switches + Networking: VLANs (192.168.20.x, 192.168.100.x, 192.168.3.x), VIPs at .2 per subnet + Domain: Home-lab DNS high availability and monitoring + Conventions: Spec-driven design docs (proposal → design → spec → tasks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/config.yaml` around lines 3 - 10, The openspec/config.yaml currently leaves the top-level "context" field empty, which deprives AI artifact generation of project-specific guidance; update the "context" key in openspec/config.yaml to include concise project details (e.g., Tech stack: TypeScript, React, Node.js; conventions: conventional commits; domain: e-commerce or whatever matches this repo) so downstream spec artifacts (DNS HA design, monitoring specs, tasks) receive proper guidance—locate the "context" field in openspec/config.yaml and populate it with the suggested tech stack, conventions, and domain information.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/dns-server-monitoring-tool/design.md`:
- Line 91: Fix the compound modifier in the mitigation sentence by changing "5
minute failure threshold" to "5-minute failure threshold" in the line starting
with "Mitigation:" so the phrase correctly uses a hyphenated adjective.
- Line 6: Remove the AI-generated parenthetical note that appears inline after
the server name; specifically delete the text "(note: user said "miyagi"
initially but I'll use what they clarified)" so the line simply contains
`miyami`, ensuring the design document only references the server name `miyami`
consistently (search for the occurrence near the `miyami` token and remove that
parenthetical).
- Line 15: The sentence "Keep changes minimal - we're breaking the house is
acceptable but we want to minimize risk" is ungrammatical; replace it with a
clear, corrected phrasing such as "Keep changes minimal — breaking the build is
acceptable, but we want to minimize risk" or "Keep changes minimal — temporary
breakages are acceptable, but we want to minimize risk" by updating that line in
the document (the problematic sentence quoted above).
- Around line 35-37: The decision description under "D1: Metrics Collection
Method" incorrectly references "Teleporter API"; update the text to reference
Pi-hole's statistics/query API instead and instruct the sidecar Prometheus
exporter to poll the proper endpoints (use /admin/api.php for v5 and /api/ or
/api/stats/summary for v6) so implementers know to query the stats endpoints
rather than the Teleporter backup/restore API.
In
`@openspec/changes/dns-server-monitoring-tool/specs/dns-restart-playbook/spec.md`:
- Around line 47-52: The idempotency claim is incorrect because the playbook
unconditionally runs the side-effecting command pihole restartdns; update the
playbook to make it truly idempotent by adding a pre-check task (e.g., a DNS
health probe or check_dns task) that assesses DNS health and only runs the
pihole restartdns handler when the check reports unhealthy, and ensure the
playbook's idempotency requirement text refers to this conditional restart
behavior (or alternatively, if you prefer not to change tasks, rewrite the
Requirement: Playbook idempotency section to instead state a "safe re-run"
guarantee such as "repeated runs do not leave the server in a degraded state"
and remove the absolute idempotency claim).
In
`@openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md`:
- Around line 14-23: Update the "Requirement: Healthcheck uses appropriate check
method" section to clarify that Podman-native healthchecks support only
exec-style commands (used via podman run --health-cmd, Dockerfile HEALTHCHECK,
or Quadlet), and that kubernetes-style httpGet is not a native Podman method
(when using podman kube play httpGet is translated to an exec that requires curl
or similar in the image). Modify the scenarios to state that for
Podman/Dockerfile/Quadlet the healthcheck must be an exec command (example:
CMD-SHELL curl -sf http://localhost/admin/ || exit 1) and for podman kube play
the YAML may include httpGet but document that it will be executed as an
internal exec and requires appropriate tooling in the container image.
---
Nitpick comments:
In `@openspec/config.yaml`:
- Around line 3-10: The openspec/config.yaml currently leaves the top-level
"context" field empty, which deprives AI artifact generation of project-specific
guidance; update the "context" key in openspec/config.yaml to include concise
project details (e.g., Tech stack: TypeScript, React, Node.js; conventions:
conventional commits; domain: e-commerce or whatever matches this repo) so
downstream spec artifacts (DNS HA design, monitoring specs, tasks) receive
proper guidance—locate the "context" field in openspec/config.yaml and populate
it with the suggested tech stack, conventions, and domain information.
| The home DNS infrastructure consists of three Pi-hole + Unbound servers: | ||
| - `all` (primary) | ||
| - `new york` | ||
| - `miyami` (note: user said "miyagi" initially but I'll use what they clarified) |
There was a problem hiding this comment.
Remove AI-generated internal note.
This parenthetical — (note: user said "miyagi" initially but I'll use what they clarified) — is an AI generation artifact and should not appear in a shared design document. Remove it; the server name miyami is already used consistently everywhere.
📝 Proposed fix
-- `miyami` (note: user said "miyagi" initially but I'll use what they clarified)
+- `miyami`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `miyami` (note: user said "miyagi" initially but I'll use what they clarified) | |
| - `miyami` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/dns-server-monitoring-tool/design.md` at line 6, Remove the
AI-generated parenthetical note that appears inline after the server name;
specifically delete the text "(note: user said "miyagi" initially but I'll use
what they clarified)" so the line simply contains `miyami`, ensuring the design
document only references the server name `miyami` consistently (search for the
occurrence near the `miyami` token and remove that parenthetical).
| - Alerts go to nowhere meaningful | ||
|
|
||
| Constraints: | ||
| - Keep changes minimal - we're breaking the house is acceptable but we want to minimize risk |
There was a problem hiding this comment.
Grammar error: "we're breaking the house is acceptable" is a run-on.
📝 Proposed fix
-- Keep changes minimal - we're breaking the house is acceptable but we want to minimize risk
+- Keep changes minimal - breaking things is acceptable but we want to minimize risk📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Keep changes minimal - we're breaking the house is acceptable but we want to minimize risk | |
| - Keep changes minimal - breaking things is acceptable but we want to minimize risk |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/dns-server-monitoring-tool/design.md` at line 15, The
sentence "Keep changes minimal - we're breaking the house is acceptable but we
want to minimize risk" is ungrammatical; replace it with a clear, corrected
phrasing such as "Keep changes minimal — breaking the build is acceptable, but
we want to minimize risk" or "Keep changes minimal — temporary breakages are
acceptable, but we want to minimize risk" by updating that line in the document
(the problematic sentence quoted above).
| ### D1: Metrics Collection Method | ||
| **Decision:** Use Pi-hole's built-in Teleporter API + a sidecar Prometheus exporter | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the design document
find . -name "design.md" -path "*/dns-server-monitoring-tool*" -type fRepository: NickJLange/mkrasberry
Length of output: 120
🌐 Web query:
Pi-hole Teleporter API documentation official
💡 Result:
Official Pi-hole API documentation is here: Pi-hole API (docs): (docs.pi-hole.net)
Pi-hole also serves the authoritative, version-matched API docs locally from your own instance at: http://pi.hole/api/docs (preferred). (docs.pi-hole.net)
For authentication/session (SID) details, see the official Authentication page: (docs.pi-hole.net)
If you’re specifically looking for “Teleporter” API docs: Teleporter is documented/positioned primarily as a Web UI + backup/restore feature, and Pi-hole’s own release notes call out the CLI method (pihole-FTL --teleporter) rather than a dedicated “Teleporter API” doc page. (pi-hole.net)
Citations:
- 1: https://docs.pi-hole.net/api/?utm_source=openai
- 2: https://docs.pi-hole.net/api/?utm_source=openai
- 3: https://docs.pi-hole.net/api/auth/?utm_source=openai
- 4: https://pi-hole.net/blog/2025/03/30/pi-hole-ftl-v6-1-web-v6-1-and-core-v6-0-6-released/?utm_source=openai
🌐 Web query:
Pi-hole statistics API query API endpoints /admin/api.php
💡 Result:
Important (Pi-hole v6 vs v5)
- Pi-hole v6: the API is not
.../admin/api.phpanymore; it’s hosted under/apiand is documented locally athttp://pi.hole/api/docs. (docs.pi-hole.net)- Auth in v6 is session-based (get an SID via
POST /api/auth). (docs.pi-hole.net)
- Auth in v6 is session-based (get an SID via
- Pi-hole v5 (AdminLTE “legacy” API): stats were commonly queried via
/admin/api.php?...(often with&auth=<token>). (discourse.pi-hole.net)
Legacy statistics endpoints (/admin/api.php) (Pi-hole v5)
Typical usage: http://pi.hole/admin/api.php?<endpoint>[=<value>][&auth=<TOKEN>]
Common stats endpoints:
?summary– formatted summary stats (npmjs.com)?summaryRaw– unformatted “raw” summary stats (npmjs.com)?overTimeData10mins– queries/blocked over time (10-min bins) (npmjs.com)?topItems=<N>– top domains & top advertisers (npmjs.com)?topClients=<N>– top clients (npmjs.com)?getQuerySources– query sources / clients (npmjs.com)?getForwardDestinations– upstream destination breakdown (npmjs.com)?getQueryTypes– query type breakdown (npmjs.com)?getAllQueries– query log data (npmjs.com)?recentBlocked– most recently blocked domain (npmjs.com)
Auth note (legacy): endpoints like status, summary, summaryRaw, overTimeData10mins may require &auth=<TOKEN> when the UI is password protected. (discourse.pi-hole.net)
If you’re on v6 and want “statistics” now
Start at your local docs: http://pi.hole/api/docs (docs.pi-hole.net)
Example stats endpoint shown in Pi-hole’s repo: GET /api/stats/summary (JSON). (github.com)
Citations:
- 1: https://docs.pi-hole.net/api/?utm_source=openai
- 2: https://docs.pi-hole.net/api/auth/?utm_source=openai
- 3: https://discourse.pi-hole.net/t/upcoming-changes-authentication-for-more-api-endpoints-required/59315?utm_source=openai
- 4: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 5: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 6: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 7: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 8: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 9: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 10: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 11: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 12: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 13: https://www.npmjs.com/package/pi-hole-remote?utm_source=openai
- 14: https://discourse.pi-hole.net/t/upcoming-changes-authentication-for-more-api-endpoints-required/59315?utm_source=openai
- 15: https://docs.pi-hole.net/api/?utm_source=openai
- 16: https://github.com/pi-hole/pi-hole/?utm_source=openai
🏁 Script executed:
cat -n openspec/changes/dns-server-monitoring-tool/design.mdRepository: NickJLange/mkrasberry
Length of output: 5097
Replace "Teleporter API" with the correct statistics/query API reference.
The design document incorrectly names the API. Pi-hole's Teleporter is exclusively for backup and restore operations, not metrics collection. The sidecar exporter should poll Pi-hole's statistics/query API instead: /admin/api.php for v5 or /api/ for v6 (e.g., /api/stats/summary). Using the wrong name will misdirect anyone implementing this decision.
📝 Proposed fix
-**Decision:** Use Pi-hole's built-in Teleporter API + a sidecar Prometheus exporter
+**Decision:** Use Pi-hole's statistics/query API (`/admin/api.php` for v5, `/api/` for v6) + a sidecar Prometheus exporter📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### D1: Metrics Collection Method | |
| **Decision:** Use Pi-hole's built-in Teleporter API + a sidecar Prometheus exporter | |
| ### D1: Metrics Collection Method | |
| **Decision:** Use Pi-hole's statistics/query API (`/admin/api.php` for v5, `/api/` for v6) + a sidecar Prometheus exporter | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/dns-server-monitoring-tool/design.md` around lines 35 - 37,
The decision description under "D1: Metrics Collection Method" incorrectly
references "Teleporter API"; update the text to reference Pi-hole's
statistics/query API instead and instruct the sidecar Prometheus exporter to
poll the proper endpoints (use /admin/api.php for v5 and /api/ or
/api/stats/summary for v6) so implementers know to query the stats endpoints
rather than the Teleporter backup/restore API.
| - **Mitigation:** Document the change, ensure backup of current container state before testing | ||
|
|
||
| - **[Risk]** Alert storms if DNS flaps | ||
| - **Mitigation:** Set reasonable hysteresis (e.g., 5 minute failure threshold before alerting) |
There was a problem hiding this comment.
Minor: missing hyphen in compound modifier.
5 minute failure threshold → 5-minute failure threshold.
📝 Proposed fix
- - **Mitigation:** Set reasonable hysteresis (e.g., 5 minute failure threshold before alerting)
+ - **Mitigation:** Set reasonable hysteresis (e.g., 5-minute failure threshold before alerting)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Mitigation:** Set reasonable hysteresis (e.g., 5 minute failure threshold before alerting) | |
| - **Mitigation:** Set reasonable hysteresis (e.g., 5-minute failure threshold before alerting) |
🧰 Tools
🪛 LanguageTool
[grammar] ~91-~91: Use a hyphen to join words.
Context: ...on:** Set reasonable hysteresis (e.g., 5 minute failure threshold before alerting...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/dns-server-monitoring-tool/design.md` at line 91, Fix the
compound modifier in the mitigation sentence by changing "5 minute failure
threshold" to "5-minute failure threshold" in the line starting with
"Mitigation:" so the phrase correctly uses a hyphenated adjective.
| ### Requirement: Playbook idempotency | ||
| The playbook SHALL be idempotent - running it multiple times produces the same result. | ||
|
|
||
| #### Scenario: Multiple runs | ||
| - **WHEN** the playbook is run multiple times on a healthy server | ||
| - **THEN** the server remains healthy and no errors are reported |
There was a problem hiding this comment.
Idempotency definition is inaccurate for an unconditional restart playbook.
As described, the playbook always executes pihole restartdns (Line 30). That is a side-effecting operation, not an idempotent one — running it N times restarts the service N times. The scenario at Line 51–52 ("server remains healthy and no errors are reported") only holds if the restart itself is harmless, but it does not describe a no-op.
Two ways to resolve this:
- Gate the restart: Add a pre-check task that queries DNS health; only restart if unhealthy. This would make the playbook genuinely idempotent.
- Reframe the requirement: Replace "idempotency" with a "safe re-run" guarantee (e.g., "repeated runs do not leave the server in a degraded state"), which is more accurate for an unconditional restart.
📝 Suggested rewrite for the idempotency section
-### Requirement: Playbook idempotency
-The playbook SHALL be idempotent - running it multiple times produces the same result.
-
-#### Scenario: Multiple runs
-- **WHEN** the playbook is run multiple times on a healthy server
-- **THEN** the server remains healthy and no errors are reported
+### Requirement: Safe re-execution
+The playbook SHALL be safe to run multiple times without leaving the server in a degraded state.
+
+#### Scenario: Multiple runs on a healthy server
+- **WHEN** the playbook is run multiple times on a healthy server
+- **THEN** each run restarts the DNS service cleanly and the server remains healthy after each execution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/dns-server-monitoring-tool/specs/dns-restart-playbook/spec.md`
around lines 47 - 52, The idempotency claim is incorrect because the playbook
unconditionally runs the side-effecting command pihole restartdns; update the
playbook to make it truly idempotent by adding a pre-check task (e.g., a DNS
health probe or check_dns task) that assesses DNS health and only runs the
pihole restartdns handler when the check reports unhealthy, and ensure the
playbook's idempotency requirement text refers to this conditional restart
behavior (or alternatively, if you prefer not to change tasks, rewrite the
Requirement: Playbook idempotency section to instead state a "safe re-run"
guarantee such as "repeated runs do not leave the server in a degraded state"
and remove the absolute idempotency claim).
| ### Requirement: Healthcheck uses appropriate check method | ||
| The healthcheck SHALL use either exec or httpGet method that works reliably with Podman. | ||
|
|
||
| #### Scenario: Exec method works | ||
| - **WHEN** healthcheck uses exec to run a command inside the container | ||
| - **THEN** the command executes successfully and returns exit code 0 | ||
|
|
||
| #### Scenario: HTTP method works | ||
| - **WHEN** healthcheck uses httpGet to ping Pi-hole web interface | ||
| - **THEN** the HTTP request returns 200 OK within the timeout period |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the spec file
find . -name "pihole-healthcheck-fix" -type d 2>/dev/nullRepository: NickJLange/mkrasberry
Length of output: 139
🏁 Script executed:
#!/bin/bash
# List files in the spec directory
find . -path "*/pihole-healthcheck-fix/*" -type f 2>/dev/nullRepository: NickJLange/mkrasberry
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Examine the spec file mentioned in the review
if [ -f "openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md" ]; then
cat -n "openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md" | head -50
fiRepository: NickJLange/mkrasberry
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Check the complete spec file to understand deployment context
cat -n "openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md"Repository: NickJLange/mkrasberry
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Search for any implementation details or deployment configuration in the repository
find . -type f \( -name "*.md" -o -name "*.yaml" -o -name "*.yml" -o -name "Dockerfile" \) | xargs grep -l "pihole\|healthcheck" | head -20Repository: NickJLange/mkrasberry
Length of output: 778
🌐 Web query:
Podman healthcheck methods supported native exec httpGet
💡 Result:
Podman healthchecks are command/exec-based only (Dockerfile HEALTHCHECK / podman run|create --health-cmd). There is no native httpGet healthcheck type like Kubernetes probes; to do HTTP you run a command such as curl/wget inside the container and return a non-zero exit code on failure. [1], [2]
Supported forms are essentially Docker’s:
CMD(exec form / JSON array)CMD-SHELL(shell form / string run via/bin/sh -c)NONE(disable) [1]
Example (HTTP via exec command):
podman create --name hc-nginx \
--health-cmd '["CMD-SHELL","wget -q --spider http://127.0.0.1:80 || exit 1"]' \
--health-interval 30s --health-timeout 5s --health-retries 3 \
nginx:alpine[2]
If you use podman kube play with Kubernetes httpGet/tcpSocket probes, Podman currently converts them into exec checks (e.g., using curl/nc inside the container), so those binaries must exist in the image or the probe will fail. [3]
Sources:
[1] Podman healthcheck docs/manpage (supported --health-cmd string vs array; disable options) (docs.podman.io)
[2] Podman run docs (example using --health-cmd with CMD-SHELL and wget) (docs.podman.io)
[3] Podman issue explaining httpGet/tcpSocket probes are converted to exec (curl/nc) (github.com)
Citations:
- 1: https://docs.podman.io/en/latest/markdown/podman-healthcheck.1.html?utm_source=openai
- 2: https://docs.podman.io/en/latest/markdown/podman-run.1.html?utm_source=openai
- 3: Cannot use Kubernetes healthcheck probes without certain executables (tcpSocket/nc, httpGet/curl) inside the container containers/podman#18318
🌐 Web query:
Podman health-cmd exec-only vs httpGet support
💡 Result:
-
Podman
--health-cmdis exec-based only. Podman healthchecks are defined as a command that runs inside the container (string form viaCMD-SHELL, array form viaCMD). There isn’t a first-class Podman healthcheck type likehttpGet; you express HTTP checks by running something likecurl/wget/nc(or your own binary/script) in the health command. [1] -
httpGetexists in Kubernetes probes, not Podman healthchecks. In Kubernetes,httpGetis a built-in probe mechanism (the kubelet performs the HTTP request), alongsidetcpSocketandexec. [2] -
When using
podman kube play, KuberneteshttpGet/tcpSocketprobes get translated intoexec(e.g.,httpGet→ runcurlin the container;tcpSocket→ runncin the container). This means your container image must include those tools (or the probe will fail even if the service is up). [3]
Implication: If you want “httpGet-style” health checks in Podman, you currently implement them as --health-cmd exec checks (and ensure the needed client binary exists in the image), or avoid relying on podman kube play probe translation for minimal images. [1][3]
Sources:
[1] Podman healthcheck man page (--health-cmd semantics) (docs.podman.io)
[2] Kubernetes probe mechanisms (httpGet, tcpSocket, exec) (kubernetes.io)
[3] Podman issue documenting httpGet/tcpSocket → exec conversion and curl/nc dependency (github.com)
Citations:
- 1: https://docs.podman.io/en/latest/markdown/podman-healthcheck.1.html
- 2: https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/?utm_source=openai
- 3: Cannot use Kubernetes healthcheck probes without certain executables (tcpSocket/nc, httpGet/curl) inside the container containers/podman#18318
🌐 Web query:
podman kube play httpGet conversion to exec
💡 Result:
podman kube play does not implement Kubernetes’ httpGet probe as an “outside-the-container” HTTP request. Instead, it translates Kubernetes probe types like httpGet (and tcpSocket) into a container exec-style healthcheck, typically by running tools like curl (or nc) inside the target container. If your image doesn’t include those binaries, the probe will fail even if the service is actually reachable. [1]
So, in practice, an httpGet probe becomes equivalent to you writing an exec probe yourself (e.g., curl --fail http://127.0.0.1:PORT/path). [1]
Notes/implications:
- Podman’s kube support is incomplete; for example, the docs list
livenessProbeas supported, butreadinessProbe/startupProbeas not supported. [2] - Workarounds are to (a) add
curl/wget(or similar) to the image, (b) rewrite the probe explicitly asexecusing whatever your image already has, or (c) remove/disable the probe forpodman kube playusage. [1]
Sources:
[1] Podman issue explaining httpGet/tcpSocket are converted to exec and rely on curl/nc inside the container: GitHub issue #18318. (github.com)
[2] podman kube play supported fields table (probe support status): Podman docs. (docs.podman.io)
Citations:
- 1: Cannot use Kubernetes healthcheck probes without certain executables (tcpSocket/nc, httpGet/curl) inside the container containers/podman#18318
- 2: https://docs.podman.io/en/latest/markdown/podman-kube-play.1.html
httpGet is a Kubernetes probe concept, not a native Podman healthcheck method.
Podman's native healthcheck executes a command inside the container and waits for an exit code. exec is the only method available when defining healthchecks via podman run --health-cmd, Dockerfile HEALTHCHECK, or Quadlet. There is no native httpGet directive in these contexts.
httpGet is a Kubernetes liveness/readiness probe field. When using podman kube play, healthcheck probes with httpGet are converted to exec commands internally (e.g., running curl inside the container), not executed as external HTTP requests.
The spec should clarify which deployment model is in use:
- If the Pi-hole container is run via
podman run, Dockerfile, or a Quadlet unit, onlyexecis available, and an HTTP check must be implemented as an exec command (e.g.,CMD-SHELL curl -sf http://localhost/admin/ || exit 1). - If
podman kube playis used,httpGetcan be specified in the YAML but it will still be executed as an exec command internally, requiring the appropriate tools (e.g.,curl) to be present in the container image.
📝 Suggested revision to Requirement lines 14-23
-### Requirement: Healthcheck uses appropriate check method
-The healthcheck SHALL use either exec or httpGet method that works reliably with Podman.
+### Requirement: Healthcheck uses an exec-based check method
+The healthcheck SHALL use an exec command that works reliably with Podman's native healthcheck mechanism.
-#### Scenario: Exec method works
-- **WHEN** healthcheck uses exec to run a command inside the container
-- **THEN** the command executes successfully and returns exit code 0
-
-#### Scenario: HTTP method works
-- **WHEN** healthcheck uses httpGet to ping Pi-hole web interface
-- **THEN** the HTTP request returns 200 OK within the timeout period
+#### Scenario: Exec method works
+- **WHEN** healthcheck uses exec to run a command inside the container (e.g., `curl -sf http://localhost/admin/` or `dig +short pi.hole `@127.0.0.1``)
+- **THEN** the command executes successfully and returns exit code 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/dns-server-monitoring-tool/specs/pihole-healthcheck-fix/spec.md`
around lines 14 - 23, Update the "Requirement: Healthcheck uses appropriate
check method" section to clarify that Podman-native healthchecks support only
exec-style commands (used via podman run --health-cmd, Dockerfile HEALTHCHECK,
or Quadlet), and that kubernetes-style httpGet is not a native Podman method
(when using podman kube play httpGet is translated to an exec that requires curl
or similar in the image). Modify the scenarios to state that for
Podman/Dockerfile/Quadlet the healthcheck must be an exec command (example:
CMD-SHELL curl -sf http://localhost/admin/ || exit 1) and for podman kube play
the YAML may include httpGet but document that it will be executed as an
internal exec and requires appropriate tooling in the container image.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
playbooks/dns-restart.yml (2)
24-27:pihole_statusis registered but never referenced.The result of
Check pihole container statusis captured inpihole_statusat line 26, but no subsequent task consults it. Either add adebugtask to surface the status before restarting, or drop theregisterclause.♻️ Proposed refactor (add debug, keep register for visibility)
- name: Check pihole container status command: sudo -u {{ pihole_user }} podman ps --filter name=pihole register: pihole_status changed_when: false + - name: Display current container status + debug: + msg: "{{ pihole_status.stdout }}" +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/dns-restart.yml` around lines 24 - 27, The task named "Check pihole container status" currently registers pihole_status but nothing uses it; either remove the register clause on that task or add a follow-up task that surfaces the result (e.g., a debug task referencing pihole_status.stdout / pihole_status.stdout_lines) so the status is visible before the restart; update the playbook by modifying the task with name "Check pihole container status" or adding a new debug task that references pihole_status.
30-30: Preferpodman restart piholeover ash -c "podman stop … && podman start …"chain viacommand.Two concerns:
- When shell operators like
&&are needed, theshellmodule should be used rather than thecommandmodule.podman restartis an atomic single operation; thestop && startchain silently skipsstartifstopexits non-zero.♻️ Proposed refactor
- - name: Restart pihole service - command: sudo -u {{ pihole_user }} sh -c "podman stop pihole && podman start pihole" + - name: Restart pihole service + command: podman restart pihole + become_user: "{{ pihole_user }}" register: restart_result failed_when: restart_result.rc != 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/dns-restart.yml` at line 30, Replace the current task that runs sudo -u {{ pihole_user }} sh -c "podman stop pihole && podman start pihole" with a single atomic restart using the shell module: run podman restart pihole as the pihole_user (preserve the existing sudo invocation or use become_user/{{ pihole_user }} if the surrounding play supports it) so you avoid the stop && start chain and ensure correct module usage (shell when using shell operators) and atomic restart behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playbooks/dns-restart.yml`:
- Around line 32-37: The two checks disagree and make the "Verify restart
succeeded" task dead — make them consistent by allowing the historical rc=3
success code: change the failed_when condition to use "failed_when:
restart_result.rc not in [0, 3]" (referencing failed_when and restart_result.rc)
and remove the redundant "Verify restart succeeded" fail task (the task named
"Verify restart succeeded") so only the single consistent failure check remains.
---
Duplicate comments:
In `@playbooks/dns-restart.yml`:
- Line 53: The when guard that checks pihole_diag currently only ensures the
variable and stdout exist but not that stdout is non-empty; update the when
expressions on the tasks that run the copy/fetch (the tasks that reference
pihole_diag and pihole_diag.stdout around the copy/fetch steps) to add
pihole_diag.stdout != "" so the tasks skip when output is empty; apply the same
change for both occurrences noted (the guard at the copy task and the similar
guard at the other fetch/copy task).
- Around line 29-47: Wrap the restart and verification tasks in an Ansible block
with a rescue section so diagnostics always run on failure: move "Restart pihole
service" and "Verify restart succeeded" into a block and place "Collect summary
diagnostics on failure" in the rescue (referencing the registered restart_result
and pihole_diag) and ensure the rescue uses the same sudo/podman exec command;
keep "Display restart result" in the block's always or after the block and
conditionally show success only when restart_result.rc in [0,3] to avoid the
play halting before diagnostics run.
- Line 25: Replace the explicit "sudo -u {{ pihole_user }}" in the task commands
(e.g., the task that currently uses "command: sudo -u {{ pihole_user }} podman
ps --filter name=pihole" and the similar commands at the other occurrences) with
Ansible privilege escalation by adding become_user: "{{ pihole_user }}" on each
task, remove the inline "sudo -u ..." from the command strings, and ensure each
task keeps become: true so elevation is handled by Ansible rather than requiring
passwordless sudo for switching to pihole.
---
Nitpick comments:
In `@playbooks/dns-restart.yml`:
- Around line 24-27: The task named "Check pihole container status" currently
registers pihole_status but nothing uses it; either remove the register clause
on that task or add a follow-up task that surfaces the result (e.g., a debug
task referencing pihole_status.stdout / pihole_status.stdout_lines) so the
status is visible before the restart; update the playbook by modifying the task
with name "Check pihole container status" or adding a new debug task that
references pihole_status.
- Line 30: Replace the current task that runs sudo -u {{ pihole_user }} sh -c
"podman stop pihole && podman start pihole" with a single atomic restart using
the shell module: run podman restart pihole as the pihole_user (preserve the
existing sudo invocation or use become_user/{{ pihole_user }} if the surrounding
play supports it) so you avoid the stop && start chain and ensure correct module
usage (shell when using shell operators) and atomic restart behavior.
| failed_when: restart_result.rc != 0 | ||
|
|
||
| - name: Verify restart succeeded | ||
| fail: | ||
| msg: "Pihole restart failed with rc={{ restart_result.rc }}" | ||
| when: restart_result.rc not in [0, 3] |
There was a problem hiding this comment.
Verify restart succeeded is dead code — inconsistent rc thresholds between lines 32 and 37.
failed_when: restart_result.rc != 0 (line 32) causes the task to fail immediately on any non-zero rc, including rc=3. Therefore:
- If
rc=3: the play halts at line 32 — line 37'swhenis never evaluated. - If
rc=0: line 37'swhen: restart_result.rc not in [0, 3]isFalse— thefailmodule never runs.
The Verify restart succeeded block (lines 34–37) is unreachable in every scenario. The not in [0, 3] guard is a leftover from the old pihole restartdns behavior and should be removed. Fix both conditions to agree:
🐛 Proposed fix
- - name: Restart pihole service
- command: sudo -u {{ pihole_user }} sh -c "podman stop pihole && podman start pihole"
- register: restart_result
- failed_when: restart_result.rc != 0
-
- - name: Verify restart succeeded
- fail:
- msg: "Pihole restart failed with rc={{ restart_result.rc }}"
- when: restart_result.rc not in [0, 3]
+ - name: Restart pihole service
+ command: sudo -u {{ pihole_user }} podman restart pihole
+ register: restart_result
+ failed_when: restart_result.rc != 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/dns-restart.yml` around lines 32 - 37, The two checks disagree and
make the "Verify restart succeeded" task dead — make them consistent by allowing
the historical rc=3 success code: change the failed_when condition to use
"failed_when: restart_result.rc not in [0, 3]" (referencing failed_when and
restart_result.rc) and remove the redundant "Verify restart succeeded" fail task
(the task named "Verify restart succeeded") so only the single consistent
failure check remains.
1 participant
Summary
Analysis document for implementing DNS high availability using keepalived/VRRP.
Key Decisions
Changes
Next Steps
This is an analysis-only change - implementation would be a separate effort.
Summary by cubic
Adds a keepalived/VRRP analysis for DNS high availability and ships DNS health monitoring, alerting, and a restart playbook. Standardizes Podman on the overlay driver, fixes the Pi-hole healthcheck, and updates the restart playbook to stop/start Pi-hole as the pihole user and restart Unbound.
New Features
Migration
Written for commit c0acd5e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Chores