Fix port name after port creation failure

CreatePorts was creating a port name based on the index of the port in
the *current reconcile*. This could be different to the absolute index
of the port if ports had been partially created in a previous reconcile.

We fix this by passing all current state into CreatePorts so it can
create an absolute index. This also ensures that partially created ports
will be persisted on failure so we don't have to rely on adoption in the
next reconcile.

Author: mdbooth

controllers/openstackcluster_controller.go

@@ -404,7 +404,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS
 		}
 	}
 
-	err = getOrCreateBastionPorts(scope, cluster, openStackCluster, networkingService, cluster.Name)
+	err = getOrCreateBastionPorts(openStackCluster, networkingService, cluster.Name)
 	if err != nil {
 		handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create ports for bastion: %w", err))
 		return ctrl.Result{}, fmt.Errorf("failed to get or create ports for bastion: %w", err)
@@ -548,34 +548,19 @@ func getBastionSecurityGroups(openStackCluster *infrav1.OpenStackCluster) []infr
 	return instanceSpecSecurityGroups
 }
 
-func getOrCreateBastionPorts(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error {
-	scope.Logger().Info("Reconciling ports for bastion", "bastion", bastionName(openStackCluster.Name))
-
-	if openStackCluster.Status.Bastion == nil {
-		openStackCluster.Status.Bastion = &infrav1.BastionStatus{}
-	}
-
+func getOrCreateBastionPorts(openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error {
 	desiredPorts := openStackCluster.Status.Bastion.ReferencedResources.Ports
-	portsToCreate := networking.MissingPorts(openStackCluster.Status.Bastion.DependentResources.Ports, desiredPorts)
-
-	// Sanity check that the number of desired ports is equal to the addition of ports to create and ports that already exist.
-	if len(desiredPorts) != len(portsToCreate)+len(openStackCluster.Status.Bastion.DependentResources.Ports) {
-		return fmt.Errorf("length of desired ports (%d) is not equal to the length of ports to create (%d) + the length of ports that already exist (%d)", len(desiredPorts), len(portsToCreate), len(openStackCluster.Status.Bastion.DependentResources.Ports))
-	}
+	dependentResources := &openStackCluster.Status.Bastion.DependentResources
 
-	if len(portsToCreate) > 0 {
-		securityGroups := getBastionSecurityGroups(openStackCluster)
-		bastionPortsStatus, err := networkingService.CreatePorts(openStackCluster, clusterName, portsToCreate, securityGroups, []string{}, bastionName(cluster.Name))
-		if err != nil {
-			return fmt.Errorf("failed to create ports for bastion %s: %w", bastionName(openStackCluster.Name), err)
-		}
-
-		openStackCluster.Status.Bastion.DependentResources.Ports = append(openStackCluster.Status.Bastion.DependentResources.Ports, bastionPortsStatus...)
+	if len(desiredPorts) == len(dependentResources.Ports) {
+		return nil
 	}
 
-	// Sanity check that the number of ports that have been put into PortsStatus is equal to the number of desired ports now that we have created them all.
-	if len(openStackCluster.Status.Bastion.DependentResources.Ports) != len(desiredPorts) {
-		return fmt.Errorf("length of ports that already exist (%d) is not equal to the length of desired ports (%d)", len(openStackCluster.Status.Bastion.DependentResources.Ports), len(desiredPorts))
+	securityGroups := getBastionSecurityGroups(openStackCluster)
+	bastionTags := []string{}
+	err := networkingService.CreatePorts(openStackCluster, clusterName, bastionName(clusterName), securityGroups, bastionTags, desiredPorts, dependentResources)
+	if err != nil {
+		return fmt.Errorf("failed to create ports for bastion %s: %w", bastionName(openStackCluster.Name), err)
 	}
 
 	return nil

controllers/openstackmachine_controller.go

@@ -527,7 +527,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, err
 	}
 
-	err = getOrCreateMachinePorts(scope, openStackCluster, machine, openStackMachine, networkingService, clusterName)
+	err = getOrCreateMachinePorts(openStackCluster, machine, openStackMachine, networkingService, clusterName)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -669,32 +669,18 @@ func (r *OpenStackMachineReconciler) reconcileAPIServerLoadBalancer(scope *scope
 	return nil
 }
 
-func getOrCreateMachinePorts(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error {
-	scope.Logger().Info("Reconciling ports for machine", "machine", machine.Name)
-	var machinePortsStatus []infrav1.PortStatus
-	var err error
-
+func getOrCreateMachinePorts(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error {
 	desiredPorts := openStackMachine.Status.ReferencedResources.Ports
-	portsToCreate := networking.MissingPorts(openStackMachine.Status.DependentResources.Ports, desiredPorts)
-
-	// Sanity check that the number of desired ports is equal to the addition of ports to create and ports that already exist.
-	if len(desiredPorts) != len(portsToCreate)+len(openStackMachine.Status.DependentResources.Ports) {
-		return fmt.Errorf("length of desired ports (%d) is not equal to the length of ports to create (%d) + the length of ports that already exist (%d)", len(desiredPorts), len(portsToCreate), len(openStackMachine.Status.DependentResources.Ports))
-	}
+	dependentResources := &openStackMachine.Status.DependentResources
 
-	if len(portsToCreate) > 0 {
-		instanceTags := getInstanceTags(openStackMachine, openStackCluster)
-		managedSecurityGroups := getManagedSecurityGroups(openStackCluster, machine, openStackMachine)
-		machinePortsStatus, err = networkingService.CreatePorts(openStackMachine, clusterName, portsToCreate, managedSecurityGroups, instanceTags, openStackMachine.Name)
-		if err != nil {
-			return fmt.Errorf("create ports: %w", err)
-		}
-		openStackMachine.Status.DependentResources.Ports = append(openStackMachine.Status.DependentResources.Ports, machinePortsStatus...)
+	if len(desiredPorts) == len(dependentResources.Ports) {
+		return nil
 	}
 
-	// Sanity check that the number of ports that have been put into PortsStatus is equal to the number of desired ports now that we have created them all.
-	if len(openStackMachine.Status.DependentResources.Ports) != len(desiredPorts) {
-		return fmt.Errorf("length of ports that already exist (%d) is not equal to the length of desired ports (%d)", len(openStackMachine.Status.DependentResources.Ports), len(desiredPorts))
+	instanceTags := getInstanceTags(openStackMachine, openStackCluster)
+	managedSecurityGroups := getManagedSecurityGroups(openStackCluster, machine, openStackMachine)
+	if err := networkingService.CreatePorts(openStackMachine, clusterName, openStackMachine.Name, managedSecurityGroups, instanceTags, desiredPorts, dependentResources); err != nil {
+		return fmt.Errorf("creating ports: %w", err)
 	}
 
 	return nil

pkg/cloud/services/networking/port.go

@@ -109,7 +109,7 @@ func (s *Service) GetPortForExternalNetwork(instanceID string, externalNetworkID
 	return nil, nil
 }
 
-func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, portName string, portOpts *infrav1.PortOpts, instanceSecurityGroups []string, instanceTags []string) (*ports.Port, error) {
+func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, portName string, portOpts *infrav1.PortOpts, defaultSecurityGroups []string, baseTags []string) (*ports.Port, error) {
 	var err error
 	networkID := portOpts.Network.ID
 
@@ -137,7 +137,7 @@ func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, por
 		}
 		// inherit port security groups from the instance if not explicitly specified
 		if len(securityGroups) == 0 {
-			securityGroups = instanceSecurityGroups
+			securityGroups = defaultSecurityGroups
 		}
 	}
 
@@ -209,7 +209,7 @@ func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, por
 	}
 
 	var tags []string
-	tags = append(tags, instanceTags...)
+	tags = append(tags, baseTags...)
 	tags = append(tags, portOpts.Tags...)
 	if len(tags) > 0 {
 		if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, tags); err != nil {
@@ -365,37 +365,32 @@ func GetPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) stri
 	return fmt.Sprintf("%s-%d", instanceName, netIndex)
 }
 
-func (s *Service) CreatePorts(eventObject runtime.Object, clusterName string, ports []infrav1.PortOpts, securityGroups []infrav1.SecurityGroupFilter, instanceTags []string, instanceName string) ([]infrav1.PortStatus, error) {
-	return s.createPortsImpl(eventObject, clusterName, ports, securityGroups, instanceTags, instanceName)
-}
-
-func (s *Service) createPortsImpl(eventObject runtime.Object, clusterName string, ports []infrav1.PortOpts, securityGroups []infrav1.SecurityGroupFilter, instanceTags []string, instanceName string) ([]infrav1.PortStatus, error) {
-	instanceSecurityGroups, err := s.GetSecurityGroups(securityGroups)
+func (s *Service) CreatePorts(eventObject runtime.Object, clusterName, baseName string, securityGroups []infrav1.SecurityGroupFilter, baseTags []string, desiredPorts []infrav1.PortOpts, dependentResources *infrav1.DependentMachineResources) error {
+	defaultSecurityGroups, err := s.GetSecurityGroups(securityGroups)
 	if err != nil {
-		return nil, fmt.Errorf("error getting security groups: %v", err)
+		return fmt.Errorf("error getting security groups: %v", err)
 	}
 
-	portsStatus := make([]infrav1.PortStatus, 0, len(ports))
-
-	for i := range ports {
-		portOpts := &ports[i]
-		iTags := []string{}
-		if len(instanceTags) > 0 {
-			iTags = instanceTags
+	for i := range desiredPorts {
+		// Skip creation of ports which already exist
+		if i < len(dependentResources.Ports) {
+			continue
 		}
-		portName := GetPortName(instanceName, portOpts, i)
+
+		portOpts := &desiredPorts[i]
+		portName := GetPortName(baseName, portOpts, i)
 		// Events are recorded in CreatePort
-		port, err := s.CreatePort(eventObject, clusterName, portName, portOpts, instanceSecurityGroups, iTags)
+		port, err := s.CreatePort(eventObject, clusterName, portName, portOpts, defaultSecurityGroups, baseTags)
 		if err != nil {
-			return nil, err
+			return err
 		}
 
-		portsStatus = append(portsStatus, infrav1.PortStatus{
+		dependentResources.Ports = append(dependentResources.Ports, infrav1.PortStatus{
 			ID: port.ID,
 		})
 	}
 
-	return portsStatus, nil
+	return nil
 }
 
 // ConstructPorts builds an array of ports from the instance spec.
@@ -691,16 +686,3 @@ func (s *Service) AdoptBastionPorts(scope *scope.WithLogger, openStackCluster *i
 
 	return changed, nil
 }
-
-// MissingPorts returns the ports that are not in the ports status but are desired ports which should be created.
-func MissingPorts(portsStatus []infrav1.PortStatus, desiredPorts []infrav1.PortOpts) []infrav1.PortOpts {
-	// missingPorts is equal to the ports status minus its length
-	missingPortsLength := len(desiredPorts) - len(portsStatus)
-
-	// rebuild desiredPorts to only contain the ports that were not adopted
-	missingPorts := make([]infrav1.PortOpts, missingPortsLength)
-	for i := 0; i < missingPortsLength; i++ {
-		missingPorts[i] = desiredPorts[i+len(portsStatus)]
-	}
-	return missingPorts
-}

pkg/cloud/services/networking/port_test.go

@@ -837,71 +837,3 @@ func Test_getPortName(t *testing.T) {
 		})
 	}
 }
-
-func Test_MissingPorts(t *testing.T) {
-	tests := []struct {
-		name            string
-		portsStatus     []infrav1.PortStatus
-		desiredPorts    []infrav1.PortOpts
-		expectedMissing []infrav1.PortOpts
-	}{
-		{
-			name: "no missing ports",
-			portsStatus: []infrav1.PortStatus{
-				{
-					ID: "06d18afd-a8d2-4c0f-b6be-63fe71d6c16d",
-				},
-				{
-					ID: "7bf62a7e-a969-40cc-b50c-87bd52e97188",
-				},
-			},
-			desiredPorts: []infrav1.PortOpts{
-				{
-					Network: &infrav1.NetworkFilter{
-						ID: "94588d9b-21f1-4583-97ed-c7367327b0ea",
-					},
-				},
-				{
-					Network: &infrav1.NetworkFilter{
-						ID: "9cc0ebba-eaec-4dc7-b5cf-ece51f699f47",
-					},
-				},
-			},
-			expectedMissing: []infrav1.PortOpts{},
-		},
-		{
-			name: "missing ports",
-			portsStatus: []infrav1.PortStatus{
-				{
-					ID: "06d18afd-a8d2-4c0f-b6be-63fe71d6c16d",
-				},
-			},
-			desiredPorts: []infrav1.PortOpts{
-				{
-					Network: &infrav1.NetworkFilter{
-						ID: "94588d9b-21f1-4583-97ed-c7367327b0ea",
-					},
-				},
-				{
-					Network: &infrav1.NetworkFilter{
-						ID: "9cc0ebba-eaec-4dc7-b5cf-ece51f699f47",
-					},
-				},
-			},
-			expectedMissing: []infrav1.PortOpts{
-				{
-					Network: &infrav1.NetworkFilter{
-						ID: "9cc0ebba-eaec-4dc7-b5cf-ece51f699f47",
-					},
-				},
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			g := NewWithT(t)
-			got := MissingPorts(tt.portsStatus, tt.desiredPorts)
-			g.Expect(got).To(Equal(tt.expectedMissing))
-		})
-	}
-}