Eliminate unnecessary periodic VMSS updates with new bootstrap data

Author: nojnhuh

azure/const.go

@@ -46,10 +46,4 @@ const (
 	// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
 	// for annotation formatting rules.
 	SecurityRuleLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-azure-last-applied-security-rules"
-
-	// CustomDataHashAnnotation is the key for the machine object annotation
-	// which tracks the hash of the custom data.
-	// See https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
-	// for annotation formatting rules.
-	CustomDataHashAnnotation = "sigs.k8s.io/cluster-api-provider-azure-vmss-custom-data-hash"
 )

azure/scope/machinepool.go

@@ -18,11 +18,9 @@ package scope
 
 import (
 	"context"
-	"crypto/sha256"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
-	"io"
 	"strings"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2"
@@ -91,11 +89,10 @@ type (
 
 	// MachinePoolCache stores common machine pool information so we don't have to hit the API multiple times within the same reconcile loop.
 	MachinePoolCache struct {
-		BootstrapData           string
-		HasBootstrapDataChanges bool
-		VMImage                 *infrav1.Image
-		VMSKU                   resourceskus.SKU
-		MaxSurge                int
+		BootstrapData string
+		VMImage       *infrav1.Image
+		VMSKU         resourceskus.SKU
+		MaxSurge      int
 	}
 )
 
@@ -148,11 +145,6 @@ func (m *MachinePoolScope) InitMachinePoolCache(ctx context.Context) error {
 			return err
 		}
 
-		m.cache.HasBootstrapDataChanges, err = m.HasBootstrapDataChanges(ctx)
-		if err != nil {
-			return err
-		}
-
 		m.cache.VMImage, err = m.GetVMImage(ctx)
 		if err != nil {
 			return err
@@ -226,8 +218,6 @@ func (m *MachinePoolScope) ScaleSetSpec(ctx context.Context) azure.ResourceSpecG
 	}
 
 	if m.cache != nil {
-		spec.ShouldPatchCustomData = m.cache.HasBootstrapDataChanges
-		log.V(4).Info("has bootstrap data changed?", "shouldPatchCustomData", spec.ShouldPatchCustomData)
 		spec.VMSSExtensionSpecs = m.VMSSExtensionSpecs()
 		spec.SKU = m.cache.VMSKU
 		spec.VMImage = m.cache.VMImage
@@ -708,10 +698,6 @@ func (m *MachinePoolScope) Close(ctx context.Context) error {
 		if err := m.updateReplicasAndProviderIDs(ctx); err != nil {
 			return errors.Wrap(err, "failed to update replicas and providerIDs")
 		}
-		if err := m.updateCustomDataHash(ctx); err != nil {
-			// ignore errors to calculating the custom data hash since it's not absolutely crucial.
-			log.V(4).Error(err, "unable to update custom data hash, ignoring.")
-		}
 	}
 
 	if err := m.PatchObject(ctx); err != nil {
@@ -745,39 +731,6 @@ func (m *MachinePoolScope) GetBootstrapData(ctx context.Context) (string, error)
 	return base64.StdEncoding.EncodeToString(value), nil
 }
 
-// calculateBootstrapDataHash calculates the sha256 hash of the bootstrap data.
-func (m *MachinePoolScope) calculateBootstrapDataHash(_ context.Context) (string, error) {
-	if m.cache == nil {
-		return "", fmt.Errorf("machinepool cache is nil")
-	}
-	bootstrapData := m.cache.BootstrapData
-	h := sha256.New()
-	n, err := io.WriteString(h, bootstrapData)
-	if err != nil || n == 0 {
-		return "", fmt.Errorf("unable to write custom data (bytes written: %q): %w", n, err)
-	}
-	return fmt.Sprintf("%x", h.Sum(nil)), nil
-}
-
-// HasBootstrapDataChanges calculates the sha256 hash of the bootstrap data and compares it with the saved hash in AzureMachinePool.Status.
-func (m *MachinePoolScope) HasBootstrapDataChanges(ctx context.Context) (bool, error) {
-	newHash, err := m.calculateBootstrapDataHash(ctx)
-	if err != nil {
-		return false, err
-	}
-	return m.AzureMachinePool.GetAnnotations()[azure.CustomDataHashAnnotation] != newHash, nil
-}
-
-// updateCustomDataHash calculates the sha256 hash of the bootstrap data and saves it in AzureMachinePool.Status.
-func (m *MachinePoolScope) updateCustomDataHash(ctx context.Context) error {
-	newHash, err := m.calculateBootstrapDataHash(ctx)
-	if err != nil {
-		return err
-	}
-	m.SetAnnotation(azure.CustomDataHashAnnotation, newHash)
-	return nil
-}
-
 // GetVMImage picks an image from the AzureMachinePool configuration, or uses a default one.
 func (m *MachinePoolScope) GetVMImage(ctx context.Context) (*infrav1.Image, error) {
 	ctx, log, done := tele.StartSpanWithLogger(ctx, "scope.MachinePoolScope.GetVMImage")

azure/scope/machinepool_test.go

@@ -18,17 +18,13 @@ package scope
 
 import (
 	"context"
-	"crypto/sha256"
-	"encoding/base64"
 	"fmt"
-	"io"
 	"reflect"
 	"testing"
 	"time"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2"
-	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5"
 	azureautorest "github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/azure/auth"
 	. "github.com/onsi/gomega"
@@ -1750,148 +1746,3 @@ func TestMachinePoolScope_setProvisioningStateAndConditions(t *testing.T) {
 		})
 	}
 }
-
-func TestBootstrapDataChanges(t *testing.T) {
-	ctx, cancel := context.WithCancel(t.Context())
-	defer cancel()
-	scheme := runtime.NewScheme()
-	_ = clusterv1.AddToScheme(scheme)
-	_ = infrav1.AddToScheme(scheme)
-	_ = infrav1exp.AddToScheme(scheme)
-	_ = corev1.AddToScheme(scheme)
-
-	var (
-		g        = NewWithT(t)
-		mockCtrl = gomock.NewController(t)
-		cb       = fake.NewClientBuilder().WithScheme(scheme)
-		cluster  = &clusterv1.Cluster{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "cluster1",
-				Namespace: "default",
-			},
-			Spec: clusterv1.ClusterSpec{
-				InfrastructureRef: &corev1.ObjectReference{
-					Name: "azCluster1",
-				},
-			},
-			Status: clusterv1.ClusterStatus{
-				InfrastructureReady: true,
-			},
-		}
-		azureCluster = &infrav1.AzureCluster{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "azCluster1",
-				Namespace: "default",
-			},
-			Spec: infrav1.AzureClusterSpec{
-				AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
-					Location: "test",
-				},
-			},
-		}
-		mp = &expv1.MachinePool{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "mp1",
-				Namespace: "default",
-				OwnerReferences: []metav1.OwnerReference{
-					{
-						Name:       "cluster1",
-						Kind:       "Cluster",
-						APIVersion: clusterv1.GroupVersion.String(),
-					},
-				},
-			},
-			Spec: expv1.MachinePoolSpec{
-				Template: clusterv1.MachineTemplateSpec{
-					Spec: clusterv1.MachineSpec{
-						Bootstrap: clusterv1.Bootstrap{
-							DataSecretName: ptr.To("mp-secret"),
-						},
-						Version: ptr.To("v1.31.0"),
-					},
-				},
-			},
-		}
-		bootstrapData     = "test"
-		bootstrapDataHash = sha256Hash(base64.StdEncoding.EncodeToString([]byte(bootstrapData)))
-		bootstrapSecret   = corev1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "mp-secret",
-				Namespace: "default",
-			},
-			Data: map[string][]byte{"value": []byte(bootstrapData)},
-		}
-		amp = &infrav1exp.AzureMachinePool{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "amp1",
-				Namespace: "default",
-				OwnerReferences: []metav1.OwnerReference{
-					{
-						Name:       "mp1",
-						Kind:       "MachinePool",
-						APIVersion: expv1.GroupVersion.String(),
-					},
-				},
-				Annotations: map[string]string{
-					azure.CustomDataHashAnnotation: fmt.Sprintf("%x", bootstrapDataHash),
-				},
-			},
-			Spec: infrav1exp.AzureMachinePoolSpec{
-				Template: infrav1exp.AzureMachinePoolMachineTemplate{
-					Image: &infrav1.Image{},
-					NetworkInterfaces: []infrav1.NetworkInterface{
-						{
-							SubnetName: "test",
-						},
-					},
-					VMSize: "VM_SIZE",
-				},
-			},
-		}
-		vmssState = &azure.VMSS{}
-	)
-	defer mockCtrl.Finish()
-
-	s := &MachinePoolScope{
-		client: cb.
-			WithObjects(&bootstrapSecret).
-			Build(),
-		ClusterScoper: &ClusterScope{
-			Cluster:      cluster,
-			AzureCluster: azureCluster,
-		},
-		skuCache: resourceskus.NewStaticCache([]armcompute.ResourceSKU{
-			{
-				Name: ptr.To("VM_SIZE"),
-			},
-		}, "test"),
-		MachinePool:      mp,
-		AzureMachinePool: amp,
-		vmssState:        vmssState,
-	}
-
-	g.Expect(s.InitMachinePoolCache(ctx)).NotTo(HaveOccurred())
-
-	spec := s.ScaleSetSpec(ctx)
-	sSpec := spec.(*scalesets.ScaleSetSpec)
-	g.Expect(sSpec.ShouldPatchCustomData).To(BeFalse())
-
-	amp.Annotations[azure.CustomDataHashAnnotation] = "old"
-
-	// reset cache to be able to build up the cache again
-	s.cache = nil
-	g.Expect(s.InitMachinePoolCache(ctx)).NotTo(HaveOccurred())
-
-	spec = s.ScaleSetSpec(ctx)
-	sSpec = spec.(*scalesets.ScaleSetSpec)
-	g.Expect(sSpec.ShouldPatchCustomData).To(BeTrue())
-}
-
-func sha256Hash(text string) []byte {
-	h := sha256.New()
-	_, err := io.WriteString(h, text)
-	if err != nil {
-		panic(err)
-	}
-	return h.Sum(nil)
-}

