Skip to content

feat: support AIManager managed VHDs - #9350

Open
zhehli688 wants to merge 1 commit into
mainfrom
aimanager
Open

feat: support AIManager managed VHDs#9350
zhehli688 wants to merge 1 commit into
mainfrom
aimanager

Conversation

@zhehli688

@zhehli688 zhehli688 commented Aug 31, 2026

Copy link
Copy Markdown

What this PR does / why we need it:
This PR added manifests for creating AIManager managed VHDs, which patches additional contents to existing AKS VHDs:

  • Added a CUE schema for AIManager managed VHD manifests and a CI check for schema validation.
  • Added model manifests for DeepSeek V4 Flash on H100 and GPT-OSS 120B on A100.

Which issue(s) this PR fixes:

Fixes #

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Windows Unit Test Results

  3 files   13 suites   47s ⏱️
406 tests 406 ✅ 0 💤 0 ❌
409 runs  409 ✅ 0 💤 0 ❌

Results for commit e83dbab.

♻️ This comment has been updated with latest results.

@zhehli688

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Microsoft"

Copilot AI left a comment

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.

🔵 Needs a closer look

The manifests depend on an external capture pipeline whose consumption behavior cannot be verified within this repository.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@yewmsft Ye Wang [msft] (yewmsft) 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.

re-checked at eace7ec7 — both nits from the last pass are in, and the cue install is better than what i asked for. pinned v0.15.4 tarball with a sha256sum -c check beats "use the release tarball": the build is now reproducible and a compromised release asset fails the job instead of silently installing. permissions: {} + job-scoped contents: read, the SHA-pinned checkout and persist-credentials: false are all the right calls too.

cue vet now runs first in the loop, so a schema violation reports as a schema violation. that was the point.

nothing blocking from me. one optional symmetry nit inline.

Comment thread .github/workflows/validate-aimanager-manifests.yml
Copilot AI review requested due to automatic review settings September 2, 2026 01:12

Copilot AI left a comment

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.

🟡 Changes recommended

The repository-key bootstrap artifact lacks required integrity metadata and downstream digest verification.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread schemas/aimanager.cue Outdated
Copilot AI review requested due to automatic review settings September 2, 2026 01:43

Copilot AI left a comment

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.

🟡 Changes recommended

The schema permits apt-specific dependency metadata for Azure Linux variants that require RPM/DNF metadata.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread schemas/aimanager.cue Outdated
sku: "2404gen2containerd"
}
} | {
osSku: "AzureLinux3.0"

@yewmsft Ye Wang [msft] (yewmsft) 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.

re-checked at 4a473c0d. the dup-dependency nit landed, and the keyring digest is a real fix rather than a paper one — i pulled cue v0.15.4 and ran it rather than trusting the shape: a dep with repoKeyringUrl and no repoKeyringSha256 fails cue vet -c with repoKeyringSha256: field is required but not present, and the reverse fails the same way. the if x != _|_ pair does enforce both directions. good.

three small things below, all in the "you just added a check, here's the one sitting next to it" bucket. none of them blocking.

Comment thread .github/workflows/validate-aimanager-manifests.yml
Comment thread schemas/aimanager.cue Outdated
Comment thread schemas/aimanager.cue
Copilot AI review requested due to automatic review settings September 2, 2026 03:36

Copilot AI left a comment

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.

🔵 Needs a closer look

The managed VHD manifests and validation pipeline require final human review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 2, 2026 03:42

Copilot AI left a comment

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.

🔵 Needs a closer look

The managed VHD manifests and validation workflow require final human review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@yewmsft Ye Wang [msft] (yewmsft) 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.

5352806d — all three from the last pass are in, and i re-ran cue v0.15.4 against the new schema instead of eyeballing the diff:

  • bakePath collision — your new jq catches the cuda-toolkit-12-9 / cudnn-9 same-path case i pasted.
  • https on the keyring urlhttp://... is now rejected. i also checked that the bare repoKeyringUrl!: string inside the line-38 guard doesn't quietly drop the regex; it unifies, so the constraint holds either way in.
  • _comment on a variant — passes now.

two things below. one is the paths: filter you added, which nobody asked for and which collides head-on with #9367. the other is me walking back half of my own _comment suggestion.

name: Validate AIManager Manifests
on:
pull_request:
paths:

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.

i'd pull this back out, because of #9367.

a paths:-filtered workflow doesn't report a skipped check on non-matching PRs — it reports nothing at all. checked it here rather than going from memory: actionlint is filtered to .github/workflows/**, and on #9368 and #9364 (neither touches that path) there is no actionlint check-run on the head sha at all — 39 and 43 check-runs respectively, none of them actionlint.

so the moment this becomes required per #9367, every PR that doesn't touch the schema, the manifests dir, or this file sits at "Expected — Waiting for status to be reported" and can't merge. that's the whole repo, blocked by a workflow that only ever needed to look at three paths.

the filter list itself is correct — workflow + schema + manifests dir is exactly what the job reads. it's the interaction with the required-check plan that's the problem. either:

  • drop the trigger back to on: pull_request — it was fine, it's a checkout, a 20MB tarball and some jq; or
  • keep the filter and add the companion always-succeeds job with the same name, per handling skipped but required checks.

first one's simpler and this job is small. if you'd rather keep the filter, worth saying so in #9367 so whoever flips the branch-protection switch knows to add the companion first.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good to know. Path filters removed.

Comment thread schemas/aimanager.cue
}
}

