HYPERFLEET-1439 - feat: cleanup desires after deletion

internal/desireclient/cleanup.go

@@ -0,0 +1,91 @@
+package desireclient
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"log/slog"
+
+	"github.com/openshift-hyperfleet/hyperfleet-adapter/internal/transportclient"
+	"github.com/openshift-hyperfleet/hyperfleet-applier/pkg/desire"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+// CleanupAfterDeletion implements transportclient.DesireCleaner. It removes
+// the delete desire (only when the applier confirms deletion) then the read
+// desire. Returns an error if the delete desire exists but is not yet confirmed,
+// or if no delete desire exists but an apply desire is still present (the
+// applier may not have applied it yet), causing the executor to retry on the
+// next reconciliation.
+func (c *Client) CleanupAfterDeletion(
+	ctx context.Context,
+	gvk schema.GroupVersionKind,
+	namespace, name string,
+	target transportclient.TransportContext,
+) error {
+	tc, err := resolveTransportContext(target)
+	if err != nil {
+		return err
+	}
+
+	deleteID, err := buildIdentity(tc, desire.TypeDelete, gvk, namespace, name)
+	if err != nil {
+		return err
+	}
+
+	dd, err := c.store.GetDeleteDesire(ctx, deleteID)
+	switch {
+	case errors.Is(err, desire.ErrNotFound):
+		applyID, buildErr := buildIdentity(tc, desire.TypeApply, gvk, namespace, name)
+		if buildErr != nil {
+			return buildErr
+		}
+		_, applyErr := c.store.GetApplyDesire(ctx, applyID)
+		switch {
+		case applyErr == nil:
+			return fmt.Errorf(
+				"desireclient: cleanup: apply desire still exists for %s/%s,"+
+					" resource may not have been created yet: %w",
+				namespace, name, ErrDeletionPending)
+		case !errors.Is(applyErr, desire.ErrNotFound):
+			return fmt.Errorf("desireclient: cleanup: failed to get apply desire for %s/%s: %w",
+				namespace, name, applyErr)
+		}
+	case err != nil:
+		return fmt.Errorf("desireclient: cleanup: failed to get delete desire for %s/%s: %w",
+			namespace, name, err)
+	case !desire.IsDeleted(dd.Status):
+		return fmt.Errorf("desireclient: cleanup: deletion not yet confirmed for %s/%s: %w",
+			namespace, name, ErrDeletionPending)
+	default:
+		if delErr := c.store.DeleteDeleteDesire(ctx, deleteID, c.owner, dd.Version); delErr != nil {
+			return fmt.Errorf("desireclient: cleanup: failed to delete delete desire for %s/%s: %w",
+				namespace, name, delErr)
+		}
+		slog.DebugContext(ctx, "desireclient: cleanup: removed confirmed delete desire",
+			"namespace", namespace, "name", name)
+	}
+
+	readID, err := buildIdentity(tc, desire.TypeRead, gvk, namespace, name)
+	if err != nil {
+		return err
+	}
+
+	rd, err := c.store.GetReadDesire(ctx, readID)
+	switch {
+	case errors.Is(err, desire.ErrNotFound):
+		return nil
+	case err != nil:
+		return fmt.Errorf("desireclient: cleanup: failed to get read desire for %s/%s: %w",
+			namespace, name, err)
+	default:
+		if delErr := c.store.DeleteReadDesire(ctx, readID, c.owner, rd.Version); delErr != nil {
+			return fmt.Errorf("desireclient: cleanup: failed to delete read desire for %s/%s: %w",
+				namespace, name, delErr)
+		}
+		slog.DebugContext(ctx, "desireclient: cleanup: removed read desire",
+			"namespace", namespace, "name", name)
+	}
+
+	return nil
+}

internal/desireclient/cleanup_test.go

