Address ONTAP benchmark review comments

piyush5netapp · Aug 7, 2026 · Sep 30, 2026 · Oct 2, 2026 · Oct 7, 2026 · Oct 12, 2026
commit 436cf62073c97cc43861ce37e75b3a5a060e83b1
diff --git a/private-cicd/benchmark/ontap/AGENTS.md b/private-cicd/benchmark/ontap/AGENTS.md
@@ -0,0 +1,25 @@
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied.  See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+# ONTAP scale benchmark
+
+- Refer to the scale matrix from `README.md` and `USAGE.md` only.
+- Protocols, checkpoints, and concurrency levels live in `config.yaml`. `run.py` is the entry point. Do not add a CLI flag for a field that already exists in config.
+- Use `benchmark_support` for run ids, resource names, `mean_seconds`, and logging. Sequential and concurrency scripts must use the same helpers and the same log format. Do not hand-roll a second average or a second logger.
+- Run ids are letters, digits, and underscores. Storage pool names disallow `-`. VM hostnames disallow `_`; `format_vm_name` / `vm_name_token` are the only rewrite, and cleanup must use them so created VMs are the ones deleted.
+- Cleanup must stay inside the configured name prefix, zone, cluster (pools), and storage provider. Do not delete a resource just because its name contains a short substring.
diff --git a/private-cicd/benchmark/ontap/README.md b/private-cicd/benchmark/ontap/README.md
@@ -100,51 +100,52 @@ real lab IPs/credentials or run output.
 
 ## Running
 
-```bash
-# Sanity-check either script/config without hitting a real management server:
-python3 benchmark_storage_pool_sequential.py --config config.yaml --dry-run
-python3 benchmark_storage_pool_concurrency.py --config config.yaml --dry-run
+`run.py` is the entry point. Protocols, checkpoints, and concurrency levels
+come from `config.yaml` (`ontap` / `benchmark` for pools, `vm_bench` for
+VMs). Drop a protocol block to skip it, and edit `sequential_checkpoints` or
+`concurrency_levels` to change the matrix.
 
-# Sequential scale matrix, both protocols, up to config's sequential_checkpoints:
-python3 benchmark_storage_pool_sequential.py --config config.yaml
+Run ids are generated as `RUN_<UTC timestamp>_<4 hex chars>` (for example
+`RUN_20260927_081500_a1b2`). Letters, digits, and underscores only: a hyphen
+is rewritten to `_` because it is embedded in the storage pool name. Reuse
+one id across sequential and concurrency when you want a single report.
 
-# Sequential scale matrix, NFS3 only:
-python3 benchmark_storage_pool_sequential.py --config config.yaml --protocol nfs3
+```bash
+# Sanity-check without hitting a real management server:
+python3 run.py --test storage-pool-sequential --config config.yaml --dry-run
+python3 run.py --test storage-pool-concurrency --config config.yaml --dry-run
 
-# Concurrency matrix, both protocols, up to config's concurrency_levels (e.g. 30):
-python3 benchmark_storage_pool_concurrency.py --config config.yaml
+# Sequential scale matrix, using the protocols and checkpoints in config.yaml:
+python3 run.py --test storage-pool-sequential --config config.yaml
 
-# Concurrency matrix, iSCSI only, overriding the levels to run:
-python3 benchmark_storage_pool_concurrency.py --config config.yaml --protocol iscsi --levels 2,5,10,20,30
+# Concurrency matrix, using config.yaml's concurrency_levels:
+python3 run.py --test storage-pool-concurrency --config config.yaml
 
-# Use the SAME --run-id across both scripts to combine 5.1.x + 6.1.x into one
-# summary_<run_id>.csv / report (run sequential first, review it, then decide
-# whether to proceed with concurrency under the same run id):
-python3 benchmark_storage_pool_sequential.py --config config.yaml --run-id RUN-0001
-python3 benchmark_storage_pool_concurrency.py --config config.yaml --run-id RUN-0001
+# Same --run-id across both tests to combine 5.1.x + 6.1.x into one report.
+# Run sequential first, review it, then decide on concurrency:
+python3 run.py --test storage-pool-sequential --config config.yaml --run-id RUN_20260927_081500_a1b2
+python3 run.py --test storage-pool-concurrency --config config.yaml --run-id RUN_20260927_081500_a1b2
 ```
 
-At the end of a real run each script automatically checks for and deletes any
-storage pool whose name still contains the run id (safety net for pools that
-were created but never got torn down because of a mid-run crash). Pass
-`--skip-cleanup` to disable that and inspect the pools yourself.
+At the end of a real run each script deletes leftover pools from that run id.
+The sweep only deletes pools that match this config's name prefix, name
+pattern, zone, cluster, and storage provider, so other pools in a shared zone
+are left alone. Pass `--skip-cleanup` to inspect the pools yourself.
 
-If a run crashes hard (script killed, network partition, etc.) and left
-pools behind, recover with either script's `--cleanup-only` (they share the
-same cleanup logic):
+If a run crashes and leaves pools behind:
 
 ```bash
