Merge pull request #1063 from shiftstack/cleanup-ports

🐛Cleanup ports

pkg/cloud/services/compute/instance.go

@@ -179,9 +179,21 @@ func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster,
 }
 
 func (s *Service) createInstance(eventObject runtime.Object, clusterName string, instanceSpec *InstanceSpec, retryInterval time.Duration) (*InstanceStatus, error) {
+	var server *ServerExt
 	accessIPv4 := ""
 	portList := []servers.Network{}
 
+	// Ensure we delete the ports we created if we haven't created the server.
+	defer func() {
+		if server != nil {
+			return
+		}
+
+		if err := s.deletePorts(eventObject, portList); err != nil {
+			s.logger.V(4).Error(err, "failed to clean up ports after failure", "cluster", clusterName, "machine", instanceSpec.Name)
+		}
+	}()
+
 	for i, network := range instanceSpec.Networks {
 		if network.ID == "" {
 			return nil, fmt.Errorf("no network was found or provided. Please check your machine configuration and try again")
@@ -208,9 +220,6 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 	}
 
 	if instanceSpec.Subnet != "" && accessIPv4 == "" {
-		if err := s.deletePorts(eventObject, portList); err != nil {
-			return nil, err
-		}
 		return nil, fmt.Errorf("no ports with fixed IPs found on Subnet %q", instanceSpec.Subnet)
 	}
 
@@ -242,16 +251,12 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 
 	serverCreateOpts = applyServerGroupID(serverCreateOpts, instanceSpec.ServerGroupID)
 
-	server, err := s.computeService.CreateServer(keypairs.CreateOptsExt{
+	server, err = s.computeService.CreateServer(keypairs.CreateOptsExt{
 		CreateOptsBuilder: serverCreateOpts,
 		KeyName:           instanceSpec.SSHKeyName,
 	})
 	if err != nil {
-		serverErr := err
-		if err = s.deletePorts(eventObject, portList); err != nil {
-			return nil, fmt.Errorf("error creating OpenStack instance: %v, error deleting ports: %v", serverErr, err)
-		}
-		return nil, fmt.Errorf("error creating Openstack instance: %v", serverErr)
+		return nil, fmt.Errorf("error creating Openstack instance: %v", err)
 	}
 
 	instanceCreateTimeout := getTimeout("CLUSTER_API_OPENSTACK_INSTANCE_CREATE_TIMEOUT", timeoutInstanceCreate)

pkg/cloud/services/compute/instance_test.go

@@ -667,8 +667,7 @@ func TestService_CreateInstance(t *testing.T) {
 
 				computeRecorder.ListImages(images.ListOpts{Name: imageName}).Return(nil, fmt.Errorf("test error"))
 
-				// FIXME: This should cleanup ports
-				// expectCleanupDefaultPort(networkRecorder)
+				expectCleanupDefaultPort(networkRecorder)
 			},
 			wantErr: true,
 		},
@@ -683,8 +682,7 @@ func TestService_CreateInstance(t *testing.T) {
 				computeRecorder.ListImages(images.ListOpts{Name: imageName}).Return([]images.Image{{ID: imageUUID}}, nil)
 				computeRecorder.GetFlavorIDFromName(flavorName).Return("", fmt.Errorf("test error"))
 
-				// FIXME: This should cleanup ports
-				// expectCleanupDefaultPort(networkRecorder)
+				expectCleanupDefaultPort(networkRecorder)
 			},
 			wantErr: true,
 		},
@@ -704,6 +702,32 @@ func TestService_CreateInstance(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name:                "Delete previously created ports on port creation error",
+			getMachine:          getDefaultMachine,
+			getOpenStackCluster: getDefaultOpenStackCluster,
+			getOpenStackMachine: func() *infrav1.OpenStackMachine {
+				m := getDefaultOpenStackMachine()
+				m.Spec.Ports = []infrav1.PortOpts{
+					{Description: "Test port 0"},
+					{Description: "Test port 1"},
+				}
+				return m
+			},
+			expect: func(computeRecorder *MockClientMockRecorder, networkRecorder *mock_networking.MockNetworkClientMockRecorder) {
+				expectCreateDefaultPort(networkRecorder)
+
+				// Looking up the second port fails
+				networkRecorder.ListPort(ports.ListOpts{
+					Name:      "test-openstack-machine-1",
+					NetworkID: networkUUID,
+				}).Return(nil, fmt.Errorf("test error"))
+
+				// We should cleanup the first port
+				expectCleanupDefaultPort(networkRecorder)
+			},
+			wantErr: true,
+		},
 		{
 			name:                "Poll until server is created",
 			getMachine:          getDefaultMachine,