Skip to content

feat(scriptless): compare AKSNodeConfig generated cse cmd with NBC cse cmd#8416

Merged
lilypan26 merged 10 commits intomainfrom
lily/scriptless/phase-3
Apr 29, 2026
Merged

feat(scriptless): compare AKSNodeConfig generated cse cmd with NBC cse cmd#8416
lilypan26 merged 10 commits intomainfrom
lily/scriptless/phase-3

Conversation

@lilypan26
Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a diagnostic capability to compare env vars derived from an AKSNodeConfig-generated CSE command vs an NBC “phase 2” CSE command, intended to help validate scriptless provisioning parity.

Changes:

  • Add compareEnvs to diff env vars between --provision-config and --nbc-cmd, plus a parser for env assignments from an NBC one-liner.
  • Adjust Provision selection so NBCCmd execution is skipped when --provision-config is also provided (while still running the comparison).
  • Add unit tests for env parsing and env comparison via captured slog output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
aks-node-controller/app.go Introduces env diffing/parsing utilities and changes Provision flow when both flags are present.
aks-node-controller/app_test.go Adds a custom slog handler to capture logs and tests for env parsing + env comparison behavior.
Comments suppressed due to low confidence (1)

aks-node-controller/app.go:427

  • When both flags are provided, compareEnvs calls buildCmdFromProvisionConfig, and then Provision calls buildCmdFromProvisionConfig again to actually execute. This duplicates JSON parsing and CSE cmd construction. Consider refactoring so the cmd/env derived during comparison is reused (or compareEnvs accepts the already-built exec.Cmd/env map) to avoid the extra work in diagnostic mode.
	// If both flags are provided, compare environments before proceeding.
	if flags.ProvisionConfig != "" && flags.NBCCmd != "" {
		compareEnvs(ctx, flags, a.getNodeCustomDataPath())
	}

	var cmd *exec.Cmd
	if flags.ProvisionConfig != "" {
		var err error
		cmd, err = buildCmdFromProvisionConfig(ctx, flags.ProvisionConfig)
		if err != nil {

Comment thread aks-node-controller/app.go
Comment thread aks-node-controller/app.go Outdated
Comment thread aks-node-controller/app.go
Comment thread aks-node-controller/app.go
Comment thread aks-node-controller/app.go Outdated
}

var cmd *exec.Cmd
if flags.ProvisionConfig != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to change teh ordering here. We always prefer nbccsecmd first and then use provisionconfig if nbccsecmd is not present

Comment thread aks-node-controller/app.go Outdated

// If both flags are provided, compare environments before proceeding.
if flags.ProvisionConfig != "" && flags.NBCCmd != "" {
compareEnvs(ctx, flags, a.getNodeCustomDataPath())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might need some kind of error checking/block so any exception or errors here are just gobbled up (so we dont fail provisioning)

Comment thread aks-node-controller/app.go Outdated
Copilot AI review requested due to automatic review settings April 29, 2026 00:29
@lilypan26 lilypan26 review requested due to automatic review settings April 29, 2026 00:29
Comment thread aks-node-controller/app.go Outdated
Copilot AI review requested due to automatic review settings April 29, 2026 18:37
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

Comment thread aks-node-controller/app.go
Comment thread aks-node-controller/app.go Outdated
Comment thread aks-node-controller/app.go
Comment thread aks-node-controller/app.go Outdated
Comment thread aks-node-controller/app_test.go
Copilot AI review requested due to automatic review settings April 29, 2026 21:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment on lines +309 to +313
eventLogger.LogEvent("CompareEnvs", "env vars match between provision-config and nbc-cmd", helpers.EventLevelInformational, now, now)
} else {
message := fmt.Sprintf("env var differences (%d): %s", len(diffs), strings.Join(diffs, "; "))
slog.Info(message)
eventLogger.LogEvent("CompareEnvs", message, helpers.EventLevelInformational, now, now)
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compareEnvs calls eventLogger.LogEvent unconditionally. If eventLogger is nil (or becomes nil in future refactors), this panics and relies on the defer/recover to keep provisioning moving, which also logs an error-level "compareEnvs panicked" message. Prefer an explicit nil check (skip event emission when nil) so panics are reserved for truly unexpected failures.

Suggested change
eventLogger.LogEvent("CompareEnvs", "env vars match between provision-config and nbc-cmd", helpers.EventLevelInformational, now, now)
} else {
message := fmt.Sprintf("env var differences (%d): %s", len(diffs), strings.Join(diffs, "; "))
slog.Info(message)
eventLogger.LogEvent("CompareEnvs", message, helpers.EventLevelInformational, now, now)
if eventLogger != nil {
eventLogger.LogEvent("CompareEnvs", "env vars match between provision-config and nbc-cmd", helpers.EventLevelInformational, now, now)
}
} else {
message := fmt.Sprintf("env var differences (%d): %s", len(diffs), strings.Join(diffs, "; "))
slog.Info(message)
if eventLogger != nil {
eventLogger.LogEvent("CompareEnvs", message, helpers.EventLevelInformational, now, now)
}

Copilot uses AI. Check for mistakes.
Comment thread aks-node-controller/app.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread aks-node-controller/app.go
@lilypan26 lilypan26 merged commit 4828603 into main Apr 29, 2026
35 of 48 checks passed
@lilypan26 lilypan26 deleted the lily/scriptless/phase-3 branch April 29, 2026 22:11
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.

5 participants