Skip to content

Commit a3de5bc

Browse files
committed
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.
1 parent e7d0e19 commit a3de5bc

File tree

4 files changed

+36
-151
lines changed

4 files changed

+36
-151
lines changed

controllers/openstackcluster_controller.go

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ func reconcileBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openS
404404
}
405405
}
406406

407-
err = getOrCreateBastionPorts(scope, cluster, openStackCluster, networkingService, cluster.Name)
407+
err = getOrCreateBastionPorts(openStackCluster, networkingService, cluster.Name)
408408
if err != nil {
409409
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to get or create ports for bastion: %w", err))
410410
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
548548
return instanceSpecSecurityGroups
549549
}
550550

551-
func getOrCreateBastionPorts(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error {
552-
scope.Logger().Info("Reconciling ports for bastion", "bastion", bastionName(openStackCluster.Name))
553-
554-
if openStackCluster.Status.Bastion == nil {
555-
openStackCluster.Status.Bastion = &infrav1.BastionStatus{}
556-
}
557-
551+
func getOrCreateBastionPorts(openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service, clusterName string) error {
558552
desiredPorts := openStackCluster.Status.Bastion.ReferencedResources.Ports
559-
portsToCreate := networking.MissingPorts(openStackCluster.Status.Bastion.DependentResources.Ports, desiredPorts)
560-
561-
// Sanity check that the number of desired ports is equal to the addition of ports to create and ports that already exist.
562-
if len(desiredPorts) != len(portsToCreate)+len(openStackCluster.Status.Bastion.DependentResources.Ports) {
563-
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))
564-
}
553+
dependentResources := &openStackCluster.Status.Bastion.DependentResources
565554

566-
if len(portsToCreate) > 0 {
567-
securityGroups := getBastionSecurityGroups(openStackCluster)
568-
bastionPortsStatus, err := networkingService.CreatePorts(openStackCluster, clusterName, portsToCreate, securityGroups, []string{}, bastionName(cluster.Name))
569-
if err != nil {
570-
return fmt.Errorf("failed to create ports for bastion %s: %w", bastionName(openStackCluster.Name), err)
571-
}
572-
573-
openStackCluster.Status.Bastion.DependentResources.Ports = append(openStackCluster.Status.Bastion.DependentResources.Ports, bastionPortsStatus...)
555+
if len(desiredPorts) == len(dependentResources.Ports) {
556+
return nil
574557
}
575558

576-
// 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.
577-
if len(openStackCluster.Status.Bastion.DependentResources.Ports) != len(desiredPorts) {
578-
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))
559+
securityGroups := getBastionSecurityGroups(openStackCluster)
560+
bastionTags := []string{}
561+
err := networkingService.CreatePorts(openStackCluster, clusterName, bastionName(clusterName), securityGroups, bastionTags, desiredPorts, dependentResources)
562+
if err != nil {
563+
return fmt.Errorf("failed to create ports for bastion %s: %w", bastionName(openStackCluster.Name), err)
579564
}
580565

581566
return nil

controllers/openstackmachine_controller.go

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
379379
return ctrl.Result{}, err
380380
}
381381

382-
err = getOrCreateMachinePorts(scope, openStackCluster, machine, openStackMachine, networkingService, clusterName)
382+
err = getOrCreateMachinePorts(openStackCluster, machine, openStackMachine, networkingService, clusterName)
383383
if err != nil {
384384
return ctrl.Result{}, err
385385
}
@@ -493,32 +493,18 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
493493
return ctrl.Result{}, nil
494494
}
495495

