Skip to content

fix: restore DNS after unexpected LocalDNS exit - #9360

Open
Saewon Kwak (saewoni) wants to merge 3 commits into
mainfrom
fix/localdns-cgroup-teardown
Open

fix: restore DNS after unexpected LocalDNS exit#9360
Saewon Kwak (saewoni) wants to merge 3 commits into
mainfrom
fix/localdns-cgroup-teardown

Conversation

@saewoni

@saewoni Saewon Kwak (saewoni) commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem

This PR addresses the LocalDNS failure reported by Azure/AKS#5930. localdns.service supervises localdns.sh, which starts CoreDNS as a background process. During an unexpected supervisor exit, such as SIGKILL, 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.

Fix

  • Add ExecStopPost=/opt/azure/containers/localdns/localdns.sh cleanup so DNS cleanup runs after both normal and unexpected service exits.
  • Add a cleanup mode to localdns.sh that restores node DNS configuration and always exits successfully; systemd remains responsible for process cleanup.
  • Handle normal SIGTERM with graceful cleanup and exit status 0; genuine error paths retain the existing failure status.
  • Add isolated AgentBaker E2E coverage for Ubuntu 22.04, Ubuntu 24.04, and Azure Linux V3, covering both the legacy Bash CSE and aks-node-controller bootstrap paths.

The cgroup delegation configuration and unrelated network reconfiguration change are intentionally not part of this PR.

Live reproduction

The failure was reproduced twice on the disposable cluster sakwa-localdns-repro-0710 using the affected Ubuntu 24.04 image AKSUbuntu-2404gen2containerd-202608.06.1. Reproduction targeted the single system-pool node aks-sysnp-14424852-vmss000000 with LocalDNS enabled.

Healthy baseline:

ActiveState=active
SubState=running
Result=success
NRestarts=0
nameserver 169.254.10.10

Fault injection:

for i in 1 2 3 4 5 6 7 8 9 10; do
  main=$(systemctl show -p MainPID --value localdns.service)
  if [ "$main" -gt 0 ]; then
    kill -9 "$main" || true
  fi
  sleep 0.25
done

Observed failure:

Failed to kill control group /localdns.slice/localdns.service, ignoring: Invalid argument
Start request repeated too quickly
Failed to start localdns.service
ActiveState=failed
SubState=failed
Result=signal

The first run reached NRestarts=13; the second reached NRestarts=6. Each run was recovered with systemctl reset-failed localdns.service followed by systemctl restart localdns.service.

Validation

The E2E scenario uses isolated, scenario-created single-VM VMSS instances. It validates normal stop/start, then repeats supervisor termination and polls for service recovery. It asserts the final service state and rejects the cgroup teardown and StartLimit errors.

The E2E package compiles locally:

cd e2e
go test ./... -run ^ -count=1 -timeout 5m

The actual cloud E2E requires VHDs built from this branch because the LocalDNS unit files are baked into the VHD. The prior cloud E2E run reached the test pipeline but failed on test-harness issues; those issues were corrected in this update. A fresh cloud E2E result for the current commit is still required.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Windows Unit Test Results

  3 files   13 suites   40s ⏱️
409 tests 409 ✅ 0 💤 0 ❌
412 runs  412 ✅ 0 💤 0 ❌

Results for commit 94d56c0.

♻️ This comment has been updated with latest results.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Removes unnecessary LocalDNS cgroup delegation to prevent teardown failures after unclean supervisor termination.

Changes:

  • Sets Delegate=no for localdns.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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new E2E test does not compile and invokes an unsupported systemctl verb.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

e2e/scenario_localdns_lifecycle_test.go:43

  • 🔴 High Risk — 🔧 Script Logic: Config.Validator is declared as func(context.Context, *Scenario) error (e2e/types.go:233-234), so this no-result function cannot be assigned at line 35 and the E2E package will not compile. Return the error from execScriptOnVMForScenarioValidateExitCode (discarding only its result value).
