Don't require an InstanceSpec for DeleteInstance

DeleteInstance requires very little data from InstanceSpec. The 2
methods to create an InstanceSpec, bastionToInstanceSpec and
machineToInstanceSpec, require more state than is required by
DeleteInstance, and have failure modes which are not relevant to
DeleteInstance.

Author: mdbooth

controllers/openstackcluster_controller.go

@@ -231,8 +231,8 @@ func deleteBastion(scope *scope.Scope, cluster *clusterv1.Cluster, openStackClus
 			}
 		}
 
-		instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
-		if err = computeService.DeleteInstance(openStackCluster, instanceSpec, instanceStatus); err != nil {
+		rootVolume := openStackCluster.Spec.Bastion.Instance.RootVolume
+		if err = computeService.DeleteInstance(openStackCluster, instanceStatus, instanceName, rootVolume); err != nil {
 			handleUpdateOSCError(openStackCluster, errors.Errorf("failed to delete bastion: %v", err))
 			return errors.Errorf("failed to delete bastion: %v", err)
 		}

controllers/openstackmachine_controller.go

@@ -278,15 +278,7 @@ func (r *OpenStackMachineReconciler) reconcileDelete(ctx context.Context, scope
 		}
 	}
 
-	instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
-	if err != nil {
-		err = errors.Errorf("machine spec is invalid: %v", err)
-		handleUpdateMachineError(scope.Logger, openStackMachine, err)
-		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InvalidMachineSpecReason, clusterv1.ConditionSeverityError, err.Error())
-		return ctrl.Result{}, err
-	}
-
-	if err := computeService.DeleteInstance(openStackMachine, instanceSpec, instanceStatus); err != nil {
+	if err := computeService.DeleteInstance(openStackMachine, instanceStatus, openStackMachine.Name, openStackMachine.Spec.RootVolume); err != nil {
 		handleUpdateMachineError(scope.Logger, openStackMachine, errors.Errorf("error deleting OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err))
 		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
 		return ctrl.Result{}, nil

pkg/cloud/services/compute/instance.go

@@ -513,7 +513,7 @@ func (s *Service) GetManagementPort(openStackCluster *infrav1.OpenStackCluster,
 	return &allPorts[0], nil
 }
 
-func (s *Service) DeleteInstance(eventObject runtime.Object, instanceSpec *InstanceSpec, instanceStatus *InstanceStatus) error {
+func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *InstanceStatus, instanceName string, rootVolume *infrav1.RootVolume) error {
 	if instanceStatus == nil {
 		/*
 			We create a boot-from-volume instance in 2 steps:
@@ -533,9 +533,8 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceSpec *Insta
 			Note that we don't need to separately delete the root volume when deleting the instance because
 			DeleteOnTermination will ensure it is deleted in that case.
 		*/
-		rootVolume := instanceSpec.RootVolume
 		if hasRootVolume(rootVolume) {
-			name := rootVolumeName(instanceSpec.Name)
+			name := rootVolumeName(instanceName)
 			volume, err := s.getVolumeByName(name)
 			if err != nil {
 				return err

pkg/cloud/services/compute/instance_test.go

@@ -1031,15 +1031,14 @@ func TestService_DeleteInstance(t *testing.T) {
 	tests := []struct {
 		name           string
 		eventObject    runtime.Object
-		instanceSpec   func() *InstanceSpec
 		instanceStatus func() *InstanceStatus
+		rootVolume     *infrav1.RootVolume
 		expect         func(computeRecorder *MockClientMockRecorder, networkRecorder *mock_networking.MockNetworkClientMockRecorder)
 		wantErr        bool
 	}{
 		{
 			name:           "Defaults",
 			eventObject:    &infrav1.OpenStackMachine{},
-			instanceSpec:   getDefaultInstanceSpec,
 			instanceStatus: getDefaultInstanceStatus,
 			expect: func(computeRecorder *MockClientMockRecorder, networkRecorder *mock_networking.MockNetworkClientMockRecorder) {
 				computeRecorder.ListAttachedInterfaces(instanceUUID).Return([]attachinterfaces.Interface{
@@ -1063,16 +1062,12 @@ func TestService_DeleteInstance(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name:        "Dangling volume",
-			eventObject: &infrav1.OpenStackMachine{},
-			instanceSpec: func() *InstanceSpec {
-				spec := getDefaultInstanceSpec()
-				spec.RootVolume = &infrav1.RootVolume{
-					Size: 50,
-				}
-				return spec
-			},
+			name:           "Dangling volume",
+			eventObject:    &infrav1.OpenStackMachine{},
 			instanceStatus: func() *InstanceStatus { return nil },
+			rootVolume: &infrav1.RootVolume{
+				Size: 50,
+			},
 			expect: func(computeRecorder *MockClientMockRecorder, networkRecorder *mock_networking.MockNetworkClientMockRecorder) {
 				// Fetch volume by name
 				volumeName := fmt.Sprintf("%s-root", openStackMachineName)
@@ -1112,7 +1107,7 @@ func TestService_DeleteInstance(t *testing.T) {
 					"", mockNetworkClient, logr.Discard(),
 				),
 			}
-			if err := s.DeleteInstance(tt.eventObject, tt.instanceSpec(), tt.instanceStatus()); (err != nil) != tt.wantErr {
+			if err := s.DeleteInstance(tt.eventObject, tt.instanceStatus(), openStackMachineName, tt.rootVolume); (err != nil) != tt.wantErr {
 				t.Errorf("Service.DeleteInstance() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})