496-
func getOrCreateMachinePorts(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error {
497-
scope.Logger().Info("Reconciling ports for machine", "machine", machine.Name)
498-
var machinePortsStatus []infrav1.PortStatus
499-
var err error
500-
496+
func getOrCreateMachinePorts(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, networkingService *networking.Service, clusterName string) error {
501497
desiredPorts := openStackMachine.Status.ReferencedResources.Ports
502-
portsToCreate := networking.MissingPorts(openStackMachine.Status.DependentResources.Ports, desiredPorts)
503-
504-
// Sanity check that the number of desired ports is equal to the addition of ports to create and ports that already exist.
505-
if len(desiredPorts) != len(portsToCreate)+len(openStackMachine.Status.DependentResources.Ports) {
506-
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))
507-
}
498+
dependentResources := &openStackMachine.Status.DependentResources
508499

509-
if len(portsToCreate) > 0 {
510-
instanceTags := getInstanceTags(openStackMachine, openStackCluster)
511-
managedSecurityGroups := getManagedSecurityGroups(openStackCluster, machine, openStackMachine)
512-
machinePortsStatus, err = networkingService.CreatePorts(openStackMachine, clusterName, portsToCreate, managedSecurityGroups, instanceTags, openStackMachine.Name)
513-
if err != nil {
514-
return fmt.Errorf("create ports: %w", err)
515-
}
516-
openStackMachine.Status.DependentResources.Ports = append(openStackMachine.Status.DependentResources.Ports, machinePortsStatus...)
500+
if len(desiredPorts) == len(dependentResources.Ports) {
501+
return nil
517502
}
518503

519-
// 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.
520-
if len(openStackMachine.Status.DependentResources.Ports) != len(desiredPorts) {
521-
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))
504+
instanceTags := getInstanceTags(openStackMachine, openStackCluster)
505+
managedSecurityGroups := getManagedSecurityGroups(openStackCluster, machine, openStackMachine)
506+
if err := networkingService.CreatePorts(openStackMachine, clusterName, openStackMachine.Name, managedSecurityGroups, instanceTags, desiredPorts, dependentResources); err != nil {
507+
return fmt.Errorf("creating ports: %w", err)
522508
}
523509

524510
return nil

pkg/cloud/services/networking/port.go

Lines changed: 17 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (s *Service) GetPortFromInstanceIP(instanceID string, ip string) ([]ports.P
5858
return s.client.ListPort(portOpts)
5959
}
6060

61-
func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, portName string, portOpts *infrav1.PortOpts, instanceSecurityGroups []string, instanceTags []string) (*ports.Port, error) {
61+
func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, portName string, portOpts *infrav1.PortOpts, defaultSecurityGroups []string, baseTags []string) (*ports.Port, error) {
6262
var err error
6363
networkID := portOpts.Network.ID
6464

@@ -86,7 +86,7 @@ func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, por
8686
}
8787
// inherit port security groups from the instance if not explicitly specified
8888
if len(securityGroups) == 0 {
89-
securityGroups = instanceSecurityGroups
89+
securityGroups = defaultSecurityGroups
9090
}
9191
}
9292

@@ -158,7 +158,7 @@ func (s *Service) CreatePort(eventObject runtime.Object, clusterName string, por
158158
}
159159

160160
var tags []string
161-
tags = append(tags, instanceTags...)
161+
tags = append(tags, baseTags...)
162162
tags = append(tags, portOpts.Tags...)
163163
if len(tags) > 0 {
164164
if err = s.replaceAllAttributesTags(eventObject, portResource, port.ID, tags); err != nil {
@@ -314,37 +314,32 @@ func GetPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) stri
314314
return fmt.Sprintf("%s-%d", instanceName, netIndex)
315315
}
316316

