fix: bound LocalDNS restart budget so failures terminate deterministically - #9439
Saewon Kwak (saewoni) wants to merge 20 commits into
Conversation
Windows Unit Test Results 3 files 17 suites 1m 3s ⏱️ Results for commit be8e8ff. ♻️ This comment has been updated with latest results. |
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
21e8b5e to
ea38772
Compare
Two fixes from Copilot review: - Teardown reused the validator context, so a cancelled scenario -- the path where cleanup matters most -- would return immediately and leave the fault file, the patched localdns.sh and the systemd drop-in on the node for whatever ran next. Derive a bounded context with context.WithoutCancel so teardown still runs, without hanging forever. - The provisioning-restart check bounded its restart with timeout 60 while the production path uses timeout 30 (cse_config_localdns.sh). A restart taking 30-60s would have passed the test while failing node provisioning, so the check was weaker than the thing it guards. Match production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new VHD restart limits can cause provisioning failures when paired with an older CSE payload.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| StartLimitIntervalSec=720 | ||
| StartLimitBurst=5 |
Pre-existing e2e bug:
|
There was a problem hiding this comment.
🔵 Needs a closer look
The new VHD unit can break provisioning when paired with an older independently delivered CSE script.
Review details
Suppressed comments (3)
parts/linux/cloud-init/artifacts/localdns.sh:944
- 🟡 Medium Risk — 🧪 Test Coverage: The new background-sleep/
waitpath is not exercised by ShellSpec: the onlystart_localdns_watchdogexample inlocaldns_spec.sh:1340-1347leavesNOTIFY_SOCKETandWATCHDOG_USECempty, so it takes the other branch. Because this change relies on Bash signal,set -e, and EXIT-trap behavior to shorten shutdown, add a ShellSpec that enters the watchdog branch, interrupts the wait, and verifies prompt cleanup/exit.
# Wait for the next watchdog interval. Run sleep in a child so
# SIGTERM can interrupt the wait and let the service's signal/exit
# cleanup run promptly.
sleep "${HEALTH_CHECK_INTERVAL}" &
wait $!
parts/linux/cloud-init/artifacts/localdns.service:30
- 🔴 High Risk — 🔄 Backward Compatibility: These limits make the new VHD depend on the matching new
enableLocalDNS. An older CSE still usessystemctlEnableAndStart, which retries viadaemon-reloadbut never callsreset-failed(cse_helpers.sh:585-625). On systemd 255, the first failed start consumes this five-start budget and all remaining provisioning retries are refused for 720 seconds, potentially failing node provisioning. Sinceprovision_configs_localdns.shis supplied through CustomData while this unit is baked into the image, please make the unit safe with the previous CSE behavior or stage/gate the rollout with a compatibility mechanism.
StartLimitIntervalSec=720
StartLimitBurst=5
spec/parts/linux/cloud-init/artifacts/cse_config_localdns_spec.sh:115
- 🟡 Medium Risk — 🧪 Test Coverage: Despite the test name, the mock makes the first
restartsucceed, so this executes only one loop iteration and merely checks that each command appeared once. It would still pass ifreset-failedwere moved outside the loop, which is the exact regression this change is intended to prevent. Make the first restart fail and a later one succeed, then assert the ordered call sequence containsreset-failedbefore both restart attempts.
It 'should clear the StartLimit budget before each start attempt'
When run enableLocalDNS
The status should be success
The output should include "systemctl reset-failed localdns"
The output should include "systemctl restart localdns"
The output should include "Enable localdns succeeded."
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This assertion concludes "the drop-in is gone" from a failed glob, so a bare ls would read a permission failure as success and let the regression under test pass silently. /run/systemd/network/*.d is 0755 today -- verified on a node, the unprivileged ls does find the file -- so this is not a live bug, but the check should not depend on a permission it does not control. The same test's own service drop-in directory is 0750 precisely because sudo mkdir applied root's umask, which is how this class of mistake arises. Use sudo so the assertion sees what is actually there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The load-bearing watchdog signal behavior lacks focused ShellSpec regression coverage.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:944
- 🟡 Medium Risk — 🧪 Test Coverage: This load-bearing signal-handling change is not exercised by the existing
start_localdns_watchdogShellSpec (it only covers the no-watchdog branch), and the lifecycle E2E only requiressystemctl stopto finish eventually, so the previous foreground sleep would still pass. Please add a signal-focused ShellSpec that enters the configured watchdog branch, sends SIGTERM while waiting, and verifies prompt EXIT cleanup without waiting for the health-check interval; this also follows AGENTS.md:62-65.
# Wait for the next watchdog interval. Run sleep in a child so
# SIGTERM can interrupt the wait and let the service's signal/exit
# cleanup run promptly.
sleep "${HEALTH_CHECK_INTERVAL}" &
wait $!
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
… fits The full matrix could not complete. validateVM runs inside vmssCtx, bounded by TestTimeoutVMSS (17m default) and shared with VM creation, so the validation phase has roughly 11 minutes. The three slow modes need ~19.6min between them at their shipped clocks -- readytimeout 318s, watchdog 370s, hungstart 487s -- and the run died mid-watchdog at 1050s with "context deadline exceeded", which surfaces through the SSH call but is the VMSS deadline, not a per-exec one. Shorten the clocks for the fault run only: WatchdogSec 60 -> 10 and TimeoutStartSec 90 -> 15 via the existing fault drop-in, wait_for_localdns_ready 60/60 -> 8/8 and START_LOCALDNS_TIMEOUT 10 -> 3 via the patched script. The same code paths still run -- a real watchdog timeout, a real start timeout, the real readiness poll, the real pid-file wait -- just on a faster clock. Worst-case deadline sum drops from 1950s to 695s. StartLimitIntervalSec, StartLimitBurst and RestartSec are untouched; they are what is under test. The trade is explicit and noted in the file: this validates the mechanism, not the shipped wall-clock durations. Those were measured by hand on a live node and are recorded on the PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The CSE retry loop can overrun its reporting reserve, and validation gaps could allow restart-budget regressions.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
e2e/scenario/scenario_localdns_restart_budget.go:505
- 🟡 Medium Risk — 🔧 Script Logic: The comment and invariant require exactly
StartLimitBurstexecutions, but this one-sided check accepts any larger count. If the limiter permits extra starts and the unit later fails for another reason, the E2E can still pass despite the restart budget being broken. Require equality so both early termination and budget overshoot fail validation.
parts/linux/cloud-init/artifacts/cse_config_localdns.sh:139 - 🟡 Medium Risk — 🔧 Script Logic: This single budget check guards up to three sequential 30-second D-Bus calls. If the loop begins near the 780-second CSE threshold, those calls plus the final 30-second status collection can exceed the 900-second outer CSE timeout, so the process is still SIGKILLed before it records the intended LocalDNS status and exit code. Recheck the CSE budget between potentially blocking calls so at most one call can consume the 120-second reporting reserve.
parts/linux/cloud-init/artifacts/localdns.sh:944
- 🟡 Medium Risk — 🧪 Test Coverage: The configured-watchdog branch now relies on background-job and signal semantics, but
localdns_spec.shonly exercises the branch whereNOTIFY_SOCKETandWATCHDOG_USECare empty. Add a ShellSpec case that enters this branch, terminates the waiting shell, and verifies cleanup begins without waiting for the full health-check interval; otherwise the behavior this change is intended to guarantee can regress unnoticed.
# Wait for the next watchdog interval. Run sleep in a child so
# SIGTERM can interrupt the wait and let the service's signal/exit
# cleanup run promptly.
sleep "${HEALTH_CHECK_INTERVAL}" &
wait $!
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Shortening the fault clocks so the matrix fits the VMSS budget removed the one
thing the PR is actually about. The design claim is a margin:
threshold = StartLimitIntervalSec / StartLimitBurst = 144s
worst cycle = TimeoutStartSec + TimeoutStopSec + RestartSec = 122s
720/5 was chosen because 144s clears 122s. With TimeoutStartSec shortened to 15s
for the matrix, the hung-start cycle drops to ~20s -- far under the threshold --
so every mode would pass even if the threshold were mis-sized. The test would go
green while the design was broken.
Two checks restore it, both before any clock is shortened:
- Arithmetic on the live values. Verified to reject every realistic mis-sizing:
the shipped 10s/5 budget (threshold 2s), an interval of 100s (20s), burst
raised to 10 without raising the interval (72s), and TimeoutStartSec raised to
5min (worst cycle 332s). Costs nothing.
- A measured worst cycle. The arithmetic checks a model of the unit; this checks
the machine. Induce the hung start at shipped clocks, time two consecutive
ExecStarts, assert the cycle is under the threshold, then abort -- the
remaining starts only repeat the same cycle. ~100s rather than the ~487s a
full burst would cost.
The fault drop-in is split so the clock shortening now lands after the
measurement rather than with the ExecStart override.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The watchdog child can delay restarts until the stop timeout, and the full E2E matrix exceeds the scenario timeout.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.sh:944
- 🔴 High Risk — Script Logic: With
KillMode=mixed, systemd sends the initialSIGTERMonly to the main Bash process. This new backgroundsleeptherefore survives when Bash exits, keeps the service cgroup non-empty, and makes systemd wait untilTimeoutStopSec=30before killing it. A normal restart can consequently consume the full 30-second stop timeout and collide with the newtimeout 30 systemctl restart localdnsprovisioning path. Track this child and explicitly terminate/reap it from aTERMtrap (or use a kill mode that signals the whole cgroup) so the intended prompt shutdown actually occurs.
sleep "${HEALTH_CHECK_INTERVAL}" &
wait $!
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| // The full failure-mode matrix runs on Ubuntu2404 only: it is | ||
| // systemd 255, where daemon-reload does not clear the start | ||
| // limiter and where the provisioning regression was found. It | ||
| // costs ~23min, so the other distros run the single | ||
| // discriminating mode instead (~40s) -- enough to catch the |
…erly Three review items. 1. The backgrounded watchdog sleep was never reaped. SIGTERM was untrapped, so bash died on the default action: the EXIT cleanup never ran, and the sleep survived as an orphan in the unit's cgroup. With KillMode=mixed systemd signals only the main process, so it then held the unit in stop-sigterm until the sleep finished on its own (up to HEALTH_CHECK_INTERVAL) or TimeoutStopSec expired and it was SIGKILLed -- delaying the stop, and with it the next start, which is the opposite of what RestartSec=2 is for. Trap SIGTERM, kill and wait on the child via a named stop_watchdog_sleep, and exit 0 because a requested stop is not a failure and Restart=on-failure must not fire for it. 2. That path had no coverage. Added tests using real processes and real signals rather than mocks: a mocked kill would prove nothing about whether the child actually goes away. Verified by deleting the kill and watching the test fail with "child N survived stop_watchdog_sleep". The first version of that test was itself broken -- it checked WATCHDOG_SLEEP_PID after stop_watchdog_sleep had cleared it, so it was evaluating 'kill -0 ""' and passed even with the kill removed. It now saves the pid first. 3. The reset-failed spec only proved both calls appeared somewhere, so hoisting reset-failed out of the retry loop would still have passed. It now records an ordered trace and asserts RSRSRS across two failed restarts and a success, and asserts the give-up path ends on a start attempt rather than a reset, so the unit is left in 'failed' for NPD. Verified by hoisting the call and watching it fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new budget can break old-CSE/new-VHD provisioning, and the E2E validation has timeout and cleanup correctness issues.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
e2e/scenario/scenario_localdns_restart_budget.go:131
- 🟡 Medium Risk — 🏗️ E2E cleanup: Teardown is registered only after installation succeeds. The installer writes the replacement script and service drop-in before its final
daemon-reload; if that or another late command fails, this function returns here without removing the partial harness, leaving the node's LocalDNS configuration mutated. Register the idempotent teardown before calling the installer so partial-install failures are restored too.
e2e/scenario/scenario_localdns_restart_budget.go:642 - 🟡 Medium Risk — 🧪 E2E logic: The comment says both fewer and more than the burst are failures, but this condition only rejects fewer starts. If an unexpected reset or limiter regression permits more than five ExecStarts before the eventual refusal, the test passes even though the configured budget was not enforced as intended. Compare for exact equality here.
parts/linux/cloud-init/artifacts/localdns.service:30
- 🔴 High Risk — 🔄 Backward Compatibility: This 12-minute window only remains provisioning-safe when the matching new CSE calls
reset-failedbefore every attempt. The repository explicitly supports old CSE + new VHD (cse_config_localdns.sh:21-23), but the old CSE path is the removedsystemctlEnableAndStartcall, whose helper only doesdaemon-reload; as this PR itself notes, that does not clear the limiter on Ubuntu 24.04/systemd 255. A transient startup failure can therefore spend all five starts and make every old-CSE retry fail with start-limit-hit until its provisioning timeout. Please gate/install the extended budget through a compatibility handshake or otherwise preserve the old retry path on new VHDs.
StartLimitIntervalSec=720
StartLimitBurst=5
e2e/scenario/scenario_localdns_hosts.go:69
- 🟡 Medium Risk — 🧪 E2E reliability: This selects a matrix documented just above as taking about 23 minutes, but the scenario runs under the default 17-minute
TestTimeoutVMSScontext (e2e/config/config.go:125), which also includes VM creation (e2e/scenario/provision.go:248-261). The context can therefore expire before this validator completes, turning the Ubuntu 24.04 lane into a deterministic/likely timeout rather than useful coverage. Please reduce the runtime further or move this destructive matrix to a separately budgeted lifecycle.
if tt.name == "Ubuntu2404" {
faults = localdnsFaultMatrix
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
reset-failed in the loop addresses my earlier point, thanks. re-reviewed the rest: 1. TimeoutStartSec=90otherwise a distro default change, or someone adding 2. what clears 3.
4. worse, 5. nit: description lists |
…endent The 720/5 threshold (144s) is sized to clear the slowest restart cycle, TimeoutStartSec + TimeoutStopSec + RestartSec. Two of those three were pinned by the unit and the third was inherited from the manager default -- which is a systemd build-time constant (-Ddefault-timeout-sec), not a constant across the images we ship. Ubuntu builds it at 90s; Azure Linux 3.0 builds it at 45s. So the worst cycle, and therefore the margin under the threshold, was silently different per distro: 122s on Ubuntu, 77s on Azure Linux. Both clear 144s today, so this is not a live defect, but the margin the unit documents was never actually the unit's to guarantee, and a change to DefaultTimeoutStartSec on any image would move it with nothing to catch that. Pin it at 90 so the arithmetic in the comment above is self-contained. This also makes the e2e assertion honest. It checks TimeoutStartUSec="1min 30s", which passed on Ubuntu and failed on AzureLinuxV3 with "got 45s" -- correctly reporting a real difference, but one no image was ever going to satisfy uniformly while the value was inherited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er VHDs Two independent failures from the check-in gate on fb79777 (build 181710671). 1. hungstart timed out at its deadline with execstarts=3, sub=stop-sigterm. installLocalDNSFastClocks shortened TimeoutStartSec 90->15 but left TimeoutStopSec at 30, so each cycle is 15+30+2 = 47s rather than the ~22s the deadline was sized for. Five of those do not fit 120s; the run gave up mid-cycle three. The stop phase is consumed in full because of the TERM trap added in fb79777. The fault holds a foreground 'sleep infinity', and bash defers a trapped signal until the foreground child returns, so nothing answers the SIGTERM and systemd waits out TimeoutStopSec before the SIGKILL. Untrapped -- which is what the by-hand matrix on the PR measured -- bash died immediately and the stop phase cost ~6s, which is why that run recorded 487s for a mode that now costs ~625s shipped. Nothing in localdns.sh blocks that way in production: the watchdog uses 'sleep & wait', which the trap does interrupt. Shorten TimeoutStopSec to 5 alongside the other two clocks. Cycle returns to 22s, the mode terminates in ~110s, and the deadline goes to 180s for real slack. Nothing that reads TimeoutStopSec runs after this point -- the margin assertion and measureLocalDNSWorstCycle both run before the fast clocks install -- so no coverage is lost. 2. The directive assertion cannot pass on the non-gate e2e lane. localdns.service is baked into the VHD, so the budget only exists on an image built from a branch carrying it. The check-in gate builds this PR's images, but .pipelines/e2e.yaml also triggers on any PR touching e2e/** and resolves images by branch=refs/heads/main, which still ship the systemd default. Build 181710682 failed all three distros with "expected StartLimitIntervalUSec=12min, got 10s" -- an accurate reading of an image that was never meant to carry it. Probe StartLimitIntervalUSec up front and skip when it is absent, the way validateLocalDNSLifecycle already skips its dead-service block on images predating the ExecStopPost hook. The interval is the right probe because it is unambiguous provenance; TimeoutStartUSec now reads 1min 30s on new images and on older Ubuntu ones alike. Also carries the comment update matching the TimeoutStartSec pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The baked restart budget can break new-VHD/old-CSE provisioning compatibility on transient failures.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.service:31
- 🔴 High Risk — This budget depends on the new CSE's
reset-failedloop, but the unit is baked into a new VHD while an older CSE can still be supplied through CustomData. The repository explicitly supports new-VHD/old-CSE skew (localdns.sh:1025-1027), and the old path uses a 100×5ssystemctlEnableAndStartloop without resetting the limiter (cse_helpers.sh:623-625). On systemd 255, one transient burst can therefore lock every remaining old-CSE attempt out for 720s; those retries finish in roughly 500s and provisioning exits 216 even if the transient has cleared. Please make the unit's rollout compatible with the old retry path rather than relying solely on the new CSE behavior.
StartLimitIntervalSec=720
StartLimitBurst=5
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| return false, fmt.Errorf("read the LocalDNS start-limit interval: %w", err) | ||
| } | ||
| interval := strings.TrimSpace(result.stdout) | ||
| if interval != localdnsExpectedStartLimitInterval { |
There was a problem hiding this comment.
The provenance probe is keyed on a value that is itself under test, so the assertion it guards can no longer fail. check StartLimitIntervalUSec "12min" (:329) only ever runs on an image where :299 already matched the same constant — sharing localdnsExpectedStartLimitInterval between the two makes that structural, not incidental.
The consequence is bigger than one dead assertion: retune the budget later (720 → 600, or a burst change that moves the interval) and every lane skips, all three distros go green, and nothing anywhere says the restart budget stopped being tested. That is the exact failure mode this scenario exists to catch.
Same for the e2e.yaml lane you're working around — a PR that only touches e2e/** now has all three distros skip, i.e. a fully green localdns lane that validated none of this.
Gate on the lane rather than on the SUT. The test already knows which image it asked for — VHD_BUILD_ID vs SIG_VERSION_TAG_VALUE=refs/heads/main, per the message you removed at :389. Skip when the lane resolved a main-built image; assert unconditionally otherwise. Then the directives assertion is falsifiable again and a retune fails loudly on the PR's own VHD.
On the precedent you cite: scenario_localdns_hosts.go:217 probes ExecStopPost to decide whether to test ExecStopPost, so it has the same property. That argues for fixing both, not for keeping the shape.
There was a problem hiding this comment.
Gate landed the way I hoped — vhdCarriesLocalDNSRestartBudget is gone and laneResolvedMainBuiltImage() reads only config, nothing from the node:
return config.Config.SIGVersionTagName == "branch" &&
config.Config.SIGVersionTagValue == "refs/heads/main"Checked both sides resolve as intended: e2e/config/config.go:117-118 defaults to branch / refs/heads/main, so a local run against a main-built image skips; .pipelines/scripts/e2e_run.sh:85-86 exports SIG_VERSION_TAG_NAME=buildId and SIG_VERSION_TAG_VALUE=$VHD_BUILD_ID, so every pipeline lane fails the condition and asserts unconditionally. Retune the budget now and the PR's own VHD goes red instead of silently skipping — which was the whole ask.
Also glad you took the same gate to validateLocalDNSLifecycle (scenario_localdns_hosts.go:87) rather than leaving the precedent as-is.
| localdnsFastClockDropIn = "/run/systemd/system/localdns.service.d/99-e2e-fastclock.conf" | ||
| // StartLimitIntervalSec=720 as systemd formats it. Shared by the provenance probe and | ||
| // the directive assertion so the two cannot drift apart. | ||
| localdnsExpectedStartLimitInterval = "12min" |
There was a problem hiding this comment.
nit, but load-bearing now that this decides run-vs-skip rather than just pass-vs-fail: "12min" is systemd's rendered timespan, and this file already declines to trust that rendering — localdnsWorstCycleScript (:215) pipes the same property through systemd-analyze timespan before doing arithmetic on it.
Do the same here and compare 720000000 numerically. Otherwise a formatting difference on some image turns the whole scenario off silently instead of failing it, which is the one outcome this skip must not produce.
There was a problem hiding this comment.
nvm on this one — the numeric comparison mattered only because a rendering difference could flip the scenario off. Now that the skip is keyed on the lane instead of on the unit, check StartLimitIntervalUSec "12min" can only fail loudly. Leave it as the literal.
| // The full failure-mode matrix runs on Ubuntu2404 only: it is | ||
| // systemd 255, where daemon-reload does not clear the start | ||
| // limiter and where the provisioning regression was found. It | ||
| // costs ~23min, so the other distros run the single |
There was a problem hiding this comment.
~23min doesn't fit the budget it's being justified against: TestTimeoutVMSS is 17min (e2e/config/config.go:125), and validation shares it with VM creation. I assume the figure is stale — a shipped-clock measurement carried forward past the shortening — but as written it says the Ubuntu2404 lane cannot pass, and whoever sizes the next fault against it will get it wrong.
The real worst case is tight anyway. Deadlines now sum to 755s (45+80+120+90+120+120+180, after this commit's hungstart 120 → 180), and measureLocalDNSWorstCycle can spend its full seq 1 240 poll before failing — ~16.8min before a single second of VM creation. When it goes over, the VMSS timeout pre-empts the per-fault deadline and you lose the one diagnostic all this careful sizing exists to produce ("hung start did not terminate within 180s"); you get a generic scenario timeout instead.
Two cheap fixes: drop the measurement poll ceiling from 240s to ~180s (the cycle it waits for is 122s, and on the success path it breaks early anyway), and add a unit test asserting sum(deadlineSeconds) + measurement ceiling < TestTimeoutVMSS - <vm-creation allowance> so the next deadline bump fails at build time instead of as a flaky lane.
There was a problem hiding this comment.
The sizing model is right — "one healthy run plus one regression" is the correct worst case to pin, and putting it in a test so the next deadline bump fails at build time is exactly the shape I asked for.
What I'd still change: the margin is implicit, and it's thin. Adding up what TestLocalDNSFaultMatrixFitsVMSSBudget computes at head:
localdnsNonMatrixAllowance300slocaldnsWorstCycleCeilingSeconds180ssum(measuredSeconds)= 11+37+64+30+75+90+110 = 417smax(deadline - measured)= 70s (hungstart, 180-110)
= 967s against TestTimeoutVMSS = 17min = 1020s. 53 seconds of headroom, and the one input you flagged as uncontrolled — VM creation — is inside the 300s allowance, not on top of it. One slow VMSS create and the lane goes over, and the failure you get is the generic scenario timeout, not "hung start did not terminate within 180s".
Make the margin a named constant the assertion has to clear, e.g.
const localdnsBudgetMargin = 120 * time.Second
...
require.Less(t, worst+localdnsBudgetMargin, budget,
"fault matrix leaves less than %s of slack under TestTimeoutVMSS", localdnsBudgetMargin)Then a future deadline bump (or a re-measure that comes in higher) fails on the number that actually matters, and anyone tightening TestTimeoutVMSS finds out here rather than on a flaky lane. Sizing localdnsNonMatrixAllowance would also benefit from a one-line comment saying where 300s came from — measured, or budgeted?
The 720/5 threshold (144s) is sized against TimeoutStartSec + TimeoutStopSec + RestartSec. Pinning the start timeout at 90 (previous commit, per review) pushed AzureLinuxV3 to a measured 153s -- above the threshold, meaning the slow failure modes would restart forever there, which is the bug this budget exists to prevent. The stop side is doubled on that distro. Azure Linux ships a global drop-in, /usr/lib/systemd/system/service.d/10-timeout-abort.conf, setting TimeoutStopFailureMode=abort (Fedora's "Shorter Shutdown Timer"). On a stop timeout systemd sends SIGABRT to capture a core dump, then waits a SECOND TimeoutStopSec in 'stop-watchdog' before SIGKILL. From the node's journal: 18:51:49 start operation timed out. Terminating. <- SIGTERM 18:52:19 State 'stop-sigterm' timed out. Aborting. <- +30s, SIGABRT 18:52:49 State 'stop-watchdog' timed out. Killing. <- +30s, SIGKILL 18:52:50 ExecStopPost done (1s), Failed with result 'timeout' 18:52:52 restart 90 + 30 + 30 + 1 + 2 = 153s, against the measured 153s. Before the start timeout was pinned this did not show, because Azure Linux also builds systemd with -Ddefault-timeout-sec=45: 45 + 60 + 2 = 108s stayed under 144. The margin was holding on a coincidence -- a shorter start timeout cancelling a doubled stop timeout -- on one distro, undocumented either way. Pin 'terminate' (the systemd default) so the distro's drop-in is out of the cycle and every image is 90 + 30 + 2 = 122s. The core dump the abort step exists to produce is already discarded on AKS nodes: configureCoreDump() in cis.sh sets Storage=none and ProcessSizeMax=0 for every distro, asserted by testCoreDumpSettings in linux-vhd-content-test.sh. Today that step costs 30s of restart cycle and produces nothing. The unit comment records that dependency and says to revisit this if the hardening is ever relaxed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…der test Three review fixes on the restart-budget validation. 1. The provenance guard was self-defeating. It skipped unless StartLimitIntervalUSec was 12min -- the same value the directive assertion then checks -- so that assertion could only run where it was already guaranteed to pass, and sharing a constant between the two made it structural rather than incidental. Worse, a later retune (720 -> 600) would have skipped every lane, gone green on all distros, and said nothing about the budget no longer being tested. Gate on the lane instead. The check-in gate sets SIG_VERSION_TAG_NAME=buildId from VHD_BUILD_ID (.pipelines/scripts/e2e_run.sh) and tests this PR's VHDs; everything else falls back to branch=refs/heads/main, which resolves a main-built image that legitimately predates the budget. Skip only that case, and assert unconditionally otherwise, so the assertion is falsifiable again. validateLocalDNSLifecycle had the identical shape -- probing ExecStopPost to decide whether to test ExecStopPost -- so it now takes the same lane gate and asserts the hook is present rather than using its presence as permission to look. 2. Assert TimeoutStopFailureMode. The margin arithmetic models the stop side as one TimeoutStopSec and cannot see the abort state Azure Linux's global drop-in introduces, so without this it keeps reporting a comfortable 122s while the machine sits at 153s. That is exactly what happened. 3. Size the matrix against TestTimeoutVMSS at build time. Going over budget does not fail like a sizing error: the VMSS context deadline pre-empts the per-fault deadline, so instead of "hung start did not terminate within 180s" -- the diagnostic all this sizing exists to produce -- you get a generic scenario timeout. TestLocalDNSFaultMatrixFitsVMSSBudget now fails the build when it stops fitting. The model is a healthy run plus one regression (allowance + worst-cycle ceiling + sum(measured) + max(deadline - measured)), not every deadline firing at once: a mode only costs its deadline when broken, and the first broken mode aborts the run. Measured times move from comments into the matrix as data so the test can use them. The worst-cycle poll ceiling drops 240s -> 180s, just above the 122s cycle it waits for, and the stale "~23min" note on the Ubuntu2404 selection is corrected -- it described shipped clocks and was never true after the shortening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
|
There was a problem hiding this comment.
🔵 Needs a closer look
The VHD-baked restart limit can break transient-failure recovery when paired with an older CSE that lacks the new reset loop.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/localdns.service:32
- 🔴 High Risk — Backward Compatibility: This 12-minute window is safe only when provisioning has the new
reset-failedloop. The unit is baked into the VHD, whilecse_config_localdns.shis CSE-delivered (vhdbuilder/packer/packer_source.sh:422-429,pkg/agent/const.go:62-64), so an older provisioner can still use the previoussystemctlEnableAndStartloop. On systemd 255, once a transient startup issue consumes these five starts, every remaining old-CSE retry is refused even after the issue clears, causing node provisioning to fail. Please provide a VHD-side compatibility mechanism or defer activating the long budget until provisioning no longer depends on the old retry loop.
StartLimitIntervalSec=720
StartLimitBurst=5
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| # raised -- revisit this directive. The abort step would start producing a usable dump, and | ||
| # the 30s it costs would have to be bought back elsewhere in the cycle or the threshold | ||
| # widened, or Azure Linux goes back over 144s. | ||
| TimeoutStopFailureMode=terminate |
There was a problem hiding this comment.
This pin likely doesn't take on the distro it's aimed at, and your own new assertion should be the thing that catches it.
man 5 systemd.unit on drop-ins: "Drop-in files under any of these directories take precedence over unit files wherever located." So /usr/lib/systemd/system/service.d/10-timeout-abort.conf beats TimeoutStopFailureMode= set in localdns.service itself — including the copy we install at /etc/systemd/system/localdns.service. The effective value on AzureLinuxV3 stays abort, the stop side still doubles, and the cycle stays at the 153s this comment is written to avoid.
Worth confirming before merge, one command on an AzureLinuxV3 node:
systemctl show -p TimeoutStopFailureMode localdns.service
If it prints abort, then check TimeoutStopFailureMode "terminate" in scenario_localdns_restart_budget.go should be red on that lane right now — which would make this a good catch by the new assertion rather than a bug that ships.
The fix is cheap because we already ship a unit-specific drop-in for this unit: packer_source.sh:431-432 installs localdns-delegate.conf to /etc/systemd/system/localdns.service.d/delegate.conf. Drop-ins apply in lexicographic order by filename regardless of which directory they live in, so a unit-specific file whose name sorts after 10- wins — either move the directive into delegate.conf's [Service] section (delegate > 10-, so it already sorts last) or add 99-timeout-terminate.conf beside it. Keep the comment block where it is either way; it's the most useful part of this change.
TL;DR
Problem: systemd's default restart budget for
localdns.serviceis unsuitable in two opposite directions. Fast crash storms exhaust it in seconds and wedge the unit almost immediately; meanwhile several slower failure modes (PID file never appears, watchdog kill, ready-check timeout, hung start) restart on a cycle slower than the default threshold and would restart forever, never reachingfailed.Fix: replace the default budget with
StartLimitIntervalSec=720/StartLimitBurst=5andRestartSec=2. The threshold (720/5 = 144s) sits just above the slowest restart cycle (~125s), so every unrecoverable failure mode deterministically terminates infailed.RestartSec=2is the load-bearing change: it lets each restart sample real system state instead of re-entering the transient it is retrying against.Why this matters:
failedis the prerequisite for handoff.OnFailure=only fires on entry tofailed, so a pod-DNS.11fallback (stacked follow-up, see #9486) can only take over if failures reliably reach that state. This PR does not itself add the fallback — it puts a floor under un-enumerated failures so they land infailed, which is what a fallback and NPD can act on. See Whatfailedactually triggers — it is not a passive state.Design intent
We want the opposite of "retry forever to keep
.11alive." We want every unrecoverable path to land infailed, so that:OnFailure=can hand off to a.11fallback responder (stacked follow-up feat: LocalDNS pod-DNS (.11) fallback with OnFailure + probe triggers #9486), andThe default budget lets slow failures restart forever and never reach
failed— precisely the modes a fallback needs to fire on. The table below (threshold = interval/burst) shows why720/5is chosen: it is above the slowest restart cycle, so all modes terminate.Slower flapping (>144s cycle) or serves-for-hours-then-crashes intentionally still restarts — those served real traffic and shouldn't escalate a node that's up 98% of the time. That class is NPD's, and is why the NPD check should probe
.11rather than read unit state.Change
RestartSec=2(load-bearing).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 sub-second retries each attempt re-enters the transient it is retrying against (five 100 ms retries are effectively one). At 2s, networkd settles and an orphaned CoreDNS releases its sockets before the nextExecStart.StartLimitIntervalSec=720/StartLimitBurst=5. Threshold 144s, just above the slowest restart cycle (~125s = inheritedTimeoutStartSec90 +TimeoutStopSec30 +RestartSec2), so every unrecoverable mode reachesfailed. Burst returns to the systemd default; only the window changes.TimeoutStartSec=90andTimeoutStopFailureMode=terminate(both pinned so the threshold math is self-contained). The worst cycle isTimeoutStartSec + TimeoutStopSec + RestartSec, and two of those three were previously whatever the image happened to default to. Neither default is the same across the images we ship: Ubuntu builds systemd with-Ddefault-timeout-sec=90and Azure Linux 3.0 with45, and Azure Linux additionally ships a globalservice.ddrop-in settingTimeoutStopFailureMode=abort, which makes a stop timeout sendSIGABRTand then wait a secondTimeoutStopSecbeforeSIGKILL. Measured on AzureLinuxV3:90 + 30 + 30 + 2 = 153s, above the 144s threshold — the slow modes would have restarted forever there. Pinning both makes every image90 + 30 + 2 = 122s. Theabortstep's core dump is already discarded on AKS nodes (configureCoreDump()incis.shsetsStorage=none), so it cost 30s of restart cycle and produced nothing.Restart=on-failure,KillMode=mixedandTimeoutStopSec=30are already in the shipped unit and are unchanged by this PR; they appear above only because the budget math depends on them.What
failedactually triggersReaching
failedis not passive, and the description above previously implied it was. Verified inCloudNativeCompute:aks-vm-extension—config/node-problem-detector/plugin/check_dns_to_localdns.shruns every 1 minute and returnsNOTOKwhensystemctl is-active localdns.servicefails.config/node-problem-detector/custom-plugin-monitor/localdns-problem-monitor.jsonis"type": "temporary"with"conditions": []— so no node condition, but it incrementsproblem_counter{reason="LocalDNSError"}.aks-operator—config/metrics/profiles/default/alerting_rules.yml:forLocalDNSErrorRestartNodeincrease(problem_counter{reason="LocalDNSError"}[10m]) >= 5RestartNodeRemediationEarly,specify: Reboot,cordonanddrain: "true",dryRun: "false"LocalDNSErrorRedeployNodeRedeployNodeRemediationEarly,specify: RedeploySo a node that reaches
failedand stays there is cordoned, drained and rebooted at roughly 10 minutes, and redeployed at roughly 15. Reboot clears systemd's failed state, so recovery does exist — but it is node replacement, not service recovery, andreset-failedruns only inenableLocalDNS()at provisioning time. Nothing on the node itself clearsfailedafterwards.Two consequences worth stating plainly:
.11fallback (feat: LocalDNS pod-DNS (.11) fallback with OnFailure + probe triggers #9486) does not prevent it. The probe checkssystemctl is-activefirst and exits before reaching its own.11and.10queries, so a node whose pods are resolving perfectly through the fallback still reports a problem every minute and still gets rebooted, then redeployed. Ye Wang [msft] (@yewmsft) called this out on 2026-09-11 ("the NPD check should probe.11rather thansystemctl is-failed"); the code confirms it. A companion change inaks-vm-extensionis tracked as a TODO on feat: LocalDNS pod-DNS (.11) fallback with OnFailure + probe triggers #9486 and should land before or with it.Scope / what this is not
wait_for_localdns_removed_from_resolv_conf, which had made the loop self-sustaining). What this PR does is put a deterministic floor under failures we haven't 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).burst × C_maxbecomes the pod-DNS outage ceiling (~10.5min worst case here), so these numbers gain a second consumer and shouldn't be treated as settled forever.Relationship to the stack
Stacked on #9360; base branch is
fix/localdns-cgroup-teardown.