feat(2951): add admin GPU quotas for Emerald and KLAB2 - #7900
Conversation
There was a problem hiding this comment.
Pull request overview
Adds GPU quota support to Private Cloud resource requests, with the intent that GPU quotas are only configurable by admins and only for the Emerald and KLAB2 clusters.
Changes:
- Extend resource request schemas/constants/types to include a
gpuquota (default 0, max 8). - Update Quotas UI to conditionally display GPU for admins on Emerald/KLAB2 and include GPU in change detection.
- Include GPU in the NATS message payload (for Emerald/KLAB2), add server-side GPU normalization on product create, and add/adjust tests + mock normalization utilities.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/validation-schemas/private-cloud.ts | Adds gpu validation (int, 0–8, default 0) to resource requests schema. |
| app/services/nats/private-cloud/index.ts | Adds conditional GPU quota to outgoing NATS quotas payload for Emerald/KLAB2. |
| app/prisma/schema.prisma | Adds gpu to ResourceRequests and ResourceType enum. |
| app/helpers/mock-resources/private-cloud-product.ts | Adds GPU to mock resource requests; adds normalization helpers to ensure GPU defaults to 0. |
| app/constants/private-cloud.ts | Adds gpu to default resource requests and resource key list. |
| app/components/private-cloud/sections/Quotas.tsx | Adds admin-gated GPU visibility and GPU-aware change detection/rendering. |
| app/app/private-cloud/products/(product)/create/page.tsx | Passes isAdmin into Quotas on create page. |
| app/app/private-cloud/products/(product)/[licencePlate]/edit/page.tsx | Normalizes missing GPU values to 0 when resetting the edit form; passes isAdmin to Quotas. |
| app/app/api/private-cloud/requests/[id]/decision/route.test.ts | Normalizes resource requests in test payloads; adds tests for GPU reset behavior on create. |
| app/app/api/private-cloud/products/_operations/create.ts | Enforces GPU = 0 unless (Emerald/KLAB2 && admin) during create. |
Suppressed comments (3)
app/components/private-cloud/sections/Quotas.tsx:145
changedcomparesoriginalVal?.[resourceKey]directly against the current form value. Sincegpuis optional/nullable on existing data,undefinedvs0will incorrectly mark every namespace as changed when GPU is visible.
const newVal = (resourceRequests[namespace] || {}) as ResourceRequests;
const changed =
hasOriginalVal &&
visibleResourceKeys.some((resourceKey) => originalVal?.[resourceKey] !== newVal?.[resourceKey]);
app/components/private-cloud/sections/Quotas.tsx:192
- When
originalResourceRequestsdoesn’t havegpupopulated,String(originalVal?.[resourceKey])renders as "undefined" in the UI and also makes the per-field change indicator incorrect. Coalesce GPU to 0 for display/comparison.
{visibleResourceKeys.map((resourceKey) => {
const oldval = String(originalVal?.[resourceKey]);
const newval = String(newVal[resourceKey]);
app/components/private-cloud/sections/Quotas.tsx:212
- The GPU input is always enabled (
disabled={resourceKey === 'gpu' ? false : disabled}), which lets admins edit GPU even when the Quotas section is meant to be read-only (e.g., create flow passesdisabled: true). This enables submitting GPU values from a UI that appears locked and is inconsistent with the rest of the form’s permission model.
step={resourceKey === 'cpu' ? 0.5 : 1}
placeholder="0"
required
disabled={resourceKey === 'gpu' ? false : disabled}
classNames={{ wrapper: 'mt-3' }}
options={{ valueAsNumber: true }}
min={0}
max={
resourceKey === 'cpu' ? 64 : resourceKey === 'memory' ? 128 : resourceKey === 'gpu' ? 8 : 512
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const canShowGpu = isAdmin && (currentCluster === Cluster.EMERALD || currentCluster === Cluster.KLAB2); | ||
|
|
||
| const visibleResourceKeys = resourceKeys.filter((resourceKey) => resourceKey !== 'gpu' || canShowGpu); | ||
|
|
| const gpuEnabledCluster = rest.cluster === Cluster.EMERALD || rest.cluster === Cluster.KLAB2; | ||
|
|
||
| rest.resourceRequests = Object.fromEntries( | ||
| Object.entries(rest.resourceRequests).map(([namespace, requests]) => [ | ||
| namespace, | ||
| { | ||
| ...requests, | ||
| gpu: gpuEnabledCluster && session.isAdmin ? requests.gpu ?? 0 : 0, | ||
| }, | ||
| ]), | ||
| ) as typeof rest.resourceRequests; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
app/components/private-cloud/sections/Quotas.tsx:202
- In the quota grid,
oldval/newvalare derived viaoriginalVal?.[resourceKey]andnewVal[resourceKey]. For legacy products/requests wheregpuis missing, this renders "undefined" and can show a misleading "Original value" even though GPU is treated as 0 elsewhere (getResourceValue). UsegetResourceValueconsistently here so GPU falls back to 0.
{visibleResourceKeys.map((resourceKey) => {
const oldval = String(originalVal?.[resourceKey]);
const newval = String(newVal[resourceKey]);
| cpu Float | ||
| memory Float | ||
| storage Float | ||
| gpu Int |
| cpu | ||
| memory | ||
| storage | ||
| gpu | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
app/helpers/quota-change.ts:30
- GPU quota increases are now treated as requiring admin review (see getQuotaChangeStatus), but isQuotaUpgrade() still ignores GPU. This can misclassify GPU-only increases as “not a quota upgrade” (e.g., TeamEditRequest email messaging), leading to incorrect user-facing behavior.
export function sanitizeGpuResourceRequests(
resourceRequests: ResourceRequestsEnv,
cluster: Cluster,
isAdmin: boolean,
): ResourceRequestsEnv {
const gpuEnabled = isAdmin && (cluster === Cluster.EMERALD || cluster === Cluster.KLAB2);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (2)
app/helpers/quota-change.ts:35
- GPU requests are only sanitized in the product create/update operations, but the request decision route currently persists
resourceRequests.gpufrom reviewer input without applying this helper. That means any user with request-review permissions (not necessarily an admin) can approve a request with non-zero GPU quotas, and GPU can also be set on non-supported clusters.
export function sanitizeGpuResourceRequests(
resourceRequests: ResourceRequestsEnv,
cluster: Cluster,
isAdmin: boolean,
): ResourceRequestsEnv {
const gpuEnabled = isAdmin && (cluster === Cluster.EMERALD || cluster === Cluster.KLAB2);
data-migrations/migrations/20260819203556-backfill-private-cloud-gpu.js:34
- This migration runs an unfiltered
updateMany({})on both collections, which forces a full collection scan and (depending on MongoDB behavior) may rewrite documents even when GPU is already set. Filtering to only docs with missing/nullresourceRequests.*.gpuwill reduce load and migration runtime.
const productResult = await PrivateCloudProduct.updateMany({}, gpuBackfillPipeline, {
session,
});
const requestDataResult = await PrivateCloudRequestData.updateMany({}, gpuBackfillPipeline, {
session,
});
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
app/components/private-cloud/sections/Quotas.tsx:56
currentClusteris introduced (fromcluster ?? formCluster), but the React Queryenabledflags and params still rely on theclusterprop. Whenclusteris not passed (e.g., create flow), selecting a cluster in the form won't trigger subnet / PDB queries, even though GPU visibility is based onformCluster. UsecurrentClusterconsistently for query keys/enabled/params so queries react to form cluster changes.
const currentCluster = cluster ?? formCluster;
const canShowGpu = isAdmin && (currentCluster === Cluster.EMERALD || currentCluster === Cluster.KLAB2);
const visibleResourceKeys = resourceKeys.filter((resourceKey) => resourceKey !== 'gpu' || canShowGpu);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (3)
app/app/api/private-cloud/products/_operations/create.ts:49
- GPU sanitization was added here, but the existing create operation tests don’t assert this behavior. Please add coverage to ensure GPU requests are forced to 0 for non-admin/non-reviewer sessions and/or when
clusteris not EMERALD/KLAB2 (and preserved when allowed).
if (rest.cluster === Cluster.GOLDDR) rest.cluster = Cluster.GOLD;
const canManageGpu = !!session?.isAdmin || !!session?.permissions.reviewAllPrivateCloudRequests;
rest.resourceRequests = sanitizeGpuResourceRequests(rest.resourceRequests, rest.cluster, canManageGpu);
data-migrations/migrations/20260819203556-backfill-private-cloud-gpu.js:34
- The migration updates every document in both collections (filter
{}), which can cause unnecessary full-collection writes and longer lock/replication time. Since this is a backfill, it’s safer and faster to target only docs where anyresourceRequests.<env>.gpuis missing or null.
const productResult = await PrivateCloudProduct.updateMany({}, gpuBackfillPipeline, {
session,
});
const requestDataResult = await PrivateCloudRequestData.updateMany({}, gpuBackfillPipeline, {
app/app/api/private-cloud/products/_operations/update.ts:57
- GPU sanitization was added here, but the existing update operation tests don’t assert this behavior. Please add coverage to ensure GPU requests are forced to 0 for non-admin/non-reviewer sessions and/or when the product cluster is not EMERALD/KLAB2 (and preserved when allowed).
const canManageGpu = !!session?.isAdmin || !!session?.permissions.reviewAllPrivateCloudRequests;
rest.resourceRequests = sanitizeGpuResourceRequests(rest.resourceRequests, product.cluster, canManageGpu);
There was a problem hiding this comment.
Optional suggestion: For the function setting the isQuotaUpgrade const, I think it could use the namespaceKeys and resourceKeys from the app/constants/private-cloud.tx file and loop through them to do the comparison. That way this code wouldn't need to be updated if a new resource or namespace is added.
Although this assumes that every resource needs to have this check. Which it currently does.
Maybe something like:
| import { namespaceKeys, resourceKeys } from '@/constants'; | |
| export const isQuotaUpgrade = (oldval: ResourceRequestsEnv, newval: ResourceRequestsEnv) => | |
| namespaceKeys.some((namespace) => | |
| resourceKeys.some((resource) => { | |
| const oldRequest = oldval[namespace][resource] ?? 0; | |
| const newRequest = newval[namespace][resource] ?? 0; | |
| return oldRequest < newRequest; | |
| }), | |
| ); |
There was a problem hiding this comment.
Good suggestion — resourceKeys and namespaceKeys are the source of truth here. I’ll refactor isQuotaUpgrade to iterate over them. thank you!
| if (hasSignificantIncrease) { | ||
| if (gpuDiff > 0) { | ||
| hasIncrease = true; | ||
| hasSignificantIncrease = true; |
There was a problem hiding this comment.
Just checking that any increase should set hasSignificantIncrease to true. It doesn't need a calculation like above?
There was a problem hiding this comment.
Yes, this is intentional. Any GPU quota increase should require manual review, so it is treated as a significant increase regardless of the amount. Unlike CPU/memory/storage, GPU doesn't go through the metrics-based auto-approval calculation.
| options={{ valueAsNumber: true }} | ||
| min={0} | ||
| max={resourceKey === 'cpu' ? 64 : resourceKey === 'memory' ? 128 : 512} | ||
| max={resourceMaxValue[resourceKey]} |
There was a problem hiding this comment.
The 3 page.tsx, create.ts, update.ts and the route.ts files all have a similar line:
const canManageGpu = !!session?.isAdmin || !!session?.permissions.reviewAllPrivateCloudRequests;
Can this logic be moved into a common helper function?
There was a problem hiding this comment.
good point. I'll extract the shared GPU permission check into a helper
| oldval.tools.memory < newval.tools.memory || | ||
| oldval.tools.storage < newval.tools.storage | ||
| oldval.tools.storage < newval.tools.storage || | ||
| (oldval.tools.gpu ?? 0) < (newval.tools.gpu ?? 0) |
There was a problem hiding this comment.
Why is there a ?? check for gpu but not the other fields? If the migration script populates the gpu field is this check needed here? Or alternatively, should the other fields have the check too?
There was a problem hiding this comment.
you are right, the migration backfills gpu for existing records, the ?? 0 fallback is redundant here. I’ll remove it.
b296fb2 to
1c5b6d7
Compare
| ...currentProduct, | ||
| ...getRepositoryFormValues(currentProduct), | ||
| resourceRequests, | ||
| repositories: currentProduct.repositories ?? [], | ||
| isAgMinistry: false, | ||
| isAgMinistryChecked: true, |
| export const isQuotaUpgrade = (oldval: ResourceRequestsEnv, newval: ResourceRequestsEnv) => | ||
| namespaceKeys.some((namespace) => | ||
| resourceKeys.some((resource) => oldval[namespace][resource] < newval[namespace][resource]), | ||
| ); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
data-migrations/migrations/20260819203556-backfill-private-cloud-gpu.js:30
- Both collections are updated with an empty filter ({}), which forces MongoDB to scan/update every document even when all
resourceRequests.*.gpufields already exist. This can significantly increase migration runtime/lock pressure (and makes the surrounding transaction more likely to hit limits). Consider restricting the update to documents missing at least one of the GPU fields.
This issue also appears on line 32 of the same file.
const productResult = await PrivateCloudProduct.updateMany({}, gpuBackfillPipeline, {
session,
});
data-migrations/migrations/20260819203556-backfill-private-cloud-gpu.js:34
- Same as above: using
{}here will scan/update all request-data documents even if GPU is already present. Filtering to documents missing GPU reduces migration work and lowers the chance of long-running transaction issues.
const requestDataResult = await PrivateCloudRequestData.updateMany({}, gpuBackfillPipeline, {
session,
});
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
data-migrations/migrations/20260819203556-backfill-private-cloud-gpu.js:34
- This migration runs
updateManyacross the fullPrivateCloudProductandPrivateCloudRequestDatacollections ({}filter). On large datasets, that can create unnecessary write load and prolong the transaction, even though$ifNullonly changes docs missing/null GPU values. Consider filtering to only documents where at least one of the GPU fields is null/missing so the migration only touches rows that actually need backfilling.
const productResult = await PrivateCloudProduct.updateMany({}, gpuBackfillPipeline, {
session,
});
const requestDataResult = await PrivateCloudRequestData.updateMany({}, gpuBackfillPipeline, {
No description provided.