improve obvious performance issues#186
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds query-driven pagination to API list handlers ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/controller/operatorconfig/controller.go (1)
545-552: Consider logging when hash computation fails.When
json.Marshalfails, the function silently returns an empty string, causing all updates to proceed (safe fallback). However, this makes debugging difficult if serialization issues occur. Adding a log statement would improve observability without changing behavior.🔧 Suggested improvement
func computeSpecHash(obj client.Object) string { data, err := json.Marshal(obj) if err != nil { + // Log but don't fail - falling back to always-update behavior return "" } h := sha256.Sum256(data) return hex.EncodeToString(h[:]) }Note: You'd need to pass a logger or use a package-level logger to actually log. Alternatively, consider returning
(string, error)and letting the caller decide how to handle failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/operatorconfig/controller.go` around lines 545 - 552, computeSpecHash currently swallows json.Marshal errors and returns an empty string; update it to log the serialization error so failures are observable by either (a) adding a package-level logger or accepting a logger parameter and calling it when json.Marshal fails, or (b) change the signature to return (string, error) so callers can log/handle the error; locate the computeSpecHash function and implement one of these options, ensuring the error message includes context (e.g., "computeSpecHash: failed to marshal object") and the original error.internal/controller/image/controller.go (1)
115-120: This still leaves one self-triggered verify per timestamp refresh.Every successful
updateLastVerifiedwrites theImagestatus, so the controller reconciles the same object again and runsverifyImageLocationa second time. The 30-minute gate reduces churn, but it does not remove the remaining duplicate check. Since this controller already schedules its own requeues, consider keeping pureLastVerifiedrefreshes off the watched update path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/image/controller.go` around lines 115 - 120, The controller currently writes status.LastVerified via updateLastVerified which triggers a watched update and causes an immediate duplicate reconcile/verify; change the flow so pure LastVerified refreshes don't hit the watched path: modify updateLastVerified to patch only the status (use client.Status().Patch with client.MergeFrom(original)) and mark those patches with a short-lived marker (e.g. annotation "internal.example.com/last-verified-only=true"); then update the controller's watch/predicate in Reconcile setup (or the existing predicate function) to ignore events whose only change is that marker or only status.LastVerified changed (i.e. compare old/new status and annotations and return false if nothing but LastVerified/marker changed), leaving verifyImageLocation and the main Reconcile path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/server.go`:
- Around line 2676-2699: parsePagination currently applies defaultPageLimit=50
even when callers don't supply a limit, causing silent truncation; change it to
preserve legacy "full list unless requested" behavior by only enforcing limits
when the client provides a "limit" query: in parsePagination (and related
constants defaultPageLimit/maxPageLimit) return a sentinel (e.g., 0 or -1) or a
boolean flag to indicate "no limit provided" so callers can fetch the full list,
and when a limit is provided validate and clamp it to maxPageLimit as before;
update any callers that consume parsePagination to treat the sentinel/flag as
“no limiting” rather than a 50-item default.
In `@internal/buildapi/workspace.go`:
- Around line 328-339: The current code builds the owned slice from wsList.Items
and calls applyPagination without a deterministic sort, causing unstable pages;
fix by sorting the owned slice deterministically before pagination (e.g., using
sort.Slice on the owned []WorkspaceResponse returned by workspaceResponseFromCR)
— compare a stable field such as workspace name or UID (fields available on the
WorkspaceResponse produced by workspaceResponseFromCR) and then call
applyPagination(owned, limit, offset); ensure you import sort if needed and keep
the sort key consistent (e.g., Name or ID) so pages are stable across requests.
In `@internal/controller/imagebuild/controller.go`:
- Around line 1487-1510: The deleteSecretWithRetry helper currently swallows
transient delete errors causing leaked secrets; change deleteSecretWithRetry on
ImageBuildReconciler to return an error (or a result/boolean) instead of only
logging, treat IsNotFound as success, and return the delete error for any other
failure; update callers in the reconcile path to check the returned error and
explicitly requeue the reconciliation (e.g., using ctrl.Result{Requeue:true} or
RequeueAfter) so cleanup is retried deterministically until the secret is
removed.
In `@internal/controller/imagereseal/controller.go`:
- Around line 724-743: The deleteSecretWithRetry function currently swallows
transient Delete errors (in Reconciler.deleteSecretWithRetry), relying on the
reconcile loop to re-run which doesn't happen once status is terminal; change
the pattern to return the error instead of only logging it so callers can handle
retries. Update deleteSecretWithRetry to return error (preserving NotFound as
nil), remove the silent log-only failure path, and modify its callers in the
Reconciler cleanup code to check the returned error and call
ctrl.Result{RequeueAfter: <shortDuration>} (or return the error to be converted
into a RequeueAfter) so transient failures trigger explicit requeueing rather
than being left behind in terminal phases.
---
Nitpick comments:
In `@internal/controller/image/controller.go`:
- Around line 115-120: The controller currently writes status.LastVerified via
updateLastVerified which triggers a watched update and causes an immediate
duplicate reconcile/verify; change the flow so pure LastVerified refreshes don't
hit the watched path: modify updateLastVerified to patch only the status (use
client.Status().Patch with client.MergeFrom(original)) and mark those patches
with a short-lived marker (e.g. annotation
"internal.example.com/last-verified-only=true"); then update the controller's
watch/predicate in Reconcile setup (or the existing predicate function) to
ignore events whose only change is that marker or only status.LastVerified
changed (i.e. compare old/new status and annotations and return false if nothing
but LastVerified/marker changed), leaving verifyImageLocation and the main
Reconcile path unchanged.
In `@internal/controller/operatorconfig/controller.go`:
- Around line 545-552: computeSpecHash currently swallows json.Marshal errors
and returns an empty string; update it to log the serialization error so
failures are observable by either (a) adding a package-level logger or accepting
a logger parameter and calling it when json.Marshal fails, or (b) change the
signature to return (string, error) so callers can log/handle the error; locate
the computeSpecHash function and implement one of these options, ensuring the
error message includes context (e.g., "computeSpecHash: failed to marshal
object") and the original error.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d2aedf1-c971-4259-959c-2e5dbfecdaa1
📒 Files selected for processing (7)
internal/buildapi/container_builds.gointernal/buildapi/server.gointernal/buildapi/workspace.gointernal/controller/image/controller.gointernal/controller/imagebuild/controller.gointernal/controller/imagereseal/controller.gointernal/controller/operatorconfig/controller.go
bennyz
force-pushed
the
perf-bottlenecks
branch
from
c68272a to
cc7ae98
Compare
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
perf-bottlenecks
branch
from
cc7ae98 to
ce3c874
Compare
2 participants
Summary by CodeRabbit
Release Notes
New Features
Refactor