fix(resolver): pre-allocate slice capacity in step functions - #3889
fix(resolver): pre-allocate slice capacity in step functions#3889FranciscoMeloJr wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @FranciscoMeloJr. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughThe resolver now preallocates slices for bundle resources and steps. Service-account resources are generated before bundle processing. Tests verify counts, capacity, metadata, and allocation behavior. ChangesResolver preallocation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 2
🤖 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 `@pkg/controller/registry/resolver/resolver_test.go`:
- Around line 2082-2083: Update the capacity assertions in the affected resolver
tests to compare against the fixtures’ expected capacities rather than
len(slice), using the 5-resource and 3-RBAC expected values. For the single-step
test, use a multi-resource fixture so capacity growth is observable, or remove
that capacity assertion; preserve the existing length and behavior checks.
In `@pkg/controller/registry/resolver/steps.go`:
- Line 151: Update the slice allocation in the resolver step construction around
NewStepResourceFromBundle to include the resources generated by
operatorServiceAccountSteps in the initial capacity. Calculate the complete
expected resource count before creating the slice, while preserving the existing
resource append order and contents.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f1da5ba4-772e-4d65-9df4-09ae9ef38366
📒 Files selected for processing (2)
pkg/controller/registry/resolver/resolver_test.gopkg/controller/registry/resolver/steps.go
| // Pre-allocation guarantees cap == len when items are appended without slice growth | ||
| assert.Equal(t, len(steps), cap(steps), "Slice capacity should match length exactly when pre-allocated") |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Make the capacity assertions detect slice growth.
cap(slice) >= len(slice) is an invariant after append, so these checks pass even when the slice starts with zero capacity. The one-element test is also non-discriminating because dynamic growth can produce cap == len == 1.
Assert the expected capacities for the 5-resource and 3-RBAC fixtures. Use a multi-resource fixture for the single-step test, or remove its capacity assertion.
Also applies to: 2152-2155, 2171-2175
🤖 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/controller/registry/resolver/resolver_test.go` around lines 2082 - 2083,
Update the capacity assertions in the affected resolver tests to compare against
the fixtures’ expected capacities rather than len(slice), using the 5-resource
and 3-RBAC expected values. For the single-step test, use a multi-resource
fixture so capacity growth is observable, or remove that capacity assertion;
preserve the existing length and behavior checks.
There was a problem hiding this comment.
Pull request overview
This PR targets a small performance optimization in the catalog resolver by reducing slice re-allocations while building StepResource / Step lists from bundle content.
Changes:
- Preallocates the
[]*v1alpha1.Stepslice inNewStepsFromBundle. - Adjusts
NewStepResourceFromBundlestep-slice initialization to use a pre-sized backing array. - Adds unit tests and benchmarks intended to validate/enforce the preallocation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/controller/registry/resolver/steps.go |
Adds slice preallocation when constructing step resources from a bundle. |
pkg/controller/registry/resolver/resolver_test.go |
Adds tests/benchmarks related to the new preallocation behavior. |
Suppressed comments (3)
pkg/controller/registry/resolver/resolver_test.go:2164
- Avoid asserting exact slice capacity here; it is not part of the API contract and can change without affecting correctness (e.g., reserving extra capacity to support future appends).
assert.NotEmpty(t, bundleSteps)
assert.Len(t, bundleSteps, 5)
assert.Equal(t, len(bundleSteps), cap(bundleSteps), "Slice capacity should equal length for pre-allocated steps")
assert.Equal(t, "test-operator.v1.0.0", bundleSteps[0].Resolving)
pkg/controller/registry/resolver/resolver_test.go:2176
- This capacity check is always true in Go (
cap(slice) >= len(slice)), so it doesn't actually verify that RBAC steps were preallocated.
// 1 SA + 1 Role + 1 RoleBinding = 3 total RBAC resources
assert.Len(t, rbacSteps, 3)
assert.GreaterOrEqual(t, cap(rbacSteps), len(rbacSteps), "Capacity should be pre-allocated for RBAC steps")
})
pkg/controller/registry/resolver/steps.go:151
- The capacity preallocation here only accounts for
bundle.Object. This function appends additional RBAC step resources later (viaNewServiceAccountStepResources), sostepscan still reallocate and grow. If the goal is to avoid dynamic growth, consider including the RBAC step count in the initial capacity (e.g., generate RBAC steps first and use1+len(bundle.Object)+len(rbacSteps)for the cap) or otherwise reserve space before appending them.
steps := append(make([]v1alpha1.StepResource, 0, 1+len(bundle.Object)), step)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| //steps := []v1alpha1.StepResource{step} | ||
| steps := append(make([]v1alpha1.StepResource, 0, 1+len(bundle.Object)), step) |
| // 1 CSV + 1 CRD + 3 RBAC resources (SA, Role, RoleBinding) = 5 total | ||
| assert.Len(t, stepResources, 5) | ||
| assert.GreaterOrEqual(t, cap(stepResources), len(stepResources), "Capacity should fit all generated step resources") | ||
| }) |
| // Pre-allocation guarantees cap == len when items are appended without slice growth | ||
| assert.Equal(t, len(steps), cap(steps), "Slice capacity should match length exactly when pre-allocated") |
|
@FranciscoMeloJr Please address copolit and CodeRabbit review comments. |
openshift-ci
Bot
added
ok-to-test
5c636de to
eff26e5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/registry/resolver/steps.go (1)
150-156: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winComplete the RBAC slice preallocation.
NewServiceAccountStepResourcesstill starts with a nil slice and appends each generated resource. The allocation at Lines [155]-[156] occurs after this helper returns, so it cannot prevent the helper's dynamic growth.Count the generated RBAC entries before the append loops and allocate
rbacStepswith that capacity.Proposed fix
func NewServiceAccountStepResources(csv *v1alpha1.ClusterServiceVersion, catalogSourceName, catalogSourceNamespace string) ([]v1alpha1.StepResource, error) { - var rbacSteps []v1alpha1.StepResource - operatorPermissions, err := RBACForClusterServiceVersion(csv) if err != nil { return nil, err } + rbacCapacity := 0 + for _, perms := range operatorPermissions { + if perms.ServiceAccount.Name != "default" { + rbacCapacity++ + } + rbacCapacity += len(perms.Roles) + rbacCapacity += len(perms.RoleBindings) + rbacCapacity += len(perms.ClusterRoles) + rbacCapacity += len(perms.ClusterRoleBindings) + } + rbacSteps := make([]v1alpha1.StepResource, 0, rbacCapacity) +🤖 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/controller/registry/resolver/steps.go` around lines 150 - 156, Preallocate the RBAC resource slice inside NewServiceAccountStepResources before its append loops, using the expected generated-entry count as capacity instead of starting with a nil slice. Keep the existing resource generation and returned ordering unchanged; the later steps capacity calculation should continue using the populated result.
🤖 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.
Nitpick comments:
In `@pkg/controller/registry/resolver/steps.go`:
- Around line 150-156: Preallocate the RBAC resource slice inside
NewServiceAccountStepResources before its append loops, using the expected
generated-entry count as capacity instead of starting with a nil slice. Keep the
existing resource generation and returned ordering unchanged; the later steps
capacity calculation should continue using the populated result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8687f33b-6218-4460-9764-bb1730d43b87
📒 Files selected for processing (2)
pkg/controller/registry/resolver/resolver_test.gopkg/controller/registry/resolver/steps.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/registry/resolver/resolver_test.go
Signed-off-by: FranciscoMeloJr <635662+FranciscoMeloJr@users.noreply.github.com>
eff26e5 to
8e0dc86
Compare
Preallocate capacity for step resource slices in NewStepResourceFromBundle, NewStepsFromBundle, and NewServiceAccountStepResources to avoid dynamic slice growth.
Summary by CodeRabbit
Performance
Tests