Skip to content

Fence ADX Function waiter readiness and retries to the intended release - #265

Open
Kevin Cho (chokevin) wants to merge 3 commits into
mainfrom
chokevin-adx-waiter-safety
Open

Kevin Cho (chokevin) wants to merge 3 commits into
mainfrom
chokevin-adx-waiter-safety

Conversation

@chokevin

@chokevin Kevin Cho (chokevin) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Harden the Terraform ADX Function installation mitigation for v0.4.2 in both PowerShell and Bash. Previously, each engine accepted foreign-only Success or eight-of-nine expected Functions without GpuHealth, and deleted a foreign throttled Function even when all required owned Functions were healthy.

  • Derive required namespace/name identities from local helm template using the exact chart, release, namespace, and ordered values used by every upgrade. Honor disabled items, fullname overrides, and resource namespaces rather than hardcoding nine or inspecting live Helm release storage. --reset-values prevents the earlier controller-bootstrap functions.enabled=false override from leaking into the Function installation.
  • Share fail-closed identity, ownership, and current-generation status validation between engines. Require every rendered Function; check managed-by/instance labels and Helm release-name/release-namespace annotations before upgrades and while polling. Foreign status cannot satisfy missing requirements, block readiness, or become a retry target. Reject ownership conflicts and malformed, partial, or duplicate sources without adoption.
  • Retain bounded throttling-only compensation and immediate actionable invalid-KQL/permanent failures. Raw Kubernetes deletes carry both UID and resourceVersion preconditions for verified required/owned objects. Conflict/NotFound re-observes within the six-attempt budget, including ownership preflight before another upgrade; no name-only fallback.
  • Wire explicit caller identity and chart/policy replacement hashes, document prerequisites and operational boundaries, and extend both existing test runners with shared real-chart-derived regressions.

Related issue

Related to #162; release gate #261.

This does not satisfy #162's upstream retry-without-deletion acceptance and must not close it automatically.

Validation

Final head: a1169f4caf1ddde37e7a986b6cae8eebbbf328ab, based on main cdcd1df87d16ce8e554baf8b3f6ce79ba064e058.

Exact-head GitHub CI: all 35 checks completed: 32 successful, 3 expected release-only skips, no failed or pending checks. Maintainer review remains CHANGES_REQUESTED / BLOCKED pending re-review. The one review thread has been answered and resolved; resolution is not approval. No merge or live acceptance is claimed.

Successful Terraform run, attempt 2 verifies Terraform 1.9.0 formatting/configuration validation, four mocked plan cases, existing lifecycle-recorder regressions, and both waiter suites. Chart validation also passed under Helm 3.18.6 and 4.2.0.

/bin/bash terraform/aks/test-wait-for-adx-functions-ready.sh
  58 cases, 0 failures (including local stock macOS Bash 3.2.57)
pwsh -NoProfile -File terraform/aks/test-wait-for-adx-functions-ready.ps1
  58 cases, 0 failures (local PowerShell 7.5.1 and Ubuntu CI)

Both runners also execute three real-kubectl / isolated-loopback-API transport cases, verifying exact DeleteOptions transmission and Conflict/exit-1 handling for recreated UIDs and updated resourceVersions. Server responses are mocked; this is not live Kubernetes recovery evidence.

Before changing production scripts, the existing runner entry points reproduced all six unsafe outcomes (three per engine); the current-Success control passed. All deletes were offline mocks. This is not a claim that all 22 previously qualified adverse cases failed.

Cases assert exit results, diagnostics, read/upgrade/delete counts, exact installation arguments, and conditional-delete targets/bodies. Coverage includes missing/foreign sets, ownership conflicts, stale/absent generation/status, malformed/partial sources, duplicate identity, custom release/resource namespaces and fullname, feature disablement, explicit empty-set intent versus failed rendering, permanent/transient/exhausted throttles, subprocess errors, and same-name recreation/ownership/resourceVersion changes.

Maintainer feedback addressed

