Don't re-reconcile when adopting resources

Unlike referenced resources, which specify a future intent, adoption
only affects resources which have already been created. Successive
executions will produce the same output, so there is no need to
re-reconcile if we found orphaned resources.

Also we seem to be hitting this a lot in practise, most likely due to
the controller-runtime read-after-write cache inconsistency issue.

Author: mdbooth

controllers/openstackcluster_controller.go

@@ -231,17 +231,13 @@ func resolveBastionResources(scope *scope.WithLogger, openStackCluster *infrav1.
 			return true, nil
 		}
 
-		changed, err = compute.AdoptDependentMachineResources(scope,
+		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

@@ -161,14 +161,10 @@ func (r *OpenStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	}
 
 	// Adopt any existing dependent resources
-	changed, err = compute.AdoptDependentMachineResources(scope, openStackMachine.Name, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources)
+	err = compute.AdoptDependentMachineResources(scope, openStackMachine.Name, &openStackMachine.Status.ReferencedResources, &openStackMachine.Status.DependentResources)
 	if err != nil {
 		return reconcile.Result{}, 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() {

pkg/cloud/services/compute/dependent_resources.go

@@ -22,10 +22,10 @@ import (
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 )
 
-func AdoptDependentMachineResources(scope *scope.WithLogger, baseName string, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) (bool, error) {
+func AdoptDependentMachineResources(scope *scope.WithLogger, baseName string, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) error {
 	networkingService, err := networking.NewService(scope)
 	if err != nil {
-		return false, err
+		return err
 	}
 
 	return networkingService.AdoptPorts(scope, baseName, referencedResources.Ports, dependentResources)

pkg/cloud/services/networking/port.go

@@ -571,12 +571,10 @@ func (s *Service) IsTrunkExtSupported() (trunknSupported bool, err error) {
 
 // AdoptPorts looks for ports in desiredPorts which were previously created, and adds them to dependentResources.Ports.
 // A port matches if it has the same name and network ID as the desired port.
-func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPorts []infrav1.PortOpts, dependentResources *infrav1.DependentMachineResources) (changed bool, err error) {
-	changed = false
-
+func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPorts []infrav1.PortOpts, dependentResources *infrav1.DependentMachineResources) error {
 	// We can skip adoption if the ports are already in the status
 	if len(desiredPorts) == len(dependentResources.Ports) {
-		return changed, nil
+		return nil
 	}
 
 	scope.Logger().V(5).Info("Adopting ports")
@@ -598,24 +596,23 @@ func (s *Service) AdoptPorts(scope *scope.WithLogger, baseName string, desiredPo
 			NetworkID: port.Network.ID,
 		})
 		if err != nil {
-			return changed, fmt.Errorf("searching for existing port %s in network %s: %v", portName, port.Network.ID, err)
+			return fmt.Errorf("searching for existing port %s in network %s: %v", portName, port.Network.ID, err)
 		}
 		// if the port is not found, we stop the adoption of ports since the rest of the ports will not be found either
 		// and will be created after the adoption
 		if len(ports) == 0 {
 			scope.Logger().V(5).Info("Port not found, stopping the adoption of ports", "port index", i)
-			return changed, nil
+			return nil
 		}
 		if len(ports) > 1 {
-			return changed, fmt.Errorf("found multiple ports with name %s", portName)
+			return fmt.Errorf("found multiple ports with name %s", portName)
 		}
 
 		// The desired port was found, so we add it to the status
 		portID := ports[0].ID
 		scope.Logger().Info("Adopted previously created port which was not in status", "port index", i, "portID", portID)
 		dependentResources.Ports = append(dependentResources.Ports, infrav1.PortStatus{ID: portID})
-		changed = true
 	}
 
-	return changed, nil
+	return nil
 }

pkg/cloud/services/networking/port_test.go

@@ -971,7 +971,7 @@ func Test_AdoptPorts(t *testing.T) {
 				client: mockClient,
 			}
 
-			_, err := s.AdoptPorts(scope.NewWithLogger(mockScopeFactory, log),
+			err := s.AdoptPorts(scope.NewWithLogger(mockScopeFactory, log),
 				"test-machine",
 				tt.desiredPorts, &tt.dependentResources)
 			if tt.wantErr {