Skip to content

fix: restore node-level DNS after unexpected LocalDNS exit #9360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Saewon Kwak (saewoni) wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: restore node-level DNS after unexpected LocalDNS exit #9360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8abba5b
fix: restore node DNS after unexpected LocalDNS exit
saewoni Sep 9, 2026
43379ba
test: require a non-empty resolver snapshot before accepting DNS revert
saewoni Sep 9, 2026
8273e13
test: restore LocalDNS after lifecycle validation failures
saewoni Sep 10, 2026
a0ee237
fix: clean LocalDNS drop-in without route discovery during exit
saewoni Sep 10, 2026
b6cba8f
test: initialize LocalDNS cleanup fixture interface
Copilot Sep 10, 2026
a3c0874
fix: harden LocalDNS cleanup and lifecycle validation
saewoni Sep 10, 2026
6eb258a
test: assert cleanup output in discovery failure coverage
saewoni Sep 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 187 additions & 0 deletions e2e/scenario/scenario_localdns_hosts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package scenario

import (
"context"

aksnodeconfigv1 "github.com/Azure/agentbaker/aks-node-controller/pkg/gen/aksnodeconfig/v1"
"github.com/Azure/agentbaker/e2e/config"
"github.com/Azure/agentbaker/pkg/agent/datamodel"
Expand All @@ -23,6 +25,7 @@ func init() {
}

for _, tt := range tests {
tt := tt
cluster := ClusterKubenet
if tt.name == "Ubuntu2604Minimal" {
cluster = ClusterLatestKubernetesVersionKubenet
Expand All @@ -42,7 +45,191 @@ func init() {
config.LocalDnsProfile.EnableLocalDns = true
},
VMConfigMutator: tt.vmConfigMutator,
Validator: func(ctx context.Context, s *Scenario) error {
// Validate the full LocalDNS service lifecycle (including the
// unexpected-exit DNS teardown this PR fixes) on the target
// distros. The hosts-plugin functionality itself is covered by
// the scenario's default provisioning validation.
if tt.name == "Ubuntu2204" || tt.name == "Ubuntu2404" || tt.name == "AzureLinuxV3" {
return validateLocalDNSLifecycle(ctx, s)
}
return nil
},
},
})
}
}

