Skip to content

Commit 06f39bd

Browse files
author
Michael Weibel
committed
fix(machinepool): clear VirtualMachineProfile if no model/custom data update
fixes #4958 Scale up/down with MachinePool always updates the VM image model to use. This change sets the VirtualMachineProfile to nil when no change is necessary and ensures therefore less churn on scale up/downs leading to model updates which may require manual fixing
1 parent 4fec820 commit 06f39bd

File tree

3 files changed

+93
-32
lines changed

3 files changed

+93
-32
lines changed

‎azure/scope/machinepool.go‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -222,10 +222,8 @@ func (m *MachinePoolScope) ScaleSetSpec(ctx context.Context) azure.ResourceSpecG
222222
}
223223

224224
if m.cache != nil {
225-
if m.HasReplicasExternallyManaged(ctx) {
226-
spec.ShouldPatchCustomData = m.cache.HasBootstrapDataChanges
227-
log.V(4).Info("has bootstrap data changed?", "shouldPatchCustomData", spec.ShouldPatchCustomData)
228-
}
225+
spec.ShouldPatchCustomData = m.cache.HasBootstrapDataChanges
226+
log.V(4).Info("has bootstrap data changed?", "shouldPatchCustomData", spec.ShouldPatchCustomData)
229227
spec.VMSSExtensionSpecs = m.VMSSExtensionSpecs()
230228
spec.SKU = m.cache.VMSKU
231229
spec.VMImage = m.cache.VMImage
@@ -705,11 +703,9 @@ func (m *MachinePoolScope) Close(ctx context.Context) error {
705703
if err := m.updateReplicasAndProviderIDs(ctx); err != nil {
706704
return errors.Wrap(err, "failed to update replicas and providerIDs")
707705
}
708-
if m.HasReplicasExternallyManaged(ctx) {
709-
if err := m.updateCustomDataHash(ctx); err != nil {
710-
// ignore errors to calculating the custom data hash since it's not absolutely crucial.
711-
log.V(4).Error(err, "unable to update custom data hash, ignoring.")
712-
}
706+
if err := m.updateCustomDataHash(ctx); err != nil {
707+
// ignore errors to calculating the custom data hash since it's not absolutely crucial.
708+
log.V(4).Error(err, "unable to update custom data hash, ignoring.")
713709
}
714710
}
715711

‎azure/services/scalesets/spec.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,9 @@ func (s *ScaleSetSpec) OwnerResourceName() string {
9393
}
9494

9595
func (s *ScaleSetSpec) existingParameters(ctx context.Context, existing interface{}) (parameters interface{}, err error) {
96+
ctx, log, done := tele.StartSpanWithLogger(ctx, "scalesets.ScaleSetSpec.existingParameters")
97+
defer done()
98+
9699
existingVMSS, ok := existing.(armcompute.VirtualMachineScaleSet)
97100
if !ok {
98101
return nil, errors.Errorf("%T is not an armcompute.VirtualMachineScaleSet", existing)
@@ -132,6 +135,15 @@ func (s *ScaleSetSpec) existingParameters(ctx context.Context, existing interfac
132135
return nil, nil
133136
}
134137

138+
// if there are no model changes and no change in custom data, remove VirtualMachineProfile to avoid unnecessary VMSS model
139+
// updates.
140+
if !hasModelChanges && !s.ShouldPatchCustomData {
141+
log.V(4).Info("removing virtual machine profile from parameters", "hasModelChanges", hasModelChanges, "shouldPatchCustomData", s.ShouldPatchCustomData)
142+
vmss.Properties.VirtualMachineProfile = nil
143+
} else {
144+
log.V(4).Info("has changes, not removing virtual machine profile from parameters", "hasModelChanges", hasModelChanges, "shouldPatchCustomData", s.ShouldPatchCustomData)
145+
}
146+
135147
return vmss, nil
136148
}
137149

‎azure/services/scalesets/spec_test.go‎

Lines changed: 76 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -33,26 +33,28 @@ import (
3333
)
3434

3535
var (
36-
defaultSpec, defaultVMSS = getDefaultVMSS()
37-
windowsSpec, windowsVMSS = getDefaultWindowsVMSS()
38-
acceleratedNetworkingSpec, acceleratedNetworkingVMSS = getAcceleratedNetworkingVMSS()
39-
customSubnetSpec, customSubnetVMSS = getCustomSubnetVMSS()
40-
customNetworkingSpec, customNetworkingVMSS = getCustomNetworkingVMSS()
41-
spotVMSpec, spotVMVMSS = getSpotVMVMSS()
42-
ephemeralSpec, ephemeralVMSS = getEPHVMSSS()
43-
resourceDiskSpec, resourceDiskVMSS = getResourceDiskVMSS()
44-
evictionSpec, evictionVMSS = getEvictionPolicyVMSS()
45-
maxPriceSpec, maxPriceVMSS = getMaxPriceVMSS()
46-
encryptionSpec, encryptionVMSS = getEncryptionVMSS()
47-
userIdentitySpec, userIdentityVMSS = getUserIdentityVMSS()
48-
hostEncryptionSpec, hostEncryptionVMSS = getHostEncryptionVMSS()
49-
hostEncryptionUnsupportedSpec = getHostEncryptionUnsupportedSpec()
50-
ephemeralReadSpec, ephemeralReadVMSS = getEphemeralReadOnlyVMSS()
51-
defaultExistingSpec, defaultExistingVMSS, defaultExistingVMSSClone = getExistingDefaultVMSS()
52-
userManagedStorageAccountDiagnosticsSpec, userManagedStorageAccountDiagnosticsVMSS = getUserManagedAndStorageAcccountDiagnosticsVMSS()
53-
managedDiagnosticsSpec, managedDiagnoisticsVMSS = getManagedDiagnosticsVMSS()
54-
disabledDiagnosticsSpec, disabledDiagnosticsVMSS = getDisabledDiagnosticsVMSS()
55-
nilDiagnosticsProfileSpec, nilDiagnosticsProfileVMSS = getNilDiagnosticsProfileVMSS()
36+
defaultSpec, defaultVMSS = getDefaultVMSS()
37+
windowsSpec, windowsVMSS = getDefaultWindowsVMSS()
38+
acceleratedNetworkingSpec, acceleratedNetworkingVMSS = getAcceleratedNetworkingVMSS()
39+
customSubnetSpec, customSubnetVMSS = getCustomSubnetVMSS()
40+
customNetworkingSpec, customNetworkingVMSS = getCustomNetworkingVMSS()
41+
spotVMSpec, spotVMVMSS = getSpotVMVMSS()
42+
ephemeralSpec, ephemeralVMSS = getEPHVMSSS()
43+
resourceDiskSpec, resourceDiskVMSS = getResourceDiskVMSS()
44+
evictionSpec, evictionVMSS = getEvictionPolicyVMSS()
45+
maxPriceSpec, maxPriceVMSS = getMaxPriceVMSS()
46+
encryptionSpec, encryptionVMSS = getEncryptionVMSS()
47+
userIdentitySpec, userIdentityVMSS = getUserIdentityVMSS()
48+
hostEncryptionSpec, hostEncryptionVMSS = getHostEncryptionVMSS()
49+
hostEncryptionUnsupportedSpec = getHostEncryptionUnsupportedSpec()
50+
ephemeralReadSpec, ephemeralReadVMSS = getEphemeralReadOnlyVMSS()
51+
defaultExistingSpec, defaultExistingVMSS, defaultExistingVMSSClone = getExistingDefaultVMSS()
52+
defaultExistingSpecOnlyCapacityChange, defaultExistingVMSSOnlyCapacityChange, defaultExistingVMSSResultOnlyCapacityChange = getExistingDefaultVMSSOnlyCapacityChange()
53+
defaultExistingSpecOnlyCapacityChangeWithCustomDataChange, defaultExistingVMSSOnlyCapacityChangeWithCustomDataChange, defaultExistingVMSSResultOnlyCapacityChangeWithCustomDataChange = getExistingDefaultVMSSOnlyCapacityChangeWithCustomDataChange()
54+
userManagedStorageAccountDiagnosticsSpec, userManagedStorageAccountDiagnosticsVMSS = getUserManagedAndStorageAcccountDiagnosticsVMSS()
55+
managedDiagnosticsSpec, managedDiagnoisticsVMSS = getManagedDiagnosticsVMSS()
56+
disabledDiagnosticsSpec, disabledDiagnosticsVMSS = getDisabledDiagnosticsVMSS()
57+
nilDiagnosticsProfileSpec, nilDiagnosticsProfileVMSS = getNilDiagnosticsProfileVMSS()
5658
)
5759

5860
func getDefaultVMSS() (ScaleSetSpec, armcompute.VirtualMachineScaleSet) {
@@ -430,19 +432,54 @@ func getExistingDefaultVMSS() (s ScaleSetSpec, existing armcompute.VirtualMachin
430432
existingVMSS := newDefaultExistingVMSS("VM_SIZE")
431433
existingVMSS.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}
432434
existingVMSS.SKU.Capacity = ptr.To[int64](2)
433-
existingVMSS.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}
434435

435436
clone := newDefaultExistingVMSS("VM_SIZE")
436437
clone.SKU.Capacity = ptr.To[int64](3)
437438
clone.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}
438439
clone.Properties.VirtualMachineProfile.NetworkProfile = nil
439440

440441
clone.Properties.VirtualMachineProfile.StorageProfile.ImageReference.Version = ptr.To("2.0")
441-
clone.Properties.VirtualMachineProfile.NetworkProfile = nil
442442

443443
return spec, existingVMSS, clone
444444
}
445445

446+
func getExistingDefaultVMSSOnlyCapacityChange() (s ScaleSetSpec, existing armcompute.VirtualMachineScaleSet, result armcompute.VirtualMachineScaleSet) {
447+
spec := newDefaultVMSSSpec()
448+
spec.Capacity = 3
449+
450+
existingVMSS := newDefaultExistingVMSS("VM_SIZE")
451+
452+
result = newDefaultExistingVMSS("VM_SIZE")
453+
result.Properties.VirtualMachineProfile = nil
454+
result.SKU.Capacity = ptr.To[int64](3)
455+
456+
return spec, existingVMSS, result
457+
}
458+
459+
func getExistingDefaultVMSSOnlyCapacityChangeWithCustomDataChange() (s ScaleSetSpec, existing armcompute.VirtualMachineScaleSet, result armcompute.VirtualMachineScaleSet) {
460+
spec := newDefaultVMSSSpec()
461+
spec.Capacity = 3
462+
spec.DataDisks = append(spec.DataDisks, infrav1.DataDisk{
463+
NameSuffix: "my_disk_with_ultra_disks",
464+
DiskSizeGB: 128,
465+
Lun: ptr.To[int32](3),
466+
ManagedDisk: &infrav1.ManagedDiskParameters{
467+
StorageAccountType: "UltraSSD_LRS",
468+
},
469+
})
470+
spec.ShouldPatchCustomData = true
471+
472+
existingVMSS := newDefaultExistingVMSS("VM_SIZE")
473+
existingVMSS.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}
474+
475+
result = newDefaultExistingVMSS("VM_SIZE")
476+
result.SKU.Capacity = ptr.To[int64](3)
477+
result.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}
478+
result.Properties.VirtualMachineProfile.NetworkProfile = nil
479+
480+
return spec, existingVMSS, result
481+
}
482+
446483
func getUserManagedAndStorageAcccountDiagnosticsVMSS() (ScaleSetSpec, armcompute.VirtualMachineScaleSet) {
447484
storageURI := "https://fakeurl"
448485
spec := newDefaultVMSSSpec()
@@ -712,6 +749,20 @@ func TestScaleSetParameters(t *testing.T) {
712749
expected: nilDiagnosticsProfileVMSS,
713750
expectedError: "",
714751
},
752+
{
753+
name: "update for existing vmss with only capacity change",
754+
spec: defaultExistingSpecOnlyCapacityChange,
755+
existing: defaultExistingVMSSOnlyCapacityChange,
756+
expected: defaultExistingVMSSResultOnlyCapacityChange,
757+
expectedError: "",
758+
},
759+
{
760+
name: "update for existing vmss with only capacity change but should patch custom data",
761+
spec: defaultExistingSpecOnlyCapacityChangeWithCustomDataChange,
762+
existing: defaultExistingVMSSOnlyCapacityChangeWithCustomDataChange,
763+
expected: defaultExistingVMSSResultOnlyCapacityChangeWithCustomDataChange,
764+
expectedError: "",
765+
},
715766
}
716767
for _, tc := range testcases {
717768
t.Run(tc.name, func(t *testing.T) {
@@ -731,7 +782,9 @@ func TestScaleSetParameters(t *testing.T) {
731782
if !ok {
732783
t.Fatalf("expected type VirtualMachineScaleSet, got %T", param)
733784
}
734-
result.Properties.VirtualMachineProfile.OSProfile.AdminPassword = nil // Override this field as it's randomly generated. We can't set anything in tc.expected to match it.
785+
if result.Properties.VirtualMachineProfile != nil {
786+
result.Properties.VirtualMachineProfile.OSProfile.AdminPassword = nil // Override this field as it's randomly generated. We can't set anything in tc.expected to match it.
787+
}
735788

736789
if !reflect.DeepEqual(tc.expected, result) {
737790
t.Errorf("Diff between actual result and expected result:\n%s", cmp.Diff(result, tc.expected))

0 commit comments

Comments
 (0)