fix: disable scriptless phase2 for subsets of overlapping tests#8430
fix: disable scriptless phase2 for subsets of overlapping tests#8430awesomenix merged 1 commit intomainfrom
Conversation
awesomenix
requested review from
AbelHu,
Devinwong,
SriHarsha001,
calvin197,
cameronmeissner,
djsly,
ganeshkumarashok,
junjiezhang1997,
lilypan26,
mxj220,
pdamianov-dev,
phealy,
r2k1,
sulixu,
surajssd,
timmy-wright and
zachary-bailey
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Disables the scriptless_nbc (phase2) RunScenario variant for selected GPU scenarios to reduce redundant overlapping GPU test runs and save quota.
Changes:
- Introduced a new tag (
DisableScriptlessNBC) and used it to suppress the scriptless NBC subtest in specific scenarios. - Updated several GPU scenarios/tests to set
DisableScriptlessNBC: trueand renamed some tests to no longer imply scriptless coverage. - Removed a
GPUtag block from the DCGM exporter compatibility test table cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/types.go | Adds a new Tags flag to disable the scriptless NBC phase2 subtest per-scenario. |
| e2e/test_helpers.go | Gates scriptless_nbc subtest execution on the new tag. |
| e2e/scenario_test.go | Updates multiple GPU scenarios to disable scriptless NBC phase2 and renames tests accordingly. |
| e2e/scenario_gpu_managed_experience_test.go | Removes GPU tagging for a table-driven GPU compatibility scenario; disables scriptless phase2 for some tests. |
| e2e/scenario_gpu_daemonset_test.go | Disables scriptless NBC phase2 for the daemonset-based GPU plugin test. |
| VHDCaching bool | ||
| MockAzureChinaCloud bool | ||
| VMSeriesCoverageTest bool | ||
| DisableScriptlessNBC bool | ||
| } |
There was a problem hiding this comment.
DisableScriptlessNBC is ambiguous because the code is only skipping the scriptless_nbc RunScenario variant (phase2), not disabling “scriptless” behavior broadly. Consider renaming to something more specific like DisableScriptlessNBCPhase2 / SkipScriptlessNBCVariant (mandatory if this tag is expected to be used by many tests, to avoid incorrect usage later).
|
|
||
| func supportsScriptlessNBCCSECmd(s *Scenario) bool { | ||
| return s.AKSNodeConfigMutator == nil && !s.IsWindows() && len(s.Config.CustomDataWriteFiles) <= 0 && !s.VHDCaching && !config.Config.TestPreProvision && !s.SkipScriptlessNBC | ||
| return s.AKSNodeConfigMutator == nil && !s.Tags.DisableScriptlessNBC && !s.IsWindows() && len(s.Config.CustomDataWriteFiles) <= 0 && !s.VHDCaching && !config.Config.TestPreProvision && !s.SkipScriptlessNBC |
There was a problem hiding this comment.
There are now two independent switches controlling the same behavior (Tags.DisableScriptlessNBC and SkipScriptlessNBC). This increases the chance of confusion/inconsistent usage across scenarios. Consider consolidating to a single source of truth (e.g., deprecate one flag or have one derived from the other) so callers don’t have to know which one to set.
| }, | ||
| Config: Config{ | ||
| Cluster: ClusterKubenet, | ||
| VHD: tc.vhd, |
There was a problem hiding this comment.
The Tags: Tags{ GPU: true } block was removed from this GPU-oriented table test. If tag-based filtering is used to select GPU tests in CI (or to apply GPU-specific behavior elsewhere), this test may no longer be included/excluded correctly. Restore GPU: true here (and if the goal is to save quota, also consider setting DisableScriptlessNBC: true rather than removing GPU classification).
| VHD: tc.vhd, | |
| VHD: tc.vhd, | |
| Tags: Tags{GPU: true}, |
| func runScenarioACLGRID(t *testing.T, vmSize string) { | ||
| RunScenario(t, &Scenario{ | ||
| Description: fmt.Sprintf("Tests that a GPU-enabled node with VM size %s using an ACL VHD and scriptless CSE can be properly bootstrapped, and that the GRID license is valid", vmSize), | ||
| Description: fmt.Sprintf("Tests that a GPU-enabled node with VM size %s using an ACL VHD, and that the GRID license is valid", vmSize), | ||
| Tags: Tags{ | ||
| GPU: true, | ||
| GPU: true, | ||
| DisableScriptlessNBC: true, | ||
| }, |
There was a problem hiding this comment.
These updated scenario descriptions no longer mention scriptless behavior, but the scenario still appears to validate scriptless CSE command behavior (e.g., ValidateScriptlessCSECmd in this scenario’s validator). Update the description to match what is actually being tested (or remove the scriptless validation/config if it’s no longer intended to be covered).
| VHDCaching bool | ||
| MockAzureChinaCloud bool | ||
| VMSeriesCoverageTest bool | ||
| DisableScriptlessNBC bool |
There was a problem hiding this comment.
Can we leave a comment to let ourselves know when full scriptless (phase3) is enabled, what toggles/flags we can clean up and which should be left?
| VHDCaching bool | ||
| MockAzureChinaCloud bool | ||
| VMSeriesCoverageTest bool | ||
| DisableScriptlessNBC bool |
There was a problem hiding this comment.
nit: could we just repurpose the SkipScriptlessNBC variable instead of adding a new tag?
awesomenix
force-pushed
the
nishp/cleanup/gputests
branch
from
7833ac5 to
1296e4e
Compare
Disable few subsets of overlapping GPU tests to save quota