Merge pull request #2834 from bnallapeta/soft-delete

🐛 Handle SOFT_DELETED and DELETED states in server deletion

Author: k8s-ci-robot

api/v1beta1/types.go

@@ -792,6 +792,11 @@ var (
 	// InstanceStateDeleted is the string representing an instance in a deleted state.
 	InstanceStateDeleted = InstanceState("DELETED")
 
+	// InstanceStateSoftDeleted is the string representing an instance in a soft-deleted state.
+	// This state occurs when OpenStack is configured with a reclaim_instance_interval > 0,
+	// allowing recovery of deleted instances within the reclaim period.
+	InstanceStateSoftDeleted = InstanceState("SOFT_DELETED")
+
 	// InstanceStateUndefined is the string representing an undefined instance state.
 	InstanceStateUndefined = InstanceState("")
 )

pkg/cloud/services/compute/instance.go

@@ -490,10 +490,15 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins
 		if err != nil {
 			return false, err
 		}
-		if i != nil {
-			return false, nil
+		// Server not found means it has been permanently deleted
+		if i == nil {
+			return true, nil
+		}
+		// Server in SOFT_DELETED or DELETED state means deletion succeeded. This respects OpenStack's soft delete policy.
+		if i.State() == infrav1.InstanceStateSoftDeleted || i.State() == infrav1.InstanceStateDeleted {
+			return true, nil
 		}
-		return true, nil
+		return false, nil
 	})
 	if err != nil {
 		record.Warnf(eventObject, "FailedDeleteServer", "Failed to delete server %s with id %s: %v", instance.Name, instance.ID, err)

pkg/cloud/services/compute/instance_test.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/go-logr/logr/testr"
 	"github.com/google/go-cmp/cmp"
+	"github.com/gophercloud/gophercloud/v2"
 	"github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes"
 	"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/keypairs"
 	"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers"
@@ -1021,3 +1022,101 @@ func TestService_ReconcileInstance(t *testing.T) {
 		})
 	}
 }
+
+func TestService_DeleteInstance(t *testing.T) {
+	const (
+		serverID   = "ce96e584-7ebc-46d6-9e55-987d72e3806c"
+		serverName = "test-server"
+	)
+
+	tests := []struct {
+		name    string
+		expect  func(m *mock.MockComputeClientMockRecorder)
+		wantErr bool
+	}{
+		{
+			name: "Server not found after delete",
+			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.DeleteServer(serverID).Return(nil)
+				m.GetServer(serverID).Return(nil, &gophercloud.ErrResourceNotFound{})
+			},
+			wantErr: false,
+		},
+		{
+			name: "Server in SOFT_DELETED state",
+			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.DeleteServer(serverID).Return(nil)
+				m.GetServer(serverID).Return(&servers.Server{
+					ID:     serverID,
+					Name:   serverName,
+					Status: "SOFT_DELETED",
+				}, nil)
+			},
+			wantErr: false,
+		},
+		{
+			name: "Server in DELETED state",
+			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.DeleteServer(serverID).Return(nil)
+				m.GetServer(serverID).Return(&servers.Server{
+					ID:     serverID,
+					Name:   serverName,
+					Status: "DELETED",
+				}, nil)
+			},
+			wantErr: false,
+		},
+		{
+			name: "Delete API returns not found",
+			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.DeleteServer(serverID).Return(&gophercloud.ErrResourceNotFound{})
+			},
+			wantErr: false,
+		},
+		{
+			name: "Delete API returns error",
+			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.DeleteServer(serverID).Return(errors.New("API error"))
+			},
+			wantErr: true,
+		},
+		{
+			name: "GetServer returns error",
+			expect: func(m *mock.MockComputeClientMockRecorder) {
+				m.DeleteServer(serverID).Return(nil)
+				m.GetServer(serverID).Return(nil, errors.New("API error"))
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			log := testr.New(t)
+			mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "")
+
+			tt.expect(mockScopeFactory.ComputeClient.EXPECT())
+
+			s, err := NewService(scope.NewWithLogger(mockScopeFactory, log))
+			if err != nil {
+				t.Fatalf("Failed to create service: %v", err)
+			}
+
+			instanceStatus := &InstanceStatus{
+				server: &servers.Server{
+					ID:   serverID,
+					Name: serverName,
+				},
+				logger: log,
+			}
+
+			eventObject := &infrav1.OpenStackMachine{}
+			err = s.DeleteInstance(eventObject, instanceStatus)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Service.DeleteInstance() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+		})
+	}
+}