Skip to content

Commit

Permalink
Fix Port Leaks
Browse files Browse the repository at this point in the history
When an instance fails to come up, there's a bug where the port doesn't
get cleaned up, and eventually you will exhaust your quota. The code
does attempt to garbage collect, but I suspect at some point in the past
ports were suffixed with an index, but the garbage collection code
wasn't made aware of this, so lists nothing. Replace the APi "filtering"
that doesn't work with a manual filter that does prefix matching. As I'm
a good boy, regression tests have been added to protect the
functionality.
  • Loading branch information
spjmurray committed Aug 21, 2023
1 parent f938384 commit 13d3d5c
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 3 deletions.
13 changes: 10 additions & 3 deletions pkg/cloud/services/networking/port.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
62 changes: 62 additions & 0 deletions pkg/cloud/services/networking/port_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 := "foo-0"
portName2 := "foo-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
}

0 comments on commit 13d3d5c

Please sign in to comment.