From a09bcb20c7c61f4ee167af41c1ff469862fd4013 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Thu, 16 Aug 2018 14:38:04 +0800 Subject: [PATCH 1/2] Reduce API calls for Azure instance metadata --- .../azure/azure_instance_metadata.go | 7 +++ .../providers/azure/azure_instances.go | 53 ++++++++++++------- .../providers/azure/azure_zones.go | 13 ++--- 3 files changed, 49 insertions(+), 24 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_instance_metadata.go b/pkg/cloudprovider/providers/azure/azure_instance_metadata.go index ee60c619845..c9cf6ce1c07 100644 --- a/pkg/cloudprovider/providers/azure/azure_instance_metadata.go +++ b/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{ diff --git a/pkg/cloudprovider/providers/azure/azure_instances.go b/pkg/cloudprovider/providers/azure/azure_instances.go index 804553c3268..3037f726ff5 100644 --- a/pkg/cloudprovider/providers/azure/azure_instances.go +++ b/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 } } } diff --git a/pkg/cloudprovider/providers/azure/azure_zones.go b/pkg/cloudprovider/providers/azure/azure_zones.go index e255bf8bae9..0d4195acc68 100644 --- a/pkg/cloudprovider/providers/azure/azure_zones.go +++ b/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{ From 680e64f6aec97d995179434d1c39d097002870b9 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Thu, 16 Aug 2018 14:38:30 +0800 Subject: [PATCH 2/2] Add unit tests for InstanceID --- pkg/cloudprovider/providers/azure/BUILD | 1 + .../providers/azure/azure_fakes.go | 7 ++ .../providers/azure/azure_instances_test.go | 115 ++++++++++++++++++ .../providers/azure/azure_test.go | 4 +- 4 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 pkg/cloudprovider/providers/azure/azure_instances_test.go diff --git a/pkg/cloudprovider/providers/azure/BUILD b/pkg/cloudprovider/providers/azure/BUILD index 1b9b3d44825..4999c921f65 100644 --- a/pkg/cloudprovider/providers/azure/BUILD +++ b/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", diff --git a/pkg/cloudprovider/providers/azure/azure_fakes.go b/pkg/cloudprovider/providers/azure/azure_fakes.go index 2434c5bb314..54d9d639659 100644 --- a/pkg/cloudprovider/providers/azure/azure_fakes.go +++ b/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 diff --git a/pkg/cloudprovider/providers/azure/azure_instances_test.go b/pkg/cloudprovider/providers/azure/azure_instances_test.go new file mode 100644 index 00000000000..106cb0f2767 --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index 8457ec92b7b..d2f4184eba8 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/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)