Repository navigation
feat: flag to restrict to phase3 provisioning only - #9746
Conversation
Windows Unit Test Results 3 files 18 suites 1m 8s ⏱️ Results for commit edbdb0b. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
Unsupported Windows and PreProvisionOnly inputs currently succeed without Phase 3 semantics.
2 open findings
What changed in this PR
Adds an opt-in Phase 3 provisioning path that delivers only AKSNodeConfig to ANC.
Changes:
- Adds Phase 3 selection, payload validation, and delivery.
- Adds AKSNodeConfig normalization and ANC validation.
- Extends unit tests, E2E scenarios, validators, and documentation.
| File | Description |
|---|---|
pkg/agent/datamodel/types.go |
Defines the Phase 3 flag contract. |
pkg/agent/bakerapi_test.go |
Tests API validation and artifact generation. |
pkg/agent/baker.go |
Implements Phase 3 payload generation. |
pkg/agent/baker_test.go |
Tests delivery modes and size fallback. |
e2e/scenario/vmss.go |
Integrates Phase 3 into VMSS creation. |
e2e/scenario/validators.go |
Adds Phase 3 and comparison validation. |
e2e/scenario/validation.go |
Registers the comparison validator. |
e2e/scenario/types.go |
Clarifies AKSNodeConfig mutation behavior. |
e2e/scenario/scenario.go |
Enables Phase 3 in selected scenarios. |
e2e/scenario/provision.go |
Converts and validates Phase 3 configuration. |
e2e/scenario/node_config.go |
Adjusts NBC-to-AKSNodeConfig conversion. |
e2e/README.md |
Documents E2E Phase 3 coverage. |
aks-node-controller/README.md |
Documents Phase 3 and normalization. |
aks-node-controller/pkg/nodeconfigutils/validation.go |
Adds configuration normalization. |
aks-node-controller/pkg/nodeconfigutils/validation_test.go |
Tests normalization and validation. |
aks-node-controller/app.go |
Validates configs before ANC rendering. |
aks-node-controller/app_test.go |
Tests ANC rejection and normalization. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func validateScriptlessAKSNodeConfig(config *datamodel.NodeBootstrappingConfiguration) error { | ||
| input := strings.TrimSpace(config.AKSNodeConfigJSON) | ||
| if input == "" { | ||
| return fmt.Errorf("EnableScriptlessAKSNodeConfig requires non-empty AKSNodeConfigJSON") | ||
| } | ||
| if !json.Valid([]byte(input)) || input[0] != '{' { | ||
| return fmt.Errorf("EnableScriptlessAKSNodeConfig requires AKSNodeConfigJSON to be a valid JSON object") | ||
| } | ||
| return nil | ||
| } |
| // EnableScriptlessAKSNodeConfig enables Phase 3 provisioning using only AKSNodeConfigJSON. | ||
| // It takes precedence over EnableScriptlessNBCCSECmd and EnableScriptlessCSECmd: | ||
| // no NBC command is rendered or delivered, and ANC does not enter comparison mode. | ||
| // Requires a non-empty serialized JSON object. Windows and PreProvisionOnly are unsupported. | ||
| EnableScriptlessAKSNodeConfig bool |
90d60bc to
51cb32a
Compare
| if version.GreaterThanEqual(semver.MustParse("1.34.0")) { | ||
| delete(flags, "--streaming-connection-idle-timeout") |
| // EnableScriptlessAKSNodeConfig enables Phase 3 provisioning using only AKSNodeConfigJSON. | ||
| // It takes precedence over EnableScriptlessNBCCSECmd and EnableScriptlessCSECmd: | ||
| // no NBC command is rendered or delivered, and ANC does not enter comparison mode. | ||
| // Requires a non-empty serialized JSON object. Windows and PreProvisionOnly are unsupported. |
AgentBaker Linux gate detectiveRun: 185078699 Detective summary: Run AgentBaker E2E failed only Likely cause / signature: e2e-localdns-coldstart-empty-hosts-start-timeout - localdns cold-start with empty hosts file timed out before the service came online. Confidence: High for the signature: failed test result, focused E2E log, timeline, and build report isolate one LocalDNS cold-start failure after prior validation steps passed. Recommended owner/action: LocalDNS/E2E owner should inspect localdns startup readiness after empty-hosts cold start and whether the phase3 provisioning PR changes affect DNS setup ordering. Strongest alternative: Shared node/network settling latency is plausible because teardown/startup is timing-sensitive, but the failure is in the deterministic LocalDNS cold-start validation path. Evidence: build run, failed timeline/test records, focused task log, build report, PR metadata, and wiki tracker. |
| // It takes precedence over EnableScriptlessNBCCSECmd and EnableScriptlessCSECmd: | ||
| // no NBC command is rendered or delivered, and ANC does not enter comparison mode. | ||
| // Requires a non-empty serialized JSON object. Windows and PreProvisionOnly are unsupported. | ||
| EnableScriptlessAKSNodeConfig bool |
There was a problem hiding this comment.
nit: EnableAKSNodeConfig seems cleaner?
There was a problem hiding this comment.
Reason for choosing EnableScriptless as prefix everywhere was for easy search in aks-rp and other reason is that to keep it similar to previous feature names.
EnableScriptlessCSECmd && EnableScriptlessNBCCSECmd
| // ValidateAndNormalizeConfiguration validates and updates provisioning settings in place. | ||
| // Producers call it before serialization; ANC calls it before rendering. It is idempotent | ||
| // and does not depend on the producer's or node's environment. | ||
| func ValidateAndNormalizeConfiguration(cfg *aksnodeconfigv1.Configuration) error { |
There was a problem hiding this comment.
Why do we need this method?
Ideally, I don't want consumers to update reference to ANC library often.
There was a problem hiding this comment.
What the purpose of it? To fail node provision early?
51cb32a to
ded00d8
Compare
AgentBaker Linux gate detectiveRun: 185399239 Detective summary: Run AgentBaker E2E failed 25 scenarios. The first failures were fast node-config validation errors ( Likely cause / signature: e2e-phase3-provision-config-nbc-cmd-env-diff - phase3 provisioning/node-config validation regression. Confidence: High that this is the failing signature: failed test records, the E2E task log, failed timeline, and PR metadata all point to the same phase3 provisioning/node-config path; medium-high on PR causality because the changed files directly cover ANC/node config and E2E provisioning. Recommended owner/action: PR owner should reconcile the phase3-only provisioning path so required node-config fields are populated and provision-config/nbc-cmd env validation stays in sync, then rerun the gate after updating the PR. Strongest alternative: Shared E2E environment drift, but unrelated scenarios passed in the same run and the failure shape is a broad, consistent validation mismatch in code paths touched by the PR rather than random infra timeouts. Evidence: build run, failed test run 686945796, E2E task log 624, failed timeline records for e2e/Run AgentBaker E2E, PR files touching �ks-node-controller, �2e/scenario, and pkg/agent, and wiki tracker. |
ded00d8 to
edbdb0b
Compare
|
|
|
AgentBaker Linux gate detectiveRun: 185430989 Detective summary: One E2E scenario failed during LocalDNS conntrack rule validation. The validator printed all LocalDNS raw-table NOTRACK rules and counted Likely cause / signature: e2e-localdns-notrack-rule-regex-missing - LocalDNS NOTRACK exact-rule regex validation reports a rule missing even while the expected rules are present in output. Confidence: High for the signature: failed test result, focused E2E log, timeline, and prior matching wiki row all isolate the same conntrack validation family; medium on root cause because the log suggests exact matcher/ordering sensitivity, but the validator still exits nonzero. Recommended owner/action: LocalDNS/test owner should review the NOTRACK exact-rule validator against iptables output ordering/format and confirm whether the PREROUTING Strongest alternative: PR phase3 provisioning changes altered node setup, but the failing check is LocalDNS raw-table validation and matches a prior LocalDNS-specific signature while PR 9746 is not primarily conntrack-focused. Evidence: build run, failed test run 687293785, E2E task log 624, failed E2E timeline records, PR metadata, prior matching row, and wiki tracker. |
Picks up #9746, #9759 and #9740, which landed while this branch was being updated. One conflict, in enableScriptlessCompilation (e2e/scenario/vmss.go). #9746 broadened the gate from usesScriptlessNBCCSECmd to usesScriptlessProvisioning so it also covers scriptless AKSNodeConfig scenarios; this branch had restructured the same expression into early returns to add the ForceScriptlessCompilation override the LocalDNS LPS scenarios need. Resolved by keeping both: main's broader predicate inside this branch's early-return form, so the force override still short-circuits the CustomDataWriteFiles check. usesScriptlessNBCCSECmd remains in use by validators.go, so nothing is left dangling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Create a new flag to swtich to phase3 only provisioning so we can get into RP before freeze