Repository navigation
fix: restore node-level DNS after unexpected LocalDNS exit - #9360
Conversation
Windows Unit Test Results 3 files 14 suites 53s ⏱️ Results for commit c57ca09. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Removes unnecessary LocalDNS cgroup delegation to prevent teardown failures after unclean supervisor termination.
Changes:
- Sets
Delegate=noforlocaldns.service. - Adds VHD-content validation for the setting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
parts/linux/cloud-init/artifacts/localdns-delegate.conf |
Disables cgroup delegation. |
vhdbuilder/packer/test/linux-vhd-content-test.sh |
Verifies delegation remains disabled. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Ye comment follow-up: the lifecycle validator now distinguishes empty or unreadable resolver state from resolver state that still contains 169.254.10.10. After successful restoration, it also positively verifies |
There was a problem hiding this comment.
🔵 Needs a closer look
The E2E cleanup can leave LocalDNS start-limited and failed when an earlier assertion fails.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
e2e/scenario/scenario_localdns_hosts.go:80
- 🟡 Medium Risk — 🔧 Script Logic:
reset-failedonly runs when the temporary override already exists. If an earlier assertion or one of the restart-loop attempts fails after LocalDNS reachesStartLimit, the EXIT trap skips the reset and the latersystemctl startremains rate-limited, leaving LocalDNS failed on the test node. Reset the unit unconditionally before checking/restarting it.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Unbounded iptables locking can prevent the post-stop hook from reaching the DNS restoration steps.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The lifecycle validator can exhaust systemd’s start limit and fail to restore LocalDNS on early failures.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
e2e/scenario/scenario_localdns_hosts.go:80
- 🟡 Medium Risk — 🔧 Script Logic:
reset-failedonly runs after the temporary override exists. If the earlier SIGKILL/restart checks hit StartLimit or otherwise leave LocalDNS failed, the EXIT trap skips the reset,systemctl startremains rate-limited, and this validator leaves the scenario node without LocalDNS. Reset the unit unconditionally before attempting restoration.
e2e/scenario/scenario_localdns_hosts.go:119 - 🟡 Medium Risk — 🧪 Test Coverage: Reset the systemd start-rate counter immediately before this crash sequence. The hosts-plugin validator cold-starts LocalDNS, and this validator then performs a restart plus a stop/start before requesting three automatic recoveries; with the repository-documented default StartLimitBurst of 5 starts per 10 seconds (
parts/linux/cloud-init/artifacts/cse_config.sh:1520-1522), a fast run can deny the third recovery and fail for test-generated rate limiting rather than the behavior under test.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
AgentBaker Linux gate detective
|
Ye Wang (yewmsft)
left a comment
Ye Wang (yewmsft)
left a comment
There was a problem hiding this comment.
went through all 20 of my threads against dd3a634f. everything is addressed except the one inline note below on NRestarts; two smaller things after that.
what I actually verified rather than read:
- ran
localdns.sh cleanupas a fresh root process withDEFAULT_ROUTE_INTERFACE,NETWORK_DROPIN_FILEandNETWORK_DROPIN_DIRall unset — it removes/run/systemd/network/*.d/70-localdns.confby glob and exits 0. andNETWORK_DROPIN_DIRis always derived under/run/systemd/network/(localdns.sh:399), so the glob is a complete sweep, not a best-effort one. this was my main concern and it's properly fixed. - with
iptables -Dforced to fail, the drop-in is still removed, the reload still runs, and the function returns 1. moving DNS restore ahead of iptables is a good call — I didn't ask for that and it's the right ordering. - the gate move is confirmed by CI, not just by inspection: on the standalone run (main's published VHD)
LocalDNSHostsPlugin/AzureLinuxV3and/Ubuntu2404pass, and on the branch-VHD run all three lifecycle scenarios pass. that's exactly the split we wanted.
on the red checks — not yours. both E2E failures are check localdns cold start behaviour, with mcr.microsoft.com coming back SERVFAIL from CoreDNS right after cold start. that validator lives in ValidateCommonLinux, which runs before Config.Validator, so the lifecycle code can't be the cause — and it hit Ubuntu2204 on one run and ACL on the other, where ACL doesn't run the lifecycle validator at all. looks like an existing upstream-resolution flake.
nit, not blocking: eval "$NETWORKCTL_RELOAD_CMD" survived into the rewritten block at localdns.sh:689. it's a constant so there's no injection, but everything around it is new code now — worth making it an array while you're in there.
Ye Wang (yewmsft)
left a comment
Ye Wang (yewmsft)
left a comment
There was a problem hiding this comment.
took another pass over the whole change rather than thread-by-thread. the fix is correct and the mechanism holds — a few notes on what I checked, plus one new finding in the test harness and a correction to my own suggestion from earlier.
the core fix is sound, and one detail makes it work that's worth naming so nobody "tidies" it later: the cleanup dispatch at localdns.sh:1020 sits above the trap installation at :1081/:1084. that's why cleanup mode never installs the EXIT trap, never reaches cleanup_localdns_configs, and so genuinely leaves the dummy interface up the way the comment promises. move that dispatch below the traps and the documented behaviour silently inverts. I ran localdns.sh cleanup as root with no env at all and confirmed the output is drop-in removal + reload + iptables only, no interface teardown.
also confirmed the glob is a complete sweep rather than a best-effort one: NETWORK_DROPIN_DIR is only ever derived as /run/systemd/network/<file>.d (:399), and /etc/localdns/environment — the unit's only EnvironmentFile — carries just the corefile/hosts/FQDN keys, so nothing can relocate the drop-in out from under the pattern. and cleanup_iptables_and_dns || echo suspends set -e for the whole function body, so a mid-cleanup failure can't abort it; every variable it touches is either a top-level constant or :--guarded, so set -u is satisfied too.
on the restart-cycle guarantee: each crash's ExecStopPost reverts node DNS, each ExecStart re-adds it, and when StartLimit finally trips the last ExecStopPost has already run — so the terminal state is upstream DNS, not a blackhole. that's the thing the PR set out to do and it does it.
one inconsistency in the rationale, not a bug: cleanup mode keeps the dummy interface so an orphaned CoreDNS can keep serving pods, but it still removes the localdns: skip conntrack rules that orphan depends on. DNS keeps working, it just goes through conntrack — which is the thing localdns exists to avoid. worth a clause in the comment so the reasoning doesn't read as stronger than it is.
There was a problem hiding this comment.
🔵 Needs a closer look
The E2E trap can leave LocalDNS failed when an early restart assertion reaches StartLimit.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
e2e/scenario/scenario_localdns_hosts.go:80
- 🟡 Medium Risk — E2E cleanup:
reset-failedonly runs after the temporary override exists. If one of the earlier restart/kill assertions trips systemd's StartLimit, the EXIT trap skips this branch and the subsequentsystemctl startcan be rejected, leaving LocalDNS failed on the shared scenario node. Reset the unit's failure state unconditionally before attempting restoration.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Changes cached containers or packages on windows VHDsPlease get a Windows SIG member to approve. The following dif file shows any additions or deletions from what will be cached on windows VHDs organised by VHD type.
diff --git a/vhd_files/2025-gen2-tl.txt b/vhd_files/2025-gen2-tl.txt
index 9e68675..d494597 100644
--- a/vhd_files/2025-gen2-tl.txt
+++ b/vhd_files/2025-gen2-tl.txt
@@ -15 +15 @@ c:\akse-cache\csi-proxy\: https://packages.aks.azure.com/csi-proxy/v1.1.2-hotfix
-c:\akse-cache\wcn\: mcr.microsoft.com/wcn/package:1.7.1-cpu-arch
+c:\akse-cache\wcn\: mcr.microsoft.com/wcn/package:1.8.0-cpu-arch
diff --git a/vhd_files/2025-gen2.txt b/vhd_files/2025-gen2.txt
index bfacb1a..00f0445 100644
--- a/vhd_files/2025-gen2.txt
+++ b/vhd_files/2025-gen2.txt
@@ -15 +15 @@ c:\akse-cache\csi-proxy\: https://packages.aks.azure.com/csi-proxy/v1.1.2-hotfix
-c:\akse-cache\wcn\: mcr.microsoft.com/wcn/package:1.7.1-cpu-arch
+c:\akse-cache\wcn\: mcr.microsoft.com/wcn/package:1.8.0-cpu-arch
diff --git a/vhd_files/2025.txt b/vhd_files/2025.txt
index dab08f5..29ac686 100644
--- a/vhd_files/2025.txt
+++ b/vhd_files/2025.txt
@@ -15 +15 @@ c:\akse-cache\csi-proxy\: https://packages.aks.azure.com/csi-proxy/v1.1.2-hotfix
-c:\akse-cache\wcn\: mcr.microsoft.com/wcn/package:1.7.1-cpu-arch
+c:\akse-cache\wcn\: mcr.microsoft.com/wcn/package:1.8.0-cpu-arch |
|
|
There was a problem hiding this comment.
🔵 Needs a closer look
The lifecycle validator silently passes when the VHD lacks the cleanup hook it is intended to verify.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
e2e/scenario/scenario_localdns_hosts.go:253
- 🟡 Medium Risk — 🧪 Test Coverage: This fallback makes the lifecycle validator pass when the branch VHD is missing the
ExecStopPosthook, so a packaging regression can silently skip the behavior this PR claims to validate. The three gated branch-VHD scenarios should fail when the hook is absent; compatibility testing for older VHDs should be separated from this regression check.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
There was a problem hiding this comment.
🔵 Needs a closer look
Production-critical DNS behavior in the latest baked artifacts still requires the pending fresh branch-VHD cloud E2E run.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Ye Wang (yewmsft)
left a comment
Ye Wang (yewmsft)
left a comment
There was a problem hiding this comment.
approving.
verified yesterday's three follow-ups actually landed in c57ca094, not just in the commit messages:
restarts_beforedropped entirely;test "$restarts_after" -ge 3with a diagnostic||block. absolute count, no baseline that can't mean anything.reset-failedhoisted out of the$NORESTARTguard, ahead of the is-active/start check.control_groupassert now prints on failure instead of dying silently underset -e.
on the change itself: ExecStopPost= is the right hook — it fires on SIGKILL, which is the incident path and where ExecStop= would not run at all. restoring DNS before iptables with a cleanup_failed accumulator is the right priority ordering: an xtables lock can no longer delay removal of the drop-in that points the node at a dead listener. dropping the initialize_network_variables fallback in favour of the /run/systemd/network/*.d/70-localdns.conf glob is correct for the post-exit case where route discovery can fail — i ran localdns.sh cleanup as a fresh root process with DEFAULT_ROUTE_INTERFACE / NETWORK_DROPIN_FILE / NETWORK_DROPIN_DIR all unset and it removed the drop-in, reloaded, and exited 0. and the cleanup dispatch sitting above trap installation is what keeps the dummy interface up, which is right given an orphaned coredns may still be serving pods.
scope: pod DNS on .11 is still dead while localdns is, and that's correct for this PR — #9486 covers it, stacked behind #9439. merge order is 9360 -> 9439 -> 9486. all three touch localdns.service, so expect a rebase between each merge rather than just a retarget.
e2e: 6 of 7 localdns scenarios green. LocalDNSHostsPlugin/AzureLinuxV3 is the cold-start SERVFAIL, which is #9500, not this change. worth noting the new lifecycle validator therefore hasn't run on AzureLinuxV3 yet — it's green on Ubuntu2204 and Ubuntu2404, the other two distros it's gated to.
|
comprehensive test result here: #9503 (comment) Conclusive Live Validation — All Three Problems ResolvedAll three PR problems were reproduced and confirmed fixed on a single fresh, LocalDNS-enabled Ubuntu 22.04 agent node, patched with the latest cherry-picked artifacts from the combined branch. Setup
PR #9439 — Deterministic terminal failure (previously inconclusive) — NOW CONFIRMEDFault: sabotaged the coredns binary path and drove repeated restarts to exhaust the budget. Result: the bounded budget lands LocalDNS deterministically in PR #9471 — Listener/invalid upstream rejection (previously inconclusive live) — NOW CONFIRMEDThe on-node branch No LocalDNS listener IP was ever written into the generated corefile in any reject case. Valid upstreams forward correctly. Confirmed live (8/8 cases). PR #9360 — Node DNS restoration after terminal exit — RE-CONFIRMEDTerminal dead-service test on the same patched node: Confirmed live. Final verdict
Based on the latest cherry-picked commits from PRs #9360, #9439, and #9471, all three problems are no longer present on the live cluster's LocalDNS-enabled agent node. The disposable node was deleted after testing. |
|
Ye Wang (@yewmsft) — this is implemented in #9486, and all three constraints held up. Point by point, with what was verified on a live node: The proposal itself. Constraint 1 — idempotent interface creation. Constraint 2 — stop before Constraint 3 — depends on #9439's numbers. Merged. Live node: You were right to reject the kubelet repoint. It got built anyway as a complement rather than a replacement, you objected again on #9486, and it has been removed. Of your three original objections, reframing it as a complement answers only the first; restarting kubelet on an already-degraded node and drifting from RP goal state apply either way. It can return as its own PR once someone measures the NotReady window across a kubelet restart against Two of your assumptions were corrected by measurement rather than quietly worked around, since both changed the design:
Outcome. Live cluster, 6/6 phases, 55 assertions, 0 failures — including the round trip you described: pre-failure pod stays pinned to The one claim of yours still unverified is the headline number: "the reported 16 minute blackhole becomes ~43s." The scenario bounds recovery at ≤30s via a And your last line turned into its own PR — the flapping classes that never reach |
Problem
This PR addresses the LocalDNS failure reported by Azure/AKS#5930.
localdns.servicesuperviseslocaldns.sh, which starts CoreDNS as a background process. During an unexpected supervisor exit, such asSIGKILL, the script cannot run its cleanup traps. The node can therefore retain the network drop-in that points DNS at LocalDNS while the resolver is no longer available. Restart attempts can also encounter leftover processes and fail, eventually leaving the service dead and causing a node-level DNS outage.Scope: this PR restores node-level (host) DNS only. It reverts the node host resolver (the
169.254.10.10node-listener drop-in and iptables rules) after an unexpected LocalDNS exit. It does not restore pod DNS. Pods receivenameserver 169.254.10.11(the cluster listener) from kubelet--cluster-dns, baked into each pod's/etc/resolv.confat creation and not repointable from the node for the pod's lifetime, soExecStopPostcannot give already-running pods a working resolver. Restoring pod DNS requires keeping the.11cluster listener answering -- guaranteeing LocalDNS service recovery from the terminal dead state (the cgroup / StartLimit cause) and/or a.11cluster-DNS fallback -- which is a distinct root cause tracked as a separate follow-up PR. This PR therefore does not by itself fully close AKS#5930; it addresses the node-DNS redirect and validates that behavior.Fix
ExecStopPost=/opt/azure/containers/localdns/localdns.sh cleanupso DNS cleanup runs after both normal and unexpected service exits.cleanupmode tolocaldns.shthat restores node DNS configuration and always exits successfully; systemd remains responsible for process cleanup.ExecStopPosthandles node-DNS restoration after unexpected exits.ExecStopPostDNS cleanup) baked into the VHD, which are identical regardless of the bootstrap path, so the scenario runs under the default (aks-node-controller / scriptless) provisioning path.The following are intentionally out of scope and tracked separately: pod-level DNS restoration (cluster-listener
.11recovery / fallback), guaranteed LocalDNS service recovery from the terminal dead state (cgroup delegation / StartLimit), and the unrelated network reconfiguration change.Live reproduction
The original failure was historically reproduced twice on the disposable cluster
sakwa-localdns-repro-0710using the affected Ubuntu 24.04 imageAKSUbuntu-2404gen2containerd-202608.06.1. That cluster is no longer retained. The historical reproduction targeted the single system-pool nodeaks-sysnp-14424852-vmss000000with LocalDNS enabled.Healthy baseline:
Fault injection:
Observed failure:
The first run reached
NRestarts=13; the second reachedNRestarts=6. Each run was recovered withsystemctl reset-failed localdns.servicefollowed bysystemctl restart localdns.service.Validation summary
sakwa-localdns-repro-0710, shipped Ubuntu 24.04 imageSIGKILLand restart-limit exhaustionStandard_D4s_v3node insouthcentralus70-localdns.confand restored node DNSlocaldns.shcleanup suitego test ./... -run '^$' -count=1ine2eThe live A/B test manually replaced the LocalDNS artifacts on a running node. The earlier branch-VHD E2E validated artifacts baked from the branch at that time; it is a separate validation layer and must be rerun after subsequent code/test hardening. The lifecycle validator is attached to the existing LocalDNS hosts-plugin scenarios for Ubuntu 22.04, Ubuntu 24.04, and Azure Linux V3.
Live validation
The core
ExecStopPostbehavior was tested on a fresh LocalDNS-enabled AKS node insouthcentralus(Standard_D4s_v3, Ubuntu 24.04) throughaz vmss run-command. The live A/B used the cleanup implementation available at that time. Subsequent cleanup and E2E hardening changes are covered by ShellSpec, syntax checks, and E2E compilation; a fresh branch-VHD cloud E2E is still required for the current head. The test used an A/B on the same node: shipped artifacts first, then the PR artifacts. Each phase installed a temporary runtimeRestart=nodrop-in, read the serviceMainPID, sentSIGKILL, waited for a terminalActiveState, and checked the node resolver state. The test always restored the original files, removed the temporary drop-in, reloaded systemd, reset the failed state, and restarted LocalDNS before exiting.SIGKILLis intentional: it cannot be caught by Bash, so the normal shell traps do not run. The test therefore exercises the systemdExecStopPostpath directly. The cleanup mode restores node DNS only; it intentionally does not delete the dummy interface or claim to recover the LocalDNS process/cgroup.Phase A — shipped artifacts
This reproduced the node-level DNS blackhole from the reported failure.
Phase B — PR #9360 artifacts
The journal confirmed the cleanup path ran on the live test node:
Normal stop/start was also verified:
Result
ExecStopPostSIGKILL70-localdns.conffailed.10; DNS blackholefailed.10removed;getentsucceedsMeaning for AKS#5930
This live test proves the node-level part of the incident: after an untrappable LocalDNS
SIGKILL, PR #9360 removes the stale node DNS configuration and restores host-level DNS even when the service remainsfailed.It does not prove pod-DNS recovery. Pods use the separate cluster listener
169.254.10.11, which is baked into existing pods'/etc/resolv.conf. Recovery of that listener is handled by the stacked follow-up work in PR #9439. Therefore, PR #9360 should claim node-level DNS restoration, not complete closure of every pod-DNS failure mode in AKS#5930.The node was restored to the shipped artifacts and left healthy after the test. The current branch passes the cleanup-focused ShellSpec coverage and E2E compilation checks. A fresh branch-VHD cloud E2E remains required to validate the latest baked artifacts.