Repository: kubernetes-sigs/cloud-provider-azure
Pull request #6567 (2024-07-10 08:41:05)

[release-1.29] fix: add getFileServicePropertiesCache to fix throttling issue
This is an automated cherry-pick of #6564

/assign andyzhangx

```release-note
fix: add getFileServicePropertiesCache to fix throttling issue
```


Patch:
diff --git a/pkg/provider/azure.go b/pkg/provider/azure.go
index 5da9454bd..4ae8a0e3b 100644
--- a/pkg/provider/azure.go
+++ b/pkg/provider/azure.go
@@ -437,6 +437,8 @@ type Cloud struct {
 	plsCache azcache.Resource
 	// a timed cache storing storage account properties to avoid querying storage account frequently
 	storageAccountCache azcache.Resource
+	// a timed cache storing storage account file service properties to avoid querying storage account file service properties frequently
+	fileServicePropertiesCache azcache.Resource
 
 	// Add service lister to always get latest service
 	serviceLister corelisters.ServiceLister
@@ -857,7 +859,11 @@ func (az *Cloud) initCaches() (err error) {
 		return err
 	}
 
-	if az.storageAccountCache, err = az.newStorageAccountCache(); err != nil {
+	getter := func(key string) (interface{}, error) { return nil, nil }
+	if az.storageAccountCache, err = azcache.NewTimedCache(time.Minute, getter, az.Config.DisableAPICallCache); err != nil {
+		return err
+	}
+	if az.fileServicePropertiesCache, err = azcache.NewTimedCache(5*time.Minute, getter, az.Config.DisableAPICallCache); err != nil {
 		return err
 	}
 	return nil
diff --git a/pkg/provider/azure_fakes.go b/pkg/provider/azure_fakes.go
index 556ff238a..85ae58b62 100644
--- a/pkg/provider/azure_fakes.go
+++ b/pkg/provider/azure_fakes.go
@@ -18,6 +18,7 @@ package provider
 
 import (
 	"context"
+	"time"
 
 	"go.uber.org/mock/gomock"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -40,6 +41,7 @@ import (
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmclient/mockvmclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmssclient/mockvmssclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmssvmclient/mockvmssvmclient"
+	azcache "sigs.k8s.io/cloud-provider-azure/pkg/cache"
 	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
 	"sigs.k8s.io/cloud-provider-azure/pkg/provider/config"
 	utilsets "sigs.k8s.io/cloud-provider-azure/pkg/util/sets"
@@ -129,7 +131,10 @@ func GetTestCloud(ctrl *gomock.Controller) (az *Cloud) {
 	az.pipCache, _ = az.newPIPCache()
 	az.plsCache, _ = az.newPLSCache()
 	az.LoadBalancerBackendPool = NewMockBackendPool(ctrl)
-	az.storageAccountCache, _ = az.newStorageAccountCache()
+
+	getter := func(key string) (interface{}, error) { return nil, nil }
+	az.storageAccountCache, _ = azcache.NewTimedCache(time.Minute, getter, az.Config.DisableAPICallCache)
+	az.fileServicePropertiesCache, _ = azcache.NewTimedCache(5*time.Minute, getter, az.Config.DisableAPICallCache)
 
 	az.regionZonesMap = map[string][]string{az.Location: {"1", "2", "3"}}
 
diff --git a/pkg/provider/azure_storageaccount.go b/pkg/provider/azure_storageaccount.go
index f6477ff76..b47dabdad 100644
--- a/pkg/provider/azure_storageaccount.go
+++ b/pkg/provider/azure_storageaccount.go
@@ -36,7 +36,6 @@ import (
 	"sigs.k8s.io/cloud-provider-azure/pkg/azclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azclient/accountclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/cache"
-	azcache "sigs.k8s.io/cloud-provider-azure/pkg/cache"
 	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
 	"sigs.k8s.io/cloud-provider-azure/pkg/retry"
 )
@@ -507,7 +506,7 @@ func (az *Cloud) EnsureStorageAccount(ctx context.Context, accountOptions *Accou
 		}
 
 		if accountOptions.DisableFileServiceDeleteRetentionPolicy != nil || accountOptions.IsMultichannelEnabled != nil {
-			prop, err := az.FileClient.WithSubscriptionID(subsID).GetServiceProperties(ctx, resourceGroup, accountName)
+			prop, err := az.getFileServicePropertiesCache(ctx, accountOptions.SubscriptionID, accountOptions.ResourceGroup, accountName)
 			if err != nil {
 				return "", "", err
 			}
@@ -672,11 +671,6 @@ func (az *Cloud) createPrivateDNSZoneGroup(ctx context.Context, dnsZoneGroupName
 	return az.privatednszonegroupclient.CreateOrUpdate(ctx, vnetResourceGroup, privateEndpointName, dnsZoneGroupName, privateDNSZoneGroup, "", false).Error()
 }
 
-func (az *Cloud) newStorageAccountCache() (azcache.Resource, error) {
-	getter := func(key string) (interface{}, error) { return nil, nil }
-	return azcache.NewTimedCache(time.Minute, getter, az.Config.DisableAPICallCache)
-}
-
 func (az *Cloud) getStorageAccountWithCache(ctx context.Context, subsID, resourceGroup, account string) (storage.Account, *retry.Error) {
 	if az.StorageAccountClient == nil {
 		return storage.Account{}, retry.NewError(false, fmt.Errorf("StorageAccountClient is nil"))
@@ -707,6 +701,34 @@ func (az *Cloud) getStorageAccountWithCache(ctx context.Context, subsID, resourc
 	return result, nil
 }
 
+func (az *Cloud) getFileServicePropertiesCache(ctx context.Context, subsID, resourceGroup, account string) (storage.FileServiceProperties, error) {
+	if az.FileClient == nil {
+		return storage.FileServiceProperties{}, fmt.Errorf("FileClient is nil")
+	}
+	if az.fileServicePropertiesCache == nil {
+		return storage.FileServiceProperties{}, fmt.Errorf("fileServicePropertiesCache is nil")
+	}
+
+	// search in cache first
+	cache, err := az.fileServicePropertiesCache.Get(account, cache.CacheReadTypeDefault)
+	if err != nil {
+		return storage.FileServiceProperties{}, err
+	}
+	var result storage.FileServiceProperties
+	if cache != nil {
+		result = cache.(storage.FileServiceProperties)
+		klog.V(2).Infof("Get service properties(%s) from cache", account)
+	} else {
+		result, err = az.FileClient.WithSubscriptionID(subsID).GetServiceProperties(ctx, resourceGroup, account)
+		if err != nil {
+			return storage.FileServiceProperties{}, err
+		}
+		az.fileServicePropertiesCache.Set(account, result)
+	}
+
+	return result, nil
+}
+
 // AddStorageAccountTags add tags to storage account
 func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGroup, account string, tags map[string]*string) *retry.Error {
 	// add lock to avoid concurrent update on the cache
@@ -914,7 +936,7 @@ func (az *Cloud) isMultichannelEnabledEqual(ctx context.Context, account storage
 		return false
 	}
 
-	prop, err := az.FileClient.WithSubscriptionID(accountOptions.SubscriptionID).GetServiceProperties(ctx, accountOptions.ResourceGroup, *account.Name)
+	prop, err := az.getFileServicePropertiesCache(ctx, accountOptions.SubscriptionID, accountOptions.ResourceGroup, *account.Name)
 	if err != nil {
 		klog.Warningf("GetServiceProperties(%s) under resource group(%s) failed with %v", *account.Name, accountOptions.ResourceGroup, err)
 		return false


Test patch:
diff --git a/pkg/provider/azure_storageaccount_test.go b/pkg/provider/azure_storageaccount_test.go
index 5290323ab..0c3be50f9 100644
--- a/pkg/provider/azure_storageaccount_test.go
+++ b/pkg/provider/azure_storageaccount_test.go
@@ -619,6 +619,61 @@ func TestGetStorageAccountWithCache(t *testing.T) {
 	}
 }
 
+func TestGetFileServicePropertiesCache(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	ctx, cancel := getContextWithCancel()
+	defer cancel()
+
+	cloud := &Cloud{}
+
+	tests := []struct {
+		name                          string
+		subsID                        string
+		resourceGroup                 string
+		account                       string
+		setFileClient                 bool
+		setFileServicePropertiesCache bool
+		expectedErr                   string
+	}{
+		{
+			name:        "[failure] FileClient is nil",
+			expectedErr: "FileClient is nil",
+		},
+		{
+			name:          "[failure] fileServicePropertiesCache is nil",
+			setFileClient: true,
+			expectedErr:   "fileServicePropertiesCache is nil",
+		},
+		{
+			name:                          "[Success]",
+			setFileClient:                 true,
+			setFileServicePropertiesCache: true,
+			expectedErr:                   "",
+		},
+	}
+
+	for _, test := range tests {
+		if test.setFileClient {
+			mockFileClient := mockfileclient.NewMockInterface(ctrl)
+			cloud.FileClient = mockFileClient
+			mockFileClient.EXPECT().WithSubscriptionID(gomock.Any()).Return(mockFileClient).AnyTimes()
+			mockFileClient.EXPECT().GetServiceProperties(gomock.Any(), gomock.Any(), gomock.Any()).Return(storage.FileServiceProperties{}, nil).AnyTimes()
+		}
+		if test.setFileServicePropertiesCache {
+			getter := func(key string) (interface{}, error) { return nil, nil }
+			cloud.fileServicePropertiesCache, _ = cache.NewTimedCache(time.Minute, getter, false)
+		}
+
+		_, err := cloud.getFileServicePropertiesCache(ctx, test.subsID, test.resourceGroup, test.account)
+		assert.Equal(t, err == nil, test.expectedErr == "", fmt.Sprintf("returned error: %v", err), test.name)
+		if test.expectedErr != "" && err != nil {
+			assert.Equal(t, err.Error(), test.expectedErr, err.Error(), test.name)
+		}
+	}
+}
+
 func TestAddStorageAccountTags(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -1502,6 +1557,7 @@ func TestIsMultichannelEnabledEqual(t *testing.T) {
 	accountName := "account2"
 
 	cloud := GetTestCloud(ctrl)
+	getter := func(key string) (interface{}, error) { return nil, nil }
 
 	multichannelEnabled := storage.FileServiceProperties{
 		FileServicePropertiesProperties: &storage.FileServicePropertiesProperties{
@@ -1556,7 +1612,7 @@ func TestIsMultichannelEnabledEqual(t *testing.T) {
 			expectedResult: false,
 		},
 		{
-			desc: "IsMultichannelEnabled not equal",
+			desc: "IsMultichannelEnabled not equal #1",
 			account: storage.Account{
 				Name:              &accountName,
 				AccountProperties: &storage.AccountProperties{},
@@ -1581,7 +1637,7 @@ func TestIsMultichannelEnabledEqual(t *testing.T) {
 			expectedResult:            false,
 		},
 		{
-			desc: "IsMultichannelEnabled not equal",
+			desc: "IsMultichannelEnabled not equal #2",
 			account: storage.Account{
 				Name:              &accountName,
 				AccountProperties: &storage.AccountProperties{},
@@ -1593,7 +1649,7 @@ func TestIsMultichannelEnabledEqual(t *testing.T) {
 			expectedResult:    false,
 		},
 		{
-			desc: "IsMultichannelEnabled is equal",
+			desc: "IsMultichannelEnabled is equal #1",
 			account: storage.Account{
 				Name:              &accountName,
 				AccountProperties: &storage.AccountProperties{},
@@ -1605,7 +1661,7 @@ func TestIsMultichannelEnabledEqual(t *testing.T) {
 			expectedResult:    true,
 		},
 		{
-			desc: "IsMultichannelEnabled is equal",
+			desc: "IsMultichannelEnabled is equal #2",
 			account: storage.Account{
 				Name:              &accountName,
 				AccountProperties: &storage.AccountProperties{},
@@ -1631,6 +1687,7 @@ func TestIsMultichannelEnabledEqual(t *testing.T) {
 	}
 
 	for _, test := range tests {
+		cloud.fileServicePropertiesCache, _ = cache.NewTimedCache(time.Minute, getter, false)
 		if test.serviceProperties != nil {
 			mockFileClient.EXPECT().GetServiceProperties(gomock.Any(), gomock.Any(), gomock.Any()).Return(*test.serviceProperties, test.servicePropertiesRetError).Times(1)
 		}