Revert to execute-test / test_execution terminology (3.0)

[OVI3D0]

Description

First of the stacked OpenSearch Benchmark 3.0 PRs. This reverts the 2.x-era terminology back to the 3.0 (and original 1.x) vocabulary, with full back-compat so existing automation and historical data keep working.

CLI

• execute-test is the primary subcommand again; run / execute remain as deprecated aliases (they warn, then dispatch).
• --test-execution-id is primary; --test-run-id remains as a legacy alias.

Internal rename

• test_run / TestRun → test_execution / TestExecution across metrics, aggregator, worker_coordinator, builder, paths, publisher, and the orchestrator (module test_run_orchestrator.py → test_execution_orchestrator.py).
• On disk: test-runs/ → test-executions/; test_run.json → test_execution.json.
• Datastore (OpenSearch): indices benchmark-test-runs-* → benchmark-test-executions-*; stored field test-run-id → test-execution-id.

Back-compat — reads accept BOTH, writes emit the new vocabulary

• TestExecution.from_dict accepts either test-execution-id/test-run-id and test-execution-timestamp/test-run-timestamp.
• The node-stats CPU-window query and list accept both old and new names.
• Seamless 1.x → 3.0 migration: the reader also accepts genuine 1.x artifacts — the pre-2.x provision-config-instance (+ -params, + cluster.provision-config-revision) field names as fallbacks for the reverted cluster-config-* names, and the 1.x on-disk test_executions/ (underscore) directory layout. Without this, a 1.x record silently dropped from list on the file path and crashed list on the OpenSearch-datastore path (both funnel through from_dict). Reads only — 3.0 always writes the new vocabulary.

No behavior change beyond naming and widened back-compat reads. Multi-engine support and the CloudWatch datastore build on this foundation in subsequent stacked PRs.

Note for reviewers

