Skip to content

Adds cluster benchmark support for benchmark-on-label#3338

Merged
zuiderkwast merged 2 commits into
valkey-io:unstablefrom
roshkhatri:add-cluster-benchmark-label
Mar 9, 2026
Merged

Adds cluster benchmark support for benchmark-on-label#3338
zuiderkwast merged 2 commits into
valkey-io:unstablefrom
roshkhatri:add-cluster-benchmark-label

Conversation

@roshkhatri

@roshkhatri roshkhatri commented Mar 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Now we will be able to add a run-cluster-benchmark label to run a benchmark with cluster-mode enabled valkey-server

Answering some questions from the comments:

What is the cluster configuration that is used to test? How many nodes, cluster settings, shard configurations etc? Is that modifiable?

Will use this benchmark-config-arm.json config for cluster mode with a single clustermode enabled instance of valkey

Does it use single metal instance only? Does it use the same metal instance as non-cluster mode?

Yes it uses the same single instance for the benchmark

If yes, are the two different benchmarks (cluster and non-cluster) sequential? So that there is no noise in the runs?

Yes they are sequential in the same concurrency group group: ec2-al-2023-pr-benchmarking-arm64
I have not changes that.

@roshkhatri
adds cluster support for benchmark-on-label
51731b9 
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
@roshkhatri roshkhatri requested a review from zuiderkwast March 9, 2026 20:36
@roshkhatri
Formatting
e89e1da 
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
@sarthakaggarwal97

sarthakaggarwal97 commented Mar 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the change. I have some questions -

  1. What is the cluster configuration that is used to test? How many nodes, cluster settings, shard configurations etc? Is that modifiable?
  2. Does it use single metal instance only? Does it use the same metal instance as non-cluster mode?
  3. If yes, are the two different benchmarks (cluster and non-cluster) sequential? So that there is no noise in the runs?

zuiderkwast
zuiderkwast approved these changes Mar 9, 2026
View reviewed changes

@zuiderkwast zuiderkwast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, LGTM.

With this workflow, will it be possible to add both labels to a PR to have cluster-mode and standalone-mode benchmark running at the same time?

Comment thread .github/workflows/benchmark-on-label.yml
if: github.event.label.name == 'run-cluster-benchmark'
run: |
CONFIG_FILE="../valkey/.github/benchmark_configs/benchmark-config-arm.json"
sed -i 's/"cluster_mode": false/"cluster_mode": true/g' "$CONFIG_FILE"

@zuiderkwast zuiderkwast Mar 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clever idea! Just hack the config in-place with sed. 😎

@sarthakaggarwal97

sarthakaggarwal97 commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

@zuiderkwast it's pull_request_target, might not run right now.

@zuiderkwast

zuiderkwast commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

@zuiderkwast it's pull_request_target, might not run right now.

Yeah, I realized. I cancelled the run.

@sarthakaggarwal97

sarthakaggarwal97 commented Mar 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@roshkhatri can the action get expired if multiple runs are queued on the same instance? I think benchmark takes some time.

@zuiderkwast

zuiderkwast commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

@roshkhatri can the action get expired if multiple runs are queued on the same instance?

I don't know. Let's find out? I'll just merge it.

@zuiderkwast zuiderkwast merged commit 8caa292 into valkey-io:unstable Mar 9, 2026
122 of 124 checks passed
@sarthakaggarwal97

sarthakaggarwal97 commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

I like the yolo style not gonna lie :D

@zuiderkwast

zuiderkwast commented Mar 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

I started it on #2790. I can see it uses the command

taskset -c 96-191 /home/ec2-user/actions-runner/_work/valkey/valkey/valkey_latest/src/valkey-benchmark -h *** -p 6379 --duration 180 -r 3000000 -d 16 -P 1 -c 1600 -t SET --threads 90 --warmup 30 --seed 758025 --csv

I.e. it's not running with --cluster. That's good to know. This works for a single-node cluster and with single-key commands (but not the mset and mget tests). The keys are 16 bytes. (With --cluster the keys are 21 bytes with a hashtag of 3 bytes.)

@codecov

codecov Bot commented Mar 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.87%. Comparing base (fc217fc) to head (e89e1da).
⚠️ Report is 5 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #3338      +/-   ##
============================================
- Coverage     75.05%   74.87%   -0.18%     
============================================
  Files           129      129              
  Lines         71553    71632      +79     
============================================
- Hits          53705    53636      -69     
- Misses        17848    17996     +148

see 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JimB123 added a commit to JimB123/valkey that referenced this pull request Mar 10, 2026
@JimB123
Revert "Adds cluster benchmark support for benchmark-on-label (valkey…
9420039 
…-io#3338)"

This reverts commit 8caa292.
JimB123 pushed a commit that referenced this pull request Mar 19, 2026
@roshkhatri @JimB123
Adds cluster benchmark support for benchmark-on-label (#3338)
977c3ab 
Now we will be able to add a `run-cluster-benchmark` label to run a
benchmark with cluster-mode enabled valkey-server

It will use the config
https://github.com/valkey-io/valkey/blob/unstable/.github/benchmark_configs/benchmark-config-arm.json
modified for for cluster mode with a single clustermode enabled instance of
valkey.

It uses the same single instance for the benchmark as for run-benchmark.
 
If both labels are used, they are sequential in the same concurrency group `group:
ec2-al-2023-pr-benchmarking-arm64`.

---------

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuiderkwast zuiderkwast zuiderkwast approved these changes

Assignees

@roshkhatri roshkhatri

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@roshkhatri @sarthakaggarwal97 @zuiderkwast