From c153a63df83863fc2aa11a9069e5479b62c70214 Mon Sep 17 00:00:00 2001 From: Benjamin Pineau Date: Tue, 18 Aug 2020 14:52:02 +0200 Subject: [PATCH 1/2] Avoid unwanted VMSS VMs caches invalidations `fetchAutoAsgs()` is called at regular intervals, fetches a list of VMSS, then call `Register()` to cache each of those. That registration function will tell the caller wether that vmss' cache is outdated (when the provided VMSS, supposedly fresh, is different than the one held in cache) and will replace existing cache entry by the provided VMSS (which in effect will require a forced refresh since that ScaleSet struct is passed by fetchAutoAsgs with a nil lastRefresh time and an empty instanceCache). To detect changes, `Register()` uses an `reflect.DeepEqual()` between the provided and the cached VMSS. Which does always find them different: cached VMSS were enriched with instances lists (while the provided one is blank, fresh from a simple vmss.list call). That DeepEqual is also fragile due to the compared structs containing mutexes (that may be held or not) and refresh timestamps, attributes that shoudln't be relevant to the comparison. As a consequence, all Register() calls causes indirect cache invalidations and a costly refresh (VMSS VMS List). The number of Register() calls is directly proportional to the number of VMSS attached to the cluster, and can easily triggers ARM API throttling. With a large number of VMSS, that throttling prevents `fetchAutoAsgs` to ever succeed (and cluster-autoscaler to start). ie.: ``` I0807 16:55:25.875907 153 azure_scale_set.go:344] GetScaleSetVms: starts I0807 16:55:25.875915 153 azure_scale_set.go:350] GetScaleSetVms: scaleSet.Name: a-testvmss-10, vmList: [] E0807 16:55:25.875919 153 azure_scale_set.go:352] VirtualMachineScaleSetVMsClient.List failed for a-testvmss-10: &{true 0 2020-08-07 17:10:25.875447854 +0000 UTC m=+913.985215807 azure cloud provider throttled for operation VMSSVMList with reason "client throttled"} E0807 16:55:25.875928 153 azure_manager.go:538] Failed to regenerate ASG cache: Retriable: true, RetryAfter: 899s, HTTPStatusCode: 0, RawError: azure cloud provider throttled for operation VMSSVMList with reason "client throttled" F0807 16:55:25.875934 153 azure_cloud_provider.go:167] Failed to create Azure Manager: Retriable: true, RetryAfter: 899s, HTTPStatusCode: 0, RawError: azure cloud provider throttled for operation VMSSVMList with reason "client throttled" goroutine 28 [running]: ``` From [`ScaleSet` struct attributes](https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go#L74-L89) (manager, sizes, mutexes, refreshes timestamps) only sizes are relevant to that comparison. `curSize` is not strictly necessary, but comparing it will provide early instance caches refreshs. --- .../cloudprovider/azure/azure_cache.go | 7 ++++--- .../cloudprovider/azure/azure_manager.go | 13 +++++++++++-- .../cloudprovider/azure/azure_manager_test.go | 4 ++-- .../cloudprovider/azure/azure_scale_set.go | 4 ++-- 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/azure/azure_cache.go b/cluster-autoscaler/cloudprovider/azure/azure_cache.go index 524cd8ccffdf..84d355de2b52 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_cache.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_cache.go @@ -17,7 +17,6 @@ limitations under the License. package azure import ( - "reflect" "regexp" "strings" "sync" @@ -62,7 +61,9 @@ func (m *asgCache) Register(asg cloudprovider.NodeGroup) bool { for i := range m.registeredAsgs { if existing := m.registeredAsgs[i]; strings.EqualFold(existing.Id(), asg.Id()) { - if reflect.DeepEqual(existing, asg) { + e := existing.(*ScaleSet) + a := asg.(*ScaleSet) + if e.minSize == a.minSize && e.maxSize == a.maxSize && e.curSize == a.curSize { return false } @@ -181,7 +182,7 @@ func (m *asgCache) regenerate() error { m.instanceToAsg = newCache - // Incalidating unowned instance cache. + // Invalidating unowned instance cache. m.invalidateUnownedInstanceCache() return nil diff --git a/cluster-autoscaler/cloudprovider/azure/azure_manager.go b/cluster-autoscaler/cloudprovider/azure/azure_manager.go index 3df00aa7e4bc..436cfc82045d 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_manager.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_manager.go @@ -466,7 +466,7 @@ func (m *AzureManager) buildAsgFromSpec(spec string) (cloudprovider.NodeGroup, e case vmTypeStandard: return NewAgentPool(s, m) case vmTypeVMSS: - return NewScaleSet(s, m) + return NewScaleSet(s, m, -1) case vmTypeAKS: return NewAKSAgentPool(s, m) default: @@ -650,7 +650,16 @@ func (m *AzureManager) listScaleSets(filter []labelAutoDiscoveryConfig) ([]cloud return asgs, fmt.Errorf("maximum size must be greater than minimum size") } - asg, _ := NewScaleSet(spec, m) + curSize := int64(-1) + if scaleSet.Sku != nil && scaleSet.Sku.Capacity != nil { + curSize = *scaleSet.Sku.Capacity + } + + asg, err := NewScaleSet(spec, m, curSize) + if err != nil { + klog.Warningf("ignoring nodegroup %q %s", *scaleSet.Name, err) + continue + } asgs = append(asgs, asg) } diff --git a/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go b/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go index d3bc76cea07d..fe64212e95c1 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_manager_test.go @@ -221,7 +221,7 @@ func TestListScalesets(t *testing.T) { minSize: 5, maxSize: 50, manager: manager, - curSize: -1, + curSize: 3, sizeRefreshPeriod: defaultVmssSizeRefreshPeriod, }}, }, @@ -327,7 +327,7 @@ func TestGetFilteredAutoscalingGroupsVmss(t *testing.T) { minSize: minVal, maxSize: maxVal, manager: manager, - curSize: -1, + curSize: 3, sizeRefreshPeriod: defaultVmssSizeRefreshPeriod, }} assert.True(t, assert.ObjectsAreEqualValues(expectedAsgs, asgs), "expected %#v, but found: %#v", expectedAsgs, asgs) diff --git a/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go b/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go index 99bfbc4c73c6..869fdde02238 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go @@ -89,7 +89,7 @@ type ScaleSet struct { } // NewScaleSet creates a new NewScaleSet. -func NewScaleSet(spec *dynamic.NodeGroupSpec, az *AzureManager) (*ScaleSet, error) { +func NewScaleSet(spec *dynamic.NodeGroupSpec, az *AzureManager, curSize int64) (*ScaleSet, error) { scaleSet := &ScaleSet{ azureRef: azureRef{ Name: spec.Name, @@ -97,7 +97,7 @@ func NewScaleSet(spec *dynamic.NodeGroupSpec, az *AzureManager) (*ScaleSet, erro minSize: spec.MinSize, maxSize: spec.MaxSize, manager: az, - curSize: -1, + curSize: curSize, } if az.config.VmssCacheTTL != 0 { From e146e3ee84f943b4144e9c3a84a5a85e805694e4 Mon Sep 17 00:00:00 2001 From: Marwan Ahmed Date: Tue, 25 Aug 2020 16:23:08 -0700 Subject: [PATCH 2/2] call in the nodegroup API to avoid type assertion errors --- .../cloudprovider/azure/azure_cache.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/azure/azure_cache.go b/cluster-autoscaler/cloudprovider/azure/azure_cache.go index 84d355de2b52..660ec9954262 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_cache.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_cache.go @@ -55,27 +55,25 @@ func newAsgCache() (*asgCache, error) { } // Register registers a node group if it hasn't been registered. -func (m *asgCache) Register(asg cloudprovider.NodeGroup) bool { +func (m *asgCache) Register(newAsg cloudprovider.NodeGroup) bool { m.mutex.Lock() defer m.mutex.Unlock() for i := range m.registeredAsgs { - if existing := m.registeredAsgs[i]; strings.EqualFold(existing.Id(), asg.Id()) { - e := existing.(*ScaleSet) - a := asg.(*ScaleSet) - if e.minSize == a.minSize && e.maxSize == a.maxSize && e.curSize == a.curSize { + if existing := m.registeredAsgs[i]; strings.EqualFold(existing.Id(), newAsg.Id()) { + if existing.MinSize() == newAsg.MinSize() && existing.MaxSize() == newAsg.MaxSize() { return false } - m.registeredAsgs[i] = asg - klog.V(4).Infof("ASG %q updated", asg.Id()) + m.registeredAsgs[i] = newAsg + klog.V(4).Infof("ASG %q updated", newAsg.Id()) m.invalidateUnownedInstanceCache() return true } } - klog.V(4).Infof("Registering ASG %q", asg.Id()) - m.registeredAsgs = append(m.registeredAsgs, asg) + klog.V(4).Infof("Registering ASG %q", newAsg.Id()) + m.registeredAsgs = append(m.registeredAsgs, newAsg) m.invalidateUnownedInstanceCache() return true }