Skip to content

Ensure all agent pool parameters are preserved in managed cluster creates #5521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 28, 2025
Merged

Ensure all agent pool parameters are preserved in managed cluster creates #5521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ linters:
- durationcheck # multiplying two durations
- errcheck # unchecked errors
- errchkjson # invalid types passed to json encoder
- exhaustruct # checks if all structure fields are initialized
- gci # ensures imports are organized
- ginkgolinter # ginkgo and gomega
- goconst # strings that can be replaced by constants
Expand Down Expand Up @@ -54,6 +55,10 @@ linters:
- whitespace # unnecessary newlines

linters-settings:
exhaustruct:
include:
# Hub ASO agent pools should be fully defined to prevent unnecessary updates
- '.*storage\..*AgentPoolProfile$'
gosec:
excludes:
- G307 # Deferring unsafe method "Close" on type "\*os.File"
Expand Down Expand Up @@ -185,9 +190,9 @@ issues:
- path: (^test/|_test.go$)
linters:
- dogsled
- exhaustruct
- goconst
- godot
- prealloc
- path: (^test/|_test.go$)
text: exported (.+) should have comment( \(or a comment on this block\))? or be unexported
- source: \"github.com/onsi/(ginkgo/v2|gomega)\"
Expand Down
81 changes: 50 additions & 31 deletions azure/converters/managedagentpool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ limitations under the License.
package converters

