fix: make sure nodeclaims hostname are also set #2156

rajpopat27 · 2025-04-21T08:20:45Z

Fixes #2123

Description
Make sure karpenter considers nodeclaims while counting Domains

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.

linux-foundation-easycla · 2025-04-21T08:20:48Z

The committers listed above are authorized under a signed CLA.

✅ login: rajpopat27 / name: Raj Popat (db7361e, 88a2680, 6bab332, 3962f1c, 86bd7d1)

k8s-ci-robot · 2025-04-21T08:20:53Z

Welcome @rajpopat27!

It looks like this is your first PR to kubernetes-sigs/karpenter 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/karpenter has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-04-21T08:20:54Z

Hi @rajpopat27. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jonathan-innis · 2025-04-21T16:59:35Z

/assign @jonathan-innis

jonathan-innis · 2025-04-21T18:40:51Z

pkg/controllers/provisioning/scheduling/topology.go

 		// ignore the node if it doesn't match the topology group
-		if !tg.nodeFilter.Matches(n.Node.Spec.Taints, scheduling.NewLabelRequirements(n.Node.Labels)) {
+		if !tg.nodeFilter.Matches(n.Taints(), scheduling.NewLabelRequirements(n.Labels())) {


n.Labels() doesn't have hostname hydrated into it if it still hasn't resolved to a Node -- we should consider injecting hostname into the labels, which would make this code work?

@jonathan-innis The nodeclaim get that kubernetes.io/hostname labels once it is launched

karpenter/pkg/controllers/nodeclaim/lifecycle/launch.go

Line 76 in fc274d1

created, err := l.cloudProvider.Create(ctx, nodeClaim)

karpenter/pkg/controllers/nodeclaim/lifecycle/launch.go

Line 70 in fc274d1

nodeClaim = PopulateNodeClaimDetails(nodeClaim, created)

And the only the nodeclaim that are launched will appear in state nodes

karpenter/pkg/controllers/state/cluster.go

Line 138 in fc274d1

if providerID == "" {

and in the countDomains we are only checking nodes that are in state nodes so I guess should be fine

The hostname does not get propagated on launch -- for instance, on AWS, this change doesn't work because we don't see the hostname until the node actually joins the cluster so the same bug is present.

I'm suspecting you might be running into this because our KWOK cloudprovider doesn't model this quite right today

Oh I see okay let me make the changes for this

k8s-ci-robot · 2025-04-28T08:40:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rajpopat27
Once this PR has been reviewed and has the lgtm label, please ask for approval from jonathan-innis. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coveralls · 2025-04-28T09:21:35Z

Pull Request Test Coverage Report for Build 15017125737

Details

38 of 45 (84.44%) changed or added relevant lines in 3 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.03%) to 81.923%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/test/expectations/expectations.go	20	27	74.07%

Files with Coverage Reduction	New Missed Lines	%
pkg/controllers/disruption/consolidation.go	4	85.55%

Totals
Change from base Build 15008130753:	0.03%
Covered Lines:	10138
Relevant Lines:	12375

💛 - Coveralls

…g logic

jonathan-innis · 2025-05-27T06:00:40Z

pkg/controllers/provisioning/scheduling/topology.go

+
+		// We special-case kubernetes.io/hostname primarily for stateNodes having NodeClaims without a node
+		//  and since their domain won't be registered until the node joins the cluster.
+		if !exists && tg.Key == corev1.LabelHostname {


I'm confused why we need to fake the value in this code here -- why don't we just fake the value at the StateNode and then we don't have to special-case code in this scheduling section. I think all this fix amounts to is passing the same value that n.HostName() has back through n.Labels(). Seems like that would simplify a lot of this and you wouldn't have to make any changes to this section of code

Currently, the NodeClaim name, which is used as the hostname in topologies, is a generated value created after the p.Create method provisions the NodeClaim using kubeClient.

Would you recommend issuing a patch request to update the NodeClaim's labels by setting the hostname label to match the NodeClaim name? If not, could you suggest a more appropriate stage or mechanism for assigning the hostname label based on the NodeClaim name?

An alternative approach I considered is to incorporate this logic into the c.UpdateNodeClaim method—specifically, to check whether the NodeClaim lacks a hostname label and has an empty hostname value, and if so, to assign the hostname label accordingly.

jonathan-innis · 2025-05-27T06:03:33Z

pkg/test/expectations/expectations.go

@@ -306,6 +306,39 @@ func ExpectProvisioned(ctx context.Context, c client.Client, cluster *state.Clus
 	return bindings
 }

+//nolint:gocyclo
+func ExpectProvisionedNoNode(ctx context.Context, c client.Client, cluster *state.Cluster, cloudProvider cloudprovider.CloudProvider, provisioner *provisioning.Provisioner, pods ...*corev1.Pod) ([]*v1.NodeClaim, error) {


nit: Why return an error here when you can just enforce the lack of errors through expectations?

jonathan-innis · 2025-05-27T06:03:55Z

pkg/test/expectations/expectations.go

+//nolint:gocyclo
+func ExpectProvisionedNoNode(ctx context.Context, c client.Client, cluster *state.Cluster, cloudProvider cloudprovider.CloudProvider, provisioner *provisioning.Provisioner, pods ...*corev1.Pod) ([]*v1.NodeClaim, error) {
+	GinkgoHelper()
+	nodeClaims := []*v1.NodeClaim{}


Suggested change

nodeClaims := []*v1.NodeClaim{}

var nodeClaims []*v1.NodeClaim

nit: style

jonathan-innis · 2025-05-27T06:04:37Z

pkg/test/expectations/expectations.go

@@ -306,6 +306,39 @@ func ExpectProvisioned(ctx context.Context, c client.Client, cluster *state.Clus
 	return bindings
 }

+//nolint:gocyclo
+func ExpectProvisionedNoNode(ctx context.Context, c client.Client, cluster *state.Cluster, cloudProvider cloudprovider.CloudProvider, provisioner *provisioning.Provisioner, pods ...*corev1.Pod) ([]*v1.NodeClaim, error) {


Since this function is a subset of the other function, can you just use this function in ExepctProvisionedNoBinding()?

jonathan-innis · 2025-05-27T06:05:06Z

pkg/cloudprovider/fake/cloudprovider.go

@@ -175,6 +175,13 @@ func (c *CloudProvider) Create(ctx context.Context, nodeClaim *v1.NodeClaim) (*v
 		labels[req.Key] = req.Any()
 	}

+	// add the derived nodeclaim requirement labels as we do in addInstanceLabels in KWOK provider
+	for _, r := range nodeClaim.Spec.Requirements {


Why specifically do you need this in your change if your change has to do with hostname specifically?

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2025

k8s-ci-robot requested review from engedaam and tallaxes April 21, 2025 08:20

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 21, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 21, 2025

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 21, 2025

k8s-ci-robot assigned jonathan-innis Apr 21, 2025

jonathan-innis mentioned this pull request Apr 21, 2025

Karpenter scaling unproportionately high nodes #2123

Open

jonathan-innis reviewed Apr 21, 2025

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 28, 2025

rajpopat27 force-pushed the fix-2123 branch from d35f234 to 44b1cf0 Compare April 28, 2025 08:46

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 28, 2025

rajpopat27 changed the title ~~make sure nodeclaims hostname are also set~~ fix: make sure nodeclaims hostname are also set Apr 28, 2025

rajpopat27 marked this pull request as ready for review April 28, 2025 09:28

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2025

rajpopat27 force-pushed the fix-2123 branch from bdf2bcb to ec09ad8 Compare May 10, 2025 09:09

rajpopat27 added 5 commits May 14, 2025 14:54

make sure nodeclaims hostname are also set

db7361e

added tests

88a2680

change nodepool name

6bab332

lint fixes

86bd7d1

refactor: remove hostname label assignment and enhance domain matchin…

3962f1c

…g logic

rajpopat27 force-pushed the fix-2123 branch from ec09ad8 to 3962f1c Compare May 14, 2025 09:24

matthchr mentioned this pull request May 23, 2025

Overscaling when soft anti-affinity is used with hostname Azure/karpenter-provider-azure#906

Open

jonathan-innis reviewed May 27, 2025

View reviewed changes

	nodeClaims := []*v1.NodeClaim{}
	var nodeClaims []*v1.NodeClaim

fix: make sure nodeclaims hostname are also set #2156

Are you sure you want to change the base?

fix: make sure nodeclaims hostname are also set #2156

Conversation

rajpopat27 commented Apr 21, 2025

Uh oh!

linux-foundation-easycla bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 21, 2025

Uh oh!

k8s-ci-robot commented Apr 21, 2025

Uh oh!

jonathan-innis commented Apr 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 28, 2025

Uh oh!

coveralls commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15017125737

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Apr 21, 2025 •

edited

Loading

coveralls commented Apr 28, 2025 •

edited

Loading