Add configuration for Resolvers on version 1.22 and 1.23 - #82769
Add configuration for Resolvers on version 1.22 and 1.23#82769deekshith-24 wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe scaling pipeline adds resolver selection and sequential multi-scenario execution. The performance configuration adds 18 optional Git, bundle, and cluster resolver tests for downstream versions 1.22 and 1.23. ChangesScaling resolver scenarios
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ScalingPipeline
participant TektonCluster
participant LoadTest
participant ArtifactStorage
participant BenchmarkNamespaces
ScalingPipeline->>TektonCluster: Set up Tekton cluster
loop Each TEST_SCENARIOS entry
ScalingPipeline->>LoadTest: Run scenario with TEST_RESOLVER_TYPE
LoadTest-->>ArtifactStorage: Store scenario-specific results
ScalingPipeline->>BenchmarkNamespaces: Delete benchmark namespaces
ScalingPipeline->>ScalingPipeline: Wait before next scenario
end
ScalingPipeline-->>ScalingPipeline: Return aggregate status
Suggested labels: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deekshith-24 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 |
|
/test all |
|
/ok-to-test |
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-concurrency |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@deekshith-24: requesting more than one rehearsal in one comment is not supported. If you would like to rehearse multiple specific jobs, please separate the job names by a space in a single command. |
4 similar comments
|
@deekshith-24: requesting more than one rehearsal in one comment is not supported. If you would like to rehearse multiple specific jobs, please separate the job names by a space in a single command. |
|
@deekshith-24: requesting more than one rehearsal in one comment is not supported. If you would like to rehearse multiple specific jobs, please separate the job names by a space in a single command. |
|
@deekshith-24: requesting more than one rehearsal in one comment is not supported. If you would like to rehearse multiple specific jobs, please separate the job names by a space in a single command. |
|
@deekshith-24: requesting more than one rehearsal in one comment is not supported. If you would like to rehearse multiple specific jobs, please separate the job names by a space in a single command. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.sh (1)
75-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerial namespace cleanup can consume most of the step timeout.
cleanup_namespacesdeletes namespaces one at a time, each with--timeout=30m. The newTEST_SCENARIOSvalues inopenshift-pipelines-performance-main.yamlraiseTEST_NAMESPACEup to 10 (for example42/10/10/1), so a single stuck delete in the loop can now take up to 5 hours for one cleanup call, repeated after every scenario. This risks exceeding the 8h step timeout across a multi-scenario run.Delete namespaces in one batched call, or issue deletes without waiting and poll once, instead of looping with a full timeout per namespace.
♻️ Proposed fix: batch namespace deletion
cleanup_namespaces() { - for ns_idx in $(seq 1 "${TEST_NAMESPACE}"); do - ns_tag=$([ "$TEST_NAMESPACE" -eq 1 ] && echo "" || echo "$ns_idx") - oc delete --cascade=foreground --timeout=30m namespace "benchmark${ns_tag}" 2>/dev/null || true - done + local namespaces=() + for ns_idx in $(seq 1 "${TEST_NAMESPACE}"); do + ns_tag=$([ "$TEST_NAMESPACE" -eq 1 ] && echo "" || echo "$ns_idx") + namespaces+=("benchmark${ns_tag}") + done + oc delete --cascade=foreground --timeout=30m namespace "${namespaces[@]}" 2>/dev/null || true }🤖 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 `@ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.sh` around lines 75 - 80, Update cleanup_namespaces to delete all benchmark namespaces in a single batched oc delete invocation, or otherwise issue deletions asynchronously and perform one shared wait/poll. Preserve the existing namespace naming behavior for TEST_NAMESPACE, foreground cascading cleanup, timeout, and failure tolerance while eliminating the per-namespace 30-minute wait in the loop.
🤖 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
`@ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.sh`:
- Around line 93-97: Update the per-scenario cleanup block in the
scaling-pipelines command script to remove
tests/scaling-pipelines/cluster-benchmark-stats.csv alongside the existing
benchmark and stats files. Keep the reset behavior consistent with the analogous
max-concurrency script.
- Around line 71-104: Add per-scenario failure handling around the load-test and
result-collection commands inside the TEST_SCENARIOS loop so errexit does not
terminate the loop on a failed scenario. Ensure failed scenarios still run
ARTIFACT_DIR="$run_artifacts" ./ci-scripts/collect-results.sh and
cleanup_namespaces, then continue processing subsequent scenarios while
preserving the failure status for reporting.
- Around line 83-86: Update the scenario parsing in the current loop to assign
each of the four extracted fields to its variable before exporting it, avoiding
masked command-substitution statuses and fixing SC2155. Validate that every
scenario contains all four slash-delimited fields, and fail immediately with a
clear error when fields are missing instead of allowing empty TEST_NAMESPACE or
TEST_BIGBANG_MULTI_STEP__STEP_COUNT values to reach downstream commands.
---
Nitpick comments:
In
`@ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.sh`:
- Around line 75-80: Update cleanup_namespaces to delete all benchmark
namespaces in a single batched oc delete invocation, or otherwise issue
deletions asynchronously and perform one shared wait/poll. Preserve the existing
namespace naming behavior for TEST_NAMESPACE, foreground cascading cleanup,
timeout, and failure tolerance while eliminating the per-namespace 30-minute
wait in the loop.
🪄 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: Pro Plus
Run ID: 260209a0-d568-4ec6-9b0e-8bdbb1551422
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift-pipelines/performance/openshift-pipelines-performance-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (3)
ci-operator/config/openshift-pipelines/performance/openshift-pipelines-performance-main.yamlci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.shci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-ref.yaml
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-steps |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
93433e0 to
7ec7b7d
Compare
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-steps |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.sh`:
- Around line 84-92: Update the scenario validation around the IFS read to
enforce exactly four slash-delimited fields and require each field to be numeric
before exporting TEST_TOTAL, TEST_CONCURRENT, TEST_NAMESPACE, and
TEST_BIGBANG_MULTI_STEP__STEP_COUNT. Reject inputs with extra fields or
non-numeric values using the existing malformed-scenario error path.
🪄 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: Pro Plus
Run ID: 10179ad1-2eb4-400c-b79a-efb7f42b95c8
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift-pipelines/performance/openshift-pipelines-performance-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (3)
ci-operator/config/openshift-pipelines/performance/openshift-pipelines-performance-main.yamlci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.shci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-ref.yaml
- ci-operator/config/openshift-pipelines/performance/openshift-pipelines-performance-main.yaml
| IFS='/' read -r t c n s <<< "$scenario" | ||
| if [[ -z "$t" || -z "$c" || -z "$n" || -z "$s" ]]; then | ||
| echo "[ERROR] Malformed scenario '$scenario': expected total/concurrent/namespace/steps" | ||
| exit 1 | ||
| fi | ||
| export TEST_TOTAL="$t" | ||
| export TEST_CONCURRENT="$c" | ||
| export TEST_NAMESPACE="$n" | ||
| export TEST_BIGBANG_MULTI_STEP__STEP_COUNT="$s" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the exact scenario field count, not just non-emptiness.
read -r t c n s assigns any extra /-delimited content, including the delimiters, to the last variable s when the input has more than 4 fields. A scenario like "1/2/3/4/5" passes the emptiness check at line 85, but sets TEST_BIGBANG_MULTI_STEP__STEP_COUNT to "4/5". This corrupts the field silently instead of failing with a clear error. Use a format check that also enforces the fields are numeric.
🐛 Proposed fix
- IFS='/' read -r t c n s <<< "$scenario"
- if [[ -z "$t" || -z "$c" || -z "$n" || -z "$s" ]]; then
- echo "[ERROR] Malformed scenario '$scenario': expected total/concurrent/namespace/steps"
- exit 1
- fi
+ if [[ ! "$scenario" =~ ^[0-9]+/[0-9]+/[0-9]+/[0-9]+$ ]]; then
+ echo "[ERROR] Malformed scenario '$scenario': expected total/concurrent/namespace/steps (all numeric)"
+ exit 1
+ fi
+ IFS='/' read -r t c n s <<< "$scenario"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IFS='/' read -r t c n s <<< "$scenario" | |
| if [[ -z "$t" || -z "$c" || -z "$n" || -z "$s" ]]; then | |
| echo "[ERROR] Malformed scenario '$scenario': expected total/concurrent/namespace/steps" | |
| exit 1 | |
| fi | |
| export TEST_TOTAL="$t" | |
| export TEST_CONCURRENT="$c" | |
| export TEST_NAMESPACE="$n" | |
| export TEST_BIGBANG_MULTI_STEP__STEP_COUNT="$s" | |
| if [[ ! "$scenario" =~ ^[0-9]+/[0-9]+/[0-9]+/[0-9]+$ ]]; then | |
| echo "[ERROR] Malformed scenario '$scenario': expected total/concurrent/namespace/steps (all numeric)" | |
| exit 1 | |
| fi | |
| IFS='/' read -r t c n s <<< "$scenario" | |
| export TEST_TOTAL="$t" | |
| export TEST_CONCURRENT="$c" | |
| export TEST_NAMESPACE="$n" | |
| export TEST_BIGBANG_MULTI_STEP__STEP_COUNT="$s" |
🤖 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
`@ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.sh`
around lines 84 - 92, Update the scenario validation around the IFS read to
enforce exactly four slash-delimited fields and require each field to be numeric
before exporting TEST_TOTAL, TEST_CONCURRENT, TEST_NAMESPACE, and
TEST_BIGBANG_MULTI_STEP__STEP_COUNT. Reject inputs with extra fields or
non-numeric values using the existing malformed-scenario error path.
7ec7b7d to
06746f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@ci-operator/config/openshift-pipelines/performance/openshift-pipelines-performance-main.yaml`:
- Line 2414: Replace the six TEST_CLUSTER_RESOLVER__TYPE environment variable
keys in the affected performance jobs with TEST_RESOLVER_TYPE, keeping each
value set to cluster so the declared resolver selection variable is populated.
🪄 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: Pro Plus
Run ID: e16b410c-e742-406c-8d30-5c1a1dedda4d
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift-pipelines/performance/openshift-pipelines-performance-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (3)
ci-operator/config/openshift-pipelines/performance/openshift-pipelines-performance-main.yamlci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.shci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-ref.yaml
- ci-operator/step-registry/openshift-pipelines/scaling-pipelines/openshift-pipelines-scaling-pipelines-commands.sh
| env: | ||
| DEPLOYMENT_VERSION: "1.22" | ||
| MUST_GATHER_TIMEOUT: 35m | ||
| TEST_CLUSTER_RESOLVER__TYPE: cluster |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the declared resolver environment variable.
TEST_CLUSTER_RESOLVER__TYPE is not defined by the scaling-step ref. These six jobs leave TEST_RESOLVER_TYPE unset. They can therefore run with the default resolver instead of the cluster resolver.
Replace each key with TEST_RESOLVER_TYPE: cluster.
Proposed fix
- TEST_CLUSTER_RESOLVER__TYPE: cluster
+ TEST_RESOLVER_TYPE: clusterThe upstream ref declares TEST_RESOLVER_TYPE for resolver selection.
Also applies to: 2426-2426, 2438-2438, 2450-2450, 2462-2462, 2474-2474
🤖 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
`@ci-operator/config/openshift-pipelines/performance/openshift-pipelines-performance-main.yaml`
at line 2414, Replace the six TEST_CLUSTER_RESOLVER__TYPE environment variable
keys in the affected performance jobs with TEST_RESOLVER_TYPE, keeping each
value set to cluster so the declared resolver selection variable is populated.
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-steps |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-br-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-br-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-br-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-br-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-br-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-br-steps |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-cr-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-cr-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-cr-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-cr-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-cr-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-cr-steps |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/retest |
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-br-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-cr-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-cr-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-steps pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-concurrency |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
…ession jobs Add multi-scenario loop to scaling-pipelines workflow so multiple parameter combinations (total/concurrent/namespace/steps) run on a single cluster, reducing provisioning overhead from 24 to 6 jobs. - Add TEST_SCENARIOS and TEST_RESOLVER_TYPE env vars to ref.yaml - Add scenario loop with per-iteration artifact collection and cleanup - Add 6 git resolver regression jobs (1.22 + 1.23) for concurrency, namespace, and step-count sweeps Co-authored-by: Cursor <cursoragent@cursor.com>
06746f1 to
a4655ae
Compare
|
/pj-rehearse pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-git-resolver-concurrency pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-br-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-br-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-22-cr-namespaces pull-ci-openshift-pipelines-performance-main-scaling-pipelines-downstream-1-23-cr-namespaces |
|
@deekshith-24: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
A total of 62 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@deekshith-24: all tests passed! 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. |
Summary by CodeRabbit
TEST_SCENARIOSandTEST_RESOLVER_TYPE.