Fence ADX Function waiter readiness and retries to the intended release - #265
Open
Kevin Cho (chokevin) wants to merge 3 commits into
Open
Kevin Cho (chokevin) wants to merge 3 commits into
Kevin Cho (chokevin) wants to merge 3 commits into
Conversation
Open
49 tasks
Pengfei Ni (feiskyer)
requested changes
Sep 10, 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.
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.
helm templateusing 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-valuesprevents the earlier controller-bootstrapfunctions.enabled=falseoverride from leaking into the Function installation.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 maincdcd1df87d16ce8e554baf8b3f6ce79ba064e058.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.
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_nameincorrectly 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. Namespacedns_labellimits 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 in0b955914by 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
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.skipvalidationdoes not establish schema readiness or lossless startup history (adx-mon ManagementCommand does not report a completion condition #190).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