Conversation
Windows Unit Test Results 3 files 13 suites 47s ⏱️ Results for commit e83dbab. ♻️ This comment has been updated with latest results. |
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
🔵 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
Ye Wang [msft] (yewmsft)
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🟡 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
There was a problem hiding this comment.
🟡 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
| sku: "2404gen2containerd" | ||
| } | ||
| } | { | ||
| osSku: "AzureLinux3.0" |
Ye Wang [msft] (yewmsft)
left a comment
There was a problem hiding this comment.
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.
Ye Wang [msft] (yewmsft)
left a comment
There was a problem hiding this comment.
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-9same-path case i pasted. - https on the keyring url —
http://...is now rejected. i also checked that the barerepoKeyringUrl!: stringinside the line-38 guard doesn't quietly drop the regex; it unifies, so the constraint holds either way in. _commenton 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good to know. Path filters removed.
| } | ||
| } | ||
|
|
||
| #Variant: { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fixed. Thanks for flagging this!
Ye Wang [msft] (yewmsft)
left a comment
There was a problem hiding this comment.
both of these landed, and i re-ran cue v0.15.4 against the new schema rather than eyeballing the diff:
on: pull_requestis back — that's the #9367 problem gone. thanks.#BaseImage[osSku]does what i claimed it would. a variant withosSku: "AzureLinux3.0"carrying the UbuntubaseImagenow reportsvariants.0.baseImage.offer: conflicting values "AKSUbuntu" and "azure-linux-3"and nothing else — the phantomosSku: conflicting valuesline is gone. and a bogusosSku: "Ubuntu2204"now reportsvariants.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.
| 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 |
There was a problem hiding this comment.
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) andvhdbuilder/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.
There was a problem hiding this comment.
Gotcha, I followed the pattern in vhdbuilder/release-notes/security-patch/.
Ye Wang [msft] (yewmsft)
left a comment
There was a problem hiding this comment.
both done, and i checked the owner lists as sets rather than by eye:
schemas/aimanager.cueandvhdbuilder/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.ymlrule 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.
| 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 |
There was a problem hiding this comment.
It's intended. I will add Fei and other AIManager team members to the owner list in a separate PR.
There was a problem hiding this comment.
forget about the ai review. You should add kaito team and my team as code owners, and whoever responsible releasing these code.
| 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 |
There was a problem hiding this comment.
It's intended. I will add Fei and other AIManager team members to the owner list in a separate PR.
There was a problem hiding this comment.
🔵 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
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:
Which issue(s) this PR fixes:
Fixes #