fix: don't crash when vmss not present or has no nodes #7708

comtalyst · 2025-01-16T23:03:44Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Clone of #6672 with an additional fix on printing format.

Testing

One unit test in azure_manager_test.go that acts as an acceptance test.

One e2e test (in-progress)

What this PR does / why we need it:

We currently crash CAS if there are no nodes or if VMSS (Azure NodeGroup implementation) is not present. Ideally, we'd like to avoid crashing in most situations and instead log.

Main problem was in BuildAzure (call path shown below):

cloudprovider.BuildAzure --> klog.Fatalf("Failed to create Azure Manager: %v", err

CreateAzureManager --> returns forceRefresh err

azureManager.forceRefresh --> klog.Errorf("Failed to regenerate Azure cache: %v", err), return err

azureCache.regenerate --> return ng.Nodes() error

scaleSet.Nodes --> klog.Errorf("Failed to get current size for vmss %q: %v", scaleSet.Name, err), return err

scaleSet.getCurSize --> klog.Errorf("failed to get information for VMSS: %s, error: %v", scaleSet.Name, err), return err

getVMSSFromCache --> return compute.VirtualMachineScaleSet{}, fmt.Errorf("could not find vmss: %s", scaleSet.Name)

Main updates include:

updating Nodes() to not error if there are zero nodes, but rather log and return an empty list of instances

updated GetOptions() to not error

updated regenerate to handle instances == nil

Key exceptions found:

TemplateNodeInfo() errors are handled by the core code. Typically, if the node group is not found, core logs a warning and skips the node group. The only exception is for GetNodeFromTemplate(). If TemplateNodeInfo() returns an error, this case is not handled since it's required to spin up a new node.

GetOptions() should not error, because if options are nil, we just use default options. GCE + AWS both return nil error here.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Should be identical to #6672 but with #6672 (comment)

Does this PR introduce a user-facing change?

Yes

- Azure: fixed an issue where CAS crashes when VMSS is not present or has no nodes

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

comtalyst · 2025-01-16T23:09:02Z

/hold

tallaxes · 2025-01-16T23:54:58Z

/lgtm
/approve

k8s-ci-robot · 2025-01-16T23:55:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: comtalyst, tallaxes

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/azure/OWNERS~~ [comtalyst,tallaxes]

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

comtalyst · 2025-01-16T23:58:41Z

/unhold
tests passing

comtalyst · 2025-01-16T23:59:30Z

/cherry-pick cluster-autoscaler-release-1.32
/cherry-pick cluster-autoscaler-release-1.31
/cherry-pick cluster-autoscaler-release-1.30
/cherry-pick cluster-autoscaler-release-1.29
/cherry-pick cluster-autoscaler-release-1.28

k8s-infra-cherrypick-robot · 2025-01-16T23:59:32Z

@comtalyst: once the present PR merges, I will cherry-pick it on top of cluster-autoscaler-release-1.28, cluster-autoscaler-release-1.29, cluster-autoscaler-release-1.30, cluster-autoscaler-release-1.31, cluster-autoscaler-release-1.32 in new PRs and assign them to you.

In response to this:

/cherry-pick cluster-autoscaler-release-1.32
/cherry-pick cluster-autoscaler-release-1.31
/cherry-pick cluster-autoscaler-release-1.30
/cherry-pick cluster-autoscaler-release-1.29
/cherry-pick cluster-autoscaler-release-1.28