Cloud provider[Packet] fixes

[ipochi]

This PR address certain issues found when deploying Cluster Autoscaler on Equinix Metal, formerly Packet.

1. fix panic on entry in nil map

   In the latest version of cluster-autoscaler (cloudprovider: packet), the
   code panics and the pods go into CrashLoopBackoff due to an entry
   assignment on a nil map.

2. Consider the string prefix equinixmetal://

   When K8s API is queried to get providerID from Node Spec, some machines
   return packet://<uuid>, whereas some return equinixmetal://, this
   creates error as the string is not trimmed properly and hence results in
   a 404 when an untrimmed string is queried to Equinix Metal API for
   device information.

3. Make controller node label configurable

   Currently the label to identify controller/master node is hard coded to
   node-role.kubernetes.io/master.

   There have been some conversations centered around replacing the label
   with node-role.kubernetes.io/control-plane.

   In Lokomotive, the label to identify
   the controller/master node is node.kubernetes.io/master, the reasons
   for this is mentioned in this issue

   Make the label configurable by setting an env variable in
   the deployment CONTROLLER_NODE_IDENTIFIER_LABEL, if set then the value
   in the env variable is used for identifying controller/master nodes, if
   not set/passed, then the existing behavior is followed choosing the
   existing label.

[detiber]

If I'm not mistaken, wouldn't this cause the autoscaler to think that any Nodes w/ the 'packet://' provider id prefix to be unregistered (and as a result, eventually deleted)?

autoscaler/cluster-autoscaler/clusterstate/clusterstate.go

Lines 295 to 329 in c64e975

	// UpdateNodes updates the state of the nodes in the ClusterStateRegistry and recalculates the stats
	func (csr *ClusterStateRegistry) UpdateNodes(nodes []*apiv1.Node, nodeInfosForGroups map[string]*schedulerframework.NodeInfo, currentTime time.Time) error {
	csr.updateNodeGroupMetrics()
	targetSizes, err := getTargetSizes(csr.cloudProvider)
	if err != nil {
	return err
	}
	cloudProviderNodeInstances, err := csr.getCloudProviderNodeInstances()
	if err != nil {
	return err
	}
	notRegistered := getNotRegisteredNodes(nodes, cloudProviderNodeInstances, currentTime)
	csr.Lock()
	defer csr.Unlock()
	csr.nodes = nodes
	csr.nodeInfosForGroups = nodeInfosForGroups
	csr.previousCloudProviderNodeInstances = csr.cloudProviderNodeInstances
	csr.cloudProviderNodeInstances = cloudProviderNodeInstances
	csr.updateUnregisteredNodes(notRegistered)
	csr.updateReadinessStats(currentTime)
	// update acceptable ranges based on requests from last loop and targetSizes
	// updateScaleRequests relies on acceptableRanges being up to date
	csr.updateAcceptableRanges(targetSizes)
	csr.updateScaleRequests(currentTime)
	csr.handleInstanceCreationErrors(currentTime)
	// recalculate acceptable ranges after removing timed out requests
	csr.updateAcceptableRanges(targetSizes)
	csr.updateIncorrectNodeGroupSizes(currentTime)
	return nil
	}

I've hit similar complications around the Node provider IDs with cluster-api-provider-packet around migration from an older version of the packet-ccm to a newer version of cloud-provider-equinix-metal, and the migration process can be a bit messy, especially since the ProviderID field on the Node is immutable.

There are probably a couple of options available:

• Switch everything to use the new equinixmetal:;// provider id prefix and require using a newer version of the cloud-provider
• Make the prefix configurable, so that it can work with either prefix, but the user must configure it correctly

Either way, we should probably document some type of migration strategy, since I'm not sure there is a way to stay flexible without changes to the core autoscaling code, or forcing node/device churn in the cluster.

For cluster-api-provider-packet, the current plan is to do something along the lines of:

• Update packet-ccm to cloud-provider-equinix-metal
• Upgrade the cluster api provider to a version that uses the new provider id prefix
• Delete and Recreate the Node resources to change the providerID.

A similar approach should be possible with the autoscaler, thought it might be better to disable the autoscaler during the upgrade process to avoid any potential of confusion that could cause workload disruption during the node deletion/recreation process.

[invidian]

Good point @detiber. Thanks for the feedback :)

@ipochi as far as I understand, it is not possible to get mixed packet:// and equnixmetal:// on a single cluster, right? Those are set by the CCM, which will either set it to one or another, or? If that's the case, we should indeed make it configurable.

