fix: bound LocalDNS restart budget so failures terminate deterministically - #9439
Merged
Merged
Conversation
Saewon Kwak (saewoni)
requested a review
from Ye Wang (yewmsft)
as a code owner
September 9, 2026 20:45
Contributor
Windows Unit Test Results 3 files 17 suites 1m 7s ⏱️ Results for commit 6ffca34. ♻️ This comment has been updated with latest results. |
Saewon Kwak (saewoni)
added this pull request to stack #9440
September 9, 2026 21:05
Contributor
Author
Node SIG review requestedCould Node SIG please review AgentBaker PR #9439? This PR addresses a LocalDNS pod-DNS outage after repeated LocalDNS crashes. When The PR adds a controlled systemd recovery policy: StartLimitIntervalSec=300
StartLimitBurst=30
RestartSec=2When LocalDNS restarts, CoreDNS rebinds Live validationWe reproduced the failure and the recovery on the same live LocalDNS-enabled AKS node:
The pod continued using the same PR #9439 is stacked on AgentBaker PR #9360, which handles node-level DNS restoration through Questions / current findings
This PR is intended to improve recovery from transient crash storms. It does not add an infinite retry policy or a zero-downtime DNS standby fallback. Persistent-failure handling and a zero-gap fallback remain separate design questions. |
Saewon Kwak (saewoni)
changed the title
Saewon Kwak (saewoni)
force-pushed
the
fix/localdns-cgroup-teardown
branch
from
September 10, 2026 01:25
21e8b5e to
ea38772
Compare
Saewon Kwak (saewoni)
requested review from
Devinwong,
Sri Harsha (SriHarsha001),
Abdul Asfari (aboodasfari),
Nishchay (awesomenix),
Ben Brady (benjamin-brady),
Calvin S. (calvin197),
Cameron Meissner (cameronmeissner),
Sylvain Boily (djsly),
fcher,
Ganeshkumar Ashokavardhanan (ganeshkumarashok),
janenotjung-hue,
Karen Chen (karenychen),
lilypan26,
Mark Ibrahim (mxj220),
Peter Damianov (pdamianov-dev),
Patrick W. Healy (phealy),
Artur Khantimirov (r2k1),
runzhen,
Shun Lyu (sinmentis),
sulixu,
Tim Wright (timmy-wright),
Thibault Cohen (titilambert),
Xu Xue (xuexu6666) and
Zachary (zachary-bailey)
as code owners
September 10, 2026 01:25
… with a spec
The TERM trap added to reap the watchdog sleep also made 'exit 0' fire the
EXIT trap, so cleanup_localdns_configs now runs in-process on every systemd
stop where previously bash died first and only ExecStopPost ran. That was a
side effect, not a decision, and nothing recorded it.
Keeping the behaviour. Measured on a node rather than argued (journal from
gate build 181777373, AzureLinuxV3, n=11 stops / n=18 starts):
stop 5-6s (essentially all of it LOCALDNS_SHUTDOWN_DELAY draining)
start 1-2s
restart 6-8s against the 'timeout 30 systemctl restart localdns' in
enableLocalDNS() and its e2e mirror -- ~22-24s of headroom
So the cost lands at about a quarter of the only bound that constrains it,
and the behaviour it buys is better than the alternative: without the drain
CoreDNS is SIGKILLed by the cgroup on every stop, dropping in-flight queries.
Trimming it would spend real shutdown quality to reclaim time nothing needs.
It does not move the restart-budget math. The worst cycle is a hung start,
where this trap cannot run at all -- bash defers a trapped signal until the
foreground command returns, and that fault never returns -- so systemd spends
the full TimeoutStopSec there regardless.
Second consequence, previously unrecorded: cleanup_localdns_configs now runs
to completion, so the dummy interface carrying .10/.11 is torn down on a stop.
localdns_cleanup_mode (ExecStopPost) deliberately leaves the link alone in case
an orphaned CoreDNS is still answering on .11, so the two paths used to differ;
for a clean stop they now agree. The teardown/recreate cycle was clean in the
same run: 12 teardowns, 18 setups, zero address-in-use or RTNETLINK errors.
Worth knowing for #9486 -- its "create the dummy interface idempotently"
constraint is now the common case on a clean stop rather than the exception.
The spec covers the trap wiring, which nothing did before. The traps sit below
"${__SOURCED__:+return}", so Include cannot reach them; the spec greps the real
trap lines out of the shipped script and executes them against stubs instead of
restating them, so deleting or altering a trap fails the test. Mutation-checked
both ways: removing the TERM trap fails all three examples, and changing its
'exit 0' to 'exit 1' fails all three.
Comment and test only; no behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he burst check
Two fixes, both from reviewing the e2e after build 181818040 lost three
LocalDNS lanes.
1. The lifecycle script outgrew the Bastion tunnel.
Scripts are SCP'd to the node, and tunnelSession.Write (bastionssh.go)
forwards each SSH packet as one websocket message with no chunking
against Bastion's 8,192-byte cap. go-scp sends a 4,096-byte first chunk
then the remainder in a single write, so
max_write = script_size - 4096 + 45 (45 = framing + MAC)
which first exceeds the cap at a script size of 12,243. The lifecycle
script had been sitting 380 bytes under that and grew 512, producing the
8,324-byte write that appears verbatim in the StatusMessageTooBig close
frame. The scenario dies as a lost SSH session and loses its node logs,
so nothing points at script length as the cause.
53% of that script was comments. Comments cost the same on the wire as
code and do nothing at runtime, so the rationale moves into the Go doc
comment -- none of it is lost, and the script goes 12,375 -> 6,821 bytes.
The same treatment on localdnsFaultRunScript takes it 5,765 -> ~4,170,
which matters more than it looks: that one is sent seven times per run.
TestLocalDNSScriptsFitBastionLimit now measures every script this package
sends, not just the one that broke, and prints the headroom.
The real fix is chunking in tunnelSession.Write. That is shared e2e
infrastructure and out of scope here; this guard holds the line until it
lands, and every other caller in the suite still has the same cliff.
2. The ExecStart count is now bounded on both sides.
The comment claimed "exactly the burst should have run" while the check
only rejected fewer, so a limiter that allowed twenty starts before
eventually tripping would pass -- the journal refusal line appears either
way. It is now [burst, burst+1].
The upper bound has deliberate slack. Copilot twice recommended equality;
the two measurements disagree on whether that is safe:
by hand, live node (2026-09-17): postready = 6, others 5
CI, gate build 181777373 (Ubuntu2404): postready = 5, every mode 5
The 6 is unreproduced and unexplained. Tolerating it costs little -- a
limiter allowing exactly one extra start slips through -- against a lane
that flakes only when it recurs. The doc comment records both numbers so
the next reader sees the conflict rather than re-tightening it, and notes
that the hungstart journal count is not usable evidence here because
measureLocalDNSWorstCycle runs that same fault before the matrix.
Also recorded: $burst is read from the live unit, so this compares the
SUT against itself, and is only sound because
assertLocalDNSBudgetDirectives pins StartLimitBurst to 5 earlier in the
same validation. Removing or reordering that assertion silently guts this
check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nour it fbe622a pinned TimeoutStopFailureMode=terminate in localdns.service. It has no effect there. systemd.unit(5): "Drop-in files under any of these directories take precedence over unit files wherever located." Azure Linux ships a type-wide drop-in at /usr/lib/systemd/system/service.d/10-timeout-abort.conf (systemd.spec line 893) setting TimeoutStopFailureMode=abort, so the unit file loses and the effective value on AzureLinuxV3 stayed 'abort'. Which means the cycle stayed at the 153s that pin was written to avoid -- above the 144s threshold, so the slow failure modes would restart forever on that distro. Exactly the bug this PR exists to remove, reintroduced by the fix for it. Reproduced and fixed with the real files rather than a synthetic unit: unit file only -> terminate + /usr/lib/.../service.d/10-abort drop-in -> abort <- the bug + localdns.service.d/delegate.conf -> terminate <- the fix Effective budget afterwards: Restart=on-failure, RestartUSec=2s, TimeoutStartUSec=1min 30s, TimeoutStopUSec=30s, TimeoutStopFailureMode=terminate, StartLimitIntervalUSec=12min, StartLimitBurst=5, Delegate=yes/cpu. Worst cycle 90+30+2 = 122s against the 144s threshold, on every image. localdns-delegate.conf is the right home because it is already a unit-specific drop-in for this unit, already installed to /etc/systemd/system/localdns.service.d/delegate.conf by packer_source.sh, and already shipped by all nine packer definitions plus azlosguard.yml. A new drop-in file would mean editing each of those, where missing one ships an image without it and fails silently months later on one distro. The cost is that a file named for Delegate=cpu now also carries a stop-timeout directive; mitigated by the comment there and a pointer from localdns.service, and a rename is a standalone change if it is wanted. The e2e assertion is unchanged and was right all along: it reads the effective value via systemctl show, so it does not care which file sets it. Credit to @yewmsft for catching that the pin could not work where it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng loop Five review items from @yewmsft. 1. The Bastion size constant was justified two ways that land a byte apart. The sweep is the measurement and the formula is a model: 12,242 is safe and 12,243 produced an 8,196-byte write, where the formula predicts 8,192. It ignores SSH block padding, so writes step rather than increment. Comment now says to trust the sweep and not to "correct" the constant upward from the arithmetic. 2. "Covers every script" was only true of the ones someone remembered to add to the map. TestLocalDNSScriptInventoryIsRegistered now derives the inventory from the source and fails the build when a declared localdns*Script is not size-checked. Mutation-tested: declaring an unregistered script fails it. 3. 'if ! retrycmd_if_failure ...' discarded the return code. '!' inverts before $? is read, so a CSE budget timeout (2) and a genuine failure (1) both reported as "could not be enabled by systemctl", sending the on-call after systemd when the cause was provisioning running out of time. Captured and distinguished, matching what localdns_giveup_reason already does for the start loop. ShellSpec covers the new path; mutation-tested against the old form. 4. The 100-attempt count named a ceiling the loop cannot reach -- check_cse_timeout reaches its limit first in any realistic run. Dropped the ceiling language from the give-up reason and the per-attempt message rather than substituting another invented number, and said so where the constant is declared. 5. Hoisted daemon-reload above the loop. Nothing rewrites a unit between iterations, and it re-parses every unit on the box. Fair catch that the same cost argument was applied to the helper's per-attempt dumps but not its reload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nify the inventory Four review items from @yewmsft. 1. The Bastion size comment named a mechanism I had not isolated, and got it wrong twice. Ye pointed out SSH pads each binary packet to a multiple of the cipher block size, so a 1-byte script change moves the wire write by 0 or 16 -- never by the 5 the comment claimed. Swept the range again and pasted the rows: 12236..12242 -> 8180 12243..12248 -> 8196 <- first over 8,192 A clean 16-byte step, and no script size lands on 8,192 at all. That also retires max_write = size - 4096 + 45, which is off by 11 at the plateau and implies a granularity the transport does not have. The constant 12242 was right; only the explanation beside it was wrong. Data now, not a story -- measured rows survive the next person doing arithmetic at 2am. 2. The inventory guard closed the gap one step short. 'registered' and the size test's 'scripts' were two hand-maintained lists with nothing tying them together, so declaring a script and adding it to one but not the other left both tests green and the script unchecked -- the same failure the guard exists to prevent, moved up a level. My own error message asked for two edits and verified one. localdnsScriptsUnderTest is now the single inventory both read, so registering and size-checking are one edit. Mutation-tested with exactly that scenario. 3. The scanned file list was the inventory restated a third time, and missed validate_localdns_exporter_metrics.go. Replaced with filepath.Glob("*.go") -- go test runs in the package dir, so new scenario files are covered on arrival. Recorded the gap it still cannot close: the regex keys on the localdns*Script naming convention, so a script sent under another name stays unguarded. 4. Rewrapped the spliced comment in cse_config_localdns.sh and split the check_cse_timeout argument, which was being made twice in four lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@yewmsft asked how many of the 100 attempts the loop actually gets. Measured against a Type=notify unit with this loop's exact shape (reset-failed, timeout 30 systemctl restart, sleep 5): failure shape per iteration iterations in 780s fails at once (pre-flight) 5s ~156 fails after ~8s (resolv.conf drain) 13s ~60 hangs, capped by 'timeout 30' 35s ~22 Cost per iteration is dominated by how long the start takes to fail, because 'systemctl restart' on a Type=notify unit blocks until ready-or-failed. Only the fastest shape reaches 100; every slower one hits the CSE deadline first, and that is before anything else in CSE consumes the budget. His ~60 estimate was right for the middle case. The point stands either way: the count is a backstop and check_cse_timeout is what ends the loop, so the comment now carries the measured range instead of asserting that without a number. Method note for anyone repeating this: a Type=simple stub reports ~5s/iteration for every shape, because systemd considers such a unit started as soon as exec succeeds and 'systemctl restart' returns before the failure happens. The shapes only separate under Type=notify, which is what localdns.service uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….conf Per @yewmsft. The directive has to live in a unit-specific drop-in -- Azure Linux's type-wide /usr/lib/systemd/system/service.d/10-timeout-abort.conf overrides localdns.service, so setting it in the unit is silently ignored. Previously it rode along in localdns-delegate.conf because that file already had the packer plumbing. That file is named for Delegate=cpu, and whoever comes back asking why the stop timeout behaves this way will not think to open a file about cgroup delegation. Worse, the PR text left a rename of delegate.conf open as a possible follow-up; a rename that quietly dropped the directive would hand Azure Linux back to a 153s cycle, above the 144s threshold, with nothing but the e2e assertion to catch it. So: localdns-timeout-terminate.conf, installed as /etc/systemd/system/localdns.service.d/99-timeout-terminate.conf. delegate.conf returns to its single purpose. The 99- prefix is for human readers, not for correctness. Unit-specific drop-ins beat type-wide ones because they are separate hierarchies and systemd applies the unit-specific one last, regardless of filename -- verified by giving the type-wide file a 99- name so it sorted after delegate.conf, which still lost. Recorded in the artifact so nobody depends on ordering that is not load-bearing. Verified with the real artifacts against a type-wide abort drop-in: TimeoutStopFailureMode=terminate Delegate=yes / DelegateControllers=cpu TimeoutStartUSec=1min 30s TimeoutStopUSec=30s RestartUSec=2s StartLimitIntervalUSec=12min StartLimitBurst=5 Two separate .conf files in one drop-in directory both apply, and systemctl cat lists both with their paths, so the directive is discoverable on the node as well as in the tree. Plumbing: nine packer JSONs, packer_source.sh, azlosguard.yml, the VHD content test and CODEOWNERS. Checked parity mechanically -- every file that ships localdns-delegate.conf now also ships localdns-timeout-terminate.conf, and all nine JSONs still parse. That check matters because a missed JSON would not fail the build; it would ship one image without the drop-in and surface months later as one distro back at 153s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fore the lifecycle Two from @yewmsft. 1. azlosguard.yml lost delegate.conf's mode. The new localdns-timeout-terminate entry went in between the existing entry's 'destination:' and 'permissions:' lines, so it took the 644 with it. Parsed, delegate.conf became the only localdns entry in the file with no permissions key -- every sibling has one -- so on the OS Guard image its mode would be whatever imagecustomizer defaults to, while linux-vhd-content-test.sh still asserts 644 for that exact path. My parity check could not see this. "Every file shipping localdns-delegate.conf also ships localdns-timeout-terminate.conf" is a presence check, and what moved was an attribute between two entries. Re-checked by parsing the YAML and asserting no entry anywhere in the file lacks a permissions key; none does now. 2. assertLocalDNSBudgetDirectives was gated behind validateLocalDNSLifecycle. scenario_localdns_hosts.go returns on any lifecycle error, and the directive check did not run until validateLocalDNSRestartBudget, so a broken lifecycle step hid whether the unit was pinned at all. That is not hypothetical: gate build 181818040's AzureLinuxV3 lane died in the lifecycle validation on the Bastion tunnel limit, and the directive assertion -- the one thing standing between a dropped pin and a 153s cycle on AzureLinux -- never ran on the one distro where it mattered. It is also the cheapest check in the package: one 'systemctl show', no fault injection, no harness, gated behind a step that SCPs a script and drives kill/recovery cycles. Hoisted ahead of the lifecycle validation, with the laneResolvedMainBuiltImage() skip moved with it so main-built images are still skipped rather than failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Baking StartLimitIntervalSec=720/StartLimitBurst=5 into localdns.service makes a new image incompatible with an older CSE -- a combination this repo explicitly supports (cse_config_localdns.sh's LOCALDNS_COREFILE_BASE fallback). Raised by Copilot on #discussion_r4041413471; reproduced rather than reasoned about. The budget is only survivable alongside the 'systemctl reset-failed' in enableLocalDNS()'s retry loop, because provisioning restarts draw on the same budget as systemd's automatic ones. An older CSE goes through systemctlEnableAndStart -> _systemctl_retry_svc_operation, which has no reset. So one failed burst wedges the unit for 720s while that loop runs only ~500-600s: every remaining attempt refused, node provisioning failed, from a transient the old image would have recovered from in seconds. The rule lived in the image and the means to survive it lived in the script, and those ship separately. Moving the rule into the script means whoever brings the rule also brings the reset, so they cannot arrive apart. An older CSE creates no drop-in and gets systemd's 10s default -- today's shipping behaviour. Measured on a clean Ubuntu 24.04.4 / systemd 255.4 VM (the build AKS ships), 14 provisioning attempts against a service that always fails, R = refused: budget in the VHD unit + old CSE 12min/5 ..RRRRRRRRRRRR wedged, never recovers budget in the VHD unit + new CSE 12min/5 .............. reset clears it budget from CSE + old CSE 10s/5 ..R.R.R.R..... default, recovers budget from CSE + new CSE 12min/5 .............. protected Two costs I asserted and then measured, both of which turned out not to be costs: the budget's terminal behaviour is identical from either location (ActiveState=failed, NRestarts=5, effective 12min/5), and the drop-in survives a real VM reboot with the budget still in force. The cost that remains is not measurable: the 144s threshold is StartLimitIntervalSec/StartLimitBurst while the worst cycle is TimeoutStartSec + TimeoutStopSec + RestartSec, so after this the numerator lives in CSE and the denominator in the image. Two values that must agree, edited in different places -- the same shape as the bug that produced 153s on AzureLinux. Both files carry the matrix and a pointer to the other so the pair is discoverable from either side. RestartSec, TimeoutStartSec and TimeoutStopFailureMode stay in the unit: none of them can wedge provisioning on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-on to b9f1887, and a correction to 08ef097. TimeoutStartSec and TimeoutStopFailureMode exist only to serve the budget: the threshold is StartLimitIntervalSec/StartLimitBurst = 144s and it has to clear the worst restart cycle, TimeoutStartSec + TimeoutStopSec + RestartSec. Leaving them in the VHD while the budget moved to CSE split that arithmetic across two components, and left old-image/new-CSE getting the budget with UNPINNED timeouts -- 45 + 60 + 2 = 107s on AzureLinux, under 144 only because two distro quirks cancel, which is the coincidence this PR exists to remove. Both now travel with the budget in one CSE-written drop-in. That also deletes localdns-timeout-terminate.conf and its 16 files of packer plumbing: TimeoutStopFailureMode needs a unit-specific drop-in, and a CSE-written one is unit-specific, so the packer artifact was solving a delivery problem that no longer exists. The 99- prefix is load-bearing, and I previously recorded the opposite. systemd applies drop-ins in lexicographic order by FILENAME across both the unit-specific (localdns.service.d/) and type-wide (service.d/) directories -- being unit-specific does not by itself win. Measured on Ubuntu 24.04.4 / systemd 255.4 against Azure Linux's type-wide 10-timeout-abort.conf, same file and directory, only the name varying: 10-localdns-budget.conf -> abort (sorts before 10-timeout-abort, loses) 99-localdns-budget.conf -> terminate (sorts after, wins) zz-localdns-budget.conf -> terminate 08ef097 claimed the prefix was "for human readers, not for correctness" on the strength of two tests that were both confounded: delegate.conf vs a type-wide 10-, then a type-wide 99- vs delegate.conf. In each the unit-specific file happened to sort last (d > 1, d > 9), so they showed "later filename wins" and I read "unit-specific wins". @yewmsft had it right in his original comment; his later retraction, which I argued him into, was wrong. Four-cell matrix on that VM with the type-wide abort drop-in present, 12 provisioning attempts, R = refused: old image + old CSE 10s/5 abort .R.R.R.R.R.R systemd default, recovers old image + new CSE 12min/5 terminate ............ new image + old CSE 10s/5 abort ............ new image + new CSE 12min/5 terminate ............ No cell wedges, and both skew cells are correct by construction rather than by two distro quirks cancelling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…get moved The Ubuntu2404 lane failed on 264c9b2's VHD: FAIL: hung start did not terminate in 'failed' within 180s (state=activating) Not a regression in the budget -- the shortened clocks stopped applying. CSE now writes the budget to /etc/systemd/system/localdns.service.d/99-localdns-budget.conf, which pins TimeoutStartSec=90. The harness drop-in was 99-e2e-fastclock.conf, and systemd applies drop-ins in lexicographic filename order across every drop-in directory, so "99-e2e" sorts before "99-localdns" and the budget's 90s won. Hung start then ran a ~97s cycle instead of ~22s, needing ~390s to exhaust the burst against a 180s deadline. Measured on Ubuntu 24.04.4 / systemd 255.4 with both drop-ins present, varying only the harness filename: 99-e2e-fastclock.conf -> TimeoutStartUSec=1min 30s (budget wins, clocks inert) zz-e2e-fastclock.conf -> TimeoutStartUSec=15s (clocks win) load order confirms it: /run/systemd/system/<unit>.d/99-e2e-fastclock.conf /etc/systemd/system/<unit>.d/99-localdns-budget.conf <- last Renamed to zz-e2e-fastclock.conf with the reason recorded at the declaration. This is the same ordering rule that makes the budget's own 99- prefix load-bearing; moving the budget into a drop-in put the harness on the wrong side of it. AzureLinuxV3 passed on the same VHD (8m53s), which is the lane that proves the CSE-written drop-in beats Azure Linux's type-wide 10-timeout-abort.conf. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts b9f1887's CSE delivery mechanism. The budget was moved into a CSE-written drop-in to keep it from wedging an older CSE's 100-iteration retry loop -- but that loop adds nothing to defend. localdns.sh already absorbs ~80s of transient per start attempt (wait_for_localdns_removed_ from_resolv_conf 5, START_LOCALDNS_TIMEOUT=10, wait_for_localdns_ready 60 60), iptables -w blocks on the xtables lock rather than failing, and every remaining fatal exit is deterministic. Attempt 6 re-runs expired timers against a failure that cannot change. So a VHD-baked budget costs an old CSE a fail at ~10s with a readable journal instead of ~584s and 95 "Start request repeated too quickly" lines. Same node outcome, cheaper, better diagnosis. With the budget back in the unit the guarantee is unconditional -- it no longer depends on which CSE provisioned the node -- and RestartSec=2 ships alongside it rather than landing alone on nodes where it would push failures further from 'failed'. Also drops the TimeoutStartSec/TimeoutStopFailureMode pins: unpinned, the worst cycle is 122s on Ubuntu and 107s on Azure Linux, both under the 144s threshold. The 153s that motivated the stop-mode pin was created by pinning TimeoutStartSec to 90 on a distro that defaults to 45. e2e keeps two assertions: that the budget ships in localdns.service rather than a drop-in, and that the unit reaches terminal failed with Result=start-limit-hit. The fault matrix, drop-in ordering cases and CSE-interaction cases tested code that no longer exists. The localdns.sh SIGTERM/watchdog fix and the hosts cleanup hardening are unchanged. The Bastion script-size guard moves to its own PR with the tunnelSession.Write chunking fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The restart-budget E2E currently starts an already-active service, so its fault override never runs, and another lifecycle assertion can falsely pass on permission errors.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 7
Open (7)
Resolved since last review (1)
Comment-only change to a file this PR has no business touching. The Delegate=cpu rationale and the missing trailing newline are both worth fixing, just not here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The restart-budget E2E test does not activate its fault injection and can silently fail to restore LocalDNS.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (8)
Measured on a live AzureLinuxV3 node: systemd does not report Result=start-limit-hit on this path. It reports the underlying cause -- exit-code for all four fast fault modes -- while the journal carries "Start request repeated too quickly." The assertion as written would have failed every run. Assert ActiveState plus the journal refusal line plus NRestarts >= burst instead, and print Result as diagnostic only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
AgentBaker Linux gate detectiveRun: https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=182353627 Detective summary: E2E reported 184 scenarios with 4 failed and 0 flaky. LocalDNS restart-budget validation injected a failing Likely cause / signature: e2e-localdns-restart-budget-hungstart-not-bounded; secondary recurring signature vhd-ubuntu2204fips-cis-cron-rules-regression. Confidence: High for deterministic LocalDNS restart-budget validation regression; high for recurring CIS drift. Recommended owner/action: PR #9439 owner should inspect LocalDNS restart-budget behavior across AzureLinuxV3/Ubuntu2204/Ubuntu2404; VHD validation owners should continue repair #39802117 for the separate CIS drift. Strongest alternative: shared systemd timing flake; less likely because three LocalDNSHostsPlugin variants fail the same injected-fault assertion and PR #9439 directly changes LocalDNS restart-budget/service behavior. Evidence: E2E log reports |
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The E2E fault injection is not activated, and cleanup and privileged-path checks can silently produce invalid results.
Review effort: Balanced
Findings: 1
Open (8)
Comment on lines
+1233
to
+1237
| # It also means the dummy interface carrying 169.254.10.10/.11 is now torn down on a stop. | ||
| # It was not before: localdns_cleanup_mode (the ExecStopPost path) deliberately leaves the link | ||
| # alone in case an orphaned CoreDNS is still answering on .11. Note this for the pod-DNS | ||
| # fallback (#9486): its idempotent-interface-creation requirement is now the common case on a | ||
| # clean stop, not the exception. |
Member
There was a problem hiding this comment.
[nit] comment change
two things in this paragraph, both comment-only.
:1234-1235 says ExecStopPost leaves the link alone, but not why that doesn't apply here — so it reads like the two paths are accidentally inconsistent, and the next person "fixes" it by making ExecStopPost tear down too. the hazard it guards is an orphaned coredns still answering on .11 after the supervisor died unexpectedly. that can't happen on this path: by the time exit 0 reaches the EXIT handler, the script has already drained for LOCALDNS_SHUTDOWN_DELAY and SIGINTed coredns.
:1235-1237 isn't right for #9486. the ABRT ERR INT PIPE trap at :1216 already calls cleanup_localdns_configs, so the interface was already torn down on every failing start — which is the path the fallback actually inherits. idempotent interface creation was always required; this only adds the clean-stop case. scoping #9486 off "now the common case, not the exception" would be scoping it off a false premise.
Suggested change
| # It also means the dummy interface carrying 169.254.10.10/.11 is now torn down on a stop. | |
| # It was not before: localdns_cleanup_mode (the ExecStopPost path) deliberately leaves the link | |
| # alone in case an orphaned CoreDNS is still answering on .11. Note this for the pod-DNS | |
| # fallback (#9486): its idempotent-interface-creation requirement is now the common case on a | |
| # clean stop, not the exception. | |
| # It also means the dummy interface carrying 169.254.10.10/.11 is now torn down on a stop. | |
| # ExecStopPost (localdns_cleanup_mode, :847) deliberately does not delete it: that path runs | |
| # when the supervisor died unexpectedly and an orphaned CoreDNS may still be answering on .11. | |
| # That hazard cannot exist here -- by the time 'exit 0' reaches the EXIT handler the script has | |
| # already drained for LOCALDNS_SHUTDOWN_DELAY and SIGINTed CoreDNS. The two paths differ because | |
| # their hazards differ; do not align them. | |
| # | |
| # Not new for #9486: the ABRT/ERR/INT/PIPE trap above already calls cleanup_localdns_configs, so | |
| # the interface was already torn down on every failing start, which is the path the fallback | |
| # inherits. Idempotent interface creation was always required; this only adds the clean stop. |
same edit in the description, the .10/.11 bullet.
Two defects, both caught by yewmsft against build 182353627 where all three gated lanes failed deterministically with "localdns is 'active' after 120s of failing starts". validateLocalDNSLifecycle runs immediately before this validator and can only exit with the unit active -- its EXIT trap retries 'systemctl start' until is-active. reset-failed clears failure state but stops nothing, and daemon-reload does not restart anything, so 'systemctl start' on an active unit was a no-op: /bin/false never executed and the loop watched a healthy service for 120s. 'restart' is the fix. The journal grep used a '-5 min' wall-clock window, which reaches back over that same lifecycle validator's three kill/recovery cycles and can pass on a refusal that is not this test's. Anchor it to fault_start instead, the way validateLocalDNSLifecycle already anchors its own. Validated by yewmsft on the same VHD (182353627), one scenario, no rebuild: PR head failed in 7m18s; head plus these two lines passed in 5m34s. NRestarts=5 against burst=5, so the existing '-ge burst' bound holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Contributor
|
Contributor
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The E2E assertion miscounts start-limit attempts and will fail when the limiter works correctly.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Resolved since last review (8)
Comment on lines
+174
to
+176
| burst=$(systemctl show localdns.service -p StartLimitBurst --value) | ||
| [ "$nrestarts" -ge "$burst" ] || | ||
| fail "localdns restarted $nrestarts times against a burst of $burst; the limiter did not bound this failure." |
Member
|
both fixes land. and i was wrong about the refused sixth start does bump the counter — systemd increments when it schedules the restart job, then the limiter refuses that start. so it lands exactly on 5, not the 4 i predicted. the bound is right, leave it. two things that follow from it being exactly 5:
one for a work item, not this PR: the threshold arithmetic in the unit comment — 144s against the 122s slowest cycle — is the load-bearing claim for the budget, and |
Ye Wang (yewmsft)
approved these changes
Sep 23, 2026
Ye Wang (yewmsft)
left a comment
Ye Wang (yewmsft)
left a comment
Member
There was a problem hiding this comment.
approving. 6ffca34f fixes the gate failure and the gate proves it — 182475673, 184 scenarios, 114 passed, 0 failed, all five LocalDNSHostsPlugin lanes green including the three that were red on 182353627. no resolved a main-built image in the log, and the durations are the passing shape (2404 5m25s, not the 7m18s failure).
that also settles the two things i left open:
- 2204 is systemd 249 and
:175had never executed on any lane before this run. it passed, soNRestarts -ge burstholds on 249 as well as the 255 i measured. - Copilot's
NRestarts=4is wrong — the refused start bumps the counter, systemd increments when it schedules the restart job. three lanes agree with the bench.
the localdns.sh:1237 nit is still open and i'm not holding the PR for it. it's comment-only, but :1233-1235 scopes #9486 off a premise that isn't true — the ABRT ERR INT PIPE trap at :1216 already tore the interface down on every failing start, so idempotent interface creation was always required and this only adds the clean-stop case. worth fixing before merge or folding into #9486, your call.
Agentbaker GPU E2E red is unrelated — GPU pod readiness over the 1m warning threshold across nine lanes, no localdns involvement.
lilypan26
approved these changes
Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
localdns.servicegets a bounded restart budget, so unrecoverable failures terminate infailedinstead of restarting forever:failedis the prerequisite for handoff.OnFailure=only fires on entry tofailed, and NPD needs a stable terminal state to observe. Under systemd's default budget most failure modes never get there.Why these numbers
Threshold =
StartLimitIntervalSec / StartLimitBurst. A failure repeating more slowly than the threshold never accumulatesStartLimitBurststarts inside the window, so it restarts forever and never reachesfailed.systemd's default is
10s/5— a 2s threshold. Every enumerated failure mode cycles slower than that except the fastest pre-flight exit, so today they restart indefinitely:720/5 gives a 144s threshold, above the slowest restart cycle —
TimeoutStartSec + TimeoutStopSec + RestartSec, which is 122s on Ubuntu and 107s on Azure Linux. So every unrecoverable mode terminates. Burst stays at the systemd default; only the window changes.Slower flapping (>144s cycle) and serves-for-hours-then-crashes still restart, deliberately. Those served real traffic, and escalating a node that is up 98% of the time is NPD's call, not systemd's.
RestartSec=2is the load-bearing change.cleanup_iptables_and_dnsrunsnetworkctl reloadon every start, and the dummy interface carrying169.254.10.10/169.254.10.11is deleted and recreated on every start. At the 100ms default each retry re-enters the transient it is retrying against — five retries are effectively one. At 2s, networkd settles and an orphaned CoreDNS releases its sockets before the nextExecStart.Why the budget ships in the VHD unit
An earlier revision of this PR wrote the budget from CSE into a drop-in, to keep it from wedging an older CSE's 100-iteration retry loop. That loop turned out not to be worth defending.
localdns.shalready absorbs ~80s of transient inside a single start attempt —wait_for_localdns_removed_from_resolv_conf 5,START_LOCALDNS_TIMEOUT=10,wait_for_localdns_ready 60 60— andiptables -wblocks on the xtables lock rather than failing, so the classic provisioning transient never surfaces as a start failure at all. Every remaining fatal exit is deterministic: attempt 300 fails for exactly the reason attempt 1 did. There is no failure shape in that script that repetition converts into a success.So what a VHD-baked budget costs an older CSE is failing at ~10s with a readable journal instead of ~584s and 95
Start request repeated too quicklylines. Same node outcome, 574 seconds cheaper, better diagnosis.Keeping the budget in the unit also means the guarantee is unconditional — it does not depend on which CSE provisioned the node — and
RestartSec=2ships alongside it rather than landing alone on nodes where it would push failures further fromfailed.TimeoutStartSecandTimeoutStopFailureModeare deliberately not pinned. Unpinned, the worst cycle is 122s on Ubuntu and 107s on Azure Linux, both under the threshold. The 153s that motivated pinningTimeoutStopFailureMode=terminatewas itself created by pinningTimeoutStartSec=90on a distro that defaults to 45.Restart=on-failure,KillMode=mixedandTimeoutStopSec=30are already in the shipped unit and are unchanged; they appear above only because the threshold math depends on them.Stop-path change
SIGTERM was previously untrapped, so bash died on the default action: the EXIT cleanup never ran, and any in-flight watchdog
sleepwas left orphaned in the cgroup, holding the unit instop-sigtermuntil it finished orTimeoutStopSecexpired. This PR traps it, reaps the child, and exits 0 — a requested stop is not a failure, andRestart=on-failuremust not fire for it.That changes what a stop does, deliberately.
exit 0fires the EXIT trap, socleanup_localdns_configsnow runs in-process on every stop, where previously onlyExecStopPostran. Two consequences:LOCALDNS_SHUTDOWN_DELAYdraining connections before CoreDNS is SIGINTed. That is against thetimeout 30bound_systemctl_retry_svc_operationputs on a restart, leaving ~22–24s of headroom. It does not affect the budget arithmetic: the worst cycle is a hung start, where the trap cannot run at all, because bash defers a trapped signal until the foreground command returns..10/.11is now torn down on a clean stop. TheExecStopPostpath deliberately leaves it alone, in case an orphaned CoreDNS is still answering on.11. Worth noting for the pod-DNS fallback (feat: LocalDNS pod-DNS (.11) fallback with OnFailure + probe triggers #9486): idempotent interface creation becomes the common case on a clean stop, not the exception.localdns_spec.shasserts the SIGTERM/watchdog behaviour.Scope / what this is not
wait_for_localdns_removed_from_resolv_confis what made the loop self-sustaining, and with that gone the reported chain does not form. What this PR does is put a deterministic floor under failures we have not enumerated, so they terminate infailed.169.254.10.11answering for already-running pods while localdns is down is the stacked follow-up feat: LocalDNS pod-DNS (.11) fallback with OnFailure + probe triggers #9486, which reads these720/5values as its trigger prerequisite.tunnelSession.Writechunking fix.One note for later: once the fallback lands,
burst × C_maxbecomes the pod-DNS outage ceiling (~10.5min worst case here), so these numbers gain a second consumer and should not be treated as settled forever.Testing
e2e asserts two things, in
scenario_localdns_restart_budget.go:localdns.serviceitself rather than a drop-in.systemctl showreads effective values and cannot tell those apart, so the unit file is checked directly — the budget living in the VHD unit is what makes the guarantee unconditional.failedwithResult=start-limit-hitonce the burst is spent. A directive can be present and still bound nothing.Relationship to the stack