fix: skip ephemeral volume topology requirements for bound pods during consolidation#2907
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: moko-poi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @moko-poi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@jmdeal Thanks for the analysis on this issue! I wanted to get your thoughts on one thing. In your comment on #2803, you noted
The current implementation skips topology requirements for all ephemeral volumes on bound pods, regardless of VolumeBindingMode: if volume.Ephemeral != nil && pod.Spec.NodeName != "" {
return nil, nil
}For WaitForFirstConsumer, this is correct — the new PVC will bind to whatever zone the pod is scheduled to, so the old PVC's zone constraint is irrelevant. For Immediate, the situation is more nuanced. If we skip the constraint, Karpenter may create a replacement node in a zone that differs from where the provisioner binds the new PVC, which could leave the pod temporarily unschedulable. However, if we don't skip, the old PVC's zone constraint will continue to cause incorrect TSC Given that Immediate + ephemeral volumes is an uncommon combination and skipping is still an improvement over the current behavior (permanently blocked consolidation), I went with the simpler approach for now. That said, if you think Immediate mode warrants additional handling, I'd like to discuss what the right approach would |
9ee5051 to
148ee66
Compare
Summary
Fix for #2803: Ephemeral volumes with zone TopologySpreadConstraints block consolidation with Unconsolidatable events.
Skip ephemeral volume topology requirements when evaluating bound pods for consolidation, since ephemeral PVCs are deleted with the pod and a new PVC will be created on rescheduling. This primarily targets WaitForFirstConsumer StorageClasses, which is the common case for ephemeral volumes. See inline comment for discussion on
Immediate binding mode.
Test plan