fix: skip adding spot requirement during consolidation when no spot offerings are available - #3252
fix: skip adding spot requirement during consolidation when no spot offerings are available#3252bwagner5 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bwagner5 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 |
kubernetes-prow
Bot
added
the
cncf-cla: yes
kubernetes-prow
Bot
added
the
size/L
…fferings are available
7595277 to
b346b0d
Compare
| // are injected on by the scheduler. | ||
| ctReq := results.NewNodeClaims[0].Requirements.Get(v1.CapacityTypeLabelKey) | ||
| if ctReq.Has(v1.CapacityTypeSpot) && ctReq.Has(v1.CapacityTypeOnDemand) { | ||
| if ctReq.Has(v1.CapacityTypeSpot) && ctReq.Has(v1.CapacityTypeOnDemand) && hasSpotOffering(results.NewNodeClaims[0]) { |
There was a problem hiding this comment.
should we also update the block comment above with this change? something like "if Spot is unavailable at consolidation time, skip the Spot requirement and let the replacement launch as OD. The pricing filter picked the instance based on available offerings, so falling back to OD does not regress cost" ? WDYT?
2 participants
Fixes #N/A
Description
This patch fixes consolidation to skip adding a Spot requirement when a NodePool is compatible with both Spot and OD AND there are no spot offerings available. Before this patch, available offerings were not checked and a spot requirement was added in the case where Spot and OD were compatible. This would result in a NodeClaim being created that the provider could not launch to fulfill. The pricing filter in consolidation already checks available offerings so the OD price is used in this case anyways.
This improves consolidation when spot is temporarily unavailable.
For providers that have 1 capacity type (only OD), this change allows NodePools to omit the capacity-type requirement all together which is a nice quality of life improvement. Omitting a capacity-type requirement already works on the provisioning path. It was only assumed in the consolidation path.
How was this change tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.