Merge pull request #3107 from nilo19/fix/cherry-pick-2949-1.23

Use TimedCache.Get() for read-only resources

pkg/cache/azure_cache.go

@@ -122,8 +122,13 @@ func (t *TimedCache) getInternal(key string) (*AzureCacheEntry, error) {
 	return newEntry, nil
 }
 
-// Get returns the requested item by key with deep copy.
+// Get returns the requested item by key.
 func (t *TimedCache) Get(key string, crt AzureCacheReadType) (interface{}, error) {
+	return t.get(key, crt)
+}
+
+// Get returns the requested item by key with deep copy.
+func (t *TimedCache) GetWithDeepCopy(key string, crt AzureCacheReadType) (interface{}, error) {
 	data, err := t.get(key, crt)
 	copied := deepcopy.Copy(data)
 	return copied, err

pkg/cache/azure_cache_test.go

@@ -98,7 +98,7 @@ func TestCacheGet(t *testing.T) {
 	for _, c := range cases {
 		dataSource, cache := newFakeCache(t)
 		dataSource.set(c.data)
-		val, err := cache.Get(c.key, CacheReadTypeDefault)
+		val, err := cache.GetWithDeepCopy(c.key, CacheReadTypeDefault)
 		assert.NoError(t, err, c.name)
 		assert.Equal(t, c.expected, val, c.name)
 	}
@@ -112,7 +112,7 @@ func TestCacheGetError(t *testing.T) {
 	cache, err := NewTimedcache(fakeCacheTTL, getter)
 	assert.NoError(t, err)
 
-	val, err := cache.Get("key", CacheReadTypeDefault)
+	val, err := cache.GetWithDeepCopy("key", CacheReadTypeDefault)
 	assert.Error(t, err)
 	assert.Equal(t, getError, err)
 	assert.Nil(t, val)
@@ -140,7 +140,7 @@ func TestCacheGetWithDeepCopy(t *testing.T) {
 			dataSource, cache := newFakeCache(t)
 			dataSource.set(c.data)
 			cache.Set(c.key, valFake)
-			val, err := cache.Get(c.key, CacheReadTypeDefault)
+			val, err := cache.GetWithDeepCopy(c.key, CacheReadTypeDefault)
 			assert.NoError(t, err)
 			assert.Equal(t, c.expected, val.(*fakeDataObj).Data)
 
@@ -160,13 +160,13 @@ func TestCacheDelete(t *testing.T) {
 	dataSource, cache := newFakeCache(t)
 	dataSource.set(data)
 
-	v, err := cache.Get(testKey, CacheReadTypeDefault)
+	v, err := cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, val, v, "cache should get correct data")
 
 	dataSource.set(nil)
 	_ = cache.Delete(testKey)
-	v, err = cache.Get(testKey, CacheReadTypeDefault)
+	v, err = cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, 1, dataSource.called)
 	assert.Equal(t, nil, v, "cache should get nil after data is removed")
@@ -180,13 +180,13 @@ func TestCacheExpired(t *testing.T) {
 	dataSource, cache := newFakeCache(t)
 	dataSource.set(data)
 
-	v, err := cache.Get(testKey, CacheReadTypeDefault)
+	v, err := cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, 1, dataSource.called)
 	assert.Equal(t, val, v, "cache should get correct data")
 
 	time.Sleep(fakeCacheTTL)
-	v, err = cache.Get(testKey, CacheReadTypeDefault)
+	v, err = cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, 2, dataSource.called)
 	assert.Equal(t, val, v, "cache should get correct data even after expired")
@@ -200,13 +200,13 @@ func TestCacheAllowUnsafeRead(t *testing.T) {
 	dataSource, cache := newFakeCache(t)
 	dataSource.set(data)
 
-	v, err := cache.Get(testKey, CacheReadTypeDefault)
+	v, err := cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, 1, dataSource.called)
 	assert.Equal(t, val, v, "cache should get correct data")
 
 	time.Sleep(fakeCacheTTL)
-	v, err = cache.Get(testKey, CacheReadTypeUnsafe)
+	v, err = cache.GetWithDeepCopy(testKey, CacheReadTypeUnsafe)
 	assert.NoError(t, err)
 	assert.Equal(t, 1, dataSource.called)
 	assert.Equal(t, val, v, "cache should return expired as allow unsafe read is allowed")
@@ -226,10 +226,10 @@ func TestCacheNoConcurrentGet(t *testing.T) {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			_, _ = cache.Get(testKey, CacheReadTypeDefault)
+			_, _ = cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 		}()
 	}
-	v, err := cache.Get(testKey, CacheReadTypeDefault)
+	v, err := cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 	wg.Wait()
 	assert.NoError(t, err)
 	assert.Equal(t, 1, dataSource.called)
@@ -244,12 +244,12 @@ func TestCacheForceRefresh(t *testing.T) {
 	dataSource, cache := newFakeCache(t)
 	dataSource.set(data)
 
-	v, err := cache.Get(testKey, CacheReadTypeDefault)
+	v, err := cache.GetWithDeepCopy(testKey, CacheReadTypeDefault)
 	assert.NoError(t, err)
 	assert.Equal(t, 1, dataSource.called)
 	assert.Equal(t, val, v, "cache should get correct data")
 
-	v, err = cache.Get(testKey, CacheReadTypeForceRefresh)
+	v, err = cache.GetWithDeepCopy(testKey, CacheReadTypeForceRefresh)
 	assert.NoError(t, err)
 	assert.Equal(t, 2, dataSource.called)
 	assert.Equal(t, val, v, "should refetch unexpired data as forced refresh")

pkg/provider/azure_backoff_test.go

@@ -234,7 +234,7 @@ func TestCreateOrUpdateSecurityGroupCanceled(t *testing.T) {
 	assert.EqualError(t, fmt.Errorf("Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: %w", fmt.Errorf("canceledandsupersededduetoanotheroperation")), err.Error())
 
 	// security group should be removed from cache if the operation is canceled
-	shouldBeEmpty, err := az.nsgCache.Get("sg", cache.CacheReadTypeDefault)
+	shouldBeEmpty, err := az.nsgCache.GetWithDeepCopy("sg", cache.CacheReadTypeDefault)
 	assert.NoError(t, err)
 	assert.Empty(t, shouldBeEmpty)
 }
@@ -287,12 +287,12 @@ func TestCreateOrUpdateLB(t *testing.T) {
 		assert.EqualError(t, test.expectedErr, err.Error())
 
 		// loadbalancer should be removed from cache if the etag is mismatch or the operation is canceled
-		shouldBeEmpty, err := az.lbCache.Get("lb", cache.CacheReadTypeDefault)
+		shouldBeEmpty, err := az.lbCache.GetWithDeepCopy("lb", cache.CacheReadTypeDefault)
 		assert.NoError(t, err)
 		assert.Empty(t, shouldBeEmpty)
 
 		// public ip cache should be populated since there's GetPIP
-		shouldNotBeEmpty, err := az.pipCache.Get(az.getPIPCacheKey(az.ResourceGroup, "pip"), cache.CacheReadTypeDefault)
+		shouldNotBeEmpty, err := az.pipCache.GetWithDeepCopy(az.getPIPCacheKey(az.ResourceGroup, "pip"), cache.CacheReadTypeDefault)
 		assert.NoError(t, err)
 		assert.NotEmpty(t, shouldNotBeEmpty)
 	}
@@ -417,7 +417,7 @@ func TestCreateOrUpdatePIP(t *testing.T) {
 		err := az.CreateOrUpdatePIP(&v1.Service{}, az.ResourceGroup, network.PublicIPAddress{Name: to.StringPtr("nic")})
 		assert.EqualError(t, test.expectedErr, err.Error())
 
-		cachedPIP, err := az.pipCache.Get(az.getPIPCacheKey(az.ResourceGroup, "nic"), cache.CacheReadTypeDefault)
+		cachedPIP, err := az.pipCache.GetWithDeepCopy(az.getPIPCacheKey(az.ResourceGroup, "nic"), cache.CacheReadTypeDefault)
 		assert.NoError(t, err)
 		if test.cacheExpectedEmpty {
 			assert.Empty(t, cachedPIP)
@@ -496,7 +496,7 @@ func TestCreateOrUpdateRouteTable(t *testing.T) {
 		assert.EqualError(t, test.expectedErr, err.Error())
 
 		// route table should be removed from cache if the etag is mismatch or the operation is canceled
-		shouldBeEmpty, err := az.rtCache.Get("rt", cache.CacheReadTypeDefault)
+		shouldBeEmpty, err := az.rtCache.GetWithDeepCopy("rt", cache.CacheReadTypeDefault)
 		assert.NoError(t, err)
 		assert.Empty(t, shouldBeEmpty)
 	}
@@ -542,7 +542,7 @@ func TestCreateOrUpdateRoute(t *testing.T) {
 			assert.EqualError(t, test.expectedErr, err.Error())
 		}
 
-		shouldBeEmpty, err := az.rtCache.Get("rt", cache.CacheReadTypeDefault)
+		shouldBeEmpty, err := az.rtCache.GetWithDeepCopy("rt", cache.CacheReadTypeDefault)
 		assert.NoError(t, err)
 		assert.Empty(t, shouldBeEmpty)
 	}

pkg/provider/azure_vmss.go

@@ -171,7 +171,7 @@ func (ss *ScaleSet) getVmssVMByNodeIdentity(node *nodeIdentity, crt azcache.Azur
 
 		virtualMachines := cached.(*sync.Map)
 		if vm, ok := virtualMachines.Load(nodeName); ok {
-			result := vm.(*VMSSVirtualMachinesEntry)
+			result := vm.(*VMSSVirtualMachineEntry)
 			found = true
 			return result.VMSSName, result.InstanceID, result.VirtualMachine, found, nil
 		}
@@ -300,7 +300,7 @@ func (ss *ScaleSet) getVmssVMByInstanceID(resourceGroup, scaleSetName, instanceI
 
 		virtualMachines := cached.(*sync.Map)
 		virtualMachines.Range(func(key, value interface{}) bool {
-			vmEntry := value.(*VMSSVirtualMachinesEntry)
+			vmEntry := value.(*VMSSVirtualMachineEntry)
 			if strings.EqualFold(vmEntry.ResourceGroup, resourceGroup) &&
 				strings.EqualFold(vmEntry.VMSSName, scaleSetName) &&
 				strings.EqualFold(vmEntry.InstanceID, instanceID) {

pkg/provider/azure_vmss_cache.go

@@ -33,7 +33,7 @@ import (
 	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
 )
 
-type VMSSVirtualMachinesEntry struct {
+type VMSSVirtualMachineEntry struct {
 	ResourceGroup  string
 	VMSSName       string
 	InstanceID     string
@@ -157,7 +157,7 @@ func (ss *ScaleSet) newVMSSVirtualMachinesCache(resourceGroupName, vmssName, cac
 	getter := func(key string) (interface{}, error) {
 		localCache := &sync.Map{} // [nodeName]*vmssVirtualMachinesEntry
 
-		oldCache := make(map[string]VMSSVirtualMachinesEntry)
+		oldCache := make(map[string]VMSSVirtualMachineEntry)
 
 		if vmssCache, ok := ss.vmssVMCache.Load(cacheKey); ok {
 			// get old cache before refreshing the cache
@@ -171,7 +171,7 @@ func (ss *ScaleSet) newVMSSVirtualMachinesCache(resourceGroupName, vmssName, cac
 				if cached != nil {
 					virtualMachines := cached.(*sync.Map)
 					virtualMachines.Range(func(key, value interface{}) bool {
-						oldCache[key.(string)] = *value.(*VMSSVirtualMachinesEntry)
+						oldCache[key.(string)] = *value.(*VMSSVirtualMachineEntry)
 						return true
 					})
 				}
@@ -196,7 +196,7 @@ func (ss *ScaleSet) newVMSSVirtualMachinesCache(resourceGroupName, vmssName, cac
 				continue
 			}
 
-			vmssVMCacheEntry := &VMSSVirtualMachinesEntry{
+			vmssVMCacheEntry := &VMSSVirtualMachineEntry{
 				ResourceGroup:  resourceGroupName,
 				VMSSName:       vmssName,
 				InstanceID:     to.String(vm.InstanceID),
@@ -231,7 +231,7 @@ func (ss *ScaleSet) newVMSSVirtualMachinesCache(resourceGroupName, vmssName, cac
 			}
 
 			klog.V(5).Infof("adding old entries to new cache for %s", name)
-			localCache.Store(name, &VMSSVirtualMachinesEntry{
+			localCache.Store(name, &VMSSVirtualMachineEntry{
 				ResourceGroup:  vmEntry.ResourceGroup,
 				VMSSName:       vmEntry.VMSSName,
 				InstanceID:     vmEntry.InstanceID,
@@ -280,7 +280,6 @@ func (ss *ScaleSet) DeleteCacheForNode(nodeName string) error {
 		return err
 	}
 
-	// Delete in VMSS VM cache
 	if err := ss.gcVMSSVMCache(); err != nil {
 		klog.Errorf("DeleteCacheForNode(%s) failed to gc stale vmss caches: %v", nodeName, err)
 	}

pkg/provider/azure_wrap.go

@@ -86,7 +86,7 @@ func (az *Cloud) getRouteTable(crt azcache.AzureCacheReadType) (routeTable netwo
 		return routeTable, false, fmt.Errorf("Route table name is not configured")
 	}
 
-	cachedRt, err := az.rtCache.Get(az.RouteTableName, crt)
+	cachedRt, err := az.rtCache.GetWithDeepCopy(az.RouteTableName, crt)
 	if err != nil {
 		return routeTable, false, err
 	}
@@ -109,7 +109,7 @@ func (az *Cloud) getPIPCacheKey(pipResourceGroup string, pipName string) string
 func (az *Cloud) getPublicIPAddress(pipResourceGroup string, pipName string, crt azcache.AzureCacheReadType) (network.PublicIPAddress, bool, error) {
 	pip := network.PublicIPAddress{}
 	cacheKey := az.getPIPCacheKey(pipResourceGroup, pipName)
-	cachedPIP, err := az.pipCache.Get(cacheKey, crt)
+	cachedPIP, err := az.pipCache.GetWithDeepCopy(cacheKey, crt)
 	if err != nil {
 		return pip, false, err
 	}
@@ -146,7 +146,7 @@ func (az *Cloud) getSubnet(virtualNetworkName string, subnetName string) (networ
 }
 
 func (az *Cloud) getAzureLoadBalancer(name string, crt azcache.AzureCacheReadType) (lb *network.LoadBalancer, exists bool, err error) {
-	cachedLB, err := az.lbCache.Get(name, crt)
+	cachedLB, err := az.lbCache.GetWithDeepCopy(name, crt)
 	if err != nil {
 		return lb, false, err
 	}
@@ -164,7 +164,7 @@ func (az *Cloud) getSecurityGroup(crt azcache.AzureCacheReadType) (network.Secur
 		return nsg, fmt.Errorf("securityGroupName is not configured")
 	}
 
-	securityGroup, err := az.nsgCache.Get(az.SecurityGroupName, crt)
+	securityGroup, err := az.nsgCache.GetWithDeepCopy(az.SecurityGroupName, crt)
 	if err != nil {
 		return nsg, err
 	}
@@ -177,7 +177,7 @@ func (az *Cloud) getSecurityGroup(crt azcache.AzureCacheReadType) (network.Secur
 }
 
 func (az *Cloud) getPrivateLinkService(frontendIPConfigID *string, crt azcache.AzureCacheReadType) (pls network.PrivateLinkService, err error) {
-	cachedPLS, err := az.plsCache.Get(*frontendIPConfigID, crt)
+	cachedPLS, err := az.plsCache.GetWithDeepCopy(*frontendIPConfigID, crt)
 	if err != nil {
 		return pls, err
 	}

tests/e2e/network/node.go

@@ -320,11 +320,11 @@ var _ = Describe("Azure nodes", func() {
 
 		utils.Logf("scaling VMSS")
 		count := *vmss.Sku.Capacity
-		err = utils.ScaleVMSS(tc, *vmss.Name, tc.GetResourceGroup(), int64(vmssScaleUpCelling))
+		err = utils.ScaleVMSS(tc, *vmss.Name, int64(vmssScaleUpCelling))
 
 		defer func() {
 			utils.Logf("restoring VMSS")
-			err = utils.ScaleVMSS(tc, *vmss.Name, tc.GetResourceGroup(), count)
+			err = utils.ScaleVMSS(tc, *vmss.Name, count)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 

tests/e2e/node/vmss.go

@@ -17,9 +17,6 @@ limitations under the License.
 package node
 
 import (
-	"os"
-	"strings"
-
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
@@ -29,7 +26,7 @@ import (
 	"sigs.k8s.io/cloud-provider-azure/tests/e2e/utils"
 )
 
-var _ = Describe("Lifecycle of VMSS", Label(utils.TestSuiteLabelVMSS), func() {
+var _ = Describe("Lifecycle of VMSS", Label(utils.TestSuiteLabelVMSS, utils.TestSuiteLabelVMSSScale), func() {
 	var (
 		ns     *v1.Namespace
 		k8sCli kubernetes.Interface
@@ -72,21 +69,13 @@ var _ = Describe("Lifecycle of VMSS", Label(utils.TestSuiteLabelVMSS), func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("deallocate VMSS instance")
-		if strings.EqualFold(os.Getenv(utils.CAPZTestCCM), "true") {
-			err = utils.ScaleMachinePool(*vmss.Name, numInstance-1)
-		} else {
-			err = utils.ScaleVMSS(azCli, *vmss.Name, azCli.GetResourceGroup(), numInstance-1)
-		}
+		err = utils.Scale(azCli, *vmss.Name, numInstance-1)
 		Expect(err).NotTo(HaveOccurred())
 		expectedCap[*vmss.Name] = numInstance - 1
 
 		defer func() {
 			By("reset VMSS instance")
-			if strings.EqualFold(os.Getenv(utils.CAPZTestCCM), "true") {
-				err = utils.ScaleMachinePool(*vmss.Name, numInstance)
-			} else {
-				err = utils.ScaleVMSS(azCli, *vmss.Name, azCli.GetResourceGroup(), numInstance)
-			}
+			err = utils.Scale(azCli, *vmss.Name, numInstance)
 			Expect(err).NotTo(HaveOccurred())
 			expectedCap[*vmss.Name] = numInstance
 
@@ -116,21 +105,13 @@ var _ = Describe("Lifecycle of VMSS", Label(utils.TestSuiteLabelVMSS), func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("allocate VMSS instance")
-		if strings.EqualFold(os.Getenv(utils.CAPZTestCCM), "true") {
-			err = utils.ScaleMachinePool(*vmss.Name, numInstance+1)
-		} else {
-			err = utils.ScaleVMSS(azCli, *vmss.Name, azCli.GetResourceGroup(), numInstance+1)
-		}
+		err = utils.Scale(azCli, *vmss.Name, numInstance+1)
 		Expect(err).NotTo(HaveOccurred())
 		expectedCap[*vmss.Name] = numInstance + 1
 
 		defer func() {
 			By("reset VMSS instance")
-			if strings.EqualFold(os.Getenv(utils.CAPZTestCCM), "true") {
-				err = utils.ScaleMachinePool(*vmss.Name, numInstance)
-			} else {
-				err = utils.ScaleVMSS(azCli, *vmss.Name, azCli.GetResourceGroup(), numInstance)
-			}
+			err = utils.Scale(azCli, *vmss.Name, numInstance)
 			Expect(err).NotTo(HaveOccurred())
 			expectedCap[*vmss.Name] = numInstance
 

tests/e2e/utils/consts.go

@@ -24,6 +24,7 @@ const (
 	TestSuiteLabelMultiNodePools     = "Multi-Nodepool"
 	TestSuiteLabelSingleNodePool     = "Single-Nodepool"
 	TestSuiteLabelVMSS               = "VMSS"
+	TestSuiteLabelVMSSScale          = "VMSS-Scale"
 	TestSuiteLabelSpotVM             = "Spot-VM"
 	TestSuiteLabelKubenet            = "Kubenet"
 	TestSuiteLabelMultiGroup         = "Multi-Group"