[ipochi]

@detiber Thanks for your valuable feedback. I think I misunderstood about the importance of packet:// in

nodes = append(nodes, fmt.Sprintf("packet://%s", d.ID))

Also @invidian @detiber

Its is quite possible to get mixed packet:// and equinixmetal:// on a single cluster. If ta cluster is created with packet-ccm and then packet-ccm is upgraded to cloud-provider-equinix-metal, then any new nodes created will have equinixmetal://, whilst the old nodes would still continue to have packet://

Example:

$ kubectl get nodes
NAME                         STATUS   ROLES    AGE     VERSION
theta-anotherpool-worker-0   Ready    <none>   9m29s   v1.21.1 - After upgrade to cloud-provider-equinix-metal
theta-controller-0           Ready    <none>   86m     v1.21.1 - Before upgrade
theta-small-worker-0         Ready    <none>   86m     v1.21.1 - Before upgrade

$ kubectl get nodes -o jsonpath='{.items[*].spec.providerID}'
equinixmetal://536e117a-6d6e-4d9c-b502-f67170662aef packet://b7057a83-732e-4ae6-b5a7-d28516556ba6 packet://4815edc6-b0e2-4bdb-bbb0-daca73e72c1a⏎

[invidian]

Its is quite possible to get mixed packet:// and equinixmetal:// on a single cluster. If ta cluster is created with packet-ccm and then packet-ccm is upgraded to cloud-provider-equinix-metal, then any new nodes created will have equinixmetal://, whilst the old nodes would still continue to have packet://

Yes, but this is the situation which can be gated. You can disable the autoscaler, do CCM -> CCEM upgrade, re-register all nodes, then upgrade the autoscaler I guess.

[ipochi]

The scenario I described above didn't involve autoscaler.

Merely upgrading from CCM TO CCEM and adding a node to the cluster.

Can deregistering and registering a node be done in case of a single controller node ?

[invidian]

The scenario I described above didn't involve autoscaler.

Merely upgrading from CCM TO CCEM and adding a node to the cluster.

It doesn't matter who adds new node :) But the autoscaler can do it unattended.

Can deregistering and registering a node be done in case of a single controller node ?

That seem Lokomotive specific question and I don't know the answer 😄

[ipochi]

Q: We should't worry about controller nodes if I understand correctly ?

[invidian]

Assuming they will be correctly skipped, yes. Good point!

[invidian]

Good point @detiber. Thanks for the feedback :)

@ipochi as far as I understand, it is not possible to get mixed packet:// and equnixmetal:// on a single cluster, right? Those are set by the CCM, which will either set it to one or another, or? If that's the case, we should indeed make it configurable.

[ipochi]

@invidian addressed your feedback.

[detiber]

/lgtm
/approve

Since the autoscaler is versioned in lock step with kubernetes minor versions, we'll need to also backport this to the appropriate release branches as well.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, ipochi

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:

• cluster-autoscaler/cloudprovider/packet/OWNERS [detiber]

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

[invidian]

Ditto PACKET_ prefix + env variable name could be a constant :)

[invidian]

@ipochi can branch be removed now?

[ipochi]

/lgtm
/approve

Since the autoscaler is versioned in lock step with kubernetes minor versions, we'll need to also backport this to the appropriate release branches as well.

@detiber

Can you please provide guidance on how to backport this to the release branches and till which autoscaler release ?

[detiber]

@detiber

Can you please provide guidance on how to backport this to the release branches and till which autoscaler release ?

You should be able to cherry-pick the commit and create a PR against the cluster-autoscaler-release-* branches that are appropriate.

I don't see a need to backport to anything that isn't currently supported, so I think 1.19, 1.20, 1.21, and 1.22 are probably the appropriate branches we'd want to backport to.

You might be able to leverage the "cherry-pick script" from the kubernetes release team: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md, but since I don't do it frequently, I generally just do it manually:

git checkout -b backport-1.22 upstream/cluster-autoscaler-release-1.22 # assuming the upstream k/autoscaler repo is configured as the remote 'upstream'
git cherry-pick <sha for commit 1> <sha for commit 2> ...

And repeating for the other branches.

#4260 is an example of a manual cherry-pick PR
#4269 is an example of an automated cherry-pick PR

[ipochi]

@detiber
Created the necessary backporting PRs