UPSTREAM 2842: Fix instance deletion order

Delete the OpenStack instance before deleting its network ports. This
prevents "Policy doesn't allow compute:detach primary port" errors that
occur when attempting to delete a primary NIC while the instance still
exists.

Deleting the instance automatically detaches all interfaces, so the
explicit detach call is no longer needed.

Author: mandre

pkg/cloud/services/compute/instance.go

@@ -657,12 +657,13 @@ func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eve
 		return err
 	}
 
-	// get and delete trunks
-	for _, port := range instanceInterfaces {
-		if err = s.deleteAttachInterface(eventObject, instanceStatus.InstanceIdentifier(), port.PortID); err != nil {
-			return err
-		}
+	// Delete the instance before ports, since openstack likely prevents the deletion of the primary NIC.
+	// Deleting the instance also automatically detach the interfaces.
+	if err := s.deleteInstance(eventObject, instanceStatus.InstanceIdentifier()); err != nil {
+		return err
+	}
 
+	for _, port := range instanceInterfaces {
 		if trunkSupported {
 			if err = networkingService.DeleteTrunk(eventObject, port.PortID); err != nil {
 				return err
@@ -685,7 +686,7 @@ func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eve
 		}
 	}
 
-	return s.deleteInstance(eventObject, instanceStatus.InstanceIdentifier())
+	return nil
 }
 
 func (s *Service) deleteVolumes(instanceSpec *InstanceSpec) error {
@@ -740,26 +741,6 @@ func (s *Service) deletePorts(eventObject runtime.Object, nets []servers.Network
 	return nil
 }
 
-func (s *Service) deleteAttachInterface(eventObject runtime.Object, instance *InstanceIdentifier, portID string) error {
-	err := s.getComputeClient().DeleteAttachedInterface(instance.ID, portID)
-	if err != nil {
-		if capoerrors.IsNotFound(err) {
-			record.Eventf(eventObject, "SuccessfulDeleteAttachInterface", "Attach interface did not exist: instance %s, port %s", instance.ID, portID)
-			return nil
-		}
-		if capoerrors.IsConflict(err) {
-			// we don't want to block deletion because of Conflict
-			// due to instance must be paused/active/shutoff in order to detach interface
-			return nil
-		}
-		record.Warnf(eventObject, "FailedDeleteAttachInterface", "Failed to delete attach interface: instance %s, port %s: %v", instance.ID, portID, err)
-		return err
-	}
-
-	record.Eventf(eventObject, "SuccessfulDeleteAttachInterface", "Deleted attach interface: instance %s, port %s", instance.ID, portID)
-	return nil
-}
-
 func (s *Service) deleteInstance(eventObject runtime.Object, instance *InstanceIdentifier) error {
 	err := s.getComputeClient().DeleteServer(instance.ID)
 	if err != nil {

pkg/cloud/services/compute/instance_test.go

@@ -1032,13 +1032,12 @@ func TestService_DeleteInstance(t *testing.T) {
 						Alias: "trunk",
 					},
 				}}, nil)
-				r.compute.DeleteAttachedInterface(instanceUUID, portUUID).Return(nil)
+				r.compute.DeleteServer(instanceUUID).Return(nil)
+				r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrDefault404{})
+
 				// FIXME: Why we are looking for a trunk when we know the port is not trunked?
 				r.network.ListTrunk(trunks.ListOpts{PortID: portUUID}).Return([]trunks.Trunk{}, nil)
 				r.network.DeletePort(portUUID).Return(nil)
-
-				r.compute.DeleteServer(instanceUUID).Return(nil)
-				r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrDefault404{})
 			},
 			wantErr: false,
 		},