Skip to content

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

Open
everettraven wants to merge 2 commits into
openshift:masterfrom
everettraven:tooling/installer-fp
Open

WIP: NO-JIRA: tooling: update job-based feature promotion analysis#2944
everettraven wants to merge 2 commits into
openshift:masterfrom
everettraven:tooling/installer-fp

Conversation

@everettraven

Copy link
Copy Markdown
Contributor

No description provided.

so that infrastructure failures are not included,
techpreview feature tests failing are not included,
and tests across all jobs must pass at least 92% of the time.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 22, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@everettraven: This pull request explicitly references no jira issue.

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci

openshift-ci Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Hello @everettraven! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

AWSDualStackInstall is enabled for default and OKD configurations, with corresponding feature table and payload manifest updates. The feature-gate test analyzer now tracks default-enabled gates, propagates them through job-result validation, filters infrastructure and non-default gate failures, and recalculates run counts. Sippy job-run data now includes an infrastructure-failure field.

Possibly related PRs

  • openshift/api#2943: Changes the same feature-gate analyzer call chain and test-result handling.

Suggested reviewers: joelspeed, mtulio, eggfoobar

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No pull request description was provided, so relevance to the changeset can't be evaluated. Add a brief description of the tooling changes and intended behavior.
✅ Passed checks (14 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No Ginkgo titles were added or changed; the only test edit is a static t.Run table-test callsite, not a dynamic test name.
Test Structure And Quality ✅ Passed PASS: The only changed test is a table-driven unit test using testing; no Ginkgo, cluster ops, Eventually/Consistently, or cleanup concerns appear.
Microshift Test Compatibility ✅ Passed PASS: the diff only changes feature-gate docs/config; no new Ginkgo e2e tests were added, and no MicroShift-unsupported APIs or guards appear in changed files.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new or changed Ginkgo e2e tests are in this diff; it only updates feature-gate docs/manifests and a feature registration.
Topology-Aware Scheduling Compatibility ✅ Passed The PR only adjusts AWSDualStackInstall feature-gate enablement/listing; no pod specs, affinity, nodeSelectors, replicas, or topology scheduling logic changed.
Ote Binary Stdout Contract ✅ Passed No process-level stdout writes were added; the new fmt.Printf calls are in helper functions, and touched init/main code only wires Cobra streams.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the only modified test file is standard Go unit tests and shows no IPv4 or external-connectivity assumptions.
No-Weak-Crypto ✅ Passed Patch only changes feature-gate docs/manifests; no MD5/SHA1/DES/RC4/3DES/Blowfish, custom crypto, or secret comparisons found.
Container-Privileges ✅ Passed PR only moves AWSDualStackInstall between feature-gate lists; no touched file adds privileged, host*, SYS_ADMIN, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs ✅ Passed HEAD diff only touches feature docs/YAMLs; no log statements or sensitive-data fields appear in the changed lines.
Title check ✅ Passed The title accurately reflects the main change: tooling updates for job-based feature promotion analysis.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 22, 2026
@qodo-for-rh-openshift

Copy link
Copy Markdown

PR Summary by Qodo

Tooling: harden feature gate promotion analysis and promote AWS DualStack

✨ Enhancement ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Exclude infrastructure-only failures from job-based promotion pass-rate calculations.
• Ignore failures from non-default (e.g., TechPreview) feature-gated tests during promotion checks.
• Promote AWSDualStackInstall to Default/OKD and update manifests/docs accordingly.
Diagram

graph TD
  A["FeatureGate YAMLs"] --> C["Promotion analyzer"] --> D{{"Sippy API"}} --> C --> E["Promotion summary"] --> F["Default enablement"]
  B["Feature registry"] --> C
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Filter at query-time in Sippy (where supported)
  • ➕ Reduces client-side logic and risk of miscounting TotalRuns vs skipped runs
  • ➕ Makes promotion criteria more transparent and reproducible by sharing query URLs/filters
  • ➖ Depends on Sippy API supporting the needed filters (infra failures, gate annotations)
  • ➖ May require Sippy schema/endpoint changes outside this repo
2. Compute failing run set (union) instead of max per-test failures
  • ➕ More accurate job-level failed-run count when different tests fail in different runs
  • ➕ Avoids undercounting failures when multiple low-pass-rate tests fail on disjoint runs
  • ➖ Slightly more complex bookkeeping (track run IDs/URLs to a failing set)
  • ➖ May increase memory/processing for 100-run windows

Recommendation: Current approach is directionally correct and improves signal quality (drops infra noise and non-default gate failures). If reviewers are concerned about accuracy of the new FailedRuns calculation, consider switching from "max failureCount" to tracking the union of failing runs; otherwise this is acceptable as a conservative approximation given the 14-run minimum and the 92% threshold rationale.

Files changed (9) +104 / -48

Bug fix (2) +89 / -33
featuregate-test-analyzer.goRefine job-based promotion checks (infra + TP filtering, 92% rule) +82/-26

Refine job-based promotion checks (infra + TP filtering, 92% rule)

• Adds a job-based required per-test pass rate (0.92) and updates job promotion verification to (1) skip infrastructure-failure job runs and (2) ignore failures tied to feature gates not enabled by Default. Refactors the job-run analysis to aggregate failures per test across runs and derive job FailedRuns/SuccessfulRuns from the worst offending test relative to the 92% threshold.

tools/codegen/cmd/featuregate-test-analyzer.go

json_types.goAdd infrastructure_failure to Sippy job run model +7/-7

Add infrastructure_failure to Sippy job run model

• Extends SippyJobRun with an InfrastructureFailure boolean to support skipping infra-only failures during promotion analysis.

tools/codegen/pkg/sippy/json_types.go

Tests (1) +1 / -1
featuregate-test-analyzer_test.goUpdate analyzer test call signature for new default-enabled gate set +1/-1

Update analyzer test call signature for new default-enabled gate set

• Updates the listTestResultFor invocation to pass the new defaultEnabledFeatureGates argument (empty set for the manual/ignored test).

tools/codegen/cmd/featuregate-test-analyzer_test.go

Documentation (1) +1 / -1
features.mdReposition AWSDualStackInstall in feature status table +1/-1

Reposition AWSDualStackInstall in feature status table

• Moves the AWSDualStackInstall row within the rendered feature table to match the updated enablement grouping/order. No status values change; it remains shown as enabled across listed releases/profiles.

features.md

Other (5) +13 / -13
features.goEnable AWSDualStackInstall in Default and OKD +1/-1

Enable AWSDualStackInstall in Default and OKD

• Updates the AWSDualStackInstall feature gate registration to be enabled for inDefault() and inOKD() in addition to TechPreview/DevPreview. This effectively promotes the gate into the default/OKD feature sets.

features/features.go

featureGate-4-10-Hypershift-Default.yamlPromote AWSDualStackInstall within 4.10 Hypershift Default set +3/-3

Promote AWSDualStackInstall within 4.10 Hypershift Default set

• Removes AWSDualStackInstall from its prior position and re-adds it later in the enabled list (near other *Install gates), reflecting the promotion/ordering update for the 4.10 Hypershift Default manifest.

payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml

featureGate-4-10-Hypershift-OKD.yamlPromote AWSDualStackInstall within 4.10 Hypershift OKD set +3/-3

Promote AWSDualStackInstall within 4.10 Hypershift OKD set

• Adjusts the enabled feature list to include AWSDualStackInstall in the updated location consistent with other install-related gates for 4.10 Hypershift OKD.

payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml

featureGate-4-10-SelfManagedHA-Default.yamlPromote AWSDualStackInstall within 4.10 SelfManagedHA Default set +3/-3

Promote AWSDualStackInstall within 4.10 SelfManagedHA Default set

• Updates the enabled features list to reflect AWSDualStackInstall promotion/ordering for the 4.10 SelfManagedHA Default manifest.

payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml

featureGate-4-10-SelfManagedHA-OKD.yamlPromote AWSDualStackInstall within 4.10 SelfManagedHA OKD set +3/-3

Promote AWSDualStackInstall within 4.10 SelfManagedHA OKD set

• Updates the enabled features list to reflect AWSDualStackInstall promotion/ordering for the 4.10 SelfManagedHA OKD manifest.

payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml

@openshift-ci
openshift-ci Bot requested review from JoelSpeed and deads2k July 22, 2026 17:50
@openshift-ci

openshift-ci Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)

