feat: rework AKS machine/VM name handling

Author: comtalyst

pkg/cloudprovider/cloudprovider.go

@@ -358,36 +358,26 @@ func (c *CloudProvider) Get(ctx context.Context, providerID string) (*karpv1.Nod
 	}
 	ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("vmName", vmName))
 
-	aksMachinesPoolName := options.FromContext(ctx).AKSMachinesPoolName
-	if aksMachineName, err := instance.GetAKSMachineNameFromVMName(aksMachinesPoolName, vmName); err == nil {
-		// This could be an AKS machine-based node; try getting the AKS machine instance
+	if aksMachineName, err := instance.GetAKSMachineNameFromVMName(options.FromContext(ctx).AKSMachinesPoolName, vmName); err == nil {
+		// AKS machine-based node
 		ctx := log.IntoContext(ctx, log.FromContext(ctx).WithValues("aksMachineName", aksMachineName))
 
 		aksMachine, err := c.aksMachineInstanceProvider.Get(ctx, aksMachineName)
-		if err == nil {
-			nodeClaim, err := c.resolveNodeClaimFromAKSMachine(ctx, aksMachine)
-			if err != nil {
-				return nil, fmt.Errorf("converting AKS machine instance to node claim, %w", err)
-			}
-			return nodeClaim, nil
-		} else if cloudprovider.IgnoreNodeClaimNotFoundError(err) != nil {
+		if err != nil {
 			return nil, fmt.Errorf("getting AKS machine instance, %w", err)
 		}
-		// Fallback to legacy VM-based node if not found
-		// In the case that it is indeed AKS machine node, but somehow fail GET AKS machine and succeeded GET VM, ideally we want this call to fail.
-		// However, being misrepresented only once is not fatal. "Illegal" in-place update will be reconciled back to the before, and there is no drift for VM nodes that won't happen with AKS machine nodes + DriftAction.
-		// So, we could live with this for now.
-	}
-
-	vm, err := c.vmInstanceProvider.Get(ctx, vmName)
-	if err != nil {
-		return nil, fmt.Errorf("getting VM instance, %w", err)
-	}
-	instanceType, err := c.resolveInstanceTypeFromVMInstance(ctx, vm)
-	if err != nil {
-		return nil, fmt.Errorf("resolving instance type, %w", err)
+		return c.resolveNodeClaimFromAKSMachine(ctx, aksMachine)
+	} else {
+		vm, err := c.vmInstanceProvider.Get(ctx, vmName)
+		if err != nil {
+			return nil, fmt.Errorf("getting VM instance, %w", err)
+		}
+		instanceType, err := c.resolveInstanceTypeFromVMInstance(ctx, vm)
+		if err != nil {
+			return nil, fmt.Errorf("resolving instance type, %w", err)
+		}
+		return c.vmInstanceToNodeClaim(ctx, vm, instanceType)
 	}
-	return c.vmInstanceToNodeClaim(ctx, vm, instanceType)
 }
 
 func (c *CloudProvider) LivenessProbe(req *http.Request) error {

pkg/cloudprovider/suite_aksmachineapi_integration_test.go

@@ -161,7 +161,7 @@ var _ = Describe("CloudProvider", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(corecloudprovider.IsNodeClaimNotFoundError(err)).To(BeTrue())
 			Expect(azureEnv.AKSMachinesAPI.AKSMachineGetBehavior.CalledWithInput.Len()).To(Equal(1))
-			Expect(azureEnv.VirtualMachinesAPI.VirtualMachineGetBehavior.CalledWithInput.Len()).To(Equal(1)) // Should be bothered as AKS machine is not found, so suspect this to be a VM
+			Expect(azureEnv.VirtualMachinesAPI.VirtualMachineGetBehavior.CalledWithInput.Len()).To(Equal(0)) // Should not be bothered
 			Expect(nodeClaim).To(BeNil())
 		})
 

pkg/fake/aksmachinesapi.go

@@ -506,9 +506,9 @@ func (c *AKSMachinesAPI) setDefaultMachineValues(machine *armcontainerservice.Ma
 	}
 
 	// Set ResourceID - simulates VM resource ID
-	// vmName = aks-<machinesPoolName>-<aksMachineName>-########-vm#
+	// vmName = aks-<machinesPoolName>-<aksMachineName>-########-vm
 	if machine.Properties.ResourceID == nil {
-		vmName := fmt.Sprintf("aks-%s-%s-12345678-vm0", agentPoolName, *machine.Name)
+		vmName := fmt.Sprintf("aks-%s-%s-12345678-vm", agentPoolName, *machine.Name)
 		vmResourceID := fmt.Sprintf("/subscriptions/subscriptionID/resourceGroups/test-resourceGroup/providers/Microsoft.Compute/virtualMachines/%s", vmName)
 		machine.Properties.ResourceID = lo.ToPtr(vmResourceID)
 	}

pkg/providers/instance/aksmachineinstance.go

@@ -35,6 +35,7 @@ import (
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/imagefamily"
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/instance/offerings"
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/instancetype"
+	"github.com/Azure/karpenter-provider-azure/pkg/providers/launchtemplate"
 	"github.com/Azure/karpenter-provider-azure/pkg/utils"
 )
 
@@ -174,7 +175,10 @@ func (p *DefaultAKSMachineProvider) BeginCreate(
 	nodeClaim *karpv1.NodeClaim,
 	instanceTypes []*corecloudprovider.InstanceType,
 ) (*AKSMachinePromise, error) {
-	aksMachineName := GetAKSMachineNameFromNodeClaimName(nodeClaim.Name)
+	aksMachineName, err := GetAKSMachineNameFromNodeClaimName(nodeClaim.Name)
+	if err != nil {
+		return nil, fmt.Errorf("failed to generate AKS machine name from NodeClaim name %q: %w", nodeClaim.Name, err)
+	}
 	instanceTypes = offerings.OrderInstanceTypesByPrice(instanceTypes, scheduling.NewNodeSelectorRequirementsWithMinValues(nodeClaim.Spec.Requirements...))
 
 	aksMachinePromise, err := p.beginCreateMachine(ctx, nodeClass, nodeClaim, instanceTypes, aksMachineName)
@@ -376,7 +380,7 @@ func (p *DefaultAKSMachineProvider) beginCreateMachine(
 	existingAKSMachine, err := p.getMachine(ctx, aksMachineName)
 	if err == nil {
 		// Existing AKS machine found, reuse it.
-		return p.reuseExistingMachine(ctx, aksMachineName, nodeClass, instanceTypes, existingAKSMachine)
+		return p.reuseExistingMachine(ctx, aksMachineName, nodeClass, nodeClaim, instanceTypes, existingAKSMachine)
 	} else if !IsAKSMachineOrMachinesPoolNotFound(err) {
 		// Not fatal. Will fall back to normal creation.
 		log.FromContext(ctx).Error(err, "failed to check for existing AKS machine", "aksMachineName", aksMachineName)
@@ -502,11 +506,17 @@ func (p *DefaultAKSMachineProvider) handleMachineProvisioningError(ctx context.C
 	return fmt.Errorf("failed to create AKS machine %q during %s, unhandled provisioning error: code=%s, message=%s", aksMachineName, phase, lo.FromPtr(innerError.Code), lo.FromPtr(innerError.Message))
 }
 
-func (p *DefaultAKSMachineProvider) reuseExistingMachine(ctx context.Context, aksMachineName string, nodeClass *v1beta1.AKSNodeClass, instanceTypes []*corecloudprovider.InstanceType, existingAKSMachine *armcontainerservice.Machine) (*AKSMachinePromise, error) {
+func (p *DefaultAKSMachineProvider) reuseExistingMachine(ctx context.Context, aksMachineName string, nodeClass *v1beta1.AKSNodeClass, nodeClaim *karpv1.NodeClaim, instanceTypes []*corecloudprovider.InstanceType, existingAKSMachine *armcontainerservice.Machine) (*AKSMachinePromise, error) {
 	// Reconstruct properties from existing AKS machine instance.
 	if err := validateRetrievedAKSMachineBasicProperties(existingAKSMachine); err != nil {
 		return nil, fmt.Errorf("found existing AKS machine %s, but %w", aksMachineName, err)
 	}
+	if existingAKSMachine.Properties.Tags == nil || existingAKSMachine.Properties.Tags[launchtemplate.KarpenterAKSMachineNodeClaimTagKey] == nil {
+		// This is not included in validateRetrievedAKSMachineBasicProperties as inplaceupdate can repair it.
+		// Although, we don't want to reuse a machine until that happens.
+		return nil, fmt.Errorf("found existing AKS machine %s, but %w", aksMachineName, fmt.Errorf("irretrievable karpenter.azure.com_aksmachine_nodeclaim tag"))
+	}
+
 	var existingAKSMachineZone string
 	if len(existingAKSMachine.Zones) == 0 || existingAKSMachine.Zones[0] == nil {
 		existingAKSMachineZone = "" // No zone
@@ -518,11 +528,18 @@ func (p *DefaultAKSMachineProvider) reuseExistingMachine(ctx context.Context, ak
 	existingAKSMachineVMResourceID := lo.FromPtr(existingAKSMachine.Properties.ResourceID)
 	existingAKSMachineID := lo.FromPtr(existingAKSMachine.ID)
 	existingAKSMachineNodeImageVersion := lo.FromPtr(existingAKSMachine.Properties.NodeImageVersion)
+	existingAKSMachineNodeClaimName := lo.FromPtr(existingAKSMachine.Properties.Tags[launchtemplate.KarpenterAKSMachineNodeClaimTagKey])
 
 	instanceType := offerings.GetInstanceTypeFromVMSize(existingAKSMachineVMSize, instanceTypes)
 	capacityType := GetCapacityTypeFromAKSScaleSetPriority(existingAKSMachinePriority)
 	zone := utils.GetAKSLabelZoneFromARMZone(p.aksMachinesPoolLocation, existingAKSMachineZone)
 
+	if existingAKSMachineNodeClaimName != nodeClaim.Name {
+		// Might be possible from NodePool name hash collision within AKS machine name
+		// See how AKS machine name is generated for more details.
+		// ASSUMPTION: repeated failure will eventually result in NodeClaim reaching registration TTL, then gets re-created with the new hash, recovering from the collision.
+		return nil, fmt.Errorf("found existing AKS machine %s, but its karpenter.azure.com_aksmachine_nodeclaim tag %q does not match the NodeClaim to create %q", aksMachineName, existingAKSMachineNodeClaimName, nodeClaim.Name)
+	}
 	if existingAKSMachine.Properties.ProvisioningState != nil && lo.FromPtr(existingAKSMachine.Properties.ProvisioningState) == "Failed" {
 		// Unfortunately, that was more like a remain than a usable aksMachine.
 		// ASSUMPTION: this is irrecoverable (i.e., polling would have failed).

pkg/providers/instance/aksmachineinstanceutils.go

@@ -100,7 +100,6 @@ func BuildNodeClaimFromAKSMachineTemplate(
 		// By the time of writing, this is being used for logging purposes within provider only.
 		// That is unlikely to change for core. But be mindful of provider is to rely on this in that situation. Still, rare.
 		// This was less of a concern for VM instance as NodeClaim name is always inferrable from instance name.
-		// XPMT: TODO(Bryce-Soghigian): see if you have better ideas. Otherwise just delete this TODO and live with this.
 		nodeClaim.Name = *tag
 	}
 	nodeClaim.Labels = labels
@@ -182,17 +181,36 @@ func FindNodePoolFromAKSMachine(ctx context.Context, aksMachine *armcontainerser
 	return nil, errors.NewNotFound(schema.GroupResource{Group: coreapis.Group, Resource: "nodepools"}, "")
 }
 
-// XPMT: TODO(Bryce-Soghigian): rework this thing below? Also consider add acceptance tests on this in cloudprovider module, if applicable.
-// Real node name would be aks-<machinesPoolName>-<aksMachineName>-########-vm#. E.g., aks-aksmanagedap-default-2jf98-11274290-vm2.
-func GetAKSMachineNameFromNodeClaimName(nodeClaimName string) string {
-	// ASSUMPTION: all AKS machines are named after the NodeClaim name.
-	// Does not guarantee that the NodeClaim is already associated with an AKS machine.
-	// This assumption is weaker than the one in GetAKSMachineNameFromNodeClaim(), but still, not breaking anytime soon.
-	// return nodeClaimName
+// ASSUMPTION: NodeClaim name is in the format of <NodePool name>-<hash suffix>
+// If total length exceeds AKS machine name limit, the exceeded part will be replaced with another deterministic hash.
+// E.g., "thisisalongnodepoolname-a1b2c" --> "thisisalongnoz9y8x7-a1b2c"
+func GetAKSMachineNameFromNodeClaimName(nodeClaimName string) (string, error) {
+	const maxAKSMachineNameLength = 35 // Defined by AKS machine API.
+	const prefixHashLength = 6         // The length of the hashed part replacing the exceeded part of the prefix.
+	// If 6, given alphanumeric hash, there will be a total of 36^6 = 2,176,782,336 combinations.
 
-	// TEMPORARY
+	if len(nodeClaimName) <= maxAKSMachineNameLength {
+		// Safe to use the whole name
+		return nodeClaimName, nil
+	}
 	splitted := strings.Split(nodeClaimName, "-")
-	return "x" + splitted[len(splitted)-1]
+	// Combine the parts except the last one (NodeClaim hash suffix)
+	prefix := strings.Join(splitted[:len(splitted)-1], "-")
+	suffix := "-" + splitted[len(splitted)-1]
+
+	// Keep the legit part of the prefix intact, but hash the rest
+	// ASSUMPTION: prefix length is at least 6 characters at this point (which means suffix length is not too large)
+	// At the time of writing, suffix length is 6 (e.g., "-a1b2c"). This is unlikely to change.
+	keepPrefixLength := maxAKSMachineNameLength - len(suffix) - prefixHashLength
+	prefixToKeep := prefix[:keepPrefixLength]
+	prefixToHash := prefix[keepPrefixLength:]
+	hashedPrefixToHash, err := utils.GetAlphanumericHash(prefixToHash, 6)
+	if err != nil {
+		return "", fmt.Errorf("failed to hash exceeded AKS machine name prefix %q: %w", prefixToHash, err)
+	}
+
+	hashTrimmedPrefix := prefixToKeep + hashedPrefixToHash
+	return hashTrimmedPrefix + suffix, nil
 }
 
 // GetAKSMachineNameFromNodeClaim extracts the AKS machine name from the NodeClaim annotations
@@ -212,10 +230,8 @@ func GetCapacityTypeFromAKSScaleSetPriority(scaleSetPriority armcontainerservice
 	return AKSScaleSetPriorityToKarpCapacityType[scaleSetPriority]
 }
 
-// vmName = aks-<machinesPoolName>-<aksMachineName>-########-vm#
-// Note: there is no accurate way to tell from the VM name that the VM is created by AKS machine API.
-// E.g., a non-AKS machine VM from Karpenter nodepool named "aksmanagedap-aks-nodepool-abcde-12345678" with suffix "vms12" would result in VM name "aks-aksmanagedap-aks-nodepool-abcde-12345678-vm12", which can be interpreted as a AKS machine with pool name "aksmanagedap" and name "aks-nodepool-abcde".
-// The validation below is, thus, best-effort.
+// vmName = aks-<machinesPoolName>-<aksMachineName>-########-vm
+// This is distinguishable from VM instance name as its suffix will always be 5 alphanumerics rather than "vm"
 func GetAKSMachineNameFromVMName(aksMachinesPoolName, vmName string) (string, error) {
 	if !strings.HasPrefix(vmName, "aks-"+aksMachinesPoolName+"-") {
 		return "", fmt.Errorf("vm name %s does not start with expected prefix aks-%s-", vmName, aksMachinesPoolName)
@@ -226,11 +242,11 @@ func GetAKSMachineNameFromVMName(aksMachinesPoolName, vmName string) (string, er
 	if len(splitted) < 3 {
 		return "", fmt.Errorf("vm name %s does not have enough parts after prefix aks-%s-", vmName, aksMachinesPoolName)
 	}
-	// Check whether the last part starts with "vm"
-	if !strings.HasPrefix(splitted[len(splitted)-1], "vm") {
-		return "", fmt.Errorf("vm name %s does not end with expected suffix vm#", vmName)
+	// Check whether the last part is "vm"
+	if splitted[len(splitted)-1] != "vm" {
+		return "", fmt.Errorf("vm name %s does not end with expected suffix vm", vmName)
 	}
-	// Remove the last two parts (########-vm#) and join the rest to get the AKS machine name
+	// Remove the last two parts (########-vm) and join the rest to get the AKS machine name
 	aksMachineName := strings.Join(splitted[:len(splitted)-2], "-")
 
 	return aksMachineName, nil