feat: support pod-level resources (KEP-2837) - #5059
Conversation
Add optional support for Kubernetes pod-level resources (podSpec.resources), gated by submission.podLevelResources (default false). When enabled, a container may omit its own resources if the pod declares a pod-level block, and resource accounting uses the effective request max(sum(container requests), pod-level request). Also filters the pod-level block through the scheduler resource allow-list and reports it in Lookout. Signed-off-by: Matt Landowski <matthew.landowski@apqx.com>
Greptile SummaryAdds feature-gated Kubernetes pod-level resource support across submission, accounting, lease filtering, and Lookout reporting.
Confidence Score: 4/5The PR is not yet safe to merge because lease-time filtering can remove the pod’s only resource budget and per-container minimum validation still rejects valid effective requests. The disabled-accounting and raw pod-level minimum issues are fixed, but two previously reported failures remain: resource allow-list filtering can leave resource-less containers with an empty pod-level block immediately before lease delivery, and successful effective-request validation is still followed by independent per-container minimum checks. Files Needing Attention: internal/scheduler/api.go; internal/server/submit/validation/submit_request.go
|
| Filename | Overview |
|---|---|
| internal/server/submit/validation/submit_request.go | Adds pod-level validation and effective-request minimum checking while retaining existing container validation. |
| internal/server/submit/conversion/post_process.go | Removes pod-level resources before downstream processing when the feature is disabled. |
| internal/scheduler/api.go | Extends lease-time resource allow-list filtering to pod-level requests and limits. |
| pkg/api/util.go | Includes pod-level requests and limits in canonical scheduling resource calculations. |
| internal/common/resource/resource.go | Includes pod-level requests in total pod request accounting. |
| internal/lookoutingester/instructions/instructions.go | Uses the canonical effective scheduling requirements when recording Lookout resource fields. |
Sequence Diagram
sequenceDiagram
participant Client
participant Server
participant Scheduler
participant Executor
participant Lookout
Client->>Server: Submit pod with pod-level resources
Server->>Server: Validate and feature-gate resources
Server->>Scheduler: Publish processed job
Scheduler->>Scheduler: Account effective resource maximum
Scheduler->>Executor: Filter resources and send lease
Server-->>Lookout: Project effective footprint
Reviews (6): Last reviewed commit: "test: make pod-level resource tests tabl..." | Re-trigger Greptile
- clear podSpec.resources in post-processing when the feature is disabled, so disabled pod-level values no longer affect scheduler/Lookout accounting - check MinJobResources against the effective request max(pod-level, container sum) rather than the raw pod-level value, so a pod whose container total meets the minimum is not wrongly rejected Addresses review feedback on armadaproject#5059. Signed-off-by: Matt Landowski <matthew.landowski@apqx.com>
|
Thanks for the review. Addressed in 4157310: 1. Disabled resources still affect accounting — fixed. Added a 2. Minimum checks ignore effective requests — fixed. 3. Filtering can erase the pod budget — this mirrors the existing container-level behaviour: the allow-list intentionally strips floating resources before the pod reaches k8s while the scheduler retains them for accounting. Applying a different rule only to the pod-level block would be inconsistent with how container resources are already handled. Open to a follow-up if you'd prefer a revalidation step, but I kept it consistent with the current container path for now. Tests added for 1 and 2. |
Convert TestTotalResourceRequest_PodLevelResources and TestDropPodLevelResourcesIfDisabled from sequential t.Run blocks to the map-based table style used elsewhere in these packages, and add a case covering an unset pod-level block in the post-processor. Signed-off-by: Matt Landowski <matthew.landowski@apqx.com>
|
@dejanzele how does that look now? |
| "container total meets minimum, low pod-level ok": { | ||
| req: req(&v1.ResourceRequirements{Requests: cpu("2"), Limits: cpu("2")}, container5), | ||
| expectSuccess: true, | ||
| }, |
There was a problem hiding this comment.
Hey I think greptile was correct here. If you define:
container2 := []v1.Container{{Name: "main", Resources: v1.ResourceRequirements{Requests: cpu("2"), Limits: cpu("2")}}}And add test:
"container below minimum covered by pod-level block, accepted": {
req: req(&v1.ResourceRequirements{Requests: cpu("4"), Limits: cpu("4")}, container2),
expectSuccess: true,
},
container 2cpu + pod-level 4cpu -> effective max(2,4)=4cpu >= 4cpu min, but the per-container minimum check only sees the container's raw 2cpu and rejects.
Adds optional support for Kubernetes pod-level resources (
podSpec.resources), gated bysubmission.podLevelResources(default false).When enabled:
TotalPodResourceRequest,SchedulingResourceRequirementsFromPodSpec) uses the effective requestmax(sum(container requests), pod-level request)Default false preserves current behaviour; the accounting branches are inert unless
podSpec.resourcesis set.Requires the executor cluster's apiserver to have the
PodLevelResourcesfeature gate enabled (k8s 1.32 alpha, 1.34+ beta).Tests added for validation and accounting. Opening as draft for feedback on approach.