add Elbencho S3 benchmark with async parallel SSH

[gitkenan]

Summary

• Add a stdlib-only parallel SSH layer to common.py using asyncio + the system ssh binary, so benchmarks can opt out of pdsh with no new dependencies
• Introduce benchmark/elbencho.py — an S3 benchmark that runs its entire lifecycle (binary check, cleandir, dropcaches, workload fan-out, result pull) through the async SSH path
• Register Elbencho in benchmarkfactory.py (import + dict entry only — no changes to get_all() or all_configs())
• 49 unit tests covering construction, validation, factory integration, CLI command building, run-loop fan-out, and pdsh-free lifecycle
• User-facing docs and a working example YAML config

How Elbencho avoids Cartesian expansion

Elbencho's list-valued parameters (threads, iodepth, blocksize) live inside the workloads: 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 pass
• Verify no regressions in existing benchmark tests

798 tests ran, all Elbencho tests (49) pass. The 7 failures are pre-existing post-processing test issues unrelated to our code.

• End-to-end run against an RGW cluster with the example YAML

[gitkenan]

██████╗ ██╗   ██╗██╗     ██╗      ██████╗ ██████╗ ███╗   ██╗ ██████╗ ██╗   ██╗███████╗███████╗████████╗
██╔══██╗██║   ██║██║     ██║     ██╔════╝██╔═══██╗████╗  ██║██╔═══██╗██║   ██║██╔════╝██╔════╝╚══██╔══╝
██████╔╝██║   ██║██║     ██║     ██║     ██║   ██║██╔██╗ ██║██║   ██║██║   ██║█████╗  ███████╗   ██║
██╔═══╝ ██║   ██║██║     ██║     ██║     ██║   ██║██║╚██╗██║██║▄▄ ██║██║   ██║██╔══╝  ╚════██║   ██║
██║     ╚██████╔╝███████╗███████╗╚██████╗╚██████╔╝██║ ╚████║╚██████╔╝╚██████╔╝███████╗███████║   ██║
╚═╝      ╚═════╝ ╚══════╝╚══════╝ ╚═════╝ ╚═════╝ ╚═╝  ╚═══╝ ╚══▀▀═╝  ╚═════╝ ╚══════╝╚══════╝   ╚═╝
                                        ── AI REVIEW ──

Scope

The 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 _run_workloads() stub is explicitly documented as Story 3 work. No scope creep detected.

Code Review

[P1] benchmark/elbencho.py:71 — The validation loop checks "s3_bucket" is present in the effective merged config, but the error message says "missing required key 's3_bucket'". However, s3_bucket could be defined at the top level (in _global_defaults) and inherited by workloads. The current code does not add s3_bucket to _global_defaults (lines 57-60 only snapshot cmd_path and auth), so a top-level s3_bucket would not be inherited. Either: (1) add s3_bucket to the snapshot if it exists in the top-level config, or (2) document that s3_bucket must be per-workload only. The yaml-config-reference.md shows s3_bucket as a workload-level key (required), so option (2) is correct — but the validation should happen before the merge to give a clearer error.

[P2] benchmarkfactory.py:48 — The comment says "yield a single instance with the raw config dict" but the code does configs = [dict(config)]. This creates a shallow copy, which is fine, but the comment could be clearer: "yield a single instance with a copy of the raw config dict (no Cartesian expansion)".

[P2] benchmark/elbencho.py:64 — The comment "Used to construct the three-tier directory structure (mode/threads/iodepth)" is accurate for Story 3 but misleading in Story 2 where base_run_dir is assigned but never used. Consider adding "(Story 3)" to the comment or deferring the assignment to Story 3.

[P2] tests/test_bm_elbencho.py:44 — The test fixture _FULL_CONFIG includes "secret_key=TEST_SECRET_KEY_DO_NOT_USE" in the auth config string. While this is clearly marked as a test value, it would be safer to use a placeholder like "secret_key=<redacted>" or "secret_key=***" to avoid any possibility of this being copy-pasted into real configs.

[P3] benchmark/elbencho.py:48 — The ValueError message "workloads must be a dict" could be more helpful by showing the actual type received: f"workloads must be a dict, got {type(self.workloads).__name__}".

[P3] benchmarkfactory.py:5-15 — The imports are now alphabetically sorted (good), but the original file had them grouped by benchmark type. The new ordering is fine, but it's a style change unrelated to the core feature. Not blocking, but worth noting for consistency with future PRs.

Security

Credential handling in tests: The test fixture _FULL_CONFIG embeds a fake secret key in plaintext. This is acceptable for unit tests but the comment should explicitly state it's a non-functional test value. The yaml-config-reference.md correctly documents three credential options (inline, env vars, session token) with appropriate warnings about inline credentials appearing in logs. No production credential leakage risk identified.

Summary

The registration is solid and the factory bypass logic is correct. The P1 issue around s3_bucket validation needs clarification — either allow top-level inheritance or validate strictly at the workload level before merging. The P2 items are minor clarity improvements. Tests are comprehensive and the pre-commit scoping is well-designed.

[gitkenan]

[perezjosibm]

Just a few changes needed, eg checking return values from pdsh, documentation. Looking very good, many thanks!

[harriscr]

I think we should make any changes in the Main branch (the one under active development) rather than the legacy Master branch which is very out of date and being used to maintain backwards compatibility with older versions of Ceph.

That explains why some of the code does not match the structure I was expecting.

[gitkenan]

I think we should make any changes in the Main branch (the one under active development) rather than the legacy Master branch which is very out of date and being used to maintain backwards compatibility with older versions of Ceph.

That explains why some of the code does not match the structure I was expecting.

Thanks for this. I've just force-updated and edited the PR to point at main instead. I've then re-done the code changes to align more with the main branch's current state and tried to quickly address your current comments.

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.

[lee-j-sanders]

Are there any dependent python modules that need to be pip install'd?
If so, we need to modify requirements.txt for those pre-reqs