diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index 3103c60907..8b0177de3a 100644 --- a/controllers/openstackcluster_controller.go +++ b/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) } diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index 02deee62ee..7b50c63a43 100644 --- a/controllers/openstackmachine_controller.go +++ b/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) } diff --git a/pkg/cloud/services/compute/instance.go b/pkg/cloud/services/compute/instance.go index ffb3fbeeda..c35f049335 100644 --- a/pkg/cloud/services/compute/instance.go +++ b/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 } } diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index 00d952494c..698392ad85 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/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) } }) diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index 0d662ea4c1..9f13608b7e 100644 --- a/pkg/cloud/services/networking/port.go +++ b/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 + } } } diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index eca3f8e61b..952de474eb 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -27,6 +27,7 @@ import ( "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" "github.com/gophercloud/gophercloud/openstack/networking/v2/subnets" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" "sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock" @@ -546,36 +547,56 @@ 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{} + eventObject := &infrav1.OpenStackMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: instanceName, + }, + } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) @@ -587,6 +608,7 @@ func Test_GarbageCollectErrorInstancesPort(t *testing.T) { err := s.GarbageCollectErrorInstancesPort( eventObject, instanceName, + tt.portOpts, ) if tt.wantErr { g.Expect(err).To(HaveOccurred())