Fix failure to delete dangling ports on error in createInstance

We were previously failing to handle several error return cases which
would result in leaking ports. The defer() method of cleanup is
idiomatic, will handle the existing missing cases, and also the volume
cleanup case which is about to be added.

pkg/cloud/services/compute/instance.go

@@ -206,10 +206,20 @@ func (s *Service) createInstance(eventObject runtime.Object, clusterName string,
 		})
 	}
 
-	if instanceSpec.Subnet != "" && accessIPv4 == "" {
+	var server *ServerExt
+
+	// 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 {
-			return nil, err
+			s.logger.V(4).Error(err, "failed to clean up ports after failure", "cluster", clusterName, "machine", instanceSpec.Name)
 		}
+	}()
+
+	if instanceSpec.Subnet != "" && accessIPv4 == "" {
 		return nil, fmt.Errorf("no ports with fixed IPs found on Subnet %q", instanceSpec.Subnet)
 	}
 
@@ -241,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

@@ -671,8 +671,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,
 		},
@@ -687,8 +686,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,
 		},