#Variant: {

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.

half-retraction on my last comment here: adding "_comment"? fixed the symptom i showed you, but the phantom osSku line isn't about _comment at all — it's the disjunction, and it fires on any failure inside a variant. your own new https rule demonstrates it. a dependency with http:// gives:

variants.0: 2 errors in empty disjunction:
variants.0.osSku: conflicting values "AzureLinux3.0" and "Ubuntu2404"
variants.0.dependencies.0.repoKeyringUrl: invalid value "http://..." (out of bound =~"^https://")

the osSku line is still noise, and it's still the one printed first.

a lookup keyed on osSku keeps the pairing and drops the disjunction:

#BaseImage: {
	Ubuntu2404:      {offer: "AKSUbuntu",     sku: "2404gen2containerd"}
	"AzureLinux3.0": {offer: "azure-linux-3", sku: "V3gen2"}
}

#Variant: {
	gpuSku:       "A100" | "H100"
	osSku:        "Ubuntu2404" | "AzureLinux3.0"
	"_comment"?:  string
	dependencies: [...#Dependency]
	baseImage:    #BaseImage[osSku]
}

i ran this before suggesting it: both real manifests still pass, an AzureLinux baseImage on a Ubuntu2404 variant still fails — now as baseImage.offer: conflicting values "AKSUbuntu" and "azure-linux-3", which points at the actual mistake — and the http:// case above reports its one real error and nothing else.

nit, take it or leave it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. Thanks for flagging this!

Copilot AI review requested due to automatic review settings September 2, 2026 05:03

@yewmsft Ye Wang [msft] (yewmsft) 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.

both of these landed, and i re-ran cue v0.15.4 against the new schema rather than eyeballing the diff:

  • on: pull_request is back — that's the #9367 problem gone. thanks.
  • #BaseImage[osSku] does what i claimed it would. a variant with osSku: "AzureLinux3.0" carrying the Ubuntu baseImage now reports variants.0.baseImage.offer: conflicting values "AKSUbuntu" and "azure-linux-3" and nothing else — the phantom osSku: conflicting values line is gone. and a bogus osSku: "Ubuntu2204" now reports variants.0.osSku, which is the truthful location. both real manifests still vet clean.

i also re-ran the same fixtures for the ^https:// regex, the two mutual-requirement guards, and the three jq dup checks — all still behave. nothing left on the schema or the workflow from me.

one new thing rode in with this commit — CODEOWNERS, inline below. it doesn't do what it looks like it does.

Comment thread CODEOWNERS Outdated
staging/cse/windows/ @timmy-wright @r2k1 @benjamin-brady @djsly @awesomenix @cameronmeissner

# Code owners for AIManager managed VHD manifest schema and release notes
schemas/aimanager.cue @Fei-Guo @zhehli688

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.

Fei Guo (@Fei-Guo) can't be a code owner here. the API reports their permission on Azure/AgentBaker as read:

$ gh api repos/Azure/AgentBaker/collaborators/Fei-Guo/permission --jq .permission
read