The reviewer identified that dns_name incorrectly applied the 63-character namespace-label limit to each segment of a Function name. The correction uses Kubernetes DNS-1123 subdomain validation: 253 characters total with no 63-character segment cap. Namespace dns_label limits remain unchanged.

Six shared regressions cover the real chart's 66-character dashboard Function, an unrelated 253-character throttled Function in preflight and polling, a dotted name with long segments, and rejection of 254-character names, empty segments, and 64-character namespaces. Valid long-name cases failed first in both engines and now pass. The failing-first run also exposed a Bash 3.2 test-runner exit-status issue; an explicit failure exit now prevents passing transport tests from masking failed scenarios. The original review thread contains the correction and evidence.

CI failure history

At f228ce94, the PowerShell harness combined setup-helm and preinstalled Helm application paths into one executable path. That exact failure was reproduced locally and fixed in 0b955914 by selecting the first PATH match. CI explicitly adds a second Helm symlink to retain regression coverage. No unchanged-job rerun was used to mask that code defect.

At the current head, the first Terraform attempt passed all 58 Bash scenarios but the unchanged loopback transport test's initial kubectl subprocess hit its 10-second timeout. The transport test then passed three unchanged local repeats. Only the failed Terraform job was retried once; attempt 2 passed both 58-case suites and all transport cases without any assertion, timeout, or source change. Successful jobs were not rerun. The original runner timeout's precise cause was not established; this targeted retry is recorded rather than presented as a code fix.

Local syntax, diff formatting, and pre-commit license checks passed. Independent Claude Opus 4.8 review covered the full implementation, the earlier CI correction, and this focused DNS-name correction without significant/actionable findings. These are advisory AI reviews, not human approval. Windows execution of the waiters and live installation/upgrade/fault-recovery acceptance are not claimed.

Compatibility and operational impact

  • PowerShell requires 7.3+ native argument passing; the Bash Function waiter supports stock macOS Bash 3.2 and later.
  • Both engines require jq 1.6+ and Mike Farah yq v4 on PATH. Tests additionally require Python 3. Public dependency provenance/licenses were reviewed (yq MIT; jq COPYING including bundled permissive notices); no third-party implementation is vendored.
  • Terraform retains release/namespace adx-mon. Standalone scripts accept explicit release/namespace; resource namespaces follow rendered manifests. Intentional Function-free installation requires explicit opt-out, and failed/empty chart rendering still fails.
  • Chart/policy changes re-trigger existing installation resources. Ownership remains Helm-managed; no state address/migration changes are introduced.
  • Direct Helm does not run the mitigation. Upstream controller recovery without deletion remains unresolved (adx-mon: retry Function reconciliation after transient ADX throttling #162), and Function Success with skipvalidation does not establish schema readiness or lossless startup history (adx-mon ManagementCommand does not report a completion condition #190).
  • No live Azure/Kubernetes/Function/identity/permission mutation, state/backend operation or real plan, deployment, merge, publication, provider fork, or upstream modification was performed. Existing Flex/demo health does not qualify this exact candidate. Live acceptance needs separately named approval.

AI assistance: GitHub Copilot implemented and validated the patch and feedback corrections; independent Claude Opus 4.8 agents provided advisory reviews. Human re-review and release acceptance remain outstanding.

Checklist

  • The change is focused and does not include unrelated edits.
  • Tests cover the changed behavior, or I explained why tests are not needed.
  • Documentation and examples are updated when public behavior changes.
  • Generated files are updated from their source and included in this pull request. (No generated application assets changed.)
  • New dependencies are publicly available and their license and provenance were reviewed.
  • The diff contains no secrets, private endpoints, customer data, or Microsoft-internal information.
  • Fork-based CI can validate the change without repository secrets, or maintainer-only validation is clearly identified.

Comment thread terraform/aks/adx-function-state.jq Outdated

@feiskyer Pengfei Ni (feiskyer) 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.

LGTM

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.

2 participants