feat: Drop the hostname requirement when handling PV topology

[jonathan-innis]

Fixes #N/A

Description

This solves one of the problems called out in: #545

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.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonathan-innis

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jonathan-innis]

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

[coveralls]

Pull Request Test Coverage Report for Build 8105346476

Details

• 19 of 19 (100.0%) changed or added relevant lines in 2 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.01%) to 81.008%

---

Files with Coverage Reduction	New Missed Lines	%
pkg/test/expectations/expectations.go	2	95.96%

Totals
Change from base Build 8102819698:	0.01%
Covered Lines:	8194
Relevant Lines:	10115

---

💛 - Coveralls

[jonathan-innis]

Testing this out: We actually get some interesting behavior here. When the PVC is deleted due to the node cleanup controller, this starts causing Karpenter to think that the capacity that it previously launched should no longer be used because we start ignore the pods due to their PVCs not existing. It's an interesting issue. Not sure that there's a good solve for that additional churn, because we shouldn't respect pods that have NotFound PDBs during our initial scale-out.

[jonathan-innis]

Closing for now, since it's still unclear who the users would be for this feature. May consider re-opening if we get more +1 on the original issue (#545).

[jonathan-innis]

Not sure that there's a good solve for that additional churn, because we shouldn't respect pods that have NotFound PDBs during our initial scale-out

Seems like the best solve for handling this kind of thing would be to just disable consolidationPolicy for nodes that are using PVs like this. Most likely, users that are going to be relying on the node ephemeral storage like this are going to be pretty cautious about their nodes anyways.

The other option here is to just enable the WhenEmpty condition and set the consolidateAfter value to long enough that the StatefulSet controller has enough time to re-hydrate the PVCs that it needs to when the pods go to reschedule.

[jonathan-innis]

The other thing that I'm noting: When I try to run this configuration with the Rancher local path provisioner, I run into some issues when the PVC isn't auto-deleted (or at least when it isn't auto-deleted quickly enough). If the kube-scheduler has enough time to react, it will see that there is a PVC that has a bound PV with a hostname affinity to a node that no longer exists. This causes the kube-scheduler to go lookup the node info and try and bind the pod to the node, which fails with a SchedulerError. That error looks like

status:
  conditions:
  - lastProbeTime: null
    lastTransitionTime: "2024-03-01T01:05:16Z"
    message: nodeinfo not found for node name "ip-192-168-16-127.us-west-2.compute.internal"
    reason: SchedulerError
    status: "False"
    type: PodScheduled

The problem with this is that this doesn't match the v1.PodReasonUnschedulable scheduling reason that Karpenter looks for, which means that Karpenter will stop seeing this node for rescheduling and think that it's provisioned too much capacity for the nodes, causing it to scale-down the node that it just launched.

Yet again, I think the right solve here is to set a long enough consolidateAfter or just to disable consolidateAfter altogether to ensure that the node sticks around long enough for the PVC to get deleted, the new PVC to get created, and the pod to successfully schedule to the new node.

[engedaam]

/lgtm