Fix review comments 2

Author: LiangquanLi930

api/v1beta2/awsmachinetemplate_types.go

@@ -33,12 +33,16 @@ const (
 	ArchitectureArm64 Architecture = "arm64"
 )
 
+// OperatingSystem represents the operating system of the node.
+// Its underlying type is a string and its value can be any of linux, windows.
+type OperatingSystem string
+
 // Operating system constants.
 const (
 	// OperatingSystemLinux represents the Linux operating system.
-	OperatingSystemLinux = "linux"
+	OperatingSystemLinux OperatingSystem = "linux"
 	// OperatingSystemWindows represents the Windows operating system.
-	OperatingSystemWindows = "windows"
+	OperatingSystemWindows OperatingSystem = "windows"
 )
 
 // NodeInfo contains information about the node's architecture and operating system.
@@ -48,11 +52,11 @@ type NodeInfo struct {
 	// +kubebuilder:validation:Enum=amd64;arm64
 	// +optional
 	Architecture Architecture `json:"architecture,omitempty"`
-	// OperatingSystem is a string representing the operating system of the node.
-	// This may be a string like 'linux' or 'windows'.
+	// OperatingSystem is the operating system of the node.
+	// Its underlying type is a string and its value can be any of linux, windows.
 	// +kubebuilder:validation:Enum=linux;windows
 	// +optional
-	OperatingSystem string `json:"operatingSystem,omitempty"`
+	OperatingSystem OperatingSystem `json:"operatingSystem,omitempty"`
 }
 
 // AWSMachineTemplateStatus defines a status for an AWSMachineTemplate.

config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml

@@ -1172,8 +1172,8 @@ spec:
                     type: string
                   operatingSystem:
                     description: |-
-                      OperatingSystem is a string representing the operating system of the node.
-                      This may be a string like 'linux' or 'windows'.
+                      OperatingSystem is the operating system of the node.
+                      Its underlying type is a string and its value can be any of linux, windows.
                     enum:
                     - linux
                     - windows

controllers/awsmachinetemplate_controller.go

@@ -32,6 +32,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
+	ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
 	ec2service "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/ec2"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/logger"
@@ -90,6 +91,13 @@ func (r *AWSMachineTemplateReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return ctrl.Result{}, nil
 	}
 
+	// Check if capacity and nodeInfo are already populated
+	// This avoids unnecessary AWS API calls when the status is already populated
+	if len(awsMachineTemplate.Status.Capacity) > 0 &&
+		awsMachineTemplate.Status.NodeInfo != nil && awsMachineTemplate.Status.NodeInfo.OperatingSystem != "" && awsMachineTemplate.Status.NodeInfo.Architecture != "" {
+		return ctrl.Result{}, nil
+	}
+
 	// Find the region by checking ownerReferences
 	region, err := r.getRegion(ctx, awsMachineTemplate)
 	if err != nil {
@@ -169,6 +177,21 @@ func (r *AWSMachineTemplateReconciler) getRegion(ctx context.Context, template *
 		}
 	}
 
+	// Get region from AWSManagedControlPlane (EKS cluster)
+	if cluster.Spec.ControlPlaneRef != nil && cluster.Spec.ControlPlaneRef.Kind == "AWSManagedControlPlane" {
+		awsManagedCP := &ekscontrolplanev1.AWSManagedControlPlane{}
+		if err := r.Get(ctx, client.ObjectKey{
+			Namespace: cluster.Namespace,
+			Name:      cluster.Spec.ControlPlaneRef.Name,
+		}, awsManagedCP); err != nil {
+			if !apierrors.IsNotFound(err) {
+				return "", errors.Wrapf(err, "failed to get AWSManagedControlPlane %s/%s", cluster.Namespace, cluster.Spec.ControlPlaneRef.Name)
+			}
+		} else if awsManagedCP.Spec.Region != "" {
+			return awsManagedCP.Spec.Region, nil
+		}
+	}
+
 	return "", nil
 }
 
@@ -288,7 +311,7 @@ func (r *AWSMachineTemplateReconciler) getNodeInfo(ctx context.Context, ec2Clien
 }
 
 // getNodeInfoFromAMI queries the AMI to determine architecture and operating system.
