Skip to content

WIP: NO-JIRA: tooling: update job-based feature promotion analysis #2944

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
everettraven wants to merge 2 commits into
base: master
Choose a base branch
from
Open

WIP: NO-JIRA: tooling: update job-based feature promotion analysis #2944

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
51ac72d
tooling: update job-based feature promotion analysis
everettraven Jul 22, 2026
6672b5f
drop! promote AWSDualStackInstall
everettraven Jul 22, 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
2 changes: 1 addition & 1 deletion features.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
| ProvisioningRequestAvailable| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | | |
| AWSClusterHostedDNS| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AWSDedicatedHosts| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AWSDualStackInstall| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AWSEuropeanSovereignCloudInstall| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AdditionalStorageConfig| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AutomatedEtcdBackup| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
Expand Down Expand Up @@ -96,6 +95,7 @@
| VolumeGroupSnapshot| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| OSStreams| | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AWSClusterHostedDNSInstall| <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AWSDualStackInstall| <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AWSServiceLBNetworkSecurityGroup| <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| AzureWorkloadIdentity| <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| BootImageSkewEnforcement| <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
Expand Down
2 changes: 1 addition & 1 deletion features/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ var (
contactPerson("sadasu").
productScope(ocpSpecific).
enhancementPR("https://github.com/openshift/enhancements/pull/1806").
enable(inTechPreviewNoUpgrade(), inDevPreviewNoUpgrade()).
enable(inOKD(), inDefault(), inTechPreviewNoUpgrade(), inDevPreviewNoUpgrade()).
mustRegister()

FeatureGateAzureDualStackInstall = newFeatureGate("AzureDualStackInstall").
Expand Down
6 changes: 3 additions & 3 deletions payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@
{
"name": "AWSDedicatedHosts"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSEuropeanSovereignCloudInstall"
},
Expand Down Expand Up @@ -301,6 +298,9 @@
{
"name": "AWSClusterHostedDNSInstall"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSServiceLBNetworkSecurityGroup"
},
Expand Down
6 changes: 3 additions & 3 deletions payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@
{
"name": "AWSDedicatedHosts"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSEuropeanSovereignCloudInstall"
},
Expand Down Expand Up @@ -303,6 +300,9 @@
{
"name": "AWSClusterHostedDNSInstall"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSServiceLBNetworkSecurityGroup"
},
Expand Down
6 changes: 3 additions & 3 deletions payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@
{
"name": "AWSDedicatedHosts"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSEuropeanSovereignCloudInstall"
},
Expand Down Expand Up @@ -295,6 +292,9 @@
{
"name": "AWSClusterHostedDNSInstall"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSServiceLBNetworkSecurityGroup"
},
Expand Down
6 changes: 3 additions & 3 deletions payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@
{
"name": "AWSDedicatedHosts"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSEuropeanSovereignCloudInstall"
},
Expand Down Expand Up @@ -297,6 +294,9 @@
{
"name": "AWSClusterHostedDNSInstall"
},
{
"name": "AWSDualStackInstall"
},
{
"name": "AWSServiceLBNetworkSecurityGroup"
},
Expand Down
108 changes: 82 additions & 26 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 @@ -35,6 +35,11 @@ const (
// required pass rate.
// nearly all current tests pass 99% of the time, but in a two week window we lack enough data to say.
requiredPassRateOfTestsPerVariant = 0.95

// required pass rate of tests per job for job-based promotion verification.
// Given a minimum sample size of 14 runs, we allow a single unique failure across job runs which equates to ~92%.
// This ensures that unique flukes don't cause a job to be considered a failed run.
requiredPassRateOfTestsPerJob = 0.92
)

type FeatureGateTestAnalyzerOptions struct {
Expand Down Expand Up @@ -131,6 +136,7 @@ func (o *FeatureGateTestAnalyzerOptions) Run(ctx context.Context) error {
md := utils.NewMarkdown("FeatureGate Promotion Summary")

recentlyEnabledFeatureGatesToClusterProfiles := map[string]sets.Set[string]{}
defaultCurrentlyEnabledFeatureGates := sets.Set[string]{}
errs := []error{}
for _, clusterProfile := range allCurrentClusterProfiles.List() {
// we only need to check test coverage for current cluster profiles
Expand All @@ -149,6 +155,8 @@ func (o *FeatureGateTestAnalyzerOptions) Run(ctx context.Context) error {
continue
}

defaultCurrentlyEnabledFeatureGates.Insert(featureGateName)

previousFeatureGateEnabled := false
if previousDefaultFeatureGateInfo != nil {
previousFeatureGateEnabled = previousDefaultFeatureGateInfo.allFeatureGates[featureGateName]
Expand Down Expand Up @@ -181,7 +189,7 @@ func (o *FeatureGateTestAnalyzerOptions) Run(ctx context.Context) error {
clusterProfiles := recentlyEnabledFeatureGatesToClusterProfiles[enabledFeatureGate]
md.Title(1, enabledFeatureGate)

testingResults, err := listTestResultFor(enabledFeatureGate, clusterProfiles)
testingResults, err := listTestResultFor(enabledFeatureGate, clusterProfiles, defaultCurrentlyEnabledFeatureGates)
if err != nil {
return err
}
Expand Down Expand Up @@ -736,7 +744,7 @@ func validateJobTiers(jobVariant JobVariant) error {
return nil
}

func listTestResultFor(featureGate string, clusterProfiles sets.Set[string]) (map[JobVariant]*TestingResults, error) {
func listTestResultFor(featureGate string, clusterProfiles sets.Set[string], defaultEnabledFeatureGates 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))

results := map[JobVariant]*TestingResults{}
Expand Down Expand Up @@ -765,7 +773,7 @@ func listTestResultFor(featureGate string, clusterProfiles sets.Set[string]) (ma
}

for _, jobVariant := range jobVariantsToCheck {
jobVariantResults, err := listTestResultForVariant(featureGate, jobVariant)
jobVariantResults, err := listTestResultForVariant(featureGate, jobVariant, defaultEnabledFeatureGates)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -862,13 +870,13 @@ func getRelease() (string, error) {
return getLatestRelease()
}

func listTestResultForVariant(featureGate string, jobVariant JobVariant) (*TestingResults, error) {
func listTestResultForVariant(featureGate string, jobVariant JobVariant, defaultEnabledGates sets.Set[string]) (*TestingResults, error) {
// Substring here matches for both [OCPFeatureGate:...] and [FeatureGate:...]
testPattern := fmt.Sprintf("FeatureGate:%s]", featureGate)

// Feature gates used by the installer don't need separate tests, use the overall install tests
if strings.Contains(featureGate, "Install") {
return verifyJobBasedFeatureGatePromotion(featureGate, jobVariant)
return verifyJobBasedFeatureGatePromotion(featureGate, jobVariant, defaultEnabledGates)
}

fmt.Printf("Query sippy for all test run results for pattern %q on variant %#v\n", testPattern, jobVariant)
Expand Down Expand Up @@ -990,7 +998,7 @@ func matchTwoNodeFeatureGates(featureGate string, topology string) bool {
return false
}

func verifyJobBasedFeatureGatePromotion(featureGate string, jobVariant JobVariant) (*TestingResults, error) {
func verifyJobBasedFeatureGatePromotion(featureGate string, jobVariant JobVariant, defaultEnabledGates sets.Set[string]) (*TestingResults, error) {
ocpRelease, err := getRelease()
if err != nil {
return nil, fmt.Errorf("getting release version: %w", err)
Expand Down Expand Up @@ -1021,7 +1029,7 @@ func verifyJobBasedFeatureGatePromotion(featureGate string, jobVariant JobVarian
testResults := []TestResults{}

for _, job := range jobs {
results, err := verifyJobPassRate(sippyClient, ocpRelease, job, jobVariant)
results, err := verifyJobPassRate(sippyClient, ocpRelease, job, jobVariant, defaultEnabledGates)
if err != nil {
return nil, fmt.Errorf("verifying job pass rate for job %q: %w", job.Name, err)
}
Expand All @@ -1035,7 +1043,7 @@ func verifyJobBasedFeatureGatePromotion(featureGate string, jobVariant JobVarian
}, nil
}

func verifyJobPassRate(client *http.Client, release string, job sippy.SippyJob, variant JobVariant) (*TestResults, error) {
func verifyJobPassRate(client *http.Client, release string, job sippy.SippyJob, variant JobVariant, defaultEnabledGates sets.Set[string]) (*TestResults, error) {
// Do an early check for 95% pass rate with at least 14 runs
runs := job.CurrentRuns
passes := job.CurrentPasses
Expand All @@ -1053,10 +1061,10 @@ func verifyJobPassRate(client *http.Client, release string, job sippy.SippyJob,
// failures analysis of the job runs to see if failed runs are true failures or known regressions.
if runs < requiredNumberOfTestRunsPerVariant {
return &TestResults{
TestName: job.Name,
TotalRuns: runs,
TestName: job.Name,
TotalRuns: runs,
SuccessfulRuns: passes,
FailedRuns: runs - passes,
FailedRuns: runs - passes,
}, nil
}

Expand All @@ -1065,15 +1073,15 @@ func verifyJobPassRate(client *http.Client, release string, job sippy.SippyJob,
//
// This saves us from unnecessarily making calls out to Sippy to perform a more nuanced
// failures analysis of the job runs to see if failed runs are true failures or known regressions.
if float32(passes) / float32(runs) >= requiredPassRateOfTestsPerVariant {
if float32(passes)/float32(runs) >= requiredPassRateOfTestsPerVariant {
return &TestResults{
TestName: job.Name,
TotalRuns: runs,
TestName: job.Name,
TotalRuns: runs,
SuccessfulRuns: passes,
FailedRuns: runs - passes,
FailedRuns: runs - passes,
}, nil
}

// We haven't passed promotion requirements with this job, but jobs might be impacted
// by known regressed tests. While important to get fixed, many regressions are either
// release blockers or require an exception to not be a release blocker.
Expand Down Expand Up @@ -1101,36 +1109,86 @@ func verifyJobPassRate(client *http.Client, release string, job sippy.SippyJob,
return nil, fmt.Errorf("getting triaged test failures from sippy: %w", err)
}

seenFailures := map[string]int{}

for _, jobRun := range jobRuns {
if jobRun.OverallResult == "F" && !jobRun.KnownFailure {
// skip infrastructure related failures so they aren't included in the analysis results.
if jobRun.InfrastructureFailure {
fmt.Println("skipping job run %s due to infrastructure related failure", jobRun.TestGridURL)
continue
Comment on lines +1115 to +1118

@qodo-for-rh-openshift qodo-for-rh-openshift Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Informational

4. Misformatted infra skip log 🐞 Bug ◔ Observability

The infrastructure-failure skip message uses fmt.Println with a format string, so the output prints
a literal "%s" and does not interpolate the URL.
Agent Prompt 
### Issue description
A format string is passed to `fmt.Println`, so `%s` is not expanded and logs are misleading.

### Issue Context
This is in the infrastructure-failure skip path inside `verifyJobPassRate`.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[1115-1118]

### Suggested implementation direction
- Replace with `fmt.Printf("skipping job run %s due to infrastructure related failure\n", jobRun.TestGridURL)` or use `fmt.Println` without `%s`.

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

}
Comment on lines 1109 to +1119

@qodo-for-rh-openshift qodo-for-rh-openshift Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

3. Infra failures inflate pass rate 🐞 Bug ≡ Correctness

Infrastructure-failure job runs are skipped during analysis but still included in TotalRuns
(len(jobRuns)) and thus treated as successes in SuccessfulRuns, inflating the computed pass rates.
Agent Prompt 
### Issue description
`TotalRuns` is initialized to `len(jobRuns)` before filtering, but infrastructure failures are `continue`d and never removed from the denominator. Later, `SuccessfulRuns` is derived from `TotalRuns`, so skipped infrastructure failures are effectively counted as successful runs.

This contradicts the intent stated in the comment and can allow promotion by inflating success rate.

### Issue Context
Sippy job-run JSON now includes `infrastructure_failure`, and the analyzer explicitly tries to exclude these runs.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[1102-1119]
- tools/codegen/cmd/featuregate-test-analyzer.go[1175-1177]
- tools/codegen/pkg/sippy/json_types.go[67-75]

### Suggested implementation direction
- Maintain an `analyzedRuns` counter (or slice) that increments only for non-infrastructure runs.
- Set `testResults.TotalRuns = analyzedRuns` (not `len(jobRuns)`).
- Ensure `SuccessfulRuns`/`FailedRuns` are also based on `analyzedRuns`.
- If `analyzedRuns == 0`, return early with zeros to avoid misleading rates.

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


if jobRun.OverallResult == "F" && !jobRun.KnownFailure {
untriagedTestFailures := []string{}
for _, failure := range jobRun.FailedTestNames {
if !triagedTestFailures.Has(failure) {
untriagedTestFailures = append(untriagedTestFailures, failure)
}
}

if len(untriagedTestFailures) > 0 {
// filter out TPNU feature test failures
untriagedNonTechPreviewTestFailures := []string{}
for _, failure := range untriagedTestFailures {
featureGate := featureGateFromTestName(failure)
if len(featureGate) == 0 {
untriagedNonTechPreviewTestFailures = append(untriagedNonTechPreviewTestFailures, failure)
continue
}

// Skip this test failure if the gate is not enabled by default
if !defaultEnabledGates.Has(featureGate) {
fmt.Printf("test %q failed for job run %s but is being removed from analysis because the feature gate %q is not enabled by default\n", failure, jobRun.TestGridURL, featureGate)
continue
}

untriagedNonTechPreviewTestFailures = append(untriagedNonTechPreviewTestFailures, failure)
}

if len(untriagedNonTechPreviewTestFailures) > 0 {
var writer strings.Builder
writer.WriteString(fmt.Sprintf("job run %s has untriaged test failures:\n", jobRun.TestGridURL))
for _, testFailure := range untriagedTestFailures {
writer.WriteString(fmt.Sprintf("job run %s has untriaged non-techpreview test failures:\n", jobRun.TestGridURL))
for _, testFailure := range untriagedNonTechPreviewTestFailures {
writer.WriteString(fmt.Sprintf("\t- %s\n", testFailure))
seenFailures[testFailure]++
}

fmt.Println(writer.String())
testResults.FailedRuns++

continue
}
}
}

testResults.SuccessfulRuns++
jobsFailed := 0

for test, failureCount := range seenFailures {
passRate := float32(testResults.TotalRuns - failureCount)/float32(testResults.TotalRuns)
if passRate < requiredPassRateOfTestsPerJob {
fmt.Printf("test %q passed at a rate of %f%% across all job runs, which is less than the required pass rate of %f%% percent.\n", test, passRate*100, requiredPassRateOfTestsPerJob*100)

// Because a test can fail at most once per job run, use the highest failure count as the number of job runs that failed.
if jobsFailed < failureCount {
jobsFailed = failureCount
}
}

@dgoodwin dgoodwin Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A single sippy query such as [https://sippy-auth.dptools.openshift.org/sippy-ng/tests/5.0/details?filters=%257B%2522items%2522%253A%255B%257B%2522columnField%2522%253A%2522variants%2522%252C%2522not%2522%253Atrue%252C%2522operatorValue%2522%253A%2522has%2520entry%2522%252C%2522value%2522%253A%2522never-stable%2522%257D%252C%257B%2522columnField%2522%253A%2522variants%2522%252C%2522not%2522%253Atrue%252C%2522operatorValue%2522%253A%2522has%2520entry%2522%252C%2522value%2522%253A%2522aggregated%2522%257D%252C%257B%2522columnField%2522%253A%2522variants%2522%252C%2522operatorValue%2522%253A%2522has%2520entry%2522%252C%2522value%2522%253A%2522Capability%253AAWSDualStackInstall%2522%257D%252C%257B%2522columnField%2522%253A%2522current_working_percentage%2522%252C%2522operatorValue%2522%253A%2522%253C%2522%252C%2522value%2522%253A%252292%2522%257D%252C%257B%2522columnField%2522%253A%2522current_runs%2522%252C%2522operatorValue%2522%253A%2522%253E%253D%2522%252C%2522value%2522%253A%25220%2522%257D%255D%252C%2522linkOperator%2522%253A%2522and%2522%257D&sort=asc&sortField=current_working_percentage&view=Working|this] will get you the list of all tests below the desired 92 pass rate. The only problem is the infra filtering, but those jobs should just fail on a couple tests one of which should explicitly state infrastructure, so we want to filter that out, perhaps all "install should succeed" can go. We also want to filter out "openshift-tests should pass" (synthetic test meaning all tests must pass). Both of those can be done in the api query.

The resulting list there are mostly network segmentation informing tests that are permafailing, they don't fail the job or appear in comp readiness, but sippy doesn't yet know how to filter them out.

}

testResults.FailedRuns = jobsFailed
testResults.SuccessfulRuns = testResults.TotalRuns - jobsFailed
Comment on lines +1161 to +1176

@coderabbitai coderabbitai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Count the union of failing job runs, not the largest per-test count.

Taking max(failureCount) undercounts failures when different tests fail in different runs. Track qualifying failed run identities and use the union size; otherwise a job can meet the pass threshold despite failures across more runs.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 1161 - 1176,
Update the failure aggregation around seenFailures to track the identities of
job runs containing tests below requiredPassRateOfTestsPerJob, rather than
taking the maximum failureCount. Set testResults.FailedRuns to the size of that
deduplicated failed-run set and derive SuccessfulRuns from TotalRuns minus the
union size, preserving the existing reporting for each qualifying test.


Comment on lines +1112 to +1177

@qodo-for-rh-openshift qodo-for-rh-openshift Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

2. Failed runs undercounted 🐞 Bug ≡ Correctness

verifyJobPassRate derives FailedRuns from the maximum per-test failure count, which can undercount
failing job runs when multiple different tests exceed the per-test failure threshold on different
runs, incorrectly marking the job as healthier than it is.
Agent Prompt 
### Issue description
`verifyJobPassRate` aggregates failures by test name (`seenFailures`) and then sets `FailedRuns` to `max(failureCount)` across tests under the per-test pass-rate threshold. This loses run-level information and can significantly undercount failed runs when different tests fail in different job runs.

Example: if test A fails in 2 distinct runs and test B fails in 2 other distinct runs (both below threshold), the code reports `FailedRuns=2` but the true number of failing runs is 4.

### Issue Context
The analyzer later uses `FailedRuns/SuccessfulRuns` as the summarized outcome for the job. Those values should represent distinct job runs, not per-test maxima.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[1112-1177]

### Suggested implementation direction
- Track run identity (e.g., by index or `TestGridURL`) while iterating job runs.
- Two-pass approach:
  1) First pass: collect per-test failure counts (as today).
  2) Determine the set of *disqualifying tests* (passRate < `requiredPassRateOfTestsPerJob`).
  3) Second pass: count distinct runs that contain at least one disqualifying test failure; set `FailedRuns` to that count.
- Keep `SuccessfulRuns = analyzedRuns - FailedRuns` (where `analyzedRuns` excludes skipped runs, see related infra finding).

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

return testResults, nil
}

var featureGateRegex = regexp.MustCompile("(\\[OCPFeatureGate:.+\\])")

func featureGateFromTestName(failure string) string {
gateAnnotation := featureGateRegex.FindString(failure)
if len(gateAnnotation) == 0 {
return ""
}

return strings.TrimRight(strings.TrimLeft(gateAnnotation, "[OCPFeatureGate:"), "]")
Comment on lines +1181 to +1189

@coderabbitai coderabbitai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the target file and inspect the relevant section with line numbers.
git ls-files tools/codegen/cmd/featuregate-test-analyzer.go
wc -l tools/codegen/cmd/featuregate-test-analyzer.go
sed -n '1160,1210p' tools/codegen/cmd/featuregate-test-analyzer.go

# Search for uses of featureGateRegex / featureGateFromTestName.
rg -n "featureGateRegex|featureGateFromTestName|OCPFeatureGate" tools/codegen -S

# Look for tests around this behavior.
rg -n "feature gate|featureGate|OCPFeatureGate" tools/codegen -g '*_test.go' -S

Repository: openshift/api

Length of output: 8310

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the tests around featureGateFromTestName and the nearby parser logic.
sed -n '1,140p' tools/codegen/cmd/featuregate-test-analyzer_test.go
sed -n '840,900p' tools/codegen/cmd/featuregate-test-analyzer.go

# Run a small read-only probe of the current regex/trim behavior on representative inputs.
python3 - <<'PY'
import re

featureGateRegex = re.compile(r"(\[OCPFeatureGate:.+\])")

samples = [
    "prefix [OCPFeatureGate:Example] suffix",
    "prefix [OCPFeatureGate:Example] [other] suffix",
    "prefix [OCPFeatureGate:Exam]ple] suffix",
    "prefix [OCPFeatureGate:[] suffix",
    "prefix [FeatureGate:Example] suffix",
]
for s in samples:
    m = featureGateRegex.search(s)
    gate = m.group(0) if m else ""
    out = gate.lstrip("[OCPFeatureGate:").rstrip("]") if gate else ""
    print(f"IN : {s}")
    print(f"MAT: {gate!r}")
    print(f"OUT: {out!r}")
    print()
PY

Repository: openshift/api

Length of output: 6164

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the call site and surrounding parsing logic.
sed -n '1100,1195p' tools/codegen/cmd/featuregate-test-analyzer.go

# Find where the failure string is built or sourced.
rg -n "featureGateFromTestName\(|failure\s*:|Failure|failed|OCPFeatureGate" tools/codegen/cmd/featuregate-test-analyzer.go tools/codegen/cmd/featuregate-test-analyzer_test.go -S

Repository: openshift/api

Length of output: 7642

Parse exactly one feature-gate annotation. The greedy .+ can consume later ] characters, and TrimLeft treats the prefix as a cutset, so gate names that start with F, G, O, etc. can be mangled and misclassified. Capture ([^\]]+) and return the capture directly.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 1181 - 1189, The
featureGateFromTestName parser can consume multiple annotations and corrupt gate
names because of the greedy regex and TrimLeft cutset behavior. Update
featureGateRegex to capture exactly one bracket-free annotation value using a
non-greedy boundary such as ([^\]]+), then have featureGateFromTestName return
the regex capture directly while preserving the empty-result behavior when no
annotation is found.

}
Comment on lines +1181 to +1190

@qodo-for-rh-openshift qodo-for-rh-openshift Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Featuregate parsing broken 🐞 Bug ≡ Correctness

featureGateFromTestName only recognizes "[OCPFeatureGate:...]" and uses TrimLeft/TrimRight cutsets,
so it can return the wrong gate name (or none) and incorrectly filter job-run failures during
promotion analysis.
Agent Prompt 
### Issue description
`featureGateFromTestName` is intended to extract a FeatureGate name from a test failure annotation, but it:
1) only matches `[OCPFeatureGate:...]` (ignores upstream `[FeatureGate:...]`), and
2) uses `strings.TrimLeft/TrimRight`, which operate on *cutsets* (not prefixes/suffixes), producing incorrect gate names.

This breaks the `defaultEnabledGates.Has(featureGate)` filtering and can incorrectly drop or keep failures.

### Issue Context
Repo documentation explicitly allows both `[OCPFeatureGate:<name>]` and upstream `[FeatureGate:<name>]` annotations, and the analyzer itself states it matches both.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[873-880]
- tools/codegen/cmd/featuregate-test-analyzer.go[1181-1190]

### Suggested implementation direction
- Replace the regex with a capturing group that supports both prefixes, e.g. `\[(?:OCPFeatureGate|FeatureGate):([^\]]+)\]`.
- Return the capture group (preferred), or use `strings.TrimPrefix`/`strings.TrimSuffix` (not `TrimLeft/TrimRight`).
- Add a small unit test table for representative strings (both prefixes; gate names starting with letters like `C`, `O`, etc.).

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


func getJobsForFeatureGateFromSippy(client *http.Client, release, featureGate string, variant JobVariant) ([]sippy.SippyJob, error) {
resp, err := client.Get(sippy.BuildSippyJobsForFeatureGateURL(featureGate, release, variant.Topology, variant.Cloud, variant.Architecture, variant.NetworkStack, variant.OS))
if err != nil {
Expand All @@ -1147,7 +1205,6 @@ func getJobsForFeatureGateFromSippy(client *http.Client, release, featureGate st
return nil, fmt.Errorf("reading response body: %w", err)
}


jobs := []sippy.SippyJob{}
err = json.Unmarshal(body, &jobs)
if err != nil {
Expand All @@ -1158,7 +1215,7 @@ func getJobsForFeatureGateFromSippy(client *http.Client, release, featureGate st
}

func getJobRunsFromSippy(client *http.Client, release, jobName string) ([]sippy.SippyJobRun, error) {
resp, err := client.Get(sippy.BuildSippyJobRunsForJobURL(release, jobName, time.Now().Add(-1 * 14 * 24 * time.Hour)))
resp, err := client.Get(sippy.BuildSippyJobRunsForJobURL(release, jobName, time.Now().Add(-1*14*24*time.Hour)))
if err != nil {
return nil, fmt.Errorf("getting job info: %w", err)
}
Expand All @@ -1173,7 +1230,6 @@ func getJobRunsFromSippy(client *http.Client, release, jobName string) ([]sippy.
return nil, fmt.Errorf("reading response body: %w", err)
}


runResults := &sippy.SippyJobRunsResult{}
err = json.Unmarshal(body, runResults)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion tools/codegen/cmd/featuregate-test-analyzer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func Test_listTestResultFor(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Skip("this is for ease of manual testing")

got, err := listTestResultFor(tt.args.featureGate, sets.New[string](tt.args.clusterProfile))
got, err := listTestResultFor(tt.args.featureGate, sets.New[string](tt.args.clusterProfile), sets.New[string]())
if (err != nil) != tt.wantErr {
t.Errorf("listTestResultFor() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down
Loading