func validateLocalDNSLifecycle(ctx context.Context, s *Scenario) error {
_, err := execScriptOnVMForScenarioValidateExitCode(ctx, s, `
set -eu

# This validation requires the ExecStopPost hook baked into the branch VHD.
# Standalone E2E may run against an older published VHD, where this behavior
# is unavailable and should be skipped rather than reported as a false failure.
if ! sudo systemctl show localdns.service -p ExecStopPost --value | grep -q 'localdns.sh cleanup'; then
echo "SKIP: VHD predates the ExecStopPost cleanup hook"
exit 0
fi

NORESTART=/run/systemd/system/localdns.service.d/99-e2e-no-restart.conf

# Install cleanup before any service mutation so set -e cannot leave the node
# with the temporary Restart=no override or a failed LocalDNS unit.
restore_localdns_test_state() {
test_status=$?
trap - EXIT
set +e
cleanup_status=0
if [ -f "$NORESTART" ]; then
sudo rm -f "$NORESTART" || { echo "ERROR: failed to remove $NORESTART"; cleanup_status=1; }
sudo systemctl daemon-reload || { echo "ERROR: systemd daemon-reload failed during test cleanup"; cleanup_status=1; }
sudo systemctl reset-failed localdns.service || { echo "ERROR: reset-failed localdns.service failed during test cleanup"; cleanup_status=1; }
fi
if ! sudo systemctl is-active --quiet localdns.service; then
sudo systemctl start localdns.service || { echo "ERROR: failed to restart localdns.service during test cleanup"; cleanup_status=1; }
fi
if ! sudo systemctl is-active --quiet localdns.service; then
echo "ERROR: localdns.service is not active after test cleanup"
cleanup_status=1
fi
if [ "$test_status" -eq 0 ] && [ "$cleanup_status" -ne 0 ]; then
test_status=$cleanup_status
fi
exit "$test_status"
}
trap restore_localdns_test_state EXIT

sudo systemctl is-active --quiet localdns.service

# Normal systemd stop must complete cleanup and return success.
sudo systemctl restart localdns.service
sudo systemctl is-active --quiet localdns.service
sudo systemctl stop localdns.service
Comment thread
saewoni marked this conversation as resolved.
test "$(sudo systemctl show localdns.service -p ActiveState --value)" = inactive
sudo systemctl start localdns.service
sudo systemctl is-active --quiet localdns.service

# Repeatedly kill the supervisor and wait for Restart=on-failure recovery.
Comment thread
saewoni marked this conversation as resolved.
# This loop validates ordinary service recovery; the terminal dead-service
# regression for ExecStopPost is covered by the block below.
# Require a genuinely new MainPID after each kill: immediately after kill -9,
# systemd may still report the killed invocation as active/running until it
# processes SIGCHLD, so checking active/running alone can observe the old
# process and falsely declare recovery. Save the killed PID and require the
# new MainPID to be nonzero and different from it.
test_start=$(date +%s)

for i in 1 2 3; do
Comment thread
saewoni marked this conversation as resolved.
killed=$(sudo systemctl show -p MainPID --value localdns.service)
test "$killed" -gt 0
sudo kill -9 "$killed"

recovered=false
for attempt in 1 2 3 4 5 6 7 8 9 10 11 12; do
state=$(sudo systemctl show localdns.service -p ActiveState -p SubState --value)
main=$(sudo systemctl show -p MainPID --value localdns.service)
if [ "$state" = $'active\nrunning' ] && [ "$main" -gt 0 ] && [ "$main" != "$killed" ]; then
recovered=true
break
fi
sleep 1
done
test "$recovered" = true
done

state=$(sudo systemctl show localdns.service -p ActiveState -p SubState -p Result -p ControlGroup)
printf '%s\n' "$state"
printf '%s\n' "$state" | grep -q '^ActiveState=active$'
printf '%s\n' "$state" | grep -q '^SubState=running$'
printf '%s\n' "$state" | grep -q '^Result=success$'
# The cgroup teardown warning is diagnostic only: fixing it is out of scope for
# this PR (which is about restoring node DNS after an unexpected exit), so we
# surface it but do not fail on it.
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
if sudo journalctl -u localdns.service --since "@$test_start" --no-pager | grep -q 'Start request repeated too quickly'; then
echo "LocalDNS reached systemd StartLimit"
exit 1
fi
dig +short +time=5 +tries=1 mcr.microsoft.com @169.254.10.10 | grep -q .

# Terminal dead-service case: this is the incident scenario the PR fixes.

@yewmsft Ye Wang [msft] (yewmsft) Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

blocker. Agentbaker E2E is a required check and it runs main's published VHD, not this branch's build — e2e/config/config.go defaults SIG_VERSION_TAG_NAME=branch / refs/heads/main, and .pipelines/scripts/e2e_run.sh:83 only overrides the selector when VHD_BUILD_ID is set. so this block asserts on an ExecStopPost that isn't in the image under test.

that's exactly what's happening right now: build 180533805 (the required check on this PR) failed all three lifecycle scenarios, while the branch-VHD run passed. merge this and the required check goes red on every PR in the repo until a main VHD carrying ExecStopPost publishes.

gate the block on the unit actually having the hook:

Suggested change
# Terminal dead-service case: this is the incident scenario the PR fixes.
# This block asserts on ExecStopPost, which is baked into the VHD. The standalone
# Agentbaker E2E runs main's published VHD, so skip until the new image ships.
if ! systemctl show localdns.service -p ExecStopPost --value | grep -q 'localdns.sh cleanup'; then
echo "SKIP: VHD predates the ExecStopPost cleanup hook"
exit 0
fi
# Terminal dead-service case: this is the incident scenario the PR fixes.

@saewoni Saewon Kwak (saewoni) Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5fc8605. The lifecycle validator now checks for the baked ExecStopPost hook before asserting the new behavior and skips with a diagnostic on published VHDs that predate the hook. The branch-VHD E2E remains the authoritative validation for the new lifecycle behavior.

@yewmsft Ye Wang [msft] (yewmsft) Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the gate works, but it sits at the top of the validator (:70-73), so on an older vhd it skips everything, not just the terminal block. the restart-loop coverage at :113-156 passes fine on main's vhd today — that's what standalone e2e was actually exercising, and now it's a no-op there. green-by-skip.

move the check down to guard only the terminal block at :158:

if sudo systemctl show localdns.service -p ExecStopPost --value | grep -q 'localdns.sh cleanup'; then
    # terminal dead-service case ... (current :158-229)
else
    echo "SKIP: VHD predates the ExecStopPost cleanup hook"
fi

the EXIT trap at :101 is already installed above that point, so restore still works on either branch. branch-vhd e2e keeps covering the new behavior; standalone keeps the old coverage instead of skipping the whole scenario.

# When localdns ends up dead (systemd exhausts restart attempts), ExecStopPost
# must still revert node DNS so the node does not keep pointing at the dead
# localdns listener (169.254.10.10). We reach the dead state deterministically
# by disabling auto-restart with a transient drop-in, then killing the
# supervisor -- tripping StartLimit via rapid kills is timing dependent and
# flaky. ExecStopPost runs on the SIGKILL path regardless of Restart=.
sudo mkdir -p "$(dirname "$NORESTART")"
printf '[Service]\nRestart=no\n' | sudo tee "$NORESTART" >/dev/null
sudo systemctl daemon-reload

dead_main=$(sudo systemctl show -p MainPID --value localdns.service)
test "$dead_main" -gt 0
sudo kill -9 "$dead_main"

# Wait for the service to reach a terminal ActiveState (failed or inactive).
# A non-running SubState is not sufficient: SubState passes through transitional
# values such as stop-post while ExecStopPost is still running the cleanup under
# test, so asserting on drop-in removal then could race the cleanup. ActiveState
# only becomes failed/inactive after ExecStopPost has completed.
dead=false
for attempt in 1 2 3 4 5 6 7 8 9 10 11 12; do
active_state=$(sudo systemctl show localdns.service -p ActiveState --value)
if [ "$active_state" = failed ] || [ "$active_state" = inactive ]; then
dead=true
break
fi
sleep 1
done
test "$dead" = true

# The localdns network drop-in must have been removed by ExecStopPost. This is
# the authoritative signal that DNS was reverted: the drop-in is what points the
# link's DNS at the localdns listener.
if ls /run/systemd/network/*.d/70-localdns.conf >/dev/null 2>&1; then
echo "FAIL: 70-localdns.conf still present after localdns died"
exit 1
fi

# The live link DNS must no longer include the localdns node listener. This is
# eventually consistent: networkctl reload propagates to systemd-resolved
# asynchronously, so poll (like wait_for_localdns_removed_from_resolv_conf does)
# until the listener IP is gone rather than checking once. Prefer resolvectl
# (the per-link view the drop-in configures); fall back to the resolved stub.
# Only accept a successful, non-empty resolver snapshot: an errored or empty
# read must not be treated as "restored", or a failed read would mask the very
# regression under test. Retry those instead.
dns_reverted=false
for attempt in 1 2 3 4 5 6 7 8 9 10 11 12; do
if command -v resolvectl >/dev/null 2>&1; then
current_dns=$(resolvectl status 2>/dev/null) || current_dns=""
else
current_dns=$(cat /run/systemd/resolve/resolv.conf 2>/dev/null) || current_dns=""
fi
# Require a non-empty snapshot before trusting the absence check.
if [ -n "$current_dns" ] && ! printf '%s' "$current_dns" | grep -q '169\.254\.10\.10'; then
dns_reverted=true
break
fi
sleep 1
done
if [ "$dns_reverted" != true ]; then
echo "FAIL: link DNS still points at 169.254.10.10 (or resolver state unreadable) after localdns died"
exit 1
fi
Comment thread
saewoni marked this conversation as resolved.

# Removing the LocalDNS address is not sufficient: verify the node has a
# working resolver after cleanup.
if ! getent hosts mcr.microsoft.com >/dev/null 2>&1; then
echo "FAIL: node cannot resolve DNS after localdns died"
exit 1
fi

# The EXIT trap removes the temporary override and restores LocalDNS even if
# an assertion above exits the validation early.
`, 0, "LocalDNS lifecycle validation failed")
return err
}
2 changes: 2 additions & 0 deletions parts/linux/cloud-init/artifacts/localdns.service
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ NotifyAccess=all
WatchdogSec=60
Restart=on-failure
KillMode=mixed
# Revert node DNS configuration even when the supervisor exits unexpectedly.
ExecStopPost=/opt/azure/containers/localdns/localdns.sh cleanup
TimeoutStopSec=30
Slice=localdns.slice
EnvironmentFile=-/etc/localdns/environment
Expand Down
90 changes: 69 additions & 21 deletions parts/linux/cloud-init/artifacts/localdns.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,15 +656,28 @@ EOF

