azure vmss cache fixes and improvements

[marwanad]

With the merge of #3717, we lost the optimistic in memory cache we've added. The PR stopped bubbling down the sizeRefreshPeriod which was then removed in #4541 because it was unused.

The realization is that we still need this back for the following scenario:

1. Assume a cache TTL of 60s, minCount = 2, currentTarget = 3
2. Two nodes are eligible for deletion by autoscaler, minCount=2 and because CA core won't do the min count check for us in empty node removal.
3. First node gets removed, we decrement scale.Set.CurSize to 2
4. Second call toscaleSet.DeleteNodes() comes in and this calls into GetScaleSetSize which in turn ends up reading from manager.cache which would have a cached count of 3 and thus will return you 3 so you end up deleting that node as well

With this PR, we'll extend the last refresh time by sizeRefreshPeriod to ensure that next time we expire, manager.cache would've had the chance to refresh and give us fresh data. That's basically the behaviour prior to #3717. See in 1.19.

The PR also cleans up the logging to refer to "in-memory size" vs the one we get back from the manager cache.

This will impact 1.21+.

/area cloudprovider/azure

[k8s-ci-robot]

@marwanad: The label(s) area/cloudprovider/azure cannot be applied, because the repository doesn't have them.

In response to this:

With the merge of #3717, we lost the optimistic in memory cache we've added. The PR stopped bubbling down the sizeRefreshPeriod which was then removed in #4541 because it was unused.

The realization is that we still need this back for the following scenario:

1. Assume a cache TTL of 60s, minCount = 2, currentTarget = 3
2. Two nodes are eligible for deletion by autoscaler, minCount=2 and because CA core won't do the min count check for us in empty node removal.
3. First node gets removed, we decrement scale.Set.CurSize to 2
4. Second call toscaleSet.DeleteNodes() comes in and this calls into GetScaleSetSize which in turn ends up reading from manager.cache which would have a cached count of 3 and thus will return you 3 so you end up deleting that node as well

With this PR, we'll extend the last refresh time by sizeRefreshPeriod to ensure that next time we expire, manager.cache would've had the chance to refresh and give us fresh data. That's basically the behaviour prior to #3717. See in 1.19.

The PR also cleans up the logging to refer to "in-memory size" vs the one we get back from the manager cache.

/area cloudprovider/azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marwanad

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 [marwanad]

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

[marwanad]

This added recently with the same motivation too. If we proactively update the instance state to Deletion, we don't want it to be invalidated in the next loop in case that cache is stale.

[marwanad]

/area provider/azure

[nilo19]

/lgtm