Skip to content

fix: add StorageAccountCache to avoid querying storage account frequently #4422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 7, 2023
Merged

fix: add StorageAccountCache to avoid querying storage account frequently #4422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/provider/azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -811,6 +813,9 @@ func (az *Cloud) initCaches() (err error) {
return err
}

if az.storageAccountCache, err = az.newStorageAccountCache(); err != nil {
return err
}
return nil
}

Expand Down
1 change: 1 addition & 0 deletions pkg/provider/azure_fakes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
58 changes: 48 additions & 10 deletions pkg/provider/azure_storageaccount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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)
}
Expand Down
58 changes: 57 additions & 1 deletion pkg/provider/azure_storageaccount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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
Expand Down