DRA API: bump maximum size of ReservedFor to 256 #129543
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
kind/api-change
|
/hold This and #129544 both need all necessary approvals before merging either of them. |
k8s-ci-robot
added
the
do-not-merge/hold
|
Thanks @pohly. As mentioned in the description we got preliminary support for this (unprecedented) update from @thockin and @liggitt in https://kubernetes.slack.com/archives/CJUQN3E4T/p1734593174791519 /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: f554f25029d1e3e9f7df66e32cb0154e6d9c9fd1
|
The original limit of 32 seemed sufficient for a single GPU on a node. But for shared non-local resources it is too low. For example, a ResourceClaim might be used to allocate an interconnect channel that connects all pods of a workload running on several different nodes, in which case the number of pods can be considerably larger. 256 is high enough for currently planned systems. If we need something even higher in the future, an alternative approach might be needed to avoid scalability problems. Normally, increasing such a limit would have to be done incrementally over two releases. In this case we decided on Slack (https://kubernetes.slack.com/archives/CJUQN3E4T/p1734593174791519) to make an exception and apply this change to current master for 1.33 and backport it to the next 1.32.x patch release for production usage. This breaks downgrades to a 1.32 release without this change if there are ResourceClaims with a number of consumers > 32 in ReservedFor. In practice, this breakage is very unlikely because there are no workloads yet which need so many consumers and such downgrades to a previous patch release are also unlikely. Downgrades to 1.31 already weren't supported when using DRA v1beta1.
pohly
force-pushed
the
dra-reserved-for-limit
branch
from
800dc29 to
1cee368
Compare
k8s-ci-robot
removed
the
lgtm
|
/triage accepted fixing paperwork |
k8s-ci-robot
added
the
triage/accepted
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
/hold Let me test one slow test more thoroughly which didn't run in the presubmit job... |
k8s-ci-robot
added
the
do-not-merge/hold
|
/retest |
This needs a bit more time. I'll continue tomorrow. |
We want to be sure that the maximum number of pods per claim are actually scheduled concurrently. Previously the test just made sure that they ran eventually. Running 256 pods only works on more than 2 nodes, so network-attached resources have to be used. This is what the increased limit is meant for anyway. Because of the tightened validation of node selectors in 1.32, the E2E test has to use MatchExpressions because they allow listing node names.
k8s-ci-robot
added
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: klueska, pohly, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
area/test
sig/node
|
/hold cancel E2E testing was updated to match the intended usage and passes for me locally with a kind cluster. /test pull-kubernetes-node-e2e-containerd Failed to get scheduled. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
I was a bit confused about which of the updated DRA jobs run "slow" tests. The good news is that the updated "on multiple nodes with network-attached resources supports sharing a claim sequentially" test did run in https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/129543/pull-kubernetes-kind-dra/1877639295167107072 and passed, so this PR should be good to be merged. Whether it should be included a presubmit is a separate discussion - it really is slow at > 12 minutes (partly due to |
|
Thanks @pohly for updating the tests and verifying that they did indeed run in the CI /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 3a8b83e9997db839a19c2287c91aa2afb4aec258
|
|
/test pull-kubernetes-node-e2e-containerd "when querying /stats/summary should report resource usage through the stats api" - unrelated. |
…3-origin-release-1.32 Automated cherry pick of #129543: DRA API: bump maximum size of ReservedFor to 256
8 participants
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
The original limit of 32 seemed sufficient for a single GPU on a node. But for shared non-local resources it is too low. For example, a ResourceClaim might be used to allocate an interconnect channel that connects all pods of a workload running on several different nodes, in which case the number of pods can be considerably larger.
256 is high enough for currently planned systems. If we need something even higher in the future, an alternative approach might be needed to avoid scalability problems.
Which issue(s) this PR fixes:
Required for NVIDIA GB200 and potentially Google TPU use cases.
Special notes for your reviewer:
Normally, increasing such a limit would have to be done incrementally over two releases. In this case we decided on
Slack (https://kubernetes.slack.com/archives/CJUQN3E4T/p1734593174791519) to make an exception and apply this change to current master for 1.33 and backport it to the next 1.32.x patch release for production usage.
This breaks downgrades to a 1.32 release without this change if there are ResourceClaims with a number of consumers > 32 in ReservedFor. In practice, this breakage is very unlikely because there are no workloads yet which need so many consumers and such downgrades to a previous patch release are also unlikely. Downgrades to 1.31 already weren't supported when using DRA v1beta1.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/assign @thockin
/cc @liggitt @johnbelamaric @klueska