feat(byoo-perf): deploy the authentic collector and wire run/cleanup - #623
feat(byoo-perf): deploy the authentic collector and wire run/cleanup#623shobham-nv wants to merge 2 commits into
Conversation
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 0.157.6 -> 0.157.7 for the collector-dir change. Relates to #416. Closes #418. Signed-off-by: shobham <shobham@nvidia.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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. Documentation and the collector version were updated. 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-->>Operator: 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 💡 1⚔️ Resolve merge conflicts 💡
🧪 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: 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
| fmt.Fprintf(stdout, "[%s] deploying to namespace %q ...\n", shape, ns) | ||
| dep, err := client.Deploy(ctx, ns, res) | ||
| if err != nil { | ||
| return fmt.Errorf("deploy %s: %w", shape, err) | ||
| } | ||
|
|
||
| fmt.Fprintf(stdout, "[%s] waiting up to %s for collector pod %q to become ready ...\n", shape, cfg.readyTimeout, dep.PodName) | ||
| if err := client.WaitPodReady(ctx, ns, dep.PodName, cfg.readyTimeout); err != nil { | ||
| if !cfg.retain { | ||
| _ = client.Cleanup(ctx, ns) | ||
| } | ||
| return fmt.Errorf("collector did not become ready for %s shape: %w", shape, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Deploy failures leave resources behind despite the documented cleanup contract.
The README and PR description promise cleanup "unless --retain is set." At line 210, if client.Deploy fails (for example, the pod is created but the Service creation fails), the function returns immediately without calling client.Cleanup. Only the WaitPodReady failure branch performs cleanup. Add the same conditional cleanup on the Deploy error path.
Separately, at line 217 the cleanup error is discarded (_ = client.Cleanup(...)). If cleanup itself fails, the user only sees the original readiness error and has no indication that the namespace still holds stale resources. Report the cleanup failure alongside the original error.
🛠️ Proposed fix for both cleanup gaps
fmt.Fprintf(stdout, "[%s] deploying to namespace %q ...\n", shape, ns)
dep, err := client.Deploy(ctx, ns, res)
if err != nil {
+ if !cfg.retain {
+ _ = client.Cleanup(ctx, ns)
+ }
return fmt.Errorf("deploy %s: %w", shape, err)
}
fmt.Fprintf(stdout, "[%s] waiting up to %s for collector pod %q to become ready ...\n", shape, cfg.readyTimeout, dep.PodName)
if err := client.WaitPodReady(ctx, ns, dep.PodName, cfg.readyTimeout); err != nil {
- if !cfg.retain {
- _ = client.Cleanup(ctx, ns)
- }
+ if !cfg.retain {
+ if cerr := client.Cleanup(ctx, ns); cerr != nil {
+ fmt.Fprintf(stdout, "[%s] warning: cleanup of namespace %q failed: %v\n", shape, ns, cerr)
+ }
+ }
return fmt.Errorf("collector did not become ready for %s shape: %w", shape, err)
}🤖 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/cmd/perf/main.go` around
lines 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.
| func (c *Client) applyPod(ctx context.Context, namespace string, pod *corev1.Pod) error { | ||
| pods := c.cs.CoreV1().Pods(namespace) | ||
| err := pods.Delete(ctx, pod.Name, metav1.DeleteOptions{}) | ||
| if err != nil && !k8serrors.IsNotFound(err) { | ||
| return fmt.Errorf("delete existing pod %q: %w", pod.Name, err) | ||
| } | ||
| if err == nil { | ||
| // Wait for the old pod to be fully gone before recreating it, so the | ||
| // create does not race with the deletion. | ||
| if werr := c.waitPodDeleted(ctx, namespace, pod.Name, 60*time.Second); werr != nil { | ||
| return werr | ||
| } | ||
| } | ||
| if _, err := pods.Create(ctx, pod, metav1.CreateOptions{}); err != nil { | ||
| return fmt.Errorf("create pod %q: %w", pod.Name, err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the pod-deletion-wait error with context.
At line 183, waitPodDeleted returns its error unchanged. The error from waitPodDeleted itself is also unwrapped (return false, err inside its poll function at line 237). When this error reaches Deploy and then runRun, the caller only sees the raw Kubernetes client error with no indication that it happened while waiting for the old pod to terminate. Add context at both points so failures are traceable.
As per path instructions, "Check Go error wrapping (%w)... " for src/**/*.go changes.
🩹 Proposed fix to add context to the deletion-wait error path
if err == nil {
// Wait for the old pod to be fully gone before recreating it, so the
// create does not race with the deletion.
if werr := c.waitPodDeleted(ctx, namespace, pod.Name, 60*time.Second); werr != nil {
- return werr
+ return fmt.Errorf("wait for pod %q deletion: %w", pod.Name, werr)
}
}🤖 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 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.
Source: Path instructions
| fmt.Fprintf(stdout, "[%s] deploying to namespace %q ...\n", shape, ns) | ||
| dep, err := client.Deploy(ctx, ns, res) | ||
| if err != nil { | ||
| return fmt.Errorf("deploy %s: %w", shape, err) |
There was a problem hiding this comment.
When Deploy fails after creating the Pod (for example, because Service creation is rejected), this returns before the normal cleanup path and leaves suite resources behind even though --retain is false. Please attempt client.Cleanup(ctx, ns) before returning on deploy failure, and preserve both errors (for example with errors.Join) so a cleanup failure is not silently discarded. Add a test that makes the Service create fail after Pod creation and verifies the Pod is removed.
2 participants
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