KEP-5729: Update ResourceClaim Support for Workloads for beta in 1.37 - #5976
Conversation
k8s-ci-robot
added
the
kind/kep
k8s-ci-robot
added
the
sig/scheduling
k8s-ci-robot
added
size/L
823ffd6 to
c2dfe22
Compare
c2dfe22 to
b00c025
Compare
pohly
left a comment
pohly
left a comment
There was a problem hiding this comment.
As documentation for what was in 1.36 this is ready for merging, with the caveat about "implemented".
We were discussing changes the behavior when kube-controller-manager has the feature off (default in 1.36) and is running with an apiserver where it's on (say, on the transition to default enabled). We could include that change in this PR in a second commit, then we only need to review one PR - up to you.
b00c025 to
b0e097a
Compare
Added as a second commit here. |
b0e097a to
a4e5961
Compare
dom4ha
changed the title
|
I updated this to include the required beta criteria in hopes of promoting this for 1.37. /retitle KEP-5729: Update for beta in 1.37 |
k8s-ci-robot
changed the title
|
/retitle KEP-5729: Update ResourceClaim Support for Workloads for beta in 1.37 |
k8s-ci-robot
changed the title
| Generated and standalone ResourceClaims referenced by a PodGroup remain | ||
| unallocated until kube-scheduler allocates the ResourceClaim by setting | ||
| `status.allocation` for the first Pod in the PodGroup that references the | ||
| PodGroup's claim. When a Pod's claim is requested through | ||
| `podGroupResourceClaim`, the ResourceClaim's `status.reservedFor` list will | ||
| PodGroup's claim. When a Pod's claim matches a claim made by its PodGroup, |
There was a problem hiding this comment.
@mm4tt @wojtek-t Is the PodGroup scheduling cycle ready to be plugged into with things like DRA for 1.37? Should we reconsider changing the ResourceClaim from being allocated with the first Pod to being allocated with the PodGroup?
Pros of allocating with the first Pod:
- Resources stay unallocated until a Pod is created which uses the claim. Useful if PodGroups are created with 0 replicas and stay that way for a while.
- That's what we already do :)
Pros of allocating with the PodGroup:
- Eliminate contention between every Pod in the group from making the same update to the same ResourceClaim during binding. Almost certainly more performant and reliable.
- This contention already surfaces a bug being addressed in DRA: scheduler_perf for DRAWorkloadResourceClaims kubernetes#138775.
There was a problem hiding this comment.
Should we reconsider changing the ResourceClaim from being allocated with the first Pod to being allocated with the PodGroup?
I'd treat it as an implementation detail. I assume that we won't change the behavior even after introducing PodGroup level extension points.
In 1.38 we may consider the migration, which should give us support for ResourceClaim in CompositePodGroup for free.
There was a problem hiding this comment.
+1 - there are so many moving bits now that I wouldn't attach it now.
Let's get back to this in 1.38 timeframe an reconsider this decision.
There was a problem hiding this comment.
Does pushing this change to 1.38 delay the beta? ie, should this integration be a beta criteria?
I obviously would like to take this to beta in 1.37 but this is really up to SIG scheduling approvers and I defer to you.
There was a problem hiding this comment.
I'm not sure it has to be. For me this is an implementation detail.
Such change should be gated on its own (so we shouldn't make it as part of GA graduation), but I can imagine this happening as a second beta or even a follow-up.
There was a problem hiding this comment.
There's no guarantee we won't encounter something unexpected the next release, but I can't come up with any obvious problem at the moment, so I'd say that beta promotion is fine.
My current understanding is that PodGroup scheduling is tightly coupled with its pods scheduling. Even a PodGroup with Basic scheduling policy won't be scheduled until the first pod is scheduled. Switching to PodGroup extension points may even not give us any reliability gain, as the PodGroup may be rescheduled when new pods appear, so we may need to face exactly the same edge cases.
So from the user POV, there should be no visible changes.
|
/hold to squash fixups |
k8s-ci-robot
added
the
do-not-merge/hold
| are missing a bunch of machinery and tooling and can't do that now. | ||
| --> | ||
|
|
||
| New automated upgrade/downgrade tests will verify: |
There was a problem hiding this comment.
I started working on a test for this but I'm not sure if it's feasible to implement now since 1.36 serves only the v1alpha2 version of the PodGroup API where 1.37 instead serves v1alpha3 (and eventually v1beta1). Do we have to wait until both the old and new versions serve the same API version?
Is there some other functionality we could test here that doesn't depend on the PodGroup persisting across upgrade/downgrade?
There was a problem hiding this comment.
Let's not bother about automated test for now - manual one with described scenario is probably good enough for now.
There was a problem hiding this comment.
Let's focus on manual for now, but let's describe the scenario you're planning to test in more details, e.g.:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/5710-workload-aware-preemption#were-upgrade-and-rollback-tested-was-the-upgrade-downgrade-upgrade-path-tested
There was a problem hiding this comment.
Sounds good, I'll add more detail here.
There was a problem hiding this comment.
Does the v1alpha2 -> v1alpha3 transition prevent even testing this manually though if a PodGroup can't survive transitions between 1.36 and 1.37?
| Pods are actually consuming the ResourceClaim without the explicit list in | ||
| `status.reservedFor` list and therefore it will not be safe to deallocate. | ||
|
|
||
| - Now the DRA plugin reuses the scheduler's internal view of PodGroups to |
There was a problem hiding this comment.
We are introducing PodGroup level PostFilter in Workload Aware Preemption KEP (beta) and disable pod-by-pod one (See #6106 (comment)).
Shall we make the migration and support deallocation a part of this KEP?
There was a problem hiding this comment.
Yes, I'll update this.
To make sure I'm understanding, the pod-by-pod PostFilter will still run for pods outside PodGroups, but only the PodGroup PostFilter will run for pods in a group? And the DRA plugin's PodGroup PostFilter will do mostly the same things as its per-pod PostFilter, but will go further than only unreserving the claim for the PodGroup and deallocate the ResourceClaim?
There was a problem hiding this comment.
Pushed an update. I moved everything about PostFilter into the "Deallocate" section and added implementing the PodGroup-level PostFilter with deallocation for PodGroup claims.
There was a problem hiding this comment.
To make sure I'm understanding, the pod-by-pod PostFilter will still run for pods outside PodGroups, but only the PodGroup PostFilter will run for pods in a group?
Yes
And the DRA plugin's PodGroup PostFilter will do mostly the same things as its per-pod PostFilter, but will go further than only unreserving the claim for the PodGroup and deallocate the ResourceClaim
I thought that for the initial implementation of DRA PodGroupPostFilter, that we need to do when introducing PodGroupPostFilter extension point, we would do basically the same thing as we do in current post filters. We would gather the unavailableClaims for pods from PodGroup in podGroupCycleState and in PodGroupPostFilter we would randomly deallocate one of them.
My understanding is that as part of this KEP you would also like to extend this logic and deallocate pod group level claims, right?
There was a problem hiding this comment.
I thought that for the initial implementation of DRA PodGroupPostFilter, that we need to do when introducing PodGroupPostFilter extension point, we would do basically the same thing as we do in current post filters. We would gather the unavailableClaims for pods from PodGroup in podGroupCycleState and in PodGroupPostFilter we would randomly deallocate one of them.
We probably need to do it as a part of KEP-5710: Workload-Aware Preemption KEP as the changes needs to be under GenericWorkload feature gate (as it's a beta blocker for Gang Scheduling and WAP), but we can accept your contribution @nojnhuh if you are willing to do so.
My understanding is that as part of this KEP you would also like to extend this logic and deallocate pod group level claims, right?
Yes, deallocation would be in turn part of this KEP beta promotion, as we should have all data on the PodGroup level at the time the PostFilter is called.
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
wojtek-t
left a comment
wojtek-t
left a comment
There was a problem hiding this comment.
Just some minor comments from PRR perspective - other than that LGTM.
|
|
||
| - [X] Feature gate (also fill in values in `kep.yaml`) | ||
| - Feature gate name: WorkloadPodGroupResourceClaimTemplate | ||
| - Feature gate name: DRAWorkloadResourceClaims |
There was a problem hiding this comment.
Since I can't comment on the unchanged line:
Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?
If the kubelet restarts with the feature disabled, existing containers continue to run with all of their allocated devices, including those from claims made by their PodGroup when the feature was enabled.
Can you clarify how that works? Isn't kubelet performing a regular admission of pods after restart, and then (given it no longer understands that the pod is referencing the claim), rejects it?
Are there any tests for feature enablement/disablement?
Were those added? Can you please add links?
There was a problem hiding this comment.
Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?
If the kubelet restarts with the feature disabled, existing containers continue to run with all of their allocated devices, including those from claims made by their PodGroup when the feature was enabled.
Can you clarify how that works? Isn't kubelet performing a regular admission of pods after restart, and then (given it no longer understands that the pod is referencing the claim), rejects it?
I can't find any indication that DRA is involved in (re-)admission. And as long as the Pod sandbox stays intact, the kubelet doesn't check the Pod's ResourceClaims again.
Are there any tests for feature enablement/disablement?
Were those added? Can you please add links?
I have an open PR to add this test in kubernetes/kubernetes#138363: https://github.com/kubernetes/kubernetes/pull/138363/changes#diff-d7f29364819f5cc467bdc62b18a49db19c3f02fdffb8f3d202e7978a2ce03c0d
I'll follow up to add a link here once it's merged.
| are missing a bunch of machinery and tooling and can't do that now. | ||
| --> | ||
|
|
||
| New automated upgrade/downgrade tests will verify: |
There was a problem hiding this comment.
Let's focus on manual for now, but let's describe the scenario you're planning to test in more details, e.g.:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/5710-workload-aware-preemption#were-upgrade-and-rollback-tested-was-the-upgrade-downgrade-upgrade-path-tested
|
/wg device-management |
k8s-ci-robot
added
the
wg/device-management
|
@nojnhuh - the robot is complaining about commit messages, please fix |
aa5f43b to
98a28e3
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
Done /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
I'm fine with that from PRR perspective, but it still requires SIG approval. /approve PRR |
|
@nojnhuh please fix the wording about /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dom4ha, nojnhuh, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
98a28e3 to
6954eac
Compare
dom4ha
left a comment
dom4ha
left a comment
There was a problem hiding this comment.
Suggested to further change the wording, WDYT?
6954eac to
1203516
Compare
|
Thanks @nojnhuh ! |
k8s-ci-robot
added
the
lgtm
10 participants
Uh oh!
There was an error while loading. Please reload this page.