317-
func (s *Service) CreatePorts(eventObject runtime.Object, clusterName string, ports []infrav1.PortOpts, securityGroups []infrav1.SecurityGroupFilter, instanceTags []string, instanceName string) ([]infrav1.PortStatus, error) {
318-
return s.createPortsImpl(eventObject, clusterName, ports, securityGroups, instanceTags, instanceName)
319-
}
320-
321-
func (s *Service) createPortsImpl(eventObject runtime.Object, clusterName string, ports []infrav1.PortOpts, securityGroups []infrav1.SecurityGroupFilter, instanceTags []string, instanceName string) ([]infrav1.PortStatus, error) {
322-
instanceSecurityGroups, err := s.GetSecurityGroups(securityGroups)
317+
func (s *Service) CreatePorts(eventObject runtime.Object, clusterName, baseName string, securityGroups []infrav1.SecurityGroupFilter, baseTags []string, desiredPorts []infrav1.PortOpts, dependentResources *infrav1.DependentMachineResources) error {
318+
defaultSecurityGroups, err := s.GetSecurityGroups(securityGroups)
323319
if err != nil {
324-
return nil, fmt.Errorf("error getting security groups: %v", err)
320+
return fmt.Errorf("error getting security groups: %v", err)
325321
}
326322

327-
portsStatus := make([]infrav1.PortStatus, 0, len(ports))
328-
329-
for i := range ports {
330-
portOpts := &ports[i]
331-
iTags := []string{}
332-
if len(instanceTags) > 0 {
333-
iTags = instanceTags
323+
for i := range desiredPorts {
324+
// Skip creation of ports which already exist
325+
if i < len(dependentResources.Ports) {
326+
continue
334327
}
335-
portName := GetPortName(instanceName, portOpts, i)
328+
329+
portOpts := &desiredPorts[i]
330+
portName := GetPortName(baseName, portOpts, i)
336331
// Events are recorded in CreatePort
337-
port, err := s.CreatePort(eventObject, clusterName, portName, portOpts, instanceSecurityGroups, iTags)
332+
port, err := s.CreatePort(eventObject, clusterName, portName, portOpts, defaultSecurityGroups, baseTags)
338333
if err != nil {
339-
return nil, err
334+
return err
340335
}
341336

342-
portsStatus = append(portsStatus, infrav1.PortStatus{
337+
dependentResources.Ports = append(dependentResources.Ports, infrav1.PortStatus{
343338
ID: port.ID,
344339
})
345340
}
346341

347-
return portsStatus, nil
342+
return nil
348343
}
349344

350345
// ConstructPorts builds an array of ports from the instance spec.
@@ -640,16 +635,3 @@ func (s *Service) AdoptBastionPorts(scope *scope.WithLogger, openStackCluster *i
640635

641636
return changed, nil
642637
}
643-
644-
// MissingPorts returns the ports that are not in the ports status but are desired ports which should be created.
645-
func MissingPorts(portsStatus []infrav1.PortStatus, desiredPorts []infrav1.PortOpts) []infrav1.PortOpts {
646-
// missingPorts is equal to the ports status minus its length
647-
missingPortsLength := len(desiredPorts) - len(portsStatus)
648-
649-
// rebuild desiredPorts to only contain the ports that were not adopted
650-
missingPorts := make([]infrav1.PortOpts, missingPortsLength)
651-
for i := 0; i < missingPortsLength; i++ {
652-
missingPorts[i] = desiredPorts[i+len(portsStatus)]
653-
}
654-
return missingPorts
655-
}

pkg/cloud/services/networking/port_test.go

Lines changed: 0 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -837,71 +837,3 @@ func Test_getPortName(t *testing.T) {
837837
})
838838
}
839839
}
840-
841-
func Test_MissingPorts(t *testing.T) {
842-
tests := []struct {
843-
name string
844-
portsStatus []infrav1.PortStatus
845-
desiredPorts []infrav1.PortOpts
846-
expectedMissing []infrav1.PortOpts
847-
}{
848-
{
849-
name: "no missing ports",
850-
portsStatus: []infrav1.PortStatus{
851-
{
852-
ID: "06d18afd-a8d2-4c0f-b6be-63fe71d6c16d",
853-
},
854-
{
855-
ID: "7bf62a7e-a969-40cc-b50c-87bd52e97188",
856-
},
857-
},
858-
desiredPorts: []infrav1.PortOpts{
859-
{
860-
Network: &infrav1.NetworkFilter{
861-
ID: "94588d9b-21f1-4583-97ed-c7367327b0ea",
862-
},
863-
},
864-
{
865-
Network: &infrav1.NetworkFilter{
866-
ID: "9cc0ebba-eaec-4dc7-b5cf-ece51f699f47",
867-
},
868-
},
869-
},
870-
expectedMissing: []infrav1.PortOpts{},
871-
},
872-
{
873-
name: "missing ports",
874-
portsStatus: []infrav1.PortStatus{
875-
{
876-
ID: "06d18afd-a8d2-4c0f-b6be-63fe71d6c16d",
877-
},
878-
},
879-
desiredPorts: []infrav1.PortOpts{
880-
{
881-
Network: &infrav1.NetworkFilter{
882-
ID: "94588d9b-21f1-4583-97ed-c7367327b0ea",
883-
},
884-
},
885-
{
886-
Network: &infrav1.NetworkFilter{
887-
ID: "9cc0ebba-eaec-4dc7-b5cf-ece51f699f47",
888-
},
889-
},
890-
},
891-
expectedMissing: []infrav1.PortOpts{
892-
{
893-
Network: &infrav1.NetworkFilter{
894-
ID: "9cc0ebba-eaec-4dc7-b5cf-ece51f699f47",
895-
},
896-
},
897-
},
898-
},
899-
}
900-
for _, tt := range tests {
901-
t.Run(tt.name, func(t *testing.T) {
902-
g := NewWithT(t)
903-
got := MissingPorts(tt.portsStatus, tt.desiredPorts)
904-
g.Expect(got).To(Equal(tt.expectedMissing))
905-
})
906-
}
907-
}

0 commit comments

Comments
 (0)