Repository navigation
Combine LocalDNS fixes from PRs 9360, 9439, and 9471 - #9503
Saewon Kwak (saewoni) wants to merge 30 commits into
Conversation
…affic disable_dhcp_use_clusterlistener() runs 'networkctl reload' and returns immediately, but the reload is asynchronous. networkd then re-configures the default route interface, dropping and re-acquiring its DHCP lease, which briefly removes the route to the upstream DNS servers. localdns is already listening and resolv.conf already points at it by then, so any query landing in that gap is forwarded onto an unreachable network. localdns.service is Type=notify and systemd-notify --ready is sent after disable_dhcp_use_clusterlistener returns, so 'systemctl start localdns' unblocks while networkd is still mid re-configure - which is how the very first DNS query on a freshly provisioned node can come back SERVFAIL. The setup path already had this asymmetry with the cleanup path, which does wait (wait_for_localdns_removed_from_resolv_conf). Add the missing wait: poll until the upstream servers are routable again, and only accept that as convergence once we have positively observed the re-configure - either the route disappearing, or resolv.conf dropping the DHCP-provided servers in favor of the localdns listener. Where neither signal is observable it falls back to the upstreams staying routable across consecutive samples. The wait is best effort and bounded at 10s; on timeout localdns logs and continues rather than failing the unit. Note that reordering (pointing DNS at localdns before the ready gate) does not work: it aims node DNS at a listener that is not up yet, and leaves the route outage unguarded either way. Also dump egress state (networkd journal, ip route, ip -br addr) in the e2e cold-start diagnostics, so a future failure captures the routing state instead of only the localdns unit.
PR Title Lint Failed ❌Current Title: Your PR title doesn't follow the expected format. Please update your PR title to follow one of these patterns: Conventional Commits Format:
Guidelines:
Examples:
Please update your PR title and the lint check will run again automatically. |
Combined PR E2E ResultScopeThis draft PR combines AgentBaker PRs #9360, #9439, and #9471. Local Validation
Azure E2EExecuted with Scenario: Result: Failure: After LocalDNS was stopped and restarted with an empty hosts file, the test queried The same scenario passed:
The VM logs showed the LocalDNS service active and healthy, with the DNS drop-in removed and recreated correctly. The failure is isolated to upstream DNS fallthrough immediately after a cold restart with an empty hosts file. Logs: |
Windows Unit Test Results 3 files 14 suites 50s ⏱️ Results for commit 6cec1d5. ♻️ This comment has been updated with latest results. |
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.
ExecStopPost can run after network state has already been torn down, so re-deriving the default route and network file can fail before the known 70-localdns.conf drop-in is removed. Sweep the known runtime drop-in path without depending on ip route or networkctl status, continue network reload, and retain aggregate failure reporting. Keep the dummy localdns interface and .10/.11 addresses during this node-level cleanup: an orphaned CoreDNS child may still be serving pods, and deleting the interface would turn a fast failure into default-route DNS blackholing. Service recovery handles the next-start interface lifecycle. Update ShellSpec coverage for cleanup without network-variable discovery.
Co-authored-by: saewoni <23280628+saewoni@users.noreply.github.com>
- Avoid route/interface discovery during ExecStopPost; sweep the known network drop-in directly so cleanup still restores node DNS after network state has been torn down. - Make SIGTERM cleanup interruptible by waiting on sleep in a child process. - Document that cleanup mode intentionally leaves the dummy interface and listener addresses for service recovery/orphaned-process safety, and make cleanup failure logs actionable. - Skip lifecycle assertions on VHDs without the ExecStopPost hook, add a positive node DNS resolution check, retry resolver reads under set -e, and install the E2E restoration trap before the first service mutation. - Clarify that the normal restart loop tests service recovery while the terminal block tests ExecStopPost.
The cleanup discovery-failure ShellSpec case intentionally exercises successful iptables cleanup while network-variable discovery would fail. Assert its successful rule-removal and network-reload output so ShellSpec reports no unexpected output warnings in CI.
…cally Follow-up to the node-level DNS restoration in the LocalDNS teardown PR (#9360). That fix reverts the node host resolver (169.254.10.10) on an unexpected exit, but pods use the cluster listener 169.254.10.11 (kubelet --cluster-dns, baked into each pod's /etc/resolv.conf and not repointable for the pod's lifetime), so it does not recover pod DNS. This change tunes the systemd restart policy so LocalDNS failures reach a deterministic terminal state instead of either wedging immediately or retrying forever. It puts a floor under failure modes we have not enumerated; it does not itself add the pod-facing recovery path. - RestartSec=2 is the load-bearing change. cleanup_iptables_and_dns runs 'networkctl reload' and the dummy interface carrying .10/.11 is deleted and recreated on every start; at sub-second retries each attempt re-enters the transient it is retrying against. 2s lets networkd settle and an orphaned coredns release its sockets before the next ExecStart. - StartLimitIntervalSec=720 / StartLimitBurst=5 set the threshold to 144s, just above the slowest restart cycle (~125s), so every unrecoverable failure mode lands in 'failed' rather than restarting forever. That terminal state is the prerequisite for an OnFailure= .11 fallback handoff and gives NPD a stable state to observe. Flapping slower than the threshold stays NPD's responsibility by design. - Drop the redundant KillSignal=SIGTERM (already the systemd default); KillMode=mixed is retained.
db26ef8 to
671dd32
Compare
Stacked E2E Rerun with PR #9500PR #9500 is the targeted fix for the original failure. It adds a bounded wait for networkd reload convergence before LocalDNS begins serving traffic, preventing the transient upstream route gap that produced The combined branch was rebased/stacked on top of PR #9500 and force-updated on the draft PR branch. Azure E2EScenario: Configuration:
Result: The scenario completed in approximately 287 seconds. The previously failing LocalDNS cold-start check passed, along with the metrics exporter, forward DNS metrics, hosts-plugin annotation, IPv4/IPv6 hosts resolution, and cleanup validations. This confirms that PR #9500 addresses the observed post-restart upstream DNS |
Live Fault-Injection ValidationA fresh retained Ubuntu 22.04 agent-pool node was provisioned in the disposable PR #9360 behaviorThe combined PR artifacts were installed on the live node, including: With a temporary Observed:
This is a live validation of PR #9360's node-level DNS cleanup behavior and confirms that the existing pod path remains functional during this controlled terminal-service test. PR #9439 behaviorThe node was restored and six rapid supervisor The retained VMSS was deleted through ARM cleanup. The restart-budget result is therefore inconclusive, not a pass or failure: the Azure command transport could not collect the terminal systemd state after the rapid-kill sequence. PR #9471 behaviorThe normal live node provisioning path passed with valid upstream DNS forwarding. Invalid/listener upstream rejection remains covered by the combined ShellSpec suite ( Overall
|
AgentBaker Linux gate detective: failed check-in runRun: 181196185 TL;DR: The E2E job failed with 18 failed scenarios: three LocalDNSHostsPlugin variants exited in the new LocalDNS lifecycle validation, while the remaining failures are the known kubelet 10250 pod-exec proxy 502 signature. Failure signature: First failing step: First published failed test result is Likely root cause: The LocalDNS lifecycle failures are likely PR-change-caused: this PR changes Classification: E2E/test - LocalDNS lifecycle validation likely PR-change-caused, plus recurring kubelet 10250 proxy infra/test flake. Assessment: PR-change-caused is strongest for the LocalDNS lifecycle failures because the changed files directly cover LocalDNS service cleanup/restart behavior. Infra/environment is strongest for the repeated kubelet proxy failures because the same 502 signature spans many unrelated scenarios and already has a repair item. Product/CSE/Packer/package/image theories are less likely because VM provisioning completed and the failures occur during E2E validation/pod exec. Confidence: Medium-high for LocalDNS PR-change-caused; medium for kubelet proxy flake. Strongest alternative: A shared test-cluster kubelet/proxy degradation may have made both LocalDNS and non-LocalDNS validations fail; it is less likely for the three LocalDNS lifecycle failures because their error path is a LocalDNS-specific validation command aligned with this PR's diff. Recommended owner/action: PR owner should first debug the LocalDNS lifecycle validation on AzureLinuxV3, Ubuntu2204, and Ubuntu2404 with the current combined LocalDNS service/script changes; E2E infrastructure owners should continue tracking the kubelet 10250 proxy 502 recurrence under repair item Evidence: Build summary, timeline failed |
|
|
|
Refreshed-Branch Live ValidationThe combined branch was refreshed from the latest heads of PRs #9360, #9439, and #9471. New commits cherry-picked from the refreshed PR #9360 head:
PRs #9439 and #9471 had no commits beyond the versions already present in the combined branch. The branch was pushed to Baseline
The live node was then patched with the branch artifacts:
After
PR #9360: terminal unexpected exitProblem: an uncatchable LocalDNS supervisor exit can leave the node DNS drop-in pointing at the dead Fault injected: temporary Observed after patching:
Result: Passed live for the node-level DNS restoration scope of PR #9360. PR #9439: bounded restart budgetProblem: LocalDNS failure modes need to reach a deterministic The patched service contained: StartLimitIntervalSec=720
StartLimitBurst=5
RestartSec=2The fresh node successfully restarted LocalDNS after the normal service restart and baseline scenario. A destructive rapid-kill budget exhaustion attempt was started, but Azure RunCommand did not return while the guest was processing the repeated kills. The terminal state could not be collected through that transport. Result: Inconclusive live fault-injection result. Configuration and normal restart behavior were observed, but the full restart-budget exhaustion state was not asserted from the guest. PR #9471: invalid upstream/listener rejectionProblem: resolver snapshots containing Valid upstream forwarding passed on the fresh live node. A live invalid-listener resolver injection was started, but intentionally breaking the node resolver caused the Azure RunCommand extension to time out before guest output could be collected. The disposable VMSS was deleted through ARM cleanup. The invalid/listener paths remain covered by the combined ShellSpec suite: Result: Valid live path passed; invalid-input live fault injection inconclusive. Unit/ShellSpec validation passed. Summary
|
PR #9439 Restart-Budget Retest (RunCommand Circumvention)The earlier inconclusive PR #9439 result was caused by the synchronous Azure VM RunCommand transport being coupled to the in-guest rapid-kill loop, which wedged the RunCommand extension handler. Circumvention used
This decoupling let the terminal state be observed deterministically. Environment
Fault injected8 rapid Observed resultInterpretation
Result
The disposable VMSS was deleted after the test. |
Combined Branch: LocalDNS Live Fault-Injection Test ReportThis document describes, in detail, every behavior that was tested against the combined branch of PRs #9360, #9439, and #9471, the specific result each test was designed to elicit, and the observed outcome on live Azure agent-pool nodes. Test Environment
The distinction that matters throughout: the retained VHD is the shipped PR #9360 — Restore node-level DNS after an unexpected LocalDNS exitProblem being tested
Behaviors tested and the result each test wantedB1. B2. Single unexpected kill with auto-restart. B3. Terminal dead-service cleanup (the incident scenario).
B4. Pod DNS is not collateral damage. B5. Pre-patch control. Observed results
Conclusion for #9360: the node-level DNS restoration problem is fixed live. The control run reproduced the blackhole; the patched run cleaned it up deterministically while leaving pods unaffected. PR #9439 — Bound the LocalDNS restart budget so failures terminate deterministicallyProblem being testedsystemd's default restart budget is wrong in two directions for LocalDNS:
The fix sets Behaviors tested and the result each test wantedB6. Config is applied on the live node. B7. Moderate kill burst self-heals (the load-bearing B8. Deterministic terminal B9. Reliable terminal-state observation despite the transport problem. Observed results
Why B8 was not force-triggered: with The transport problem and its circumvention (B9): the initial attempt ran the whole kill/wait loop inside a single synchronous
Conclusion for #9439: the config is correctly applied and the load-bearing self-heal behavior is confirmed live. The deterministic terminal- PR #9471 — Reject LocalDNS listener upstreamsProblem being testedDuring startup, That could insert the LocalDNS listener into the CoreDNS Behaviors tested and the result each test wantedB10. Valid upstream path on a real node. B11. Listener rejection at startup (node listener B12. Cluster listener rejection ( Observed results
Why B11 was inconclusive live: to test rejection you must first put the node into the exact broken resolver state the PR guards against. Doing that on a live node disrupts the guest agent's own name resolution, which is what wedged the RunCommand extension mid-test. The valid path (B10) passed live; the invalid paths (B11/B12) are proven by ShellSpec, which exercises the identical Conclusion for #9471: the valid forwarding path is confirmed live and the full rejection matrix passes in unit tests. The live invalid-input case is best validated with the async Cross-cutting behaviorsB13. Node stays healthy after every test. B14. Baseline scenario passes on the combined branch before any injection. B15. Deterministic, transport-independent result collection. Summary matrix
Overall
|
AgentBaker Linux gate detective: failed check-in runRun: 181214800 TL;DR: The rerun reduced the E2E blast radius from the prior 18 failures to 4 failures, but the three LocalDNSHostsPlugin lifecycle failures reproduced and one accelerated-networking VF validation flake appeared. Failure signature: First failing step: First published failed test result is Likely root cause: The LocalDNS failures are likely PR-change-caused because PR 9503 directly changes LocalDNS service/script and lifecycle validation code, and the same LocalDNS lifecycle signature reproduced from build 181196185. The accelerated-networking failure matches the existing VF-not-bonded infra/test VM signature and is likely unrelated to the LocalDNS diff. Classification: E2E/test - LocalDNS lifecycle validation likely PR-change-caused, plus accelerated networking VF validation infra/test flake. Assessment: PR-change-caused is strongest for the LocalDNS lifecycle failures because they line up with the changed LocalDNS service, cleanup, and validator code. Infra/environment is strongest for the accelerated-networking VF failure because it is a VM/NIC validation symptom already seen in this gate. Product/CSE/Packer/package/image theories are less likely because provisioning completed and failures occur during E2E node validation. Confidence: Medium-high for LocalDNS PR-change-caused; medium for accelerated-networking infra/test flake. Strongest alternative: A shared E2E environment issue could explain the residual non-LocalDNS VF failure and might affect timing-sensitive LocalDNS lifecycle checks, but the repeated three-variant LocalDNS failure across consecutive PR 9503 runs makes PR validation logic/service cleanup the stronger lead. Recommended owner/action: PR owner should debug the LocalDNS lifecycle command output for AzureLinuxV3, Ubuntu2204, and Ubuntu2404 after the latest cgroup/restart-count fixes; treat the AzureLinux3OSGuard VF failure as a likely unrelated gate flake unless it reproduces on a focused rerun. Evidence: Build summary, failed timeline records for |
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. |
|
this branch is not what its title says, and that matters because the E2E result from it is being quoted on the stacked PRs. 1. it's 67 commits behind 2. the #9439 half is missing its load-bearing piece. this branch touches no CSE file at all — so whatever the E2E measured about restart behaviour, it measured a configuration we are not shipping. please rebase and re-run before quoting the result anywhere, or drop the citation. 3. #9360 is merged. a combine-branch for it is pointless now — rebase on 4. the E2E failure you reported is the interesting part of this PR. cold-start upstream fallthrough returning is "empty hosts file" the same class as the missing-corefile case? if localdns comes back up with no hosts data and answers 5. what's the merge story for this branch? if it's a throwaway review vehicle, say so in the description and close it once #9439/#9471 land. right now it reads like a fourth PR in the stack. |
Summary
Combined changes from AgentBaker PRs #9360, #9439, and #9471 into one review branch.
The branch includes:
Validation
git diff --checkpassed.E2E
The Azure E2E result is posted as a PR discussion comment. The targeted
LocalDNSHostsPlugin/Ubuntu2204scenario reached the VM and passed the metrics, hosts-plugin, IPv4, IPv6, and cleanup validations, but failed the cold-start upstream fallthrough check whenmcr.microsoft.comreturnedSERVFAILafter LocalDNS restarted with an empty hosts file.