-# Deletes every pool whose name contains "bench_ontap" (the default prefix)
-python3 benchmark_storage_pool_sequential.py --config config.yaml --cleanup-only
+# Deletes benchmark pools for this config's pool_name_prefix (still provider/zone/cluster scoped):
+python3 run.py --test storage-pool-sequential --config config.yaml --cleanup-only
 
-# Or scope it to one specific run:
-python3 benchmark_storage_pool_sequential.py --config config.yaml --cleanup-only RUN_20260722_101500
+# Or scope it to one specific run (underscores, not hyphens):
+python3 run.py --test storage-pool-sequential --config config.yaml --cleanup-only RUN_20260927_081500_a1b2
 ```
 
 ## Rendering the Confluence-ready report
 
 ```bash
-python3 render_report.py --run-id RUN-20260722-101500 \
+python3 render_report.py --run-id RUN_20260722_101500_a1b2 \
     --cloudstack-build 4.23.0.0-SNAPSHOT --ontap-version 9.15.1
 ```
 
@@ -182,25 +183,23 @@ matching offerings - it does not create/destroy the pool itself:
 
 ```bash
 # Sanity-check without hitting a real management server:
-python3 benchmark_vm_instance_sequential.py --config config.yaml --dry-run
-python3 benchmark_vm_instance_concurrency.py --config config.yaml --dry-run
+python3 run.py --test vm-instance-sequential --config config.yaml --dry-run
+python3 run.py --test vm-instance-concurrency --config config.yaml --dry-run
 
-# Sequential scale matrix (5.2.1/5.2.2), both protocols:
-python3 benchmark_vm_instance_sequential.py --config config.yaml
+# Sequential scale matrix (5.2.1/5.2.2), protocols from config.yaml:
+python3 run.py --test vm-instance-sequential --config config.yaml
 
-# Concurrency matrix (6.2.1/6.2.2), iSCSI only, custom levels:
-python3 benchmark_vm_instance_concurrency.py --config config.yaml --protocol iscsi --levels 1,2,5,10
+# Concurrency matrix (6.2.1/6.2.2). Levels come from vm_bench.concurrency_levels:
+python3 run.py --test vm-instance-concurrency --config config.yaml
 
-# Both matrices in one invocation, combined into a single summary/report:
-python3 benchmark_vm_instance_combined.py --config config.yaml
+# Both matrices in one invocation:
+python3 run.py --test vm-instance-combined --config config.yaml
 
-# Or run the two standalone scripts under the SAME --run-id to combine them
-# instead (lets you review the sequential results before committing to
-# concurrency):
-python3 benchmark_vm_instance_sequential.py --config config.yaml --run-id RUN-0001
-python3 benchmark_vm_instance_concurrency.py --config config.yaml --run-id RUN-0001
+# Or run the two tests under the SAME --run-id (underscores only):
+python3 run.py --test vm-instance-sequential --config config.yaml --run-id RUN_20260927_081500_a1b2
+python3 run.py --test vm-instance-concurrency --config config.yaml --run-id RUN_20260927_081500_a1b2
 
