Skip to content

ROX-33758: Fix quadratic runtime in endpointsStore.addToHistory#19572

Open
rhacs-bot wants to merge 1 commit intorelease-4.10from
backport-19566-to-release-4.10
Open

ROX-33758: Fix quadratic runtime in endpointsStore.addToHistory#19572
rhacs-bot wants to merge 1 commit intorelease-4.10from
backport-19566-to-release-4.10

Conversation

@rhacs-bot
Copy link
Contributor

Backport bfd67f2 from #19566.

@rhacs-bot rhacs-bot requested a review from a team as a code owner March 24, 2026 14:58
@github-actions github-actions bot added area/sensor backport PR to backport changes from master to release branch ai-review labels Mar 24, 2026
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • In benchmarkSeedEndpointsStore, for i := range numEndpoints will not compile because range does not work over an int; use a standard counter loop like for i := 0; i < numEndpoints; i++ instead.
  • In BenchmarkEndpointsStoreAddToHistory, verify that b.Loop() is intentional and supported in your test harness; in standard Go benchmarks this should be a for i := 0; i < b.N; i++ loop.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `benchmarkSeedEndpointsStore`, `for i := range numEndpoints` will not compile because `range` does not work over an `int`; use a standard counter loop like `for i := 0; i < numEndpoints; i++` instead.
- In `BenchmarkEndpointsStoreAddToHistory`, verify that `b.Loop()` is intentional and supported in your test harness; in standard Go benchmarks this should be a `for i := 0; i < b.N; i++` loop.

## Individual Comments

### Comment 1
<location path="sensor/common/clusterentities/store_endpoints_benchmark_test.go" line_range="17" />
<code_context>
+	epSet := set.NewSet[net.NumericEndpoint]()
+
+	var firstEndpoint net.NumericEndpoint
+	for i := range numEndpoints {
+		ep := buildEndpoint(fmt.Sprintf("10.%d.%d.%d", (i/65536)%256, (i/256)%256, i%256), 80)
+		if i == 0 {
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix the loop over numEndpoints so the benchmark compiles and exercises the intended number of endpoints

`range` can't be used on an `int`, so this code won't compile and the benchmark can't run. Use an indexed loop instead:

```go
for i := 0; i < numEndpoints; i++ {
    // ...
}
```

This will correctly seed `numEndpoints` entries into the store and keep the benchmark at the intended scale.
</issue_to_address>

### Comment 2
<location path="sensor/common/clusterentities/store_endpoints_benchmark_test.go" line_range="44-47" />
<code_context>
+		{name: "endpoints_1000", numEndpoints: 1000},
+		{name: "endpoints_5000", numEndpoints: 5000},
+	} {
+		b.Run(tc.name, func(b *testing.B) {
+			store, deploymentID, endpoint := benchmarkSeedEndpointsStore(tc.numEndpoints)
+			b.ResetTimer()
+			for b.Loop() {
+				// Keep the current-map shape constant and measure addToHistory only.
+				store.historicalEndpoints = make(map[net.NumericEndpoint]map[string]map[EndpointTargetInfo]*entityStatus)
</code_context>
<issue_to_address>
**issue (testing):** Use the standard `for i := 0; i < b.N; i++` pattern instead of `b.Loop()` in the benchmark

`testing.B` doesn’t define `Loop()`, so this code won’t compile. To integrate with Go’s benchmark harness, use the standard `for i := 0; i < b.N; i++ { ... }` pattern with `b.ResetTimer()` instead of `for b.Loop()`.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

epSet := set.NewSet[net.NumericEndpoint]()

var firstEndpoint net.NumericEndpoint
for i := range numEndpoints {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Fix the loop over numEndpoints so the benchmark compiles and exercises the intended number of endpoints

range can't be used on an int, so this code won't compile and the benchmark can't run. Use an indexed loop instead:

for i := 0; i < numEndpoints; i++ {
    // ...
}

This will correctly seed numEndpoints entries into the store and keep the benchmark at the intended scale.

Comment on lines +44 to +47
b.Run(tc.name, func(b *testing.B) {
store, deploymentID, endpoint := benchmarkSeedEndpointsStore(tc.numEndpoints)
b.ResetTimer()
for b.Loop() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (testing): Use the standard for i := 0; i < b.N; i++ pattern instead of b.Loop() in the benchmark

testing.B doesn’t define Loop(), so this code won’t compile. To integrate with Go’s benchmark harness, use the standard for i := 0; i < b.N; i++ { ... } pattern with b.ResetTimer() instead of for b.Loop().

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The new benchmark code does not compile as written (for i := range numEndpoints and for b.Loop() are invalid); update the loops to use standard forms like for i := 0; i < numEndpoints; i++ and for i := 0; i < b.N; i++.
  • Consider removing or moving the large commented benchmark output block from store_endpoints_benchmark_test.go to keep the test file focused on code rather than runtime logs.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new benchmark code does not compile as written (`for i := range numEndpoints` and `for b.Loop()` are invalid); update the loops to use standard forms like `for i := 0; i < numEndpoints; i++` and `for i := 0; i < b.N; i++`.
- Consider removing or moving the large commented benchmark output block from `store_endpoints_benchmark_test.go` to keep the test file focused on code rather than runtime logs.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@rhacs-bot
Copy link
Contributor Author

Images are ready for the commit at 71d9ed5.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.10.1-rc.0-3-g71d9ed509a.

@github-actions
Copy link
Contributor

/konflux-retest operator-bundle-on-push

2 similar comments
@github-actions
Copy link
Contributor

/konflux-retest operator-bundle-on-push

@github-actions
Copy link
Contributor

/konflux-retest operator-bundle-on-push

@github-actions
Copy link
Contributor

/konflux-retest main-on-push

@openshift-ci
Copy link

openshift-ci bot commented Mar 24, 2026

@rhacs-bot: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/gke-sensor-integration-tests 71d9ed5 link false /test gke-sensor-integration-tests
ci/prow/gke-qa-e2e-tests 71d9ed5 link false /test gke-qa-e2e-tests
ci/prow/ocp-next-candidate-nongroovy-e2e-tests 71d9ed5 link false /test ocp-next-candidate-nongroovy-e2e-tests
ci/prow/ocp-4-12-nongroovy-e2e-tests 71d9ed5 link false /test ocp-4-12-nongroovy-e2e-tests
ci/prow/gke-nongroovy-compatibility-tests 71d9ed5 link false /test gke-nongroovy-compatibility-tests
ci/prow/ocp-dev-preview-nongroovy-e2e-tests 71d9ed5 link false /test ocp-dev-preview-nongroovy-e2e-tests
ci/prow/ocp-4-21-nongroovy-e2e-tests 71d9ed5 link false /test ocp-4-21-nongroovy-e2e-tests
ci/prow/ocp-4-20-nongroovy-e2e-tests 71d9ed5 link false /test ocp-4-20-nongroovy-e2e-tests

Full PR test history. Your PR dashboard.

Details

Instructions 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review area/sensor backport PR to backport changes from master to release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants