Enable upgrade of Kubernetes clusters with master VMs using managed disks (#1008)

Introduction of Kubernetes master VMs to start using managed disks both for OS and etcddisk broke upgrade operation:
Upgrade operation was supported on unmanaged disks only and making manageddisk the default StoragrProfile broke upgrade operation OOB.
Switch to using managed disk also started using a default disk name to be assigned by Disk RP (vs. one assigned by ACS Engine like for other resources). This could be problematic in many ways: 
With main one being relying on and understanding DiskRP’s naming convention to discover the right etcd disk for each master VM. This might not be a big issue in the RP because disk names/ids can be saved in the database but from ACS Engine standpoint for operations to be idempotent disk names need to be deterministic.
This also adds unnecessary complexity of loading and editing the template during upgrade with the disk name generated by Disk RP.

This PR adds support to enable upgrade of clusters using managed disk master VMs.
The code has been updated to use a deterministic name for etcd disks. However, any cluster created between June 22nd to until this PR gets merged still gets non-deterministic names for etcd disks.
Change upgrade template to have a managedDisk section when attaching an existing etcddisk
Pending fixes:
Supported attaching of auto generated etcd disks (names) during upgrade.

glide.lock

@@ -1,5 +1,5 @@
-hash: 73d446bde305bc460c8eda98525b92ae41ec1676a34643d34d274311b4b62bae
-updated: 2017-07-14T14:44:35.503228239-07:00
+hash: 47bdadf4225410bca8b7b7ad7e6512555c80a60db5ec90d874a0ea2ca876b3e5
+updated: 2017-07-15T01:36:15.006608-07:00
 imports:
 - name: github.com/alexcesaro/statsd
   version: 7fea3f0d2fab1ad973e641e51dba45443a311a90
@@ -8,6 +8,7 @@ imports:
   subpackages:
   - arm/authorization
   - arm/compute
+  - arm/disk
   - arm/graphrbac
   - arm/network
   - arm/resources/resources

glide.yaml

@@ -8,6 +8,7 @@ import:
   - arm/resources/resources
   - arm/resources/subscriptions
   - arm/storage
+  - arm/disk
   - storage
 - package: github.com/Azure/go-autorest
   version: ^8.0.0

parts/kubernetesmasterresources.t

@@ -435,8 +435,8 @@
               "createOption": "Empty"
               ,"diskSizeGB": "128"
               ,"lun": 0
-          {{if .MasterProfile.IsStorageAccount}}
               ,"name": "[concat(variables('masterVMNamePrefix'), copyIndex(variables('masterOffset')),'-etcddisk')]"
+          {{if .MasterProfile.IsStorageAccount}}
               ,"vhd": {
                 "uri": "[concat(reference(concat('Microsoft.Storage/storageAccounts/',variables('masterStorageAccountName')),variables('apiVersionStorage')).primaryEndpoints.blob,'vhds/', variables('masterVMNamePrefix'),copyIndex(variables('masterOffset')),'-etcddisk.vhd')]"
               }

pkg/acsengine/transform.go

@@ -5,6 +5,7 @@ import (
 	"log"
 	"strings"
 
+	"github.com/Azure/azure-sdk-for-go/arm/compute"
 	"github.com/Sirupsen/logrus"
 )
 
@@ -25,6 +26,7 @@ const (
 	dataDisksFieldName             = "dataDisks"
 	createOptionFieldName          = "createOption"
 	tagsFieldName                  = "tags"
+	managedDiskFieldName           = "managedDisk"
 
 	// ARM resource Types
 	nsgResourceType  = "Microsoft.Network/networkSecurityGroups"
@@ -199,7 +201,7 @@ func removeImageReference(logger *logrus.Entry, resourceProperties map[string]in
 }
 
 // NormalizeResourcesForK8sMasterUpgrade takes a template and removes elements that are unwanted in any scale up/down case
-func NormalizeResourcesForK8sMasterUpgrade(logger *logrus.Entry, templateMap map[string]interface{}, agentPoolsToPreserve map[string]bool) error {
+func NormalizeResourcesForK8sMasterUpgrade(logger *logrus.Entry, templateMap map[string]interface{}, isMasterManagedDisk bool, agentPoolsToPreserve map[string]bool) error {
 	resources := templateMap[resourcesFieldName].([]interface{})
 	logger.Infoln(fmt.Sprintf("Resource count before running NormalizeResourcesForK8sMasterUpgrade: %d", len(resources)))
 
@@ -246,6 +248,15 @@ func NormalizeResourcesForK8sMasterUpgrade(logger *logrus.Entry, templateMap map
 			dataDisks := storageProfile[dataDisksFieldName].([]interface{})
 			dataDisk, ok := dataDisks[0].(map[string]interface{})
 			dataDisk[createOptionFieldName] = "attach"
+
+			if isMasterManagedDisk {
+				managedDisk := compute.ManagedDiskParameters{}
+				id := "[concat('/subscriptions/', variables('subscriptionId'), '/resourceGroups/', variables('resourceGroup'),'/providers/Microsoft.Compute/disks/', variables('masterVMNamePrefix'), copyIndex(variables('masterOffset')),'-etcddisk')]"
+				managedDisk.ID = &id
+				var diskInterface interface{}
+				diskInterface = &managedDisk
+				dataDisk[managedDiskFieldName] = diskInterface
+			}
 		}
 
 		tags, ok := resourceMap[tagsFieldName].(map[string]interface{})
@@ -298,9 +309,9 @@ func NormalizeResourcesForK8sMasterUpgrade(logger *logrus.Entry, templateMap map
 }
 
 // NormalizeResourcesForK8sAgentUpgrade takes a template and removes elements that are unwanted in any scale up/down case
-func NormalizeResourcesForK8sAgentUpgrade(logger *logrus.Entry, templateMap map[string]interface{}, agentPoolsToPreserve map[string]bool) error {
+func NormalizeResourcesForK8sAgentUpgrade(logger *logrus.Entry, templateMap map[string]interface{}, isMasterManagedDisk bool, agentPoolsToPreserve map[string]bool) error {
 	logger.Infoln(fmt.Sprintf("Running NormalizeResourcesForK8sMasterUpgrade...."))
-	if err := NormalizeResourcesForK8sMasterUpgrade(logger, templateMap, agentPoolsToPreserve); err != nil {
+	if err := NormalizeResourcesForK8sMasterUpgrade(logger, templateMap, isMasterManagedDisk, agentPoolsToPreserve); err != nil {
 		log.Fatalln(err)
 		return err
 	}

pkg/armhelpers/azureclient.go

@@ -27,6 +27,7 @@ import (
 	"github.com/mitchellh/go-homedir"
 
 	"github.com/Azure/acs-engine/pkg/acsengine"
+	"github.com/Azure/azure-sdk-for-go/arm/disk"
 )
 
 const (
@@ -60,6 +61,7 @@ type AzureClient struct {
 	subscriptionsClient           subscriptions.GroupClient
 	virtualMachinesClient         compute.VirtualMachinesClient
 	virtualMachineScaleSetsClient compute.VirtualMachineScaleSetsClient
+	disksClient                   disk.DisksClient
 
 	applicationsClient      graphrbac.ApplicationsClient
 	servicePrincipalsClient graphrbac.ServicePrincipalsClient
@@ -264,6 +266,7 @@ func getClient(env azure.Environment, subscriptionID, tenantID string, armSpt *a
 		providersClient:               resources.NewProvidersClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
 		virtualMachinesClient:         compute.NewVirtualMachinesClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
 		virtualMachineScaleSetsClient: compute.NewVirtualMachineScaleSetsClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
+		disksClient:                   disk.NewDisksClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
 
 		applicationsClient:      graphrbac.NewApplicationsClientWithBaseURI(env.GraphEndpoint, tenantID),
 		servicePrincipalsClient: graphrbac.NewServicePrincipalsClientWithBaseURI(env.GraphEndpoint, tenantID),
@@ -280,6 +283,7 @@ func getClient(env azure.Environment, subscriptionID, tenantID string, armSpt *a
 	c.providersClient.Authorizer = authorizer
 	c.virtualMachinesClient.Authorizer = authorizer
 	c.virtualMachineScaleSetsClient.Authorizer = authorizer
+	c.disksClient.Authorizer = authorizer
 
 	c.deploymentsClient.PollingDelay = time.Second * 5
 	c.resourcesClient.PollingDelay = time.Second * 5
@@ -370,6 +374,7 @@ func (az *AzureClient) AddAcceptLanguages(languages []string) {
 	az.providersClient.ManagementClient.Client.RequestInspector = az.addAcceptLanguages()
 	az.virtualMachinesClient.ManagementClient.Client.RequestInspector = az.addAcceptLanguages()
 	az.virtualMachineScaleSetsClient.ManagementClient.Client.RequestInspector = az.addAcceptLanguages()
+	az.disksClient.ManagementClient.Client.RequestInspector = az.addAcceptLanguages()
 
 	az.applicationsClient.ManagementClient.Client.RequestInspector = az.addAcceptLanguages()
 	az.servicePrincipalsClient.ManagementClient.Client.RequestInspector = az.addAcceptLanguages()

pkg/armhelpers/disk.go

@@ -0,0 +1,13 @@
+package armhelpers
+
+import "github.com/Azure/azure-sdk-for-go/arm/disk"
+
+// DeleteManagedDisk deletes a managed disk.
+func (az *AzureClient) DeleteManagedDisk(resourceGroupName string, diskName string, cancel <-chan struct{}) (<-chan disk.OperationStatusResponse, <-chan error) {
+	return az.disksClient.Delete(resourceGroupName, diskName, cancel)
+}
+
+// ListManagedDisksByResourceGroup lists managed disks in a resource group.
+func (az *AzureClient) ListManagedDisksByResourceGroup(resourceGroupName string) (result disk.ListType, err error) {
+	return az.disksClient.ListByResourceGroup(resourceGroupName)
+}

pkg/armhelpers/interfaces.go

@@ -3,6 +3,7 @@ package armhelpers
 import (
 	"github.com/Azure/azure-sdk-for-go/arm/authorization"
 	"github.com/Azure/azure-sdk-for-go/arm/compute"
+	"github.com/Azure/azure-sdk-for-go/arm/disk"
 	"github.com/Azure/azure-sdk-for-go/arm/graphrbac"
 	"github.com/Azure/azure-sdk-for-go/arm/resources/resources"
 	"github.com/Azure/go-autorest/autorest"
@@ -65,6 +66,10 @@ type ACSEngineClient interface {
 	// RBAC
 	CreateRoleAssignment(scope string, roleAssignmentName string, parameters authorization.RoleAssignmentCreateParameters) (authorization.RoleAssignment, error)
 	CreateRoleAssignmentSimple(applicationID, roleID string) error
+
+	// MANAGED DISKS
+	DeleteManagedDisk(resourceGroupName string, diskName string, cancel <-chan struct{}) (<-chan disk.OperationStatusResponse, <-chan error)
+	ListManagedDisksByResourceGroup(resourceGroupName string) (result disk.ListType, err error)
 }
 
 // ACSStorageClient interface models the azure storage client

pkg/armhelpers/mockclients.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/Azure/azure-sdk-for-go/arm/authorization"
 	"github.com/Azure/azure-sdk-for-go/arm/compute"
+	"github.com/Azure/azure-sdk-for-go/arm/disk"
 	"github.com/Azure/azure-sdk-for-go/arm/graphrbac"
 	"github.com/Azure/azure-sdk-for-go/arm/resources/resources"
 	"github.com/Azure/go-autorest/autorest"
@@ -260,3 +261,25 @@ func (mc *MockACSEngineClient) CreateRoleAssignment(scope string, roleAssignment
 func (mc *MockACSEngineClient) CreateRoleAssignmentSimple(applicationID, roleID string) error {
 	return nil
 }
+
+// DeleteManagedDisk is a wrapper around disksClient.Delete
+func (mc *MockACSEngineClient) DeleteManagedDisk(resourceGroupName string, diskName string, cancel <-chan struct{}) (<-chan disk.OperationStatusResponse, <-chan error) {
+	errChan := make(chan error)
+	respChan := make(chan disk.OperationStatusResponse)
+	go func() {
+		defer func() {
+			close(errChan)
+		}()
+		defer func() {
+			close(respChan)
+		}()
+		errChan <- nil
+		respChan <- disk.OperationStatusResponse{}
+	}()
+	return respChan, errChan
+}
+
+// ListManagedDisksByResourceGroup is a wrapper around disksClient.ListManagedDisksByResourceGroup
+func (mc *MockACSEngineClient) ListManagedDisksByResourceGroup(resourceGroupName string) (result disk.ListType, err error) {
+	return disk.ListType{}, nil
+}

pkg/operations/deletevm.go

@@ -16,16 +16,15 @@ func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry,
 		return err
 	}
 
-	// NOTE: This code assumes a non-managed disk!
 	vhd := vm.VirtualMachineProperties.StorageProfile.OsDisk.Vhd
-	if vhd == nil {
-		logger.Warnf("found an OS Disk with no VHD URI. This is probably a VM with a managed disk")
+	managedDisk := vm.VirtualMachineProperties.StorageProfile.OsDisk.ManagedDisk
+	if vhd == nil && managedDisk == nil {
+		logger.Errorf("failed to get a valid os disk URI for VM: %s/%s", resourceGroup, name)
+
 		return fmt.Errorf("os disk does not have a VHD URI")
 	}
-	accountName, vhdContainer, vhdBlob, err := armhelpers.SplitBlobURI(*vhd.URI)
-	if err != nil {
-		return err
-	}
+
+	osDiskName := vm.VirtualMachineProperties.StorageProfile.OsDisk.Name
 
 	nicID := (*vm.VirtualMachineProperties.NetworkProfile.NetworkInterfaces)[0].ID
 	nicName, err := armhelpers.ResourceName(*nicID)
@@ -33,17 +32,11 @@ func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry,
 		return err
 	}
 
-	logger.Infof("found os disk storage reference: %s %s %s", accountName, vhdContainer, vhdBlob)
 	logger.Infof("found nic name for VM (%s/%s): %s", resourceGroup, name, nicName)
 
 	logger.Infof("deleting VM: %s/%s", resourceGroup, name)
 	_, deleteErrChan := az.DeleteVirtualMachine(resourceGroup, name, nil)
 
-	as, err := az.GetStorageClient(resourceGroup, accountName)
-	if err != nil {
-		return err
-	}
-
 	logger.Infof("waiting for vm deletion: %s/%s", resourceGroup, name)
 	if err := <-deleteErrChan; err != nil {
 		return err
@@ -51,6 +44,21 @@ func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry,
 
 	logger.Infof("deleting nic: %s/%s", resourceGroup, nicName)
 	_, nicErrChan := az.DeleteNetworkInterface(resourceGroup, nicName, nil)
+
+	logger.Infof("waiting for nic deletion: %s/%s", resourceGroup, nicName)
+	if nicErr := <-nicErrChan; nicErr != nil {
+		return nicErr
+	}
+
+	if vhd != nil {
+		accountName, vhdContainer, vhdBlob, err := armhelpers.SplitBlobURI(*vhd.URI)
+		if err != nil {
+			return err
+		}
+
+		logger.Infof("found os disk storage reference: %s %s %s", accountName, vhdContainer, vhdBlob)
+
+		as, err := az.GetStorageClient(resourceGroup, accountName)
 		if err != nil {
 			return err
 		}
@@ -59,10 +67,13 @@ func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry,
 		if err = as.DeleteBlob(vhdContainer, vhdBlob); err != nil {
 			return err
 		}
+	} else if managedDisk != nil {
+		logger.Infof("deleting managed disk: %s/%s", resourceGroup, *osDiskName)
+		_, diskErrChan := az.DeleteManagedDisk(resourceGroup, *osDiskName, nil)
 
-	logger.Infof("waiting for nic deletion: %s/%s", resourceGroup, nicName)
-	if nicErr := <-nicErrChan; nicErr != nil {
-		return nicErr
+		if err := <-diskErrChan; err != nil {
+			return err
+		}
 	}
 
 	return nil

pkg/operations/kubernetesupgrade/upgradecluster.go

@@ -47,6 +47,9 @@ type UpgradeCluster struct {
 	UpgradeModel *api.UpgradeContainerService
 }
 
+// MasterVMNamePrefix is the prefix for all master VM names for Kubernetes clusters
+const MasterVMNamePrefix = "k8s-master-"
+
 // UpgradeCluster runs the workflow to upgrade a Kubernetes cluster.
 // UpgradeContainerService contains target state of the cluster that
 // the operation will drive towards.
@@ -105,7 +108,7 @@ func (uc *UpgradeCluster) getClusterNodeStatus(subscriptionID uuid.UUID, resourc
 
 		vmOrchestratorTypeAndVersion := *(*vm.Tags)["orchestrator"]
 		if vmOrchestratorTypeAndVersion == orchestratorTypeVersion {
-			if strings.Contains(*(vm.Name), "k8s-master-") {
+			if strings.Contains(*(vm.Name), MasterVMNamePrefix) {
 				if !strings.Contains(*(vm.Name), uc.NameSuffix) {
 					log.Infoln(fmt.Sprintf("Skipping VM: %s for upgrade as it does not belong to cluster with expected name suffix: %s",
 						*vm.Name, uc.NameSuffix))
@@ -122,7 +125,7 @@ func (uc *UpgradeCluster) getClusterNodeStatus(subscriptionID uuid.UUID, resourc
 					*vm.Name, uc.NameSuffix))
 				continue
 			}
-			if strings.Contains(*(vm.Name), "k8s-master-") {
+			if strings.Contains(*(vm.Name), MasterVMNamePrefix) {
 				log.Infoln(fmt.Sprintf("Master VM name: %s, orchestrator: %s (UpgradedMasterVMs)", *vm.Name, vmOrchestratorTypeAndVersion))
 				*uc.UpgradedMasterVMs = append(*uc.UpgradedMasterVMs, vm)
 			} else {

pkg/operations/kubernetesupgrade/v162upgrader.go

@@ -58,6 +58,7 @@ func (ku *Kubernetes162upgrader) Validate() error {
 }
 
 func (ku *Kubernetes162upgrader) upgradeMasterNodes() error {
+	log.Infoln(fmt.Sprintf("Master nodes StorageProfile: %s", ku.GoalStateDataModel.Properties.MasterProfile.StorageProfile))
 	// Upgrade Master VMs
 	templateMap, parametersMap, err := ku.generateUpgradeTemplate(ku.GoalStateDataModel)
 	if err != nil {
@@ -66,7 +67,7 @@ func (ku *Kubernetes162upgrader) upgradeMasterNodes() error {
 
 	log.Infoln(fmt.Sprintf("Prepping master nodes for upgrade..."))
 
-	if err := acsengine.NormalizeResourcesForK8sMasterUpgrade(log.NewEntry(log.New()), templateMap, nil); err != nil {
+	if err := acsengine.NormalizeResourcesForK8sMasterUpgrade(log.NewEntry(log.New()), templateMap, ku.DataModel.Properties.MasterProfile.IsManagedDisks(), nil); err != nil {
 		log.Fatalln(err)
 		return err
 	}
@@ -177,7 +178,7 @@ func (ku *Kubernetes162upgrader) upgradeAgentPools() error {
 		log.Infoln(fmt.Sprintf("Prepping agent pool: %s for upgrade...", *agentPool.Name))
 
 		preservePools := map[string]bool{*agentPool.Name: true}
-		if err := acsengine.NormalizeResourcesForK8sAgentUpgrade(log.NewEntry(log.New()), templateMap, preservePools); err != nil {
+		if err := acsengine.NormalizeResourcesForK8sAgentUpgrade(log.NewEntry(log.New()), templateMap, ku.DataModel.Properties.MasterProfile.IsManagedDisks(), preservePools); err != nil {
 			log.Fatalln(err)
 			return err
 		}