-python3 render_report.py --run-id RUN-0001 --raw-prefix raw_ops_vm --summary-prefix summary_vm --report-suffix _vm
+python3 render_report.py --run-id RUN_20260927_081500_a1b2 --raw-prefix raw_ops_vm --summary-prefix summary_vm --report-suffix _vm
 ```
 
 All three scripts share the same `--skip-cleanup` / `--cleanup-only` safety

diff --git a/private-cicd/benchmark/ontap/USAGE.md b/private-cicd/benchmark/ontap/USAGE.md
@@ -71,6 +71,7 @@ cluster IDs, and ONTAP connection details (see inline comments in
 
 | File | Purpose |
 |---|---|
+| `benchmark_support.py` | Shared run ids, resource names, `mean_seconds`, and the one log format used by every script. |
 | `cloudstack_client.py` | Minimal CloudStack HTTP/REST client (session-key login, generic `call()`, automatic `queryAsyncJobResult` polling). Used by every benchmark script. |
 | `storage_pool_common.py` | Shared helpers for the storage-pool scripts: `createStoragePool`/`deleteStoragePool` request building, CSV logging (`RawLogger`, `append_summary_csv`), cleanup-by-filter, config loading. |
 | `vm_instance_common.py` | Shared helpers for the VM-instance scripts: `deployVirtualMachine`/`destroyVirtualMachine` request building (incl. force-purging data disks and `Destroy`-state volumes so ONTAP space is reclaimed immediately instead of waiting for CloudStack's 24h cleanup delay), CSV logging, cleanup-by-filter, config loading. |
@@ -82,20 +83,25 @@ Reproduces Confluence sections 5.1 (sequential) and 6.1 (concurrency) -
 
 | Script | What it does | Typical command |
 |---|---|---|
-| `benchmark_storage_pool_sequential.py` | Creates storage pools one at a time (per protocol) up to N=30, then deletes them one at a time, logging every call and reporting checkpoint totals at N = 1, 5, 10, 20, 30. | `python3 benchmark_storage_pool_sequential.py --config config.yaml` |
-| `benchmark_storage_pool_concurrency.py` | For each concurrency level C (default 2, 5, 10, 20, 30), creates C pools in parallel via a thread pool, records wall-clock time for the whole batch, then deletes the same C pools in parallel. | `python3 benchmark_storage_pool_concurrency.py --config config.yaml` |
+| `run.py` | Entry point. Loads `config.yaml` and runs the test named by `--test`. | `python3 run.py --test storage-pool-sequential --config config.yaml` |
+| `benchmark_storage_pool_sequential.py` | Creates storage pools one at a time (per protocol) up to N=30, then deletes them one at a time, logging every call and reporting checkpoint totals at N = 1, 5, 10, 20, 30. | `python3 run.py --test storage-pool-sequential --config config.yaml` |
+| `benchmark_storage_pool_concurrency.py` | For each concurrency level C (default 2, 5, 10, 20, 30), creates C pools in parallel via a thread pool, records wall-clock time for the whole batch, then deletes the same C pools in parallel. | `python3 run.py --test storage-pool-concurrency --config config.yaml` |
 
 Common flags (both scripts):
 
 | Flag | Meaning |
 |---|---|
-| `--config PATH` | Config YAML to use (default `config.yaml`). |
-| `--protocol {nfs3,iscsi,both}` | Restrict the run to one protocol (default `both`). |
-| `--run-id ID` | Reuse a specific run id (auto-generated otherwise) - use the **same id** across the sequential and concurrency scripts to merge both into one `summary_<run_id>.csv`/report. |
+| `--config PATH` | Config YAML to use (default `config.yaml`). Protocols, checkpoints, and concurrency levels are read from here. |
+| `--run-id ID` | Reuse a run id. Auto-generated otherwise as `RUN_<UTC timestamp>_<4 hex chars>`. Letters, digits, and underscores only — a hyphen is rewritten to `_` because storage-pool names (and the ONTAP volume behind them) disallow `-`. Use the **same id** across sequential and concurrency to merge one report. |
 | `--dry-run` | Simulate timings with no real API calls - use this first to sanity-check your config. |
 | `--skip-cleanup` | Skip the automatic post-run sweep for leftover pools from this run id. |
-| `--cleanup-only [FILTER]` | Don't run the benchmark - just delete every pool whose name contains `FILTER` (default: config's `pool_name_prefix`) and exit. Use this to recover after a crashed run. |
-| `--levels 2,5,10` | *(concurrency script only)* Override the concurrency levels to run instead of the config's `concurrency_levels`. |
+| `--cleanup-only [FILTER]` | Don't run the benchmark. Delete pools that match `FILTER` (default: config's `pool_name_prefix`) **and** this config's name pattern, zone, cluster, and storage provider. |
+
+`run.py` also requires `--test`. Protocols, checkpoints, and concurrency levels are not flags; change them in `config.yaml`.
+
+| Flag | Meaning |
+|---|---|
+| `--test NAME` | Required. `storage-pool-sequential`, `storage-pool-concurrency`, `vm-instance-sequential`, `vm-instance-concurrency`, or `vm-instance-combined`. |
 
 ## 4. VM-instance benchmark scripts
 
@@ -121,13 +127,15 @@ VM getting both a root disk and a data disk on the same tagged pool.
 
 | Script | What it does | Typical command |
 |---|---|---|
-| `benchmark_vm_instance_sequential.py` | Deploys VMs one at a time (per protocol) up to N=30, then destroys them one at a time, reporting checkpoint totals at N = 1, 2, 5, 10, 20, 30. | `python3 benchmark_vm_instance_sequential.py --config config.yaml` |
-| `benchmark_vm_instance_concurrency.py` | For each concurrency level C, deploys C VMs in parallel, records wall-clock time, then destroys the same C VMs in parallel. | `python3 benchmark_vm_instance_concurrency.py --config config.yaml` |
-| `benchmark_vm_instance_combined.py` | Runs the sequential matrix immediately followed by the concurrency matrix, under one shared run id, in a single invocation. Use once you already trust your config/environment. | `python3 benchmark_vm_instance_combined.py --config config.yaml` |
+| `benchmark_vm_instance_sequential.py` | Deploys VMs one at a time (per protocol) up to N=30, then destroys them one at a time, reporting checkpoint totals at N = 1, 2, 5, 10, 20, 30. | `python3 run.py --test vm-instance-sequential --config config.yaml` |
+| `benchmark_vm_instance_concurrency.py` | For each concurrency level C, deploys C VMs in parallel, records wall-clock time, then destroys the same C VMs in parallel. | `python3 run.py --test vm-instance-concurrency --config config.yaml` |
+| `benchmark_vm_instance_combined.py` | Runs the sequential matrix immediately followed by the concurrency matrix, under one shared run id, in a single invocation. Use once you already trust your config/environment. | `python3 run.py --test vm-instance-combined --config config.yaml` |
 
 Common flags: same as the storage-pool scripts above (`--config`,
-`--protocol`, `--run-id`, `--dry-run`, `--skip-cleanup`, `--cleanup-only
-[FILTER]`; `--levels` on the concurrency/combined scripts). Failed
+`--run-id`, `--dry-run`, `--skip-cleanup`, `--cleanup-only [FILTER]`).
+VM cleanup rewrites `_` in the filter to `-` so it matches the hostname
+form of the run id, and it only destroys VMs in the configured zone whose
+names match the benchmark prefix and pattern. Failed
 `deployVirtualMachine` calls are intentionally **left in place** (not
 auto-destroyed) so you can inspect what got left behind - clean them up
 with `--cleanup-only` once done.
@@ -136,14 +144,14 @@ with `--cleanup-only` once done.
 
 | Script | What it does | Typical command |
 |---|---|---|
-| `render_report.py` | Turns a run's `raw_ops_<run_id>.csv` + `summary_<run_id>.csv` into Confluence-ready markdown tables (matches the page's section layout, plus Min/Avg/P95/Max rows for the Results Log). Writes to stdout and to `results/report_<run_id>.md`. | `python3 render_report.py --run-id RUN-0001 --cloudstack-build 4.23.0.0-SNAPSHOT --ontap-version 9.17.1` |
+| `render_report.py` | Turns a run's `raw_ops_<run_id>.csv` + `summary_<run_id>.csv` into markdown tables (section layout plus Min/Avg/P95/Max rows for the Results Log). Writes to stdout and to `results/report_<run_id>.md`. | `python3 render_report.py --run-id RUN_20260927_081500_a1b2 --cloudstack-build 4.23.0.0-SNAPSHOT --ontap-version 9.17.1` |
 
 For VM-instance runs (which write `raw_ops_vm_<run_id>.csv` /
 `summary_vm_<run_id>.csv` instead of the storage-pool scripts' `raw_ops_`/
 `summary_` prefix), pass the matching prefixes:
 
 ```bash
-python3 render_report.py --run-id RUN-0001 \
+python3 render_report.py --run-id RUN_20260927_081500_a1b2 \
     --raw-prefix raw_ops_vm --summary-prefix summary_vm --report-suffix _vm
 ```