Don't deref nil nodegroup in deleteCreatedNodesWithErrors

[bpineau]

Various cloudproviders' NodeGroupForNode() implementations (including aws, azure, and gce) can returns a nil error and a nil nodegroup. For instance we're seeing AWS returning that on failed upscales on live clusters, with a recent cluster-autoscaler build.

So checking that deleteCreatedNodesWithErrors doesn't return an error is not enough to safely dereference the nodegroup (as returned by NodeGroupForNode()) by calling nodegroup.Id().

In that situation, logging and returning early seems the safest option, to give various caches (eg. clusterstateregistry's and cloud provider's) the opportunity to eventually converge, rather than resuming with an inconsistent internal state.

===

With regards to the AWS cloudprovider triggering that issue with recent CA builds:

What we're seeing is:

I0523 16:37:53.847080     228 aws_manager.go:262] Refreshed ASG list, next refresh after 2022-05-23 16:38:53.847076185 +0000 UTC m=+559.468661724
W0523 16:37:53.905286     228 clusterstate.go:594] Nodegroup is nil for aws:///us-east-1b/i-placeholder-redacted-766c0982e71f-2
I0523 16:37:53.905533     228 clusterstate.go:1008] Found 1 instances with errorCode OutOfResource.placeholder-cannot-be-fulfilled in nodeGroup redacted-d63729665fff
I0523 16:37:53.905546     228 clusterstate.go:1026] Failed adding 1 nodes (0 unseen previously) to group redacted-d63729665fff due to OutOfResource.placeholder-cannot-be-fulfilled; errorMessages=[]string{"AWS cannot provision any more instances for this node group"}
[...]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x3639435]
goroutine 63 [running]:
k8s.io/autoscaler/cluster-autoscaler/core.(*StaticAutoscaler).deleteCreatedNodesWithErrors(0xc03e307d00)
        /go/src/k8s.io/autoscaler/cluster-autoscaler/core/static_autoscaler.go:677 +0x275
k8s.io/autoscaler/cluster-autoscaler/core.(*StaticAutoscaler).RunOnce(0xc03e307d00, {0x4, 0x0, 0x7b9b840})
        /go/src/k8s.io/autoscaler/cluster-autoscaler/core/static_autoscaler.go:360 +0x12a5
main.run(0xc00088ae00, {0x4dc8998, 0xc000855230})
        /go/src/k8s.io/autoscaler/cluster-autoscaler/main.go:392 +0x2ad
main.main.func2({0x0, 0x0})
        /go/src/k8s.io/autoscaler/cluster-autoscaler/main.go:479 +0x25
created by k8s.io/autoscaler/cluster-autoscaler/vendor/k8s.io/client-go/tools/leaderelection.(*LeaderElector).Run
        /go/src/k8s.io/autoscaler/cluster-autoscaler/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:211 +0x154

I can't easily reproduce the issue to verify that hypothesis, but it seems likely that this is due to the clusterstateregistry and the AWS caches being out-of-sync. That failure path is new, because flagging "failed at creation" instances was recently introduced for AWS provider (8ac87b3).

We might get the following sequence:

1. RunOnce 1 : an AWS upscale is attempted (but the AWS ASG fails to create an instance)
2. RunOnce 2 : a Refresh() call prompts the AWS cloudprovider to regenerate the instances lists and ASG mappings, including by generating a new fake/placeholder for that failed instance
3. RunOnce 2 : clusterstateregistry gather and caches (for 2mn) the nodes list (which includes the placeholder instance)
4. RunOnce 2 : deleteCreatedNodesWithErrors() is called to gc the failed instance
5. RunOnce 3 : a Refresh() call regenerates the nodes/asg mapping, now there's no dangling placeholder nodes anymore
6. RunOnce 3 : deleteCreatedNodesWithErrors() is called and uses the outdated clusterstateregistry cache, tries to deref a nil nodegroup for a deposed placeholder instance, causes a segfault

/kind bug
/kind regression

[mwielgus]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bpineau, mwielgus

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/OWNERS [mwielgus]

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

[qianlei90]

RunOnce will always returned when nodegroup is nil and CA can not go on until CloudProviderNodeInstancesCache is refreshed, which may up to 2 min.
Should we refresh CloudProviderNodeInstancesCache here to sync up with cloud provider?

@bpineau @mwielgus

[qianlei90]

We might get the following sequence:

1. RunOnce 1 : an AWS upscale is attempted (but the AWS ASG fails to create an instance)
2. RunOnce 2 : a Refresh() call prompts the AWS cloudprovider to regenerate the instances lists and ASG mappings, including by generating a new fake/placeholder for that failed instance
3. RunOnce 2 : clusterstateregistry gather and caches (for 2mn) the nodes list (which includes the placeholder instance)
4. RunOnce 2 : deleteCreatedNodesWithErrors() is called to gc the failed instance
5. RunOnce 3 : a Refresh() call regenerates the nodes/asg mapping, now there's no dangling placeholder nodes anymore
6. RunOnce 3 : deleteCreatedNodesWithErrors() is called and uses the outdated clusterstateregistry cache, tries to deref a nil nodegroup for a deposed placeholder instance, causes a segfault

I think the correct sequence is:

1. RunOnce 1: scale up and failed
2. RunOnce 2: cloudprovider Refresh(), generate a new fake/placeholder for that failed instance
3. RunOnce 2: deleteCreatedNodesWithErrors() is called to gc the failed instance, and the instance cache in clusterstateregistry is removed.
4. Between RunOnce 2 and RunOnce 3: clusterstateregistry refresh instance cache at this time and get the out-of-sync instances from provider. Notice that CloudProviderNodeInstancesCache runs every 2 min to refresh cache.
5. RunOnce 3: a Refresh() call regenerates the nodes/asg mapping, now there's no dangling placeholder nodes anymore
6. RunOnce 3: the outdated clusterstateregistry cache is used and we get a failed instance, then deleteCreatedNodesWithErrors() is called again, tries to deref a nil nodegroup for a deposed placeholder instance, causes a segfault