Export BEARER_TOKEN and build kube-burner-ocp from fork for kube-apiserver pprof#82349
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
WalkthroughThe six density workload scripts now enable fixed-interval pprof polling, provision kube-burner profiling access, build a custom kube-burner binary with kube-apiserver targets, and export the resulting credentials and local artifact URL. Perfscale CI also adds a build-validation step and updates its workload chain. ChangesDensity profiling workflows
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DensityScript
participant OpenShiftCluster
participant CustomBuild
participant KubeBurner
DensityScript->>OpenShiftCluster: Provision profiling RBAC
OpenShiftCluster-->>DensityScript: Return BEARER_TOKEN
DensityScript->>CustomBuild: Clone and build kube-burner-ocp
CustomBuild-->>DensityScript: Return packaged local tarball
DensityScript->>KubeBurner: Export KUBE_BURNER_URL and pprof flags
KubeBurner->>OpenShiftCluster: Run workload with authenticated pprof collection
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: redhat-chai-bot 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh (2)
84-95: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAll six scripts build
kube-burner-ocpfrom an unpinned branch on an unofficial fork with a hardcodedbin/amd64/output path. Cloningadd-kube-apiserver-pprof-targetsby branch name (not commit SHA) means the build can change or break unexpectedly if the branch is amended/force-pushed, and the hardcoded arch path assumes a fixed Makefile output layout.
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh#L84-L95: pinFORK_BRANCH/clone to a specific commit SHA and confirm the runner image hasgit/make/Go available.ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh#L71-L82: same pinning/verification.ci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.sh#L66-L77: same pinning/verification.ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh#L74-L85: same pinning/verification.ci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.sh#L77-L88: same pinning/verification.ci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh#L87-L98: same pinning/verification.🤖 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-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh` around lines 84 - 95, Pin FORK_BRANCH in the kube-burner-ocp build flow to a specific commit SHA and clone that immutable revision; replace the hardcoded bin/amd64/kube-burner-ocp lookup with a validated output path or discovery compatible with the build. Confirm git, make, and Go are available before building. Apply the same changes in ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh lines 84-95, node-density-cni/openshift-qe-node-density-cni-commands.sh lines 71-82, node-density-heavy/openshift-qe-node-density-heavy-commands.sh lines 66-77, node-density/openshift-qe-node-density-commands.sh lines 74-85, cudn-density/openshift-qe-cudn-density-commands.sh lines 77-88, and udn-density-pods/openshift-qe-udn-density-pods-commands.sh lines 87-98.
79-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlanket
|| trueon ns/SA/CRB creation masks real failures, not just AlreadyExists, in all six scripts. A genuine RBAC/API failure here silently propagates into a broken/emptyBEARER_TOKENlater, making failures much harder to diagnose.
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh#L79-L81: check for existing resources explicitly (e.g.oc get ... || oc create ...) instead of a blind|| true.ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh#L66-L68: same fix.ci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.sh#L61-L63: same fix.ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh#L69-L71: same fix.ci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.sh#L72-L74: same fix.ci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh#L82-L84: same fix.🤖 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-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh` around lines 79 - 81, Replace the blanket “|| true” handling for namespace, service account, and cluster role binding creation with explicit existence checks followed by creation only when absent, while allowing genuine oc/API failures to propagate. Apply this consistently at the resource-creation blocks in ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh:79-81, ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh:66-68, ci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.sh:61-63, ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh:69-71, ci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.sh:72-74, and ci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh:82-84.
🤖 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-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh`:
- Around line 78-81: The kube-burner service account currently receives
cluster-admin solely for kube-apiserver pprof access. In all six
scripts—ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh:78-81,
node-density-cni/openshift-qe-node-density-cni-commands.sh:65-68,
node-density-heavy/openshift-qe-node-density-heavy-commands.sh:60-63,
node-density/openshift-qe-node-density-commands.sh:68-71,
cudn-density/openshift-qe-cudn-density-commands.sh:71-74, and
udn-density-pods/openshift-qe-udn-density-pods-commands.sh:81-84—replace the
cluster-admin binding with a dedicated ClusterRole granting only get on
nonResourceURLs /debug/pprof/* and a corresponding ClusterRoleBinding for the
kube-burner service account.
- Around line 78-82: The BEARER_TOKEN assignment is exposed by shell tracing and
also violates SC2155. In
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh
lines 78-82, disable tracing before token generation, declare BEARER_TOKEN
separately from the command substitution, then re-enable tracing afterward.
Apply the same set +x/set -x wrapping to the BEARER_TOKEN assignment in
ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh
lines 65-69,
ci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.sh
lines 60-64,
ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh
lines 68-72,
ci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.sh
lines 71-75, and
ci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh
lines 81-85.
---
Nitpick comments:
In
`@ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh`:
- Around line 84-95: Pin FORK_BRANCH in the kube-burner-ocp build flow to a
specific commit SHA and clone that immutable revision; replace the hardcoded
bin/amd64/kube-burner-ocp lookup with a validated output path or discovery
compatible with the build. Confirm git, make, and Go are available before
building. Apply the same changes in
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh
lines 84-95, node-density-cni/openshift-qe-node-density-cni-commands.sh lines
71-82, node-density-heavy/openshift-qe-node-density-heavy-commands.sh lines
66-77, node-density/openshift-qe-node-density-commands.sh lines 74-85,
cudn-density/openshift-qe-cudn-density-commands.sh lines 77-88, and
udn-density-pods/openshift-qe-udn-density-pods-commands.sh lines 87-98.
- Around line 79-81: Replace the blanket “|| true” handling for namespace,
service account, and cluster role binding creation with explicit existence
checks followed by creation only when absent, while allowing genuine oc/API
failures to propagate. Apply this consistently at the resource-creation blocks
in
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh:79-81,
ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh:66-68,
ci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.sh:61-63,
ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh:69-71,
ci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.sh:72-74,
and
ci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh:82-84.
🪄 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: 28805ad0-8fd2-4224-b79d-4712c9f28e6e
📒 Files selected for processing (6)
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.shci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.shci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.shci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.shci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.shci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh
| # Setup pprof secrets for kube-apiserver profiling | ||
| oc create ns benchmark-operator || true | ||
| oc create serviceaccount kube-burner -n benchmark-operator || true | ||
| oc create clusterrolebinding kube-burner-crb --clusterrole=cluster-admin --serviceaccount=benchmark-operator:kube-burner || true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
All six scripts bind cluster-admin to the kube-burner SA solely to scrape kube-apiserver pprof. A ClusterRole scoped to nonResourceURLs: ["/debug/pprof/*"] with verb: get would be sufficient and avoids granting full cluster-admin to a token that lives in benchmark-operator.
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh#L78-L81: replace thecluster-adminclusterrolebinding with a scoped pprof ClusterRole/binding.ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh#L65-L68: same replacement.ci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.sh#L60-L63: same replacement.ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh#L68-L71: same replacement.ci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.sh#L71-L74: same replacement.ci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh#L81-L84: same replacement.
📍 Affects 6 files
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh#L78-L81(this comment)ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh#L65-L68ci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.sh#L60-L63ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh#L68-L71ci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.sh#L71-L74ci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh#L81-L84
🤖 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-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh`
around lines 78 - 81, The kube-burner service account currently receives
cluster-admin solely for kube-apiserver pprof access. In all six
scripts—ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.sh:78-81,
node-density-cni/openshift-qe-node-density-cni-commands.sh:65-68,
node-density-heavy/openshift-qe-node-density-heavy-commands.sh:60-63,
node-density/openshift-qe-node-density-commands.sh:68-71,
cudn-density/openshift-qe-cudn-density-commands.sh:71-74, and
udn-density-pods/openshift-qe-udn-density-pods-commands.sh:81-84—replace the
cluster-admin binding with a dedicated ClusterRole granting only get on
nonResourceURLs /debug/pprof/* and a corresponding ClusterRoleBinding for the
kube-burner service account.
|
/pj-rehearse periodic-ci-openshift-eng-ocp-perfscale-main-aws-5.0-nightly-x86-payload-control-plane-6nodes |
|
@mcornea: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
f2e2f7b to
67afbaf
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh (1)
108-117: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPoint the timeout recovery check at the custom binary path.
The script ships the built binary from
/tmp/kube-burner-ocp-bin/kube-burner-ocpintofile:///tmp/kube-burner-ocp-custom.tar.gz. If the wrapper leaves that tarball unpacked or extracts to a non-executable location,if /tmp/kube-burner-ocp cluster-health; thensilently fails onrun.shexit code 2 and skips the intended health-check fallback. Match the health check to the actual executable path used byKUBE_BURNER_URL.🤖 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-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh` around lines 108 - 117, Update the cluster-health invocation in the RUN_EXIT_CODE 2 recovery block to use the executable path configured through KUBE_BURNER_URL, specifically the custom binary location used by the shipped kube-burner artifact, instead of /tmp/kube-burner-ocp. Preserve the existing health-check fallback and cleanup 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.
Outside diff comments:
In
`@ci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.sh`:
- Around line 108-117: Update the cluster-health invocation in the RUN_EXIT_CODE
2 recovery block to use the executable path configured through KUBE_BURNER_URL,
specifically the custom binary location used by the shipped kube-burner
artifact, instead of /tmp/kube-burner-ocp. Preserve the existing health-check
fallback and cleanup behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 859c8ea5-ffd9-4eff-8ca8-093ebf4b7397
📒 Files selected for processing (6)
ci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.shci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.shci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.shci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.shci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.shci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh
|
/pj-rehearse periodic-ci-openshift-eng-ocp-perfscale-main-aws-5.0-nightly-x86-payload-control-plane-6nodes |
|
@mcornea: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
67afbaf to
45bbd1f
Compare
|
/pj-rehearse periodic-ci-openshift-eng-ocp-perfscale-main-aws-5.0-nightly-x86-payload-control-plane-6nodes |
|
@mcornea: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-ocp-perfscale-main-aws-5.0-nightly-x86-payload-control-plane-6nodes |
|
@mcornea: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
45bbd1f to
b344ce2
Compare
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
b344ce2 to
da19004
Compare
openshift-ci
Bot
removed
the
do-not-merge/invalid-owners-file
da19004 to
7f16348
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh`:
- Around line 8-9: Replace the mutable FORK_BRANCH-based kube-burner fork
references with one immutable commit ref, and use that identical pinned ref in
build-kube-burner-test at
ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh
lines 8-9 and 22-24, plus node-density at
ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh
lines 78-82. Update the clone/build and workload execution commands consistently
so both use the same pinned source before building and running.
- Around line 13-16: Validate the downloaded Go tarball before extraction in
both
ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh
(lines 13-16) and
ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh
(lines 84-88). Add download failure handling and verify each archive against the
official checksum before allowing tar to extract it, or centralize the
installation while preserving uniform integrity validation across both
workflows.
In
`@ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh`:
- Around line 68-72: Update the pprof setup near the kube-burner service account
to define and bind a dedicated least-privilege role instead of using the
cluster-admin ClusterRole. Grant only the exact non-resource or proxy
permissions required by the configured custom profiling targets, and update
kube-burner-crb to reference that dedicated role.
- Around line 69-72: Update the benchmark-operator RBAC setup commands around
the kube-burner service account and cluster role binding to stop unconditionally
swallowing errors. Use an idempotent apply approach or explicitly ignore only
expected AlreadyExists responses, while propagating forbidden, API outage, and
other failures so the workload does not start with incomplete RBAC.
- Around line 73-74: Update the BEARER_TOKEN assignment to fall back to oc sa
get-token only when oc create token is genuinely unsupported or unavailable, not
when it fails due to authorization or API errors. Preserve the 6-hour token
semantics whenever oc create token succeeds, propagate other failures so CI
stops, and add an explicit comment documenting the narrowly scoped fallback.
🪄 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: c8e53f34-a1ec-4ca9-8bfb-e04c6002ff7b
📒 Files selected for processing (10)
ci-operator/config/openshift-eng/ocp-perfscale/openshift-eng-ocp-perfscale-main__aws-5.0-nightly-x86.yamlci-operator/step-registry/openshift-qe/build-kube-burner-test/OWNERSci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.shci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-ref.yamlci-operator/step-registry/openshift-qe/cluster-density-v2/openshift-qe-cluster-density-v2-commands.shci-operator/step-registry/openshift-qe/cudn-density/openshift-qe-cudn-density-commands.shci-operator/step-registry/openshift-qe/node-density-cni/openshift-qe-node-density-cni-commands.shci-operator/step-registry/openshift-qe/node-density-heavy/openshift-qe-node-density-heavy-commands.shci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.shci-operator/step-registry/openshift-qe/udn-density-pods/openshift-qe-udn-density-pods-commands.sh
| FORK_REPO="https://github.com/redhat-chai-bot/kube-burner_kube-burner-ocp.git" | ||
| FORK_BRANCH="add-kube-apiserver-pprof-targets" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'openshift-qe-(build-kube-burner-test|node-density)-commands\.sh$' . || true
echo
echo "== relevant build-kube-burner-test lines =="
file1='ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh'
if [ -f "$file1" ]; then
wc -l "$file1"
sed -n '1,80p' "$file1" | cat -n | sed -n '1,90p'
fi
echo
echo "== relevant node-density lines =="
file2='ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh'
if [ -f "$file2" ]; then
wc -l "$file2"
sed -n '1,140p' "$file2" | cat -n | sed -n '1,150p'
fi
echo
echo "== search kube-burner build variables/usages =="
rg -n "FORK_REPO|FORK_BRANCH|kb.*clone|kube-burner|kube-burner-ocp|Build|GOFLAGS|GOCACHE|mktemp -d|KUBE_BURNER|KB_OCP" ci-operator/step-registry/openshift-qe -S || trueRepository: openshift/release
Length of output: 50373
Pin the kube-burner-o fork at immutable commit refs.
Both test-build and workload execution clone mutable branch refs (--branch "${FORK_BRANCH}") from an external fork. Use the same pinned commit in build-kube-burner-test and node-density, or a shared upstream version before building and running.
📍 Affects 2 files
ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh#L8-L9(this comment)ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh#L22-L24ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh#L78-L82
🤖 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-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh`
around lines 8 - 9, Replace the mutable FORK_BRANCH-based kube-burner fork
references with one immutable commit ref, and use that identical pinned ref in
build-kube-burner-test at
ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh
lines 8-9 and 22-24, plus node-density at
ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh
lines 78-82. Update the clone/build and workload execution commands consistently
so both use the same pinned source before building and running.
| GO_VERSION="1.25.9" | ||
| curl -sL "https://go.dev/dl/go${GO_VERSION}.linux-amd64.tar.gz" -o /tmp/go.tar.gz | ||
| mkdir -p /tmp/goroot | ||
| tar -C /tmp/goroot -xzf /tmp/go.tar.gz |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate scripts =="
git ls-files | grep -F 'ci-operator/step-registry/openshift-qe' | sed -n '1,120p'
echo
echo "== target files around Go download =="
for f in \
ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh \
ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh
do
if [ -f "$f" ]; then
echo "--- $f wc $(wc -l < "$f")"
sed -n '1,35p' "$f"
echo "-----"
sed -n '70,100p' "$f"
echo
rg -n 'GO_VERSION|curl .*go\.dev|sha256|sha256sum|go\.tar\.gz|curl_options|set -euo pipefail|set -x' "$f" || true
else
echo "MISSING $f"
fi
done
echo
echo "== same patterns in repo (read-only text search) =="
rg -n 'curl -sL "https://go\.dev/dl/go\$\{GO_VERSION\}\.linux-amd64\.tar\.gz"|sha256sum|go\.dev/dl/go|GO_VERSION' ci-operator/step-registry/openshift-qe || true
echo
echo "== deterministic behavior probe: bash -c 'set -euo pipefail; GO_VERSION=1.25.9; exec curl ...' invalid URL would fail =="
python3 - <<'PY'
import os, pathlib
for path in [
pathlib.Path('ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh'),
pathlib.Path('ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh'),
]:
text = path.read_text()
print(f"{path}: set_errexit={'set -euo pipefail' in text or 'set -e' in text}")
print(f"{path}: curl_before_tar={'curl' in text and text.find('curl') < text.find('tar')}")
print(f"{path}: tar_without_error='&&' not in text[text.find('tar -C /tmp/goroot'):text.find('tar -C /tmp/goroot')+40])")
PYRepository: openshift/release
Length of output: 18639
Validate the Go toolchain before extracting it.
Both build scripts download go.dev tarballs with unauthenticated curl and then extract them without checksum validation or failure handling. Add a checksum/integrity check in each path, or centralize this installation so both workflows validate the toolchain uniformly.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 13-13: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/go.tar.gz
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 15-15: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/goroot
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 15-15: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/go.tar.gz
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 16-16: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/go.tar.gz
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
📍 Affects 2 files
ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh#L13-L16(this comment)ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh#L84-L88
🤖 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-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh`
around lines 13 - 16, Validate the downloaded Go tarball before extraction in
both
ci-operator/step-registry/openshift-qe/build-kube-burner-test/openshift-qe-build-kube-burner-test-commands.sh
(lines 13-16) and
ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh
(lines 84-88). Add download failure handling and verify each archive against the
official checksum before allowing tar to extract it, or centralize the
installation while preserving uniform integrity validation across both
workflows.
| # Setup pprof secrets for kube-apiserver profiling | ||
| oc create ns benchmark-operator || true | ||
| set +x | ||
| oc create serviceaccount kube-burner -n benchmark-operator || true | ||
| oc create clusterrolebinding kube-burner-crb --clusterrole=cluster-admin --serviceaccount=benchmark-operator:kube-burner || true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Replace cluster-admin with least-privilege pprof access.
The six-hour bearer token is bound to cluster-admin, granting unrestricted cluster access for a profiling operation. Define a dedicated role containing only the exact pprof non-resource or proxy permissions required by the custom targets.
🤖 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-qe/node-density/openshift-qe-node-density-commands.sh`
around lines 68 - 72, Update the pprof setup near the kube-burner service
account to define and bind a dedicated least-privilege role instead of using the
cluster-admin ClusterRole. Grant only the exact non-resource or proxy
permissions required by the configured custom profiling targets, and update
kube-burner-crb to reference that dedicated role.
| oc create ns benchmark-operator || true | ||
| set +x | ||
| oc create serviceaccount kube-burner -n benchmark-operator || true | ||
| oc create clusterrolebinding kube-burner-crb --clusterrole=cluster-admin --serviceaccount=benchmark-operator:kube-burner || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not swallow RBAC setup failures.
|| true treats “already exists” the same as forbidden, API outage, or another real failure. Use idempotent apply or ignore only an expected AlreadyExists result; otherwise fail before starting the workload.
🤖 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-qe/node-density/openshift-qe-node-density-commands.sh`
around lines 69 - 72, Update the benchmark-operator RBAC setup commands around
the kube-burner service account and cluster role binding to stop unconditionally
swallowing errors. Use an idempotent apply approach or explicitly ignore only
expected AlreadyExists responses, while propagating forbidden, API outage, and
other failures so the workload does not start with incomplete RBAC.
| BEARER_TOKEN=$(oc create token -n benchmark-operator kube-burner --duration=6h || oc sa get-token kube-burner -n benchmark-operator) | ||
| export BEARER_TOKEN |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
FILE="ci-operator/step-registry/openshift-qe/node-density/openshift-qe-node-density-commands.sh"
echo "== file exists and size =="
wc -l "$FILE"
echo "== relevant context =="
sed -n '1,140p' "$FILE" | cat -n
echo "== token usage and related commands =="
rg -n "BEARER_TOKEN|kube-burner|oc create token|oc sa get-token|create token" "$FILE"Repository: openshift/release
Length of output: 7641
🌐 Web query:
openshift oc create token ServiceAccountToken --duration default behavior oc sa get-token difference
💡 Result:
The command oc create token is the current, recommended method for generating service account tokens in OpenShift, while oc sa get-token is deprecated and subject to removal [1][2][3]. Key differences and behaviors: 1. Deprecation Status: The oc sa get-token command is deprecated [1][3]. It was designed to retrieve existing, long-lived secret-based tokens. In modern OpenShift versions (4.11+), these long-lived secrets are no longer automatically generated for service accounts [4][5][6], which is why oc sa get-token often fails to find a token [1]. 2. Default Behavior of oc create token: When you run oc create token <service_account_name>, it requests a short-lived, projected-style token from the API server [7][3]. If the --duration flag is not specified (or set to 0), the lifetime is determined automatically by the server [7]. In practice, this defaults to a lifetime of one hour [8][3]. Note that the API server reserves the right to return a token with a different lifetime than requested [7][3]. 3. Token Management: If you require a long-lived token (e.g., for legacy CI/CD pipelines that cannot handle token rotation), the recommended approach is to manually create a service account token secret [4][3][6]. Otherwise, for application access, using projected volumes is the standard, secure practice [4][5][6]. Summary of command usage: - Modern/Recommended: oc create token <service_account_name> [--duration=] [7][3] - Deprecated: oc sa get-token <service_account_name> [1][2]
Citations:
- 1: https://access.redhat.com/solutions/7043390
- 2: Deprecate oc serviceaccounts command oc#1126
- 3: https://www.vshn.ch/en/blog/most-interesting-new-features-of-openshift-4-11/
- 4: https://docs.redhat.com/en/documentation/openshift_container_platform/4.14/html/authentication_and_authorization/using-service-accounts
- 5: https://docs.redhat.com/en/documentation/openshift_container_platform/4.12/html/authentication_and_authorization/using-service-accounts
- 6: https://docs.redhat.com/en/documentation/openshift_container_platform/4.19/html/authentication_and_authorization/understanding-and-creating-service-accounts
- 7: https://manpages.opensuse.org/Tumbleweed/oc/oc-create-token.1.en.html
- 8: https://www.freekb.net/Article?id=4904
Avoid the token fallback for auth/API failures.
The || oc sa get-token fallback also runs on authorization failures and API unavailable errors, and oc sa get-token is deprecated/token-source behavior is not the intended 6h-limited token. Only fall back when oc create token is genuinely unavailable and add an explicit comment; otherwise let non-unsupported errors fail so CI does not continue with a different token semantics.
🤖 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-qe/node-density/openshift-qe-node-density-commands.sh`
around lines 73 - 74, Update the BEARER_TOKEN assignment to fall back to oc sa
get-token only when oc create token is genuinely unsupported or unavailable, not
when it fails due to authorization or API errors. Preserve the 6-hour token
semantics whenever oc create token succeeds, propagate other failures so CI
stops, and add an explicit comment documenting the narrowly scoped fallback.
|
@redhat-chai-bot: 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. |
…f investigation - Configure all 6 workload scripts to build kube-burner-ocp from fork with kube-apiserver pprof targets and export BEARER_TOKEN - Install Go to /tmp/goroot (user-writable) instead of /usr/local - Wrap bearer token creation with set +x/set -x to prevent log redaction - Change payload-control-plane-6nodes test to run only node-density-cni Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7f16348 to
130cbb4
Compare
|
[REHEARSALNOTIFIER]
A total of 737 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: |
2 participants
Summary
Enable kube-apiserver CPU pprof collection for all PerfScale workloads in the
payload-control-plane-6nodesjob. This is an investigation aid for PERFSCALE-5323 — a confirmed 11-19%apiserverCPU_avgregression after the Kubernetes 1.36.2 rebase in OCP 5.0.Problem
Audit log analysis confirmed that the API call volume during node-density-cni is essentially unchanged between pre-rebase (k8s v1.35.3) and post-rebase (k8s v1.36.2) — the regression is caused by each API call being more CPU-expensive, not by more calls. CPU pprof profiles are needed to identify which code paths cost more after the rebase.
The existing
PPROF=truesetting only collects OVN profiles (ovnkube-controller, ovnk-control-plane). kube-apiserver pprof requires bearer token auth and is not included in the upstream kube-burner-ocp binary.Changes
All 6 step ref command scripts are updated with:
BEARER_TOKEN setup — creates a
kube-burnerServiceAccount inbenchmark-operatornamespace withcluster-adminClusterRoleBinding, then generates a 6-hour bearer token. This follows theget_pprof_secrets()pattern from e2e-benchmarking common.sh L134-148.Custom kube-burner-ocp build — clones the fork branch
redhat-chai-bot/kube-burner-ocp:add-kube-apiserver-pprof-targetswhich adds akube-apiserver-cpupprof target to all workload configs. Builds withmake build, tarballs the binary, and setsKUBE_BURNER_URLto the local tarball sorun.shuses it instead of downloading from GitHub releases.Recurring pprof collection — adds
--pprof-interval=1mtoEXTRA_FLAGSfor periodic 30-second CPU profile capture every minute during the workload (instead of the default single collection at end).Modified scripts
cluster-density-v2-commands.shcudn-density-commands.shnode-density-cni-commands.shnode-density-heavy-commands.shnode-density-commands.shudn-density-pods-commands.shNext steps
Once merged, trigger pre-rebase and post-rebase runs to compare kube-apiserver CPU profiles and identify which code paths regressed in k8s 1.36.2.
Cleanup
After the investigation, the fork build logic should be removed and replaced with the appropriate
KUBE_BURNER_VERSIONoverride once upstream kube-burner-ocp includes kube-apiserver pprof targets natively.@mcornea requested in Slack thread
Summary by CodeRabbit
payload-control-plane-6nodesPerfScale CI job (inci-operator/config/openshift-eng/ocp-perfscale/...) to adjust the test/chain wiring: it adds anopenshift-qe-build-kube-burner-teststep and includesopenshift-qe-node-density-cni, while removing the prioropenshift-qe-control-planeandopenshift-qe-orion-consolidatedchains from that job section.--pprof-interval=1mto the workloadEXTRA_FLAGSin:cluster-density-v2cudn-densitynode-density-cninode-density-heavynode-densityudn-density-podsbenchmark-operatornamespace, provisioning akube-burnerServiceAccount withcluster-adminvia a ClusterRoleBinding, and exporting a 6-hourBEARER_TOKEN.add-kube-apiserver-pprof-targets), exportingKUBE_BURNER_URLas a localfile:///tmp/kube-burner-ocp-custom.tar.gzso the workload run consumes the profiled kube-apiserver-enabled kube-burner-ocp.openshift-qe-build-kube-burner-test(plus OWNERS updates) to validate buildingkube-burner-ocpfrom the fork in CI.