1102-1119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Exclude infrastructure runs from the denominator.

TotalRuns is set from all runs before infrastructure failures are skipped. Those runs therefore lower pass rates despite being excluded from failure analysis, which can incorrectly block or classify promotion.

Proposed fix
 testResults := &TestResults{
-    TestName:  job.Name,
-    TotalRuns: len(jobRuns),
+    TestName: job.Name,
 }

 for _, jobRun := range jobRuns {
     if jobRun.InfrastructureFailure {
         continue
     }
+    testResults.TotalRuns++
🤖 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 1102 - 1119,
Update the TestResults initialization in the job-run analysis flow so TotalRuns
counts only non-infrastructure runs. Adjust the count while iterating in the
loop that checks jobRun.InfrastructureFailure, ensuring skipped infrastructure
runs are excluded from the denominator while preserving the existing
failure-analysis behavior.
🧹 Nitpick comments (1)
tools/codegen/cmd/featuregate-test-analyzer_test.go (1)

68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add executable coverage for the new filtering rules.

This remains a skipped live-call test, so infrastructure exclusion and default-gate parsing have no CI coverage. Add table-driven tests for the pure failure-classification/run-counting logic.

🤖 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_test.go` at line 68, Add
executable, table-driven unit tests for the pure failure-classification and
run-counting logic used by listTestResultFor, covering infrastructure exclusions
and default-gate parsing without live calls. Remove the skipped-only coverage
gap while preserving the existing test setup and expected filtering behavior.
🤖 Prompt for all review comments with 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.

Inline comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 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.
- Around line 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.

---

Outside diff comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1102-1119: Update the TestResults initialization in the job-run
analysis flow so TotalRuns counts only non-infrastructure runs. Adjust the count
while iterating in the loop that checks jobRun.InfrastructureFailure, ensuring
skipped infrastructure runs are excluded from the denominator while preserving
the existing failure-analysis behavior.

---

Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer_test.go`:
- Line 68: Add executable, table-driven unit tests for the pure
failure-classification and run-counting logic used by listTestResultFor,
covering infrastructure exclusions and default-gate parsing without live calls.
Remove the skipped-only coverage gap while preserving the existing test setup
and expected filtering behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: dbf150a5-e337-4db9-b4a8-8ae4e9c5c8be

📥 Commits

Reviewing files that changed from the base of the PR and between 581cfdf and 6672b5f.

📒 Files selected for processing (9)
  • features.md
  • features/features.go
  • payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
  • payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
  • payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
  • payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
  • tools/codegen/cmd/featuregate-test-analyzer.go
  • tools/codegen/cmd/featuregate-test-analyzer_test.go
  • tools/codegen/pkg/sippy/json_types.go

Comment on lines +1161 to +1176
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
}
}
}

