Skip to content

Makefile: add more bench-* targets for performance suite - #22158

Closed
Somesh42 wants to merge 2 commits into
etcd-io:mainfrom
Somesh42:etcd-test
Closed

Makefile: add more bench-* targets for performance suite#22158
Somesh42 wants to merge 2 commits into
etcd-io:mainfrom
Somesh42:etcd-test

Conversation

@Somesh42

Copy link
Copy Markdown

Summary

Extends the existing make bench-put / scripts/benchmark_test.sh path from #16467
instead of introducing a separate benchmark suite tool.

  • Add make targets for additional workloads: bench-range, bench-txn-put,
    bench-stm, bench-lease-keepalive, bench-watch, bench-watch-latency
  • Keep the same lifecycle as bench-put: build etcd, install benchmark if needed,
    start a temporary local etcd, run the workload with --report-perfdash, tear down
  • Improve scripts/benchmark_test.sh so it resolves the benchmark binary from
    PATH, $(go env GOPATH)/bin, or ./bin/tools/benchmark
  • Document the new targets in tools/benchmark/README.md

This is a follow-up to maintainer feedback that new performance work should expand
the existing benchmarking workflow rather than add another runner.

Fixes: (open/link related issue if any; otherwise reference #16467)

Test plan

  • make bench-put ARGS='--total=1000'
  • make bench-range ARGS='--total=1000'
  • make bench-txn-put ARGS='--total=1000'
  • make bench-stm ARGS='--total=1000'
  • make bench-lease-keepalive ARGS='--total=1000'
  • Confirm each run starts/stops local etcd and emits perfdash-compatible output
  • Confirm benchmark is found when installed under $(go env GOPATH)/bin but not on PATH

@kubernetes-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Somesh42
Once this PR has been reviewed and has the lgtm label, please assign serathius for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow

Copy link
Copy Markdown

Hi @Somesh42. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Comment thread scripts/benchmark_test.sh Outdated
@@ -29,6 +36,36 @@ fi
BENCHMARK_NAME="$1"
ARGS="${*:2}"

resolve_benchmark_bin() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not needed.

Comment thread scripts/benchmark_test.sh Outdated
@@ -13,7 +13,14 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# This script runs a benchmark on a locally started etcd server
# This script runs a benchmark against a locally started etcd server.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't want

Extend the existing make bench-put workflow with additional make targets
for range, txn-put, stm, lease-keepalive, watch, and watch-latency.
Document usage in tools/benchmark/README.md.

Signed-off-by: Somesh Kumar Gouda <someshkumargauda@gmail.com>
@Somesh42

Copy link
Copy Markdown
Author

@serathius removed the benchmark_test.sh changes.

@serathius

serathius commented Jul 23, 2026

Copy link
Copy Markdown
Member

Think we can add all benchmarks, however I think they need to be redesigned and updated to be more reflecting for performance dimensions we care about. See #16467 (comment) section about etcd SLI

/ok-to-test

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.68%. Comparing base (74aba8c) to head (19b0fc9).
⚠️ Report is 236 commits behind head on main.

Additional details and impacted files

see 37 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #22158      +/-   ##
==========================================
- Coverage   69.77%   69.68%   -0.09%     
==========================================
  Files         449      449              
  Lines       38208    38188      -20     
==========================================
- Hits        26660    26613      -47     
- Misses      10122    10140      +18     
- Partials     1426     1435       +9     

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74aba8c...19b0fc9. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Somesh Kumar Gouda <someshkumargauda@gmail.com>

Signed-off-by: Somesh Kumar Gouda <someshkumargauda@gmail.com>
@Somesh42

Copy link
Copy Markdown
Author

/test pull-etcd-markdown-lint
/test pull-etcd-grpcproxy-integration-amd64

@Somesh42

Copy link
Copy Markdown
Author

Think we can add all benchmarks, however I think they need to be redesigned and updated to be more reflecting for performance dimensions we care about. See #16467 (comment) section about etcd SLI

/ok-to-test

Thanks @serathius.. makes sense.
Happy to keep these make targets as a first step to expose the existing
benchmarks through the same benchmark_test.sh path as bench-put.

For follow-up, I'll look at #16467 (SLI section) and propose dedicated
benchmark scenarios that map to the performance dimensions we care about,
rather than treating the current subcommands as the final suite.
Fixed the markdown lint failure in the README.

@Somesh42

Copy link
Copy Markdown
Author

All required checks are green now (including markdown-lint and the earlier grpcproxy flake).
Ready for review/approval when you have a moment @serathius

@serathius

serathius commented Jul 27, 2026

Copy link
Copy Markdown
Member

I don't think there is much value with replacing /scripts/benchmark_test.sh range mykey myend --total=10000 with make benchmark-range. What's the end goal?

@Somesh42

Copy link
Copy Markdown
Author

Thanks @serathius fair point.

You're right that wrapping scripts/benchmark_test.sh with more make bench-*
targets doesn't add much by itself. My goal was to follow the existing
bench-put pattern for discoverability, but that isn't enough of an end goal
for #16467.

What I want to work toward is:

  1. Define a small set of SLI-oriented benchmark scenarios (dedicated dimensions,
    not just one make target per existing subcommand)
  2. Wire those into the existing periodic/CI path with --report-perfdash
  3. Use that as the reproducible baseline for regression tracking

Given that, I'm happy to close this PR (or leave only docs if useful) and come
back with a proposal for SLI scenarios based on
#16467 (comment).

Would you prefer I close this and open a design/issue discussion first?

@serathius

Copy link
Copy Markdown
Member

You are not current assignee of #16467, not have you discussed what you are doing. Definitely sending PRs without discussing the direction will not move us forward.

@Somesh42

Copy link
Copy Markdown
Author

thanks @serathius — you're right, and I apologize for jumping ahead without
coordinating on #16467.
I'll close this PR. Adding more make bench-* wrappers isn't the right next
step without agreeing on SLI scenarios and who owns what on the issue.
I'll comment on #16467 to ask how I can help under the current plan, rather
than opening more implementation PRs on my own.
Thanks for the guidance.

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

Development

Successfully merging this pull request may close these issues.

2 participants