Merge pull request #850 from nilo19/fix/zone

fix: do not crash if the region does not support zones
kubernetes-sigs · Oct 15, 2021 · a25721f · a25721f
2 parents aec01dd + f4e9cab
commit a25721f
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 13 deletions.
diff --git a/pkg/provider/azure_zones.go b/pkg/provider/azure_zones.go
@@ -18,6 +18,7 @@ package provider
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"strconv"
@@ -31,6 +32,7 @@ import (
 
 	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"
 )
 
 func (az *Cloud) refreshZones(refreshFunc func() error) {
@@ -43,15 +45,14 @@ func (az *Cloud) refreshZones(refreshFunc func() error) {
 }
 
 func (az *Cloud) syncRegionZonesMap() error {
-	klog.V(2).Infof("refreshZones: starting to fetch all available zones for the subscription %s", az.SubscriptionID)
+	klog.V(2).Infof("syncRegionZonesMap: starting to fetch all available zones for the subscription %s", az.SubscriptionID)
 	zones, rerr := az.ZoneClient.GetZones(context.Background(), az.SubscriptionID)
 	if rerr != nil {
-		klog.Warningf("refreshZones: error when get zones: %s, will retry after %s", rerr.Error().Error(), consts.ZoneFetchingInterval.String())
+		klog.Warningf("syncRegionZonesMap: error when get zones: %s, will retry after %s", rerr.Error().Error(), consts.ZoneFetchingInterval.String())
 		return rerr.Error()
 	}
 	if len(zones) == 0 {
-		klog.Warningf("refreshZones: empty zone list, will retry after %s", consts.ZoneFetchingInterval.String())
-		return fmt.Errorf("empty zone list")
+		klog.Warning("syncRegionZonesMap: empty zone list")
 	}
 
 	az.updateRegionZonesMap(zones)
@@ -89,25 +90,34 @@ func (az *Cloud) getRegionZonesBackoff(region string) ([]string, error) {
 
 	klog.V(2).Infof("getRegionZonesMapWrapper: the region-zones map is not initialized successfully, retrying immediately")
 
+	var (
+		zones map[string][]string
+		rerr  *retry.Error
+	)
 	err := wait.ExponentialBackoff(az.RequestBackoff(), func() (done bool, err error) {
-		zones, rerr := az.ZoneClient.GetZones(context.Background(), az.SubscriptionID)
-		if len(zones) == 0 || rerr != nil {
-			klog.Warningf("getRegionZonesMapWrapper: failed to fetch zones information: %v", rerr.Error())
+		zones, rerr = az.ZoneClient.GetZones(context.Background(), az.SubscriptionID)
+		if rerr != nil {
+			klog.Errorf("getRegionZonesMapWrapper: failed to fetch zones information: %v", rerr.Error())
 			return false, nil
 		}
 
-		az.updateRegionZonesMap(zones)
 		return true, nil
 	})
 
-	if err != nil {
-		return []string{}, fmt.Errorf("cannot get zones information of %s after %d time retry", region, az.RequestBackoff().Steps)
+	if errors.Is(err, wait.ErrWaitTimeout) {
+		return []string{}, rerr.Error()
 	}
 
-	az.refreshZonesLock.RLock()
-	defer az.refreshZonesLock.RUnlock()
+	az.updateRegionZonesMap(zones)
+
+	if len(az.regionZonesMap) != 0 {
+		az.refreshZonesLock.RLock()
+		defer az.refreshZonesLock.RUnlock()
+
+		return az.regionZonesMap[region], nil
+	}
 
-	return az.regionZonesMap[region], nil
+	return []string{}, nil
 }
 
 // makeZone returns the zone value in format of <region>-<zone-id>.

diff --git a/pkg/provider/azure_zones_test.go b/pkg/provider/azure_zones_test.go
@@ -23,6 +23,8 @@ import (
 	"net/http"
 	"testing"
 
+	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/zoneclient/mockzoneclient"
+
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-12-01/compute"
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/golang/mock/gomock"
@@ -32,6 +34,7 @@ import (
 	cloudprovider "k8s.io/cloud-provider"
 
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmclient/mockvmclient"
+	"sigs.k8s.io/cloud-provider-azure/pkg/retry"
 )
 
 const (
@@ -241,3 +244,77 @@ func TestAvailabilitySetGetZoneByNodeName(t *testing.T) {
 	assert.Equal(t, fmt.Errorf("node informer is not synced when trying to GetUnmanagedNodes"), err)
 	assert.Equal(t, cloudprovider.Zone{}, zone)
 }
+
+func TestSyncRegionZonesMap(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	for _, testCase := range []struct {
+		description   string
+		zones         map[string][]string
+		getZonesErr   *retry.Error
+		expectedZones map[string][]string
+		expectedErr   error
+	}{
+		{
+			description: "syncRegionZonesMap should report an error if failed to get zones",
+			getZonesErr: retry.NewError(false, fmt.Errorf("error")),
+			expectedErr: fmt.Errorf("Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: error"),
+		},
+		{
+			description:   "syncRegionZonesMap should not report an error if the given zones map is empty",
+			expectedZones: make(map[string][]string),
+		},
+	} {
+		t.Run(testCase.description, func(t *testing.T) {
+			az := &Cloud{
+				ZoneClient: mockzoneclient.NewMockInterface(ctrl),
+			}
+			mockZoneClient := az.ZoneClient.(*mockzoneclient.MockInterface)
+			mockZoneClient.EXPECT().GetZones(gomock.Any(), gomock.Any()).Return(testCase.expectedZones, testCase.getZonesErr)
+
+			err := az.syncRegionZonesMap()
+			if err != nil {
+				assert.EqualError(t, testCase.expectedErr, err.Error())
+			}
+			assert.Equal(t, testCase.expectedZones, az.regionZonesMap)
+		})
+	}
+}
+
+func TestGetRegionZonesBackoff(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	for _, testCase := range []struct {
+		description   string
+		zones         map[string][]string
+		getZonesErr   *retry.Error
+		expectedZones map[string][]string
+		expectedErr   error
+	}{
+		{
+			description: "syncRegionZonesMap should report an error if failed to get zones",
+			getZonesErr: retry.NewError(false, fmt.Errorf("error")),
+			expectedErr: fmt.Errorf("Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: error"),
+		},
+		{
+			description:   "syncRegionZonesMap should not report an error if the given zones map is empty",
+			expectedZones: make(map[string][]string),
+		},
+	} {
+		t.Run(testCase.description, func(t *testing.T) {
+			az := &Cloud{
+				ZoneClient: mockzoneclient.NewMockInterface(ctrl),
+			}
+			mockZoneClient := az.ZoneClient.(*mockzoneclient.MockInterface)
+			mockZoneClient.EXPECT().GetZones(gomock.Any(), gomock.Any()).Return(testCase.expectedZones, testCase.getZonesErr)
+
+			_, err := az.getRegionZonesBackoff("eastus2")
+			if err != nil {
+				assert.EqualError(t, testCase.expectedErr, err.Error())
+			}
+			assert.Equal(t, testCase.expectedZones, az.regionZonesMap)
+		})
+	}
+}