feat: implement reconciliation logic for AgentDeployment, including c… - #2
Conversation
…hild resource creation and status management
|
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:
📝 WalkthroughWalkthroughThe PR replaces the placeholder controller with a full ChangesAgentDeployment controller
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AgentDeploymentReconciler
participant KubernetesAPI
participant Deployment
participant Service
participant ServiceMonitor
User->>KubernetesAPI: create AgentDeployment
KubernetesAPI->>AgentDeploymentReconciler: enqueue reconciliation
AgentDeploymentReconciler->>KubernetesAPI: check ServiceMonitor CRD
AgentDeploymentReconciler->>KubernetesAPI: create or update Deployment
AgentDeploymentReconciler->>KubernetesAPI: create or update Service
AgentDeploymentReconciler->>KubernetesAPI: create or update ServiceMonitor
Deployment-->>AgentDeploymentReconciler: report readiness and pod state
AgentDeploymentReconciler->>KubernetesAPI: update phase and conditions
🚥 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: 6
🤖 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/controller/agentdeployment_controller_test.go`:
- Around line 119-140: Add an injectable `Deregister func(ctx context.Context,
ad *AgentDeployment) error` hook to `AgentDeploymentReconciler`, and have
`runDeletionCleanup` invoke it before finalizer removal. Update the deletion
test to record whether the child Service exists when deregistration runs, assert
the hook was called while the Service still exists, then verify the Service is
eventually garbage-collected.
In `@internal/controller/agentdeployment_controller.go`:
- Around line 216-220: Update the re-fetch Get error handling for latest in the
status update flow to check apierrors.IsNotFound immediately; return
ctrl.Result{} with no error when the AgentDeployment was deleted, and preserve
the existing wrapped error for all other failures.
- Around line 402-407: Update AgentDeploymentReconciler.SetupWithManager to
resolve the existing ServiceMonitor CRD availability check once during setup,
then conditionally add Owns(&monitoringv1.ServiceMonitor{}) only when that CRD
is present. Preserve the existing AgentDeployment, Deployment, and Service
watches and return any setup errors appropriately.
- Around line 142-150: Update the Deployment reconciliation around
controllerutil.CreateOrUpdate and the existing.Spec assignment to preserve
immutable, stable selector labels while keeping the tenant label outside
spec.selector.matchLabels; migrate or recreate existing Deployments whose
selectors use the old tenant-derived labels. Replace wholesale spec and
container-list assignments with updates only to controller-owned fields,
preserving API-defaulted values, and wrap the returned CreateOrUpdate error with
deployment context.
In `@internal/controller/crd_check.go`:
- Around line 32-42: Resolve ServiceMonitor CRD availability once during manager
setup instead of during reconciliation. In
internal/controller/crd_check.go:32-42, update serviceMonitorCRDExists to accept
client.Reader, use apierrors.IsNotFound, and invoke it with an uncached reader.
In internal/controller/agentdeployment_controller.go:402-407, call it once from
SetupWithManager using mgr.GetAPIReader(), store the result on
AgentDeploymentReconciler, conditionally add
Owns(&monitoringv1.ServiceMonitor{}) when present, and make
reconcileServiceMonitor use the stored result.
In `@internal/controller/suite_test.go`:
- Around line 85-96: Register the apiextensionsv1 Kubernetes types in the test
scheme before constructing the client, alongside the existing agentrax, apps,
core, and monitoring registrations. Update the scheme setup in suite
initialization so serviceMonitorCRDExists can read CustomResourceDefinition
objects without an unregistered-kind error.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 0862e113-e209-4bf4-ab7d-8196d0a62bd2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (9)
config/rbac/role.yamlconfig/samples/agentrax_v1alpha1_agentdeployment.yamlgo.modinternal/controller/agentdeployment_builder_test.gointernal/controller/agentdeployment_controller.gointernal/controller/agentdeployment_controller_test.gointernal/controller/conditions.gointernal/controller/crd_check.gointernal/controller/suite_test.go
…reconciliation and ServiceMonitor check efficiency
|
@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 (2)
internal/controller/agentdeployment_controller.go (1)
287-289: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid a status-update reconcile loop.
SetupWithManagerwatchesAgentDeploymentupdates.Status().Updateruns even when the computed status equals the stored status. Each write emits another update event, so a stable object can reconcile continuously.Save the previous status and skip
Status().Updatewhen the status is unchanged. Keep the explicitRequeueAfterpath forPending.🤖 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/controller/agentdeployment_controller.go` around lines 287 - 289, In the reconcile flow around Status().Update, preserve the existing AgentDeployment status before computing the new status, compare it afterward, and update the status only when it changed. Keep the explicit RequeueAfter behavior for Pending unchanged.internal/controller/agentdeployment_controller_test.go (1)
197-206: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the complete controller owner reference.
A reference with the expected name and kind but a stale UID still passes these tests. A non-controller reference also passes, but it breaks owned-resource watch behavior. Fetch the parent and assert the child reference UID equals the parent UID and
Controlleristrue.
internal/controller/agentdeployment_controller_test.go#L197-L206: Assert the Deployment owner reference UID and controller flag.internal/controller/agentdeployment_controller_test.go#L219-L228: Assert the Service owner reference UID and controller flag.As per path instructions: "Assert owner references on every created child resource."
🤖 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/controller/agentdeployment_controller_test.go` around lines 197 - 206, Update the owner-reference assertions in internal/controller/agentdeployment_controller_test.go:197-206 for the Deployment and 219-228 for the Service. Fetch the parent AgentDeployment in each test, then assert the child’s sole owner reference UID matches the parent UID and its Controller flag is true, while retaining the existing name and kind assertions.Source: Path instructions
🤖 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/controller/agentdeployment_controller_test.go`:
- Around line 140-162: Replace the unsynchronized hookCalled and
serviceExistedDuringDeregister variables in the Deregister test with a buffered
result channel. Have the Deregister hook send its observed Service-existence
result through the channel, then use Eventually to receive that result before
asserting it, while preserving the existing assertions that deregistration
occurred and the Service was still present.
---
Outside diff comments:
In `@internal/controller/agentdeployment_controller_test.go`:
- Around line 197-206: Update the owner-reference assertions in
internal/controller/agentdeployment_controller_test.go:197-206 for the
Deployment and 219-228 for the Service. Fetch the parent AgentDeployment in each
test, then assert the child’s sole owner reference UID matches the parent UID
and its Controller flag is true, while retaining the existing name and kind
assertions.
In `@internal/controller/agentdeployment_controller.go`:
- Around line 287-289: In the reconcile flow around Status().Update, preserve
the existing AgentDeployment status before computing the new status, compare it
afterward, and update the status only when it changed. Keep the explicit
RequeueAfter behavior for Pending unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 47a49af1-3f45-4320-a515-cdd915c9be15
📒 Files selected for processing (4)
internal/controller/agentdeployment_controller.gointernal/controller/agentdeployment_controller_test.gointernal/controller/crd_check.gointernal/controller/suite_test.go
… owner reference test convergence
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/agentdeployment_controller.go (1)
263-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCompare the complete status before updating it.
len(latest.Status.Conditions)misses changes to existing conditions. When scalar status fields remain unchanged,Status().Updateis skipped. Deep-copylatestbefore mutation and compare the complete status withequality.Semantic.DeepEqual.🤖 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/controller/agentdeployment_controller.go` around lines 263 - 303, Update the status-change detection around the reconciliation status mutations by deep-copying latest before modifying its status, then compare the complete previous and current statuses with equality.Semantic.DeepEqual. Replace the scalar and conditions-length checks in statusChanged, while preserving the existing conditional Status().Update behavior.
🤖 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/controller/agentdeployment_controller_test.go`:
- Around line 200-216: Add explicit cleanup for child resources created by each
test case, deleting retained Deployments before creating or reconciling the next
AgentDeployment. Update the test teardown/setup around the existing parent and
child objects so stale resources and owner references cannot persist across
runs; keep the Eventually assertion focused on verifying convergence to the
current parent UID.
- Around line 236-257: Install the monitoring.coreos.com ServiceMonitor CRD
before SetupWithManager so the ServiceMonitor child-resource path is exercised
in envtest. Extend the relevant AgentDeployment controller test to fetch the
ServiceMonitor during Eventually and assert its owner UID matches the parent,
its kind is ServiceMonitor, and its Controller reference is true.
---
Outside diff comments:
In `@internal/controller/agentdeployment_controller.go`:
- Around line 263-303: Update the status-change detection around the
reconciliation status mutations by deep-copying latest before modifying its
status, then compare the complete previous and current statuses with
equality.Semantic.DeepEqual. Replace the scalar and conditions-length checks in
statusChanged, while preserving the existing conditional Status().Update
behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 30674f59-e8a3-4e41-83be-4b88412e162d
📒 Files selected for processing (2)
internal/controller/agentdeployment_controller.gointernal/controller/agentdeployment_controller_test.go
…itor support with integration tests
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/agentdeployment_controller_test.go (1)
225-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert against
testNginxImage, not a duplicated literal.Line 215 creates the fixture with
testNginxImage. Line 232 asserts"nginx:latest". The two only agree by coincidence. Changing the constant breaks this spec for no real reason.♻️ Proposed refactor
- Expect(dep.Spec.Template.Spec.Containers[0].Image).To(Equal("nginx:latest")) + Expect(dep.Spec.Template.Spec.Containers[0].Image).To(Equal(testNginxImage))🤖 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/controller/agentdeployment_controller_test.go` around lines 225 - 234, Update the image assertion in the “creates a Deployment with correct image and port” test to compare against the existing testNginxImage fixture constant instead of the duplicated "nginx:latest" literal; leave the port assertion unchanged.internal/controller/agentdeployment_controller.go (1)
282-286: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRead
StableVersionfromlatest, not the staleadcopy.Every other write in this block targets
latest, which was re-fetched at line 257. Line 284 readsad.Spec.Imagefrom the copy captured at the start of reconcile. If the spec changed in between, the status records the previous image as stable.🐛 Proposed fix
if dep.Status.ReadyReplicas > 0 { latest.Status.Phase = agentraxv1alpha1.PhaseRunning - latest.Status.StableVersion = ad.Spec.Image + latest.Status.StableVersion = latest.Spec.Image🤖 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/controller/agentdeployment_controller.go` around lines 282 - 286, Update the StableVersion assignment in the ReadyReplicas branch of the reconciliation flow to read the image from the refreshed latest object, matching the other status updates in that block, while leaving the surrounding phase and condition updates unchanged.
♻️ Duplicate comments (1)
internal/controller/agentdeployment_controller_test.go (1)
174-183: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe hook assignment itself is still racy under
-race.The buffered channel fixed the observation race. It did not fix this write. Line 178 writes
testReconciler.Deregisterfrom the test goroutine. The manager goroutine reads that same field during any in-flight reconcile ofad-finalizer, and status reconciles requeue every 5 seconds while the phase is Pending. Line 183 repeats the write.go test -racecan report both.Guard the field with a mutex on the reconciler, or store the hook in an
atomic.Pointer[func...].🤖 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/controller/agentdeployment_controller_test.go` around lines 174 - 183, Protect testReconciler.Deregister from concurrent access during the test, including both the hook assignment and cleanup. Add and use a mutex on the reconciler (or an atomic function pointer), and ensure reconciliation reads the hook through the same synchronization mechanism so the injected Deregister callback and DeferCleanup reset are race-free.
🤖 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.
Outside diff comments:
In `@internal/controller/agentdeployment_controller_test.go`:
- Around line 225-234: Update the image assertion in the “creates a Deployment
with correct image and port” test to compare against the existing testNginxImage
fixture constant instead of the duplicated "nginx:latest" literal; leave the
port assertion unchanged.
In `@internal/controller/agentdeployment_controller.go`:
- Around line 282-286: Update the StableVersion assignment in the ReadyReplicas
branch of the reconciliation flow to read the image from the refreshed latest
object, matching the other status updates in that block, while leaving the
surrounding phase and condition updates unchanged.
---
Duplicate comments:
In `@internal/controller/agentdeployment_controller_test.go`:
- Around line 174-183: Protect testReconciler.Deregister from concurrent access
during the test, including both the hook assignment and cleanup. Add and use a
mutex on the reconciler (or an atomic function pointer), and ensure
reconciliation reads the hook through the same synchronization mechanism so the
injected Deregister callback and DeferCleanup reset are race-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd61153c-c29c-4a89-bac1-db7576542416
📒 Files selected for processing (4)
config/crd/external/monitoring.coreos.com_servicemonitors.yamlinternal/controller/agentdeployment_controller.gointernal/controller/agentdeployment_controller_test.gointernal/controller/suite_test.go
…revent data races during testing and fix stable version update logic
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/controller/agentdeployment_controller.go`:
- Around line 302-306: Update the readiness logic in the controller
reconciliation block around dep.Status.ReadyReplicas so StableVersion is
advanced only after the Deployment observes the reconciled generation and its
updated and available replica counts satisfy rollout completion. Derive
StableVersion from the reconciled Deployment container image rather than
latest.Spec.Image, and add an image-update test covering a partial rollout where
StableVersion remains unchanged.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 206d0d4a-57b0-4669-a2ed-944f3d8e43df
📒 Files selected for processing (2)
internal/controller/agentdeployment_controller.gointernal/controller/agentdeployment_controller_test.go
Fixes Applied SuccessfullyFixed 4 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
… in AgentDeployment controller
1 participant
…hild resource creation and status management
Summary by CodeRabbit
New Features
Tests