feat: implement AgentDeployment admission webhooks and resource quota… - #5
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded GPU-aware tenant quota enforcement, AgentDeployment admission webhooks, TenantQuota reconciliation, Kubernetes webhook manifests, RBAC updates, and integration tests. ChangesTenant quota flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant APIServer
participant AgentDeploymentCustomValidator
participant Enforcer
participant TenantQuotaReconciler
participant AgentDeployment
APIServer->>AgentDeploymentCustomValidator: Validate AgentDeployment
AgentDeploymentCustomValidator->>Enforcer: Check quota and reserve usage
Enforcer-->>AgentDeploymentCustomValidator: Admission result
AgentDeploymentCustomValidator-->>APIServer: Admission response
AgentDeployment->>TenantQuotaReconciler: AgentDeployment event
TenantQuotaReconciler->>Enforcer: Compute committed usage
Enforcer-->>TenantQuotaReconciler: TenantQuota usage
TenantQuotaReconciler-->>APIServer: Update TenantQuota status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…ressions to test helpers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/suite_test.go (1)
156-161: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
BeforeSuitereturns before the webhook server accepts connections.The manager starts in a goroutine and
BeforeSuiteends immediately. The webhook server binds asynchronously. The first spec that creates anAgentDeploymentcan reach the API server before the endpoint listens.The outcome depends on the
failurePolicyinconfig/webhook/manifests.yaml. WithFail, the create returns a connection error and the spec fails. WithIgnore, the object is admitted without validation, and the quota rejection tests ininternal/controller/tenantquota_controller_test.gopass for the wrong reason.Poll the serving address before returning.
💚 Proposed fix
mgrDone = make(chan struct{}) go func() { defer GinkgoRecover() defer close(mgrDone) Expect(mgr.Start(ctx)).To(Succeed()) }() + + // Wait until the webhook server accepts connections. Without this the + // first AgentDeployment create can race the webhook listener. + wio := &testEnv.WebhookInstallOptions + addr := net.JoinHostPort(wio.LocalServingHost, strconv.Itoa(wio.LocalServingPort)) + Eventually(func() error { + conn, err := tls.Dial("tcp", addr, &tls.Config{InsecureSkipVerify: true}) //nolint:gosec // envtest self-signed cert + if err != nil { + return err + } + return conn.Close() + }, timeout, interval).Should(Succeed()) })Add
"crypto/tls","net", and"strconv"to the imports.🤖 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/suite_test.go` around lines 156 - 161, Update BeforeSuite around mgr.Start and mgrDone to wait until the webhook serving address accepts TLS connections before returning. Use crypto/tls, net, and strconv to construct and poll the configured host/port, preserving the existing GinkgoRecover and manager lifecycle handling.
🤖 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 `@api/v1alpha1/error_rate_test.go`:
- Around line 32-41: Add table cases in the error-rate parser tests for
percentage strings with trailing garbage and for non-finite numeric input such
as NaN or infinity, marking each as invalid and asserting the expected zero
value. Use the existing table-driven test cases alongside the entries for
malformed values like "abc%" and "101%".
In `@api/v1alpha1/error_rate.go`:
- Around line 31-37: The error-rate parser in api/v1alpha1/error_rate.go lines
31-37 must use strconv.ParseFloat on the numeric portion so trailing characters
and leading whitespace are rejected, and must reject NaN or infinite values via
math.IsNaN/math.IsInf before the existing [0,100] range check. Extend the table
in api/v1alpha1/error_rate_test.go lines 32-41 with invalid cases for "5x%",
"NaN%", and " 5%", each expecting an error and zero.
In `@internal/controller/suite_test.go`:
- Line 140: Update the suite teardown in AfterSuite to call testEnforcer.Stop()
exactly once for the enforcer initialized by NewEnforcer. Ensure the stop occurs
during cleanup and avoid any additional Stop calls, since repeated invocation
panics.
In `@internal/controller/tenantquota_controller_test.go`:
- Around line 209-217: Update the TenantQuota status assertion in the Eventually
block to require that FindStatusCondition returns nil after usage normalizes.
Replace the conditional status check with a direct g.Expect(cond).To(BeNil())
assertion, preserving the existing resource lookup and retry behavior.
- Around line 259-273: Make the “rejects an AgentDeployment that would exceed
maxAgents” test wait until the tenant quota’s committed usage reflects the first
AgentDeployment before creating ad2. After creating ad1, poll the tq-reject
TenantQuota via the existing test client until status.usedAgents equals 1, then
create ad2 and retain the existing invalid/forbidden assertion.
In `@internal/controller/tenantquota_controller.go`:
- Around line 74-82: In internal/controller/tenantquota_controller.go#L74-L82,
update the tenant quota reconciliation loop to collect committed reservation
keys instead of releasing them immediately, then release those keys only after
Status().Update succeeds or on the no-change path when persisted status already
includes the ADs. In
internal/controller/tenantquota_controller_test.go#L259-L273, add an Eventually
assertion waiting for status.usedAgents to reach 1 after creating ad-reject-1,
before asserting rejection of ad-reject-2.
- Around line 67-70: Register a field index for AgentDeployment.spec.tenantRef
in SetupWithManager, then update the AgentDeployment list in the reconcile flow
to use client.MatchingFields with the tenant reference instead of listing the
entire namespace for in-memory filtering. Preserve the existing tenant-specific
behavior and error handling.
In `@internal/controller/test_helpers_test.go`:
- Around line 36-38: Update the namespacedName helper to accept arguments in
namespace, name order, matching types.NamespacedName and the package’s
namespace/name convention; update every call site accordingly while preserving
the resulting Name and Namespace fields.
In `@internal/quota/enforcer_test.go`:
- Around line 346-348: Update the ParseErrorRate test-section comment to
reference api/v1alpha1/error_rate_test.go, and clean up the quota arithmetic
comments near the relevant assertions by replacing the duplicated, unfinished
reasoning with one concise statement.
- Around line 232-260: Add a true concurrent-create test alongside
TestReservation_BlocksConcurrentCreate that launches two goroutines against the
combined TryAdmit path with exactly one available slot, synchronizes their
start, and asserts exactly one succeeds; also cover concurrent reservation
release. Make the requested quota scenarios table-driven, including at-limit,
over-limit, GPU extraction edge cases, and concurrent reservation release, using
the existing quota test helpers and running safely under the race detector.
- Around line 364-373: Replace the hand-rolled logic in containsSubstring with
strings.Contains, adding the strings import as needed; alternatively remove
containsSubstring and update its call sites to use strings.Contains directly.
In `@internal/quota/enforcer.go`:
- Around line 159-227: The quota check and reservation are not atomic, allowing
concurrent admissions to exceed capacity. In internal/quota/enforcer.go lines
159-227, add exported TryAdmit that holds mu through evaluation and reservation,
refactor CanAdmit, sumInflight, and Reserve into *Locked helpers, and retain
existing exported methods as lock-and-delegate wrappers; update the admission
webhook to use TryAdmit. In internal/quota/enforcer_test.go lines 232-260, add
concurrent two-goroutine one-slot coverage asserting exactly one admission, plus
concurrent reservation-release coverage; run both with -race -count=100.
- Around line 81-83: Make Enforcer.Stop idempotent so repeated calls cannot
panic when closing e.done. Update the Stop method to guard the channel close
using the existing synchronization mechanism or an equivalent one-time shutdown
guard, while preserving the first-call shutdown behavior.
- Around line 128-131: Update Enforcer.gpusForAD to perform the per-replica GPU
multiplication in int64 without converting to int32 first, then clamp the result
to the int32 range before returning it; preserve the existing resource
extraction and replica calculation while preventing truncation and overflow.
In `@internal/webhook/agentdeployment_webhook.go`:
- Line 174: Make spec.tenantRef immutable in the validation flow around
validateSpec and CanAdmit by comparing the new and old AgentDeployment specs and
rejecting updates when the tenant reference changes. Add a regression test
covering transfer into a TenantQuota already at its limit, verifying the update
is denied.
---
Outside diff comments:
In `@internal/controller/suite_test.go`:
- Around line 156-161: Update BeforeSuite around mgr.Start and mgrDone to wait
until the webhook serving address accepts TLS connections before returning. Use
crypto/tls, net, and strconv to construct and poll the configured host/port,
preserving the existing GinkgoRecover and manager lifecycle handling.
🪄 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: 9900a7e2-54ca-4151-aa19-991860656a05
📒 Files selected for processing (21)
.agents/AGENTS.md.agents/skills/agentrax-context/SKILL.mdapi/v1alpha1/error_rate.goapi/v1alpha1/error_rate_test.gocmd/main.goconfig/rbac/role.yamlconfig/webhook/kustomization.yamlconfig/webhook/manifests.yamlconfig/webhook/service.yamlgo.modinternal/controller/agentdeployment_controller_test.gointernal/controller/enqueue_handlers.gointernal/controller/suite_test.gointernal/controller/tenantquota_controller.gointernal/controller/tenantquota_controller_test.gointernal/controller/test_helpers_test.gointernal/quota/enforcer.gointernal/quota/enforcer_test.gointernal/webhook/agentdeployment_validator_test.gointernal/webhook/agentdeployment_webhook.gointernal/webhook/agentdeployment_webhook_test.go
…ity in quota enforcer and controller logic
Contributor
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 (3)
internal/controller/tenantquota_controller_test.go (3)
96-105: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert owner references on reconciled children.
This test creates an
AgentDeploymentthrough the live envtest manager but only checksTenantQuota.Status. Add or reuse assertions for every child resource produced by theAgentDeploymentreconciler, includingDeployment,Service,ServiceMonitor, andHPAwhen applicable. Apply the helper to the other AgentDeployment fixtures in this file.As per path instructions, tests must assert owner references on every created child resource because missing owner references can orphan resources.
🤖 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/tenantquota_controller_test.go` around lines 96 - 105, Extend the AgentDeployment fixtures in the tenant quota controller tests to verify owner references for every reconciled child resource: Deployment, Service, ServiceMonitor, and applicable HPA. Add or reuse a helper that checks each child is owned by its AgentDeployment, invoke it for the ad-count-1 fixture and all other AgentDeployment fixtures, while preserving the existing status assertions.Source: Path instructions
51-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake
AfterEachcleanup deterministic.
AfterEachignoresUpdateandDeleteerrors and does not wait for resources to disappear. Because all specs reusetqNS, a terminatingAgentDeploymentorTenantQuotacan affect later quota counts and list assertions. TreatNotFoundas success, fail on other errors, and wait until test resources are deleted.As per path instructions, errors must not be swallowed silently.
🤖 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/tenantquota_controller_test.go` around lines 51 - 70, Make the AfterEach cleanup for AgentDeployments and TenantQuotas fail on any Update or Delete error except NotFound, and wait until each resource is confirmed deleted before cleanup completes. Update the loops around adList.Items and tqList.Items to handle errors explicitly, then use the test client’s existing polling or Eventually mechanism to verify deletion, preserving the finalizer removal step for AgentDeployments.Source: Path instructions
209-216: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert normalized usage with condition removal.
The test only checks that
OverQuotais absent. It can pass if the condition is removed whileStatus.UsedAgentsremains2. Also assertUsedAgents == 1and the expected replica total in the sameEventuallyblock.Proposed assertion
cond := apimeta.FindStatusCondition(f.Status.Conditions, agentraxv1alpha1.ConditionOverQuota) + g.Expect(f.Status.UsedAgents).To(BeNumerically("==", 1)) + g.Expect(f.Status.UsedTotalReplicas).To(BeNumerically("==", 2)) // The reconciler calls RemoveStatusCondition, so the condition🤖 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/tenantquota_controller_test.go` around lines 209 - 216, Extend the `Eventually` block for the normalized `tq-clearoq` TenantQuota to assert that `f.Status.UsedAgents` equals 1 and that the status reports the expected replica total, in addition to verifying the `ConditionOverQuota` condition is absent.
♻️ Duplicate comments (2)
api/v1alpha1/error_rate_test.go (1)
41-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover infinity inputs.
The table covers
NaN%, but it does not cover themath.IsInfbranch. AddInf%and-Inf%invalid cases.🤖 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 `@api/v1alpha1/error_rate_test.go` around lines 41 - 46, Add table-driven invalid cases for both "Inf%" and "-Inf%" in the existing error-rate parsing tests, alongside the existing "NaN%" case, with the expected zero value and error result matching the other non-finite inputs.internal/quota/enforcer.go (1)
130-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck multiplication overflow before the conversion.
perReplica * int64(ad.Replicas.Max)can overflowint64before the upper-bound check. A wrapped value can then reachint32(total)and undercount GPU usage. Reject negative operands or fail closed, check multiplication overflow, and convert only after validation.Suggested guard
func (e *Enforcer) gpusForAD(ad agentraxv1alpha1.AgentDeploymentSpec) int32 { perReplica := e.extractGPUs(ad.Resources) - total := perReplica * int64(ad.Replicas.Max) + maxReplicas := int64(ad.Replicas.Max) + if perReplica < 0 || maxReplicas < 0 { + return math.MaxInt32 + } + if perReplica != 0 && maxReplicas > math.MaxInt64/perReplica { + return math.MaxInt32 + } + total := perReplica * maxReplicas🤖 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/quota/enforcer.go` around lines 130 - 139, Update Enforcer.gpusForAD to validate perReplica and ad.Replicas.Max are non-negative, detect int64 multiplication overflow before calculating total, and fail closed with the maximum GPU value when validation fails. Perform the existing int32 upper-bound clamp only after safe multiplication, then convert total to int32.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.
Outside diff comments:
In `@internal/controller/tenantquota_controller_test.go`:
- Around line 96-105: Extend the AgentDeployment fixtures in the tenant quota
controller tests to verify owner references for every reconciled child resource:
Deployment, Service, ServiceMonitor, and applicable HPA. Add or reuse a helper
that checks each child is owned by its AgentDeployment, invoke it for the
ad-count-1 fixture and all other AgentDeployment fixtures, while preserving the
existing status assertions.
- Around line 51-70: Make the AfterEach cleanup for AgentDeployments and
TenantQuotas fail on any Update or Delete error except NotFound, and wait until
each resource is confirmed deleted before cleanup completes. Update the loops
around adList.Items and tqList.Items to handle errors explicitly, then use the
test client’s existing polling or Eventually mechanism to verify deletion,
preserving the finalizer removal step for AgentDeployments.
- Around line 209-216: Extend the `Eventually` block for the normalized
`tq-clearoq` TenantQuota to assert that `f.Status.UsedAgents` equals 1 and that
the status reports the expected replica total, in addition to verifying the
`ConditionOverQuota` condition is absent.
---
Duplicate comments:
In `@api/v1alpha1/error_rate_test.go`:
- Around line 41-46: Add table-driven invalid cases for both "Inf%" and "-Inf%"
in the existing error-rate parsing tests, alongside the existing "NaN%" case,
with the expected zero value and error result matching the other non-finite
inputs.
In `@internal/quota/enforcer.go`:
- Around line 130-139: Update Enforcer.gpusForAD to validate perReplica and
ad.Replicas.Max are non-negative, detect int64 multiplication overflow before
calculating total, and fail closed with the maximum GPU value when validation
fails. Perform the existing int32 upper-bound clamp only after safe
multiplication, then convert total to int32.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c297b81-066a-4685-9567-f68aab43a9cc
📒 Files selected for processing (6)
api/v1alpha1/error_rate.goapi/v1alpha1/error_rate_test.gointernal/controller/suite_test.gointernal/controller/tenantquota_controller_test.gointernal/quota/enforcer.gointernal/quota/enforcer_test.go
…ion against overflow, and expand error rate parsing test cases
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
1 participant
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.
… enforcement
Summary by CodeRabbit
New Features
Bug Fixes
Documentation