Feat: Can override machine request and limit independently for kubernetes #2469
Conversation
|
WalkthroughAdds optional cpuRequest and memoryRequest fields to MachinePreset and machine override schemas. Updates Kubernetes resource request calculations in kubernetes-provider and supervisor to use these overrides when provided, falling back to previous defaults (CPU: cpu*0.75; Memory: memory) and formatting as strings with “G” for memory. Webapp validation (Zod) now accepts these override fields. Documentation updated to describe per-machine Kubernetes “requests,” include warnings about zero values, and provide a JSON example. No public API signatures change beyond the expanded MachinePreset schema types. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/v3/schemas/common.ts (1)
117-126: Validate cpuRequest/memoryRequest at the schema layer (non-negative and ≤ limits)Prevents K8s admission errors when requests exceed limits and guards against negative values.
-export const MachinePreset = z.object({ - name: MachinePresetName, - /** unit: vCPU */ - cpu: z.number(), - cpuRequest: z.number().optional(), // Only used for k8s fallback to cpu - /** unit: GB */ - memory: z.number(), - memoryRequest: z.number().optional(), // Only used for k8s fallback to memory - centsPerMs: z.number(), -}); +export const MachinePreset = z + .object({ + name: MachinePresetName, + /** unit: vCPU */ + cpu: z.number(), + // Used for Kubernetes resource requests when provided; runtime falls back if undefined + cpuRequest: z.number().nonnegative().optional(), + /** unit: GB */ + memory: z.number(), + // Used for Kubernetes resource requests when provided; runtime falls back if undefined + memoryRequest: z.number().nonnegative().optional(), + centsPerMs: z.number(), + }) + .refine((p) => p.cpuRequest === undefined || p.cpuRequest <= p.cpu, { + path: ["cpuRequest"], + message: "cpuRequest must be <= cpu", + }) + .refine((p) => p.memoryRequest === undefined || p.memoryRequest <= p.memory, { + path: ["memoryRequest"], + message: "memoryRequest must be <= memory", + });
🧹 Nitpick comments (3)
docs/self-hosting/overview.mdx (1)
94-104: Clarify defaults and enforce "requests ≤ limits" guidanceAdd explicit defaults and the limit constraint to prevent misconfiguration.
-You can also set cpu/memory requests for each machine type. This is used in k8s environments to set the minimum resources required for each machine type. -We do not recommend setting these values to zero, as it can cause the k8s scheduler to schedule an infinite number of pods in parallel and cause the pods to get killed due to resource exhaustion. +You can also set cpu/memory requests for each machine type. This is used in k8s environments to set the minimum resources required for each machine type. If omitted, CPU request defaults to 75% of the machine's cpu, and memory request defaults to the machine's memory. +Requests must be less than or equal to their corresponding limits (cpu and memory); otherwise, the pod will be rejected by Kubernetes. We do not recommend setting these values to zero, as it can lead to aggressive over‑scheduling and OOM evictions.apps/supervisor/src/workloadManager/kubernetes.ts (1)
297-302: Deduplicate resource-request logic with kubernetes-providerThe same calculation exists in two places and is easy to drift. Consider a shared utility in a common package (e.g., @trigger.dev/core/v3/serverOnly) to format requests/limits from MachinePreset.
apps/webapp/app/services/platform.v3.server.ts (1)
81-86: Basic validation: requests should be non-negative; clarify commentsLocal validation reduces bad configs before they reach core/provider.
const MachineOverrideValues = z.object({ cpu: z.number(), - cpuRequest: z.number().optional(), // Only used for k8s fallback to cpu + // Used for Kubernetes resource requests when provided + cpuRequest: z.number().nonnegative().optional(), memory: z.number(), - memoryRequest: z.number().optional(), // Only used for k8s fallback to memory + // Used for Kubernetes resource requests when provided + memoryRequest: z.number().nonnegative().optional(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/kubernetes-provider/src/index.ts(1 hunks)apps/supervisor/src/workloadManager/kubernetes.ts(1 hunks)apps/webapp/app/services/platform.v3.server.ts(1 hunks)docs/self-hosting/overview.mdx(1 hunks)packages/core/src/v3/schemas/common.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/core/src/v3/schemas/common.tsapps/kubernetes-provider/src/index.tsapps/supervisor/src/workloadManager/kubernetes.tsapps/webapp/app/services/platform.v3.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/schemas/common.tsapps/webapp/app/services/platform.v3.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/services/platform.v3.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/services/platform.v3.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/services/platform.v3.server.ts
🧠 Learnings (1)
📚 Learning: 2025-01-13T18:31:48.160Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1608
File: apps/webapp/app/v3/services/triggerTask.server.ts:418-418
Timestamp: 2025-01-13T18:31:48.160Z
Learning: The `MachinePresetName` schema is used to validate machine preset values in the trigger.dev codebase, ensuring type safety and validation of machine preset options.
Applied to files:
packages/core/src/v3/schemas/common.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
| #getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities { | ||
| return { | ||
| cpu: `${preset.cpu * 0.75}`, | ||
| memory: `${preset.memory}G`, | ||
| cpu: `${preset.cpuRequest ?? preset.cpu * 0.75}`, | ||
| memory: `${preset.memoryRequest ?? preset.memory}G`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp requests to limits (mirror supervisor change)
Prevents admission failures when overrides set requests above limits.
#getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities {
- return {
- cpu: `${preset.cpuRequest ?? preset.cpu * 0.75}`,
- memory: `${preset.memoryRequest ?? preset.memory}G`,
- };
+ const cpuReq = Math.min(Math.max(0, preset.cpuRequest ?? preset.cpu * 0.75), preset.cpu);
+ const memReq = Math.min(Math.max(0, preset.memoryRequest ?? preset.memory), preset.memory);
+ return {
+ cpu: `${cpuReq}`,
+ memory: `${memReq}G`,
+ };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities { | |
| return { | |
| cpu: `${preset.cpu * 0.75}`, | |
| memory: `${preset.memory}G`, | |
| cpu: `${preset.cpuRequest ?? preset.cpu * 0.75}`, | |
| memory: `${preset.memoryRequest ?? preset.memory}G`, | |
| }; | |
| } | |
| #getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities { | |
| const cpuReq = Math.min( | |
| Math.max(0, preset.cpuRequest ?? preset.cpu * 0.75), | |
| preset.cpu | |
| ); | |
| const memReq = Math.min( | |
| Math.max(0, preset.memoryRequest ?? preset.memory), | |
| preset.memory | |
| ); | |
| return { | |
| cpu: `${cpuReq}`, | |
| memory: `${memReq}G`, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/kubernetes-provider/src/index.ts around lines 546 to 551, the method
computing resource requests currently returns values that may exceed preset
limits; update it to clamp the computed cpu and memory requests to the
corresponding limits (e.g. cpuLimit and memoryLimit) so requests never exceed
limits. Compute cpuReq = min(preset.cpuRequest ?? preset.cpu * 0.75,
preset.cpuLimit ?? Infinity) and memoryReq = min(numeric(preset.memoryRequest ??
preset.memory), preset.memoryLimit ?? Infinity) then format cpu as string and
memory with trailing "G"; return those clamped values.
| #getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities { | ||
| return { | ||
| cpu: `${preset.cpu * 0.75}`, | ||
| memory: `${preset.memory}G`, | ||
| cpu: `${preset.cpuRequest ?? preset.cpu * 0.75}`, | ||
| memory: `${preset.memoryRequest ?? preset.memory}G`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp requests to limits to avoid K8s 400s on misconfiguration
Even with schema checks, defensive clamping avoids failed pod creations when overrides drift or come from older configs.
#getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities {
- return {
- cpu: `${preset.cpuRequest ?? preset.cpu * 0.75}`,
- memory: `${preset.memoryRequest ?? preset.memory}G`,
- };
+ const cpuReq = Math.min(Math.max(0, preset.cpuRequest ?? preset.cpu * 0.75), preset.cpu);
+ const memReq = Math.min(Math.max(0, preset.memoryRequest ?? preset.memory), preset.memory);
+ return {
+ cpu: `${cpuReq}`,
+ memory: `${memReq}G`,
+ };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities { | |
| return { | |
| cpu: `${preset.cpu * 0.75}`, | |
| memory: `${preset.memory}G`, | |
| cpu: `${preset.cpuRequest ?? preset.cpu * 0.75}`, | |
| memory: `${preset.memoryRequest ?? preset.memory}G`, | |
| }; | |
| } | |
| #getResourceRequestsForMachine(preset: MachinePreset): ResourceQuantities { | |
| const cpuReq = Math.min(Math.max(0, preset.cpuRequest ?? preset.cpu * 0.75), preset.cpu); | |
| const memReq = Math.min(Math.max(0, preset.memoryRequest ?? preset.memory), preset.memory); | |
| return { | |
| cpu: `${cpuReq}`, | |
| memory: `${memReq}G`, | |
| }; | |
| } |
✅ Checklist
Testing
I deployed a built version of the webapp, supervisor into a public registry and tried it in my cluster.
Webapp:
public.ecr.aws/n5q7l0s4/trigger-supervisor:latestSupervisor:
public.ecr.aws/n5q7l0s4/trigger-webapp:latestChangelog
You can now add memory/cpu request specification to the machine override which allows you ton control request and limit independently for kubernetes.
💯