Merge pull request kubernetes-sigs#1944 from shiftstack/issue1942

🌱 Deduplicate AdoptMachinePorts and AdoptBastionPorts

Author: k8s-ci-robot

controllers/openstackcluster_controller.go

@@ -231,14 +231,13 @@ func resolveBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.
 			return true, nil
 		}
 
-		changed, err = compute.ResolveDependentBastionResources(scope, openStackCluster, bastionName(openStackCluster.Name))
+		err = compute.AdoptDependentMachineResources(scope,
+			bastionName(openStackCluster.Name),
+			&openStackCluster.Status.Bastion.ReferencedResources,
+			&openStackCluster.Status.Bastion.DependentResources)
 		if err != nil {
 			return false, err
 		}
-		if changed {
-			// If the dependent resources have changed, we need to update the OpenStackCluster status now.
-			return true, nil
-		}
 	}
 	return false, nil
 }

controllers/openstackmachine_controller.go

@@ -150,33 +150,32 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	}
 	scope := scope.NewWithLogger(clientScope, log)
 
-	// Resolve and store referenced resources
-	changed, err := compute.ResolveReferencedMachineResources(scope, infraCluster, &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
-	if changed {
-		// If the referenced resources have changed, we need to update the OpenStackMachine status now.
-		return reconcile.Result{}, nil
+	clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name)
+
+	// Handle deleted machines
+	if !openStackMachine.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(scope, clusterName, infraCluster, machine, openStackMachine)
 	}
 
-	// Resolve and store dependent resources
-	changed, err = compute.ResolveDependentMachineResources(scope, 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,
+		openStackCluster,
+		&openStackMachine.Spec, &openStackMachine.Status.ReferencedResources)
 	if err != nil {
-		return reconcile.Result{}, err
+		return false, err
 	}
 	if changed {
-		// If the dependent resources have changed, we need to update the OpenStackMachine status now.
-		return reconcile.Result{}, nil
-	}
-
-	// Handle deleted machines
-	if !openStackMachine.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(scope, cluster, infraCluster, machine, openStackMachine)
+		// If the referenced resources have changed, we need to update the OpenStackMachine status now.
+		return true, nil
 	}
 
-	// Handle non-deleted clusters
-	return r.reconcileNormal(ctx, scope, cluster, infraCluster, machine, openStackMachine)
+	// Adopt any existing dependent resources
+	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 {
@@ -231,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
@@ -246,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 {
@@ -458,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.
@@ -473,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")
@@ -491,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/dependent_resources.go

@@ -22,24 +22,11 @@ import (
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 )
 
-func ResolveDependentMachineResources(scope *scope.WithLogger, openStackMachine *infrav1.OpenStackMachine) (changed bool, err error) {
-	changed = false
-
-	networkingService, err := networking.NewService(scope)
-	if err != nil {
-		return changed, err
-	}
-
-	return networkingService.AdoptMachinePorts(scope, openStackMachine, openStackMachine.Status.ReferencedResources.Ports)
-}
-
-func ResolveDependentBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, bastionName string) (changed bool, err error) {
-	changed = false
-
+func AdoptDependentMachineResources(scope *scope.WithLogger, baseName string, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) error {
 	networkingService, err := networking.NewService(scope)
 	if err != nil {
-		return changed, err
+		return err
 	}
 
-	return networkingService.AdoptBastionPorts(scope, openStackCluster, bastionName)
+	return networkingService.AdoptPorts(scope, baseName, referencedResources.Ports, dependentResources)
 }