Skip to content

Commit bc54df5

Browse files
andyzhangxandyzhangx
committed
fix: add StorageAccountCache to avoid querying frequently
fix golint fix comments
1 parent 38d89da commit bc54df5

File tree

4 files changed

+115
-11
lines changed

4 files changed

+115
-11
lines changed

‎pkg/provider/azure.go

Lines changed: 5 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,9 @@ func (az *Cloud) initCaches() (err error) {
811813
return err
812814
}
813815

816+
if az.storageAccountCache, err = az.newStorageAccountCache(); err != nil {
817+
return err
818+
}
814819
return nil
815820
}
816821

‎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: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ import (
3131
"k8s.io/klog/v2"
3232
"k8s.io/utils/pointer"
3333

34+
"sigs.k8s.io/cloud-provider-azure/pkg/cache"
35+
azcache "sigs.k8s.io/cloud-provider-azure/pkg/cache"
3436
"sigs.k8s.io/cloud-provider-azure/pkg/consts"
3537
"sigs.k8s.io/cloud-provider-azure/pkg/retry"
3638
)
@@ -586,12 +588,44 @@ func (az *Cloud) createPrivateDNSZoneGroup(ctx context.Context, dnsZoneGroupName
586588
return az.privatednszonegroupclient.CreateOrUpdate(ctx, vnetResourceGroup, privateEndpointName, dnsZoneGroupName, privateDNSZoneGroup, "", false).Error()
587589
}
588590

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 {
591+
func (az *Cloud) newStorageAccountCache() (azcache.Resource, error) {
592+
getter := func(key string) (interface{}, error) { return nil, nil }
593+
return azcache.NewTimedCache(time.Minute, getter, az.Config.DisableAPICallCache)
594+
}
595+
596+
func (az *Cloud) getStorageAccountWithCache(ctx context.Context, subsID, resourceGroup, account string) (storage.Account, *retry.Error) {
591597
if az.StorageAccountClient == nil {
592-
return retry.NewError(false, fmt.Errorf("StorageAccountClient is nil"))
598+
return storage.Account{}, retry.NewError(false, fmt.Errorf("StorageAccountClient is nil"))
599+
}
600+
601+
if az.storageAccountCache == nil {
602+
return storage.Account{}, retry.NewError(false, fmt.Errorf("storageAccountCache is nil"))
603+
}
604+
605+
// search in cache first
606+
cache, err := az.storageAccountCache.Get(account, cache.CacheReadTypeDefault)
607+
if err != nil {
608+
return storage.Account{}, retry.NewError(false, err)
609+
}
610+
var result storage.Account
611+
if cache != nil {
612+
result = cache.(storage.Account)
613+
klog.V(2).Infof("Get storage account(%s) from cache", account)
614+
} else {
615+
var rerr *retry.Error
616+
result, rerr = az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account)
617+
if rerr != nil {
618+
return storage.Account{}, rerr
619+
}
620+
az.storageAccountCache.Set(account, result)
593621
}
594-
result, rerr := az.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account)
622+
623+
return result, nil
624+
}
625+
626+
// AddStorageAccountTags add tags to storage account
627+
func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGroup, account string, tags map[string]*string) *retry.Error {
628+
result, rerr := az.getStorageAccountWithCache(ctx, subsID, resourceGroup, account)
595629
if rerr != nil {
596630
return rerr
597631
}
@@ -606,16 +640,18 @@ func (az *Cloud) AddStorageAccountTags(ctx context.Context, subsID, resourceGrou
606640
newTags[k] = v
607641
}
608642

609-
updateParams := storage.AccountUpdateParameters{Tags: newTags}
610-
return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams)
643+
if len(newTags) > len(result.Tags) {
644+
// only update when newTags is different from old tags
645+
_ = az.storageAccountCache.Delete(account) // clean cache
646+
updateParams := storage.AccountUpdateParameters{Tags: newTags}
647+
return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams)
648+
}
649+
return nil
611650
}
612651

613652
// RemoveStorageAccountTag remove tag from storage account
614653
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)
654+
result, rerr := az.getStorageAccountWithCache(ctx, subsID, resourceGroup, account)
619655
if rerr != nil {
620656
return rerr
621657
}
@@ -627,6 +663,8 @@ func (az *Cloud) RemoveStorageAccountTag(ctx context.Context, subsID, resourceGr
627663
originalLen := len(result.Tags)
628664
delete(result.Tags, key)
629665
if originalLen != len(result.Tags) {
666+
// only update when newTags is different from old tags
667+
_ = az.storageAccountCache.Delete(account) // clean cache
630668
updateParams := storage.AccountUpdateParameters{Tags: result.Tags}
631669
return az.StorageAccountClient.Update(ctx, subsID, resourceGroup, account, updateParams)
632670
}

‎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)