diff --git a/pkg/cloud/services/compute/instance.go b/pkg/cloud/services/compute/instance.go index 14d5d2db58..ffb3fbeeda 100644 --- a/pkg/cloud/services/compute/instance.go +++ b/pkg/cloud/services/compute/instance.go @@ -283,7 +283,7 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste if len(instanceSpec.Tags) > 0 { iTags = instanceSpec.Tags } - portName := getPortName(instanceSpec.Name, portOpts, i) + portName := networking.GetPortName(instanceSpec.Name, portOpts, i) port, err := networkingService.GetOrCreatePort(eventObject, clusterName, portName, portOpts, securityGroups, iTags) if err != nil { return nil, err @@ -380,14 +380,6 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste return createdInstance, nil } -// getPortName appends a suffix to an instance name in order to try and get a unique name per port. -func getPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) string { - if opts != nil && opts.NameSuffix != "" { - return fmt.Sprintf("%s-%s", instanceName, opts.NameSuffix) - } - return fmt.Sprintf("%s-%d", instanceName, netIndex) -} - func rootVolumeName(instanceName string) string { return fmt.Sprintf("%s-root", instanceName) } diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index b6be591860..00d952494c 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/pkg/cloud/services/compute/instance_test.go @@ -48,6 +48,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" "sigs.k8s.io/cluster-api-provider-openstack/pkg/clients" "sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock" + "sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking" "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" ) @@ -110,7 +111,7 @@ func Test_getPortName(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := getPortName(tt.args.instanceName, tt.args.opts, tt.args.netIndex); got != tt.want { + if got := networking.GetPortName(tt.args.instanceName, tt.args.opts, tt.args.netIndex); got != tt.want { t.Errorf("getPortName() = %v, want %v", got, tt.want) } }) diff --git a/pkg/cloud/services/networking/port.go b/pkg/cloud/services/networking/port.go index c354f8c43f..0d662ea4c1 100644 --- a/pkg/cloud/services/networking/port.go +++ b/pkg/cloud/services/networking/port.go @@ -290,13 +290,20 @@ func (s *Service) DeletePorts(openStackCluster *infrav1.OpenStackCluster) error } func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string) error { - portList, err := s.client.ListPort(ports.ListOpts{ - Name: instanceName, - }) + // 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 } + for _, p := range portList { + if !strings.HasPrefix(p.Name, instanceName) { + continue + } + if err := s.DeletePort(eventObject, p.ID); err != nil { return err } @@ -304,3 +311,11 @@ func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, i return nil } + +// GetPortName appends a suffix to an instance name in order to try and get a unique name per port. +func GetPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) string { + if opts != nil && opts.NameSuffix != "" { + return fmt.Sprintf("%s-%s", instanceName, opts.NameSuffix) + } + return fmt.Sprintf("%s-%d", instanceName, netIndex) +} diff --git a/pkg/cloud/services/networking/port_test.go b/pkg/cloud/services/networking/port_test.go index 88c8b9e7b3..eca3f8e61b 100644 --- a/pkg/cloud/services/networking/port_test.go +++ b/pkg/cloud/services/networking/port_test.go @@ -535,6 +535,68 @@ func Test_GetOrCreatePort(t *testing.T) { } } +func Test_GarbageCollectErrorInstancesPort(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + instanceName := "foo" + portID1 := "dc6e0ae3-dad6-4240-a9cb-e541916f20d3" + portID2 := "a38ab1cb-c2cc-4c1b-9d1d-696ec73356d2" + portName1 := GetPortName(instanceName, nil, 0) + portName2 := GetPortName(instanceName, nil, 1) + + tests := []struct { + name string + expect func(m *mock.MockNetworkClientMockRecorder) + 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", + }, + { + ID: portID1, + Name: portName1, + }, + { + ID: portID2, + Name: portName2, + }, + } + m.ListPort(ports.ListOpts{}).Return(p, nil) + m.DeletePort(portID1) + m.DeletePort(portID2) + }, + wantErr: false, + }, + } + + eventObject := &infrav1.OpenStackMachine{} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + mockClient := mock.NewMockNetworkClient(mockCtrl) + tt.expect(mockClient.EXPECT()) + s := Service{ + client: mockClient, + } + err := s.GarbageCollectErrorInstancesPort( + eventObject, + instanceName, + ) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} + func pointerTo(b bool) *bool { return &b }