diff --git a/pkg/provider/azure.go b/pkg/provider/azure.go index 0332e17e4f..cf5f9b8254 100644 --- a/pkg/provider/azure.go +++ b/pkg/provider/azure.go @@ -417,6 +417,8 @@ type Cloud struct { pipCache azcache.Resource // use LB frontEndIpConfiguration ID as the key and search for PLS attached to the frontEnd plsCache azcache.Resource + // a timed cache storing storage account properties to avoid querying storage account frequently + storageAccountCache azcache.Resource // Add service lister to always get latest service serviceLister corelisters.ServiceLister @@ -811,6 +813,9 @@ func (az *Cloud) initCaches() (err error) { return err } + if az.storageAccountCache, err = az.newStorageAccountCache(); err != nil { + return err + } return nil } diff --git a/pkg/provider/azure_fakes.go b/pkg/provider/azure_fakes.go index d136ca9053..58d764473f 100644 --- a/pkg/provider/azure_fakes.go +++ b/pkg/provider/azure_fakes.go @@ -126,6 +126,7 @@ func GetTestCloud(ctrl *gomock.Controller) (az *Cloud) { az.pipCache, _ = az.newPIPCache() az.plsCache, _ = az.newPLSCache() az.LoadBalancerBackendPool = NewMockBackendPool(ctrl) + az.storageAccountCache, _ = az.newStorageAccountCache() _ = initDiskControllers(az) diff --git a/pkg/provider/azure_storageaccount.go b/pkg/provider/azure_storageaccount.go index 684406246d..ee0fe7db91 100644 --- a/pkg/provider/azure_storageaccount.go +++ b/pkg/provider/azure_storageaccount.go @@ -31,6 +31,8 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/pointer" + "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" ) @@ -586,12 +588,44 @@ func (az *Cloud) createPrivateDNSZoneGroup(ctx context.Context, dnsZoneGroupName return az.privatednszonegroupclient.CreateOrUpdate(ctx, vnetResourceGroup, privateEndpointName, dnsZoneGroupName, privateDNSZoneGroup, "", false).Error() } -// AddStorageAccountTags add tags to storage account -func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGroup, account string, tags map[string]*string) *retry.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 retry.NewError(false, fmt.Errorf("StorageAccountClient is nil")) + return storage.Account{}, retry.NewError(false, fmt.Errorf("StorageAccountClient is nil")) + } + + if az.storageAccountCache == nil { + return storage.Account{}, retry.NewError(false, fmt.Errorf("storageAccountCache is nil")) + } + + // search in cache first + cache, err := az.storageAccountCache.Get(account, cache.CacheReadTypeDefault) + if err != nil { + return storage.Account{}, retry.NewError(false, err) + } + var result storage.Account + if cache != nil { + result = cache.(storage.Account) + klog.V(2).Infof("Get storage account(%s) from cache", account) + } else { + var rerr *retry.Error + result, rerr = az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account) + if rerr != nil { + return storage.Account{}, rerr + } + az.storageAccountCache.Set(account, result) } - result, rerr := az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account) + + 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 { + result, rerr := az.getStorageAccountWithCache(ctx, subsID, resourceGroup, account) if rerr != nil { return rerr } @@ -606,16 +640,18 @@ func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGrou newTags[k] = v } - updateParams := storage.AccountUpdateParameters{Tags: newTags} - return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams) + if len(newTags) > len(result.Tags) { + // only update when newTags is different from old tags + _ = az.storageAccountCache.Delete(account) // clean cache + updateParams := storage.AccountUpdateParameters{Tags: newTags} + return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams) + } + return nil } // RemoveStorageAccountTag remove tag from storage account func (az *Cloud) RemoveStorageAccountTag(ctx context.Context, subsID, resourceGroup, account, key string) *retry.Error { - if az.StorageAccountClient == nil { - return retry.NewError(false, fmt.Errorf("StorageAccountClient is nil")) - } - result, rerr := az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account) + result, rerr := az.getStorageAccountWithCache(ctx, subsID, resourceGroup, account) if rerr != nil { return rerr } @@ -627,6 +663,8 @@ func (az *Cloud) RemoveStorageAccountTag(ctx context.Context, subsID, resourceGr originalLen := len(result.Tags) delete(result.Tags, key) if originalLen != len(result.Tags) { + // only update when newTags is different from old tags + _ = az.storageAccountCache.Delete(account) // clean cache updateParams := storage.AccountUpdateParameters{Tags: result.Tags} return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams) } diff --git a/pkg/provider/azure_storageaccount_test.go b/pkg/provider/azure_storageaccount_test.go index 8e4abaaa32..0c7a3adb2b 100644 --- a/pkg/provider/azure_storageaccount_test.go +++ b/pkg/provider/azure_storageaccount_test.go @@ -39,6 +39,7 @@ import ( "sigs.k8s.io/cloud-provider-azure/pkg/azureclients/storageaccountclient/mockstorageaccountclient" "sigs.k8s.io/cloud-provider-azure/pkg/azureclients/subnetclient/mocksubnetclient" "sigs.k8s.io/cloud-provider-azure/pkg/azureclients/virtualnetworklinksclient/mockvirtualnetworklinksclient" + "sigs.k8s.io/cloud-provider-azure/pkg/cache" "sigs.k8s.io/cloud-provider-azure/pkg/retry" ) @@ -136,7 +137,7 @@ func TestGetStorageAccessKeys(t *testing.T) { } } -func TestGetStorageAccount(t *testing.T) { +func TestGetStorageAccounts(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -560,6 +561,61 @@ func TestEnsureStorageAccount(t *testing.T) { } } +func TestGetStorageAccountWithCache(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 + setStorageAccountClient bool + setStorageAccountCache bool + expectedErr string + }{ + { + name: "[failure] StorageAccountClient is nil", + expectedErr: "StorageAccountClient is nil", + }, + { + name: "[failure] storageAccountCache is nil", + setStorageAccountClient: true, + expectedErr: "storageAccountCache is nil", + }, + { + name: "[Success]", + setStorageAccountClient: true, + setStorageAccountCache: true, + expectedErr: "", + }, + } + + for _, test := range tests { + if test.setStorageAccountClient { + mockStorageAccountsClient := mockstorageaccountclient.NewMockInterface(ctrl) + cloud.StorageAccountClient = mockStorageAccountsClient + mockStorageAccountsClient.EXPECT().GetProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(storage.Account{}, nil).AnyTimes() + } + + if test.setStorageAccountCache { + getter := func(key string) (interface{}, error) { return nil, nil } + cloud.storageAccountCache, _ = cache.NewTimedCache(time.Minute, getter, false) + } + + _, err := cloud.getStorageAccountWithCache(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.RawError.Error(), test.expectedErr, err.RawError.Error(), test.name) + } + } +} + func TestIsPrivateEndpointAsExpected(t *testing.T) { tests := []struct { account storage.Account