import (
// NOTE: when the hub API version is updated, verify the
// ManagedClusterAgentPoolProfile below has every field defined. If a field
// isn't defined, the agent pool will be created with a zero/null value, and
// then updated to the user-defined value. If the field is immutable, this
// update will fail. The linter should catch if there are missing fields,
// but verify that check is actually working.
asocontainerservicev1hub "github.com/Azure/azure-service-operator/v2/api/containerservice/v1api20240901/storage"
"k8s.io/utils/ptr"
)
Expand All @@ -25,37 +31,50 @@ import (
func AgentPoolToManagedClusterAgentPoolProfile(pool *asocontainerservicev1hub.ManagedClustersAgentPool) asocontainerservicev1hub.ManagedClusterAgentPoolProfile {
properties := pool.Spec
agentPool := asocontainerservicev1hub.ManagedClusterAgentPoolProfile{
Name: ptr.To(pool.AzureName()),
VmSize: properties.VmSize,
OsType: properties.OsType,
OsDiskSizeGB: properties.OsDiskSizeGB,
Count: properties.Count,
Type: properties.Type,
OrchestratorVersion: properties.OrchestratorVersion,
VnetSubnetReference: properties.VnetSubnetReference,
Mode: properties.Mode,
EnableAutoScaling: properties.EnableAutoScaling,
MaxCount: properties.MaxCount,
MinCount: properties.MinCount,
NodeTaints: properties.NodeTaints,
AvailabilityZones: properties.AvailabilityZones,
MaxPods: properties.MaxPods,
OsDiskType: properties.OsDiskType,
NodeLabels: properties.NodeLabels,
EnableUltraSSD: properties.EnableUltraSSD,
EnableNodePublicIP: properties.EnableNodePublicIP,
NodePublicIPPrefixReference: properties.NodePublicIPPrefixReference,
ScaleSetPriority: properties.ScaleSetPriority,
ScaleDownMode: properties.ScaleDownMode,
SpotMaxPrice: properties.SpotMaxPrice,
Tags: properties.Tags,
KubeletDiskType: properties.KubeletDiskType,
LinuxOSConfig: properties.LinuxOSConfig,
EnableFIPS: properties.EnableFIPS,
EnableEncryptionAtHost: properties.EnableEncryptionAtHost,
}
if properties.KubeletConfig != nil {
agentPool.KubeletConfig = properties.KubeletConfig
AvailabilityZones: properties.AvailabilityZones,
Copy link
Contributor Author

@nojnhuh nojnhuh Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the new fields here aren't possible to set from an AzureManagedMachinePool, so there's a fair bit of no-op here. I was too lazy to audit which ones can actually be set, but keeping everything here makes for one less thing to worry about in case we do add or remove any features from CAPZ.

Copy link
Contributor

@jackfrancis jackfrancis Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, thx for adding the maintenance comments so we're less likely to omit this in the future

CapacityReservationGroupReference: properties.CapacityReservationGroupReference,
Count: properties.Count,
CreationData: properties.CreationData,
EnableAutoScaling: properties.EnableAutoScaling,
EnableEncryptionAtHost: properties.EnableEncryptionAtHost,
EnableFIPS: properties.EnableFIPS,
EnableNodePublicIP: properties.EnableNodePublicIP,
EnableUltraSSD: properties.EnableUltraSSD,
GpuInstanceProfile: properties.GpuInstanceProfile,
HostGroupReference: properties.HostGroupReference,
KubeletConfig: properties.KubeletConfig,
KubeletDiskType: properties.KubeletDiskType,
LinuxOSConfig: properties.LinuxOSConfig,
MaxCount: properties.MaxCount,
MaxPods: properties.MaxPods,
MinCount: properties.MinCount,
Mode: properties.Mode,
Name: ptr.To(pool.AzureName()),
NetworkProfile: properties.NetworkProfile,
NodeLabels: properties.NodeLabels,
NodePublicIPPrefixReference: properties.NodePublicIPPrefixReference,
NodeTaints: properties.NodeTaints,
OrchestratorVersion: properties.OrchestratorVersion,
OsDiskSizeGB: properties.OsDiskSizeGB,
OsDiskType: properties.OsDiskType,
OsSKU: properties.OsSKU,
OsType: properties.OsType,
PodSubnetReference: properties.PodSubnetReference,
PowerState: properties.PowerState,
PropertyBag: properties.PropertyBag,
ProximityPlacementGroupReference: properties.ProximityPlacementGroupReference,
ScaleDownMode: properties.ScaleDownMode,
ScaleSetEvictionPolicy: properties.ScaleSetEvictionPolicy,
ScaleSetPriority: properties.ScaleSetPriority,
SecurityProfile: properties.SecurityProfile,
SpotMaxPrice: properties.SpotMaxPrice,
Tags: properties.Tags,
Type: properties.Type,
UpgradeSettings: properties.UpgradeSettings,
VmSize: properties.VmSize,
VnetSubnetReference: properties.VnetSubnetReference,
WindowsProfile: properties.WindowsProfile,
WorkloadRuntime: properties.WorkloadRuntime,
}
return agentPool
}
8 changes: 8 additions & 0 deletions pkg/mutators/azureasomanagedcontrolplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ import (
"strings"

asocontainerservicev1 "github.com/Azure/azure-service-operator/v2/api/containerservice/v1api20231001"
// NOTE: when the hub API version is updated, verify the
// ManagedClusterAgentPoolProfile below has every field defined. If a field
// isn't defined, the agent pool will be created with a zero/null value, and
// then updated to the user-defined value. If the field is immutable, this
// update will fail. The linter should catch if there are missing fields,
// but verify that check is actually working.
asocontainerservicev1hub "github.com/Azure/azure-service-operator/v2/api/containerservice/v1api20240901/storage"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
Expand Down Expand Up @@ -350,12 +356,14 @@ func setAgentPoolProfilesFromAgentPools(managedCluster conversion.Convertible, a
ScaleDownMode: hubPool.Spec.ScaleDownMode,
ScaleSetEvictionPolicy: hubPool.Spec.ScaleSetEvictionPolicy,
ScaleSetPriority: hubPool.Spec.ScaleSetPriority,
SecurityProfile: hubPool.Spec.SecurityProfile,
Copy link
Contributor Author

@nojnhuh nojnhuh Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main alternative to this approach is to use unkeyed struct fields and let the compiler check that everything is defined, but I think I prefer the sanity check of having both field names on one line like this.

SpotMaxPrice: hubPool.Spec.SpotMaxPrice,
Tags: hubPool.Spec.Tags,
Type: hubPool.Spec.Type,
UpgradeSettings: hubPool.Spec.UpgradeSettings,
VmSize: hubPool.Spec.VmSize,
VnetSubnetReference: hubPool.Spec.VnetSubnetReference,
WindowsProfile: hubPool.Spec.WindowsProfile,
WorkloadRuntime: hubPool.Spec.WorkloadRuntime,
}

Expand Down