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") }