testops: add shared runner platform - #48
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the TestOps framework, a YAML-driven scenario runner for storage-product validation, including various product-agnostic actions, product-specific packs, a web dashboard, and a queue controller. The review feedback highlights several critical correctness and robustness issues, including a potential command injection vulnerability in the cleanup actions, compatibility and silent failure issues in the PostgreSQL benchmark initialization script, code duplication of shell quoting functions, potential nil pointer panics in volume field extraction, and hanging commands in git/hostname helpers due to missing timeouts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if p == "" { | ||
| continue | ||
| } | ||
| node.RunRoot(ctx, fmt.Sprintf("pkill -9 %s 2>/dev/null || true", p)) |
There was a problem hiding this comment.
Potential Command Injection / Missing Shell Quoting
The p parameter (derived from kill_patterns) is directly interpolated into the shell command without any quoting or sanitization. If a pattern contains spaces or shell metacharacters, it could lead to command injection or command failure.
Use the shellQuoteCleanup helper function (defined on line 157 of this file) to safely quote the argument before executing it.
| node.RunRoot(ctx, fmt.Sprintf("pkill -9 %s 2>/dev/null || true", p)) | |
| node.RunRoot(ctx, fmt.Sprintf("pkill -9 %s 2>/dev/null || true", shellQuoteCleanup(p))) |
| pgBin := paramDefault(act.Params, "pg_bin", "/usr/lib/postgresql/16/bin") | ||
|
|
||
| node, err := GetNode(actx, act.Node) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| pgdata := mount + "/pgdata" | ||
|
|
||
| // Format, mount, init PostgreSQL, start, create bench DB, run pgbench -i. | ||
| script := fmt.Sprintf(`set -e | ||
| # Stop any previous instance | ||
| sudo -u postgres %s/pg_ctl -D %s stop 2>/dev/null || true | ||
| sleep 1 | ||
| # Format and mount | ||
| mkfs.%s -F %s > /dev/null 2>&1 | ||
| mkdir -p %s | ||
| mount %s %s | ||
| # Init PostgreSQL | ||
| mkdir -p %s | ||
| chown postgres:postgres %s | ||
| sudo -u postgres %s/initdb -D %s > /dev/null 2>&1 | ||
| echo "listen_addresses = '127.0.0.1'" >> %s/postgresql.conf | ||
| echo "port = %s" >> %s/postgresql.conf | ||
| echo "unix_socket_directories = '/tmp'" >> %s/postgresql.conf | ||
| echo "shared_buffers = 256MB" >> %s/postgresql.conf | ||
| echo "effective_cache_size = 512MB" >> %s/postgresql.conf | ||
| echo "work_mem = 4MB" >> %s/postgresql.conf | ||
| echo "wal_buffers = 16MB" >> %s/postgresql.conf | ||
| echo "max_connections = 200" >> %s/postgresql.conf | ||
| chown -R postgres:postgres %s | ||
| # Start | ||
| sudo -u postgres %s/pg_ctl -D %s -l %s/logfile start | ||
| sleep 3 | ||
| # Create DB and init pgbench | ||
| sudo -u postgres %s/createdb -h /tmp -p %s benchdb 2>/dev/null || true | ||
| sudo -u postgres pgbench -h /tmp -i -s %s -p %s benchdb 2>&1 | tail -3 | ||
| echo PGBENCH_INIT_OK`, | ||
| pgBin, pgdata, | ||
| fstype, device, | ||
| mount, | ||
| device, mount, | ||
| pgdata, | ||
| pgdata, | ||
| pgBin, pgdata, | ||
| pgdata, port, pgdata, pgdata, | ||
| pgdata, pgdata, pgdata, pgdata, pgdata, | ||
| pgdata, | ||
| pgBin, pgdata, pgdata, | ||
| pgBin, port, | ||
| scale, port, | ||
| ) |
There was a problem hiding this comment.
Robustness and Correctness Issues in pgbenchInit Script
There are several critical issues in the current shell script template for pgbenchInit:
mkfs.xfsCompatibility:mkfs.xfsdoes not support the-Fflag (it uses-fto force overwrite). Iffstypeisxfs, the command will fail.- Silent Failures: Because
set -o pipefailis not enabled, the exit status of thepgbench ... | tail -3pipeline is determined solely bytail -3(which always succeeds). Any initialization failure inpgbenchwill be silently ignored, and the script will incorrectly report success. - Robustness of Re-initialization: If the device is already mounted or busy from a previous run,
mkfswill fail. It is safer to explicitly unmount the device and mountpoint first usingumount -l(lazy unmount) to ensure a clean state.
pgBin := paramDefault(act.Params, "pg_bin", "/usr/lib/postgresql/16/bin")
node, err := GetNode(actx, act.Node)
if err != nil {
return nil, err
}
pgdata := mount + "/pgdata"
forceFlag := "-F"
if fstype == "xfs" {
forceFlag = "-f"
}
// Format, mount, init PostgreSQL, start, create bench DB, run pgbench -i.
script := fmt.Sprintf(`set -eo pipefail
# Stop any previous instance
sudo -u postgres %s/pg_ctl -D %s stop 2>/dev/null || true
sleep 1
# Ensure unmounted and clean
sudo umount -l %s 2>/dev/null || true
sudo umount -l %s 2>/dev/null || true
# Format and mount
mkfs.%s %s %s > /dev/null 2>&1
mkdir -p %s
mount %s %s
# Init PostgreSQL
mkdir -p %s
chown postgres:postgres %s
sudo -u postgres %s/initdb -D %s > /dev/null 2>&1
echo "listen_addresses = '127.0.0.1'" >> %s/postgresql.conf
echo "port = %s" >> %s/postgresql.conf
echo "unix_socket_directories = '/tmp'" >> %s/postgresql.conf
echo "shared_buffers = 256MB" >> %s/postgresql.conf
echo "effective_cache_size = 512MB" >> %s/postgresql.conf
echo "work_mem = 4MB" >> %s/postgresql.conf
echo "wal_buffers = 16MB" >> %s/postgresql.conf
echo "max_connections = 200" >> %s/postgresql.conf
chown -R postgres:postgres %s
# Start
sudo -u postgres %s/pg_ctl -D %s -l %s/logfile start
sleep 3
# Create DB and init pgbench
sudo -u postgres %s/createdb -h /tmp -p %s benchdb 2>/dev/null || true
sudo -u postgres pgbench -h /tmp -i -s %s -p %s benchdb 2>&1 | tail -3
echo PGBENCH_INIT_OK`,
pgBin, pgdata,
device, mount,
fstype, forceFlag, device,
mount,
device, mount,
pgdata,
pgdata,
pgBin, pgdata,
pgdata, port, pgdata, pgdata,
pgdata, pgdata, pgdata, pgdata, pgdata,
pgdata,
pgBin, pgdata, pgdata,
pgBin, port,
scale, port,
)| // execShellQuote single-quote escapes a value for POSIX shell. Empty | ||
| // becomes an empty quoted string; otherwise wraps in single quotes and escapes any | ||
| // embedded single-quote by closing-escaping-reopening. | ||
| func execShellQuote(s string) string { |
There was a problem hiding this comment.
Code Duplication of Shell Quoting Functions
There are multiple identical or nearly identical implementations of shell quoting functions across different files in the actions package:
testops/actions/system.go->execShellQuotetestops/actions/build.go->shellQuotetestops/actions/cleanup.go->shellQuoteCleanuptestops/actions/iscsi.go->shellQuoteIscsi
Since all these files belong to the same actions package, they can share a single package-level function without any imports. Consolidating these into a single exported/package-level helper function (e.g., ShellQuote) improves maintainability and prevents copy-paste bugs.
| func execShellQuote(s string) string { | |
| func ShellQuote(s string) string { |
| func extractVolumeField(info *blockapi.VolumeInfo, field string) (string, error) { | ||
| switch field { |
There was a problem hiding this comment.
Potential Nil Pointer Panic
extractVolumeField accesses fields of info (a pointer to blockapi.VolumeInfo) without checking if info is nil. If info is nil (e.g., due to an unhandled error or empty response from the API client), this function will panic.
Add a nil check at the beginning of extractVolumeField (and similarly in setISCSIVars on line 43) to prevent potential nil pointer panics.
func extractVolumeField(info *blockapi.VolumeInfo, field string) (string, error) {
if info == nil {
return "", fmt.Errorf("volume info is nil")
}
switch field {| func gitSHAShort() string { | ||
| out, err := exec.Command("git", "rev-parse", "--short", "HEAD").Output() | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return strings.TrimSpace(string(out)) | ||
| } |
There was a problem hiding this comment.
Hanging Commands due to Missing Timeout/Context
gitSHAShort uses exec.Command without a context or timeout. If the git command hangs (e.g., due to a repository lock or file system issues), the entire test runner will hang indefinitely.
Use exec.CommandContext with a reasonable timeout (e.g., 5 seconds) to ensure the command terminates and does not block the execution pipeline indefinitely. Apply the same pattern to gitBranch (line 319) and hostname (line 327).
func gitSHAShort() string {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
out, err := exec.CommandContext(ctx, "git", "rev-parse", "--short", "HEAD").Output()
if err != nil {
return ""
}
return strings.TrimSpace(string(out))
}
The unified gate build step never built seaweedfs-sw-rdma-kvcache, so the kvcache loader smoke failed with UNIFIED_NO_KVCACHE. The lab filer (weed-uds) also predates sw-rdma-kd's VFS conditional mutations, so the VFS cross-access and read-matrix blocks hard-fail; skip them via SKIP_VFS=1 to gate the RDMA object/loader hot path independently. Both were already live on the deployed M01 copy; this lands them in source so a redeploy keeps them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What changed
Adds the shared TestOps runner platform under
testops/inseaweedfs/artifactory.This brings in:
sweedrdma,sweeds3,swblock, etc.);scenarios/nixl-provider-cpu-gate.yaml);It also adds
docs/nixl-kvcache-testbed.md, which defines the next testbed path for NIXL object compatibility, RDMA hardening, and a CPU in-memory KVCache MVP.Why
The product code should stay in
seaweed-mono, but the shared test controller, suite registry, scenario metadata, QA profiles, and evidence tooling need a stable public home.artifactory/testopsbecomes that home.This lets RDMA/NIXL/KVCache work use the same TestOps bundle contract and dashboard flow instead of relying on private/local runner copies.
NIXL scope
The NIXL addition is intentionally thin. TestOps does not implement NIXL or RDMA data movement. It runs the mono-side NIXL provider gate and parses stable witness lines such as:
NIXL_PROVIDER_GET_GATE_PASSNIXL_PROVIDER_READ_BENCH_RESULTNIXL_PROVIDER_PUT_GATE_PASSNIXL_PROVIDER_CPU_GATE_PASSThe current NIXL gate is CPU provider only. It does not claim GPU/cuObject/VRAM support.
Validation
Run locally from
testops/:go test ./...go vet ./...go run ./cmd/sweedrdma validate scenarios/rdma-unified-lab-gate.yamlgo run ./cmd/sweedrdma validate scenarios/nixl-provider-cpu-gate.yamlpython -m mkdocs build --strictgit diff --checkGitHub Actions on this PR:
Go test + docspassed.No result bundles, generated docs site, or binaries are included.