OCPBUGS-63411: machinesync: Create Cluster API InfraMachine if not exists while Cluster API Machine exists

pkg/controllers/machinesync/machine_sync_controller.go

@@ -191,14 +191,14 @@ func (r *MachineSyncReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 // Reconcile reconciles CAPI and MAPI machines for their respective namespaces.
 //
-//nolint:funlen
+//nolint:funlen,cyclop
 func (r *MachineSyncReconciler) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) {
 	logger := logf.FromContext(ctx, "namespace", req.Namespace, "name", req.Name)
 
 	logger.V(1).Info("Reconciling machine")
 	defer logger.V(1).Info("Finished reconciling machine")
 
-	var mapiMachineNotFound, capiMachineNotFound bool
+	var mapiMachineNotFound, capiMachineNotFound, capiInfraMachineExists bool
 
 	// Get the MAPI Machine.
 	mapiMachine := &mapiv1beta1.Machine{}
@@ -254,12 +254,25 @@ func (r *MachineSyncReconciler) Reconcile(ctx context.Context, req reconcile.Req
 		return ctrl.Result{}, nil
 	}
 
+	// Check for existence of the Cluster API Infrastructure Machine or if it needs to get created from MAPI first.
+	capiInfraMachineExists, err := r.doesCAPIInfraMachineExist(ctx, capiMachine, mapiMachine)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to check for existence of Cluster API infrastructure machine: %w", err)
+	}
+
 	authoritativeAPI := mapiMachine.Status.AuthoritativeAPI
 
 	switch {
 	case authoritativeAPI == mapiv1beta1.MachineAuthorityMachineAPI:
 		return r.reconcileMAPIMachinetoCAPIMachine(ctx, mapiMachine, capiMachine)
 	case authoritativeAPI == mapiv1beta1.MachineAuthorityClusterAPI && !capiMachineNotFound:
+		// Create Cluster API Infrastructure Machine from MAPI if it doesn't exist and no deletion is in progress.
+		// the CAPI machine is not marked for deletion and the MAPI machine was created before the Cluster API machine.
+		if capiMachine.DeletionTimestamp.IsZero() && mapiMachine.DeletionTimestamp.IsZero() &&
+			!capiInfraMachineExists && !mapiMachine.CreationTimestamp.After(capiMachine.CreationTimestamp.Time) {
+			return r.reconcileMAPIMachinetoCAPIMachine(ctx, mapiMachine, capiMachine)
+		}
+
 		return r.reconcileCAPIMachinetoMAPIMachine(ctx, capiMachine, mapiMachine)
 	case authoritativeAPI == mapiv1beta1.MachineAuthorityClusterAPI && capiMachineNotFound:
 		return r.reconcileMAPIMachinetoCAPIMachine(ctx, mapiMachine, capiMachine)
@@ -987,18 +1000,14 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap
 		return nil, nil, nil
 	}
 
-	var infraCluster, infraMachine client.Object
+	var infraCluster client.Object
 
 	infraClusterKey := client.ObjectKey{
 		Namespace: capiMachine.Namespace,
 		Name:      capiMachine.Spec.ClusterName,
 	}
 
 	infraMachineRef := capiMachine.Spec.InfrastructureRef
-	infraMachineKey := client.ObjectKey{
-		Namespace: infraMachineRef.Namespace,
-		Name:      infraMachineRef.Name,
-	}
 
 	// Validate that required references are not empty to avoid nil pointer issues later.
 	// These are terminal configuration errors that require user intervention.
@@ -1012,7 +1021,7 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap
 			capiMachine.Namespace, capiMachine.Name, errInvalidInfraMachineReference)
 	}
 
-	infraMachine, infraCluster, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform)
+	_, infraCluster, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform)
 	if err != nil {
 		return nil, nil, fmt.Errorf("unable to devise Cluster API infra resources: %w", err)
 	}
@@ -1021,13 +1030,50 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap
 		return nil, nil, fmt.Errorf("failed to get Cluster API infrastructure cluster: %w", err)
 	}
 
+	infraMachine, err := r.fetchCAPIInfraMachine(ctx, infraMachineRef.Name, infraMachineRef.Namespace)
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to fetch Cluster API infrastructure machine: %w", err)
+	}
+
+	return infraCluster, infraMachine, nil
+}
+
+func (r *MachineSyncReconciler) fetchCAPIInfraMachine(ctx context.Context, name, namespace string) (client.Object, error) {
+	infraMachine, _, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform)
+	if err != nil {
+		return nil, fmt.Errorf("unable to devise Cluster API infra resources: %w", err)
+	}
+
+	infraMachineKey := client.ObjectKey{
+		Namespace: namespace,
+		Name:      name,
+	}
+
 	if err := r.Get(ctx, infraMachineKey, infraMachine); err != nil && !apierrors.IsNotFound(err) {
-		return nil, nil, fmt.Errorf("failed to get Cluster API infrastructure machine: %w", err)
+		return nil, fmt.Errorf("failed to get: %w", err)
 	} else if apierrors.IsNotFound(err) {
 		infraMachine = nil
 	}
 
