[improvement] : add new phase to check if we can use cloudinit or not…

… to reduce API calls (#522) * add new phase to check if we can use cloudinit or not to reduce number of GET API calls * fix tests
linode · Oct 2, 2024 · c9b73ad · c9b73ad
1 parent f828b25
commit c9b73ad
Show file tree

Hide file tree

Showing 8 changed files with 101 additions and 179 deletions.
diff --git a/api/v1alpha1/conversion.go b/api/v1alpha1/conversion.go
@@ -47,6 +47,11 @@ func Convert_v1alpha2_LinodeMachineSpec_To_v1alpha1_LinodeMachineSpec(in *infras
 	return autoConvert_v1alpha2_LinodeMachineSpec_To_v1alpha1_LinodeMachineSpec(in, out, s)
 }
 
+func Convert_v1alpha2_LinodeMachineStatus_To_v1alpha1_LinodeMachineStatus(in *infrastructurev1alpha2.LinodeMachineStatus, out *LinodeMachineStatus, s conversion.Scope) error {
+	// Ok to use the auto-generated conversion function
+	return autoConvert_v1alpha2_LinodeMachineStatus_To_v1alpha1_LinodeMachineStatus(in, out, s)
+}
+
 func Convert_v1alpha1_LinodeMachineSpec_To_v1alpha2_LinodeMachineSpec(in *LinodeMachineSpec, out *infrastructurev1alpha2.LinodeMachineSpec, s conversion.Scope) error {
 	return autoConvert_v1alpha1_LinodeMachineSpec_To_v1alpha2_LinodeMachineSpec(in, out, s)
 }

diff --git a/api/v1alpha1/zz_generated.conversion.go b/api/v1alpha1/zz_generated.conversion.go
diff --git a/api/v1alpha2/linodemachine_types.go b/api/v1alpha2/linodemachine_types.go
@@ -161,6 +161,11 @@ type LinodeMachineStatus struct {
 	// Addresses contains the Linode instance associated addresses.
 	Addresses []clusterv1.MachineAddress `json:"addresses,omitempty"`
 
+	// CloudinitMetadataSupport determines whether to use cloud-init or not.
+	// +optional
+	// +kubebuilder:default=true
+	CloudinitMetadataSupport bool `json:"cloudinitMetadataSupport,omitempty"`
+
 	// InstanceState is the state of the Linode instance for this machine.
 	// +optional
 	InstanceState *linodego.InstanceStatus `json:"instanceState,omitempty"`

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml
@@ -769,6 +769,11 @@ spec:
                   - type
                   type: object
                 type: array
+              cloudinitMetadataSupport:
+                default: true
+                description: CloudinitMetadataSupport determines whether to use cloud-init
+                  or not.
+                type: boolean
               conditions:
                 description: Conditions defines current service state of the LinodeMachine.
                 items:

diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go
@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"slices"
 	"strings"
 	"time"
 
@@ -58,13 +59,14 @@ const (
 	defaultDiskFilesystem = string(linodego.FilesystemExt4)
 
 	// conditions for preflight instance creation
-	ConditionPreflightCreated                clusterv1.ConditionType = "PreflightCreated"
-	ConditionPreflightRootDiskResizing       clusterv1.ConditionType = "PreflightRootDiskResizing"
-	ConditionPreflightRootDiskResized        clusterv1.ConditionType = "PreflightRootDiskResized"
-	ConditionPreflightAdditionalDisksCreated clusterv1.ConditionType = "PreflightAdditionalDisksCreated"
-	ConditionPreflightConfigured             clusterv1.ConditionType = "PreflightConfigured"
-	ConditionPreflightBootTriggered          clusterv1.ConditionType = "PreflightBootTriggered"
-	ConditionPreflightReady                  clusterv1.ConditionType = "PreflightReady"
+	ConditionPreflightMetadataSupportConfigured clusterv1.ConditionType = "PreflightMetadataSupportConfigured"
+	ConditionPreflightCreated                   clusterv1.ConditionType = "PreflightCreated"
+	ConditionPreflightRootDiskResizing          clusterv1.ConditionType = "PreflightRootDiskResizing"
+	ConditionPreflightRootDiskResized           clusterv1.ConditionType = "PreflightRootDiskResized"
+	ConditionPreflightAdditionalDisksCreated    clusterv1.ConditionType = "PreflightAdditionalDisksCreated"
+	ConditionPreflightConfigured                clusterv1.ConditionType = "PreflightConfigured"
+	ConditionPreflightBootTriggered             clusterv1.ConditionType = "PreflightBootTriggered"
+	ConditionPreflightReady                     clusterv1.ConditionType = "PreflightReady"
 
 	// WaitingForBootstrapDataReason used when machine is waiting for bootstrap data to be ready before proceeding.
 	WaitingForBootstrapDataReason = "WaitingForBootstrapData"
@@ -212,7 +214,7 @@ func (r *LinodeMachineReconciler) reconcile(ctx context.Context, logger logr.Log
 	// Make sure bootstrap data is available and populated.
 	if machineScope.Machine.Spec.Bootstrap.DataSecretName == nil {
 		logger.Info("Bootstrap data secret is not yet available")
-		conditions.MarkFalse(machineScope.LinodeMachine, ConditionPreflightCreated, WaitingForBootstrapDataReason, clusterv1.ConditionSeverityInfo, "")
+		conditions.MarkFalse(machineScope.LinodeMachine, ConditionPreflightMetadataSupportConfigured, WaitingForBootstrapDataReason, clusterv1.ConditionSeverityInfo, "")
 		return ctrl.Result{}, nil
 	}
 
@@ -240,6 +242,13 @@ func (r *LinodeMachineReconciler) reconcileCreate(
 		return ctrl.Result{}, err
 	}
 
+	if !reconciler.ConditionTrue(machineScope.LinodeMachine, ConditionPreflightMetadataSupportConfigured) && machineScope.LinodeMachine.Spec.ProviderID == nil {
+		res, err := r.reconcilePreflightMetadataSupportConfigure(ctx, logger, machineScope)
+		if err != nil || !res.IsZero() {
+			return res, err
+		}
+	}
+
 	if !reconciler.ConditionTrue(machineScope.LinodeMachine, ConditionPreflightCreated) && machineScope.LinodeMachine.Spec.ProviderID == nil {
 		res, err := r.reconcilePreflightCreate(ctx, logger, machineScope)
 		if err != nil || !res.IsZero() {
@@ -278,6 +287,32 @@ func (r *LinodeMachineReconciler) reconcileCreate(
 	return ctrl.Result{}, nil
 }
 
+func (r *LinodeMachineReconciler) reconcilePreflightMetadataSupportConfigure(ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope) (ctrl.Result, error) {
+	region, err := machineScope.LinodeClient.GetRegion(ctx, machineScope.LinodeMachine.Spec.Region)
+	if err != nil {
+		logger.Error(err, fmt.Sprintf("Failed to fetch region %s", machineScope.LinodeMachine.Spec.Region))
+		return retryIfTransient(err)
+	}
+	regionMetadataSupport := slices.Contains(region.Capabilities, linodego.CapabilityMetadata)
+	imageName := reconciler.DefaultMachineControllerLinodeImage
+	if machineScope.LinodeMachine.Spec.Image != "" {
+		imageName = machineScope.LinodeMachine.Spec.Image
+	}
+	image, err := machineScope.LinodeClient.GetImage(ctx, imageName)
+	if err != nil {
+		logger.Error(err, fmt.Sprintf("Failed to fetch image %s", imageName))
+		return retryIfTransient(err)
+	}
+	imageMetadataSupport := slices.Contains(image.Capabilities, "cloud-init")
+	machineScope.LinodeMachine.Status.CloudinitMetadataSupport = true
+	if !imageMetadataSupport || !regionMetadataSupport {
+		logger.Info("cloud-init metadata support not available", "imageMetadataSupport", imageMetadataSupport, "regionMetadataSupport", regionMetadataSupport)
+		machineScope.LinodeMachine.Status.CloudinitMetadataSupport = false
+	}
+	conditions.MarkTrue(machineScope.LinodeMachine, ConditionPreflightMetadataSupportConfigured)
+	return ctrl.Result{}, nil
+}
+
 func (r *LinodeMachineReconciler) reconcilePreflightCreate(ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope) (ctrl.Result, error) {
 	// get the bootstrap data for the Linode instance and set it for create config
 	createOpts, err := newCreateConfig(ctx, machineScope, logger)

diff --git a/controller/linodemachine_controller_helpers.go b/controller/linodemachine_controller_helpers.go
@@ -444,29 +444,12 @@ func setUserData(ctx context.Context, machineScope *scope.MachineScope, createCo
 		return err
 	}
 
-	region, err := machineScope.LinodeClient.GetRegion(ctx, machineScope.LinodeMachine.Spec.Region)
-	if err != nil {
-		return fmt.Errorf("get region: %w", err)
-	}
-	regionMetadataSupport := slices.Contains(region.Capabilities, "Metadata")
-	imageName := reconciler.DefaultMachineControllerLinodeImage
-	if machineScope.LinodeMachine.Spec.Image != "" {
-		imageName = machineScope.LinodeMachine.Spec.Image
-	}
-	image, err := machineScope.LinodeClient.GetImage(ctx, imageName)
-	if err != nil {
-		return fmt.Errorf("get image: %w", err)
-	}
-	imageMetadataSupport := slices.Contains(image.Capabilities, "cloud-init")
-	if imageMetadataSupport && regionMetadataSupport {
+	if machineScope.LinodeMachine.Status.CloudinitMetadataSupport {
 		createConfig.Metadata = &linodego.InstanceMetadataOptions{
 			UserData: b64.StdEncoding.EncodeToString(bootstrapData),
 		}
 	} else {
-		logger.Info("using StackScripts for bootstrapping",
-			"imageMetadataSupport", imageMetadataSupport,
-			"regionMetadataSupport", regionMetadataSupport,
-		)
+		logger.Info("using StackScripts for bootstrapping")
 		capiStackScriptID, err := services.EnsureStackscript(ctx, machineScope)
 		if err != nil {
 			return fmt.Errorf("ensure stackscript: %w", err)

diff --git a/controller/linodemachine_controller_helpers_test.go b/controller/linodemachine_controller_helpers_test.go
@@ -103,7 +103,7 @@ func TestSetUserData(t *testing.T) {
 					Namespace: "default",
 				},
 				Spec:   infrav1alpha2.LinodeMachineSpec{Region: "us-ord", Image: "linode/ubuntu22.04"},
-				Status: infrav1alpha2.LinodeMachineStatus{},
+				Status: infrav1alpha2.LinodeMachineStatus{CloudinitMetadataSupport: true},
 			}},
 			createConfig: &linodego.InstanceCreateOptions{},
 			wantConfig: &linodego.InstanceCreateOptions{Metadata: &linodego.InstanceMetadataOptions{
@@ -119,12 +119,6 @@ func TestSetUserData(t *testing.T) {
 					*obj = cred
 					return nil
 				})
-				mockClient.EXPECT().GetRegion(gomock.Any(), "us-ord").Return(&linodego.Region{
-					Capabilities: []string{"Metadata"},
-				}, nil)
-				mockClient.EXPECT().GetImage(gomock.Any(), "linode/ubuntu22.04").Return(&linodego.Image{
-					Capabilities: []string{"cloud-init"},
-				}, nil)
 			},
 		},
 		{
@@ -143,7 +137,7 @@ func TestSetUserData(t *testing.T) {
 					Namespace: "default",
 				},
 				Spec:   infrav1alpha2.LinodeMachineSpec{Region: "us-east", Image: "linode/ubuntu22.04", Type: "g6-standard-1"},
-				Status: infrav1alpha2.LinodeMachineStatus{},
+				Status: infrav1alpha2.LinodeMachineStatus{CloudinitMetadataSupport: false},
 			}},
 			createConfig: &linodego.InstanceCreateOptions{},
 			wantConfig: &linodego.InstanceCreateOptions{StackScriptID: 1234, StackScriptData: map[string]string{
@@ -160,10 +154,6 @@ func TestSetUserData(t *testing.T) {
 					*obj = cred
 					return nil
 				})
-				mockClient.EXPECT().GetRegion(gomock.Any(), "us-east").Return(&linodego.Region{
-					Capabilities: []string{"Metadata"},
-				}, nil)
-				mockClient.EXPECT().GetImage(gomock.Any(), "linode/ubuntu22.04").Return(&linodego.Image{}, nil)
 				mockClient.EXPECT().ListStackscripts(gomock.Any(), &linodego.ListOptions{Filter: "{\"label\":\"CAPL-dev\"}"}).Return([]linodego.Stackscript{{
 					Label: "CAPI Test 1",
 					ID:    1234,
@@ -228,77 +218,6 @@ func TestSetUserData(t *testing.T) {
 			},
 			expectedError: fmt.Errorf("bootstrap data secret is nil for LinodeMachine default/test-cluster"),
 		},
-		{
-			name: "Error - SetUserData failed to get regions",
-			machineScope: &scope.MachineScope{Machine: &v1beta1.Machine{
-				Spec: v1beta1.MachineSpec{
-					ClusterName: "",
-					Bootstrap: v1beta1.Bootstrap{
-						DataSecretName: ptr.To("test-data"),
-					},
-					InfrastructureRef: corev1.ObjectReference{},
-				},
-			}, LinodeMachine: &infrav1alpha2.LinodeMachine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-cluster",
-					Namespace: "default",
-				},
-				Spec:   infrav1alpha2.LinodeMachineSpec{Region: "us-ord", Image: "linode/ubuntu22.04"},
-				Status: infrav1alpha2.LinodeMachineStatus{},
-			}},
-			createConfig: &linodego.InstanceCreateOptions{},
-			wantConfig:   &linodego.InstanceCreateOptions{},
-			expects: func(mockClient *mock.MockLinodeClient, kMock *mock.MockK8sClient) {
-				kMock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error {
-					cred := corev1.Secret{
-						Data: map[string][]byte{
-							"value": []byte("hello"),
-						},
-					}
-					*obj = cred
-					return nil
-				})
-				mockClient.EXPECT().GetRegion(gomock.Any(), "us-ord").Return(nil, fmt.Errorf("cannot find region"))
-			},
-			expectedError: fmt.Errorf("cannot find region"),
-		},
-		{
-			name: "Error - SetUserData failed to get images",
-			machineScope: &scope.MachineScope{Machine: &v1beta1.Machine{
-				Spec: v1beta1.MachineSpec{
-					ClusterName: "",
-					Bootstrap: v1beta1.Bootstrap{
-						DataSecretName: ptr.To("test-data"),
-					},
-					InfrastructureRef: corev1.ObjectReference{},
-				},
-			}, LinodeMachine: &infrav1alpha2.LinodeMachine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-cluster",
-					Namespace: "default",
-				},
-				Spec:   infrav1alpha2.LinodeMachineSpec{Region: "us-ord", Image: "linode/ubuntu22.04"},
-				Status: infrav1alpha2.LinodeMachineStatus{},
-			}},
-			createConfig: &linodego.InstanceCreateOptions{},
-			wantConfig:   &linodego.InstanceCreateOptions{},
-			expects: func(mockClient *mock.MockLinodeClient, kMock *mock.MockK8sClient) {
-				kMock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error {
-					cred := corev1.Secret{
-						Data: map[string][]byte{
-							"value": []byte("hello"),
-						},
-					}
-					*obj = cred
-					return nil
-				})
-				mockClient.EXPECT().GetRegion(gomock.Any(), "us-ord").Return(&linodego.Region{
-					Capabilities: []string{"Metadata"},
-				}, nil)
-				mockClient.EXPECT().GetImage(gomock.Any(), "linode/ubuntu22.04").Return(nil, fmt.Errorf("cannot find image"))
-			},
-			expectedError: fmt.Errorf("cannot find image"),
-		},
 		{
 			name: "Error - SetUserData failed to get stackscripts",
 			machineScope: &scope.MachineScope{Machine: &v1beta1.Machine{
@@ -315,7 +234,7 @@ func TestSetUserData(t *testing.T) {
 					Namespace: "default",
 				},
 				Spec:   infrav1alpha2.LinodeMachineSpec{Region: "us-east", Image: "linode/ubuntu22.04", Type: "g6-standard-1"},
-				Status: infrav1alpha2.LinodeMachineStatus{},
+				Status: infrav1alpha2.LinodeMachineStatus{CloudinitMetadataSupport: false},
 			}},
 			createConfig: &linodego.InstanceCreateOptions{},
 			wantConfig: &linodego.InstanceCreateOptions{StackScriptID: 1234, StackScriptData: map[string]string{
@@ -332,10 +251,6 @@ func TestSetUserData(t *testing.T) {
 					*obj = cred
 					return nil
 				})
-				mockClient.EXPECT().GetRegion(gomock.Any(), "us-east").Return(&linodego.Region{
-					Capabilities: []string{"Metadata"},
-				}, nil)
-				mockClient.EXPECT().GetImage(gomock.Any(), "linode/ubuntu22.04").Return(&linodego.Image{}, nil)
 				mockClient.EXPECT().ListStackscripts(gomock.Any(), &linodego.ListOptions{Filter: "{\"label\":\"CAPL-dev\"}"}).Return(nil, fmt.Errorf("failed to get stackscripts"))
 			},
 			expectedError: fmt.Errorf("ensure stackscript: failed to get stackscript with label CAPL-dev: failed to get stackscripts"),