Consolidation with spot by default is not appropriate

[leoryu]

Description

Observed Behavior:

From :

karpenter/pkg/controllers/disruption/consolidation.go

Lines 192 to 205 in 372b9c8

	// We are consolidating a node from OD -> [OD,Spot] but have filtered the instance types by cost based on the
	// assumption, that the spot variant will launch. We also need to add a requirement to the node to ensure that if
	// spot capacity is insufficient we don't replace the node with a more expensive on-demand node. Instead the launch
	// should fail and we'll just leave the node alone.
	ctReq := results.NewNodeClaims[0].Requirements.Get(v1.CapacityTypeLabelKey)
	if ctReq.Has(v1.CapacityTypeSpot) && ctReq.Has(v1.CapacityTypeOnDemand) {
	results.NewNodeClaims[0].Requirements.Add(scheduling.NewRequirement(v1.CapacityTypeLabelKey, corev1.NodeSelectorOpIn, v1.CapacityTypeSpot))
	}
	return Command{
	candidates: candidates,
	replacements: results.NewNodeClaims,
	}, results, nil
	}

Karpenter will set nodeclaim's CapacityType with spot if reqs allow CapacityType with [OD, spot].

This logic will let karpenter always creates spot machine, even the cheapest machine is OD but not the spot one.

And the worst case is that there is no spot machine avaliabel, the karpenter wiill report err:

logger":"controller","message":"failed launching nodeclaim","controller":"nodeclaim.lifecycle","controllerGroup":"karpenter.sh","controllerKind":"NodeClaim","NodeClaim": ...

Since the created nodeclaim has reqs with 'spot', the consolidation will not sucess even we have cheaper OD machine.

Expected Behavior:

What I expected in consolidation is that:

Do not modify the CapacityType, just choose the chpeast machine if my reqs has ignore the capacity type.

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

[k8s-ci-robot]

This issue is currently awaiting triage.

If Karpenter contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions 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.

[leoryu]

@njtran Hi, I found the code is commit by you 2 yeases ago. Could you explain why nodeclaim is always spot in consolidation? Since the real world spot machine might not be available, I think karpenter should choose the cheapeast one, even the machine is on-demand.