register Elbencho as a CBT benchmark

[gitkenan]

Story 2 of 11

This is the foundation story for Elbencho S3 support in CBT. It does one thing: makes elbencho: a valid benchmark in a YAML test plan, on par with fio, radosbench, etc. However, this is incomplete; I've intentionally opted for making this work incremental to allow for more focus during reviews and better handling of requirements, especially since I'm new to this codebase and tool.

The only non-trivial decision here is how the factory handles Elbencho's config. Most benchmarks take scalar parameters and the factory Cartesian-expands any list values into separate runs through benchmarkfactory.py. Elbencho works differently — its iteration axes live inside a workloads dict that the class owns. A new guard in benchmarkfactory.py prevents the factory from expanding those lists and breaking that structure by checking for a particular function in the elbencho.py (and all other tools') class. This sets the pattern for any future benchmark that uses the same workloads model.

The run loop itself (the three-tier blocksize → threads → iodepth iteration and pdsh execution) is Story 3 and comes in the next PR. The _run_workloads() method here is a stub.

25 unit tests cover the construction, validation, and factory bypass behaviour.

Regarding the Pull Conquest review below

This is the result of an tool that I developed. It's a configurable & semi-automated AI code review tool using bob -p, python and vanilla html, js, and css.

[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!