Don't set OSMachine Ready until all config is complete

With this change, an OpenStackMachine will not be marked Ready until it
has been completely initialised, including setting its Addresses and
adding it to the APIServer loadbalancer if required.

Also fix a missing early return when the instance has been unexpectedly
deleted.

Author: mdbooth

controllers/openstackmachine_controller.go

@@ -365,11 +365,6 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, err
 	}
 
-	result := r.reconcileMachineState(scope, openStackMachine, machine, machineServer)
-	if result != nil {
-		return *result, nil
-	}
-
 	computeService, err := compute.NewService(scope)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -383,9 +378,9 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 	}
 
 	if instanceStatus == nil {
-		msg := "server has been unexpectedly deleted"
-		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeletedReason, clusterv1.ConditionSeverityError, msg)
-		openStackMachine.SetFailure(capoerrors.DeprecatedCAPIUpdateMachineError, errors.New(msg))
+		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeletedReason, clusterv1.ConditionSeverityError, infrav1.ServerUnexpectedDeletedMessage)
+		openStackMachine.SetFailure(capoerrors.DeprecatedCAPIUpdateMachineError, errors.New(infrav1.ServerUnexpectedDeletedMessage)) //nolint:stylecheck // This error is not used as an error
+		return ctrl.Result{}, nil
 	}
 
 	instanceNS, err := instanceStatus.NetworkStatus()
@@ -413,6 +408,11 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		conditions.MarkTrue(openStackMachine, infrav1.APIServerIngressReadyCondition)
 	}
 
+	result := r.reconcileMachineState(scope, openStackMachine, machine, machineServer)
+	if result != nil {
+		return *result, nil
+	}
+
 	scope.Logger().Info("Reconciled Machine create successfully")
 	return ctrl.Result{}, nil
 }

test/e2e/suites/e2e/e2e_test.go

@@ -56,6 +56,8 @@ import (
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-openstack/internal/util/ssa"
+	"sigs.k8s.io/cluster-api-provider-openstack/pkg/generated/applyconfiguration/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-openstack/test/e2e/shared"
 )
 
@@ -1014,20 +1016,48 @@ var _ = Describe("e2e tests [PR-Blocking]", func() {
 			serverToDelete := allServers[controlPlaneMachines[0].Spec.InfrastructureRef.Name]
 			err = shared.DeleteOpenStackServer(ctx, e2eCtx, serverToDelete.ID)
 			Expect(err).NotTo(HaveOccurred())
+
 			shared.Logf("Waiting for the OpenStackMachine to have a condition that the server has been unexpectedly deleted")
-			Eventually(func() bool {
+			retries := 0
+			Eventually(func() (clusterv1.Condition, error) {
+				k8sClient := e2eCtx.Environment.BootstrapClusterProxy.GetClient()
+
 				openStackMachine := &infrav1.OpenStackMachine{}
-				err := e2eCtx.Environment.BootstrapClusterProxy.GetClient().Get(ctx, crclient.ObjectKey{Name: controlPlaneMachines[0].Name, Namespace: controlPlaneMachines[0].Namespace}, openStackMachine)
+				err := k8sClient.Get(ctx, crclient.ObjectKey{Name: controlPlaneMachines[0].Name, Namespace: controlPlaneMachines[0].Namespace}, openStackMachine)
 				if err != nil {
-					return false
+					return clusterv1.Condition{}, err
 				}
 				for _, condition := range openStackMachine.Status.Conditions {
-					if condition.Type == infrav1.InstanceReadyCondition && condition.Status == corev1.ConditionFalse && condition.Reason == infrav1.InstanceDeletedReason && condition.Message == "server has been unexpectedly deleted" {
-						return true
+					if condition.Type == infrav1.InstanceReadyCondition {
+						return condition, nil
 					}
 				}
-				return false
-			}, e2eCtx.E2EConfig.GetIntervals(specName, "wait-delete-machine")...).Should(BeTrue())
+
+				// Make some non-functional change to the object which will
+				// cause CAPO to reconcile it, otherwise we won't notice the
+				// server is gone until the configured controller-runtime
+				// resync.
+				retries++
+				applyConfig := v1beta1.OpenStackMachine(openStackMachine.Name, openStackMachine.Namespace).
+					WithAnnotations(map[string]string{
+						"e2e-test-retries": fmt.Sprintf("%d", retries),
+					})
+				err = k8sClient.Patch(ctx, openStackMachine, ssa.ApplyConfigPatch(applyConfig), crclient.ForceOwnership, crclient.FieldOwner("capo-e2e"))
+				if err != nil {
+					return clusterv1.Condition{}, err
+				}
+
+				return clusterv1.Condition{}, errors.New("condition InstanceReadyCondition not found")
+			}, time.Minute*3, time.Second*10).Should(MatchFields(
+				IgnoreExtras,
+				Fields{
+					"Type":     Equal(infrav1.InstanceReadyCondition),
+					"Status":   Equal(corev1.ConditionFalse),
+					"Reason":   Equal(infrav1.InstanceDeletedReason),
+					"Message":  Equal(infrav1.ServerUnexpectedDeletedMessage),
+					"Severity": Equal(clusterv1.ConditionSeverityError),
+				},
+			), "OpenStackMachine should be marked not ready with InstanceDeletedReason")
 		})
 	})
 })