@@ -0,0 +1,269 @@
+package desireclient
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/openshift-hyperfleet/hyperfleet-adapter/internal/desireclient/desiretest"
+	"github.com/openshift-hyperfleet/hyperfleet-applier/pkg/desire"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestCleanupAfterDeletion_ConfirmedDelete_RemovesBoth(t *testing.T) {
+	ctx := context.Background()
+	store := newMemoryStore()
+	c := newTestClient(store)
+
+	deleteID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeDelete,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+	readID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeRead,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+
+	desiretest.PutConfirmedDeleteDesire(t, ctx, store, testID.Delete(), testOwner)
+
+	_, err := store.CreateReadDesire(ctx, desire.ReadDesire{
+		Identity: readID, Owner: testOwner, TargetVersion: "v1",
+	})
+	require.NoError(t, err)
+
+	err = c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.NoError(t, err)
+
+	_, err = store.GetDeleteDesire(ctx, deleteID)
+	assert.True(t, errors.Is(err, desire.ErrNotFound), "delete desire must be removed")
+
+	_, err = store.GetReadDesire(ctx, readID)
+	assert.True(t, errors.Is(err, desire.ErrNotFound), "read desire must be removed")
+}
+
+func TestCleanupAfterDeletion_PendingDelete_SkipsCleanup(t *testing.T) {
+	ctx := context.Background()
+	store := newMemoryStore()
+	c := newTestClient(store)
+
+	deleteID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeDelete,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+	readID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeRead,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+
+	desiretest.PutDeleteDesire(t, ctx, store, testID.Delete(), testOwner,
+		metav1.ConditionFalse, desire.ReasonWaitingForDeletion)
+
+	_, err := store.CreateReadDesire(ctx, desire.ReadDesire{
+		Identity: readID, Owner: testOwner, TargetVersion: "v1",
+	})
+	require.NoError(t, err)
+
+	err = c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.Error(t, err, "pending delete desire must return an error")
+	assert.Contains(t, err.Error(), "deletion not yet confirmed")
+	assert.True(t, errors.Is(err, ErrDeletionPending), "must wrap ErrDeletionPending")
+
+	_, err = store.GetDeleteDesire(ctx, deleteID)
+	assert.NoError(t, err, "delete desire must still exist")
+
+	_, err = store.GetReadDesire(ctx, readID)
+	assert.NoError(t, err, "read desire must still exist")
+}
+
+func TestCleanupAfterDeletion_NoDeleteDesire_RemovesReadDesire(t *testing.T) {
+	ctx := context.Background()
+	store := newMemoryStore()
+	c := newTestClient(store)
+
+	readID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeRead,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+	_, err := store.CreateReadDesire(ctx, desire.ReadDesire{
+		Identity: readID, Owner: testOwner, TargetVersion: "v1",
+	})
+	require.NoError(t, err)
+
+	err = c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.NoError(t, err)
+
+	_, err = store.GetReadDesire(ctx, readID)
+	assert.True(t, errors.Is(err, desire.ErrNotFound), "read desire must be removed")
+}
+
+func TestCleanupAfterDeletion_NoDesires_NoError(t *testing.T) {
+	ctx := context.Background()
+	store := newMemoryStore()
+	c := newTestClient(store)
+
+	err := c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.NoError(t, err)
+}
+
+func TestCleanupAfterDeletion_ApplyDesireExists_NoDeleteDesire_ReturnsError(t *testing.T) {
+	ctx := context.Background()
+	store := newMemoryStore()
+	c := newTestClient(store)
+
+	applyID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeApply,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+	_, err := store.CreateApplyDesire(ctx, desire.ApplyDesire{
+		Identity: applyID, Owner: testOwner,
+		Spec: desire.ApplySpec{KubeContent: configMapManifest(1)},
+	})
+	require.NoError(t, err)
+
+	readID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeRead,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+	_, err = store.CreateReadDesire(ctx, desire.ReadDesire{
+		Identity: readID, Owner: testOwner, TargetVersion: "v1",
+	})
+	require.NoError(t, err)
+
+	err = c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "apply desire still exists")
+	assert.True(t, errors.Is(err, ErrDeletionPending), "must wrap ErrDeletionPending")
+
+	_, err = store.GetApplyDesire(ctx, applyID)
+	assert.NoError(t, err, "apply desire must still exist")
+
+	_, err = store.GetReadDesire(ctx, readID)
+	assert.NoError(t, err, "read desire must still exist")
+}
+
+func TestCleanupAfterDeletion_DeleteDesireOnly_NoReadDesire(t *testing.T) {
+	ctx := context.Background()
+	store := newMemoryStore()
+	c := newTestClient(store)
+
+	deleteID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeDelete,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+
+	desiretest.PutConfirmedDeleteDesire(t, ctx, store, testID.Delete(), testOwner)
+
+	err := c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.NoError(t, err)
+
+	_, err = store.GetDeleteDesire(ctx, deleteID)
+	assert.True(t, errors.Is(err, desire.ErrNotFound), "delete desire must be removed")
+}
+
+func TestCleanupAfterDeletion_RequiresTransportContext(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(newMemoryStore())
+
+	err := c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, nil)
+	require.Error(t, err)
+}
+
+func TestCleanupAfterDeletion_GetDeleteDesireError(t *testing.T) {
+	ctx := context.Background()
+	store := &failingGetDeleteDesireStore{SpecStore: newMemoryStore()}
+	c := newTestClient(store)
+
+	err := c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to get delete desire")
+}
+
+func TestCleanupAfterDeletion_DeleteDeleteDesireError(t *testing.T) {
+	ctx := context.Background()
+	inner := newMemoryStore()
+
+	desiretest.PutConfirmedDeleteDesire(t, ctx, inner, testID.Delete(), testOwner)
+
+	store := &failingDeleteDeleteDesireStore{SpecStore: inner}
+	c := newTestClient(store)
+
+	err := c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to delete delete desire")
+}
+
+func TestCleanupAfterDeletion_DeleteReadDesireError(t *testing.T) {
+	ctx := context.Background()
+	inner := newMemoryStore()
+
+	readID := desire.Identity{
+		ManagementCluster: testManagementCluster, Type: desire.TypeRead,
+		Resource: testResource, Namespace: testNamespace, Name: testName,
+	}
+	_, err := inner.CreateReadDesire(ctx, desire.ReadDesire{
+		Identity: readID, Owner: testOwner, TargetVersion: "v1",
+	})
+	require.NoError(t, err)
+
+	store := &failingDeleteReadDesireStore{SpecStore: inner}
+	c := newTestClient(store)
+
+	err = c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to delete read desire")
+}
+
+func TestCleanupAfterDeletion_GetApplyDesireError_NoDeleteDesire_ReturnsStoreError(t *testing.T) {
+	ctx := context.Background()
+	store := &failingGetApplyDesireStore{SpecStore: newMemoryStore()}
+	c := newTestClient(store)
+
+	err := c.CleanupAfterDeletion(ctx, testGVK(), testNamespace, testName, testTransportContext())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "failed to get apply desire")
+	assert.False(t, errors.Is(err, ErrDeletionPending), "genuine store error must not be wrapped as ErrDeletionPending")
+}
+
+// --- Test store wrappers ---
+
+type failingGetDeleteDesireStore struct {
+	desire.SpecStore
+}
+
+func (f *failingGetDeleteDesireStore) GetDeleteDesire(
+	_ context.Context, _ desire.Identity,
+) (desire.DeleteDesire, error) {
+	return desire.DeleteDesire{}, errors.New("boom: store unavailable")
+}
+
+type failingGetApplyDesireStore struct {
+	desire.SpecStore
+}
+
+func (f *failingGetApplyDesireStore) GetApplyDesire(
+	_ context.Context, _ desire.Identity,
+) (desire.ApplyDesire, error) {
+	return desire.ApplyDesire{}, errors.New("boom: store unavailable")
+}
+
+type failingDeleteDeleteDesireStore struct {
+	desire.SpecStore
+}
+
+func (f *failingDeleteDeleteDesireStore) DeleteDeleteDesire(
+	_ context.Context, _ desire.Identity, _ string, _ int64,
+) error {
+	return errors.New("boom: version conflict")
+}
+
+type failingDeleteReadDesireStore struct {
+	desire.SpecStore
+}
+
+func (f *failingDeleteReadDesireStore) DeleteReadDesire(
+	_ context.Context, _ desire.Identity, _ string, _ int64,
+) error {
+	return errors.New("boom: version conflict")
+}

internal/desireclient/client.go

@@ -26,4 +26,7 @@ func NewClient(store desire.SpecStore, owner string) *Client {
 	return &Client{store: store, owner: owner}
 }
 
-var _ transportclient.TransportClient = (*Client)(nil)
+var (
+	_ transportclient.TransportClient = (*Client)(nil)
+	_ transportclient.DesireCleaner   = (*Client)(nil)
+)