• version.txt is intentionally left at 2.4.0; the 3.0.0 bump lands in its own dedicated PR at release time (matching repo convention, e.g. Bump version to 2.4.0 #1090).
• Pre-existing minor flag inconsistency, not introduced here: aggregate uses --test-executions-id (plural) while execute-test uses --test-execution-id (singular); both have working legacy aliases. Can be smoothed here or left.

Issues Resolved

Restores the original 1.x execute-test / test_execution vocabulary that 2.x had renamed.

Testing

• New functionality includes testing

• Unit: full suite passes (1435 passed / 5 skipped; engine + metrics_stores suites belong to later stacked PRs). Includes 13+ back-compat assertions exercising the legacy test-run-id key, plus two new tests for genuine 1.x artifacts (from_dict parsing the provision-config-* vocab, and reading the 1.x underscore on-disk layout).

• Live e2e on real AWS infra (OpenSearch 2.19.1): full execute-test write→read round-trip confirming the new test-executions/ dir, test_execution.json, and test-execution-id/test-execution-timestamp fields with no legacy keys emitted; OpenSearch-datastore path creating benchmark-{metrics,results,test-executions}-* indices with all metric docs queryable by test-execution-id; read-both proven on a live index (synthetic legacy test-run-id doc matched by the back-compat query); list / compare / aggregate / visualize exercised via both new and legacy flags; a full (non-test-mode) benchmark producing real throughput/latency.

• 1.x → 3.0 migration proven live: genuine 1.x artifacts produced by OpenSearch Benchmark 1.18.0 (on-disk test_executions/ underscore dir + a datastore run doc carrying provision-config-instance) were read by this branch — list, find, and compare all succeed on the on-disk run, and the datastore list reads the 1.x doc without the previous from_dict crash.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 62d5ded.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
create-notice.sh	82	high	Three new optional dependencies (pyvespa, pymilvus, clickhouse-connect) are being added to the project's license notice, indicating these packages are being bundled as optional extras. Per mandatory supply-chain policy, any dependency addition must be flagged for maintainer verification — namespace hijacking and typosquatting cannot be ruled out without explicit review of the referenced package artifacts.
osbenchmark/metrics.py	1595	low	A `print("[DEBUG]: ", report_file)` statement was introduced in `FileTestExecutionStore.store_html_results()`. This unconditionally leaks the on-disk report file path to stdout in production, which could expose internal directory structure (including test execution IDs) to anyone with access to process output.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f69100c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections osbenchmark/metrics.py [1477]: cluster is optional for BWC. This can be removed after some grace period.

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue extract_user_tags_from_config uses or to fall back from the reverted test_execution section to the legacy test_run section. When the primary key returns an empty dict {} (a valid "no tags" value), the or will evaluate falsy and fall through to the legacy key. If the legacy key is missing (mandatory=False returns None), this yields None which is then passed to extract_user_tags_from_string, potentially causing a TypeError depending on that function's handling of None vs empty dict. # Phase B: prefer the reverted config section, fall back to the pre-revert # section so the tree stays runnable until the CLI producer flips. user_tags = cfg.opts("test_execution", "user.tag", mandatory=False) \ or cfg.opts("test_run", "user.tag", mandatory=False) return extract_user_tags_from_string(user_tags) Possible Issue TestExecution.from_dict uses explicit "in" checks for test-execution-id/test-run-id and cluster-config-instance/provision-config-instance, so a document missing both raises KeyError. The _to_test_executions helper catches BaseException and logs "incompatible format" but this means genuinely malformed OS-stored docs will be silently skipped during list. More critically, find_by_test_execution_id (OpenSearch path) does not wrap from_dict in try/except, so a single legacy doc missing required fields will crash find_by_test_execution_id rather than raising NotFound. # back-compat: accept both 3.x test-execution-id and pre-3.x test-run-id. # Explicit "in" checks (not .get) so a doc missing BOTH keys still raises KeyError. test_execution_id = d["test-execution-id"] if "test-execution-id" in d else d["test-run-id"] test_execution_timestamp = d["test-execution-timestamp"] if "test-execution-timestamp" in d \ else d["test-run-timestamp"] # back-compat: accept the pre-2.x provision-config-* field names (1.x vocab) as # fallbacks for the reverted cluster-config-* names. Explicit "in" on the required # config-instance field so a doc missing BOTH still raises KeyError (matches id/timestamp). cluster_config = d["cluster-config-instance"] if "cluster-config-instance" in d \ else d["provision-config-instance"] cluster_config_params = d.get("cluster-config-instance-params", d.get("provision-config-instance-params")) return TestExecution(d["benchmark-version"], d.get("benchmark-revision"), d["environment"], test_execution_id, time.from_is8601(test_execution_timestamp), d["pipeline"], user_tags, d["workload"], d.get("workload-params"), d.get("test_procedure"), cluster_config, cluster_config_params, d.get("plugin-params"), workload_revision=d.get("workload-revision"), cluster_config_revision=cluster.get("cluster-config-revision", cluster.get("provision-config-revision")), distribution_version=cluster.get("distribution-version"), distribution_flavor=cluster.get("distribution-flavor"), revision=cluster.get("revision"), results=d.get("results"), meta_data=d.get("meta", {})) CLI Regression The --database-type argument and associated config wiring (cfg.add(..., "database", "type", database_type)) were removed. If worker_coordinator or the database/registry module still reads cfg.opts("database", "type") to select the DatabaseClient factory, execute-test will fail at runtime with a missing config option. Verify that the database registry code path is removed consistently or that a default is provided elsewhere. grpc_target_hosts = opts.TargetHosts(args.grpc_target_hosts) if hasattr(args, "grpc_target_hosts") and args.grpc_target_hosts else None cfg.add(config.Scope.applicationOverride, "client", "grpc_hosts", grpc_target_hosts) if "timeout" not in client_options.default: console.info("You did not provide an explicit timeout in the client options. Assuming default of 10 seconds.") if list(target_hosts.all_hosts) != list(client_options.all_client_options): Argument Naming Inconsistency The aggregate subcommand defines --test-executions-id (plural "executions") as the new primary name alongside legacy --test-runs-id, both mapping to dest test_execution_id. The singular form (--test-execution-id) would be more consistent with all other subcommands (start, visualize, execute-test) which use singular. This inconsistency will confuse users following the documented convention. aggregate_parser.add_argument( "--test-executions-id", "--test-runs-id", "-tid", dest="test_execution_id", help="Define a unique id for this aggregated test-execution.", default="") Assertion Message The assertion messages still reference "test run id" / "test run timestamp" instead of "test execution id" / "test execution timestamp" after the rename. Minor, but inconsistent with the rest of the reverted terminology and could confuse debugging. assert self._test_execution_id is not None, "Attempting to open metrics store without a test run id" assert self._test_execution_timestamp is not None, "Attempting to open metrics store without a test run timestamp"

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f69100c
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Make fallback config lookup non-mandatory The fallback cfg.opts("system", "test_run.id") is called without mandatory=False, so if neither key is set it will raise instead of returning None. Since this is a Phase B compatibility fallback, pass mandatory=False to both calls and then explicitly validate/raise a clear error if the result is None. osbenchmark/metrics.py [316-318] test_execution_id = cfg.opts("system", "test_execution.id", mandatory=False) \ - or cfg.opts("system", "test_run.id") + or cfg.opts("system", "test_run.id", mandatory=False) +if not test_execution_id: + raise exceptions.SystemSetupError("No test execution id configured (system.test_execution.id).") test_execution_timestamp = cfg.opts("system", "time.start") Suggestion importance[1-10]: 7 __ Why: Valid concern: the fallback cfg.opts("system", "test_run.id") lacks mandatory=False, which could raise an exception when neither key is set, defeating the purpose of the Phase B compatibility fallback.	Medium
General	Apply Phase B fallback for consistency dispatch_visualize reads only the reverted key system.test_execution.id with mandatory=True, but elsewhere in this PR the same key is read with a fallback to system.test_run.id during the Phase B migration. This will crash if only the legacy key is populated. Apply the same fallback pattern for consistency. osbenchmark/benchmark.py [871] def dispatch_visualize(cfg): - test_execution_id = cfg.opts("system", "test_execution.id") + test_execution_id = cfg.opts("system", "test_execution.id", mandatory=False) \ + or cfg.opts("system", "test_run.id") Suggestion importance[1-10]: 6 __ Why: Reasonable consistency concern; other places in the PR apply the Phase B fallback pattern, and dispatch_visualize could crash if only the legacy key is populated during migration.	Low
	Robustly parse mixed legacy/new ctx keys A ctx produced by the new open_context (which only has test-execution-* keys) will work, but a mixed/partial ctx containing only test-execution-id but not test-execution-timestamp (or vice versa) would raise KeyError on the missing legacy key. Use .get with a fallback to make the back-compat parsing symmetric and robust. osbenchmark/metrics.py [426-430] if ctx: # back-compat: accept both 3.x test-execution-id and pre-3.x test-run-id. - self._test_execution_id = ctx["test-execution-id"] if "test-execution-id" in ctx else ctx["test-run-id"] - self._test_execution_timestamp = ctx["test-execution-timestamp"] if "test-execution-timestamp" in ctx \ - else ctx["test-run-timestamp"] + self._test_execution_id = ctx.get("test-execution-id", ctx.get("test-run-id")) + self._test_execution_timestamp = ctx.get("test-execution-timestamp", ctx.get("test-run-timestamp")) Suggestion importance[1-10]: 5 __ Why: The suggestion improves robustness for mixed ctx dicts, though in practice ctx is produced by open_context in a consistent format, so the risk is limited.	Low
	Clarify workload-drift error message The error message wording ("aggregate results generated with the same workload version...") is confusing — it reads like an instruction but is missing a verb. Consider rewording for clarity so users know they must re-aggregate using the original workload version. osbenchmark/aggregator.py [229-233] iterations_per_run = [] for test_id in self.test_executions.keys(): run_iterations = self.accumulated_iterations.get(test_id, {}) if task_name not in run_iterations: raise exceptions.SystemSetupError( f"Task '{task_name}' is present in the stored results of test execution " f"'{test_id}' but not in the loaded workload's schedule. The workload " - f"definition likely changed since the run; aggregate results generated " - f"with the same workload version used for the original runs.") + f"definition likely changed since the run; please re-run aggregation " + f"using the same workload version that produced the original runs.") iterations_per_run.append(run_iterations[task_name]) Suggestion importance[1-10]: 3 __ Why: Minor wording improvement for an error message; the original is understandable but slightly awkward. Low impact.	Low