testResults.FailedRuns = jobsFailed
testResults.SuccessfulRuns = testResults.TotalRuns - jobsFailed

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 +1181 to +1189
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:"), "]")

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.

@qodo-for-rh-openshift

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 29 rules
✅ Skills: api-review

Grey Divider


Action required

1. FeatureGate parsing broken 🐞 Bug ≡ Correctness
Description
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.
Code

tools/codegen/cmd/featuregate-test-analyzer.go[R1181-1190]

+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:"), "]")
+}
Relevance

⭐⭐⭐ High

Promotion-analysis correctness bugs have been fixed historically; parser mismatch with documented
patterns likely accepted.

PR-#2763
PR-#2903

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The repo explicitly allows both [OCPFeatureGate:...] and [FeatureGate:...], and the analyzer
says its pattern matches both, but the new regex only matches OCPFeatureGate and the trimming logic
can corrupt extracted names.

README.md[90-103]
tools/codegen/cmd/featuregate-test-analyzer.go[873-880]
tools/codegen/cmd/featuregate-test-analyzer.go[1181-1190]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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


2. Infra failures inflate pass rate 🐞 Bug ≡ Correctness
Description
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.
Code

tools/codegen/cmd/featuregate-test-analyzer.go[R1109-1119]

		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
+		}
Relevance

