add: performance analysis#1881
Conversation
|
Warning Review limit reached
Next review available in: 47 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: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Kubernetes label-based filtering for cluster workflows and labels newly created workflows with owner and flavor metadata. Also adds a comprehensive performance and scaling strategy covering bottlenecks, deployment, monitoring, testing, and rollout phases. ChangesCluster filtering and scaling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant clusterImpl.List
participant buildLabelSelector
participant ArgoWorkflows
Caller->>clusterImpl.List: ClusterListRequest and context email
clusterImpl.List->>buildLabelSelector: Build owner and flavor selector
buildLabelSelector-->>clusterImpl.List: Kubernetes labels.Selector
clusterImpl.List->>ArgoWorkflows: ListWorkflows with LabelSelector
ArgoWorkflows-->>clusterImpl.List: Matching workflows
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
|
A single node development cluster (infra-pr-1881) was allocated in production infra for this PR. CI will attempt to deploy 🔌 You can connect to this cluster with: 🛠️ And pull infractl from the deployed dev infra-server with: 🔓 You must go to to export an . Your token from the prod infra instance will not work with dev environments. 🚲 You can then use the dev infra instance e.g.: Further Development☕ If you make changes, you can commit and push and CI will take care of updating the development cluster. 🚀 If you only modify configuration (chart/infra-server/configuration) or templates (chart/infra-server/{static,templates}), you can get a faster update with: LogsLogs for the development infra depending on your @redhat.com authuser: Or: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/service/cluster/cluster.go (1)
154-180: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBackfill owner labels or keep a fallback. Pre-existing workflows lack
labelOwner, soAll=falseowner queries filter them out and hide them from their owners. A migration/backfill or a transitional annotation-based fallback is needed.🤖 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 `@pkg/service/cluster/cluster.go` around lines 154 - 180, Update the List flow around buildLabelSelector and ListWorkflows so owner-filtered requests with All=false still return pre-existing workflows that lack the labelOwner label. Implement either a migration/backfill for existing workflows or a transitional annotation-based fallback, while preserving server-side filtering for workflows with owner labels.
🧹 Nitpick comments (1)
performance-analysis.md (1)
1097-1107: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFrame these as targets, not outcomes.
performance-analysis.md:1103-1107still reads like these numbers are already achieved, while the surrounding section only outlines a validation plan. Tie each figure to an explicit assumption or benchmark, and soften “without further changes” unless that’s demonstrated.🤖 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 `@performance-analysis.md` around lines 1097 - 1107, Revise the “Expected Outcomes” section to present the latency, API-load, availability, and scalability figures as targets or estimates rather than achieved results. State the assumptions or benchmark conditions supporting each figure, and replace “without further changes” in the scalability target with qualified language tied to validation.Source: Path instructions
🤖 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 `@performance-analysis.md`:
- Around line 14-16: Narrow the “Scaling Blocker” statement in
performance-analysis.md to safe horizontal scaling of background workers,
reflecting the coordination requirement for duplicate side effects while not
implying that stateless request-serving API replicas require leader election.
- Around line 185-189: Update the caching recommendation for
getMostRecentArgoWorkflowFromClusterID() to define authorization-safe cache keys
before implementation. Require keys to include the authenticated principal or
tenant, namespace, and every request filter, matching the email-derived selector
logic in the cluster service, and document that cached list results must never
be shared across authorization boundaries.
- Around line 1007-1012: Correct the GCS API call calculations in the “GCS API
Calls” section: apply the 90% cache hit rate to 2 operations per minute across
100 artifacts, yielding 20 cache-miss reads per minute and approximately 28,800
reads per day. Update the resulting Class A operation cost and “Total Current”
figure to match the corrected projection, including the existing ~$4.34/month
estimate where applicable.
- Around line 232-251: Complete cleanupExpiredClusters by adding explicit
exponential-backoff state that increases after processExpiredClusters errors and
resets after successful processing, while preserving jittered scheduling. Wire
the service-level context used by cleanup goroutines to an explicit cancellation
hook invoked during server shutdown, ensuring cleanupExpiredClusters exits
promptly and does not remain live after termination.
- Around line 942-958: The alert expressions for InfraServerHighLatency and
InfraServerHighMemory need correction. Update the latency expression to apply
histogram_quantile to summed, rate-based *_bucket series grouped by le, and
guard the memory ratio against missing or zero container memory limits (or
establish limits in the deployment configuration) so InfraServerHighMemory
cannot misfire.
In `@pkg/service/cluster/cluster.go`:
- Around line 411-414: Update the owner label value in the workflow labels built
by the surrounding workflow creation logic to use a Kubernetes label-safe
encoding instead of the raw email address. Apply the identical encoding in
buildLabelSelector so labels and owner filtering remain consistent, while
leaving the cluster ID and flavor labels unchanged.
In `@pkg/service/cluster/helpers.go`:
- Around line 271-296: The owner filter in buildLabelSelector must use the same
label-safe encoding as the labelOwner value written in cluster.go. Replace the
raw email used by labels.NewRequirement with the shared encoding/helper, and
update the labelOwner assignment path if necessary so both write and query use
identical encoded values while preserving owner filtering behavior.
---
Outside diff comments:
In `@pkg/service/cluster/cluster.go`:
- Around line 154-180: Update the List flow around buildLabelSelector and
ListWorkflows so owner-filtered requests with All=false still return
pre-existing workflows that lack the labelOwner label. Implement either a
migration/backfill for existing workflows or a transitional annotation-based
fallback, while preserving server-side filtering for workflows with owner
labels.
---
Nitpick comments:
In `@performance-analysis.md`:
- Around line 1097-1107: Revise the “Expected Outcomes” section to present the
latency, API-load, availability, and scalability figures as targets or estimates
rather than achieved results. State the assumptions or benchmark conditions
supporting each figure, and replace “without further changes” in the scalability
target with qualified language tied to validation.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e20a386a-102a-4541-a0c8-4d66095fd893
📒 Files selected for processing (4)
performance-analysis.mdpkg/service/cluster/cluster.gopkg/service/cluster/helpers.gopkg/service/cluster/labels.go
| - **Scaling Blocker:** No leader election or distributed coordination prevents safe horizontal scaling | ||
| - **Resource Constraints:** No CPU/memory limits defined for any component | ||
| - **Background Load:** Two polling loops list all workflows every 60 seconds per instance |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Narrow the horizontal-scaling blocker claim.
Leader election is required to coordinate the duplicate background side effects shown in pkg/service/cluster/cluster.go:640-690 and 730-780; it is not a blanket prerequisite for scaling the request-serving read path. Reframe this as “safe horizontal scaling of background workers” to avoid unnecessarily blocking stateless API replicas.
As per path instructions, this review focuses on major performance, maintainability, and security risks.
🤖 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 `@performance-analysis.md` around lines 14 - 16, Narrow the “Scaling Blocker”
statement in performance-analysis.md to safe horizontal scaling of background
workers, reflecting the coordination requirement for duplicate side effects
while not implying that stateless request-serving API replicas require leader
election.
Source: Path instructions
| **Solution:** | ||
| - Use label selector for `getMostRecentArgoWorkflowFromClusterID()`: `labels.Set{"infra.stackrox.com/cluster-id": clusterID}.AsSelector()` | ||
| - Implement watch-based monitoring instead of polling for background loops | ||
| - Add caching layer for list results with short TTL (5-10 seconds) | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Define authorization-safe cache keys before recommending list caching.
pkg/service/cluster/cluster.go:158-166 derives selectors from the authenticated user’s email. A shared list cache must include the principal/tenant, namespace, and every request filter; otherwise one user could receive another user’s cluster list. Document the cache isolation and authorization contract before implementation.
As per path instructions, this review focuses on major performance, maintainability, and security risks.
🤖 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 `@performance-analysis.md` around lines 185 - 189, Update the caching
recommendation for getMostRecentArgoWorkflowFromClusterID() to define
authorization-safe cache keys before implementation. Require keys to include the
authenticated principal or tenant, namespace, and every request filter, matching
the email-derived selector logic in the cluster service, and document that
cached list results must never be shared across authorization boundaries.
Source: Path instructions
| ```go | ||
| func (s *clusterImpl) cleanupExpiredClusters(ctx context.Context) { | ||
| // Add jitter to prevent thundering herd | ||
| jitter := time.Duration(rand.Intn(30)) * time.Second | ||
| ticker := time.NewTicker(resumeExpiredClusterInterval + jitter) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return // Graceful shutdown | ||
| case <-ticker.C: | ||
| // Use watch API or cached list | ||
| if err := s.processExpiredClusters(ctx); err != nil { | ||
| // Exponential backoff on errors | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make the shutdown and backoff design complete.
The sample adds a fixed ticker, but does not implement exponential backoff despite claiming it will. The service-level context proposed at lines 283-294 also needs an explicit cancellation hook tied to server shutdown; otherwise the goroutines remain live. Include the retry state and lifecycle wiring in the strategy.
As per path instructions, this review focuses on major performance, maintainability, and security risks.
🤖 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 `@performance-analysis.md` around lines 232 - 251, Complete
cleanupExpiredClusters by adding explicit exponential-backoff state that
increases after processExpiredClusters errors and resets after successful
processing, while preserving jittered scheduling. Wire the service-level context
used by cleanup goroutines to an explicit cancellation hook invoked during
server shutdown, ensuring cleanupExpiredClusters exits promptly and does not
remain live after termination.
Source: Path instructions
| - alert: InfraServerHighLatency | ||
| expr: histogram_quantile(0.95, infra_server_list_operation_duration_seconds) > 2 | ||
| for: 5m | ||
| annotations: | ||
| summary: "Infra-server list operations exceeding 2s p95 latency" | ||
|
|
||
| - alert: InfraServerNoLeader | ||
| expr: sum(infra_server_leader_election_status) == 0 | ||
| for: 1m | ||
| annotations: | ||
| summary: "No infra-server instance is leader (background tasks not running)" | ||
|
|
||
| - alert: InfraServerHighMemory | ||
| expr: container_memory_working_set_bytes{pod=~"infra-server.*"} / container_spec_memory_limit_bytes{pod=~"infra-server.*"} > 0.9 | ||
| for: 5m | ||
| annotations: | ||
| summary: "Infra-server memory usage > 90% of limit" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant document and nearby sections.
git ls-files performance-analysis.md
wc -l performance-analysis.md
sed -n '900,980p' performance-analysis.md
# Find any references to these metrics or alert rules elsewhere.
rg -n --hidden --no-ignore-vcs \
'infra_server_list_operation_duration_seconds|infra_server_leader_election_status|container_memory_working_set_bytes|container_spec_memory_limit_bytes|InfraServerHighLatency|InfraServerNoLeader|InfraServerHighMemory' \
.
# If there are metric definition snippets, inspect them.
rg -n --hidden --no-ignore-vcs 'histogram|bucket|memory limit|limit_bytes|leader_election' performance-analysis.md .Repository: stackrox/infra
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the specific alert section with line numbers.
sed -n '904,960p' performance-analysis.md | cat -n
# Inspect the earlier resource-constraints statement.
sed -n '1,40p' performance-analysis.md | cat -n
sed -n '500,520p' performance-analysis.md | cat -n
# Look for infra-server resource limits in manifests.
rg -n --hidden --no-ignore-vcs -C 3 'resources:|limits:|requests:|cpu:|memory:' chart/infra-server
# Check whether the latency metric is ever emitted or described as a histogram bucket series.
rg -n --hidden --no-ignore-vcs -C 2 'infra_server_list_operation_duration_seconds(_bucket)?|histogram_quantile|Observe\(|Histogram' .Repository: stackrox/infra
Length of output: 43161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the infra-server deployment/chart files.
fd -H -t f 'values.yaml|deployment.yaml|*.yaml' chart/infra-server
# Inspect the infra-server chart for resource limits and the workload definition.
rg -n --hidden --no-ignore-vcs -C 4 'kind: Deployment|resources:|limits:|requests:|container_memory_working_set_bytes|container_spec_memory_limit_bytes|infra_server_list_operation_duration_seconds' chart/infra-server
# Check the server metric configuration around histogram emission.
sed -n '90,120p' pkg/server/server.go | cat -nRepository: stackrox/infra
Length of output: 494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the source of the listed metrics in the actual codebase, excluding vendored/dependency dirs.
rg -n --hidden --no-ignore-vcs -g '!ui/node_modules/**' -g '!node_modules/**' \
'infra_server_list_operation_duration_seconds|infra_server_gcs_artifact_fetch_duration_seconds|infra_server_leader_election_status|container_spec_memory_limit_bytes|container_memory_working_set_bytes' \
.
# Inspect the infra-server chart for workload resources, if present.
fd -H -t f 'deployment.yaml' chart/infra-server
fd -H -t f 'values.yaml' chart/infra-serverRepository: stackrox/infra
Length of output: 1257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the infra-server deployment template and values around resource settings.
sed -n '1,220p' chart/infra-server/templates/deployment.yaml | cat -n
printf '\n--- values.yaml ---\n'
sed -n '1,220p' chart/infra-server/values.yaml | cat -n
printf '\n--- argo-values.yaml ---\n'
sed -n '1,120p' chart/infra-server/argo-values.yaml | cat -n
# Search for the custom latency metric implementation or any Prometheus histogram exposition.
rg -n --hidden --no-ignore-vcs -g '!ui/node_modules/**' -g '!node_modules/**' \
'infra_server_list_operation_duration_seconds|list_operation_duration|EnableHandlingTimeHistogram|Histogram|Observe\(' \
pkg chart certifierRepository: stackrox/infra
Length of output: 3679
Fix the Prometheus alert expressions in performance-analysis.md:942-955.
histogram_quantileneeds the histogram bucket series, e.g.sum by (le)(rate(infra_server_list_operation_duration_seconds_bucket[5m])); the current query won’t calculate p95.chart/infra-server/templates/deployment.yamldefines no memory limits, so the memory ratio has no usable denominator and can misfire; add a guard or set limits first.
🤖 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 `@performance-analysis.md` around lines 942 - 958, The alert expressions for
InfraServerHighLatency and InfraServerHighMemory need correction. Update the
latency expression to apply histogram_quantile to summed, rate-based *_bucket
series grouped by le, and guard the memory ratio against missing or zero
container memory limits (or establish limits in the deployment configuration) so
InfraServerHighMemory cannot misfire.
Source: Path instructions
| **GCS API Calls:** | ||
| - Assuming 100 clusters, 2 list operations/minute × 60 min × 24 hr = 2,880 list ops/day | ||
| - Each list reads 100 artifacts → 288,000 GCS reads/day | ||
| - GCS Class A operations: $0.05 per 10,000 → ~$1.44/day → ~$43/month | ||
|
|
||
| **Total Current: ~$103/month** (excluding storage, egress) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the projected GCS-read arithmetic.
With 2 operations/minute, 100 artifacts, and a 90% cache hit rate, the projected miss rate is 20 reads/minute, or about 28,800 reads/day, not 200 reads/minute and 288,000 reads/day. The stated ~$4.34/month cost matches the corrected lower count, so the document is internally inconsistent and currently overstates projected API volume by 10×.
As per path instructions, this review focuses on major performance, maintainability, and security risks.
Also applies to: 1024-1029
🤖 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 `@performance-analysis.md` around lines 1007 - 1012, Correct the GCS API call
calculations in the “GCS API Calls” section: apply the 90% cache hit rate to 2
operations per minute across 100 artifacts, yielding 20 cache-miss reads per
minute and approximately 28,800 reads per day. Update the resulting Class A
operation cost and “Total Current” figure to match the corrected projection,
including the existing ~$4.34/month estimate where applicable.
Source: Path instructions
2 participants
No description provided.