Fix premature attempt to resolve machine resources

We can't resolve machine resources until the cluster is initialised.

Author: mdbooth

controllers/openstackmachine_controller.go

@@ -150,29 +150,32 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	}
 	scope := scope.NewWithLogger(clientScope, log)
 
+	clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name)
+
+	// Handle deleted machines
+	if !openStackMachine.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(scope, clusterName, infraCluster, machine, openStackMachine)
+	}
+
+	// Handle non-deleted clusters
+	return r.reconcileNormal(ctx, scope, clusterName, infraCluster, machine, openStackMachine)
+}
+
+func resolveMachineResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, openStackMachine *infrav1.OpenStackMachine) (bool, error) {
 	// Resolve and store referenced resources
-	changed, err := compute.ResolveReferencedMachineResources(scope, infraCluster, &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources)
+	changed, err := compute.ResolveReferencedMachineResources(scope,
+		openStackCluster,
+		&openStackMachine.Spec, &openStackMachine.Status.ReferencedResources)
 	if err != nil {
-		return reconcile.Result{}, err
+		return false, err
 	}
 	if changed {
 		// If the referenced resources have changed, we need to update the OpenStackMachine status now.
-		return reconcile.Result{}, nil
+		return true, nil
 	}
 
 	// Adopt any existing dependent resources
-	err = compute.AdoptDependentMachineResources(scope, openStackMachine.Name, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
-
-	// Handle deleted machines
-	if !openStackMachine.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(scope, cluster, infraCluster, machine, openStackMachine)
-	}
-
-	// Handle non-deleted clusters
-	return r.reconcileNormal(ctx, scope, cluster, infraCluster, machine, openStackMachine)
+	return false, compute.AdoptDependentMachineResources(scope, openStackMachine.Name, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources)
 }
 
 func patchMachine(ctx context.Context, patchHelper *patch.Helper, openStackMachine *infrav1.OpenStackMachine, machine *clusterv1.Machine, options ...patch.Option) error {
@@ -227,11 +230,9 @@ func (r *OpenStackMachineReconciler) SetupWithManager(ctx context.Context, mgr c
 		Complete(r)
 }
 
-func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine) (ctrl.Result, error) { //nolint:unparam
+func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, clusterName string, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine) (ctrl.Result, error) { //nolint:unparam
 	scope.Logger().Info("Reconciling Machine delete")
 
-	clusterName := fmt.Sprintf("%s-%s", cluster.ObjectMeta.Namespace, cluster.Name)
-
 	computeService, err := compute.NewService(scope)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -242,6 +243,13 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl
 		return ctrl.Result{}, err
 	}
 
+	// We may have resources to adopt if the cluster is ready
+	if openStackCluster.Status.Ready && openStackCluster.Status.Network != nil {
+		if _, err := resolveMachineResources(scope, openStackCluster, openStackMachine); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
 	if openStackCluster.Spec.APIServerLoadBalancer.IsEnabled() {
 		loadBalancerService, err := loadbalancer.NewService(scope)
 		if err != nil {
@@ -454,7 +462,7 @@ func (r *OpenStackMachineReconciler) reconcileDeleteFloatingAddressFromPool(scop
 	return r.Client.Update(context.Background(), claim)
 }
 
-func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine) (_ ctrl.Result, reterr error) {
+func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope *scope.WithLogger, clusterName string, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine) (_ ctrl.Result, reterr error) {
 	var err error
 
 	// If the OpenStackMachine is in an error state, return early.
@@ -469,12 +477,16 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, nil
 	}
 
-	if !cluster.Status.InfrastructureReady {
+	if !openStackCluster.Status.Ready {
 		scope.Logger().Info("Cluster infrastructure is not ready yet, re-queuing machine")
 		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.WaitingForClusterInfrastructureReason, clusterv1.ConditionSeverityInfo, "")
 		return ctrl.Result{RequeueAfter: waitForClusterInfrastructureReadyDuration}, nil
 	}
 
+	if changed, err := resolveMachineResources(scope, openStackCluster, openStackMachine); changed || err != nil {
+		return ctrl.Result{}, err
+	}
+
 	// Make sure bootstrap data is available and populated.
 	if machine.Spec.Bootstrap.DataSecretName == nil {
 		scope.Logger().Info("Bootstrap data secret reference is not yet available")
@@ -487,8 +499,6 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 	}
 	scope.Logger().Info("Reconciling Machine")
 
-	clusterName := fmt.Sprintf("%s-%s", cluster.ObjectMeta.Namespace, cluster.Name)
-
 	computeService, err := compute.NewService(scope)
 	if err != nil {
 		return ctrl.Result{}, err

pkg/cloud/services/compute/referenced_resources.go

@@ -17,6 +17,8 @@ limitations under the License.
 package compute
 
 import (
+	"fmt"
+
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
@@ -61,8 +63,15 @@ func ResolveReferencedMachineResources(scope *scope.WithLogger, openStackCluster
 		changed = true
 	}
 
+	// ConstructPorts requires the cluster network to have been set. We only
+	// call this from places where we know it should have been set, but the
+	// cluster status is externally-provided data so we check it anyway.
+	if openStackCluster.Status.Network == nil {
+		return changed, fmt.Errorf("called ResolveReferencedMachineResources with nil OpenStackCluster.Status.Network")
+	}
+
 	// Network resources are required in order to get ports options.
-	if len(resources.Ports) == 0 && openStackCluster.Status.Network != nil {
+	if len(resources.Ports) == 0 {
 		// For now we put this here but realistically an OpenStack administrator could enable/disable trunk
 		// support at any time, so we should probably check this on every reconcile.
 		trunkSupported, err := networkingService.IsTrunkExtSupported()