Conversation
Author
ScopeThe diff matches the Jira ticket and PR description. Story 2 requires registering Elbencho as a benchmark class with factory integration, workload storage, and validation — all present. The Code Review[P1] [P2] [P2] [P2] [P3] [P3] SecurityCredential handling in tests: The test fixture SummaryThe registration is solid and the factory bypass logic is correct. The P1 issue around |
gitkenan
force-pushed
the
elbencho-register-bench
branch
11 times, most recently
from
August 10, 2026 08:21
7e64adb to
89776a3
Compare
perezjosibm
reviewed
Aug 10, 2026
perezjosibm
reviewed
Aug 10, 2026
gitkenan
force-pushed
the
elbencho-register-bench
branch
3 times, most recently
from
August 10, 2026 13:12
c6bd7e0 to
550e5d4
Compare
Author
|
gitkenan
force-pushed
the
elbencho-register-bench
branch
6 times, most recently
from
August 11, 2026 16:35
6463326 to
f0c2812
Compare
perezjosibm
requested changes
Aug 12, 2026
perezjosibm
left a comment
perezjosibm
left a comment
Contributor
There was a problem hiding this comment.
Just a few changes needed, eg checking return values from pdsh, documentation. Looking very good, many thanks!
gitkenan
changed the title
gitkenan
force-pushed
the
elbencho-register-bench
branch
2 times, most recently
from
August 13, 2026 08:03
7831e76 to
9a34804
Compare
Author
Thanks for this. I've just force-updated and edited the PR to point at I've decided to just lump all of the current changes into this PR, because it's difficult to manage two branches at once as the commits are changing 😄 Apologies that it's become a big PR, but I think for now, hopefully it's good enough that I've split it into different commits. Since the PR is now larger in scope (effectively covering stories 2 and 3), I've also updated the PR title and description. |
gitkenan
changed the title
Author
|
This code change has now been verified to be working on a live cluster. Here's the raw log of the run: |
perezjosibm
approved these changes
Sep 1, 2026
gitkenan
force-pushed
the
elbencho-register-bench
branch
from
September 2, 2026 11:10
ca6b119 to
31a9e54
Compare
harriscr
reviewed
Sep 3, 2026
harriscr
reviewed
Sep 3, 2026
harriscr
reviewed
Sep 3, 2026
pdsh is being dropped from Rocky Linux 10, so the command fan-out mechanism is being moved behind an interface it can be swapped out from incrementally rather than replaced in place. The new remote/ package holds a RemoteExecutor abstract base class (run_command / run_command_with_error_checking) and a stdlib-only AsyncSSHExecutor implementation (asyncio + the system ssh binary, no third-party packages), plus the pdsh-free cluster helpers make_remote_dir/clean_remote_dir/ sync_files. common.py's pdsh code is left untouched so every existing benchmark keeps working unchanged; docs/ReplacingPdsh.md tracks the remaining migration (ceph tracker #80193). Signed-off-by: Kenan Al-Shamie <kenan.al-shamie@ibm.com> Assisted-by: Claude-v2.1.212:claude-opus-4-8
gitkenan
force-pushed
the
elbencho-register-bench
branch
7 times, most recently
from
September 8, 2026 09:07
ca7c886 to
c2acff8
Compare
harriscr
reviewed
Sep 8, 2026
| @@ -0,0 +1,41 @@ | |||
| """ | |||
Contributor
There was a problem hiding this comment.
It would be good to type all the function variables as well e.g.
def make_remote_dir(self, remote_dir) -> None:
becomes (assuming remote_dir is a string):
def make_remote_dir(self, remote_dir: str) -> None:
harriscr
reviewed
Sep 8, 2026
harriscr
reviewed
Sep 8, 2026
harriscr
reviewed
Sep 8, 2026
harriscr
left a comment
harriscr
left a comment
Contributor
There was a problem hiding this comment.
Looking good. I like the fact we have a good set of unit tests for the new code. The structure for the remote_executor works nicely and should be easy to fit pdsh into.
Just some minor things to look at and it will be good to go
| if isinstance(item, BaseException): | ||
| errors.append(str(item)) | ||
| logger.warning("scp: failed to launch process: %s", item) | ||
| continue |
Contributor
There was a problem hiding this comment.
This code is duplicated in 2 places. It could be an opportunity to streamline the code in the future. I wouldn't necessarily worry about it for this PR though
gitkenan
force-pushed
the
elbencho-register-bench
branch
from
September 11, 2026 14:24
c2acff8 to
a093d6f
Compare
Introduce the Elbencho benchmark for S3 object workloads. It builds commands through the shared Workloads pipeline the same way librbdfio does — an ElbenchoCommand (command/elbencho_command.py) created via Workload._create_command_class, with list-valued params expanded by all_configs() — and fans the generated command strings out through a RemoteExecutor, so its whole lifecycle (binary check, dropcaches, directory setup, workload runs, result sync) is pdsh-free. The one non-obvious piece is auth threading: S3 credentials arrive as a nested dict, which the Workloads global-option collection would stringify, so they are flattened into flat string options before the base class builds the pipeline. A new execution-agnostic Workloads.command_groups() generator exposes the per-cell commands so Elbencho can drive them without the still-pdsh-based Workloads.run(). Includes the unit test suite. Signed-off-by: Kenan Al-Shamie <kenan.al-shamie@ibm.com> Assisted-by: Claude-v2.1.212:claude-opus-4-8
User-facing guide for running the Elbencho S3 benchmark via CBT: the test-plan YAML structure, the blocksize/size interaction (single-PUT vs multipart upload and the 5 MB minimum part size), running instructions, and expected result output. Links Elbencho from docs/Workloads.md. Signed-off-by: Kenan Al-Shamie <kenan.al-shamie@ibm.com> Assisted-by: Claude-v2.1.212:claude-opus-4-8
gitkenan
force-pushed
the
elbencho-register-bench
branch
from
September 11, 2026 14:42
a093d6f to
3ecc16d
Compare
Author
|
All of the above should be acknowledged now, or documented for soon-to-comes. The latest force-push has been tested with the following success log for running Elbencho: |
harriscr
reviewed
Sep 15, 2026
|
|
||
| _MODES_NO_BLOCKSIZE = {"stat", "list"} | ||
|
|
||
| def __init__(self, options: dict, workload_output_directory: str) -> None: |
Contributor
There was a problem hiding this comment.
we're still not sub-typing the dictionary here. and at line 53, 81, 106
harriscr
reviewed
Sep 15, 2026
| # ``options`` is the raw config from the YAML/test plan: heterogeneous | ||
| # values (ints, bools, strings). _parse_options() is the boundary that | ||
| # normalizes it into the str|None CliOptions store. | ||
| def __init__(self, options: Mapping[str, Any]) -> None: |
Contributor
There was a problem hiding this comment.
I don't like the fact we have loosened the typing here and in the signatures of the rest of the methods. Mypy will ignore checking any Any typed things, so we lose strictness inherent here.
I'm fairly sure that on the rbdfio path options are always a dict[str,str]. It's definitely processed from the raw yaml before it arrives here
Bob suggests:
"By the time options reaches RbdFioCommand.init or ElbenchoCommand.init, the type is provably dict[str, str]:"
but take that with the correct level of scepticism
harriscr
reviewed
Sep 15, 2026
| _DIRECT_TRANSLATIONS: list[str] = ["numjobs", "iodepth"] | ||
|
|
||
| def __init__(self, options: dict[str, str], workload_output_directory: str) -> None: | ||
| def __init__(self, options: Mapping[str, Any], workload_output_directory: str) -> None: |
Contributor
There was a problem hiding this comment.
as with the comment in command.py, I don't like the loosening of the type checking here. Especially as it's been changed for the existing classes, but not for the elbencho command class which is being added
Same for lines 40, 46 and 51
harriscr
reviewed
Sep 15, 2026
| _RBD_DEFAULT_OPTIONS: dict[str, str] = {"ioengine": "rbd", "clientname": "admin"} | ||
|
|
||
| def __init__(self, options: dict[str, str], workload_output_directory: str) -> None: | ||
| def __init__(self, options: Mapping[str, Any], workload_output_directory: str) -> None: |
Contributor
There was a problem hiding this comment.
as with the comment in command.py, I don't like the loosening of the type checking here. Especially as it's been changed for the existing classes, but not for the elbencho command class which is being added
and line 39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
common.pyusingasyncio+ the systemsshbinary, so benchmarks can opt out ofpdshwith no new dependenciesbenchmark/elbencho.py— an S3 benchmark that runs its entire lifecycle (binary check, cleandir, dropcaches, workload fan-out, result pull) through the async SSH pathbenchmarkfactory.py(import + dict entry only — no changes toget_all()orall_configs())How Elbencho avoids Cartesian expansion
Elbencho's list-valued parameters (
threads,iodepth,blocksize) live inside theworkloads:block, not at the top level.all_configs()only sees top-level keys, which are all scalars or dicts, so it naturally yields a single config — no special-case code in the factory needed.Test plan
python -m unittest tests.test_bm_elbencho— 49 tests pass798 tests ran, all Elbencho tests (49) pass. The 7 failures are pre-existing post-processing test issues unrelated to our code.