⭐⭐⭐ High

Analyzer correctness/aggregation fixes were accepted before (pass-rate totals/merging). Similar
change likely welcomed.

PR-#2763
PR-#2903

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The analyzer sets TotalRuns to the full number of fetched runs, then skips infra failures without
adjusting totals, and later computes SuccessfulRuns from TotalRuns; the Sippy type shows the
infra-failure field being used for this skip.

tools/codegen/cmd/featuregate-test-analyzer.go[1097-1105]
tools/codegen/cmd/featuregate-test-analyzer.go[1114-1119]
tools/codegen/cmd/featuregate-test-analyzer.go[1175-1177]
tools/codegen/pkg/sippy/json_types.go[67-75]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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


3. Failed runs undercounted 🐞 Bug ≡ Correctness
Description
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.
Code

tools/codegen/cmd/featuregate-test-analyzer.go[R1112-1177]

+	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
+		}

+		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
+			}
+		}
	}

+	testResults.FailedRuns = jobsFailed
+	testResults.SuccessfulRuns = testResults.TotalRuns - jobsFailed
+
Relevance

⭐⭐ Medium

No prior review evidence on counting failed runs vs per-test max; correctness fixes usually accepted
but unclear.

PR-#2763
PR-#2903

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The code collapses run-level failures into a per-test count map and then reduces that to a single
maximum value for jobsFailed, which cannot represent distinct failing runs when multiple tests
fail in different runs.

tools/codegen/cmd/featuregate-test-analyzer.go[1112-1177]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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



Informational

4. Misformatted infra skip log 🐞 Bug ◔ Observability
Description
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.
Code

tools/codegen/cmd/featuregate-test-analyzer.go[R1115-1118]

+		// 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
Relevance

⭐⭐⭐ High

Team has accepted/flagged Go formatting/printing issues in analyzer area; fmt.Println format misuse
likely fixed.

PR-#2903

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The current call uses fmt.Println("...%s...", url); fmt.Println does not apply formatting
directives, so the message won’t include the URL in the intended location.

tools/codegen/cmd/featuregate-test-analyzer.go[1115-1118]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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


Grey Divider

Qodo Logo

Comment on lines +1181 to +1190
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:"), "]")
}

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

Comment on lines +1112 to +1177
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
}

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

testResults.FailedRuns = jobsFailed
testResults.SuccessfulRuns = testResults.TotalRuns - jobsFailed

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

Comment on lines 1109 to +1119
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
}

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

Comment on lines +1115 to +1118
// 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

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

@everettraven everettraven changed the title NO-JIRA: tooling: update job-based feature promotion analysis WIP: NO-JIRA: tooling: update job-based feature promotion analysis Jul 22, 2026
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2026
@openshift-ci

openshift-ci Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

@everettraven: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/integration 6672b5f link true /test integration
ci/prow/verify-feature-promotion 6672b5f link true /test verify-feature-promotion
ci/prow/verify-hypershift-integration 6672b5f link true /test verify-hypershift-integration
ci/prow/verify 6672b5f link true /test verify

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

if jobsFailed < failureCount {
jobsFailed = failureCount
}
}

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants