Skip to content

Commit

Permalink
fix(machinepool): clear VirtualMachineProfile if no model/custom data…
Browse files Browse the repository at this point in the history 
… update

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
  • Loading branch information
@mweibel
mweibel committed Oct 7, 2024
1 parent 4fec820 commit 19d3251
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 32 deletions.
14 changes: 5 additions & 9 deletions azure/scope/machinepool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,8 @@ func (m *MachinePoolScope) ScaleSetSpec(ctx context.Context) azure.ResourceSpecG
}

if m.cache != nil {
if m.HasReplicasExternallyManaged(ctx) {
spec.ShouldPatchCustomData = m.cache.HasBootstrapDataChanges
log.V(4).Info("has bootstrap data changed?", "shouldPatchCustomData", spec.ShouldPatchCustomData)
}
spec.ShouldPatchCustomData = m.cache.HasBootstrapDataChanges
log.V(4).Info("has bootstrap data changed?", "shouldPatchCustomData", spec.ShouldPatchCustomData)

Check warning on line 226 in azure/scope/machinepool.go

View check run for this annotation

Codecov / codecov/patch

azure/scope/machinepool.go#L225-L226 

Added lines #L225 - L226 were not covered by tests
spec.VMSSExtensionSpecs = m.VMSSExtensionSpecs()
spec.SKU = m.cache.VMSKU
spec.VMImage = m.cache.VMImage
Expand Down Expand Up @@ -705,11 +703,9 @@ 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 m.HasReplicasExternallyManaged(ctx) {
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.updateCustomDataHash(ctx); err != nil {

Check warning on line 706 in azure/scope/machinepool.go

View check run for this annotation

Codecov / codecov/patch

azure/scope/machinepool.go#L706 

Added line #L706 was not covered by tests
// 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.")

Check warning on line 708 in azure/scope/machinepool.go

View check run for this annotation

Codecov / codecov/patch

azure/scope/machinepool.go#L708 

Added line #L708 was not covered by tests
}
}

Expand Down
12 changes: 12 additions & 0 deletions azure/services/scalesets/spec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ func (s *ScaleSetSpec) OwnerResourceName() string {
}

func (s *ScaleSetSpec) existingParameters(ctx context.Context, existing interface{}) (parameters interface{}, err error) {
ctx, log, done := tele.StartSpanWithLogger(ctx, "scalesets.ScaleSetSpec.existingParameters")
defer done()

existingVMSS, ok := existing.(armcompute.VirtualMachineScaleSet)
if !ok {
return nil, errors.Errorf("%T is not an armcompute.VirtualMachineScaleSet", existing)
Expand Down Expand Up @@ -132,6 +135,15 @@ func (s *ScaleSetSpec) existingParameters(ctx context.Context, existing interfac
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)
vmss.Properties.VirtualMachineProfile = nil
} else {
log.V(4).Info("has changes, not removing virtual machine profile from parameters", "hasModelChanges", hasModelChanges, "shouldPatchCustomData", s.ShouldPatchCustomData)
}

return vmss, nil
}

Expand Down
99 changes: 76 additions & 23 deletions azure/services/scalesets/spec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,28 @@ import (
)

var (
defaultSpec, defaultVMSS = getDefaultVMSS()
windowsSpec, windowsVMSS = getDefaultWindowsVMSS()
acceleratedNetworkingSpec, acceleratedNetworkingVMSS = getAcceleratedNetworkingVMSS()
customSubnetSpec, customSubnetVMSS = getCustomSubnetVMSS()
customNetworkingSpec, customNetworkingVMSS = getCustomNetworkingVMSS()
spotVMSpec, spotVMVMSS = getSpotVMVMSS()
ephemeralSpec, ephemeralVMSS = getEPHVMSSS()
resourceDiskSpec, resourceDiskVMSS = getResourceDiskVMSS()
evictionSpec, evictionVMSS = getEvictionPolicyVMSS()
maxPriceSpec, maxPriceVMSS = getMaxPriceVMSS()
encryptionSpec, encryptionVMSS = getEncryptionVMSS()
userIdentitySpec, userIdentityVMSS = getUserIdentityVMSS()
hostEncryptionSpec, hostEncryptionVMSS = getHostEncryptionVMSS()
hostEncryptionUnsupportedSpec = getHostEncryptionUnsupportedSpec()
ephemeralReadSpec, ephemeralReadVMSS = getEphemeralReadOnlyVMSS()
defaultExistingSpec, defaultExistingVMSS, defaultExistingVMSSClone = getExistingDefaultVMSS()
userManagedStorageAccountDiagnosticsSpec, userManagedStorageAccountDiagnosticsVMSS = getUserManagedAndStorageAcccountDiagnosticsVMSS()
managedDiagnosticsSpec, managedDiagnoisticsVMSS = getManagedDiagnosticsVMSS()
disabledDiagnosticsSpec, disabledDiagnosticsVMSS = getDisabledDiagnosticsVMSS()
nilDiagnosticsProfileSpec, nilDiagnosticsProfileVMSS = getNilDiagnosticsProfileVMSS()
defaultSpec, defaultVMSS = getDefaultVMSS()
windowsSpec, windowsVMSS = getDefaultWindowsVMSS()
acceleratedNetworkingSpec, acceleratedNetworkingVMSS = getAcceleratedNetworkingVMSS()
customSubnetSpec, customSubnetVMSS = getCustomSubnetVMSS()
customNetworkingSpec, customNetworkingVMSS = getCustomNetworkingVMSS()
spotVMSpec, spotVMVMSS = getSpotVMVMSS()
ephemeralSpec, ephemeralVMSS = getEPHVMSSS()
resourceDiskSpec, resourceDiskVMSS = getResourceDiskVMSS()
evictionSpec, evictionVMSS = getEvictionPolicyVMSS()
maxPriceSpec, maxPriceVMSS = getMaxPriceVMSS()
encryptionSpec, encryptionVMSS = getEncryptionVMSS()
userIdentitySpec, userIdentityVMSS = getUserIdentityVMSS()
hostEncryptionSpec, hostEncryptionVMSS = getHostEncryptionVMSS()
hostEncryptionUnsupportedSpec = getHostEncryptionUnsupportedSpec()
ephemeralReadSpec, ephemeralReadVMSS = getEphemeralReadOnlyVMSS()
defaultExistingSpec, defaultExistingVMSS, defaultExistingVMSSClone = getExistingDefaultVMSS()
defaultExistingSpecOnlyCapacityChange, defaultExistingVMSSOnlyCapacityChange, defaultExistingVMSSResultOnlyCapacityChange = getExistingDefaultVMSSOnlyCapacityChange()
defaultExistingSpecOnlyCapacityChangeWithCustomDataChange, defaultExistingVMSSOnlyCapacityChangeWithCustomDataChange, defaultExistingVMSSResultOnlyCapacityChangeWithCustomDataChange = getExistingDefaultVMSSOnlyCapacityChangeWithCustomDataChange()
userManagedStorageAccountDiagnosticsSpec, userManagedStorageAccountDiagnosticsVMSS = getUserManagedAndStorageAcccountDiagnosticsVMSS()
managedDiagnosticsSpec, managedDiagnoisticsVMSS = getManagedDiagnosticsVMSS()
disabledDiagnosticsSpec, disabledDiagnosticsVMSS = getDisabledDiagnosticsVMSS()
nilDiagnosticsProfileSpec, nilDiagnosticsProfileVMSS = getNilDiagnosticsProfileVMSS()
)

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

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

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

return spec, existingVMSS, clone
}

