Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: nodeclaim.spec.minimumPriceImprovementPercent #1454

Closed
wants to merge 1 commit into from
Closed

feat: nodeclaim.spec.minimumPriceImprovementPercent #1454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions kwok/charts/crds/karpenter.sh_nodeclaims.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ spec:
memory leak protection, and disruption testing.
pattern: ^(([0-9]+(s|m|h))+)|(Never)$
type: string
minimumPriceImprovementPercent:
description: |-
MinimumPriceImprovementPercent is the minimum price improvement necessary to disrupt this node, as an integer percentage.
The default is 0%, which maintains the existing consolidation behavior prior to this feature.
format: int32
maximum: 100
minimum: 0
type: integer
nodeClassRef:
description: NodeClassRef is a reference to an object that defines provider specific configuration
properties:
Expand Down
8 changes: 8 additions & 0 deletions pkg/apis/crds/karpenter.sh_nodeclaims.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ spec:
memory leak protection, and disruption testing.
pattern: ^(([0-9]+(s|m|h))+)|(Never)$
type: string
minimumPriceImprovementPercent:
description: |-
MinimumPriceImprovementPercent is the minimum price improvement necessary to disrupt this node, as an integer percentage.
The default is 0%, which maintains the existing consolidation behavior prior to this feature.
format: int32
maximum: 100
minimum: 0
type: integer
nodeClassRef:
description: NodeClassRef is a reference to an object that defines provider specific configuration
properties:
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/v1/nodeclaim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ type NodeClaimSpec struct {
// +kubebuilder:validation:Schemaless
// +optional
ExpireAfter NillableDuration `json:"expireAfter,omitempty"`
// MinimumPriceImprovementPercent is the minimum price improvement necessary to disrupt this node, as an integer percentage.
// The default is 0%, which maintains the existing consolidation behavior prior to this feature.
// +kubebuilder:validation:Minimum:=0
// +kubebuilder:validation:Maximum:=100
// +optional
MinimumPriceImprovementPercent *int32 `json:"minimumPriceImprovementPercent,omitempty"`
Copy link
Contributor

@ellistarn ellistarn Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one makes sense at the NodePool level, rather than the NodeClaim level. This is because the decision to consolidate is at the pool level, unlike expiry/tgp, which make sense outside of the context of a pool (i.e. standalone nodeclaim).

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it in NodeClaim because I think it could be helpful to have the flexibility to set different improvement thresholds for each NodeClaim in a shared NodePool. The existing candidate filtering logic already considers the price of each NodeClaim individually, so there's no added complexity there.

}

// A node selector requirement with min values is a selector that contains values, a key, an operator that relates the key and values
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 22 additions & 3 deletions pkg/controllers/disruption/consolidation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/utils/clock"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/log"

v1 "sigs.k8s.io/karpenter/pkg/apis/v1"
"sigs.k8s.io/karpenter/pkg/cloudprovider"
Expand Down Expand Up @@ -149,7 +150,7 @@ func (c *consolidation) computeConsolidation(ctx context.Context, candidates ...

// get the current node price based on the offering
// fallback if we can't find the specific zonal pricing data
candidatePrice, err := getCandidatePrices(candidates)
candidatePrice, err := getCandidatePrices(ctx, candidates)
if err != nil {
return Command{}, pscheduling.Results{}, fmt.Errorf("getting offering price from candidate node, %w", err)
}
Expand Down Expand Up @@ -285,14 +286,32 @@ func (c *consolidation) computeSpotToSpotConsolidation(ctx context.Context, cand
}

// getCandidatePrices returns the sum of the prices of the given candidates
func getCandidatePrices(candidates []*Candidate) (float64, error) {
func getCandidatePrices(ctx context.Context, candidates []*Candidate) (float64, error) {
var price float64
for _, c := range candidates {
compatibleOfferings := c.instanceType.Offerings.Compatible(scheduling.NewLabelRequirements(c.StateNode.Labels()))
if len(compatibleOfferings) == 0 {
return 0.0, fmt.Errorf("unable to determine offering for %s/%s/%s", c.instanceType.Name, c.capacityType, c.zone)
}
price += compatibleOfferings.Cheapest().Price

// limit maximum candidate replacement price for consideration
originalCheapestPrice := compatibleOfferings.Cheapest().Price
cheapestConsiderablePrice := originalCheapestPrice
if candidates[0].NodeClaim.Spec.MinimumPriceImprovementPercent != nil {
candidateMaxPriceFactor := 1 - (float64(*c.NodeClaim.Spec.MinimumPriceImprovementPercent) / 100)
cheapestConsiderablePrice *= candidateMaxPriceFactor

log.FromContext(ctx).WithValues(
"originalPrice", originalCheapestPrice,
"requiredPrice", cheapestConsiderablePrice,
"minimumPriceImprovementPercent", *c.NodeClaim.Spec.MinimumPriceImprovementPercent,
).Info("price threshold for consolidation")

// con.recorder.Publish(disruptionevents.PriceThreshold(c.Node, c.NodeClaim, fmt.Sprintf("PriceThreshold for Consolidation, original price: %v, required price: %v, price factor: %v%",
// originalCheapestPrice, cheapestConsiderablePrice, *c.NodeClaim.Spec.MinimumPriceImprovementPercent)))
}

price += cheapestConsiderablePrice
}
return price, nil
}
22 changes: 22 additions & 0 deletions pkg/controllers/disruption/events/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,28 @@ func Unconsolidatable(node *corev1.Node, nodeClaim *v1.NodeClaim, reason string)
}
}

// PriceThreshold is an event that notes the maximum price threshold that a NodeClaim/Node requires to allow consolidation
func PriceThreshold(node *corev1.Node, nodeClaim *v1.NodeClaim, reason string) []events.Event {
return []events.Event{
{
InvolvedObject: node,
Type: corev1.EventTypeNormal,
Reason: "PriceThreshold",
Message: reason,
DedupeValues: []string{string(node.UID)},
DedupeTimeout: time.Minute * 15,
},
{
InvolvedObject: nodeClaim,
Type: corev1.EventTypeNormal,
Reason: "PriceThreshold",
Message: reason,
DedupeValues: []string{string(nodeClaim.UID)},
DedupeTimeout: time.Minute * 15,
},
}
}

// Blocked is an event that informs the user that a NodeClaim/Node combination is blocked on deprovisioning
// due to the state of the NodeClaim/Node or due to some state of the pods that are scheduled to the NodeClaim/Node
func Blocked(node *corev1.Node, nodeClaim *v1.NodeClaim, reason string) (evs []events.Event) {
Expand Down
Loading