From 1622881124cb1e3c3f1c22f4719e12163bfb226c Mon Sep 17 00:00:00 2001 From: Ganeshkumar Ashokavardhanan Date: Thu, 3 Sep 2026 13:25:26 -0700 Subject: [PATCH] fix(e2e): only tolerate VHD-sourced env skew when the VHD isn't built from the source under test ValidateScriptlessPhase3 tolerates diffs confined to GPU_DRIVER_VERSION and GPU_IMAGE_SHA, because provision-config reads them from the components.json baked into the VHD while nbc-cmd generates them from the source under test. That tolerance is only legitimate when the two sides genuinely come from different commits. The VHD builder pipelines run their e2e stage with VHD_BUILD_ID: $(Build.BuildId), so the VHD under test was built from the same commit and both sides must agree exactly -- there the tolerance would mask a real GPU env bug. Gate it on that existing signal: e2e_run.sh exports SIG_VERSION_TAG_NAME=buildId only when VHD_BUILD_ID is set, so config.VHDBuiltFromSourceUnderTest() distinguishes the two cases. The standalone e2e check keeps tolerating the skew and stays unblocked for driver bumps; VHD-build-triggered e2e is now strict again. Every non-VHD-sourced var still fails in both modes, so this keeps full phase 3 coverage on every PR rather than skipping the validator outright. --- e2e/config/config.go | 21 +++++++ e2e/validators.go | 44 +++++++++----- e2e/validators_envcompare_test.go | 95 ++++++++++++++++++++++++------- 3 files changed, 126 insertions(+), 34 deletions(-) diff --git a/e2e/config/config.go b/e2e/config/config.go index 8bdc6fd4e57..b9785c25c40 100644 --- a/e2e/config/config.go +++ b/e2e/config/config.go @@ -167,6 +167,27 @@ func (c *Configuration) IsLocalBuild() bool { return c.BuildID == "local" } +// SIGVersionTagNameBuildID is the value .pipelines/scripts/e2e_run.sh exports as +// SIG_VERSION_TAG_NAME when VHD_BUILD_ID is set, which selects the VHD produced by that +// specific build rather than the newest VHD tagged branch=refs/heads/main. +const SIGVersionTagNameBuildID = "buildId" + +// VHDBuiltFromSourceUnderTest reports whether the VHD under test was built from the same +// AgentBaker source these tests are running from. +// +// The VHD builder pipelines run their e2e stage with VHD_BUILD_ID: $(Build.BuildId) (see +// .vsts-vhd-builder.yaml, .vsts-vhd-builder-release.yaml), so the VHD selected by that tag is +// the one built earlier in the same pipeline run, from the same commit. e2e_run.sh turns that +// into SIG_VERSION_TAG_NAME=buildId. +// +// The standalone e2e check leaves VHD_BUILD_ID unset, so SIGVersionTagName keeps its +// "branch"/"refs/heads/main" default and the PR's code runs against a VHD built from main. +// Validations that compare VHD-baked state against source-generated state are only meaningful +// when this returns true. +func (c *Configuration) VHDBuiltFromSourceUnderTest() bool { + return c.SIGVersionTagName == SIGVersionTagNameBuildID +} + func (c *Configuration) BlobStorageAccountURL() string { return "https://" + c.BlobStorageAccount() + ".blob.core.windows.net" } diff --git a/e2e/validators.go b/e2e/validators.go index d39f2489f54..eb4633d035d 100644 --- a/e2e/validators.go +++ b/e2e/validators.go @@ -3488,16 +3488,16 @@ func ValidateScriptlessNBCCSECmd(ctx context.Context, s *Scenario) error { // source under test. The nbc-cmd side is generated from the source under test instead, so // the two sides only agree when the VHD was built from that same source. // -// E2E deliberately does not do that: it runs the PR's code against the newest VHD tagged -// branch=refs/heads/main (see config.SIGVersionTagValue, which defaults to "refs/heads/main"). -// So any PR that changes a GPU driver version in parts/common/components.json will -// legitimately differ on these vars until a VHD is rebuilt from that PR, which cannot happen -// before merge. Failing on them makes GPU driver bumps unmergeable for a reason unrelated to -// the change under test. +// The standalone e2e check deliberately does not do that: it runs the PR's code against the +// newest VHD tagged branch=refs/heads/main (see config.SIGVersionTagValue). So any PR that +// changes a GPU driver version in parts/common/components.json will legitimately differ on +// these vars until a VHD is rebuilt from that PR, which cannot happen before merge. Failing +// on them makes GPU driver bumps unmergeable for a reason unrelated to the change under test. // -// Differences limited to these vars are therefore tolerated here. Any other differing var -// still fails, and aks-node-controller still reports the full diff (including these vars) as -// a guest agent event for Kusto, so production observability is unchanged. +// Differences limited to these vars are therefore tolerated, but only when the VHD was not +// built from the source under test — see unexpectedEnvCompareDiffVars. aks-node-controller +// still reports the full diff (including these vars) as a guest agent event for Kusto, so +// production observability is unchanged. var vhdSourcedEnvVars = map[string]struct{}{ "GPU_DRIVER_VERSION": {}, "GPU_IMAGE_SHA": {}, @@ -3525,14 +3525,20 @@ func parseEnvCompareDiffVars(grepOutput string) []string { return names } -// unexpectedEnvCompareDiffVars returns the differing env vars that are not explained by -// VHD-vs-source skew. -func unexpectedEnvCompareDiffVars(diffVars []string) []string { +// unexpectedEnvCompareDiffVars returns the differing env vars that phase 3 should fail on. +// +// When the VHD under test was built from the source under test, both sides of the comparison +// come from the same commit, so every difference is real and nothing is tolerated. Otherwise +// the vars in vhdSourcedEnvVars are read from the VHD on the provision-config side while the +// nbc-cmd side is generated from source, so they differ for a reason unrelated to the change +// under test and are excluded. +func unexpectedEnvCompareDiffVars(diffVars []string, vhdBuiltFromSourceUnderTest bool) []string { var unexpected []string for _, name := range diffVars { - if _, ok := vhdSourcedEnvVars[name]; !ok { - unexpected = append(unexpected, name) + if _, isVHDSourced := vhdSourcedEnvVars[name]; isVHDSourced && !vhdBuiltFromSourceUnderTest { + continue } + unexpected = append(unexpected, name) } return unexpected } @@ -3565,7 +3571,15 @@ func ValidateScriptlessPhase3(ctx context.Context, s *Scenario) error { "but the success marker is absent and no diff entries were found in %s:\n%s", logFile, result.stdout) } - if unexpected := unexpectedEnvCompareDiffVars(diffVars); len(unexpected) > 0 { + builtFromSource := config.Config.VHDBuiltFromSourceUnderTest() + + if unexpected := unexpectedEnvCompareDiffVars(diffVars, builtFromSource); len(unexpected) > 0 { + if builtFromSource { + return fmt.Errorf("expected no env var differences between provision-config and nbc-cmd, but found differences in %v. "+ + "The VHD under test was built from the source under test (%s=%s), so both sides come from the same commit "+ + "and every difference is real:\n%s", + unexpected, config.Config.SIGVersionTagName, config.Config.SIGVersionTagValue, result.stdout) + } return fmt.Errorf("expected no env var differences between provision-config and nbc-cmd, but found differences in %v:\n%s", unexpected, result.stdout) } diff --git a/e2e/validators_envcompare_test.go b/e2e/validators_envcompare_test.go index 6a83eab7498..c2012c2810e 100644 --- a/e2e/validators_envcompare_test.go +++ b/e2e/validators_envcompare_test.go @@ -3,6 +3,7 @@ package e2e import ( "testing" + "github.com/Azure/agentbaker/e2e/config" "github.com/stretchr/testify/assert" ) @@ -52,43 +53,99 @@ func TestParseEnvCompareDiffVars(t *testing.T) { func TestUnexpectedEnvCompareDiffVars(t *testing.T) { for _, tc := range []struct { - name string - diffVars []string - expected []string + name string + // vhdBuiltFromSourceUnderTest mirrors config.Configuration.VHDBuiltFromSourceUnderTest: + // true in the VHD builder pipelines (VHD_BUILD_ID set), false for the standalone e2e check. + vhdBuiltFromSourceUnderTest bool + diffVars []string + expected []string }{ { - name: "GPU vars alone are explained by VHD skew", - diffVars: []string{"GPU_DRIVER_VERSION", "GPU_IMAGE_SHA"}, - expected: nil, + name: "GPU vars alone are explained by VHD skew when the VHD is not from this source", + vhdBuiltFromSourceUnderTest: false, + diffVars: []string{"GPU_DRIVER_VERSION", "GPU_IMAGE_SHA"}, + expected: nil, }, { - name: "a non-GPU var is still reported", - diffVars: []string{"GPU_DRIVER_VERSION", "KUBELET_FLAGS"}, - expected: []string{"KUBELET_FLAGS"}, + name: "GPU vars are NOT tolerated when the VHD was built from this source", + vhdBuiltFromSourceUnderTest: true, + diffVars: []string{"GPU_DRIVER_VERSION", "GPU_IMAGE_SHA"}, + expected: []string{"GPU_DRIVER_VERSION", "GPU_IMAGE_SHA"}, }, { - name: "only non-GPU vars are reported", - diffVars: []string{"KUBELET_FLAGS", "NETWORK_PLUGIN"}, - expected: []string{"KUBELET_FLAGS", "NETWORK_PLUGIN"}, + name: "a non-GPU var is still reported", + vhdBuiltFromSourceUnderTest: false, + diffVars: []string{"GPU_DRIVER_VERSION", "KUBELET_FLAGS"}, + expected: []string{"KUBELET_FLAGS"}, }, { - name: "no diffs yields none", - diffVars: nil, - expected: nil, + name: "only non-GPU vars are reported", + vhdBuiltFromSourceUnderTest: false, + diffVars: []string{"KUBELET_FLAGS", "NETWORK_PLUGIN"}, + expected: []string{"KUBELET_FLAGS", "NETWORK_PLUGIN"}, + }, + { + name: "non-GPU vars are reported regardless of VHD provenance", + vhdBuiltFromSourceUnderTest: true, + diffVars: []string{"KUBELET_FLAGS", "NETWORK_PLUGIN"}, + expected: []string{"KUBELET_FLAGS", "NETWORK_PLUGIN"}, + }, + { + name: "no diffs yields none", + vhdBuiltFromSourceUnderTest: false, + diffVars: nil, + expected: nil, }, } { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, unexpectedEnvCompareDiffVars(tc.diffVars)) + assert.Equal(t, tc.expected, unexpectedEnvCompareDiffVars(tc.diffVars, tc.vhdBuiltFromSourceUnderTest)) + }) + } +} + +// TestVHDBuiltFromSourceUnderTest asserts the provenance signal matches what +// .pipelines/scripts/e2e_run.sh exports: SIG_VERSION_TAG_NAME=buildId only when VHD_BUILD_ID +// is set, which the VHD builder pipelines set to $(Build.BuildId). +func TestVHDBuiltFromSourceUnderTest(t *testing.T) { + for _, tc := range []struct { + name string + tagName string + expected bool + }{ + { + name: "buildId tag means the VHD came from this pipeline run", + tagName: "buildId", + expected: true, + }, + { + name: "default branch tag means the VHD came from main, not this source", + tagName: "branch", + expected: false, + }, + { + name: "empty tag is not treated as same-source", + tagName: "", + expected: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + c := &config.Configuration{SIGVersionTagName: tc.tagName} + assert.Equal(t, tc.expected, c.VHDBuiltFromSourceUnderTest()) }) } } // TestEnvCompareGPUBumpIsTolerated asserts the end-to-end decision for the exact failure that -// blocked the aks-gpu-grid 570.237 bump: the diff parses to only VHD-sourced vars, so it must -// not fail the scenario. +// blocked the aks-gpu-grid 570.237 bump: on the standalone e2e check the diff parses to only +// VHD-sourced vars, so it must not fail the scenario. The same diff must still fail when the +// VHD was built from the source under test, where the two sides are expected to agree. func TestEnvCompareGPUBumpIsTolerated(t *testing.T) { diffVars := parseEnvCompareDiffVars(realGPUBumpLogLine) assert.Equal(t, []string{"GPU_DRIVER_VERSION", "GPU_IMAGE_SHA"}, diffVars) - assert.Empty(t, unexpectedEnvCompareDiffVars(diffVars), + + assert.Empty(t, unexpectedEnvCompareDiffVars(diffVars, false), "a GPU driver version bump must not fail phase 3 when the VHD is built from a different source") + + assert.Equal(t, diffVars, unexpectedEnvCompareDiffVars(diffVars, true), + "the same diff must still fail once the VHD is built from the source under test") }