Skip to content

Commit 09fded4

Browse files
andyzhangxandyzhangx
committed
fix: add StorageAccountCache to avoid querying frequently
1 parent 38d89da commit 09fded4

File tree

4 files changed

+110
-11
lines changed

4 files changed

+110
-11
lines changed

‎pkg/provider/azure.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,8 @@ type Cloud struct {
417417
pipCache azcache.Resource
418418
// use LB frontEndIpConfiguration ID as the key and search for PLS attached to the frontEnd
419419
plsCache azcache.Resource
420+
// a timed cache storing storage account properties to avoid querying storage account frequently
421+
StorageAccountCache azcache.Resource
420422

421423
// Add service lister to always get latest service
422424
serviceLister corelisters.ServiceLister
@@ -811,6 +813,10 @@ func (az *Cloud) initCaches() (err error) {
811813
return err
812814
}
813815

816+
getter := func(key string) (interface{}, error) { return nil, nil }
817+
if az.StorageAccountCache, err = azcache.NewTimedCache(time.Minute, getter, false); err != nil {
818+
return err
819+
}
814820
return nil
815821
}
816822

‎pkg/provider/azure_fakes.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@ package provider
1919
import (
2020
"context"
2121
"fmt"
22+
"time"
2223

24+
"sigs.k8s.io/cloud-provider-azure/pkg/cache"
2325
"sigs.k8s.io/cloud-provider-azure/pkg/provider/config"
2426

2527
"github.com/golang/mock/gomock"
@@ -127,6 +129,9 @@ func GetTestCloud(ctrl *gomock.Controller) (az *Cloud) {
127129
az.plsCache, _ = az.newPLSCache()
128130
az.LoadBalancerBackendPool = NewMockBackendPool(ctrl)
129131

132+
getter := func(key string) (interface{}, error) { return nil, nil }
133+
az.StorageAccountCache, _ = cache.NewTimedCache(time.Minute, getter, false)
134+
130135
_ = initDiskControllers(az)
131136

132137
az.regionZonesMap = map[string][]string{az.Location: {"1", "2", "3"}}

‎pkg/provider/azure_storageaccount.go

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
"k8s.io/klog/v2"
3232
"k8s.io/utils/pointer"
3333

34+
"sigs.k8s.io/cloud-provider-azure/pkg/cache"
3435
"sigs.k8s.io/cloud-provider-azure/pkg/consts"
3536
"sigs.k8s.io/cloud-provider-azure/pkg/retry"
3637
)
@@ -586,12 +587,39 @@ func (az *Cloud) createPrivateDNSZoneGroup(ctx context.Context, dnsZoneGroupName
586587
return az.privatednszonegroupclient.CreateOrUpdate(ctx, vnetResourceGroup, privateEndpointName, dnsZoneGroupName, privateDNSZoneGroup, "", false).Error()
587588
}
588589

589-
// AddStorageAccountTags add tags to storage account
590-
func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGroup, account string, tags map[string]*string) *retry.Error {
590+
func (az *Cloud) getStorageAccountWithCache(ctx context.Context, subsID, resourceGroup, account string) (storage.Account, *retry.Error) {
591591
if az.StorageAccountClient == nil {
592-
return retry.NewError(false, fmt.Errorf("StorageAccountClient is nil"))
592+
return storage.Account{}, retry.NewError(false, fmt.Errorf("StorageAccountClient is nil"))
593+
}
594+
595+
if az.StorageAccountCache == nil {
596+
return storage.Account{}, retry.NewError(false, fmt.Errorf("StorageAccountCache is nil"))
597+
}
598+
599+
// search in cache first
600+
cache, err := az.StorageAccountCache.Get(account, cache.CacheReadTypeDefault)
601+
if err != nil {
602+
return storage.Account{}, retry.NewError(false, err)
603+
}
604+
var result storage.Account
605+
if cache != nil {
606+
result = cache.(storage.Account)
607+
klog.V(2).Infof("Get storage account(%s) from cache", account)
608+
} else {
609+
var rerr *retry.Error
610+
result, rerr = az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account)
611+
if rerr != nil {
612+
return storage.Account{}, rerr
613+
}
614+
az.StorageAccountCache.Set(account, result)
593615
}
594-
result, rerr := az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account)
616+
617+
return result, nil
618+
}
619+
620+
// AddStorageAccountTags add tags to storage account
621+
func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGroup, account string, tags map[string]*string) *retry.Error {
622+
result, rerr := az.getStorageAccountWithCache(ctx, subsID, resourceGroup, account)
595623
if rerr != nil {
596624
return rerr
597625
}
@@ -606,16 +634,18 @@ func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGrou
606634
newTags[k] = v
607635
}
608636

