🌱 improve getNodeRefMap

[cahillsf]

What this PR does / why we need it:
improves efficiency for how the MP controller retrieves nodes

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #8856

/area machinepool

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vincepri for approval. For more information see the Kubernetes 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

[cahillsf]

not sure if we want to incorporate this check for more than one node matching a providerID:

cluster-api/internal/controllers/machine/machine_controller_noderef.go

Lines 223 to 225 in f62294e

	if len(nodeList.Items) != 1 {
	return nil, fmt.Errorf("unexpectedly found more than one Node matching the providerID %s", providerID)
	}

the full list approach does not catch this since it's just looping over the full list and adding entries in the map, so it would just be "last seen wins"

[cahillsf]

/test pull-cluster-api-test-main

[cahillsf]

we fall back to retrieving the whole list when len(providerIDList) == 0 because createOrUpdateMachines will pull the providerID from the infraMachines:

cluster-api/exp/internal/controllers/machinepool_controller_phases.go

Lines 417 to 424 in c62c864

	var providerID string
	var node *corev1.Node
	if err := util.UnstructuredUnmarshalField(infraMachine, &providerID, "spec", "providerID"); err != nil {
	log.V(4).Info("could not retrieve providerID for infraMachine", "infraMachine", klog.KObj(infraMachine))
	} else {
	// Retrieve the Node for the infraMachine from the nodeRefsMap using the providerID.
	node = s.nodeRefMap[providerID]
	}

prior to the providerID list in the machinePool.spec getting set from the infraConfig:

cluster-api/exp/internal/controllers/machinepool_controller_phases.go

Lines 327 to 328 in c62c864

	if !reflect.DeepEqual(mp.Spec.ProviderIDList, providerIDList) {
	mp.Spec.ProviderIDList = providerIDList

[cahillsf]

unit tests such as this one:

cluster-api/exp/internal/controllers/machinepool_controller_phases_test.go

Line 1958 in c62c864

t.Run("Should set `Running` when scaled from zero to one", func(t *testing.T) {

depend on this behavior

[cahillsf]

/test pull-cluster-api-e2e-main

looks like the same flake as on main: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/11071/pull-cluster-api-e2e-main/1826605867118628864

[cahillsf]

/test pull-cluster-api-e2e-main

[sbueringer]

I'm wondering if it's really more efficient to do the list call for every single Node vs. doing one list call (e.g. are 100 Node List calls with field selector better then just 1 without?)

(not sure in which way it might matter, but let's consider that both cases are just hitting the local cache)

[cahillsf]

hm yeah i guess i took this for granted since the issue had been accepted and marked as help wanted

any thoughts on how i could go about proving out this efficiency improvement (or lack thereof 😄 )? or you think it's not worth the effort?

[sbueringer]

Yup sorry I didn't realize this on the issue. It's probably not worth the effort to be honest

[cahillsf]

sure thing, fine with me will close both the PR and the issue then 👍

[sbueringer]

Thx for working on it!!