From 4eec036d2b11a6326fdab89045f8dc7e2d6f8d5d Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Thu, 28 May 2026 16:18:05 +0200 Subject: [PATCH] StoragePool: during deletion get if the pool exists and requeue --- api/v1alpha1/pool_types.go | 4 + .../bases/storage.simplyblock.io_pools.yaml | 18 +++- .../controller/simplyblockpool_controller.go | 24 ++++- .../simplyblockpool_controller_unit_test.go | 100 ++++++++++++++++-- 4 files changed, 134 insertions(+), 12 deletions(-) diff --git a/api/v1alpha1/pool_types.go b/api/v1alpha1/pool_types.go index 0e907300..2860d53d 100644 --- a/api/v1alpha1/pool_types.go +++ b/api/v1alpha1/pool_types.go @@ -167,6 +167,10 @@ type PoolStatus struct { // +kubebuilder:object:root=true // +kubebuilder:subresource:status +// +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.status",description="Backend pool status" +// +kubebuilder:printcolumn:name="UUID",type="string",JSONPath=".status.uuid",description="Backend pool UUID" +// +kubebuilder:printcolumn:name="Capacity",type="string",JSONPath=".spec.capacityLimit",description="Configured capacity limit" +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" // +operator-sdk:csv:customresourcedefinitions:displayName="Pool" // Pool is the Schema for the pools API diff --git a/config/crd/bases/storage.simplyblock.io_pools.yaml b/config/crd/bases/storage.simplyblock.io_pools.yaml index c1bbeab8..cffbf708 100644 --- a/config/crd/bases/storage.simplyblock.io_pools.yaml +++ b/config/crd/bases/storage.simplyblock.io_pools.yaml @@ -14,7 +14,23 @@ spec: singular: pool scope: Namespaced versions: - - name: v1alpha1 + - additionalPrinterColumns: + - description: Backend pool status + jsonPath: .status.status + name: Status + type: string + - description: Backend pool UUID + jsonPath: .status.uuid + name: UUID + type: string + - description: Configured capacity limit + jsonPath: .spec.capacityLimit + name: Capacity + type: string + - jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 schema: openAPIV3Schema: description: Pool is the Schema for the pools API diff --git a/internal/controller/simplyblockpool_controller.go b/internal/controller/simplyblockpool_controller.go index faa83c8d..ffbcf1b4 100644 --- a/internal/controller/simplyblockpool_controller.go +++ b/internal/controller/simplyblockpool_controller.go @@ -107,7 +107,29 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. apiClient := webapi.NewClient() if !poolCR.DeletionTimestamp.IsZero() { - if utils.ContainsString(poolCR.Finalizers, utils.FinalizerPool) && poolCR.Status.UUID != "" { + if utils.ContainsString(poolCR.Finalizers, utils.FinalizerPool) { + if poolCR.Status.UUID == "" { + // Pool was never successfully created (or status update failed after creation). + // Check the backend to be safe before removing the finalizer. + if existing, lookupErr := utils.GetPoolByName(ctx, apiClient, clusterSecret, clusterUUID, poolCR.Name); lookupErr == nil && existing != nil { + log.Info("Pool exists on backend but UUID missing in status, adopting for deletion", "name", poolCR.Name, "uuid", existing.UUID) + poolCR.Status.UUID = existing.UUID + if err := r.Status().Update(ctx, poolCR); err != nil { + log.Error(err, "Failed to update pool status with adopted UUID") + return ctrl.Result{RequeueAfter: 10 * time.Second}, nil + } + return ctrl.Result{Requeue: true}, nil + } + // Pool does not exist on backend — safe to remove finalizer. + log.Info("Pool was never created on backend, removing finalizer", "name", poolCR.Name) + poolCR.Finalizers = utils.RemoveString(poolCR.Finalizers, utils.FinalizerPool) + if err := r.Update(ctx, poolCR); err != nil { + log.Error(err, "Failed to remove finalizer") + return ctrl.Result{RequeueAfter: 10 * time.Second}, nil + } + return ctrl.Result{}, nil + } + endpoint := fmt.Sprintf("/api/v2/clusters/%s/storage-pools/%s", clusterUUID, poolCR.Status.UUID) body, status, err := apiClient.Do(ctx, clusterSecret, http.MethodDelete, endpoint, nil) if err != nil || status >= 300 { diff --git a/internal/controller/simplyblockpool_controller_unit_test.go b/internal/controller/simplyblockpool_controller_unit_test.go index e6cc769a..b76122eb 100644 --- a/internal/controller/simplyblockpool_controller_unit_test.go +++ b/internal/controller/simplyblockpool_controller_unit_test.go @@ -49,7 +49,27 @@ func TestPoolReconcileAddsFinalizer(t *testing.T) { } } -func TestPoolReconcileDeletionWithoutUUIDDoesNotProgress(t *testing.T) { +func TestPoolReconcileDeletionWithoutUUIDRemovesFinalizer(t *testing.T) { + const clusterUUID = "cluster-uuid-del-no-uuid" + + mock := webapimock.NewSpecServerFromFile(t, "../../openapi.json", false) + defer mock.Close() + + // Pool does not exist on backend — GET returns empty list. + mock.Register( + http.MethodGet, + "/api/v2/clusters/"+clusterUUID+"/storage-pools/", + webapimock.RouteResponse{ + Status: http.StatusOK, + Body: `[]`, + Headers: map[string]string{ + "Content-Type": "application/json", + }, + }, + ) + + t.Setenv("SIMPLYBLOCK_WEBAPI_BASE_URL", mock.URL()) + pool := &simplyblockv1alpha1.Pool{ ObjectMeta: metav1.ObjectMeta{ Name: "pool-b", @@ -63,8 +83,8 @@ func TestPoolReconcileDeletionWithoutUUIDDoesNotProgress(t *testing.T) { r := newPoolStateTestReconciler(t, pool, - testCluster("default", "cluster-a", "cluster-uuid"), - testClusterSecret("default", "cluster-a", "cluster-uuid", "secret"), + testCluster("default", "cluster-a", clusterUUID), + testClusterSecret("default", "cluster-a", clusterUUID, "secret"), ) if err := r.Delete(context.Background(), pool); err != nil { t.Fatalf("failed to trigger deletion: %v", err) @@ -78,12 +98,72 @@ func TestPoolReconcileDeletionWithoutUUIDDoesNotProgress(t *testing.T) { current := &simplyblockv1alpha1.Pool{} if err := r.Get(context.Background(), client.ObjectKeyFromObject(pool), current); err != nil { if apierrors.IsNotFound(err) { - return + return // object fully gone — finalizer removed successfully } t.Fatalf("failed to get pool: %v", err) } - if !contains(current.Finalizers, utils.FinalizerPool) { - t.Fatalf("expected finalizer to remain because deletion requires status.uuid") + if contains(current.Finalizers, utils.FinalizerPool) { + t.Fatalf("expected finalizer to be removed when pool was never created on backend") + } +} + +func TestPoolReconcileDeletionWithoutUUIDAdoptsExistingPool(t *testing.T) { + const clusterUUID = "cluster-uuid-del-adopt" + const poolUUID = "pool-uuid-adopt" + + mock := webapimock.NewSpecServerFromFile(t, "../../openapi.json", false) + defer mock.Close() + + // Pool exists on backend even though status.UUID is empty. + mock.Register( + http.MethodGet, + "/api/v2/clusters/"+clusterUUID+"/storage-pools/", + webapimock.RouteResponse{ + Status: http.StatusOK, + Body: `[{"id":"` + poolUUID + `","name":"pool-adopt","status":"active"}]`, + Headers: map[string]string{ + "Content-Type": "application/json", + }, + }, + ) + + t.Setenv("SIMPLYBLOCK_WEBAPI_BASE_URL", mock.URL()) + + pool := &simplyblockv1alpha1.Pool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pool-adopt", + Namespace: "default", + Finalizers: []string{utils.FinalizerPool}, + }, + Spec: simplyblockv1alpha1.PoolSpec{ + ClusterName: "cluster-a", + }, + // Status.UUID intentionally empty — simulates failed status update after creation. + } + + r := newPoolStateTestReconciler(t, + pool, + testCluster("default", "cluster-a", clusterUUID), + testClusterSecret("default", "cluster-a", clusterUUID, "secret"), + ) + if err := r.Delete(context.Background(), pool); err != nil { + t.Fatalf("failed to trigger deletion: %v", err) + } + + res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(pool)}) + if err != nil { + t.Fatalf("reconcile returned error: %v", err) + } + if !res.Requeue { + t.Fatalf("expected immediate requeue after adopting UUID for deletion, got %+v", res) + } + + current := &simplyblockv1alpha1.Pool{} + if err := r.Get(context.Background(), client.ObjectKeyFromObject(pool), current); err != nil { + t.Fatalf("failed to get pool: %v", err) + } + if current.Status.UUID != poolUUID { + t.Fatalf("expected adopted UUID %q in status, got %q", poolUUID, current.Status.UUID) } } @@ -101,7 +181,7 @@ func TestPoolReconcilePreventsStatusRegressionWhenClusterMissing(t *testing.T) { }, Status: simplyblockv1alpha1.PoolStatus{ UUID: "pool-uuid", - Status: "online", + Status: "active", }, } @@ -239,7 +319,7 @@ func TestPoolReconcileStorageClassNameIncludesNamespace(t *testing.T) { } func TestPoolReconcileCreatesPoolViaOpenAPIMock(t *testing.T) { - const statusOnline = "online" + const statusActive = "active" const clusterUUID = "cluster-uuid-pool-create" mock := webapimock.NewSpecServerFromFile(t, "../../openapi.json", false) @@ -252,7 +332,7 @@ func TestPoolReconcileCreatesPoolViaOpenAPIMock(t *testing.T) { Status: http.StatusOK, Body: `{ "uuid":"pool-created", - "status":"online", + "status":"active", "max_rw_ios_per_sec":100, "max_rw_mbytes_per_sec":200, "max_r_mbytes_per_sec":50, @@ -297,7 +377,7 @@ func TestPoolReconcileCreatesPoolViaOpenAPIMock(t *testing.T) { if err := r.Get(context.Background(), client.ObjectKeyFromObject(pool), current); err != nil { t.Fatalf("failed to get pool: %v", err) } - if current.Status.UUID != "pool-created" || current.Status.Status != statusOnline { + if current.Status.UUID != "pool-created" || current.Status.Status != statusActive { t.Fatalf("unexpected status after mocked pool create: %#v", current.Status) } reqs := mock.Requests()