diff --git a/e2e/scenario/scenario_localdns_hosts.go b/e2e/scenario/scenario_localdns_hosts.go index a0bf3dcde8d..960b5b2a26d 100644 --- a/e2e/scenario/scenario_localdns_hosts.go +++ b/e2e/scenario/scenario_localdns_hosts.go @@ -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" @@ -23,6 +25,7 @@ func init() { } for _, tt := range tests { + tt := tt cluster := ClusterKubenet if tt.name == "Ubuntu2604Minimal" { cluster = ClusterLatestKubernetesVersionKubenet @@ -42,7 +45,177 @@ 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 +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 +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. +# 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) + +# The terminal-dead test installs a runtime systemd drop-in below. Always +# remove it when this validation exits, including when set -e stops the script +# after a failed assertion, so a failed scenario cannot contaminate a node or +# subsequent validation. Preserve the original test result and report cleanup +# failures separately instead of masking either result. +NORESTART=/run/systemd/system/localdns.service.d/99-e2e-no-restart.conf +restore_localdns_test_state() { + test_status=$? + trap - EXIT + set +e + + if [ -f "$NORESTART" ]; then + cleanup_status=0 + 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; } + 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 + fi + + exit "$test_status" +} +trap restore_localdns_test_state EXIT + +for i in 1 2 3; do + 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. +# 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) + else + current_dns=$(cat /run/systemd/resolve/resolv.conf 2>/dev/null) + 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 + +# 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 +} diff --git a/parts/linux/cloud-init/artifacts/localdns.service b/parts/linux/cloud-init/artifacts/localdns.service index 2f9620b03ea..cbcf0fe4b22 100644 --- a/parts/linux/cloud-init/artifacts/localdns.service +++ b/parts/linux/cloud-init/artifacts/localdns.service @@ -7,13 +7,32 @@ Before=kubelet.service Before=containerd.service # don't run on old images; we're not compatible ConditionKernelVersion=>=5.15 +# Keep recovering from repeated unexpected exits instead of wedging the unit in +# a terminal 'failed' state. LocalDNS binds the pod cluster listener +# (169.254.10.11); if the unit stops being restarted, that listener stays dead +# and every pod on the node black-holes DNS (its /etc/resolv.conf nameserver is +# baked to 169.254.10.11 and cannot be repointed). A wide interval with a high +# burst, combined with the RestartSec backoff below, means a crash storm slows +# restarts but never gives up. +StartLimitIntervalSec=300 +StartLimitBurst=30 [Service] Type=notify NotifyAccess=all WatchdogSec=60 Restart=on-failure +# Back off between restarts so a crash loop does not exhaust the start-limit +# burst in a couple of seconds, and so each restart has time to re-bind the +# node (169.254.10.10) and cluster (169.254.10.11) listeners and re-apply config. +RestartSec=2 KillMode=mixed +# On stop, SIGTERM the control group; on an unexpected exit / SIGKILL of the +# supervisor, systemd still reaps orphaned coredns children so the next +# ExecStart can re-bind the listeners cleanly. +KillSignal=SIGTERM +# 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 diff --git a/parts/linux/cloud-init/artifacts/localdns.sh b/parts/linux/cloud-init/artifacts/localdns.sh index 1da583e476a..3f6c9973664 100644 --- a/parts/linux/cloud-init/artifacts/localdns.sh +++ b/parts/linux/cloud-init/artifacts/localdns.sh @@ -656,6 +656,13 @@ EOF # Remove iptables rules and revert DNS configuration. cleanup_iptables_and_dns() { + # 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 + # 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 @@ -688,7 +695,8 @@ 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." @@ -696,24 +704,38 @@ cleanup_iptables_and_dns() { # Revert DNS configuration and network reload. echo "Removing network drop-in file ${NETWORK_DROPIN_FILE}." - rm -f "$NETWORK_DROPIN_FILE" - if [ "$?" -ne 0 ]; then + if ! rm -f "$NETWORK_DROPIN_FILE"; then echo "Failed to remove network drop-in file ${NETWORK_DROPIN_FILE}." - return 1 + cleanup_failed=true + else + echo "Successfully removed network drop-in file." fi - echo "Successfully removed network drop-in file." 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; systemd owns process cleanup. It always exits +# 0 so that a best-effort cleanup failure cannot wedge systemd's recovery of the +# unit. Cleanup failures are logged (and surfaced by cleanup_iptables_and_dns). +localdns_cleanup_mode() { + cleanup_iptables_and_dns || echo "Best-effort LocalDNS DNS cleanup reported errors." + 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. @@ -983,6 +1005,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. @@ -1042,6 +1071,9 @@ build_localdns_iptable_rules # Ensure cleanup runs before exiting on an error. trap 'echo "Error occurred. Cleaning up..."; cleanup_localdns_configs; exit $ERR_LOCALDNS_FAIL' ABRT ERR INT PIPE +# SIGTERM is the normal systemd stop signal and must be reported as a clean stop. +trap 'echo "Received SIGTERM. Cleaning up..."; cleanup_localdns_configs || true; exit 0' TERM + # Always cleanup when exiting. trap 'echo "Executing cleanup function."; cleanup_localdns_configs || echo "Cleanup failed with error code: $ERR_LOCALDNS_FAIL."' EXIT diff --git a/spec/parts/linux/cloud-init/artifacts/localdns_spec.sh b/spec/parts/linux/cloud-init/artifacts/localdns_spec.sh index e6b95abe9b1..dddc45b10ea 100644 --- a/spec/parts/linux/cloud-init/artifacts/localdns_spec.sh +++ b/spec/parts/linux/cloud-init/artifacts/localdns_spec.sh @@ -2010,4 +2010,107 @@ KUBECTL_EOF The stdout should include "Timeout waiting for node testnode123 to be registered" End End + +# This section tests cleanup_iptables_and_dns and the "cleanup" mode contract +# invoked by localdns.service ExecStopPost. The key guarantees under test: +# 1. DNS restoration (drop-in removal + network reload) still runs even when +# iptables rule deletion fails (no early return). +# 2. cleanup_iptables_and_dns reports overall failure when any step fails. +# 3. "cleanup" mode exits 0 whether cleanup succeeds or fails, so a cleanup +# error cannot wedge systemd recovery. +#------------------------------------------------------------------------------------------------------------------------------------ + Describe 'cleanup_iptables_and_dns' + setup() { + Include "./parts/linux/cloud-init/artifacts/localdns.sh" + + TEST_DIR="$(mktemp -d)" + NETWORK_DROPIN_DIR="${TEST_DIR}/run/systemd/network/eth0.network.d" + NETWORK_DROPIN_FILE="${NETWORK_DROPIN_DIR}/70-localdns.conf" + mkdir -p "${NETWORK_DROPIN_DIR}" + cat > "${NETWORK_DROPIN_FILE}" <<'EOF' +[Network] +DNS=169.254.10.10 +EOF + # No localdns iptables rules by default (empty listing). + iptables() { return 0; } + # networkctl reload succeeds by default. + NETWORKCTL_RELOAD_CMD="networkctl_reload_mock" + networkctl_reload_mock() { return 0; } + } + + cleanup_dirs() { + rm -rf "$TEST_DIR" + } + + BeforeEach 'setup' + AfterEach 'cleanup_dirs' + + It 'removes the DNS drop-in and reloads network on success' + When call cleanup_iptables_and_dns + The status should be success + The stdout should include "Successfully removed network drop-in file." + The stdout should include "Reloading network configuration succeeded." + The path "$NETWORK_DROPIN_FILE" should not be exist + End + + It 'removes existing localdns iptables rules and reports success' + # Simulate existing localdns rules whose deletion succeeds. The + # listing output must contain the "localdns: skip conntrack" comment + # so the script's grep keeps it; the rule number is the first field. + iptables() { + case "$*" in + *"-D "*) return 0 ;; # deletion succeeds + *"-L "*) echo "1 RETURN all -- 0.0.0.0/0 0.0.0.0/0 /* localdns: skip conntrack */" ;; + *) return 0 ;; + esac + } + When call cleanup_iptables_and_dns + The status should be success + The stdout should include "Successfully removed existing localdns iptables rule" + The stdout should include "Successfully removed network drop-in file." + The stdout should include "Reloading network configuration succeeded." + The path "$NETWORK_DROPIN_FILE" should not be exist + End + + It 'still removes the DNS drop-in and reloads when iptables deletion fails' + # Simulate existing localdns rules whose deletion fails. The listing + # output must contain the "localdns: skip conntrack" comment so the + # script's grep keeps it; the rule number is the first field. + iptables() { + case "$*" in + *"-D "*) return 1 ;; # deletion always fails + *"-L "*) echo "1 RETURN all -- 0.0.0.0/0 0.0.0.0/0 /* localdns: skip conntrack */" ;; + *) return 0 ;; + esac + } + When call cleanup_iptables_and_dns + # Overall status is failure because iptables cleanup failed... + The status should be failure + # ...but DNS restoration still ran. + The stdout should include "Failed to remove existing localdns iptables rule" + The stdout should include "Successfully removed network drop-in file." + The stdout should include "Reloading network configuration succeeded." + The path "$NETWORK_DROPIN_FILE" should not be exist + End + + It 'reports failure when network reload fails' + networkctl_reload_mock() { return 1; } + When call cleanup_iptables_and_dns + The status should be failure + The stdout should include "Failed to reload network after removing the DNS configuration." + End + + It 'cleanup mode exits 0 when cleanup succeeds' + cleanup_iptables_and_dns() { return 0; } + When run localdns_cleanup_mode + The status should be success + End + + It 'cleanup mode exits 0 even when cleanup fails' + cleanup_iptables_and_dns() { return 1; } + When run localdns_cleanup_mode + The status should be success + The stdout should include "Best-effort LocalDNS DNS cleanup reported errors." + End + End End