func validateLocalDNSLifecycle(ctx context.Context, s *Scenario) {
	execScriptOnVMForScenarioValidateExitCode(ctx, s, `
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated

# Verify the normal stop path completes before the service is started again.
systemctl stop localdns.service
systemctl is-inactive --quiet localdns.service

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 commit c3778a344. The E2E now uses sudo for mutating systemctl and kill operations, and polls boundedly for active/running instead of relying on a fixed two-second delay.

Delegate=cpu No newline at end of file
# LocalDNS does not create or manage child cgroups. Keeping the service
# non-delegated lets systemd reliably tear down all CoreDNS descendants.
Delegate=no

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.

I think this is the correct fix. But let's verify the metrics exporter is still working after the change.

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.

This comment was on the earlier Delegate=no change, which has been removed from PR #9360. The current PR does not change cgroup layout, LOCALDNS_CGROUP_DIR, cpu.stat, memory.current, resources.prom, or the exporter. The existing LocalDNS exporter E2E validation remains applicable and checks non-zero CPU and memory metrics. No exporter-specific regression is introduced by the current Fix 1 changes.

@yewmsft Ye Wang [msft] (yewmsft) left a comment

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.

Review — LocalDNS delegated cgroup teardown

good root cause work and the live repro is genuinely useful. but I think the framing is off, and there are two blockers.

this is not a cgroup bug, it is a missing teardown guarantee. nothing reverts the node DNS config when the supervisor dies without cooperating. SIGKILL is not trappable, so no trap and no ExecStop= can ever cover the kill -9 path you reproduced. ExecStopPost= is the hook that runs on every exit path — details inline on localdns.service.

blockers:

  1. ExecStop= does not run when the main process dies unexpectedly, so it does nothing for the bug in the description — and it regresses ordinary stop into failed. the copilot comment on that line is correct, verified: ERR_LOCALDNS_FAIL=216 at localdns.sh:10, trap at :1056, no SuccessExitStatus= in the unit.
  2. e2e/scenario_localdns_lifecycle_test.go:43 drops both return values, so the new test cannot fail.

also: ${DEFAULT_ROUTE_INTERFACE} is unbound under set -u on the cleanup path (localdns.sh:719-722).

please split this. Delegate=cpuno and the networkctl reconfigure addition are two unrelated changes riding along. both stand on their own merits, and the delegate one can merge immediately.

one question before any of this: on the AKS#5930 nodes, was 70-localdns.conf still on disk while the service was dead? if yes that confirms missing-teardown over the cgroup story, and reshapes the fix.

KillMode=control-group
# Ask localdns.sh to run its graceful cleanup first. The stop helper waits for
# the parent to exit before systemd applies KillMode=control-group to leftovers.
ExecStop=/bin/sh -c 'kill -SIGINT "$MAINPID" 2>/dev/null || true; while kill -0 "$MAINPID" 2>/dev/null; do sleep 0.1; done'

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.

ExecStop only runs for an explicit stop job. when the main process dies unexpectedly — your kill -9 $MAINPID repro — systemd skips ExecStop and goes straight to ExecStopPost. man systemd.service under ExecStopPost=: "this includes cases where the commands configured in ExecStop= were used, or where the service exited unexpectedly."

so this line does nothing for the bug in the description. it only changes systemctl stop/restart.

and the copilot comment above is right. SIGINT lands on the error trap at localdns.sh:1056 which exits $ERR_LOCALDNS_FAIL (216, localdns.sh:10), and there is no SuccessExitStatus=. so every ordinary stop now leaves the unit in failed. today SIGTERM is untrapped, bash dies from the signal systemd itself sent, and stop is clean — so this is a regression, not a fix.

drop ExecStop.

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 commit c3778a344. Removed ExecStop; added cleanup-only ExecStopPost=/opt/azure/containers/localdns/localdns.sh cleanup, which runs after unexpected exits and always returns 0. Added a dedicated SIGTERM trap that runs graceful cleanup and exits 0. The existing 216 failure status remains for genuine error paths.

KillMode=mixed
# CoreDNS is launched as a child of localdns.sh. Kill the complete service
# cgroup together so no descendant survives an unclean supervisor exit.
KillMode=control-group

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.

this is the wrong layer. the real problem is that nothing reverts the node DNS config when the supervisor dies without cooperating — SIGKILL is not trappable, so no trap and no ExecStop can ever cover it.

use ExecStopPost, which runs on every exit path including kill -9:

ExecStopPost=/opt/azure/containers/localdns/localdns.sh cleanup

localdns.sh already defines all its functions above the main flow, and cleanup_iptables_and_dns self-initializes its network vars (:665), so this is small:

# after function definitions, before the main flow
if [ "${1:-}" = "cleanup" ]; then
    cleanup_localdns_configs || echo "Best-effort cleanup reported errors."
    exit 0
fi

it must exit 0 unconditionally — a failing ExecStopPost marks the unit failed, on exactly the teardown path we are trying to keep unwedged.

now walk your repro with that in place: kill -9 → ExecStopPost removes the drop-in → restart → ExecStart re-adds it → ... → StartLimit trips, unit stays dead, but the last ExecStopPost already ran, so resolv.conf is back on upstream DNS. no blackhole. "unit is dead" becomes degraded-but-working instead of an outage.

and add a TERM trap in the script so ordinary stop is graceful and exits 0:

trap 'echo "Received SIGTERM, shutting down."; cleanup_localdns_configs; exit 0' TERM

keep 216 on ABRT ERR INT PIPE for genuine failures. note cleanup can then run more than once (TERM trap + EXIT trap + ExecStopPost), so it has to be idempotent.

KillMode=control-group is fine to keep, but it is not the fix — mixed already SIGKILLs the whole cgroup at TimeoutStopSec.

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 commit c3778a344. ExecStopPost now invokes the script in cleanup mode, so it handles both explicit stops and unexpected exits. Cleanup-only mode performs DNS/network cleanup and exits 0; systemd remains responsible for process/cgroup cleanup. The normal SIGTERM trap also exits 0 after graceful cleanup.

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.

Applied Fix 1 in commit c9862f958. The PR is now focused on unexpected-exit DNS teardown: ExecStopPost invokes cleanup mode after any service exit, normal SIGTERM cleanup exits 0, and process cleanup remains systemd-owned. The unrelated delegation and network reconfigure changes were removed from the branch.

Delegate=cpu No newline at end of file
# LocalDNS does not create or manage child cgroups. Keeping the service
# non-delegated lets systemd reliably tear down all CoreDNS descendants.
Delegate=no

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.

this one is right and stands on its own. localdns.sh forks coredns as a plain child and never writes cgroup.procs or cgroup.subtree_control — it only reads cpu.stat and memory.current at :816-817. delegation for a payload that never builds a subtree is dead config. and if the original intent was cpu accounting, the directive is CPUAccounting=yes, not Delegate=.

nit: Delegate=no is the systemd default, so this file is now a no-op. keeping it for explicitness is fine — say that in the comment.

please split this into its own PR. it is unrelated cleanup and can merge immediately, while the teardown fix needs an isolated repro.

one check before it goes: the exporter reads $LOCALDNS_CGROUP_DIR/cpu.stat and memory.current with || echo "0", so it would break silently. ValidateLocalDNSExporterMetrics already asserts non-zero and runs for every localdns scenario (validation.go:111) — but e2e runs main's VHD, so it will not exercise this change. run it against a VHD built from the branch, or just eyeball cat /sys/fs/cgroup/localdns.slice/cgroup.subtree_control on the repro node (expect cpu memory).

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.

The delegation change is no longer part of this PR. localdns-delegate.conf remains unchanged at Delegate=cpu; the PR now focuses on unexpected-exit DNS teardown and the service lifecycle. The exporter remains covered by the existing LocalDNS exporter validation; this change does not alter the cgroup path or resource accounting.

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.

I traced the history and current code for Delegate=cpu.

It was introduced in the original LocalDNS VHD packaging change, 9b65e12735, Add localdns related files to VHDs (#6110), on April 7, 2025. The commit introduced localdns.service, localdns.sh, and localdns-delegate.conf, but does not document why CPU delegation was added.

The current code shows that LocalDNS:

  • Launches CoreDNS as a normal child of localdns.sh.
  • Reads cpu.stat and memory.current for metrics.
  • Does not write cgroup.procs or cgroup.subtree_control.
  • Does not create or manage child cgroups.

The resource limits are configured separately on localdns.slice with CPUQuota and MemoryMax. Reading cgroup metric files does not require Delegate=cpu; if the original intent was CPU accounting, the relevant systemd setting would be CPUAccounting=yes, not Delegate=.

So Delegate=cpu appears to be legacy or unnecessary configuration, but the history does not prove the original intent. I agree it should be treated as a separate cleanup PR rather than part of the unexpected-exit DNS teardown fix. Any follow-up should validate the exporter and cgroup metrics across Ubuntu, Azure Linux, and Mariner using VHDs built with and without delegation.

echo "Reloading network configuration succeeded."

echo "Reconfiguring network interface ${DEFAULT_ROUTE_INTERFACE}."
"${NETWORKCTL_RECONFIGURE_CMD[@]}" "${DEFAULT_ROUTE_INTERFACE}"

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.

three things here:

  1. ${DEFAULT_ROUTE_INTERFACE} with no :-. set -euo pipefail at :2, and every other reference in this file uses ${DEFAULT_ROUTE_INTERFACE:-}. the guard at :666 only initializes when NETWORK_DROPIN_FILE/DIR is empty, so this can be unbound here → bash aborts mid-cleanup with iptables and resolv.conf half reverted. set +e in cleanup_localdns_configs does not cover set -u.

  2. if [ "$?" -ne 0 ]if ! "${NETWORKCTL_RECONFIGURE_CMD[@]}" "${DEFAULT_ROUTE_INTERFACE:-}"; then

  3. should a reconfigure failure really fail cleanup? you are adding a second hard-fail to the teardown path in a PR whose whole point is that teardown must not get stuck. log and continue.

also :712 right above is still eval "$NETWORKCTL_RELOAD_CMD" — same injection shape, same function. you converted the new one to an array, convert that one too.

# Reapply the loaded network configuration to the default-route link after removing
# the LocalDNS drop-in. Reload only refreshes networkd's configuration; reconfigure
# causes the live link state, including DNS settings, to be reapplied.
NETWORKCTL_RECONFIGURE_CMD=(networkctl reconfigure)

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.

this whole reconfigure change is a separate fix — different root cause (stale resolver entry), different failure mode — appended to a cgroup-teardown PR. please split it out. it makes the teardown fix reviewable and shippable on its own.

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 commit c3778a344. Removed the unrelated networkctl reconfigure addition and its tests from this PR. The existing network reload path is unchanged.

Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
systemctl start localdns.service
systemctl is-active --quiet localdns.service

for i in 1 2 3 4 5 6 7 8 9 10; do

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.

StartLimitBurst=5 in 10s, and you fire 10 kills over 2.5s. whether you trip the limit depends on how fast the unit comes back up between kills, so this is timing dependent — it will flake.

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 commit c3778a344. The E2E now waits for each restart to reach active/running with a bounded polling loop rather than depending on a fixed delay.

Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
return 1
fi

if ! grep -Fq 'ExecStop=/bin/sh -c' /etc/systemd/system/localdns.service || \

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.

this asserts on the exact shell text of ExecStop. reformat the line and the test breaks with zero behavior change.

assert the merged unit instead:

systemctl show localdns.service -p KillMode -p Delegate --value

that also proves the drop-in actually merged, which grepping two separate files does not.

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 commit c3778a344. The VHD content test now checks for the ExecStopPost cleanup hook rather than asserting the exact old ExecStop shell text. Runtime merged-unit behavior is covered by the lifecycle E2E.

Comment thread spec/parts/linux/cloud-init/artifacts/localdns_spec.sh Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The lifecycle test rejects the correct resolver state and does not reproduce the reported rapid-restart race.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

e2e/scenario_localdns_lifecycle_test.go:85

  • 🟡 Medium Risk — 🔧 Script Logic: This assertion is inverted for the final state: the service has been started and confirmed active, and disable_dhcp_use_clusterlistener configures the live node resolver to use 169.254.10.10 (localdns.sh:635-648). The existing ValidateLocalDNSResolution likewise requires default dig output to report that server (e2e/validators.go:2108-2120). As written, a correctly configured LocalDNS node fails this new lifecycle test; poll until the listener appears instead of asserting that it is absent.
  • Files reviewed: 4/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
Comment on lines +65 to +78
for i in 1 2 3; do
main=$(sudo systemctl show -p MainPID --value localdns.service)
sudo kill -9 "$main"
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)
if [ "$state" = $'active\nrunning' ]; then
recovered=true
break
fi
sleep 1
done
test "$recovered" = true
done
Copilot AI review requested due to automatic review settings September 2, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Three critical issues leave the intended cgroup and DNS recovery fixes incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/5 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
printf '%s\n' "$state" | grep -q '^ActiveState=active$'
printf '%s\n' "$state" | grep -q '^SubState=running$'
printf '%s\n' "$state" | grep -q '^Result=success$'
! sudo awk '/^nameserver/ {print $2}' /run/systemd/resolve/resolv.conf | grep -qx '169.254.10.10'
@@ -1,2 +1,2 @@
[Service]
Delegate=cpu No newline at end of file
Delegate=cpu
Comment on lines 706 to 710
echo "Attempt to reload network configuration."
eval "$NETWORKCTL_RELOAD_CMD"
if [ "$?" -ne 0 ]; then
if ! "${NETWORKCTL_RELOAD_CMD}" reload; then
echo "Failed to reload network after removing the DNS configuration."
return 1
fi
Copilot AI review requested due to automatic review settings September 2, 2026 20:59
@saewoni
Saewon Kwak (saewoni) force-pushed the fix/localdns-cgroup-teardown branch from c3778a3 to c9862f9 Compare September 2, 2026 20:59
@saewoni Saewon Kwak (saewoni) changed the title fix: prevent LocalDNS delegated cgroup teardown failures fix: restore node DNS after LocalDNS exits unexpectedly Sep 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

DNS restoration can be skipped after iptables failures, and the automated tests have correctness and coverage gaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 5
  • Review effort level: Balanced

# 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
cleanup_iptables_and_dns || echo "Best-effort LocalDNS DNS cleanup reported errors."
Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
Comment on lines +35 to +38
AKSNodeConfigMutator: func(_ *Cluster, cfg *aksnodeconfigv1.Configuration) {
cfg.LocalDnsProfile.EnableLocalDns = true
},
Validator: validateLocalDNSLifecycle,
Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
Comment on lines +58 to +62
# Repeatedly kill the supervisor and wait for Restart=on-failure recovery.
for i in 1 2 3; do
main=$(sudo systemctl show -p MainPID --value localdns.service)
test "$main" -gt 0
sudo kill -9 "$main"
Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
Comment on lines +64 to +72
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)
if [ "$state" = $'active\nrunning' ]; then
recovered=true
break
fi
sleep 1
done
Comment on lines +989 to +991
if [ "${1:-}" = "cleanup" ]; then
cleanup_iptables_and_dns || echo "Best-effort LocalDNS DNS cleanup reported errors."
exit 0
@saewoni Saewon Kwak (saewoni) changed the title fix: restore node DNS after LocalDNS exits unexpectedly fix: restore DNS after unexpected LocalDNS exit Sep 2, 2026
Comment thread e2e/scenario_localdns_lifecycle_test.go Outdated
AKSNodeConfigMutator: func(_ *Cluster, cfg *aksnodeconfigv1.Configuration) {
cfg.LocalDnsProfile.EnableLocalDns = true
},
Validator: validateLocalDNSLifecycle,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If possible, could you attach validateLocalDNSLifecycle as the Validator on the existing Test_LocalDNSHostsPlugin distros instead of 3 new scenarios, gated to the 3 target distros and running last, to reuse already-provisioned LocalDNS nodes. This can save some test resources and reduce test time.

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 commit c1b9b1209c. The standalone lifecycle scenarios were removed, and validateLocalDNSLifecycle is now attached to the existing Test_LocalDNSHostsPlugin scenarios for Ubuntu 22.04, Ubuntu 24.04, and Azure Linux V3. It runs after the existing validation and reuses the already-provisioned LocalDNS VM for each distro. Both BootstrapConfigMutator and AKSNodeConfigMutator remain enabled. The E2E module compiles successfully.

@aks-node-assistant

Copy link
Copy Markdown
Contributor

AgentBaker Linux gate detective

Run: https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=179413719
Failed job/stage/task: Run AgentBaker E2E / Test_LocalDNSLifecycle/Ubuntu2404 and Test_LocalDNSLifecycle/AzureLinuxV3

TL;DR: LocalDNS lifecycle validation failed after stop/restart/kill recovery because journalctl still contained localdns.service: Failed to kill control group /localdns.slice/localdns.service, ignoring: Invalid argument; the lifecycle validation command exited 1.

Likely cause / signature: e2e-localdns-lifecycle-controlgroup-not-cleaned — LocalDNS lifecycle cleanup/control-group validation failure.
Confidence: High.
Assessment: PR-change-caused is likely: this PR directly changes localdns.service, localdns.sh, and adds LocalDNS lifecycle E2E coverage, and both Ubuntu2404 and AzureLinuxV3 lifecycle scenarios fail on the new validation.

Strongest alternative: systemd/cgroup VM-local flake; less likely because the same validation failed across two OS scenarios in the same PR run and the changed files directly target LocalDNS lifecycle cleanup.

Recommended owner/action: PR author / LocalDNS owner: inspect service cleanup behavior around the LocalDNS cgroup and ensure the new lifecycle test tolerates/handles systemd Failed to kill control group correctly only when safe.

Evidence: build summary shows Run AgentBaker E2E exit 1; failed tests include LocalDNSLifecycle scenarios; E2E log shows node/pod readiness and localdns exporter validation passed, then lifecycle command failed on repeated Failed to kill control group journal entries.

Wiki signature: e2e-localdns-lifecycle-controlgroup-not-cleaned

Copilot AI review requested due to automatic review settings September 3, 2026 01:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Cleanup errors can leave DNS unrestored, and the E2E test has recovery-detection and terminal-state coverage gaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

parts/linux/cloud-init/artifacts/localdns.sh:990

  • 🔴 High Risk — 🔧 Script Logic: This post-stop path does not reliably restore DNS on a cleanup error. cleanup_iptables_and_dns returns immediately when any iptables deletion fails (lines 690-692), before it removes 70-localdns.conf (lines 697-704); this branch then logs the error and exits 0, leaving the node pointed at the dead resolver—the outage this change is intended to prevent. Make DNS drop-in removal/reload unconditional and aggregate any iptables error afterward.
    cleanup_iptables_and_dns || echo "Best-effort LocalDNS DNS cleanup reported errors."
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment on lines +80 to +84
# Repeatedly kill the supervisor and wait for Restart=on-failure recovery.
for i in 1 2 3; do
main=$(sudo systemctl show -p MainPID --value localdns.service)
test "$main" -gt 0
sudo kill -9 "$main"
Comment on lines +88 to +92
state=$(sudo systemctl show localdns.service -p ActiveState -p SubState --value)
if [ "$state" = $'active\nrunning' ]; then
recovered=true
break
fi
@Devinwong

Copy link
Copy Markdown
Collaborator

LGTM in terms of the integration with overall provisioning process and AgentBaker's e2e infra.
For localDNS feature domain knowledge and implementation, I don't see any concern in this PR but will defer to the feature owner team to approve.

Copilot AI review requested due to automatic review settings September 3, 2026 23:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

DNS restoration is not guaranteed, and the regression tests can false-pass or overlook expected failure signals.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

e2e/scenario_localdns_hosts_test.go:93

  • 🟡 Medium Risk — 🔧 Script Logic: This poll can accept the pre-kill service instance: immediately after kill -9, systemd may still report active/running before processing the supervisor's death. The loop then breaks without proving that Restart=on-failure created a new supervisor, so the regression can false-pass. Require a positive MainPID different from the killed PID before declaring recovery.
        state=$(sudo systemctl show localdns.service -p ActiveState -p SubState --value)
        if [ "$state" = $'active\nrunning' ]; then
            recovered=true
            break
        fi

parts/linux/cloud-init/artifacts/localdns.sh:990

  • 🔴 High Risk — 🔧 Script Logic: cleanup_iptables_and_dns returns immediately when any iptables deletion fails (lines 690–692), before it removes the DNS drop-in and reloads the network (lines 697–711). This new post-exit path then masks that failure and exits successfully, so a failed service can still leave node DNS pointing at the dead LocalDNS listener. Make DNS restoration an unconditional/finally-style step, and report any iptables error only after the DNS step has run.
    cleanup_iptables_and_dns || echo "Best-effort LocalDNS DNS cleanup reported errors."

parts/linux/cloud-init/artifacts/localdns.sh:991

  • 🟡 Medium Risk — 🧪 Test Coverage: The repository requires ShellSpec coverage for shell-script changes, but the existing localdns_spec.sh sources this file and returns at line 984, so it never exercises this new executable cleanup dispatch or its always-success contract. Add ShellSpec cases that invoke cleanup mode with successful and failing cleanup implementations; the VHD-content assertion only verifies the unit text, not this behavior.
if [ "${1:-}" = "cleanup" ]; then
    cleanup_iptables_and_dns || echo "Best-effort LocalDNS DNS cleanup reported errors."
    exit 0
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +104 to +106
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants