Merge pull request #67478 from feiskyer/zone-fix

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

 Reduce API calls for Azure instance metadata

**What this PR does / why we need it**:

Azure cloud provider gets a lot of `"Too many requests"` error when getting availability zones, instance types and node addresses. Hence kubelet won't be able to initialize itself sometimes.

This PR reduces such calls and alos switches to json API which is more stable.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes https://github.com/Azure/acs-engine/issues/3681

**Special notes for your reviewer**:

**Release note**:

```release-note
Reduce API calls for Azure instance metadata.
```

cc @ritazh @khenidak @andyzhangx

pkg/cloudprovider/providers/azure/BUILD

@@ -74,6 +74,7 @@ go_test(
     srcs = [
         "azure_backoff_test.go",
         "azure_cache_test.go",
+        "azure_instances_test.go",
         "azure_loadbalancer_test.go",
         "azure_metrics_test.go",
         "azure_routes_test.go",

pkg/cloudprovider/providers/azure/azure_fakes.go

@@ -302,6 +302,13 @@ func (fVMC *fakeAzureVirtualMachinesClient) List(ctx context.Context, resourceGr
 	return result, nil
 }
 
+func (fVMC *fakeAzureVirtualMachinesClient) setFakeStore(store map[string]map[string]compute.VirtualMachine) {
+	fVMC.mutex.Lock()
+	defer fVMC.mutex.Unlock()
+
+	fVMC.FakeStore = store
+}
+
 type fakeAzureSubnetsClient struct {
 	mutex     *sync.Mutex
 	FakeStore map[string]map[string]network.Subnet

pkg/cloudprovider/providers/azure/azure_instance_metadata.go

@@ -59,6 +59,13 @@ type InstanceMetadata struct {
 	baseURL string
 }
 
+// ComputeMetadata represents compute information
+type ComputeMetadata struct {
+	Name   string `json:"name,omitempty"`
+	Zone   string `json:"zone,omitempty"`
+	VMSize string `json:"vmSize,omitempty"`
+}
+
 // NewInstanceMetadata creates an instance of the InstanceMetadata accessor object.
 func NewInstanceMetadata() *InstanceMetadata {
 	return &InstanceMetadata{

pkg/cloudprovider/providers/azure/azure_instances.go

@@ -51,7 +51,12 @@ func (az *Cloud) NodeAddresses(ctx context.Context, name types.NodeName) ([]v1.N
 	}
 
 	if az.UseInstanceMetadata {
-		isLocalInstance, err := az.isCurrentInstance(name)
+		computeMetadata, err := az.getComputeMetadata()
+		if err != nil {
+			return nil, err
+		}
+
+		isLocalInstance, err := az.isCurrentInstance(name, computeMetadata.Name)
 		if err != nil {
 			return nil, err
 		}
@@ -119,23 +124,30 @@ func (az *Cloud) InstanceShutdownByProviderID(ctx context.Context, providerID st
 	return false, cloudprovider.NotImplemented
 }
 
-func (az *Cloud) isCurrentInstance(name types.NodeName) (bool, error) {
-	nodeName := mapNodeNameToVMName(name)
-	metadataName, err := az.metadata.Text("instance/compute/name")
+// getComputeMetadata gets compute information from instance metadata.
+func (az *Cloud) getComputeMetadata() (*ComputeMetadata, error) {
+	computeInfo := ComputeMetadata{}
+	err := az.metadata.Object(computeMetadataURI, &computeInfo)
 	if err != nil {
-		return false, err
+		return nil, err
 	}
 
+	return &computeInfo, nil
+}
+
+func (az *Cloud) isCurrentInstance(name types.NodeName, metadataVMName string) (bool, error) {
+	var err error
+	nodeName := mapNodeNameToVMName(name)
 	if az.VMType == vmTypeVMSS {
 		// VMSS vmName is not same with hostname, use hostname instead.
-		metadataName, err = os.Hostname()
+		metadataVMName, err = os.Hostname()
 		if err != nil {
 			return false, err
 		}
 	}
 
-	metadataName = strings.ToLower(metadataName)
-	return (metadataName == nodeName), err
+	metadataVMName = strings.ToLower(metadataVMName)
+	return (metadataVMName == nodeName), err
 }
 
 // InstanceID returns the cloud provider ID of the specified instance.
@@ -144,7 +156,12 @@ func (az *Cloud) InstanceID(ctx context.Context, name types.NodeName) (string, e
 	nodeName := mapNodeNameToVMName(name)
 
 	if az.UseInstanceMetadata {
-		isLocalInstance, err := az.isCurrentInstance(name)
+		computeMetadata, err := az.getComputeMetadata()
+		if err != nil {
+			return "", err
+		}
+
+		isLocalInstance, err := az.isCurrentInstance(name, computeMetadata.Name)
 		if err != nil {
 			return "", err
 		}
@@ -160,11 +177,7 @@ func (az *Cloud) InstanceID(ctx context.Context, name types.NodeName) (string, e
 		}
 
 		// Get scale set name and instanceID from vmName for vmss.
-		metadataName, err := az.metadata.Text("instance/compute/name")
-		if err != nil {
-			return "", err
-		}
-		ssName, instanceID, err := extractVmssVMName(metadataName)
+		ssName, instanceID, err := extractVmssVMName(computeMetadata.Name)
 		if err != nil {
 			if err == ErrorNotVmssInstance {
 				// Compose machineID for standard Node.
@@ -197,14 +210,18 @@ func (az *Cloud) InstanceTypeByProviderID(ctx context.Context, providerID string
 //       Adding node label from cloud provider: beta.kubernetes.io/instance-type=[value]
 func (az *Cloud) InstanceType(ctx context.Context, name types.NodeName) (string, error) {
 	if az.UseInstanceMetadata {
-		isLocalInstance, err := az.isCurrentInstance(name)
+		computeMetadata, err := az.getComputeMetadata()
+		if err != nil {
+			return "", err
+		}
+
+		isLocalInstance, err := az.isCurrentInstance(name, computeMetadata.Name)
 		if err != nil {
 			return "", err
 		}
 		if isLocalInstance {
-			machineType, err := az.metadata.Text("instance/compute/vmSize")
-			if err == nil {
-				return machineType, nil
+			if computeMetadata.VMSize != "" {
+				return computeMetadata.VMSize, nil
 			}
 		}
 	}

pkg/cloudprovider/providers/azure/azure_instances_test.go

@@ -0,0 +1,115 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package azure
+
+import (
+	"context"
+	"fmt"
+	"net"
+	"net/http"
+	"testing"
+
+	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-04-01/compute"
+	"k8s.io/apimachinery/pkg/types"
+)
+
+func setTestVirtualMachines(c *Cloud, vmList []string) {
+	virtualMachineClient := c.VirtualMachinesClient.(*fakeAzureVirtualMachinesClient)
+	store := map[string]map[string]compute.VirtualMachine{
+		"rg": make(map[string]compute.VirtualMachine),
+	}
+
+	for i := range vmList {
+		nodeName := vmList[i]
+		instanceID := fmt.Sprintf("/subscriptions/script/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/%s", nodeName)
+		store["rg"][nodeName] = compute.VirtualMachine{
+			Name:     &nodeName,
+			ID:       &instanceID,
+			Location: &c.Location,
+		}
+	}
+
+	virtualMachineClient.setFakeStore(store)
+}
+
+func TestInstanceID(t *testing.T) {
+	cloud := getTestCloud()
+	cloud.metadata = &InstanceMetadata{}
+
+	testcases := []struct {
+		name         string
+		vmList       []string
+		nodeName     string
+		metadataName string
+		expected     string
+		expectError  bool
+	}{
+		{
+			name:         "InstanceID should get instanceID if node's name are equal to metadataName",
+			vmList:       []string{"vm1"},
+			nodeName:     "vm1",
+			metadataName: "vm1",
+			expected:     "/subscriptions/script/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/vm1",
+		},
+		{
+			name:         "InstanceID should get instanceID from Azure API if node is not local instance",
+			vmList:       []string{"vm2"},
+			nodeName:     "vm2",
+			metadataName: "vm1",
+			expected:     "/subscriptions/script/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/vm2",
+		},
+		{
+			name:        "InstanceID should report error if VM doesn't exist",
+			vmList:      []string{"vm1"},
+			nodeName:    "vm3",
+			expectError: true,
+		},
+	}
+
+	for _, test := range testcases {
+		listener, err := net.Listen("tcp", "127.0.0.1:0")
+		if err != nil {
+			t.Errorf("Test [%s] unexpected error: %v", test.name, err)
+		}
+
+		mux := http.NewServeMux()
+		mux.Handle("/instance/compute", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprintf(w, fmt.Sprintf("{\"name\":\"%s\"}", test.metadataName))
+		}))
+		go func() {
+			http.Serve(listener, mux)
+		}()
+		defer listener.Close()
+
+		cloud.metadata.baseURL = "http://" + listener.Addr().String() + "/"
+		setTestVirtualMachines(cloud, test.vmList)
+		instanceID, err := cloud.InstanceID(context.Background(), types.NodeName(test.nodeName))
+		if test.expectError {
+			if err == nil {
+				t.Errorf("Test [%s] unexpected nil err", test.name)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("Test [%s] unexpected error: %v", test.name, err)
+			}
+		}
+
+		if instanceID != test.expected {
+			t.Errorf("Test [%s] unexpected instanceID: %s, expected %q", test.name, instanceID, test.expected)
+		}
+	}
+}

pkg/cloudprovider/providers/azure/azure_test.go

@@ -1715,8 +1715,8 @@ func TestGetZone(t *testing.T) {
 		mux.Handle("/v1/InstanceInfo/FD", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			fmt.Fprintf(w, test.faultDomain)
 		}))
-		mux.Handle("/instance/compute/zone", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			fmt.Fprintf(w, test.zone)
+		mux.Handle("/instance/compute", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprintf(w, fmt.Sprintf("{\"zone\":\"%s\"}", test.zone))
 		}))
 		go func() {
 			http.Serve(listener, mux)

pkg/cloudprovider/providers/azure/azure_zones.go

@@ -29,8 +29,8 @@ import (
 )
 
 const (
-	faultDomainURI  = "v1/InstanceInfo/FD"
-	zoneMetadataURI = "instance/compute/zone"
+	faultDomainURI     = "v1/InstanceInfo/FD"
+	computeMetadataURI = "instance/compute"
 )
 
 var faultMutex = &sync.Mutex{}
@@ -58,19 +58,20 @@ func (az *Cloud) GetZoneID(zoneLabel string) string {
 // GetZone returns the Zone containing the current availability zone and locality region that the program is running in.
 // If the node is not running with availability zones, then it will fall back to fault domain.
 func (az *Cloud) GetZone(ctx context.Context) (cloudprovider.Zone, error) {
-	zone, err := az.metadata.Text(zoneMetadataURI)
+	computeInfo := ComputeMetadata{}
+	err := az.metadata.Object(computeMetadataURI, &computeInfo)
 	if err != nil {
 		return cloudprovider.Zone{}, err
 	}
 
-	if zone == "" {
+	if computeInfo.Zone == "" {
 		glog.V(3).Infof("Availability zone is not enabled for the node, falling back to fault domain")
 		return az.getZoneFromFaultDomain()
 	}
 
-	zoneID, err := strconv.Atoi(zone)
+	zoneID, err := strconv.Atoi(computeInfo.Zone)
 	if err != nil {
-		return cloudprovider.Zone{}, fmt.Errorf("failed to parse zone ID %q: %v", zone, err)
+		return cloudprovider.Zone{}, fmt.Errorf("failed to parse zone ID %q: %v", computeInfo.Zone, err)
 	}
 
 	return cloudprovider.Zone{