func getExistingDefaultVMSSOnlyCapacityChange() (s ScaleSetSpec, existing armcompute.VirtualMachineScaleSet, result armcompute.VirtualMachineScaleSet) {
spec := newDefaultVMSSSpec()
spec.Capacity = 3

existingVMSS := newDefaultExistingVMSS("VM_SIZE")

result = newDefaultExistingVMSS("VM_SIZE")
result.Properties.VirtualMachineProfile = nil
result.SKU.Capacity = ptr.To[int64](3)

return spec, existingVMSS, result
}

func getExistingDefaultVMSSOnlyCapacityChangeWithCustomDataChange() (s ScaleSetSpec, existing armcompute.VirtualMachineScaleSet, result armcompute.VirtualMachineScaleSet) {
spec := newDefaultVMSSSpec()
spec.Capacity = 3
spec.DataDisks = append(spec.DataDisks, infrav1.DataDisk{
NameSuffix: "my_disk_with_ultra_disks",
DiskSizeGB: 128,
Lun: ptr.To[int32](3),
ManagedDisk: &infrav1.ManagedDiskParameters{
StorageAccountType: "UltraSSD_LRS",
},
})
spec.ShouldPatchCustomData = true

existingVMSS := newDefaultExistingVMSS("VM_SIZE")
existingVMSS.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}

result = newDefaultExistingVMSS("VM_SIZE")
result.SKU.Capacity = ptr.To[int64](3)
result.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}
result.Properties.VirtualMachineProfile.NetworkProfile = nil

return spec, existingVMSS, result
}

func getUserManagedAndStorageAcccountDiagnosticsVMSS() (ScaleSetSpec, armcompute.VirtualMachineScaleSet) {
storageURI := "https://fakeurl"
spec := newDefaultVMSSSpec()
Expand Down Expand Up @@ -712,6 +749,20 @@ func TestScaleSetParameters(t *testing.T) {
expected: nilDiagnosticsProfileVMSS,
expectedError: "",
},
{
name: "update for existing vmss with only capacity change",
spec: defaultExistingSpecOnlyCapacityChange,
existing: defaultExistingVMSSOnlyCapacityChange,
expected: defaultExistingVMSSResultOnlyCapacityChange,
expectedError: "",
},
{
name: "update for existing vmss with only capacity change but should patch custom data",
spec: defaultExistingSpecOnlyCapacityChangeWithCustomDataChange,
existing: defaultExistingVMSSOnlyCapacityChangeWithCustomDataChange,
expected: defaultExistingVMSSResultOnlyCapacityChangeWithCustomDataChange,
expectedError: "",
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
Expand All @@ -731,7 +782,9 @@ func TestScaleSetParameters(t *testing.T) {
if !ok {
t.Fatalf("expected type VirtualMachineScaleSet, got %T", param)
}
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.
if result.Properties.VirtualMachineProfile != nil {
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.
}

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

0 comments on commit 19d3251

Please sign in to comment.