# Remove iptables rules and revert DNS configuration.
cleanup_iptables_and_dns() {
# Ensure network variables are initialized if not already set.
# This is needed here because this function can be called from cleanup traps or systemd restarts initiated by watchdog.
if [ -z "${DEFAULT_ROUTE_INTERFACE:-}" ] || [ -z "${NETWORK_DROPIN_FILE:-}" ] || [ -z "${NETWORK_DROPIN_DIR:-}" ]; then
echo "Network variables not initialized, attempting to determine them..."
if ! initialize_network_variables; then
echo "Failed to initialize network variables during cleanup."
return 1
fi
# Track failures across all cleanup steps so that a failure in one step
# (e.g. removing an iptables rule) does not skip the more important DNS
# restoration steps below. Restoring node DNS is the priority: if we return
# early on an iptables error we could leave the node pointed at a dead
# localdns listener via the network drop-in.
local cleanup_failed=false
Comment thread
saewoni marked this conversation as resolved.

# Do not derive the route/interface during post-exit cleanup. At this point
# network state may already be torn down, so ip route/networkctl discovery
# can fail and leave the node pointed at the dead LocalDNS listener. Sweep
# the known drop-in name directly; the glob also handles a cleanup call
# where NETWORK_DROPIN_FILE was never initialized in this process.
local network_dropin_file
local -a network_dropin_files=()
if [ -n "${NETWORK_DROPIN_FILE:-}" ]; then
network_dropin_files+=("${NETWORK_DROPIN_FILE}")
fi
for network_dropin_file in /run/systemd/network/*.d/70-localdns.conf; do
if [ -e "$network_dropin_file" ] && [ "$network_dropin_file" != "${NETWORK_DROPIN_FILE:-}" ]; then
network_dropin_files+=("$network_dropin_file")
fi
done

# Remove any existing localdns iptables rules by searching for our comment.
echo "Cleaning up any existing localdns iptables rules..."
Expand All @@ -688,32 +701,57 @@ cleanup_iptables_and_dns() {
done
done
if [ "$failure_occurred" = true ]; then
return 1
# Record the failure but continue so DNS restoration still runs.
cleanup_failed=true
fi
else
echo "No existing localdns iptables rules found."
fi

# Revert DNS configuration and network reload.
echo "Removing network drop-in file ${NETWORK_DROPIN_FILE}."
rm -f "$NETWORK_DROPIN_FILE"
if [ "$?" -ne 0 ]; then
echo "Failed to remove network drop-in file ${NETWORK_DROPIN_FILE}."
return 1
fi
echo "Successfully removed network drop-in file."
# Revert DNS configuration and network reload. Keep the dummy interface
# and its .10/.11 addresses here: if an orphaned CoreDNS child survived a
# failed cgroup teardown, removing the interface would break a listener
# that may still be serving pods. The service-recovery path handles the
# next-start interface lifecycle separately.
for network_dropin_file in "${network_dropin_files[@]}"; do
echo "Removing network drop-in file ${network_dropin_file}."
if ! rm -f "$network_dropin_file"; then
echo "Failed to remove network drop-in file ${network_dropin_file}."
cleanup_failed=true
else
echo "Successfully removed network drop-in file."
fi
done

echo "Attempt to reload network configuration."
eval "$NETWORKCTL_RELOAD_CMD"
if [ "$?" -ne 0 ]; then
if ! eval "$NETWORKCTL_RELOAD_CMD"; then
echo "Failed to reload network after removing the DNS configuration."
cleanup_failed=true
else
echo "Reloading network configuration succeeded."
fi

if [ "$cleanup_failed" = true ]; then
return 1
fi
echo "Reloading network configuration succeeded."

return 0
}

# localdns_cleanup_mode is the entry point for `localdns.sh cleanup`, invoked by
# localdns.service ExecStopPost after both graceful and unexpected exits. It only
# restores node DNS configuration. It intentionally does not delete the dummy
# localdns interface or its .10/.11 addresses: if an orphaned CoreDNS child
# survives a failed cgroup teardown, removing the interface could break a
# listener that is still serving pods and turn a fast failure into default-route
# DNS timeouts. Service/process recovery handles the next-start interface
# lifecycle separately. It always exits 0 so that a best-effort cleanup failure
# cannot wedge systemd's recovery. Cleanup failures are logged.
localdns_cleanup_mode() {
cleanup_iptables_and_dns || echo "LocalDNS cleanup failed: network drop-in may not have been removed; node DNS may still point at the dead listener ${LOCALDNS_NODE_LISTENER_IP}."
exit 0
}

# Cleanup function to remove localdns related configurations.
cleanup_localdns_configs() {
# Disable error handling so that we don't get into a recursive loop.
Expand Down Expand Up @@ -907,7 +945,10 @@ start_localdns_watchdog() {
# Update resource metrics .prom file for the exporter (best-effort, non-fatal)
export_resource_metrics

sleep "${HEALTH_CHECK_INTERVAL}"
# Run sleep in a child so SIGTERM can interrupt the wait and let
# the service's signal/exit cleanup run promptly.
sleep "${HEALTH_CHECK_INTERVAL}" &
wait $!
done
else
# No watchdog configured — write metrics once then wait for CoreDNS to exit
Expand Down Expand Up @@ -983,6 +1024,13 @@ select_localdns_corefile() {

${__SOURCED__:+return}

# ExecStopPost invokes this mode after both graceful and unexpected exits.
# Only restore node DNS configuration here; systemd owns process cleanup.
# Always exit successfully so a cleanup error cannot wedge systemd recovery.
if [ "${1:-}" = "cleanup" ]; then
localdns_cleanup_mode
fi

# --------------------------------------- Main Execution starts here --------------------------------------------------

# Regenerate corefile on every startup to enable dynamic variant selection.
Expand Down
Loading
Loading