Take 2

Author: spjmurray

controllers/openstackcluster_controller.go

@@ -237,8 +237,9 @@ func deleteBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackClust
 			}
 		}
 
-		rootVolume := openStackCluster.Spec.Bastion.Instance.RootVolume
-		if err = computeService.DeleteInstance(openStackCluster, instanceStatus, instanceName, rootVolume); err != nil {
+		instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
+
+		if err = computeService.DeleteInstance(openStackCluster, openStackCluster, instanceStatus, instanceSpec); err != nil {
 			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete bastion: %w", err))
 			return fmt.Errorf("failed to delete bastion: %w", err)
 		}

controllers/openstackmachine_controller.go

@@ -273,7 +273,9 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope scope.Scope, cluster
 		}
 	}
 
-	if err := computeService.DeleteInstance(openStackMachine, instanceStatus, openStackMachine.Name, openStackMachine.Spec.RootVolume); err != nil {
+	instanceSpec := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
+
+	if err := computeService.DeleteInstance(openStackCluster, openStackMachine, instanceStatus, instanceSpec); err != nil {
 		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
 		return ctrl.Result{}, fmt.Errorf("delete instance: %w", err)
 	}

pkg/cloud/services/compute/instance.go

@@ -542,7 +542,7 @@ func (s *Service) GetManagementPort(openStackCluster *infrav1.OpenStackCluster,
 	return &allPorts[0], nil
 }
 
-func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *InstanceStatus, instanceName string, rootVolume *infrav1.RootVolume) error {
+func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eventObject runtime.Object, instanceStatus *InstanceStatus, instanceSpec *InstanceSpec) error {
 	if instanceStatus == nil {
 		/*
 			We create a boot-from-volume instance in 2 steps:
@@ -562,8 +562,8 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins
 			Note that we don't need to separately delete the root volume when deleting the instance because
 			DeleteOnTermination will ensure it is deleted in that case.
 		*/
-		if hasRootVolume(rootVolume) {
-			name := rootVolumeName(instanceName)
+		if hasRootVolume(instanceSpec.RootVolume) {
+			name := rootVolumeName(instanceSpec.Name)
 			volume, err := s.getVolumeByName(name)
 			if err != nil {
 				return err
@@ -612,7 +612,12 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins
 
 	// delete port of error instance
 	if instanceStatus.State() == infrav1.InstanceStateError {
-		if err := networkingService.GarbageCollectErrorInstancesPort(eventObject, instanceStatus.Name()); err != nil {
+		portOpts, err := s.constructPorts(openStackCluster, instanceSpec)
+		if err != nil {
+			return err
+		}
+
+		if err := networkingService.GarbageCollectErrorInstancesPort(eventObject, instanceSpec.Name, portOpts); err != nil {
 			return err
 		}
 	}

pkg/cloud/services/compute/instance_test.go

@@ -820,7 +820,13 @@ func TestService_DeleteInstance(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Failed to create service: %v", err)
 			}
-			if err := s.DeleteInstance(tt.eventObject, tt.instanceStatus(), openStackMachineName, tt.rootVolume); (err != nil) != tt.wantErr {
+
+			instanceSpec := &InstanceSpec{
+				Name:       openStackMachineName,
+				RootVolume: tt.rootVolume,
+			}
+
+			if err := s.DeleteInstance(&infrav1.OpenStackCluster{}, tt.eventObject, tt.instanceStatus(), instanceSpec); (err != nil) != tt.wantErr {
 				t.Errorf("Service.DeleteInstance() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

pkg/cloud/services/networking/port.go

@@ -289,23 +289,24 @@ func (s *Service) DeletePorts(openStackCluster *infrav1.OpenStackCluster) error
 	return nil
 }
 
-func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string) error {
-	// NOTE: when creating an instance, ports are named using getPortName(), which
-	// features an index, so we must list all ports and delete those that start with
-	// the instance name.  Specifying the name in the list options will only return
-	// a direct match.
-	portList, err := s.client.ListPort(ports.ListOpts{})
-	if err != nil {
-		return err
-	}
+func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string, portOpts []infrav1.PortOpts) error {
+	for i := range portOpts {
+		portOpt := &portOpts[i]
 
-	for _, p := range portList {
-		if !strings.HasPrefix(p.Name, instanceName) {
-			continue
+		// TODO: whould be nice if gophercloud could be persuaded to accept multiple
+		// names as is allowed by the API in order to reduce API traffic.
+		portList, err := s.client.ListPort(ports.ListOpts{Name: GetPortName(instanceName, portOpt, i)})
+		if err != nil {
+			return err
 		}
 
-		if err := s.DeletePort(eventObject, p.ID); err != nil {
-			return err
+		// NOTE: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/1476
+		// It is up to the end user to specify a UNIQUE cluster name when provisioning in the
+		// same project, otherwise things will alias and we could delete more than we should.
+		for _, p := range portList {
+			if err := s.DeletePort(eventObject, p.ID); err != nil {
+				return err
+			}
 		}
 	}
 

pkg/cloud/services/networking/port_test.go

@@ -546,36 +546,52 @@ func Test_GarbageCollectErrorInstancesPort(t *testing.T) {
 	portName2 := GetPortName(instanceName, nil, 1)
 
 	tests := []struct {
-		name    string
-		expect  func(m *mock.MockNetworkClientMockRecorder)
+		// man is the name of the test.
+		name string
+		// expect allows definition of any expected calls to the mock.
+		expect func(m *mock.MockNetworkClientMockRecorder)
+		// portOpts defines the instance ports as defined in the OSM spec.
+		portOpts []infrav1.PortOpts
+		// wantErr defines whether the test is supposed to fail.
 		wantErr bool
 	}{
 		{
 			name: "garbage collects all ports for an instance",
 			expect: func(m *mock.MockNetworkClientMockRecorder) {
-				p := []ports.Port{
-					{
-						ID:   "9278e096-5c63-4ffb-9289-2bacf948dc51",
-						Name: "bar-0",
-					},
+				o1 := ports.ListOpts{
+					Name: portName1,
+				}
+				p1 := []ports.Port{
 					{
 						ID:   portID1,
 						Name: portName1,
 					},
+				}
+				m.ListPort(o1).Return(p1, nil)
+				m.DeletePort(portID1)
+				o2 := ports.ListOpts{
+					Name: portName2,
+				}
+				p2 := []ports.Port{
 					{
 						ID:   portID2,
 						Name: portName2,
 					},
 				}
-				m.ListPort(ports.ListOpts{}).Return(p, nil)
-				m.DeletePort(portID1)
+
+				m.ListPort(o2).Return(p2, nil)
 				m.DeletePort(portID2)
 			},
+			portOpts: []infrav1.PortOpts{
+				{},
+				{},
+			},
 			wantErr: false,
 		},
 	}
 
 	eventObject := &infrav1.OpenStackMachine{}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
@@ -587,6 +603,7 @@ func Test_GarbageCollectErrorInstancesPort(t *testing.T) {
 			err := s.GarbageCollectErrorInstancesPort(
 				eventObject,
 				instanceName,
+				tt.portOpts,
 			)
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())