GitHub silently drops CODEOWNERS entries for anyone without write access — no error, no warning, no failed check, the rule just quietly loses that owner. so all three rules in this block resolve to zhehli688 alone, and GitHub never auto-requests review from the PR author.

net effect for a PR touching only these paths: the * rule at line 1 (the whole node team) gets overridden — last matching rule wins — and replaced with nobody. that's strictly worse than not adding the block at all.

two ways out:

  • get Fei-Guo write access on the repo first. that fixes the silent-drop, but it's still 1 effective owner for anything they author, so i'd want a third name on it.
  • or follow what this file already does for shared paths — pkg/agent/testdata/ (line 12) and vhdbuilder/release-notes/security-patch/ (line 16) both append their new owners to the catch-all list instead of replacing it.

either way, please confirm against the effective owner list rather than the file — this is easy to get wrong and silent when you do.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Gotcha, I followed the pattern in vhdbuilder/release-notes/security-patch/.

Comment thread CODEOWNERS Outdated

Copilot AI left a comment

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.

🔵 Needs a closer look

The managed-VHD manifests, schema, and CI workflow require final human review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

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.

🔵 Needs a closer look

The image manifests and validation workflow require final human review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@yewmsft Ye Wang [msft] (yewmsft) 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.

both done, and i checked the owner lists as sets rather than by eye:

  • schemas/aimanager.cue and vhdbuilder/release-notes/AKSAIManager/ are now exactly the 25 names from the * rule at line 1, plus zhehli688. nothing dropped, nothing invented.
  • the .github/workflows/validate-aimanager-manifests.yml rule is gone, so the workflow stays on the catch-all. that was the other half.

worth knowing: you did better than the two lines i pointed you at. pkg/agent/testdata/ (line 12) is 6 names behind the catch-all today and vhdbuilder/release-notes/security-patch/ (line 16) is 10 behind — sulixu, Runzhen (@runzhen), Karen Chen (@karenychen), Xu Xue (@xuexu6666), Thibault Cohen (@titilambert), Ben Brady (@benjamin-brady) are missing from both. so the pattern is real but it drifts, and yours will too the next time someone joins the node team. nothing to do about it in this PR — just don't treat the copy as self-maintaining.

one question inline. otherwise this is done from my side: schema, workflow trigger and codeowners all check out, and i've re-run cue against the manifests at every head since 01:00.

Comment thread CODEOWNERS Outdated
staging/cse/windows/ @timmy-wright @r2k1 @benjamin-brady @djsly @awesomenix @cameronmeissner

# Code owners for AIManager managed VHD manifest schema and release notes
schemas/aimanager.cue @zhehli688 @cameronmeissner @ganeshkumarashok @Devinwong @lilypan26 @djsly @phealy @r2k1 @timmy-wright @zachary-bailey @awesomenix @mxj220 @pdamianov-dev @calvin197 @sulixu @SriHarsha001 @runzhen @karenychen @xuexu6666 @titilambert @benjamin-brady @fcher @sinmentis @aboodasfari @janenotjung-hue @abigailliang-aks-sig-node

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's intended. I will add Fei and other AIManager team members to the owner list in a separate PR.

@yewmsft Ye Wang [msft] (yewmsft) Sep 2, 2026

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.

forget about the ai review. You should add kaito team and my team as code owners, and whoever responsible releasing these code.

Comment thread CODEOWNERS Outdated
staging/cse/windows/ @timmy-wright @r2k1 @benjamin-brady @djsly @awesomenix @cameronmeissner

# Code owners for AIManager managed VHD manifest schema and release notes
schemas/aimanager.cue @zhehli688 @cameronmeissner @ganeshkumarashok @Devinwong @lilypan26 @djsly @phealy @r2k1 @timmy-wright @zachary-bailey @awesomenix @mxj220 @pdamianov-dev @calvin197 @sulixu @SriHarsha001 @runzhen @karenychen @xuexu6666 @titilambert @benjamin-brady @fcher @sinmentis @aboodasfari @janenotjung-hue @abigailliang-aks-sig-node

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's intended. I will add Fei and other AIManager team members to the owner list in a separate PR.

Copilot AI left a comment

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.

🔵 Needs a closer look

The manifests drive an external VHD capture pipeline whose artifact installation behavior is not exercised by repository CI.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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.

3 participants