---

Previous suggestions

Suggestions up to commit f24e550

Category	Suggestion	Impact
General	Remove dead legacy CLI fallback The aggregate parser now sets dest="test_execution_id" for both --test-executions-id and --test-runs-id, so args.test_run_id will never be populated by argparse — the getattr(self.args, 'test_run_id', None) fallback is dead code. Either remove the fallback, or (if intending real back-compat) restore a separate legacy dest. As written, the comment misleads readers about actual behavior. osbenchmark/aggregator.py [187-192] -elif getattr(self.args, 'test_execution_id', None) or getattr(self.args, 'test_run_id', None): - # back-compat: the CLI dest becomes test_execution_id, but the deprecated - # --test-run-id alias may still populate test_run_id until the CLI flip lands. - arg_test_execution_id = getattr(self.args, 'test_execution_id', None) \ - or getattr(self.args, 'test_run_id', None) +elif getattr(self.args, 'test_execution_id', None): + arg_test_execution_id = self.args.test_execution_id test_execution_id = f"aggregate_results_{test_execution.workload}_{arg_test_execution_id}" Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that since the argparse dest is test_execution_id for both aliases, test_run_id fallback is dead code. This is a legitimate cleanup, though minor and the misleading comment is the main issue.	Low
	Remove dead alias branches in dispatch Registering run and execute as argparse aliases means handle_command_suggestions() will never trigger the deprecation warning through the normal dispatch path — argparse resolves the alias before your sys.argv[1] check runs (it does run first, but subsequent parsing succeeds silently). This is fine, but note that dispatch_sub_command checks sub_command in ("execute-test", "run", "execute") — however, argparse sets args.subcommand to the canonical name "execute-test" when aliases are used, so the "run"/"execute" branches are dead code. Simplify to just sub_command == "execute-test". osbenchmark/benchmark.py [124] +test_execution_parser = subparsers.add_parser("execute-test", aliases=["run", "execute"], help="Run a benchmark") - Suggestion importance[1-10]: 4 __ Why: The observation about argparse aliases resolving to the canonical name is correct, making the "run"/"execute" checks in dispatch dead code. However, the improved_code is identical to existing_code, so it's more of an observation than an actionable fix.	Low
	Handle unmapped timestamp sort ordering The test_procedure field is now read via d.get("test_procedure") while other required fields still use direct indexing. If callers rely on parsing back-compat docs, ensure required fields are consistently handled; additionally, the sort in list() may return docs missing both timestamps at the top since unmapped_type treats missing values as null. Consider adding a missing clause (e.g. "missing": "_last") to keep such docs from surfacing first. osbenchmark/metrics.py [1481-1488] test_execution_id = d["test-execution-id"] if "test-execution-id" in d else d["test-run-id"] test_execution_timestamp = d["test-execution-timestamp"] if "test-execution-timestamp" in d \ else d["test-run-timestamp"] -# back-compat: accept the pre-2.x provision-config-* field names (1.x vocab) as -# fallbacks for the reverted cluster-config-* names. Explicit "in" on the required -# config-instance field so a doc missing BOTH still raises KeyError (matches id/timestamp). cluster_config = d["cluster-config-instance"] if "cluster-config-instance" in d \ else d["provision-config-instance"] Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid but minor concern about sort ordering when timestamp fields are missing. The improved_code is essentially identical to the existing_code (just removes comments), providing no actionable change.	Low
	Stabilize result schema when samples missing When stats is None or percentiles_list is empty, the function falls through without populating percentile keys, but the returned dict may still be consumed by downstream code expecting those keys. Ensure downstream consumers tolerate absent percentile keys, or explicitly set them to None here to keep the schema stable across runs with/without samples. osbenchmark/metrics.py [2049-2054] if percentiles_list and stats: # modified from single_latency() - # Guard on `stats`: get_stats() returns None when the metric has no - # Normal samples (e.g. a task where every request errored). Without - # this guard `stats["count"]` raises TypeError and aborts the whole - # results calculation of an otherwise-completed run. sample_size = stats["count"] Suggestion importance[1-10]: 3 __ Why: Valid concern about schema stability, but the improved_code is identical to existing_code, and the suggestion is speculative without evidence downstream consumers actually require these keys.	Low

[github-actions]

Persistent review updated to latest commit f69100c

[rishabh6788]

LGTM, hold the merge for now.