Fix crash on delete with no bastion

Author: mdbooth

controllers/openstackcluster_controller.go

@@ -291,15 +291,13 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac
 	}
 
 	// If no instance was created we currently need to check for orphaned
-	// volumes. This requires resolving the instance spec.
-	// TODO: write volumes to status resources on creation so this is no longer required.
+	// volumes.
 	if instanceStatus == nil {
-		instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster)
-		if err != nil {
-			return err
-		}
-		if err := computeService.DeleteVolumes(instanceSpec); err != nil {
-			return fmt.Errorf("delete volumes: %w", err)
+		bastion := openStackCluster.Spec.Bastion
+		if bastion != nil && bastion.Spec != nil {
+			if err := computeService.DeleteVolumes(bastionName(cluster.Name), bastion.Spec.RootVolume, bastion.Spec.AdditionalBlockDevices); err != nil {
+				return fmt.Errorf("delete volumes: %w", err)
+			}
 		}
 	} else {
 		instanceNS, err := instanceStatus.NetworkStatus()

controllers/openstackmachine_controller.go

@@ -300,19 +300,12 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl
 	}
 
 	// If no instance was created we currently need to check for orphaned
-	// volumes. This requires resolving the instance spec.
-	// TODO: write volumes to status resources on creation so this is no longer required.
-	if instanceStatus == nil && openStackMachine.Status.Resolved != nil {
-		instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-		if err := computeService.DeleteVolumes(instanceSpec); err != nil {
+	// volumes.
+	if instanceStatus == nil {
+		if err := computeService.DeleteVolumes(openStackMachine.Name, openStackMachine.Spec.RootVolume, openStackMachine.Spec.AdditionalBlockDevices); err != nil {
 			return ctrl.Result{}, fmt.Errorf("delete volumes: %w", err)
 		}
-	}
-
-	if instanceStatus != nil {
+	} else {
 		if err := computeService.DeleteInstance(openStackMachine, instanceStatus); err != nil {
 			conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
 			return ctrl.Result{}, fmt.Errorf("delete instance: %w", err)

pkg/cloud/services/compute/instance.go

@@ -431,11 +431,14 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins
 }
 
 // DeleteVolumes deletes any cinder volumes which were created for the instance.
-// Note that this must only be called when the server was not successfully
+// Note that this need only be called when the server was not successfully
 // created. If the server was created the volume will have been added with
 // DeleteOnTermination=true, and will be automatically cleaned up with the
 // server.
-func (s *Service) DeleteVolumes(instanceSpec *InstanceSpec) error {
+// We don't pass InstanceSpec here because we only require instance name,
+// rootVolume, and additionalBlockDevices, and resolving the whole InstanceSpec
+// introduces unnecessary failure modes.
+func (s *Service) DeleteVolumes(instanceName string, rootVolume *infrav1.RootVolume, additionalBlockDevices []infrav1.AdditionalBlockDevice) error {
 	/*
 		Attaching volumes to an instance is a two-step process:
 
@@ -455,13 +458,13 @@ func (s *Service) DeleteVolumes(instanceSpec *InstanceSpec) error {
 		DeleteOnTermination will ensure it is deleted in that case.
 	*/
 
-	if hasRootVolume(instanceSpec) {
-		if err := s.deleteVolume(instanceSpec.Name, "root"); err != nil {
+	if rootVolume != nil && rootVolume.SizeGiB > 0 {
+		if err := s.deleteVolume(instanceName, "root"); err != nil {
 			return err
 		}
 	}
-	for _, volumeSpec := range instanceSpec.AdditionalBlockDevices {
-		if err := s.deleteVolume(instanceSpec.Name, volumeSpec.Name); err != nil {
+	for _, volumeSpec := range additionalBlockDevices {
+		if err := s.deleteVolume(instanceName, volumeSpec.Name); err != nil {
 			return err
 		}
 	}