azure/services/scalesets/scalesets_test.go

@@ -813,12 +813,14 @@ func newDefaultWindowsVMSS() armcompute.VirtualMachineScaleSet {
 
 func newDefaultVMSS(vmSize string) armcompute.VirtualMachineScaleSet {
 	dataDisk := fetchDataDiskBasedOnSize(vmSize)
+	bootstrapData := "fake-bootstrap-data"
 	return armcompute.VirtualMachineScaleSet{
 		Location: ptr.To("test-location"),
 		Tags: map[string]*string{
 			"Name": ptr.To(defaultVMSSName),
 			"sigs.k8s.io_cluster-api-provider-azure_cluster_my-cluster": ptr.To("owned"),
 			"sigs.k8s.io_cluster-api-provider-azure_role":               ptr.To("node"),
+			customDataHashTagName:                                       ptr.To(mustCalculateBootstrapDataHash(bootstrapData)),
 		},
 		SKU: &armcompute.SKU{
 			Name:     ptr.To(vmSize),
@@ -837,7 +839,7 @@ func newDefaultVMSS(vmSize string) armcompute.VirtualMachineScaleSet {
 				OSProfile: &armcompute.VirtualMachineScaleSetOSProfile{
 					ComputerNamePrefix: ptr.To(defaultVMSSName),
 					AdminUsername:      ptr.To(azure.DefaultUserName),
-					CustomData:         ptr.To("fake-bootstrap-data"),
+					CustomData:         ptr.To(bootstrapData),
 					LinuxConfiguration: &armcompute.LinuxConfiguration{
 						SSH: &armcompute.SSHConfiguration{
 							PublicKeys: []*armcompute.SSHPublicKey{

azure/services/scalesets/spec.go

@@ -18,8 +18,10 @@ package scalesets
 
 import (
 	"context"
+	"crypto/sha256"
 	"encoding/base64"
 	"fmt"
+	"io"
 	"strconv"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5"
@@ -34,6 +36,8 @@ import (
 	"sigs.k8s.io/cluster-api-provider-azure/util/tele"
 )
 
+const customDataHashTagName = "capz-custom-data-hash"
+
 // ScaleSetSpec defines the specification for a Scale Set.
 type ScaleSetSpec struct {
 	Name                         string
@@ -70,7 +74,6 @@ type ScaleSetSpec struct {
 	VMSSInstances                []armcompute.VirtualMachineScaleSetVM
 	MaxSurge                     int
 	ClusterName                  string
-	ShouldPatchCustomData        bool
 	HasReplicasExternallyManaged bool
 	AdditionalTags               infrav1.Tags
 	PlatformFaultDomainCount     *int32
@@ -129,19 +132,22 @@ func (s *ScaleSetSpec) existingParameters(ctx context.Context, existing interfac
 	}
 
 	// If there are no model changes and no increase in the replica count, do not update the VMSS.
-	// Decreases in replica count is handled by deleting AzureMachinePoolMachine instances in the MachinePoolScope
-	if *vmss.SKU.Capacity <= existingInfraVMSS.Capacity && !hasModelChanges && !s.ShouldPatchCustomData {
+	// Decreases in replica count is handled by deleting AzureMachinePoolMachine instances in the MachinePoolScope.
+	// Bootstrap data is allowed to get stale here and will be updated alongside changes to the model or
+	// replica count which require fresh bootstrap data.
+	if *vmss.SKU.Capacity <= existingInfraVMSS.Capacity && !hasModelChanges {
 		// up to date, nothing to do
 		return nil, nil
 	}
 
 	// if there are no model changes and no change in custom data, remove VirtualMachineProfile to avoid unnecessary VMSS model
 	// updates.
-	if !hasModelChanges && !s.ShouldPatchCustomData {
-		log.V(4).Info("removing virtual machine profile from parameters", "hasModelChanges", hasModelChanges, "shouldPatchCustomData", s.ShouldPatchCustomData)
+	shouldPatchCustomData := ptr.Deref(existingVMSS.Tags[customDataHashTagName], "") != ptr.Deref(vmss.Tags[customDataHashTagName], "")
+	if !hasModelChanges && !shouldPatchCustomData {
+		log.V(4).Info("removing virtual machine profile from parameters", "hasModelChanges", hasModelChanges, "shouldPatchCustomData", shouldPatchCustomData)
 		vmss.Properties.VirtualMachineProfile = nil
 	} else {
-		log.V(4).Info("has changes, not removing virtual machine profile from parameters", "hasModelChanges", hasModelChanges, "shouldPatchCustomData", s.ShouldPatchCustomData)
+		log.V(4).Info("has changes, not removing virtual machine profile from parameters", "hasModelChanges", hasModelChanges, "shouldPatchCustomData", shouldPatchCustomData)
 	}
 
 	return vmss, nil
@@ -293,6 +299,15 @@ func (s *ScaleSetSpec) Parameters(ctx context.Context, existing interface{}) (pa
 	})
 
 	vmss.Tags = converters.TagsToMap(tags)
+
+	// Custom data is not returned in GET responses, so we store a hash to detect when it changes. This allows
+	// CAPZ to update the VMSS model only when necessary.
+	customDataHash, err := calculateBootstrapDataHash(s.BootstrapData)
+	if err != nil {
+		return armcompute.VirtualMachineScaleSet{}, err
+	}
+	vmss.Tags[customDataHashTagName] = ptr.To(customDataHash)
+
 	return vmss, nil
 }
 
@@ -547,3 +562,13 @@ func (s *ScaleSetSpec) getSecurityProfile() (*armcompute.SecurityProfile, error)
 		EncryptionAtHost: ptr.To(*s.SecurityProfile.EncryptionAtHost),
 	}, nil
 }
+
+// calculateBootstrapDataHash calculates the sha256 hash of the bootstrap data.
+func calculateBootstrapDataHash(bootstrapData string) (string, error) {
+	h := sha256.New()
+	n, err := io.WriteString(h, bootstrapData)
+	if err != nil || n == 0 {
+		return "", fmt.Errorf("unable to write custom data (bytes written: %q): %w", n, err)
+	}
+	return fmt.Sprintf("%x", h.Sum(nil)), nil
+}