-	return infraCluster, infraMachine, nil
+	return infraMachine, nil
+}
+
+// doesCAPIInfraMachineExist checks if the Cluster API Infrastructure Machine exists. It uses the infrastructureRef of the Cluster API Machine with fallback to the name of the MAPI Machine.
+func (r *MachineSyncReconciler) doesCAPIInfraMachineExist(ctx context.Context, capiMachine *clusterv1.Machine, mapiMachine *mapiv1beta1.Machine) (bool, error) {
+	namespace := r.CAPINamespace
+	name := mapiMachine.Name
+
+	if capiMachine != nil {
+		name = capiMachine.Spec.InfrastructureRef.Name
+		namespace = capiMachine.Spec.InfrastructureRef.Namespace
+	}
+
+	infraMachine, err := r.fetchCAPIInfraMachine(ctx, name, namespace)
+	if err != nil {
+		return false, fmt.Errorf("checking existence: %w", err)
+	}
+
+	return infraMachine != nil, nil
 }
 
 //nolint:funlen,gocognit,cyclop

pkg/controllers/machinesync/machine_sync_controller_test.go

@@ -949,20 +949,19 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
 					})
 
 					Context("and the InfraMachine does not exist", func() {
-						It("should update the synchronized condition on the MAPI machine to False", func() {
-							// TODO: Revert this to a useful error message.
-							// We changed how fetchCAPIInfraResources behaves, so now we fail at a later point in the code.
-							// This still fails, the behaviour is the same - it's just a less useful error for an end user.
-							// We need to revisit fetchCAPIInfraResources.
+						It("should create the Cluster API InfraMachine", func() {
 							Eventually(k.Object(mapiMachine), timeout).Should(
 								HaveField("Status.Conditions", ContainElement(
 									SatisfyAll(
 										HaveField("Type", Equal(consts.SynchronizedCondition)),
-										HaveField("Status", Equal(corev1.ConditionFalse)),
-										HaveField("Reason", Equal("FailedToConvertCAPIMachineToMAPI")),
-										HaveField("Message", ContainSubstring("unexpected InfraMachine type, expected AWSMachine, got <nil>")),
+										HaveField("Status", Equal(corev1.ConditionTrue)),
+										HaveField("Reason", Equal("ResourceSynchronized")),
+										HaveField("Message", ContainSubstring("Successfully synchronized CAPI Machine to MAPI")),
 									))),
 							)
+
+							capiInfraMachine := awsv1resourcebuilder.AWSMachine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build()
+							Eventually(k.Get(capiInfraMachine), timeout).Should(Succeed())
 						})
 					})
 				})

pkg/controllers/machinesync/machine_sync_controller_unit_test.go

@@ -0,0 +1,119 @@
+/*
+Copyright 2024 Red Hat, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package machinesync
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	configv1 "github.com/openshift/api/config/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+// Unit tests for fetchCAPIInfraResources because reconciling deletion depends on it.
+var _ = Describe("Unit tests for fetchCAPIInfraResources", func() {
+	var reconciler *MachineSyncReconciler
+	var capiMachine *clusterv1.Machine
+
+	BeforeEach(func() {
+		capiMachine = &clusterv1.Machine{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-machine",
+				Namespace: "test-namespace",
+			},
+			Spec: clusterv1.MachineSpec{
+				ClusterName: "test-cluster",
+				InfrastructureRef: corev1.ObjectReference{
+					Kind:      "AWSMachine",
+					Name:      "test-machine",
+					Namespace: "test-namespace",
+				},
+			},
+		}
+
+		reconciler = &MachineSyncReconciler{
+			Scheme:   testEnv.Scheme,
+			Platform: configv1.AWSPlatformType,
+		}
+	})
+
+	Describe("when fetching Cluster API Infrastructure Resources", func() {
+		Context("when capiMachine is nil", func() {
+			It("should return nil and no error", func() {
+				reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).Build()
+				infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, nil)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(infraCluster).To(BeNil())
+				Expect(infraMachine).To(BeNil())
+			})
+		})
+
+		Context("and Infrastructure Cluster is not present", func() {
+			It("should return nil for both infra cluster and infra machine", func() {
+				reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).Build()
+
+				infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, capiMachine)
+				Expect(err).To(HaveOccurred())
+				Expect(err).To(MatchError(ContainSubstring("failed to get Cluster API infrastructure cluster")))
+				Expect(infraCluster).To(BeNil())
+				Expect(infraMachine).To(BeNil())
+			})
+		})
+
+		Context("and Infrastructure is present", func() {
+			var objs []client.Object
+			BeforeEach(func() {
+				objs = []client.Object{
+					&awsv1.AWSCluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      capiMachine.Spec.ClusterName,
+							Namespace: capiMachine.Namespace,
+						},
+					},
+				}
+			})
+
+			It("should return nil if infrastructure machine is not present", func() {
+				reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).WithObjects(objs...).Build()
+				infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, capiMachine)
+
+				Expect(err).ToNot(HaveOccurred())
+				Expect(infraCluster).ToNot(BeNil())
+				Expect(infraMachine).To(BeNil())
+			})
+
+			It("should return infrastructure machine if it is present", func() {
+				objs = append(objs, &awsv1.AWSMachine{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      capiMachine.Name,
+						Namespace: capiMachine.Namespace,
+					},
+				})
+				reconciler.Client = fake.NewClientBuilder().WithScheme(testEnv.Scheme).WithObjects(objs...).Build()
+				infraCluster, infraMachine, err := reconciler.fetchCAPIInfraResources(ctx, capiMachine)
+
+				Expect(err).ToNot(HaveOccurred())
+				Expect(infraCluster).ToNot(BeNil())
+				Expect(infraMachine).ToNot(BeNil())
+			})
+		})
+	})
+})