Skip to content

TRT-2518: Add variant OS and JobTier to promotion check #2763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xueqzhan wants to merge 2 commits into
base: master
Choose a base branch
from
+438 −58
Open

TRT-2518: Add variant OS and JobTier to promotion check #2763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cece552
Add variant OS and JobTier to promotion check
xueqzhan Mar 12, 2026
f3165b7
address review comments
xueqzhan Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 138 additions & 19 deletions tools/codegen/cmd/featuregate-test-analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,22 +188,59 @@ func (o *FeatureGateTestAnalyzerOptions) Run(ctx context.Context) error {

writeTestingMarkDown(testingResults, md)

currErrs := checkIfTestingIsSufficient(enabledFeatureGate, testingResults)
if len(currErrs) == 0 {
validationResults := checkIfTestingIsSufficient(enabledFeatureGate, testingResults)

// Separate warnings from blocking errors
blockingErrors := []error{}
warnings := []error{}
for _, vr := range validationResults {
if vr.IsWarning {
warnings = append(warnings, vr.Error)
} else {
blockingErrors = append(blockingErrors, vr.Error)
}
}

if len(validationResults) == 0 {
md.Textf("Sufficient CI testing for %q.\n", enabledFeatureGate)
fmt.Fprintf(o.Out, "Sufficient CI testing for %q.\n", enabledFeatureGate)
} else {
md.Textf("INSUFFICIENT CI testing for %q.\n", enabledFeatureGate)
if len(blockingErrors) > 0 {
md.Textf("INSUFFICIENT CI testing for %q.\n", enabledFeatureGate)
fmt.Fprintf(o.Out, "INSUFFICIENT CI testing for %q.\n", enabledFeatureGate)
} else {
md.Textf("CI testing issues found for %q (non-blocking warnings).\n", enabledFeatureGate)
fmt.Fprintf(o.Out, "CI testing issues found for %q (non-blocking warnings).\n", enabledFeatureGate)
}

md.Textf("* At least five tests are expected for a feature\n")
md.Textf("* Tests must be be run on every TechPreview platform (ask for an exception if your feature doesn't support a variant)")
md.Textf("* All tests must run at least 14 times on every platform")
md.Textf("* All tests must pass at least 95%% of the time")
md.Textf("* JobTier must be one of: standard, informing, blocking\n")
md.Text("")

if len(warnings) > 0 {
md.Textf("**Non-blocking warnings (optional variants):**\n")
for _, warn := range warnings {
md.Textf(" - %s\n", warn.Error())
}
md.Text("")
}

if len(blockingErrors) > 0 {
md.Textf("**Blocking errors:**\n")
for _, err := range blockingErrors {
md.Textf(" - %s\n", err.Error())
}
md.Text("")
}
md.Text("")
fmt.Fprintf(o.Out, "INSUFFICIENT CI testing for %q.\n", enabledFeatureGate)
}
errs = append(errs, currErrs...)
featureGateHTMLData = append(featureGateHTMLData, buildHTMLFeatureGateData(enabledFeatureGate, testingResults, currErrs, release))

// Only add blocking errors to the error list (warnings don't fail the job)
errs = append(errs, blockingErrors...)
featureGateHTMLData = append(featureGateHTMLData, buildHTMLFeatureGateData(enabledFeatureGate, testingResults, blockingErrors, release))

}

Expand All @@ -223,7 +260,7 @@ func (o *FeatureGateTestAnalyzerOptions) Run(ctx context.Context) error {
return errors.Join(errs...)
}

func buildHTMLFeatureGateData(name string, testingResults map[JobVariant]*TestingResults, errs []error, release string) utils.HTMLFeatureGate {
func buildHTMLFeatureGateData(name string, testingResults map[JobVariant]*TestingResults, blockingErrors []error, release string) utils.HTMLFeatureGate {
jobVariantsSet := sets.KeySet(testingResults)
jobVariants := jobVariantsSet.UnsortedList()

Expand All @@ -244,6 +281,9 @@ func buildHTMLFeatureGateData(name string, testingResults map[JobVariant]*Testin
Cloud: jv.Cloud,
Architecture: jv.Architecture,
NetworkStack: jv.NetworkStack,
OS: jv.OS,
JobTiers: jv.JobTiers,
Optional: jv.Optional,
ColIndex: i + 1,
})
}
Expand Down Expand Up @@ -272,6 +312,7 @@ func buildHTMLFeatureGateData(name string, testingResults map[JobVariant]*Testin
jobVariant.Cloud,
jobVariant.Architecture,
jobVariant.NetworkStack,
jobVariant.OS,
),
}
if testResults == nil {
Expand All @@ -296,7 +337,7 @@ func buildHTMLFeatureGateData(name string, testingResults map[JobVariant]*Testin

return utils.HTMLFeatureGate{
Name: name,
Sufficient: len(errs) == 0,
Sufficient: len(blockingErrors) == 0,
Variants: variants,
Tests: tests,
}
Expand Down Expand Up @@ -326,15 +367,29 @@ func writeHTMLFromTemplate(filename string, featureGateHTMLData []utils.HTMLFeat
return nil
}

func checkIfTestingIsSufficient(featureGate string, testingResults map[JobVariant]*TestingResults) []error {
errs := []error{}
func checkIfTestingIsSufficient(featureGate string, testingResults map[JobVariant]*TestingResults) []ValidationResult {
results := []ValidationResult{}

for jobVariant, testedVariant := range testingResults {
// Use the Optional field to determine if validation failures are warnings or errors
// Optional variants (like RHEL 10 in 4.22) have non-blocking warnings
isOptional := jobVariant.Optional

if len(testedVariant.TestResults) < requiredNumberOfTests {
errs = append(errs, fmt.Errorf("error: only %d tests found, need at least %d for %q on %v", len(testedVariant.TestResults), requiredNumberOfTests, featureGate, jobVariant))
results = append(results, ValidationResult{
Error: fmt.Errorf("error: only %d tests found, need at least %d for %q on %v",
len(testedVariant.TestResults), requiredNumberOfTests, featureGate, jobVariant),
IsWarning: isOptional,
})
}

for _, testResults := range testedVariant.TestResults {
if testResults.TotalRuns < requiredNumberOfTestRunsPerVariant {
errs = append(errs, fmt.Errorf("error: %q only has %d runs, need at least %d runs for %q on %v", testResults.TestName, testResults.TotalRuns, requiredNumberOfTestRunsPerVariant, featureGate, jobVariant))
results = append(results, ValidationResult{
Error: fmt.Errorf("error: %q only has %d runs, need at least %d runs for %q on %v",
testResults.TestName, testResults.TotalRuns, requiredNumberOfTestRunsPerVariant, featureGate, jobVariant),
IsWarning: isOptional,
})
}
if testResults.TotalRuns == 0 {
continue
Expand All @@ -343,12 +398,16 @@ func checkIfTestingIsSufficient(featureGate string, testingResults map[JobVarian
if passPercent < requiredPassRateOfTestsPerVariant {
displayExpected := int(requiredPassRateOfTestsPerVariant * 100)
displayActual := int(passPercent * 100)
errs = append(errs, fmt.Errorf("error: %q only passed %d%%, need at least %d%% for %q on %v", testResults.TestName, displayActual, displayExpected, featureGate, jobVariant))
results = append(results, ValidationResult{
Error: fmt.Errorf("error: %q only passed %d%%, need at least %d%% for %q on %v",
testResults.TestName, displayActual, displayExpected, featureGate, jobVariant),
IsWarning: isOptional,
})
}
}
}

return errs
return results
}

func writeTestingMarkDown(testingResults map[JobVariant]*TestingResults, md *utils.Markdown) {
Expand All @@ -364,6 +423,12 @@ func writeTestingMarkDown(testingResults map[JobVariant]*TestingResults, md *uti
if jobVariant.NetworkStack != "" {
columnHeader = columnHeader + fmt.Sprintf("<br/> %v ", jobVariant.NetworkStack)
}
if jobVariant.OS != "" {
columnHeader = columnHeader + fmt.Sprintf("<br/> OS:%v ", jobVariant.OS)
}
if jobVariant.JobTiers != "" {
columnHeader = columnHeader + fmt.Sprintf("<br/> Tiers:%v ", jobVariant.JobTiers)
}
md.Exact(columnHeader)
}
md.EndTableRow()
Expand Down Expand Up @@ -458,6 +523,13 @@ var (
Architecture: "amd64",
Topology: "single",
},
{
Cloud: "aws",
Architecture: "amd64",
Topology: "ha",
OS: "rhel10",
Optional: true, // RHEL 10 is optional in 4.22, will be required in OCP 5
Copy link
Contributor

@JoelSpeed JoelSpeed Mar 13, 2026

Choose a reason for hiding this comment

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

Will optionality be represented on the rendered HTML page?

},

// TODO restore these once we run TechPreview jobs that contain them
//{
Expand Down Expand Up @@ -533,6 +605,9 @@ type JobVariant struct {
Architecture string
Topology string
NetworkStack string
OS string
JobTiers string // Comma-separated tiers (e.g., "standard,informing,blocking"). If empty, defaults to "standard,informing,blocking"
Optional bool // If true, validation failures for this variant are non-blocking warnings
}

type OrderedJobVariants []JobVariant
Expand Down Expand Up @@ -575,6 +650,12 @@ type TestResults struct {
FlakedRuns int
}

// ValidationResult represents a validation error or warning
type ValidationResult struct {
Error error
IsWarning bool // if true, this is a non-blocking warning (for optional variants)
}

func testResultByName(results []TestResults, testName string) *TestResults {
for _, curr := range results {
if curr.TestName == testName {
Expand All @@ -584,6 +665,36 @@ func testResultByName(results []TestResults, testName string) *TestResults {
return nil
}

func validateJobTiers(jobVariant JobVariant) error {
if jobVariant.JobTiers == "" {
return nil // Empty is valid - will default to standard,informing,blocking
}

validTiers := map[string]bool{
"standard": true,
"informing": true,
"blocking": true,
}

hasValidTier := false
for _, tier := range strings.Split(jobVariant.JobTiers, ",") {
tier = strings.TrimSpace(tier)
if tier != "" {
hasValidTier = true
if !validTiers[tier] {
return fmt.Errorf("invalid JobTier %q in variant %+v - must be one of: standard, informing, blocking", tier, jobVariant)
}
}
}

// Reject malformed strings like "," or " , " that contain no valid tiers
if !hasValidTier {
return fmt.Errorf("JobTiers string %q contains no valid tier names in variant %+v", jobVariant.JobTiers, jobVariant)
}

return nil
}
Comment on lines +668 to +696
Copy link

@qodo-code-review qodo-code-review bot Mar 13, 2026

Choose a reason for hiding this comment

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

Action required

2. Empty jobtier skips queries 🐞 Bug ✓ Correctness

JobTier strings containing only commas/whitespace (e.g. "," or " , ") pass validateJobTiers but
produce an empty jobTiersList, causing QueriesFor to return zero queries and
listTestResultForVariant to make no API calls. This silently returns empty test results and can
wrongly report insufficient testing.
Agent Prompt 
### Issue description
Malformed `JobTiers` strings like `","` or whitespace-only values currently pass validation but result in *zero* JobTier filters, producing zero Sippy queries and silently empty results.

### Issue Context
`validateJobTiers` only rejects unknown non-empty tiers. `QueriesFor` drops empty trimmed tiers and does not apply a default if the resulting list is empty.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[667-685]
- tools/codegen/pkg/sippy/json_types.go[101-113]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


func listTestResultFor(featureGate string, clusterProfiles sets.Set[string]) (map[JobVariant]*TestingResults, error) {
fmt.Printf("Query sippy for all test run results for feature gate %q on clusterProfile %q\n", featureGate, sets.List(clusterProfiles))

Expand All @@ -605,6 +716,13 @@ func listTestResultFor(featureGate string, clusterProfiles sets.Set[string]) (ma
jobVariantsToCheck = append(jobVariantsToCheck, selfManagedPlatformVariants...)
}

// Validate all variants before making expensive API calls
for _, jobVariant := range jobVariantsToCheck {
if err := validateJobTiers(jobVariant); err != nil {
return nil, err
}
}

for _, jobVariant := range jobVariantsToCheck {
jobVariantResults, err := listTestResultForVariant(featureGate, jobVariant)
if err != nil {
Expand Down Expand Up @@ -720,7 +838,7 @@ func listTestResultForVariant(featureGate string, jobVariant JobVariant) (*Testi
}

testNameToResults := map[string]*TestResults{}
queries := sippy.QueriesFor(jobVariant.Cloud, jobVariant.Architecture, jobVariant.Topology, jobVariant.NetworkStack, testPattern)
queries := sippy.QueriesFor(jobVariant.Cloud, jobVariant.Architecture, jobVariant.Topology, jobVariant.NetworkStack, jobVariant.OS, jobVariant.JobTiers, testPattern)
release, err := getRelease()
if err != nil {
return nil, fmt.Errorf("couldn't fetch latest release version: %w", err)
Expand Down Expand Up @@ -776,11 +894,12 @@ func listTestResultForVariant(featureGate string, jobVariant JobVariant) (*Testi

// Try to find enough test results in the last week, but if we have to we can extend
// the window to two weeks.
// NOTE: Use += to accumulate results across multiple JobTier queries
if currTest.CurrentRuns >= requiredNumberOfTestRunsPerVariant {
testResults.TotalRuns = currTest.CurrentRuns
testResults.SuccessfulRuns = currTest.CurrentSuccesses
testResults.FailedRuns = currTest.CurrentFailures
testResults.FlakedRuns = currTest.CurrentFlakes
testResults.TotalRuns += currTest.CurrentRuns
testResults.SuccessfulRuns += currTest.CurrentSuccesses
testResults.FailedRuns += currTest.CurrentFailures
testResults.FlakedRuns += currTest.CurrentFlakes
} else {
fmt.Printf("Insufficient results in last 7 days, increasing lookback to 2 weeks...")
testResults.TotalRuns += currTest.CurrentRuns + currTest.PreviousRuns
Expand Down
Loading