feat(orchestrator): add AWS S3 seed objects to LocalStack init#47
feat(orchestrator): add AWS S3 seed objects to LocalStack init#47erenaslandev wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds validated AWS S3 seed-object configuration and LocalStack uploads, multi-container metrics collection, receive-window tracking, correctness-flow updates, resource-metric persistence, and zero-snapshot aggregation filtering. ChangesAWS seed objects
Container metrics collection
Correctness runner flows
Metrics aggregation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant runInflightCrashCorrectness
participant runMidDeliveryAction
participant Subject
participant MetricsCollector
Runner->>runInflightCrashCorrectness: dispatch persistence in-flight crash
runInflightCrashCorrectness->>runMidDeliveryAction: configure mid-delivery flow
runMidDeliveryAction->>Subject: stop and restart during delivery
runMidDeliveryAction->>MetricsCollector: harvest resource metrics
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
1596-1634: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "Kafka" comment left over from generalization.
The renamed/generalized driver's doc block (Lines 1596-1634) is accurate, but the inline comment at Line 1762-1763 ("The generator produces to Kafka independently of the subject") is now stale — this code path is shared by the kafka cert-rotation, syslog Vault cert-rotation, and object-storage in-flight-crash flows too, none of which involve Kafka. Worth updating to avoid misleading future readers debugging a non-Kafka flow.
✏️ Suggested comment fix
- // The generator produces to Kafka independently of the subject; collect - // its final count. + // The generator produces to the durable source independently of the + // subject; collect its final count.Also applies to: 1762-1763
🤖 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/runner/runner.go` around lines 1596 - 1634, Update the stale inline comment within runMidDeliveryAction to describe generator production independently of the subject/source generically, rather than referring specifically to Kafka. Keep the behavior and surrounding logic unchanged so the comment accurately covers Kafka, syslog, and object-storage flows.
🤖 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/runner/runner.go`:
- Around line 1919-1947: Update the midDeliveryFlow literal in
runInflightCrashCorrectness to provide an extraCleanup hook that force-removes
the fixed-name LocalStack, Azurite, and Azure-init containers, matching the
cleanup performed by Run(). Preserve the existing crash action and ensure
cleanup runs before services are started to prevent collisions from prior
interrupted runs.
---
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 1596-1634: Update the stale inline comment within
runMidDeliveryAction to describe generator production independently of the
subject/source generically, rather than referring specifically to Kafka. Keep
the behavior and surrounding logic unchanged so the comment accurately covers
Kafka, syslog, and object-storage flows.
🪄 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: ccc80bb9-f473-462f-8221-95c7f050425c
📒 Files selected for processing (5)
internal/config/cloud.gointernal/config/cloud_test.gointernal/orchestrator/awsinit.gointernal/orchestrator/compose_render_test.gointernal/runner/runner.go
correct formatting
Deploying pipebench with
|
| Latest commit: |
fb36f09
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a7335ae7.pipebench.pages.dev |
| Branch Preview URL: | https://dt-838.pipebench.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/results/compare.go`:
- Around line 288-292: Update the CPU and memory parsing in the record
comparison flow to retain both ParseFloat errors and require successful parsing
of both fields before applying the stopped-container check. Only continue when
both parses succeed and cpuV and memV are zero; preserve rows containing
malformed or blank CPU/memory values.
In `@internal/runner/runner.go`:
- Around line 1657-1672: Update harvestResourceMetrics to call
orch.StopCollector before orch.CopyMetricsCSV, preserving the existing warnings
and CSV aggregation flow. Copy and aggregate the CSV only after the collector
has stopped and flushed its final samples.
🪄 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: 70f83834-77dd-42cb-80b9-a34b6e689be7
📒 Files selected for processing (3)
internal/results/compare.gointernal/results/compare_test.gointernal/runner/runner.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/orchestrator/docker.go (1)
2040-2052: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a render test for the cluster
CollectorTargetsjoin.The comma-joined multi-node case (Lines 2044-2051) has no test asserting the rendered
COLLECTOR_TARGET_CONTAINERvalue for a cluster case. Given this directly determines which containers the collector samples, a small render-test case would guard against a future regression (e.g. wrong separator, wrong node naming).🤖 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 2040 - 2052, Add a focused render test covering the cluster path that builds collectorTargets from multiple clusterNodes. Assert the rendered COLLECTOR_TARGET_CONTAINER value contains the expected node container names joined by commas, preserving the existing single-container behavior for non-cluster cases.
🤖 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 `@containers/collector/main.go`:
- Around line 129-152: Update the aggregation around addRow and row.CpuIdl so
multi-target CPU usage cannot produce a negative idle percentage in the
persisted metrics. Prefer averaging the per-target CPU usage before calculating
idle, or otherwise clamp the calculated idle value to a minimum of zero, while
preserving single-target behavior and the dstat-compatible CpuIdl field.
---
Nitpick comments:
In `@internal/orchestrator/docker.go`:
- Around line 2040-2052: Add a focused render test covering the cluster path
that builds collectorTargets from multiple clusterNodes. Assert the rendered
COLLECTOR_TARGET_CONTAINER value contains the expected node container names
joined by commas, preserving the existing single-container behavior for
non-cluster cases.
🪄 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: 0fa16758-801d-4168-81a9-687e618d5295
📒 Files selected for processing (6)
containers/collector/main.gocontainers/collector/main_test.gocontainers/receiver/main.gocontainers/receiver/main_test.gointernal/orchestrator/docker.gointernal/runner/runner.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runner/runner.go
1 participant
Summary by CodeRabbit