fix: restore node-level DNS after unexpected LocalDNS exit - #9360
fix: restore node-level DNS after unexpected LocalDNS exit#9360Saewon Kwak (saewoni) wants to merge 3 commits into
Conversation
Windows Unit Test Results 3 files 14 suites 50s ⏱️ Results for commit ea38772. ♻️ 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.
There was a problem hiding this comment.
🟡 Changes recommended
The E2E validation can falsely detect recovery and does not substantiate the claimed rapid-failure or dual-bootstrap coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
e2e/scenario_localdns_hosts_test.go:106
- 🟡 Medium Risk — 🧪 Test Coverage: The PR description says this validation rejects cgroup teardown errors, but this branch only prints a warning and still passes. Either fail the scenario here so that guarantee is enforced, or update the stated validation scope if this warning is intentionally allowed until the separate cgroup fix.
if sudo journalctl -u localdns.service --since "@$test_start" --no-pager | grep -q 'Failed to kill control group'; then
echo "WARNING: LocalDNS cgroup teardown warning observed"
fi
e2e/scenario_localdns_hosts_test.go:90
- 🟡 Medium Risk — 🔧 Script Logic: Immediately after
kill, systemd may still expose the pre-killactive/runningstate, so this can declare recovery without observing a restart and then target the stale PID on the next iteration. Capture the old PID and only acceptactive/runningafterMainPIDchanges to a new nonzero value.
state=$(sudo systemctl show localdns.service -p ActiveState -p SubState --value)
if [ "$state" = $'active\nrunning' ]; then
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
E2E validation has asynchronous cleanup races and does not currently cover both claimed bootstrap paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
e2e/scenario_localdns_hosts_test.go:57
- 🟡 Medium Risk — 🧪 Test Coverage: This validator does not provide the claimed coverage of both bootstrap paths.
RunScenarioenablesRuntime.EnableScriptlessNBCCSECmdby default for these three scriptless-capable images (e2e/test_helpers.go:98-102), and the repository has no E2E pipeline settingDISABLE_SCRIPTLESS, so these cases exercise aks-node-controller only. Add a dedicated legacy-mode run that actually disables scriptless provisioning, or revise the stated validation scope.
Validator: func(ctx context.Context, s *Scenario) error {
if tt.name == "Ubuntu2204" || tt.name == "Ubuntu2404" || tt.name == "AzureLinuxV3" {
return validateLocalDNSLifecycle(ctx, s)
e2e/scenario_localdns_hosts_test.go:165
- 🟡 Medium Risk — 🔧 Script Logic: This is described as best-effort but exits immediately if the old DNS value is still visible.
localdns.sh:1047-1052explicitly documents thatnetworkctl reloadupdates resolved asynchronously and that shutdown does not wait, so a correct cleanup can transiently retain169.254.10.10here and make the E2E flaky. Poll boundedly for propagation before failing.
if command -v resolvectl >/dev/null 2>&1; then
if resolvectl status 2>/dev/null | grep -q '169\.254\.10\.10'; then
echo "FAIL: link DNS still points at 169.254.10.10 after localdns died"
exit 1
fi
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The terminal recovery restores host DNS but leaves ClusterFirst pods dependent on the dead 169.254.10.11 listener reported in the linked incident.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The live DNS assertion can falsely pass when its resolver probe fails.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
e2e/scenario_localdns_hosts_test.go:172
- 🟡 Medium Risk — 🧪 Test Coverage: Both resolver probes convert command failure into an empty string, and the following negative
grepthen treats that empty result as proof that DNS was reverted. Ifresolvectl/D-Bus is temporarily unavailable during the reload (or the fallback file cannot be read), this phase passes without validating the live resolver state. Retry when the probe itself fails and only test output from a successful probe.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
c2e15d3 to
96adef0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The terminal-state E2E can falsely pass when resolver-state retrieval fails.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
E2E status: branch-VHD E2E passed; the red
|
| Check | VHD under test | Result |
|---|---|---|
| AKS Linux VHD Build - PR check-in gate (e2e Run AgentBaker E2E) | VHD built from this branch (has ExecStopPost) |
✅ pass |
| Agentbaker E2E | currently published VHD (no ExecStopPost) |
❌ fail (expected) |
The branch-VHD E2E passed — the LocalDNSHostsPlugin/{Ubuntu2204,Ubuntu2404,AzureLinuxV3} lifecycle validation, including the terminal dead-service SIGKILL case, ran on nodes provisioned from branch-built VHDs and succeeded.
The red Agentbaker E2E ran against the published VHD, which does not yet have ExecStopPost baked in, so the new lifecycle assertion correctly fails there with FAIL: 70-localdns.conf still present after localdns died — i.e. it reproduces the very blackhole this PR fixes on the unpatched image. Its 4th failure, Ubuntu2204_ArtifactStreaming_TrustedLaunch (a waagent iptables ... CHAIN_ZERO failed ExtHandler error), is unrelated to this change.
This matches the live-node A/B validation in the PR description: without ExecStopPost the node stays pointed at the dead 169.254.10.10 listener; with it, ExecStopPost removes 70-localdns.conf and reverts DNS on the untrappable SIGKILL path.
There was a problem hiding this comment.
🟡 Changes recommended
The E2E validation can exit without restoring its service override and does not correctly retry transient resolver-read failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
e2e/scenario_localdns_hosts.go:170
- 🟡 Medium Risk — 🔧 Script Logic: Because this script starts with
set -e, a transient failure from either resolver read terminates the entire validation at the assignment instead of retrying as the comment promises. This can make the eventual-consistency check flaky and also bypass the restoration below. Convert read failures to an empty snapshot; the existing non-empty guard will then retry safely.
if command -v resolvectl >/dev/null 2>&1; then
current_dns=$(resolvectl status 2>/dev/null)
else
current_dns=$(cat /run/systemd/resolve/resolv.conf 2>/dev/null)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
Failed gate run
Detective summaryTwo tests failed: Likely causeSignatures: Recommended owner/actionNode Lifecycle/E2E owner: continue CSE threshold work under #39622938 and inspect whether bootstrap-token validation is a transient STLS/bootstrap timing issue before treating it as PR-caused. Strongest alternativePR-change-caused LocalDNS regression is the strongest alternative because PR 9360 modifies LocalDNS service/script behavior; it is less likely because neither primary failure is a LocalDNS functional assertion and the CSE timing threshold is a known recurring signature. Evidence
Wiki signature
|
Rebased onto latest main; the e2e scenario file was renamed from scenario_localdns_hosts_test.go to scenario_localdns_hosts.go by the standalone-CLI e2e refactor (#9321), so the lifecycle validator is re-attached to the new Register-based scenario. When the localdns supervisor exits unexpectedly (SIGKILL), the shell cleanup traps do not run, so the node can retain the network drop-in that points DNS at the dead localdns listener (169.254.10.10), causing a node-level DNS outage. - localdns.service: add ExecStopPost=/opt/azure/containers/localdns/localdns.sh cleanup so DNS is reverted after both graceful and unexpected exits. - localdns.sh: add cleanup mode (localdns_cleanup_mode) that restores node DNS and always exits 0 so a cleanup error cannot wedge systemd recovery; make cleanup_iptables_and_dns aggregate failures instead of returning early so DNS drop-in removal and network reload always run even when iptables rule deletion fails. - localdns_spec.sh: ShellSpec coverage for cleanup_iptables_and_dns and cleanup mode (success, successful rule removal, iptables-failure still restores DNS, reload failure reported, cleanup mode exits 0 on success and failure). - e2e: lifecycle validator covering normal stop/start, kill+recovery with a genuinely-new-MainPID check, and the terminal dead-service case (disable auto-restart via a transient Restart=no drop-in, kill, then assert the 70-localdns.conf drop-in was removed and DNS no longer points at 169.254.10.10, polling for a terminal ActiveState and DNS revert).
The DNS-revert settle loop suppressed resolver-read errors (|| true), so an errored or empty resolvectl/resolv.conf read produced an empty current_dns, which the absence check then treated as 'listener gone' -> success. A failed read would therefore mask the terminal-outage regression the check exists to catch. Drop the error suppression and only accept a successful, non-empty snapshot that omits 169.254.10.10; empty/failed reads keep polling and fail the test if the resolver state never becomes readable.
Register an EXIT cleanup handler before installing the temporary Restart=no systemd drop-in used by the terminal dead-service validation. The handler removes the override, reloads systemd, resets the failed unit, restarts LocalDNS, and verifies the service is active. Preserve the original test status and report cleanup failures instead of masking them, so a failed assertion cannot contaminate the node or subsequent validation.
There was a problem hiding this comment.
🟡 Changes recommended
The resolver polling loop exits on transient read errors instead of retrying them.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| if command -v resolvectl >/dev/null 2>&1; then | ||
| current_dns=$(resolvectl status 2>/dev/null) | ||
| else | ||
| current_dns=$(cat /run/systemd/resolve/resolv.conf 2>/dev/null) | ||
| fi |
|
Ye Wang [msft] (@yewmsft) could you please re-review PR #9360 after the latest updates? The PR has been rebased onto the latest Since your earlier review, we have:
The remaining scope clarification is intentional: #9360 restores the node resolver ( |
There was a problem hiding this comment.
🔵 Needs a closer look
The E2E resolver poll exits on transient read errors instead of retrying as intended.
Review details
Suppressed comments (1)
e2e/scenario_localdns_hosts.go:204
- 🟡 Medium Risk — 🔧 Script Logic: With
set -e, a transient nonzero exit fromresolvectl statusor the fallbackcatterminates the validation at this assignment, so the surrounding 12-attempt poll never retries errored reads as intended. Handle the read failure inside the loop and leavecurrent_dnsempty so the next iteration can retry.
if command -v resolvectl >/dev/null 2>&1; then
current_dns=$(resolvectl status 2>/dev/null)
else
current_dns=$(cat /run/systemd/resolve/resolv.conf 2>/dev/null)
fi
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.SIGTERMwith graceful cleanup and exit status 0; genuine error paths retain the existing failure status.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 failure was reproduced twice on the disposable cluster
sakwa-localdns-repro-0710using the affected Ubuntu 24.04 imageAKSUbuntu-2404gen2containerd-202608.06.1. 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.Live validation
The fix was validated on a real LocalDNS-enabled AKS node rather than only in unit tests, because the failure it addresses is a runtime systemd-ordering behavior that static checks cannot exercise. A fresh cluster was provisioned in
southcentraluswith a LocalDNS nodepool (Standard_D4s_v3, the >= 4 vCPU minimum LocalDNS requires), giving a node in the exact vulnerable configuration:localdns.serviceactive, the network drop-in70-localdns.confpresent, and the link resolver pointing at the node listener169.254.10.10. The node's shippedlocaldns.servicehas noExecStopPost, so it is the unfixed baseline.The test is an A/B on the same node, driven through
az vmss run-command. Each phase disables auto-restart with a transientRestart=nodrop-in,SIGKILLs the supervisor'sMainPID, waits for the unit to reach a terminalActiveState, and then inspects whether node DNS was reverted.SIGKILLis the important detail: it is not trappable, so the script's own cleanup traps never run — the only thing that can revert DNS on this path isExecStopPost, which systemd invokes after a process exits unexpectedly.Phase A — shipped unit, no
ExecStopPost. After the supervisor is killed the unit goesfailed, and the node is left blackholed: the drop-in is still present and the link still resolves through169.254.10.10, which is no longer answered. This reproduces the reported incident on demand.Phase B — fixed
localdns.service+localdns.shswapped in on the same node. After the identical kill, the drop-in is gone and the listener is no longer in the link's DNS. The journal showsExecStopPostdoing the work on the unexpected-exit path — removing the localdns iptables rules, removing the network drop-in, and reloading networkd — and the unit restarts cleanly afterward.Result
ExecStopPostSIGKILL70-localdns.conf169.254.10.10failedfailedPhase B journal, unexpected-exit path:
The distinction the PR turns on is that the shell traps and
ExecStopare irrelevant onSIGKILL— systemd skips them and goes straight toExecStopPost— soExecStopPostis the only hook that can undo the node DNS redirection when the supervisor dies uncooperatively. Phase A shows the node stays pointed at the dead listener without it; Phase B shows it is reverted with it. The node was restored to the shipped artifacts and left healthy after the run.