fix-quota-cpu-memory-checks #15
Conversation
Convert stored Docker CPU shares into CPU cores when returning quota results. Previously the function returned raw cpu_shares values, which did not reflect user-visible CPU capacity. The change divides cpu_shares by 1024 (1024 shares = 1 core) and rounds up any partial core so that consumers see a whole-core allocation estimate. This prevents under-reporting CPU needs and aligns quota reporting with how CPU resources are commonly interpreted.
Add quota checks that include memory and CPU when starting or scaling deployments. When starting a deployment we now read the deployment's memory and CPU (falling back to sensible defaults) and call the quota checker to ensure the allocation is allowed before proceeding. The scale endpoint similarly fetches the deployment, derives memory/CPU values, and validates the requested replica count together with those resources. Refactor quota checker internals: rename getPlanLimits to getPlanLimitsFromQuota and adjust callers to supply the already-loaded OrgQuota. Also tidyup the currentAllocations query comment and formatting. These changes prevent exceeding memory/CPU plan limits during start and scale operations and ensure quota decisions consider per-deployment resource usage, reducing risk of resource overcommit.
There was a problem hiding this comment.
Pull request overview
This PR tightens and corrects quota enforcement for deployments, especially around CPU/memory checks and how plan limits are derived. It also updates scaling and start logic to consult the shared quota checker with more accurate resource inputs.
Changes:
- Use an already-loaded
OrgQuotato resolve plan limits and refine how current CPU/memory allocations are calculated, including replica-aware aggregation and status filtering. - Add a quota check to
StartDeploymentso starting a deployment considers replicas, memory, and CPU against organization limits. - Extend the
ScaleDeploymentquota check to include memory/CPU, reordering DB lookups accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/shared/pkg/quota/quota.go | Refactors plan-limit lookup to use an existing OrgQuota and updates currentAllocations to compute memory/CPU usage per replica and by deployment status. |
| apps/deployments-service/internal/service/lifecycle.go | Adds quota checks for start/scale operations using the shared quota checker, including replicas, memory, and CPU shares. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Quota check: replicas + memory/CPU | ||
| newReplicas := int(req.Msg.GetReplicas()) | ||
| if newReplicas <= 0 { | ||
| return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("replicas must be > 0")) | ||
| } | ||
| if err := s.quotaChecker.CanAllocate(ctx, orgID, quota.RequestedResources{Replicas: newReplicas}); err != nil { | ||
| return nil, connect.NewError(connect.CodeFailedPrecondition, err) | ||
| dbDep, err := s.repo.GetByID(ctx, deploymentID) | ||
| if err != nil { | ||
| return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("deployment %s not found", deploymentID)) | ||
| } | ||
| scaleMemory := int64(512 * 1024 * 1024) // Default 512MB | ||
| if dbDep.MemoryBytes != nil { | ||
| scaleMemory = *dbDep.MemoryBytes | ||
| } | ||
| scaleCPU := int64(1024) // Default | ||
| if dbDep.CPUShares != nil { | ||
| scaleCPU = *dbDep.CPUShares | ||
| } | ||
| if err := s.quotaChecker.CanAllocate(ctx, orgID, quota.RequestedResources{ | ||
| Replicas: newReplicas, | ||
| MemoryBytes: scaleMemory, | ||
| CPUshares: scaleCPU, | ||
| }); err != nil { |
There was a problem hiding this comment.
The quota check here passes newReplicas as the requested replica count (along with per-replica memory/CPU) to CanAllocate, but ScaleDeploymentRequest.replicas represents the target replica count, not an increment. As a result, the quota checker treats the full target replicas as an additional allocation on top of the current replica count, which can incorrectly reject valid scaling operations (and also mis-compute the delta for memory/CPU usage). Consider fetching the current replica count for this deployment and passing only the positive delta (e.g., max(0, newReplicas-currentReplicas)) and corresponding incremental memory/CPU to CanAllocate, so the quota check models the actual change in resource usage.
| type agg struct{ Mem int64; CPU int64 } | ||
| var a agg | ||
| if err = database.DB.Model(&database.Deployment{}). | ||
| Select("COALESCE(SUM(COALESCE(memory_bytes,0)),0) as mem, COALESCE(SUM(COALESCE(cpu_shares,0)),0) as cpu"). | ||
| Where("organization_id = ?", orgID).Scan(&a).Error; err != nil { return } | ||
| return int(count), a.Mem, int(a.CPU), nil | ||
| Select("COALESCE(SUM(COALESCE(memory_bytes,0) * COALESCE(replicas,1)),0) as mem, COALESCE(SUM(COALESCE(cpu_shares,0) * COALESCE(replicas,1)),0) as cpu"). | ||
| Where("organization_id = ? AND deleted_at IS NULL AND status IN (2,3,6)", orgID). | ||
| Scan(&a).Error; err != nil { | ||
| return | ||
| } | ||
| // Convert Docker CPU shares to cores (1024 shares = 1 core) | ||
| cpuCores = int(a.CPU) / 1024 | ||
| if a.CPU%1024 != 0 { | ||
| cpuCores++ // round up partial cores | ||
| } | ||
| return int(count), a.Mem, cpuCores, nil |
There was a problem hiding this comment.
currentAllocations now aggregates CPU usage as cpu_shares * replicas and converts that to cores, but the corresponding CPU quota check in CanAllocate still derives requested cores only from the per-replica CPUshares value and does not multiply by the requested replica count. This inconsistency means that scaling or starting deployments with multiple replicas can under-count the additional CPU usage in the quota check and allow effective CPU consumption to exceed the plan/override limit. Consider updating the requested-CPU calculation in CanAllocate to match this aggregation strategy (e.g., using per-replica CPU shares times the requested replica count before converting to cores).
Adjust quota checks to account for only the delta when scaling a deployment, rather than counting the deployment's current replicas as new allocations. Compute currentReplicas from the DB and calculate deltaReplicas = newReplicas - currentReplicas; perform memory/CPU quota validation only if deltaReplicas > 0. This prevents false quota failures when reducing replicas or keeping the same count. Also change CPU accounting in quota calculations to sum CPUshares across replicas before converting to cores. Multiply CPUshares by replicas, then divide by 1024 and round up partial cores. This makes CPU and memory checks consistent with currentAllocations and the new delta-based replication logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.