609-
updateParams := storage.AccountUpdateParameters{Tags: newTags}
610-
return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams)
637+
if len(newTags) > len(result.Tags) {
638+
// only update when newTags is different from old tags
639+
az.StorageAccountCache.Delete(account) // clean cache
640+
updateParams := storage.AccountUpdateParameters{Tags: newTags}
641+
return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams)
642+
}
643+
return nil
611644
}
612645

613646
// RemoveStorageAccountTag remove tag from storage account
614647
func (az *Cloud) RemoveStorageAccountTag(ctx context.Context, subsID, resourceGroup, account, key string) *retry.Error {
615-
if az.StorageAccountClient == nil {
616-
return retry.NewError(false, fmt.Errorf("StorageAccountClient is nil"))
617-
}
618-
result, rerr := az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account)
648+
result, rerr := az.getStorageAccountWithCache(ctx, subsID, resourceGroup, account)
619649
if rerr != nil {
620650
return rerr
621651
}
@@ -627,6 +657,8 @@ func (az *Cloud) RemoveStorageAccountTag(ctx context.Context, subsID, resourceGr
627657
originalLen := len(result.Tags)
628658
delete(result.Tags, key)
629659
if originalLen != len(result.Tags) {
660+
// only update when newTags is different from old tags
661+
az.StorageAccountCache.Delete(account) // clean cache
630662
updateParams := storage.AccountUpdateParameters{Tags: result.Tags}
631663
return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams)
632664
}

‎pkg/provider/azure_storageaccount_test.go

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/storageaccountclient/mockstorageaccountclient"
4040
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/subnetclient/mocksubnetclient"
4141
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/virtualnetworklinksclient/mockvirtualnetworklinksclient"
42+
"sigs.k8s.io/cloud-provider-azure/pkg/cache"
4243
"sigs.k8s.io/cloud-provider-azure/pkg/retry"
4344
)
4445

@@ -136,7 +137,7 @@ func TestGetStorageAccessKeys(t *testing.T) {
136137
}
137138
}
138139

139-
func TestGetStorageAccount(t *testing.T) {
140+
func TestGetStorageAccounts(t *testing.T) {
140141
ctrl := gomock.NewController(t)
141142
defer ctrl.Finish()
142143

@@ -560,6 +561,61 @@ func TestEnsureStorageAccount(t *testing.T) {
560561
}
561562
}
562563

564+
func TestGetStorageAccountWithCache(t *testing.T) {
565+
ctrl := gomock.NewController(t)
566+
defer ctrl.Finish()
567+
568+
ctx, cancel := getContextWithCancel()
569+
defer cancel()
570+
571+
cloud := &Cloud{}
572+
573+
tests := []struct {
574+
name string
575+
subsID string
576+
resourceGroup string
577+
account string
578+
setStorageAccountClient bool
579+
setStorageAccountCache bool
580+
expectedErr string
581+
}{
582+
{
583+
name: "[failure] StorageAccountClient is nil",
584+
expectedErr: "StorageAccountClient is nil",
585+
},
586+
{
587+
name: "[failure] StorageAccountCache is nil",
588+
setStorageAccountClient: true,
589+
expectedErr: "StorageAccountCache is nil",
590+
},
591+
{
592+
name: "[Success]",
593+
setStorageAccountClient: true,
594+
setStorageAccountCache: true,
595+
expectedErr: "",
596+
},
597+
}
598+
599+
for _, test := range tests {
600+
if test.setStorageAccountClient {
601+
mockStorageAccountsClient := mockstorageaccountclient.NewMockInterface(ctrl)
602+
cloud.StorageAccountClient = mockStorageAccountsClient
603+
mockStorageAccountsClient.EXPECT().GetProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(storage.Account{}, nil).AnyTimes()
604+
}
605+
606+
if test.setStorageAccountCache {
607+
getter := func(key string) (interface{}, error) { return nil, nil }
608+
cloud.StorageAccountCache, _ = cache.NewTimedCache(time.Minute, getter, false)
609+
}
610+
611+
_, err := cloud.getStorageAccountWithCache(ctx, test.subsID, test.resourceGroup, test.account)
612+
assert.Equal(t, err == nil, test.expectedErr == "", fmt.Sprintf("returned error: %v", err), test.name)
613+
if test.expectedErr != "" && err != nil {
614+
assert.Equal(t, err.RawError.Error(), test.expectedErr, err.RawError.Error(), test.name)
615+
}
616+
}
617+
}
618+
563619
func TestIsPrivateEndpointAsExpected(t *testing.T) {
564620
tests := []struct {
565621
account storage.Account

0 commit comments

Comments
 (0)