-func (r *AWSMachineTemplateReconciler) getNodeInfoFromAMI(ctx context.Context, ec2Client *ec2.Client, amiID string) (infrav1.Architecture, string, error) {
+func (r *AWSMachineTemplateReconciler) getNodeInfoFromAMI(ctx context.Context, ec2Client *ec2.Client, amiID string) (infrav1.Architecture, infrav1.OperatingSystem, error) {
 	input := &ec2.DescribeImagesInput{
 		ImageIds: []string{amiID},
 	}
@@ -325,7 +348,7 @@ func (r *AWSMachineTemplateReconciler) getNodeInfoFromAMI(ctx context.Context, e
 	// 2. Check PlatformDetails field for Windows indication
 	if os != infrav1.OperatingSystemWindows && image.PlatformDetails != nil {
 		platformDetails := strings.ToLower(*image.PlatformDetails)
-		if strings.Contains(platformDetails, infrav1.OperatingSystemWindows) {
+		if strings.Contains(platformDetails, string(infrav1.OperatingSystemWindows)) {
 			os = infrav1.OperatingSystemWindows
 		}
 	}

controllers/awsmachinetemplate_controller_unit_test.go

@@ -192,6 +192,32 @@ func TestAWSMachineTemplateReconciler(t *testing.T) {
 	// integration tests.
 
 	t.Run("Reconcile", func(t *testing.T) {
+		t.Run("should skip reconcile when capacity and nodeInfo are already populated", func(t *testing.T) {
+			g := NewWithT(t)
+			template := newAWSMachineTemplate("test-template")
+			template.Status.Capacity = corev1.ResourceList{
+				corev1.ResourceCPU:    *resource.NewQuantity(2, resource.DecimalSI),
+				corev1.ResourceMemory: resource.MustParse("4Gi"),
+			}
+			template.Status.NodeInfo = &infrav1.NodeInfo{
+				Architecture:    infrav1.ArchitectureAmd64,
+				OperatingSystem: infrav1.OperatingSystemLinux,
+			}
+
+			reconciler := &AWSMachineTemplateReconciler{
+				Client: newFakeClient(template),
+			}
+
+			// Should skip reconcile and return early without calling AWS APIs
+			// No need to set up owner cluster or region since the early return happens before that
+			result, err := reconciler.Reconcile(context.Background(), ctrl.Request{
+				NamespacedName: client.ObjectKeyFromObject(template),
+			})
+
+			g.Expect(err).To(BeNil())
+			g.Expect(result.Requeue).To(BeFalse())
+		})
+
 		t.Run("should reconcile when capacity set but nodeInfo is not", func(t *testing.T) {
 			g := NewWithT(t)
 			template := newAWSMachineTemplate("test-template")

main.go

@@ -360,9 +360,19 @@ func setupReconcilersAndWebhooks(ctx context.Context, mgr ctrl.Manager,
 			setupLog.Error(err, "unable to create controller", "controller", "AWSCluster")
 			os.Exit(1)
 		}
+
+		setupLog.Info("enabling AWSMachineTemplate controller")
+		if err := (&controllers.AWSMachineTemplateReconciler{
+			Client:           mgr.GetClient(),
+			WatchFilterValue: watchFilterValue,
+		}).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: awsClusterConcurrency, RecoverPanic: ptr.To[bool](true)}); err != nil {
+			setupLog.Error(err, "unable to create controller", "controller", "AWSMachineTemplate")
+			os.Exit(1)
+		}
 	} else {
 		setupLog.Info("controller disabled", "controller", "AWSMachine", "controller-group", controllers.Unmanaged)
 		setupLog.Info("controller disabled", "controller", "AWSCluster", "controller-group", controllers.Unmanaged)
+		setupLog.Info("controller disabled", "controller", "AWSMachineTemplate", "controller-group", controllers.Unmanaged)
 	}
 
 	if feature.Gates.Enabled(feature.MachinePool) {
@@ -407,15 +417,6 @@ func setupReconcilersAndWebhooks(ctx context.Context, mgr ctrl.Manager,
 		}
 	}
 
-	setupLog.Debug("enabling AWSMachineTemplate controller")
-	if err := (&controllers.AWSMachineTemplateReconciler{
-		Client:           mgr.GetClient(),
-		WatchFilterValue: watchFilterValue,
-	}).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: awsClusterConcurrency, RecoverPanic: ptr.To[bool](true)}); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "AWSMachineTemplate")
-		os.Exit(1)
-	}
-
 	if err := (&infrav1.AWSMachineTemplate{}).SetupWebhookWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create webhook", "webhook", "AWSMachineTemplate")
 		os.Exit(1)