From 6bfd2be2eaee5f1841dbca2242125ae35bd29450 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Tue, 24 Jul 2018 14:24:27 +0800 Subject: [PATCH] Add documentation and unit tests --- pkg/cloudprovider/providers/azure/BUILD | 4 + .../azure/azure_managedDiskController.go | 57 ++++++----- .../providers/azure/azure_test.go | 3 + .../providers/azure/azure_zones_test.go | 73 ++++++++++++++ pkg/volume/azure_dd/BUILD | 2 + pkg/volume/azure_dd/azure_provision.go | 4 + pkg/volume/azure_dd/azure_provision_test.go | 96 +++++++++++++++++++ .../storage/persistentvolume/label/BUILD | 1 + .../persistentvolume/label/admission.go | 2 +- 9 files changed, 219 insertions(+), 23 deletions(-) create mode 100644 pkg/cloudprovider/providers/azure/azure_zones_test.go create mode 100644 pkg/volume/azure_dd/azure_provision_test.go diff --git a/pkg/cloudprovider/providers/azure/BUILD b/pkg/cloudprovider/providers/azure/BUILD index c2e97654b2..1b9b3d4482 100644 --- a/pkg/cloudprovider/providers/azure/BUILD +++ b/pkg/cloudprovider/providers/azure/BUILD @@ -40,6 +40,7 @@ go_library( "//pkg/cloudprovider:go_default_library", "//pkg/cloudprovider/providers/azure/auth:go_default_library", "//pkg/controller:go_default_library", + "//pkg/kubelet/apis:go_default_library", "//pkg/version:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/util:go_default_library", @@ -50,6 +51,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", + "//staging/src/k8s.io/client-go/informers:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", "//staging/src/k8s.io/client-go/util/flowcontrol:go_default_library", "//vendor/github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-04-01/compute:go_default_library", @@ -82,6 +84,7 @@ go_test( "azure_vmss_cache_test.go", "azure_vmss_test.go", "azure_wrap_test.go", + "azure_zones_test.go", ], embed = [":go_default_library"], deps = [ @@ -92,6 +95,7 @@ go_test( "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-04-01/compute:go_default_library", "//vendor/github.com/Azure/azure-sdk-for-go/services/network/mgmt/2017-09-01/network:go_default_library", "//vendor/github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2017-10-01/storage:go_default_library", diff --git a/pkg/cloudprovider/providers/azure/azure_managedDiskController.go b/pkg/cloudprovider/providers/azure/azure_managedDiskController.go index fd780ffece..d521d9e750 100644 --- a/pkg/cloudprovider/providers/azure/azure_managedDiskController.go +++ b/pkg/cloudprovider/providers/azure/azure_managedDiskController.go @@ -43,16 +43,27 @@ type ManagedDiskController struct { // ManagedDiskOptions specifies the options of managed disks. type ManagedDiskOptions struct { - DiskName string - SizeGB int - PVCName string - ResourceGroup string - Zoned bool - ZonePresent bool - ZonesPresent bool - AvailabilityZone string - AvailabilityZones string - Tags map[string]string + // The name of the disk. + DiskName string + // The size in GB. + SizeGB int + // The name of PVC. + PVCName string + // The name of resource group. + ResourceGroup string + // Wether the disk is zoned. + Zoned bool + // Wether AvailabilityZone is set. + ZonePresent bool + // Wether AvailabilityZones is set. + ZonesPresent bool + // The AvailabilityZone to create the disk. + AvailabilityZone string + // List of AvailabilityZone to create the disk. + AvailabilityZones string + // The tags of the disk. + Tags map[string]string + // The SKU of storage account. StorageAccountType storage.SkuName } @@ -90,6 +101,18 @@ func (c *ManagedDiskController) CreateManagedDisk(options *ManagedDiskOptions) ( zones = make(sets.String) zones.Insert(options.AvailabilityZone) } + var createZones *[]string + if len(zones.List()) > 0 { + createAZ := util.ChooseZoneForVolume(zones, options.PVCName) + // Do not allow creation of disks in zones that are do not have nodes. Such disks + // are not currently usable. + if !activeZones.Has(createAZ) { + return "", fmt.Errorf("kubernetes does not have a node in zone %q", createAZ) + } + + zoneList := []string{c.common.cloud.GetZoneID(createAZ)} + createZones = &zoneList + } // insert original tags to newTags newTags := make(map[string]*string) @@ -108,6 +131,7 @@ func (c *ManagedDiskController) CreateManagedDisk(options *ManagedDiskOptions) ( model := compute.Disk{ Location: &c.common.location, Tags: newTags, + Zones: createZones, Sku: &compute.DiskSku{ Name: compute.StorageAccountTypes(options.StorageAccountType), }, @@ -120,17 +144,6 @@ func (c *ManagedDiskController) CreateManagedDisk(options *ManagedDiskOptions) ( if options.ResourceGroup == "" { options.ResourceGroup = c.common.resourceGroup } - if len(zones.List()) > 0 { - createAZ := util.ChooseZoneForVolume(zones, options.PVCName) - // Do not allow creation of disks in zones that are do not have nodes. Such disks - // are not currently usable. - if !activeZones.Has(createAZ) { - return "", fmt.Errorf("kubernetes does not have a node in zone %q", createAZ) - } - - createZones := []string{c.common.cloud.GetZoneID(createAZ)} - model.Zones = &createZones - } ctx, cancel := getContextWithCancel() defer cancel() @@ -307,7 +320,7 @@ func (c *Cloud) GetAzureDiskLabels(diskURI string) (map[string]string, error) { } zone := c.makeZone(zoneID) - glog.V(4).Infof("Get zone %q for Azure disk %q", zone, diskName) + glog.V(4).Infof("Got zone %q for Azure disk %q", zone, diskName) labels := map[string]string{ kubeletapis.LabelZoneRegion: c.Location, kubeletapis.LabelZoneFailureDomain: zone, diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index d1267b9b19..45ff6ab5b2 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -32,6 +32,7 @@ import ( "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" serviceapi "k8s.io/kubernetes/pkg/api/v1/service" "k8s.io/kubernetes/pkg/cloudprovider/providers/azure/auth" kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" @@ -953,6 +954,8 @@ func getTestCloud() (az *Cloud) { MaximumLoadBalancerRuleCount: 250, VMType: vmTypeStandard, }, + nodeZones: map[string]sets.String{}, + nodeInformerSynced: func() bool { return true }, } az.DisksClient = newFakeDisksClient() az.InterfacesClient = newFakeAzureInterfacesClient() diff --git a/pkg/cloudprovider/providers/azure/azure_zones_test.go b/pkg/cloudprovider/providers/azure/azure_zones_test.go new file mode 100644 index 0000000000..92ac43d93b --- /dev/null +++ b/pkg/cloudprovider/providers/azure/azure_zones_test.go @@ -0,0 +1,73 @@ +/* +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 ( + "testing" +) + +func TestIsAvailabilityZone(t *testing.T) { + location := "eastus" + az := &Cloud{ + Config: Config{ + Location: location, + }, + } + tests := []struct { + desc string + zone string + expected bool + }{ + {"empty string should return false", "", false}, + {"wrong farmat should return false", "123", false}, + {"wrong location should return false", "chinanorth-1", false}, + {"correct zone should return true", "eastus-1", true}, + } + + for _, test := range tests { + actual := az.isAvailabilityZone(test.zone) + if actual != test.expected { + t.Errorf("test [%q] get unexpected result: %v != %v", test.desc, actual, test.expected) + } + } +} + +func TestGetZoneID(t *testing.T) { + location := "eastus" + az := &Cloud{ + Config: Config{ + Location: location, + }, + } + tests := []struct { + desc string + zone string + expected string + }{ + {"empty string should return empty string", "", ""}, + {"wrong farmat should return empty string", "123", ""}, + {"wrong location should return empty string", "chinanorth-1", ""}, + {"correct zone should return zone ID", "eastus-1", "1"}, + } + + for _, test := range tests { + actual := az.GetZoneID(test.zone) + if actual != test.expected { + t.Errorf("test [%q] get unexpected result: %q != %q", test.desc, actual, test.expected) + } + } +} diff --git a/pkg/volume/azure_dd/BUILD b/pkg/volume/azure_dd/BUILD index dd0fff9b99..f14aee0349 100644 --- a/pkg/volume/azure_dd/BUILD +++ b/pkg/volume/azure_dd/BUILD @@ -63,6 +63,7 @@ go_test( "azure_common_test.go", "azure_dd_block_test.go", "azure_dd_test.go", + "azure_provision_test.go", ], embed = [":go_default_library"], deps = [ @@ -73,5 +74,6 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/client-go/util/testing:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", ], ) diff --git a/pkg/volume/azure_dd/azure_provision.go b/pkg/volume/azure_dd/azure_provision.go index 9d1f8db388..42ec17e8fa 100644 --- a/pkg/volume/azure_dd/azure_provision.go +++ b/pkg/volume/azure_dd/azure_provision.go @@ -82,6 +82,10 @@ func parseZoned(zonedString string, kind v1.AzureDataDiskKind) (bool, error) { return false, fmt.Errorf("failed to parse 'zoned': %v", err) } + if zoned && kind != v1.AzureManagedDisk { + return false, fmt.Errorf("zoned is only supported by managed disks") + } + return zoned, nil } diff --git a/pkg/volume/azure_dd/azure_provision_test.go b/pkg/volume/azure_dd/azure_provision_test.go new file mode 100644 index 0000000000..0854c497ac --- /dev/null +++ b/pkg/volume/azure_dd/azure_provision_test.go @@ -0,0 +1,96 @@ +/* +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_dd + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/api/core/v1" +) + +func TestParseZoned(t *testing.T) { + tests := []struct { + msg string + zoneString string + diskKind v1.AzureDataDiskKind + expected bool + expectError bool + }{ + { + msg: "managed disk should default to zoned", + diskKind: v1.AzureManagedDisk, + expected: true, + }, + { + msg: "shared blob disk should default to un-zoned", + diskKind: v1.AzureSharedBlobDisk, + expected: false, + }, + { + msg: "shared dedicated disk should default to un-zoned", + diskKind: v1.AzureDedicatedBlobDisk, + expected: false, + }, + { + msg: "managed disk should support zoned=true", + diskKind: v1.AzureManagedDisk, + zoneString: "true", + expected: true, + }, + { + msg: "managed disk should support zoned=false", + diskKind: v1.AzureManagedDisk, + zoneString: "false", + expected: false, + }, + { + msg: "shared blob disk should support zoned=false", + diskKind: v1.AzureSharedBlobDisk, + zoneString: "false", + expected: false, + }, + { + msg: "shared blob disk shouldn't support zoned=true", + diskKind: v1.AzureSharedBlobDisk, + zoneString: "true", + expectError: true, + }, + { + msg: "shared dedicated disk should support zoned=false", + diskKind: v1.AzureDedicatedBlobDisk, + zoneString: "false", + expected: false, + }, + { + msg: "dedicated blob disk shouldn't support zoned=true", + diskKind: v1.AzureDedicatedBlobDisk, + zoneString: "true", + expectError: true, + }, + } + + for i, test := range tests { + real, err := parseZoned(test.zoneString, test.diskKind) + if test.expectError { + assert.Error(t, err, fmt.Sprintf("TestCase[%d]: %s", i, test.msg)) + } else { + assert.Equal(t, test.expected, real, fmt.Sprintf("TestCase[%d]: %s", i, test.msg)) + } + } +} diff --git a/plugin/pkg/admission/storage/persistentvolume/label/BUILD b/plugin/pkg/admission/storage/persistentvolume/label/BUILD index 2674d29ebd..cffd35726a 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/BUILD +++ b/plugin/pkg/admission/storage/persistentvolume/label/BUILD @@ -17,6 +17,7 @@ go_library( "//pkg/apis/core:go_default_library", "//pkg/cloudprovider:go_default_library", "//pkg/cloudprovider/providers/aws:go_default_library", + "//pkg/cloudprovider/providers/azure:go_default_library", "//pkg/cloudprovider/providers/gce:go_default_library", "//pkg/features:go_default_library", "//pkg/kubeapiserver/admission:go_default_library", diff --git a/plugin/pkg/admission/storage/persistentvolume/label/admission.go b/plugin/pkg/admission/storage/persistentvolume/label/admission.go index 64c48d68b7..4bbd16fee9 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission.go @@ -299,7 +299,7 @@ func (l *persistentVolumeLabel) getAzureCloudProvider() (*azure.Cloud, error) { azureProvider, ok := cloudProvider.(*azure.Cloud) if !ok { // GetCloudProvider has gone very wrong - return nil, fmt.Errorf("error retrieving GCE cloud provider") + return nil, fmt.Errorf("error retrieving Azure cloud provider") } l.azureProvider = azureProvider }