feat(orchestrator): add database integration support#43
Conversation
WalkthroughThis PR adds configurable database backends with engine-specific initialization and TLS, cancellation-aware CLI and runner execution, broader cleanup, and cgroup-aware container memory accounting. ChangesDatabase backend support
Cancellation-aware execution flow
Container memory metrics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TestCase
participant NewComposeRunner
participant database
participant database-init
participant subject
TestCase->>NewComposeRunner: configure database case
NewComposeRunner->>database: render engine and TLS settings
database->>database-init: report healthy
database-init->>database: execute seed SQL
subject->>database-init: wait for successful completion
sequenceDiagram
participant CLI
participant Runner
participant ComposeRunner
participant PollingLoop
CLI->>Runner: propagate signal cancellation
Runner->>ComposeRunner: cancel run context
ComposeRunner->>PollingLoop: stop waits and polling
Runner->>Runner: skip persistence for canceled run
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/config/case.go (1)
2249-2280: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating SA password strength for mssql at config-load time.
validateDatabase()checks engine,seed_sql, and database-name charset, but never validatesPasswordwhen a case overrides the default. MSSQL enforces SA password complexity (8+ chars, upper/lower/digit/symbol per theDefaultPasswordcomment indatabase_engines.go); a case author overriding with a weak password will fail silently at container startup, surfacing only as an opaque healthcheck timeout after ~4 minutes of retries — hard to diagnose without digging intodocker logs.Since only
mssqlexists today, a simple length/charset check here (or as aValidatePasswordhook onDatabaseEngine) would fail fast with a clear error instead.♻️ Sketch of a minimal check
if tc.Database.SeedSQL == "" { return fmt.Errorf("case %q: database block requires `seed_sql`", tc.Name) } + if tc.Database.Password != "" && len(tc.Database.Password) < 8 { + return fmt.Errorf("case %q: database.password must be at least 8 characters (mssql SA complexity policy)", tc.Name) + } if !databaseNameRe.MatchString(tc.Database.DatabaseOrDefault()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/case.go` around lines 2249 - 2280, Add fast-fail validation for overridden database passwords in validateDatabase so weak MSSQL SA passwords are rejected at config load instead of surfacing later at container startup. Update the TestCase.validateDatabase path to inspect tc.Database.Password when present, and use a database-engine-specific check for mssql (ideally via a ValidatePassword hook on DatabaseEngines or a small helper near databaseNameRe) that enforces the existing password policy. Make sure the error mentions the case name and the database engine so authors can locate the bad override quickly.internal/orchestrator/docker.go (1)
2304-2327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a database compose-rendering test
writeComposealready has render coverage for vault, Kafka, AWS, Azure, agent, and verifier paths, but the newdatabase:block still has no compose assertion. Add adatabase:-enabled case that checks thedatabase/database-initservices, the subjectdepends_onedge, and the seed mount path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/orchestrator/docker.go` around lines 2304 - 2327, The new database rendering branch in writeCompose lacks test coverage. Add a database-enabled compose-rendering test that exercises tc.UsesDatabase() and verifies the rendered database and database-init services, the subject service’s depends_on relationship to seeding completion, and the seed volume/mount path derived from cfg.DatabaseSeedHost. Use the existing compose assertion patterns in writeCompose tests to locate and extend the coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/config/case.go`:
- Around line 2249-2280: Add fast-fail validation for overridden database
passwords in validateDatabase so weak MSSQL SA passwords are rejected at config
load instead of surfacing later at container startup. Update the
TestCase.validateDatabase path to inspect tc.Database.Password when present, and
use a database-engine-specific check for mssql (ideally via a ValidatePassword
hook on DatabaseEngines or a small helper near databaseNameRe) that enforces the
existing password policy. Make sure the error mentions the case name and the
database engine so authors can locate the bad override quickly.
In `@internal/orchestrator/docker.go`:
- Around line 2304-2327: The new database rendering branch in writeCompose lacks
test coverage. Add a database-enabled compose-rendering test that exercises
tc.UsesDatabase() and verifies the rendered database and database-init services,
the subject service’s depends_on relationship to seeding completion, and the
seed volume/mount path derived from cfg.DatabaseSeedHost. Use the existing
compose assertion patterns in writeCompose tests to locate and extend the
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ef19b4f-019e-41ea-950e-d55a3ed4e986
📒 Files selected for processing (5)
internal/config/case.gointernal/config/database_config_test.gointernal/config/database_engines.gointernal/orchestrator/database.gointernal/orchestrator/docker.go
Deploying pipebench with
|
| Latest commit: |
9ca33fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ef1442f3.pipebench.pages.dev |
| Branch Preview URL: | https://dt-866.pipebench.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/orchestrator/tls.go (1)
215-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
writeRSAKeyswallows theCloseerror.Unlike
writePEMCert(Lines 447-458), which propagates a deferredCloseerror, this helper usesdefer f.Close()and discards it — a flush failure on the private-key write would go unnoticed. Optional, for consistency.♻️ Align Close-error handling with writePEMCert
-func writeRSAKey(path string, key *rsa.PrivateKey, perm os.FileMode) error { +func writeRSAKey(path string, key *rsa.PrivateKey, perm os.FileMode) (err error) { f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, perm) if err != nil { return err } - defer f.Close() + defer func() { + if closeErr := f.Close(); err == nil && closeErr != nil { + err = closeErr + } + }() return pem.Encode(f, &pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/orchestrator/tls.go` around lines 215 - 222, The writeRSAKey helper is dropping any error returned by closing the file handle, unlike writePEMCert which propagates deferred Close failures. Update writeRSAKey to use the same Close-error handling pattern as writePEMCert so that any flush or close failure after pem.Encode is returned, and keep the change localized to writeRSAKey in tls.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/orchestrator/tls.go`:
- Line 206: The server.key file is being written with permissions that are too
permissive in the TLS setup path. Update the key-writing call in the tls
generation flow around writeRSAKey in internal/orchestrator/tls.go so the
private key is created with non-world-readable permissions such as 0400 or 0600,
while keeping ownership/access compatible with the mssql process.
---
Nitpick comments:
In `@internal/orchestrator/tls.go`:
- Around line 215-222: The writeRSAKey helper is dropping any error returned by
closing the file handle, unlike writePEMCert which propagates deferred Close
failures. Update writeRSAKey to use the same Close-error handling pattern as
writePEMCert so that any flush or close failure after pem.Encode is returned,
and keep the change localized to writeRSAKey in tls.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2917d0c-84cf-481f-8080-d652b79b984b
📒 Files selected for processing (5)
internal/config/case.gointernal/config/database_engines.gointernal/orchestrator/database.gointernal/orchestrator/docker.gointernal/orchestrator/tls.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/case.go
* feat(orchestrator): implement postgres database engine support * fix(config): force TCP in pg_isready to avoid startup race
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/database_engines.go`:
- Around line 209-246: Update validateDatabase() so the oracle engine explicitly
rejects database.tls being enabled, since the oracle config in
database_engines.go has no TLS support wiring (no TLSServerCertPath,
TLSServerKeyPath, or BuildTLSConf). Add the check alongside the existing oracle
validation path and return a validation error when database.tls is true, using
the engine name “oracle” and validateDatabase() as the key symbols to locate the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3df20c6a-e02d-4e31-96b4-1232ce38ba06
📒 Files selected for processing (2)
internal/config/database_config_test.gointernal/config/database_engines.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/harness/main.go (1)
601-631: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSatisfy the
noctxlinter on the newexec.Commandcalls.golangci-lint (noctx) flags Lines 605 and 608 (
os/exec.Command must not be called. use os/exec.CommandContext), which will fail the pipeline. The new network-cleanup calls at Lines 627/629 share the same pattern.cleanCmd'sRunEhascmdin scope, socmd.Context()is available and ties cleanup to the root cancellable context introduced by this PR.🔧 Proposed fix
- if out, err := exec.Command("docker", "ps", "-aq", "--filter", "name=^bench-").Output(); err == nil { + if out, err := exec.CommandContext(cmd.Context(), "docker", "ps", "-aq", "--filter", "name=^bench-").Output(); err == nil { if ids := strings.Fields(string(out)); len(ids) > 0 { rmArgs := append([]string{"rm", "-f"}, ids...) - _ = exec.Command("docker", rmArgs...).Run() + _ = exec.CommandContext(cmd.Context(), "docker", rmArgs...).Run() } }Apply the same
CommandContextchange to the networkls/rmcalls at Lines 627 and 629.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/harness/main.go` around lines 601 - 631, The new cleanup path in main’s bench teardown uses exec.Command directly, which triggers the noctx linter and ignores the root cancellation context. Update the docker ps/network ls and rm calls in the cleanup block to use the existing command context available from cleanCmd.RunE via cmd.Context(), matching the rest of the cleanup logic in cmd/harness/main.go. Keep the same behavior, just thread the context through all newly added exec invocations so the network cleanup is cancellable and lint-clean.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/orchestrator/docker.go`:
- Around line 1484-1488: `WaitForVerifierExit` is wrapping interrupt
cancellation as a verifier error instead of passing it through. In the verifier
wait path in `docker.go`, special-case `errors.Is(err, context.Canceled)` just
like the generator handling in `runner.go`, and return the cancellation directly
without adding the “verifier logs” wrapper; keep the existing wrapped error path
for all other failures.
---
Outside diff comments:
In `@cmd/harness/main.go`:
- Around line 601-631: The new cleanup path in main’s bench teardown uses
exec.Command directly, which triggers the noctx linter and ignores the root
cancellation context. Update the docker ps/network ls and rm calls in the
cleanup block to use the existing command context available from cleanCmd.RunE
via cmd.Context(), matching the rest of the cleanup logic in
cmd/harness/main.go. Keep the same behavior, just thread the context through all
newly added exec invocations so the network cleanup is cancellable and
lint-clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e104f44e-a616-4bf3-aa71-0d237917dc3d
📒 Files selected for processing (4)
cmd/harness/main.gointernal/orchestrator/docker.gointernal/runner/runner.gointernal/runner/sleep_test.go
| select { | ||
| case <-r.runCtx.Done(): | ||
| return fmt.Errorf("interrupted while waiting for verifier: %w", r.runCtx.Err()) | ||
| case <-time.After(2 * time.Second): | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the verifier wait caller for context.Canceled handling
rg -nP -C6 'WaitForVerifierExit' internal/runner/runner.goRepository: VirtualMetric/PipeBench
Length of output: 657
🏁 Script executed:
#!/bin/bash
sed -n '1468,1496p' internal/orchestrator/docker.go
printf '\n---\n'
sed -n '7040,7065p' internal/runner/runner.go
printf '\n---\n'
rg -n 'context\.Canceled|errors\.Is\(err, context\.Canceled\)|WaitForGeneratorExit|WaitForVerifierExit' internal/runner/runner.go internal/orchestrator/docker.goRepository: VirtualMetric/PipeBench
Length of output: 4044
🏁 Script executed:
#!/bin/bash
sed -n '1,170p' internal/runner/runner.go
printf '\n---\n'
sed -n '392,445p' internal/runner/runner.go
printf '\n---\n'
sed -n '1738,1768p' internal/runner/runner.go
printf '\n---\n'
sed -n '6538,6568p' internal/runner/runner.go
printf '\n---\n'
sed -n '6836,6865p' internal/runner/runner.goRepository: VirtualMetric/PipeBench
Length of output: 11484
🏁 Script executed:
#!/bin/bash
rg -n -C4 'errors\.Is\(.*context\.Canceled|context\.Canceled' internal/runner internalRepository: VirtualMetric/PipeBench
Length of output: 9172
Handle verifier cancellation like the generator path internal/runner/runner.go:7057-7058
WaitForVerifierExit can return a wrapped context.Canceled, but this call always wraps the error with verifier logs, so a user interrupt shows up as a verifier failure with extra noise. Special-case errors.Is(err, context.Canceled) here and return it directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/orchestrator/docker.go` around lines 1484 - 1488,
`WaitForVerifierExit` is wrapping interrupt cancellation as a verifier error
instead of passing it through. In the verifier wait path in `docker.go`,
special-case `errors.Is(err, context.Canceled)` just like the generator handling
in `runner.go`, and return the cancellation directly without adding the
“verifier logs” wrapper; keep the existing wrapped error path for all other
failures.
1 participant
Summary by CodeRabbit
databasebackend with engine defaults,seed_sqlinitialization, and one-shotdatabase-initcompose support (mssql/mysql/postgres/oracle).cleanto remove allbench-*containers and related bench networks.