From 961a450b18b4c39b82eaf4a03415d0b3b694c323 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Tue, 23 Sep 2025 11:11:52 +0100 Subject: [PATCH 1/2] Updates InfraMachine watch_filters --- .../machinesync/machine_sync_controller.go | 55 ++++- .../machine_sync_controller_test.go | 229 ++++++++++++------ pkg/controllers/machinesync/suite_test.go | 5 +- pkg/util/watch_filters.go | 46 +++- 4 files changed, 245 insertions(+), 90 deletions(-) diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index 39ae65243..de43d3525 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -122,6 +122,12 @@ var ( // errUnsupportedCPMSOwnedMachineConversion is returned when attempting to convert ControlPlaneMachineSet owned machines. errUnsupportedCPMSOwnedMachineConversion = errors.New("conversion of control plane machines owned by control plane machine set is currently not supported") + + // errInvalidInfraClusterReference is returned when the cluster name is empty in CAPI machine spec. + errInvalidInfraClusterReference = errors.New("cluster name is empty in CAPI machine spec") + + // errInvalidInfraMachineReference is returned when the infrastructure machine reference is invalid or incomplete. + errInvalidInfraMachineReference = errors.New("infrastructure machine reference is invalid or incomplete") ) // MachineSyncReconciler reconciles CAPI and MAPI machines. @@ -136,7 +142,7 @@ type MachineSyncReconciler struct { MAPINamespace string } -// SetupWithManager sets the CoreClusterReconciler controller up with the given manager. +// SetupWithManager sets the MachineSyncReconciler controller up with the given manager. func (r *MachineSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { infraMachine, _, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform) if err != nil { @@ -163,7 +169,7 @@ func (r *MachineSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { ). Watches( infraMachine, - handler.EnqueueRequestsFromMapFunc(util.RewriteNamespace(r.MAPINamespace)), + handler.EnqueueRequestsFromMapFunc(util.ResolveCAPIMachineFromInfraMachine(r.MAPINamespace)), builder.WithPredicates(util.FilterNamespace(r.CAPINamespace)), ). Complete(r); err != nil { @@ -282,11 +288,20 @@ func (r *MachineSyncReconciler) reconcileCAPIMachinetoMAPIMachine(ctx context.Co } infraCluster, infraMachine, err := r.fetchCAPIInfraResources(ctx, sourceCAPIMachine) - if err != nil { + if err != nil { //nolint: nestif fetchErr := fmt.Errorf("failed to fetch Cluster API infra resources: %w", err) if existingMAPIMachine == nil { + // Don't requeue for terminal configuration errors + if errors.Is(err, errInvalidInfraClusterReference) || errors.Is(err, errInvalidInfraMachineReference) { + logger.Error(err, "Detected terminal configuration error - cluster name or infrastructure machine reference is empty, not requeuing") + r.Recorder.Event(sourceCAPIMachine, corev1.EventTypeWarning, "SynchronizationError", fetchErr.Error()) + + return ctrl.Result{}, nil + } + r.Recorder.Event(sourceCAPIMachine, corev1.EventTypeWarning, "SynchronizationWarning", fetchErr.Error()) + return ctrl.Result{}, fetchErr } @@ -295,6 +310,12 @@ func (r *MachineSyncReconciler) reconcileCAPIMachinetoMAPIMachine(ctx context.Co return ctrl.Result{}, utilerrors.NewAggregate([]error{fetchErr, condErr}) } + // Don't requeue for terminal configuration errors + if errors.Is(err, errInvalidInfraClusterReference) || errors.Is(err, errInvalidInfraMachineReference) { + logger.Error(err, "Detected terminal configuration error - cluster name or infrastructure machine reference is empty, not requeuing") + return ctrl.Result{}, nil + } + return ctrl.Result{}, fetchErr } @@ -410,6 +431,12 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co return ctrl.Result{}, utilerrors.NewAggregate([]error{fetchErr, condErr}) } + // Don't requeue for terminal configuration errors + if errors.Is(err, errInvalidInfraClusterReference) || errors.Is(err, errInvalidInfraMachineReference) { + logger.Error(err, "Detected terminal configuration error - cluster name or infrastructure machine reference is empty, not requeuing") + return ctrl.Result{}, nil + } + return ctrl.Result{}, fetchErr } @@ -949,6 +976,18 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap 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. + if capiMachine.Spec.ClusterName == "" { + return nil, nil, fmt.Errorf("machine %s/%s: %w", + capiMachine.Namespace, capiMachine.Name, errInvalidInfraClusterReference) + } + + if infraMachineRef.Name == "" || infraMachineRef.Namespace == "" { + return nil, nil, fmt.Errorf("machine %s/%s: %w", + capiMachine.Namespace, capiMachine.Name, errInvalidInfraMachineReference) + } + infraMachine, infraCluster, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform) if err != nil { return nil, nil, fmt.Errorf("unable to devise Cluster API infra resources: %w", err) @@ -968,7 +1007,9 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap } //nolint:funlen,gocognit,cyclop -func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (bool, error) { +func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (shouldRequeue bool, err error) { + logger := logf.FromContext(ctx) + if mapiMachine.DeletionTimestamp.IsZero() { if capiMachine == nil || capiMachine.DeletionTimestamp.IsZero() { // Neither MAPI authoritative machine nor its CAPI non-authoritative machine mirror @@ -986,8 +1027,6 @@ func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.C return true, nil } - logger := logf.FromContext(ctx) - if capiMachine == nil && util.IsNilObject(infraMachine) { logger.Info("Cluster API machine and infra machine do not exist, removing corresponding Machine API machine sync finalizer") // We don't have a capi machine or infra resouorces to clean up we can @@ -1185,9 +1224,7 @@ func (r *MachineSyncReconciler) reconcileCAPItoMAPIMachineDeletion(ctx context.C // ensureSyncFinalizer ensures the sync finalizer is present across the mapi // machine, capi machine and capi infra machine. -func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (bool, error) { - var shouldRequeue bool - +func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (shouldRequeue bool, err error) { var errors []error if mapiMachine != nil { diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index 939e73303..aa23de93a 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -191,6 +191,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() { mgr, err = ctrl.NewManager(controllerCfg, ctrl.Options{ Scheme: testScheme, + Logger: GinkgoLogr, Controller: config.Controller{ SkipNameValidation: ptr.To(true), }, @@ -235,11 +236,6 @@ var _ = Describe("With a running MachineSync Reconciler", func() { }) Context("when all the CAPI infra resources exist", func() { - BeforeEach(func() { - By("Creating the CAPI infra machine") - Eventually(k8sClient.Create(ctx, capaMachine)).Should(Succeed(), "capa machine should be able to be created") - }) - Context("when the MAPI machine has MachineAuthority set to Machine API", func() { BeforeEach(func() { By("Creating the MAPI machine") @@ -272,11 +268,28 @@ var _ = Describe("With a running MachineSync Reconciler", func() { }) }) - Context("when the CAPI machine does exist", func() { + Context("when the CAPI machine and infra machine do exist", func() { BeforeEach(func() { By("Creating the CAPI machine") capiMachine = capiMachineBuilder.Build() Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + + By("Creating the CAPI infra machine") + // we must set the capi machine as an owner of the capa machine + // in order to ensure we reconcile capa changes in our sync controller. + capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{ + { + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }, + }).Build() + + Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created") + }) It("should update the synchronized condition on the MAPI machine to True", func() { @@ -290,41 +303,41 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ))), ) }) - }) - Context("when the MAPI machine providerSpec gets updated", func() { - BeforeEach(func() { - By("Updating the MAPI machine providerSpec") - modifiedMAPIMachineBuilder := machinev1resourcebuilder.Machine(). - WithNamespace(mapiNamespace.GetName()). - WithName(mapiMachine.Name). - WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil).WithInstanceType("m6i.2xlarge")).Build() + Context("when the MAPI machine providerSpec gets updated", func() { + BeforeEach(func() { + By("Updating the MAPI machine providerSpec") + modifiedMAPIMachineBuilder := machinev1resourcebuilder.Machine(). + WithNamespace(mapiNamespace.GetName()). + WithName(mapiMachine.Name). + WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil).WithInstanceType("m6i.2xlarge")).Build() - Eventually(k.Update(mapiMachine, func() { - mapiMachine.Spec.ProviderSpec = modifiedMAPIMachineBuilder.Spec.ProviderSpec - })).Should(Succeed(), "mapi machine providerSpec should be able to be updated") - }) + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.ProviderSpec = modifiedMAPIMachineBuilder.Spec.ProviderSpec + })).Should(Succeed(), "mapi machine providerSpec should be able to be updated") + }) - It("should recreate the CAPI infra machine", func() { - capaMachineBuilder = awsv1resourcebuilder.AWSMachine(). - WithNamespace(capiNamespace.GetName()). - WithName(mapiMachine.Name) + It("should recreate the CAPI infra machine", func() { + capaMachineBuilder = awsv1resourcebuilder.AWSMachine(). + WithNamespace(capiNamespace.GetName()). + WithName(mapiMachine.Name) - Eventually(k.Object(capaMachineBuilder.Build()), timeout).Should( - HaveField("Spec.InstanceType", Equal("m6i.2xlarge")), - ) - }) + Eventually(k.Object(capaMachineBuilder.Build()), timeout).Should( + HaveField("Spec.InstanceType", Equal("m6i.2xlarge")), + ) + }) - It("should update the synchronized condition on the MAPI machine to True", func() { - Eventually(k.Object(mapiMachine), timeout).Should( - HaveField("Status.Conditions", ContainElement( - SatisfyAll( - HaveField("Type", Equal(consts.SynchronizedCondition)), - HaveField("Status", Equal(corev1.ConditionTrue)), - HaveField("Reason", Equal("ResourceSynchronized")), - HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), - ))), - ) + It("should update the synchronized condition on the MAPI machine to True", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), + ))), + ) + }) }) }) }) @@ -340,6 +353,26 @@ var _ = Describe("With a running MachineSync Reconciler", func() { capiMachine = capiMachineBuilder.Build() Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + By("Creating the CAPI infra machine") + // we must set the capi machine as an owner of the capa machine + // in order to ensure we reconcile capa changes in our sync controller. + + // Updates the capaMachineBuilder with the correct owner ref, + // so when we use it later on, we don't need to repeat ourselves. + capaMachineBuilder = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{ + { + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }, + }) + + capaMachine = capaMachineBuilder.Build() + Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created") + By("Setting the MAPI machine AuthoritativeAPI to Cluster API") Eventually(k.UpdateStatus(mapiMachine, func() { mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI @@ -429,18 +462,32 @@ var _ = Describe("With a running MachineSync Reconciler", func() { Eventually(k.UpdateStatus(mapiMachine, func() { mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI })).Should(Succeed()) + + capiMachine = capiMachineBuilder.WithName(mapiMachine.Name).Build() }) It("should successfully create the CAPI machine", func() { - Eventually(k.Get( - clusterv1resourcebuilder.Machine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(), - )).Should(Succeed()) + Eventually(k.Get(capiMachine)).Should(Succeed()) }) - It("should successfully create the CAPA machine", func() { - Eventually(k.Get( - awsv1resourcebuilder.AWSMachine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(), - )).Should(Succeed()) + It("should successfully create the CAPA machine with correct owner references", func() { + // Get the CAPI machine, so we know the UID in the api server + Eventually(k.Get(capiMachine)).Should(Succeed()) + + // the MAPI Machine builder uses generateName, the controller makes an infra machine + // with the same name, so in order to avoid a 404 we rebuild the capa machine. + capaMachine = capaMachineBuilder.WithName(mapiMachine.Name).Build() + + Eventually(k.Object(capaMachine), timeout).Should( + HaveField("ObjectMeta.OwnerReferences", ContainElement( + SatisfyAll( + HaveField("Kind", Equal(machineKind)), + HaveField("APIVersion", Equal(clusterv1.GroupVersion.String())), + HaveField("Name", Equal(capiMachine.Name)), + HaveField("UID", Equal(capiMachine.UID)), + ))), + ) + }) It("should update the synchronized condition on the MAPI machine to True", func() { @@ -497,7 +544,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() { Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") - By("Setting the status.authoritativeAPI to Migrating") + By("Setting the status.authoritativeAPI to the empty string") Eventually(k.UpdateStatus(mapiMachine, func() { mapiMachine.Status.AuthoritativeAPI = "" })).Should(Succeed()) @@ -514,21 +561,32 @@ var _ = Describe("With a running MachineSync Reconciler", func() { Context("when the MAPI machine does not exist and the CAPI machine does", func() { Context("and there is no CAPI machineSet owning the machine", func() { BeforeEach(func() { - By("Creating the CAPI machine") - capiMachine = capiMachineBuilder.Build() - Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") - By("Updating the CAPA machine adding the CAPI machine as an owner") - Eventually(k.Update(capaMachine, func() { - capaMachine.OwnerReferences = append(capaMachine.OwnerReferences, metav1.OwnerReference{ - Kind: machineKind, - APIVersion: clusterv1.GroupVersion.String(), - Name: capiMachine.Name, - UID: capiMachine.UID, - BlockOwnerDeletion: ptr.To(true), - Controller: ptr.To(false), - }) - })).Should(Succeed(), "capa machine should be able to be updated") + // The CAPI Machine must reference the CAPA machine, + // so build the CAPA machine (without the owner reference) fist. + // Note we don't create it on the API server yet. + noMachineSetCapaMachineBuilder := capaMachineBuilder.WithName("test-machine-no-machineset") + capaMachine = noMachineSetCapaMachineBuilder.Build() + + capiMachine = capiMachineBuilder.WithName("test-machine-no-machineset").WithInfrastructureRef(corev1.ObjectReference{ + Kind: capaMachine.Kind, + Name: capaMachine.GetName(), + Namespace: capaMachine.GetNamespace(), + }).Build() + Expect(k8sClient.Create(ctx, capiMachine)).Should(Succeed()) + + // Once we have created the CAPI Machine, and have a UID, add the owner reference to the CAPA machine + // then we can Create() on the API server + noMachineSetCapaMachineBuilder = noMachineSetCapaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{{ + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }}) + capaMachine = noMachineSetCapaMachineBuilder.Build() + Expect(k8sClient.Create(ctx, capaMachine)).Should(Succeed()) }) It("should not make any changes to the CAPI machine", func() { @@ -546,7 +604,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() { Context("when MAPI machine with the same name and status.authoritativeAPI set to ClusterAPI is created", func() { BeforeEach(func() { - mapiMachine = mapiMachineBuilder.WithName(capiMachine.Name).WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).Build() + mapiMachine = mapiMachineBuilder.WithGenerateName("").WithName(capiMachine.Name).WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).Build() Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") By("Setting the status.authoritativeAPI to Cluster API") @@ -589,6 +647,19 @@ var _ = Describe("With a running MachineSync Reconciler", func() { By("Creating the CAPI machine") capiMachine = capiMachineBuilder.WithOwnerReferences(ownerReferencesToCapiMachineSet).Build() Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + + // Because we expect our controller to sync from CAPI -> MAPI, + // we must ensure the CAPA machine exists, with the correct owner ref. + capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{{ + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }}).Build() + Expect(k8sClient.Create(ctx, capaMachine)).Should(Succeed()) + }) Context("with no MAPI counterpart", func() { @@ -859,9 +930,6 @@ var _ = Describe("With a running MachineSync Reconciler", func() { }, timeout).Should(Succeed()) } - By("Creating the CAPI infra machine") - Eventually(k8sClient.Create(ctx, capaMachine), timeout).Should(Succeed(), "capa machine should be able to be created") - By("Creating the MAPI machine") mapiMachine = mapiMachineBuilder.WithName("test-machine").WithLabels(map[string]string{ "machine.openshift.io/cluster-api-cluster": "ci-op-gs2k97d6-c9e33-2smph", @@ -892,6 +960,20 @@ var _ = Describe("With a running MachineSync Reconciler", func() { Eventually(k.Get(capiMachine)).Should(Succeed()) Eventually(k.Get(mapiMachine)).Should(Succeed()) + By("Creating the CAPI infra machine") + capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{ + { + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }, + }).Build() + + Eventually(k8sClient.Create(ctx, capaMachine), timeout).Should(Succeed(), "capa machine should be able to be created") + }) AfterEach(func() { @@ -947,21 +1029,22 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ) By("Creating a throwaway MAPI machine") - testMachine := mapiMachineBuilder.WithGenerateName("test-machine").Build() - Eventually(k8sClient.Create(ctx, testMachine), timeout).Should(Succeed()) + sentinelMachine := mapiMachineBuilder.WithGenerateName("sentinel-machine").Build() + Eventually(k8sClient.Create(ctx, sentinelMachine), timeout).Should(Succeed()) By("Setting the throwaway MAPI machine AuthoritativeAPI to Cluster API") - Eventually(k.UpdateStatus(testMachine, func() { - testMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + Eventually(k.UpdateStatus(sentinelMachine, func() { + sentinelMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI })).Should(Succeed()) - Eventually(k.Object(testMachine), timeout).Should( + Eventually(k.Object(sentinelMachine), timeout).Should( HaveField("Status.AuthoritativeAPI", Equal(mapiv1beta1.MachineAuthorityClusterAPI))) - Eventually(k.Update(testMachine, func() { - testMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} + Eventually(k.Update(sentinelMachine, func() { + sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} }), timeout).Should(MatchError(ContainSubstring("policy in place"))) }) + Context("with status.AuthoritativeAPI: Machine API", func() { BeforeEach(func() { By("Setting the MAPI machine AuthoritativeAPI to Machine API") @@ -1146,12 +1229,12 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ), ) - checkVAPMachine := clusterv1resourcebuilder.Machine().WithName("vap-checking-machine").WithNamespace(capiNamespace.Name).Build() - Eventually(k8sClient.Create(ctx, checkVAPMachine)).Should(Succeed(), "check vap machine should be able to be created") + sentinelMachine := clusterv1resourcebuilder.Machine().WithName("sentinel-machine").WithNamespace(capiNamespace.Name).Build() + Expect(k8sClient.Create(ctx, sentinelMachine)).To(Succeed()) // Continually try to update the capiMachine to a forbidden field until the VAP blocks it - Eventually(k.Update(checkVAPMachine, func() { - checkVAPMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} + Eventually(k.Update(sentinelMachine, func() { + sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"} }), timeout).Should(MatchError(ContainSubstring("policy in place"))) }) diff --git a/pkg/controllers/machinesync/suite_test.go b/pkg/controllers/machinesync/suite_test.go index 187d6f07a..d6cead13f 100644 --- a/pkg/controllers/machinesync/suite_test.go +++ b/pkg/controllers/machinesync/suite_test.go @@ -33,7 +33,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "k8s.io/klog/v2" - "k8s.io/klog/v2/textlogger" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" @@ -64,7 +64,8 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { klog.SetOutput(GinkgoWriter) - logf.SetLogger(textlogger.NewLogger(textlogger.NewConfig())) + logf.SetLogger(GinkgoLogr) + ctrl.SetLogger(GinkgoLogr) By("bootstrapping test environment") var err error diff --git a/pkg/util/watch_filters.go b/pkg/util/watch_filters.go index 32c9525bd..6e23b6716 100644 --- a/pkg/util/watch_filters.go +++ b/pkg/util/watch_filters.go @@ -21,8 +21,10 @@ import ( "fmt" "github.com/openshift/cluster-capi-operator/pkg/controllers" - "k8s.io/klog/v2" + "k8s.io/apimachinery/pkg/runtime/schema" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -36,12 +38,12 @@ const machineSetKind = "MachineSet" // there to be a mirror object in the MAPI namespace. func RewriteNamespace(namespace string) func(context.Context, client.Object) []reconcile.Request { return func(ctx context.Context, obj client.Object) []reconcile.Request { - klog.V(4).Info( - "reconcile triggered by object", + logger := logf.FromContext(ctx).WithValues( "objectType", fmt.Sprintf("%T", obj), "namespace", obj.GetNamespace(), "name", obj.GetName(), ) + logger.V(4).Info("reconcile triggered") return []reconcile.Request{{ NamespacedName: client.ObjectKey{Namespace: namespace, Name: obj.GetName()}, @@ -55,19 +57,19 @@ func RewriteNamespace(namespace string) func(context.Context, client.Object) []r // for the corresponding MachineSet in the MAPI namespace to trigger reconciliation of the mirror MAPI MachineSet. func ResolveCAPIMachineSetFromInfraMachineTemplate(namespace string) func(context.Context, client.Object) []reconcile.Request { return func(ctx context.Context, obj client.Object) []reconcile.Request { - klog.V(4).Info( - "reconcile triggered by object", + logger := logf.FromContext(ctx).WithValues( "objectType", fmt.Sprintf("%T", obj), "namespace", obj.GetNamespace(), "name", obj.GetName(), ) + logger.V(4).Info("reconcile triggered") objLabels := obj.GetLabels() requests := []reconcile.Request{} machineSetName, ok := objLabels[controllers.MachineSetOpenshiftLabelKey] if ok { - klog.V(4).Info("Object has machine.openshift.io/cluster-api-machineset label, enqueueing request", + logger.V(4).Info("Object has machine.openshift.io/cluster-api-machineset label, enqueueing request", "InfraMachineTemplate", obj.GetName(), machineSetKind, machineSetName) requests = append(requests, reconcile.Request{ @@ -79,6 +81,38 @@ func ResolveCAPIMachineSetFromInfraMachineTemplate(namespace string) func(contex } } +// ResolveCAPIMachineFromInfraMachine resolves a CAPI Machine from an InfraMachine. It takes client.Object, +// and uses owner references to determine the owning CAPI machine. If one is found, it returns a reconcile.Request +// for the corresponding MAPI Machine in the MAPI namespace to trigger reconciliation of the mirror MAPI Machine. +func ResolveCAPIMachineFromInfraMachine(namespace string) func(context.Context, client.Object) []reconcile.Request { + return func(ctx context.Context, obj client.Object) []reconcile.Request { + logger := logf.FromContext(ctx).WithValues( + "objectType", fmt.Sprintf("%T", obj), + "namespace", obj.GetNamespace(), + "name", obj.GetName(), + ) + logger.V(4).Info("reconcile triggered") + + requests := []reconcile.Request{} + + for _, ref := range obj.GetOwnerReferences() { + gv, err := schema.ParseGroupVersion(ref.APIVersion) + if err != nil { + logger.Error(err, "Failed to parse GroupVersion", "APIVersion", ref.APIVersion) + continue + } + + if ref.Kind == "Machine" && gv.Group == clusterv1.GroupVersion.Group { + requests = append(requests, reconcile.Request{ + NamespacedName: client.ObjectKey{Namespace: namespace, Name: ref.Name}, + }) + } + } + + return requests + } +} + // FilterNamespace filters a client.Object request, ensuring they are in the // namespace provided. func FilterNamespace(namespace string) predicate.Predicate { From 72dbdfdea1e99fd92fad9f6070b154861ff293c5 Mon Sep 17 00:00:00 2001 From: Theo Barber-Bany Date: Tue, 28 Oct 2025 17:08:36 +0000 Subject: [PATCH 2/2] WIP: test-fu --- .../machinesync/machine_sync_controller.go | 14 +- .../machine_sync_controller_test.go | 746 +++++++++--------- 2 files changed, 381 insertions(+), 379 deletions(-) diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index de43d3525..59708d4a4 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -124,12 +124,16 @@ var ( errUnsupportedCPMSOwnedMachineConversion = errors.New("conversion of control plane machines owned by control plane machine set is currently not supported") // errInvalidInfraClusterReference is returned when the cluster name is empty in CAPI machine spec. - errInvalidInfraClusterReference = errors.New("cluster name is empty in CAPI machine spec") + errInvalidInfraClusterReference = errors.New("cluster name is empty in Cluster API machine spec") // errInvalidInfraMachineReference is returned when the infrastructure machine reference is invalid or incomplete. errInvalidInfraMachineReference = errors.New("infrastructure machine reference is invalid or incomplete") ) +const ( + terminalConfigurationErrorLog string = "Detected terminal configuration error - cluster name or infrastructure machine reference is empty, not requeuing" +) + // MachineSyncReconciler reconciles CAPI and MAPI machines. type MachineSyncReconciler struct { client.Client @@ -288,13 +292,13 @@ func (r *MachineSyncReconciler) reconcileCAPIMachinetoMAPIMachine(ctx context.Co } infraCluster, infraMachine, err := r.fetchCAPIInfraResources(ctx, sourceCAPIMachine) - if err != nil { //nolint: nestif + if err != nil { //nolint:nestif fetchErr := fmt.Errorf("failed to fetch Cluster API infra resources: %w", err) if existingMAPIMachine == nil { // Don't requeue for terminal configuration errors if errors.Is(err, errInvalidInfraClusterReference) || errors.Is(err, errInvalidInfraMachineReference) { - logger.Error(err, "Detected terminal configuration error - cluster name or infrastructure machine reference is empty, not requeuing") + logger.Error(err, terminalConfigurationErrorLog) r.Recorder.Event(sourceCAPIMachine, corev1.EventTypeWarning, "SynchronizationError", fetchErr.Error()) return ctrl.Result{}, nil @@ -312,7 +316,7 @@ func (r *MachineSyncReconciler) reconcileCAPIMachinetoMAPIMachine(ctx context.Co // Don't requeue for terminal configuration errors if errors.Is(err, errInvalidInfraClusterReference) || errors.Is(err, errInvalidInfraMachineReference) { - logger.Error(err, "Detected terminal configuration error - cluster name or infrastructure machine reference is empty, not requeuing") + logger.Error(err, terminalConfigurationErrorLog) return ctrl.Result{}, nil } @@ -433,7 +437,7 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co // Don't requeue for terminal configuration errors if errors.Is(err, errInvalidInfraClusterReference) || errors.Is(err, errInvalidInfraMachineReference) { - logger.Error(err, "Detected terminal configuration error - cluster name or infrastructure machine reference is empty, not requeuing") + logger.Error(err, terminalConfigurationErrorLog) return ctrl.Result{}, nil } diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index aa23de93a..c5e2d4e9a 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -235,24 +235,95 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ) }) - Context("when all the CAPI infra resources exist", func() { - Context("when the MAPI machine has MachineAuthority set to Machine API", func() { + Context("when the MAPI machine has MachineAuthority set to Machine API", func() { + BeforeEach(func() { + By("Creating the MAPI machine") + mapiMachine = mapiMachineBuilder.Build() + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + + By("Setting the MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + }) + + Context("when the CAPI machine does not exist", func() { + It("should create a paused CAPI machine", func() { + Eventually(k.Get( + clusterv1resourcebuilder.Machine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(), + )).Should(Succeed()) + }) + + It("should update the synchronized condition on the MAPI machine to True", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), + ))), + ) + }) + }) + + Context("when the CAPI machine and infra machine do exist", func() { BeforeEach(func() { - By("Creating the MAPI machine") - mapiMachine = mapiMachineBuilder.Build() - Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + By("Creating the CAPI machine") + capiMachine = capiMachineBuilder.Build() + Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + + By("Creating the CAPI infra machine") + // we must set the capi machine as an owner of the capa machine + // in order to ensure we reconcile capa changes in our sync controller. + capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{ + { + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }, + }).Build() + + Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created") - By("Setting the MAPI machine AuthoritativeAPI to MachineAPI") - Eventually(k.UpdateStatus(mapiMachine, func() { - mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI - })).Should(Succeed()) }) - Context("when the CAPI machine does not exist", func() { - It("should create a paused CAPI machine", func() { - Eventually(k.Get( - clusterv1resourcebuilder.Machine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(), - )).Should(Succeed()) + It("should update the synchronized condition on the MAPI machine to True", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), + ))), + ) + }) + + Context("when the MAPI machine providerSpec gets updated", func() { + BeforeEach(func() { + By("Updating the MAPI machine providerSpec") + modifiedMAPIMachineBuilder := machinev1resourcebuilder.Machine(). + WithNamespace(mapiNamespace.GetName()). + WithName(mapiMachine.Name). + WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil).WithInstanceType("m6i.2xlarge")).Build() + + Eventually(k.Update(mapiMachine, func() { + mapiMachine.Spec.ProviderSpec = modifiedMAPIMachineBuilder.Spec.ProviderSpec + })).Should(Succeed(), "mapi machine providerSpec should be able to be updated") + }) + + It("should recreate the CAPI infra machine", func() { + capaMachineBuilder = awsv1resourcebuilder.AWSMachine(). + WithNamespace(capiNamespace.GetName()). + WithName(mapiMachine.Name) + + Eventually(k.Object(capaMachineBuilder.Build()), timeout).Should( + HaveField("Spec.InstanceType", Equal("m6i.2xlarge")), + ) }) It("should update the synchronized condition on the MAPI machine to True", func() { @@ -267,29 +338,61 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ) }) }) + }) + }) - Context("when the CAPI machine and infra machine do exist", func() { - BeforeEach(func() { - By("Creating the CAPI machine") - capiMachine = capiMachineBuilder.Build() - Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") - - By("Creating the CAPI infra machine") - // we must set the capi machine as an owner of the capa machine - // in order to ensure we reconcile capa changes in our sync controller. - capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{ - { - Kind: machineKind, - APIVersion: clusterv1.GroupVersion.String(), - Name: capiMachine.Name, - UID: capiMachine.UID, - BlockOwnerDeletion: ptr.To(true), - Controller: ptr.To(false), - }, - }).Build() + Context("when the MAPI machine has MachineAuthority set to Cluster API", func() { + BeforeEach(func() { + + By("Creating the MAPI machine") + mapiMachine = mapiMachineBuilder.Build() + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + + By("Creating the CAPI Machine") + capiMachine = capiMachineBuilder.Build() + Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + + By("Creating the CAPI infra machine") + // we must set the capi machine as an owner of the capa machine + // in order to ensure we reconcile capa changes in our sync controller. + + // Updates the capaMachineBuilder with the correct owner ref, + // so when we use it later on, we don't need to repeat ourselves. + capaMachineBuilder = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{ + { + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }, + }) + + capaMachine = capaMachineBuilder.Build() + Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created") + + By("Setting the MAPI machine AuthoritativeAPI to Cluster API") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + })).Should(Succeed()) + + }) - Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created") + Context("when a MAPI counterpart exists", func() { + Context("when the CAPI Infra Machine gets updated", func() { + BeforeEach(func() { + By("Updating the CAPI Infra Machine (CAPA Machine)") + modifiedCapaMachine := capaMachineBuilder.WithInstanceType("m7i.4xlarge").Build() + modifiedCapaMachine.ResourceVersion = capaMachine.GetResourceVersion() + Eventually(k8sClient.Update(ctx, modifiedCapaMachine)).Should(Succeed(), "capa machine should be able to be updated") + }) + It("should update the MAPI provider spec", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + WithTransform(awsProviderSpecFromMachine, + HaveField("InstanceType", Equal("m7i.4xlarge")), + )) }) It("should update the synchronized condition on the MAPI machine to True", func() { @@ -299,296 +402,266 @@ var _ = Describe("With a running MachineSync Reconciler", func() { HaveField("Type", Equal(consts.SynchronizedCondition)), HaveField("Status", Equal(corev1.ConditionTrue)), HaveField("Reason", Equal("ResourceSynchronized")), - HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), + HaveField("Message", Equal("Successfully synchronized CAPI Machine to MAPI")), ))), ) }) + }) + }) + }) - Context("when the MAPI machine providerSpec gets updated", func() { - BeforeEach(func() { - By("Updating the MAPI machine providerSpec") - modifiedMAPIMachineBuilder := machinev1resourcebuilder.Machine(). - WithNamespace(mapiNamespace.GetName()). - WithName(mapiMachine.Name). - WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil).WithInstanceType("m6i.2xlarge")).Build() - - Eventually(k.Update(mapiMachine, func() { - mapiMachine.Spec.ProviderSpec = modifiedMAPIMachineBuilder.Spec.ProviderSpec - })).Should(Succeed(), "mapi machine providerSpec should be able to be updated") - }) + Context("when the MAPI machine has status.authoritativeAPI set to MachineAPI and has CPMS owner reference", func() { + BeforeEach(func() { + fakeCPMSOwnerReference := metav1.OwnerReference{ + APIVersion: mapiv1beta1.GroupVersion.String(), + Kind: "ControlPlaneMachineSet", + Name: "cluster", + UID: "cpms-uid", + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), + } - It("should recreate the CAPI infra machine", func() { - capaMachineBuilder = awsv1resourcebuilder.AWSMachine(). - WithNamespace(capiNamespace.GetName()). - WithName(mapiMachine.Name) + By("Creating the MAPI machine") + mapiMachine = mapiMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{fakeCPMSOwnerReference}).Build() + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") - Eventually(k.Object(capaMachineBuilder.Build()), timeout).Should( - HaveField("Spec.InstanceType", Equal("m6i.2xlarge")), - ) - }) + By("Setting the MAPI machine status.authoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + }) - It("should update the synchronized condition on the MAPI machine to True", func() { - Eventually(k.Object(mapiMachine), timeout).Should( - HaveField("Status.Conditions", ContainElement( - SatisfyAll( - HaveField("Type", Equal(consts.SynchronizedCondition)), - HaveField("Status", Equal(corev1.ConditionTrue)), - HaveField("Reason", Equal("ResourceSynchronized")), - HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), - ))), - ) - }) - }) - }) + It("should not create the CAPI machine", func() { + Consistently(k.Get( + clusterv1resourcebuilder.Machine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(), + )).Should(Not(Succeed())) }) - Context("when the MAPI machine has MachineAuthority set to Cluster API", func() { - BeforeEach(func() { + It("should update the synchronized condition on the MAPI machine to False", 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("FailedToConvertMAPIMachineToCAPI")), + HaveField("Message", Equal("conversion of control plane machines owned by control plane machine set is currently not supported")), + ))), + ) + }) - By("Creating the MAPI machine") - mapiMachine = mapiMachineBuilder.Build() - Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + }) - By("Creating the CAPI Machine") - capiMachine = capiMachineBuilder.Build() - Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + Context("when the MAPI machine has status.authoritativeAPI set to MachineAPI and has no owner references", func() { + BeforeEach(func() { + By("Creating the MAPI machine") + mapiMachine = mapiMachineBuilder.Build() + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") - By("Creating the CAPI infra machine") - // we must set the capi machine as an owner of the capa machine - // in order to ensure we reconcile capa changes in our sync controller. + By("Setting the MAPI machine status.authoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) - // Updates the capaMachineBuilder with the correct owner ref, - // so when we use it later on, we don't need to repeat ourselves. - capaMachineBuilder = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{ - { - Kind: machineKind, - APIVersion: clusterv1.GroupVersion.String(), - Name: capiMachine.Name, - UID: capiMachine.UID, - BlockOwnerDeletion: ptr.To(true), - Controller: ptr.To(false), - }, - }) + capiMachine = capiMachineBuilder.WithName(mapiMachine.Name).Build() + }) - capaMachine = capaMachineBuilder.Build() - Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created") + It("should successfully create the CAPI machine", func() { + Eventually(k.Get(capiMachine)).Should(Succeed()) + }) - By("Setting the MAPI machine AuthoritativeAPI to Cluster API") - Eventually(k.UpdateStatus(mapiMachine, func() { - mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI - })).Should(Succeed()) + It("should successfully create the CAPA machine with correct owner references", func() { + // Get the CAPI machine, so we know the UID in the api server + Eventually(k.Get(capiMachine)).Should(Succeed()) - }) + // the MAPI Machine builder uses generateName, the controller makes an infra machine + // with the same name, so in order to avoid a 404 we rebuild the capa machine. + capaMachine = capaMachineBuilder.WithName(mapiMachine.Name).Build() - Context("when a MAPI counterpart exists", func() { - Context("when the CAPI Infra Machine gets updated", func() { - BeforeEach(func() { - By("Updating the CAPI Infra Machine (CAPA Machine)") - modifiedCapaMachine := capaMachineBuilder.WithInstanceType("m7i.4xlarge").Build() - modifiedCapaMachine.ResourceVersion = capaMachine.GetResourceVersion() - Eventually(k8sClient.Update(ctx, modifiedCapaMachine)).Should(Succeed(), "capa machine should be able to be updated") - }) + Eventually(k.Object(capaMachine), timeout).Should( + HaveField("ObjectMeta.OwnerReferences", ContainElement( + SatisfyAll( + HaveField("Kind", Equal(machineKind)), + HaveField("APIVersion", Equal(clusterv1.GroupVersion.String())), + HaveField("Name", Equal(capiMachine.Name)), + HaveField("UID", Equal(capiMachine.UID)), + ))), + ) - It("should update the MAPI provider spec", func() { - Eventually(k.Object(mapiMachine), timeout).Should( - WithTransform(awsProviderSpecFromMachine, - HaveField("InstanceType", Equal("m7i.4xlarge")), - )) - }) + }) - It("should update the synchronized condition on the MAPI machine to True", func() { - Eventually(k.Object(mapiMachine), timeout).Should( - HaveField("Status.Conditions", ContainElement( - SatisfyAll( - HaveField("Type", Equal(consts.SynchronizedCondition)), - HaveField("Status", Equal(corev1.ConditionTrue)), - HaveField("Reason", Equal("ResourceSynchronized")), - HaveField("Message", Equal("Successfully synchronized CAPI Machine to MAPI")), - ))), - ) - }) - }) - }) + It("should update the synchronized condition on the MAPI machine to True", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), + ))), + ) }) - Context("when the MAPI machine has status.authoritativeAPI set to MachineAPI and has CPMS owner reference", func() { - BeforeEach(func() { - fakeCPMSOwnerReference := metav1.OwnerReference{ - APIVersion: mapiv1beta1.GroupVersion.String(), - Kind: "ControlPlaneMachineSet", - Name: "cluster", - UID: "cpms-uid", - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - } + }) - By("Creating the MAPI machine") - mapiMachine = mapiMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{fakeCPMSOwnerReference}).Build() - Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + Context("when the MAPI machine has status.authoritativeAPI set to Migrating", func() { + BeforeEach(func() { + By("Creating the CAPI and MAPI machines") + // We want a difference, so if we try to reconcile either way we + // will get a new resourceversion + mapiMachine = mapiMachineBuilder.Build() + capiMachine = capiMachineBuilder.Build() + + Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + + By("Setting the status.authoritativeAPI to Migrating") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMigrating + })).Should(Succeed()) + }) - By("Setting the MAPI machine status.authoritativeAPI to MachineAPI") - Eventually(k.UpdateStatus(mapiMachine, func() { - mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI - })).Should(Succeed()) - }) + It("should not make any changes to either machine", func() { + // We want to make sure that this is the original ResourceVersion + // since we haven't fetched the resource since it was created. + mapiResourceVersion := mapiMachine.GetResourceVersion() + capiResourceVersion := capiMachine.GetResourceVersion() + Consistently(k.Object(mapiMachine), timeout).Should( + HaveField("ResourceVersion", Equal(mapiResourceVersion)), + ) + Consistently(k.Object(capiMachine), timeout).Should( + HaveField("ResourceVersion", Equal(capiResourceVersion)), + ) + }) + }) - It("should not create the CAPI machine", func() { - Consistently(k.Get( - clusterv1resourcebuilder.Machine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(), - )).Should(Not(Succeed())) - }) + Context("when the MAPI machine has status.authoritativeAPI not set", func() { + BeforeEach(func() { + By("Creating the CAPI and MAPI Machines") + mapiMachine = mapiMachineBuilder.Build() + capiMachine = capiMachineBuilder.Build() - It("should update the synchronized condition on the MAPI machine to False", 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("FailedToConvertMAPIMachineToCAPI")), - HaveField("Message", Equal("conversion of control plane machines owned by control plane machine set is currently not supported")), - ))), - ) - }) + Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + By("Setting the status.authoritativeAPI to the empty string") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = "" + })).Should(Succeed()) }) - Context("when the MAPI machine has status.authoritativeAPI set to MachineAPI and has no owner references", func() { - BeforeEach(func() { - By("Creating the MAPI machine") - mapiMachine = mapiMachineBuilder.Build() - Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") - - By("Setting the MAPI machine status.authoritativeAPI to MachineAPI") - Eventually(k.UpdateStatus(mapiMachine, func() { - mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI - })).Should(Succeed()) + It("should not make any changes", func() { + resourceVersion := mapiMachine.GetResourceVersion() + Consistently(k.Object(mapiMachine), timeout).Should( + HaveField("ResourceVersion", Equal(resourceVersion)), + ) + }) + }) - capiMachine = capiMachineBuilder.WithName(mapiMachine.Name).Build() - }) + Context("when the MAPI machine does not exist and the CAPI machine does", func() { + Context("and there is no CAPI machineSet owning the machine", func() { + BeforeEach(func() { - It("should successfully create the CAPI machine", func() { - Eventually(k.Get(capiMachine)).Should(Succeed()) + // The CAPI Machine must reference the CAPA machine, + // so build the CAPA machine (without the owner reference) fist. + // Note we don't create it on the API server yet. + noMachineSetCapaMachineBuilder := capaMachineBuilder.WithName("test-machine-no-machineset") + capaMachine = noMachineSetCapaMachineBuilder.Build() + + capiMachine = capiMachineBuilder.WithName("test-machine-no-machineset").WithInfrastructureRef(corev1.ObjectReference{ + Kind: capaMachine.Kind, + Name: capaMachine.GetName(), + Namespace: capaMachine.GetNamespace(), + }).Build() + Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed()) + + // Once we have created the CAPI Machine, and have a UID, add the owner reference to the CAPA machine + // then we can Create() on the API server + noMachineSetCapaMachineBuilder = noMachineSetCapaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{{ + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }}) + capaMachine = noMachineSetCapaMachineBuilder.Build() + Eventually(k8sClient.Create(ctx, capaMachine)).Should(Succeed()) }) - It("should successfully create the CAPA machine with correct owner references", func() { - // Get the CAPI machine, so we know the UID in the api server - Eventually(k.Get(capiMachine)).Should(Succeed()) - - // the MAPI Machine builder uses generateName, the controller makes an infra machine - // with the same name, so in order to avoid a 404 we rebuild the capa machine. - capaMachine = capaMachineBuilder.WithName(mapiMachine.Name).Build() - - Eventually(k.Object(capaMachine), timeout).Should( - HaveField("ObjectMeta.OwnerReferences", ContainElement( - SatisfyAll( - HaveField("Kind", Equal(machineKind)), - HaveField("APIVersion", Equal(clusterv1.GroupVersion.String())), - HaveField("Name", Equal(capiMachine.Name)), - HaveField("UID", Equal(capiMachine.UID)), - ))), + It("should not make any changes to the CAPI machine", func() { + resourceVersion := capiMachine.GetResourceVersion() + Consistently(k.Object(capiMachine), timeout).Should( + HaveField("ResourceVersion", Equal(resourceVersion)), ) - }) - It("should update the synchronized condition on the MAPI machine to True", func() { - Eventually(k.Object(mapiMachine), timeout).Should( - HaveField("Status.Conditions", ContainElement( - SatisfyAll( - HaveField("Type", Equal(consts.SynchronizedCondition)), - HaveField("Status", Equal(corev1.ConditionTrue)), - HaveField("Reason", Equal("ResourceSynchronized")), - HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), - ))), - ) + It("should not create a MAPI machine", func() { + Consistently(k.ObjectList(&mapiv1beta1.MachineList{}), timeout).ShouldNot(HaveField("Items", + ContainElement(HaveField("ObjectMeta.Name", Equal(capiMachine.GetName()))), + )) }) - }) - - Context("when the MAPI machine has status.authoritativeAPI set to Migrating", func() { - BeforeEach(func() { - By("Creating the CAPI and MAPI machines") - // We want a difference, so if we try to reconcile either way we - // will get a new resourceversion - mapiMachine = mapiMachineBuilder.Build() - capiMachine = capiMachineBuilder.Build() + Context("when MAPI machine with the same name and status.authoritativeAPI set to ClusterAPI is created", func() { + BeforeEach(func() { + mapiMachine = mapiMachineBuilder.WithGenerateName("").WithName(capiMachine.Name).WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).Build() + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") - Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") - Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + By("Setting the status.authoritativeAPI to Cluster API") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI + })).Should(Succeed()) + }) - By("Setting the status.authoritativeAPI to Migrating") - Eventually(k.UpdateStatus(mapiMachine, func() { - mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMigrating - })).Should(Succeed()) - }) + It("should update the synchronized condition on the MAPI machine to True", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized CAPI Machine to MAPI")), + ))), + ) + }) - It("should not make any changes to either machine", func() { - // We want to make sure that this is the original ResourceVersion - // since we haven't fetched the resource since it was created. - mapiResourceVersion := mapiMachine.GetResourceVersion() - capiResourceVersion := capiMachine.GetResourceVersion() - Consistently(k.Object(mapiMachine), timeout).Should( - HaveField("ResourceVersion", Equal(mapiResourceVersion)), - ) - Consistently(k.Object(capiMachine), timeout).Should( - HaveField("ResourceVersion", Equal(capiResourceVersion)), - ) }) }) - Context("when the MAPI machine has status.authoritativeAPI not set", func() { + Context("And there is a CAPI Machineset owning the machine", func() { + var ownerReferencesToCapiMachineSet []metav1.OwnerReference BeforeEach(func() { - By("Creating the CAPI and MAPI Machines") - mapiMachine = mapiMachineBuilder.Build() - capiMachine = capiMachineBuilder.Build() + By("Creating the CAPI machineset") + capiMachineSet = capiMachineSetBuilder.Build() + Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed(), "capi machine set should be able to be created") + + ownerReferencesToCapiMachineSet = []metav1.OwnerReference{{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: machineSetKind, + Name: capiMachineSet.Name, + UID: capiMachineSet.UID, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), + }} + By("Creating the CAPI machine") + capiMachine = capiMachineBuilder.WithOwnerReferences(ownerReferencesToCapiMachineSet).Build() Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") - Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") - By("Setting the status.authoritativeAPI to the empty string") - Eventually(k.UpdateStatus(mapiMachine, func() { - mapiMachine.Status.AuthoritativeAPI = "" - })).Should(Succeed()) - }) + // Because we expect our controller to sync from CAPI -> MAPI, + // we must ensure the CAPA machine exists, with the correct owner ref. + capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{{ + Kind: machineKind, + APIVersion: clusterv1.GroupVersion.String(), + Name: capiMachine.Name, + UID: capiMachine.UID, + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(false), + }}).Build() + Expect(k8sClient.Create(ctx, capaMachine)).Should(Succeed()) - It("should not make any changes", func() { - resourceVersion := mapiMachine.GetResourceVersion() - Consistently(k.Object(mapiMachine), timeout).Should( - HaveField("ResourceVersion", Equal(resourceVersion)), - ) }) - }) - - Context("when the MAPI machine does not exist and the CAPI machine does", func() { - Context("and there is no CAPI machineSet owning the machine", func() { - BeforeEach(func() { - - // The CAPI Machine must reference the CAPA machine, - // so build the CAPA machine (without the owner reference) fist. - // Note we don't create it on the API server yet. - noMachineSetCapaMachineBuilder := capaMachineBuilder.WithName("test-machine-no-machineset") - capaMachine = noMachineSetCapaMachineBuilder.Build() - - capiMachine = capiMachineBuilder.WithName("test-machine-no-machineset").WithInfrastructureRef(corev1.ObjectReference{ - Kind: capaMachine.Kind, - Name: capaMachine.GetName(), - Namespace: capaMachine.GetNamespace(), - }).Build() - Expect(k8sClient.Create(ctx, capiMachine)).Should(Succeed()) - - // Once we have created the CAPI Machine, and have a UID, add the owner reference to the CAPA machine - // then we can Create() on the API server - noMachineSetCapaMachineBuilder = noMachineSetCapaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{{ - Kind: machineKind, - APIVersion: clusterv1.GroupVersion.String(), - Name: capiMachine.Name, - UID: capiMachine.UID, - BlockOwnerDeletion: ptr.To(true), - Controller: ptr.To(false), - }}) - capaMachine = noMachineSetCapaMachineBuilder.Build() - Expect(k8sClient.Create(ctx, capaMachine)).Should(Succeed()) - }) + Context("with no MAPI counterpart", func() { It("should not make any changes to the CAPI machine", func() { resourceVersion := capiMachine.GetResourceVersion() Consistently(k.Object(capiMachine), timeout).Should( @@ -601,121 +674,46 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ContainElement(HaveField("ObjectMeta.Name", Equal(capiMachine.GetName()))), )) }) - - Context("when MAPI machine with the same name and status.authoritativeAPI set to ClusterAPI is created", func() { - BeforeEach(func() { - mapiMachine = mapiMachineBuilder.WithGenerateName("").WithName(capiMachine.Name).WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).Build() - Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") - - By("Setting the status.authoritativeAPI to Cluster API") - Eventually(k.UpdateStatus(mapiMachine, func() { - mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI - })).Should(Succeed()) - }) - - It("should update the synchronized condition on the MAPI machine to True", func() { - Eventually(k.Object(mapiMachine), timeout).Should( - HaveField("Status.Conditions", ContainElement( - SatisfyAll( - HaveField("Type", Equal(consts.SynchronizedCondition)), - HaveField("Status", Equal(corev1.ConditionTrue)), - HaveField("Reason", Equal("ResourceSynchronized")), - HaveField("Message", Equal("Successfully synchronized CAPI Machine to MAPI")), - ))), - ) - }) - - }) }) - Context("And there is a CAPI Machineset owning the machine", func() { - var ownerReferencesToCapiMachineSet []metav1.OwnerReference + Context("with a MAPI counterpart", func() { BeforeEach(func() { - By("Creating the CAPI machineset") - capiMachineSet = capiMachineSetBuilder.Build() - Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed(), "capi machine set should be able to be created") - - ownerReferencesToCapiMachineSet = []metav1.OwnerReference{{ - APIVersion: clusterv1.GroupVersion.String(), - Kind: machineSetKind, - Name: capiMachineSet.Name, - UID: capiMachineSet.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }} - - By("Creating the CAPI machine") - capiMachine = capiMachineBuilder.WithOwnerReferences(ownerReferencesToCapiMachineSet).Build() - Eventually(k8sClient.Create(ctx, capiMachine)).Should(Succeed(), "capi machine should be able to be created") - - // Because we expect our controller to sync from CAPI -> MAPI, - // we must ensure the CAPA machine exists, with the correct owner ref. - capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{{ - Kind: machineKind, - APIVersion: clusterv1.GroupVersion.String(), - Name: capiMachine.Name, - UID: capiMachine.UID, - BlockOwnerDeletion: ptr.To(true), - Controller: ptr.To(false), - }}).Build() - Expect(k8sClient.Create(ctx, capaMachine)).Should(Succeed()) - - }) - - Context("with no MAPI counterpart", func() { - It("should not make any changes to the CAPI machine", func() { - resourceVersion := capiMachine.GetResourceVersion() - Consistently(k.Object(capiMachine), timeout).Should( - HaveField("ResourceVersion", Equal(resourceVersion)), - ) - }) + mapiMachineSet := mapiMachineSetBuilder.Build() - It("should not create a MAPI machine", func() { - Consistently(k.ObjectList(&mapiv1beta1.MachineList{}), timeout).ShouldNot(HaveField("Items", - ContainElement(HaveField("ObjectMeta.Name", Equal(capiMachine.GetName()))), - )) - }) + Eventually(k8sClient.Create(ctx, mapiMachineSet)).Should(Succeed(), "mapi machine set should be able to be created") }) - Context("with a MAPI counterpart", func() { - BeforeEach(func() { - mapiMachineSet := mapiMachineSetBuilder.Build() - - Eventually(k8sClient.Create(ctx, mapiMachineSet)).Should(Succeed(), "mapi machine set should be able to be created") - }) + // We now set finalizers regardless, so this does not work any more. - // We now set finalizers regardless, so this does not work any more. + // It("should not make any changes to the CAPI machine", func() { + // resourceVersion := capiMachine.GetResourceVersion() + // Consistently(k.Object(capiMachine), timeout).Should( + // HaveField("ResourceVersion", Equal(resourceVersion)), + // ) + // }) - // It("should not make any changes to the CAPI machine", func() { - // resourceVersion := capiMachine.GetResourceVersion() - // Consistently(k.Object(capiMachine), timeout).Should( - // HaveField("ResourceVersion", Equal(resourceVersion)), - // ) - // }) - - It("should create a MAPI machine", func() { - Eventually(k.ObjectList(&mapiv1beta1.MachineList{}), timeout).Should(HaveField("Items", - ContainElement(HaveField("ObjectMeta.Name", Equal(capiMachine.GetName()))), - )) - - mapiMachine = machinev1resourcebuilder.Machine().WithName(capiMachine.Name).WithNamespace(mapiNamespace.Name).Build() - Eventually(k.Object(mapiMachine), timeout).Should(HaveField("ObjectMeta.OwnerReferences", ContainElement( - SatisfyAll( - HaveField("APIVersion", Equal(mapiv1beta1.GroupVersion.String())), - HaveField("Kind", Equal(machineSetKind)), - HaveField("Name", Equal(capiMachineSet.Name)), - HaveField("Controller", Equal(ptr.To(true))), - HaveField("BlockOwnerDeletion", Equal(ptr.To(true))), - ), - ))) + It("should create a MAPI machine", func() { + Eventually(k.ObjectList(&mapiv1beta1.MachineList{}), timeout).Should(HaveField("Items", + ContainElement(HaveField("ObjectMeta.Name", Equal(capiMachine.GetName()))), + )) - }) + mapiMachine = machinev1resourcebuilder.Machine().WithName(capiMachine.Name).WithNamespace(mapiNamespace.Name).Build() + Eventually(k.Object(mapiMachine), timeout).Should(HaveField("ObjectMeta.OwnerReferences", ContainElement( + SatisfyAll( + HaveField("APIVersion", Equal(mapiv1beta1.GroupVersion.String())), + HaveField("Kind", Equal(machineSetKind)), + HaveField("Name", Equal(capiMachineSet.Name)), + HaveField("Controller", Equal(ptr.To(true))), + HaveField("BlockOwnerDeletion", Equal(ptr.To(true))), + ), + ))) }) }) }) + }) Context("when the CAPI infra machine resource does not exist", func() {