feat(byoo-perf): deploy the authentic collector and wire run/cleanup - #623
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe performance suite now deploys authentic BYOO OpenTelemetry Collector workloads to Kubernetes, waits for readiness, reports endpoints, and cleans up suite-owned resources. The CLI supports cluster configuration, shape-specific namespaces, retention, and cleanup commands. ChangesBYOO performance deployment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant PerfCLI
participant Renderer
participant DeployClient
participant KubernetesCluster
Operator->>PerfCLI: Run selected shape
PerfCLI->>Renderer: Render and validate workload
PerfCLI->>DeployClient: Deploy workload
DeployClient->>KubernetesCluster: Create or replace resources
PerfCLI->>DeployClient: Wait for readiness
DeployClient-->>PerfCLI: Report in-cluster endpoints
PerfCLI->>DeployClient: Cleanup unless retention is enabled
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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: 2
🧹 Nitpick comments (1)
src/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.go (1)
243-271: 🧹 Nitpick | 🔵 TrivialCleanup never removes the namespace itself.
Cleanupdeletes suite-labeled pods and Services but leaves the namespace in place. Over repeated runs, namespaces created byEnsureNamespaceaccumulate. Confirm this is intentional (e.g., to avoid slow namespace termination) or consider deleting the namespace when it was created by this suite and is now empty.🤖 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 `@src/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.go` around lines 243 - 271, Update Client.Cleanup to remove the suite-created namespace after deleting its labeled pods and services, using the existing ownership/creation tracking from EnsureNamespace. Delete the namespace only when this suite created it and cleanup completed successfully, while preserving the current behavior for pre-existing namespaces and propagating non-NotFound deletion errors.
🤖 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 `@src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.go`:
- Around line 208-220: Update the deployment flow around client.Deploy and
client.WaitPodReady to clean up resources whenever either operation fails and
cfg.retain is false. On Deploy errors, invoke client.Cleanup before returning;
on readiness errors, preserve the original failure while also reporting any
cleanup failure instead of discarding it, using an appropriate combined error.
In `@src/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.go`:
- Around line 173-190: Update applyPod to wrap errors returned by waitPodDeleted
with context identifying the pod deletion wait, and update the waitPodDeleted
poll function to wrap the underlying Kubernetes error before returning it. Use
Go %w wrapping at both points while preserving the existing error propagation
behavior.
---
Nitpick comments:
In `@src/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.go`:
- Around line 243-271: Update Client.Cleanup to remove the suite-created
namespace after deleting its labeled pods and services, using the existing
ownership/creation tracking from EnsureNamespace. Delete the namespace only when
this suite created it and cleanup completed successfully, while preserving the
current behavior for pre-existing namespaces and propagating non-NotFound
deletion errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 221f0fa3-ce78-4b69-95ad-e3eafec641c7
⛔ Files ignored due to path filters (1)
src/compute-plane-services/byoo-otel-collector/perf/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
src/compute-plane-services/byoo-otel-collector/VERSIONsrc/compute-plane-services/byoo-otel-collector/perf/README.mdsrc/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.gosrc/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main_test.gosrc/compute-plane-services/byoo-otel-collector/perf/go.modsrc/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy_test.go
4 tasks
sbaum1994
reviewed
Aug 5, 2026
sbaum1994
approved these changes
Aug 5, 2026
shobham-nv
force-pushed
the
shobham/418-byoo-perf-deploy
branch
from
August 10, 2026 13:01
abb1df5 to
af99feb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@src/compute-plane-services/byoo-otel-collector/perf/README.md`:
- Around line 33-34: Update the README layout entry around pkg/deploy and the
referenced text near the additional affected section: replace em dash characters
with permitted punctuation, remove bold Markdown emphasis around does not, and
ensure the changed committed text contains only allowed ASCII characters without
emojis or other prohibited emphasis.
- Line 90: Update the documentation wording in the README sentence near the
kubeconfig cleanup description, replacing “afterwards” with the American English
spelling “afterward.”
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 26a81f3f-0278-4546-b430-2c4acb17ec6b
⛔ Files ignored due to path filters (1)
src/compute-plane-services/byoo-otel-collector/perf/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
src/compute-plane-services/byoo-otel-collector/VERSIONsrc/compute-plane-services/byoo-otel-collector/perf/README.mdsrc/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.gosrc/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main_test.gosrc/compute-plane-services/byoo-otel-collector/perf/go.modsrc/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- src/compute-plane-services/byoo-otel-collector/perf/go.mod
- src/compute-plane-services/byoo-otel-collector/VERSION
- src/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy_test.go
- src/compute-plane-services/byoo-otel-collector/perf/pkg/deploy/deploy.go
- src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.go
- src/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main_test.go
shobham-nv
force-pushed
the
shobham/418-byoo-perf-deploy
branch
from
August 10, 2026 13:12
af99feb to
435be42
Compare
Add the deployment milestone for the BYOO collector perf suite: render the production workload shape, deploy the authentic collector to a cluster, wait for it to become ready, and tear it back down. Load generation and measurement land in a later milestone. - pkg/deploy: Kubernetes client that applies the rendered bench pod plus a harness ClusterIP OTLP Service, waits for pod readiness (failing fast on crash/image-pull loops), and cleans up scoped by the suite's part-of label so it never touches unrelated resources. Repeatable deploys replace existing objects. Unit-tested against a fake clientset. - cmd/perf: implement the cobra run and cleanup commands (previously stubs) against pkg/deploy, with per-shape namespaces so container and helm shapes never collide, and --retain to keep resources for inspection. - cmd/perf tests: run/cleanup flag defaults, hermetic invalid-selector checks (validation happens before any cluster access), and namespaceForShape. - go.mod: add k8s.io/client-go (pinned to v0.34.2 via replace, mirroring the shared lib's convention). - README: document run and cleanup. - VERSION: bump to 0.157.12 for the collector-dir change. Relates to #416. Closes #418. Signed-off-by: shobham <shobham@nvidia.com>
shobham-nv
force-pushed
the
shobham/418-byoo-perf-deploy
branch
from
August 10, 2026 14:59
435be42 to
a5a2164
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Adds the deployment milestone to the BYOO collector perf suite.
runnow renders the production workload shape viaicms-translate, validates it, deploys the authentic collector to a cluster (k3d or remote) behind a harness OTLP Service, waits for readiness, and tears it down;cleanupremoves what the suite created. Load generation and measurement land in a later milestone.Additional Details
pkg/deploy(new): a Kubernetes client that applies the rendered bench pod plus a harness ClusterIP OTLP Service, waits for pod readiness (failing fast onCrashLoopBackOff/ImagePullBackOff/config errors instead of blocking to timeout), and cleans up scoped by theapp.kubernetes.io/part-of=byoo-perflabel so it never deletes unrelated resources. Repeatable deploys replace existing objects. The harness Service gives load generators a stable in-cluster address regardless of shape; for the container shape (a localhost sidecar in production) it is harness-only and never alters the collector spec under test.cmd/perf: the previously-stubbed cobrarunandcleanupcommands are implemented againstpkg/deploy, with per-shape namespaces (byoo-perf-container/byoo-perf-helm) so both shapes can be deployed without colliding, and--retainto keep resources for inspection.go.mod: addsk8s.io/client-go, pinned tov0.34.2viareplace, mirroring the shared lib's convention.GOWORK=offmodule wired into the requiredperfCI job (from feat(byoo-perf): add scaffolding, translation-driven rendering, and validation #422), so these tests run on every change to this subtree.For the Reviewer
pkg/deploy/deploy.go— the deploy/wait/cleanup logic and the label-scoped teardown.cmd/perf/main.go—runRun/runCleanupand per-shape namespacing.runvalidates all selectors before it ever contacts a cluster, which keeps the new CLI tests hermetic.For QA
make perf-test(build + vet + test,GOWORK=off) passes locally;pkg/deployis unit-tested against a fake clientset (namespace idempotency, pod+service creation, repeatable deploys, ready/crash-loop paths, label-scoped cleanup).perf runagainst k3d/remote) exercise real deployment; unit tests do not require a cluster.Issues
Relates to #416
Closes #418
Checklist
Summary by CodeRabbit