From a7969d3346bd394aca8862d92f48f63059f47d7e Mon Sep 17 00:00:00 2001 From: Martin Liptak Date: Thu, 30 Apr 2026 14:54:01 +0200 Subject: [PATCH 1/2] HYPERFLEET-854 - feat: implement hard deletion for clusters and nodepools --- CHANGELOG.md | 3 + pkg/api/adapter_status_types.go | 14 ++ pkg/dao/adapter_status.go | 94 ++-------- pkg/dao/cluster.go | 21 +++ pkg/dao/mocks/cluster.go | 14 ++ pkg/dao/mocks/node_pool.go | 27 +++ pkg/dao/node_pool.go | 41 +++++ pkg/services/adapter_status.go | 63 ++++++- pkg/services/aggregation.go | 46 ++++- pkg/services/aggregation_test.go | 61 +++++-- pkg/services/cluster.go | 236 ++++++++++++++++++------ pkg/services/cluster_test.go | 158 +++++++++++++++- pkg/services/node_pool.go | 182 +++++++++++++++---- pkg/services/node_pool_test.go | 35 +++- pkg/services/status_helpers.go | 10 +- test/integration/clusters_test.go | 272 ++++++++++++++++++++++++++++ test/integration/node_pools_test.go | 235 ++++++++++++++++++++++++ 17 files changed, 1302 insertions(+), 210 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd85deb1..64124af7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Hard deletion for Clusters and NodePools: resources and their adapter statuses are permanently removed from the database once all required adapters report `Finalized=True` and no child resources remain ([#HYPERFLEET-854](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/HYPERFLEET-854)) +- `Finalized` condition aggregation with `WaitingForChildResources` intermediate state when all adapters are finalized but child node pools still exist ([#HYPERFLEET-854](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/HYPERFLEET-854)) - Soft deletion for Clusters and NodePools with `deleted_time` and `deleted_by` fields for tracking deletion requests ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106)) - Aggregation logic for resource data ([#91](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/91)) - Version subcommand to CLI ([#84](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/84)) @@ -26,6 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Refactored `AdapterStatusDao.Upsert()` to accept a pre-fetched existing record, moving lookup and `LastTransitionTime` preservation logic to the service layer ([#HYPERFLEET-854](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/HYPERFLEET-854)) - Refactored DAO methods to remove Unscoped calls for fetching Clusters and NodePools ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106)) - Bumped oapi-codegen version to fix missing `omitempty` on generated response objects ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106)) - Updated OpenAPI spec with examples for Cluster and NodePool schemas ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106)) diff --git a/pkg/api/adapter_status_types.go b/pkg/api/adapter_status_types.go index a0646c05..d9790356 100644 --- a/pkg/api/adapter_status_types.go +++ b/pkg/api/adapter_status_types.go @@ -1,6 +1,7 @@ package api import ( + "encoding/json" "fmt" "time" @@ -52,3 +53,16 @@ func (as *AdapterStatus) BeforeCreate(tx *gorm.DB) error { as.ID = id return nil } + +func (as *AdapterStatus) IsFinalized() bool { + var conditions []AdapterCondition + if err := json.Unmarshal(as.Conditions, &conditions); err != nil { + return false + } + for _, cond := range conditions { + if cond.Type == ConditionTypeFinalized && cond.Status == AdapterConditionTrue { + return true + } + } + return false +} diff --git a/pkg/dao/adapter_status.go b/pkg/dao/adapter_status.go index f4adc87b..727f1272 100644 --- a/pkg/dao/adapter_status.go +++ b/pkg/dao/adapter_status.go @@ -2,16 +2,10 @@ package dao import ( "context" - "encoding/json" - "errors" - "time" - "gorm.io/datatypes" - "gorm.io/gorm" "gorm.io/gorm/clause" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" ) @@ -19,8 +13,9 @@ type AdapterStatusDao interface { Get(ctx context.Context, id string) (*api.AdapterStatus, error) Create(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) Replace(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) - Upsert(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) + Upsert(ctx context.Context, adapterStatus *api.AdapterStatus, existing *api.AdapterStatus) (*api.AdapterStatus, error) Delete(ctx context.Context, id string) error + DeleteByResource(ctx context.Context, resourceType, resourceID string) error FindByResource(ctx context.Context, resourceType, resourceID string) (api.AdapterStatusList, error) FindByResourceIDs(ctx context.Context, resourceType string, resourceIDs []string) (api.AdapterStatusList, error) FindByResourcePaginated( @@ -73,30 +68,12 @@ func (d *sqlAdapterStatusDao) Replace( return adapterStatus, nil } -// Upsert creates or updates an adapter status based on resource_type, resource_id, and adapter -// This implements the upsert semantic required by the new API spec func (d *sqlAdapterStatusDao) Upsert( - ctx context.Context, adapterStatus *api.AdapterStatus, + ctx context.Context, adapterStatus *api.AdapterStatus, existing *api.AdapterStatus, ) (*api.AdapterStatus, error) { g2 := (*d.sessionFactory).New(ctx) - // Keep deterministic observed time from the incoming report when provided (observed_time). - if adapterStatus.LastReportTime.IsZero() { - adapterStatus.LastReportTime = time.Now() - } - - existing, err := d.FindByResourceAndAdapter( - ctx, adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter, - ) - if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { - db.MarkForRollback(ctx, err) - return nil, err - } - - if err == nil && existing != nil { - // Preserve LastTransitionTime for conditions whose status hasn't changed. - adapterStatus.Conditions = preserveLastTransitionTime(existing.Conditions, adapterStatus.Conditions) - + if existing != nil { updateResult := g2.Model(&api.AdapterStatus{}). Where("resource_type = ? AND resource_id = ? AND adapter = ?", adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter). @@ -118,9 +95,8 @@ func (d *sqlAdapterStatusDao) Upsert( return nil, updateResult.Error } - // No-op when the stored row is fresher or equal. if updateResult.RowsAffected == 0 { - return d.FindByResourceAndAdapter(ctx, adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter) + return existing, nil } return d.FindByResourceAndAdapter(ctx, adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter) @@ -135,7 +111,6 @@ func (d *sqlAdapterStatusDao) Upsert( return adapterStatus, nil } - // A row was inserted concurrently; return the latest stored row without overwriting it. return d.FindByResourceAndAdapter(ctx, adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter) } @@ -150,6 +125,16 @@ func (d *sqlAdapterStatusDao) Delete(ctx context.Context, id string) error { return nil } +func (d *sqlAdapterStatusDao) DeleteByResource(ctx context.Context, resourceType, resourceID string) error { + g2 := (*d.sessionFactory).New(ctx) + if err := g2.Where("resource_type = ? AND resource_id = ?", resourceType, resourceID). + Delete(&api.AdapterStatus{}).Error; err != nil { + db.MarkForRollback(ctx, err) + return err + } + return nil +} + func (d *sqlAdapterStatusDao) FindByResource( ctx context.Context, resourceType, resourceID string, ) (api.AdapterStatusList, error) { @@ -220,52 +205,3 @@ func (d *sqlAdapterStatusDao) All(ctx context.Context) (api.AdapterStatusList, e } return statuses, nil } - -// preserveLastTransitionTime preserves LastTransitionTime for conditions whose status hasn't changed -// This implements the Kubernetes condition semantic where LastTransitionTime is only updated when status changes -func preserveLastTransitionTime(oldConditionsJSON, newConditionsJSON datatypes.JSON) datatypes.JSON { - // Unmarshal old conditions - var oldConditions []openapi.AdapterCondition - if len(oldConditionsJSON) > 0 { - if err := json.Unmarshal(oldConditionsJSON, &oldConditions); err != nil { - // If we can't unmarshal old conditions, return new conditions as-is - return newConditionsJSON - } - } - - // Unmarshal new conditions - var newConditions []openapi.AdapterCondition - if len(newConditionsJSON) > 0 { - if err := json.Unmarshal(newConditionsJSON, &newConditions); err != nil { - // If we can't unmarshal new conditions, return new conditions as-is - return newConditionsJSON - } - } - - // Build a map of old conditions by type for quick lookup - oldConditionsMap := make(map[string]openapi.AdapterCondition) - for _, oldCond := range oldConditions { - oldConditionsMap[oldCond.Type] = oldCond - } - - // Update new conditions: preserve LastTransitionTime if status hasn't changed - for i := range newConditions { - if oldCond, exists := oldConditionsMap[newConditions[i].Type]; exists { - // If status hasn't changed, preserve the old LastTransitionTime - if oldCond.Status == newConditions[i].Status { - newConditions[i].LastTransitionTime = oldCond.LastTransitionTime - } - // If status changed, keep the new LastTransitionTime (already set to now) - } - // If this is a new condition type, keep the new LastTransitionTime - } - - // Marshal back to JSON - updatedJSON, err := json.Marshal(newConditions) - if err != nil { - // If we can't marshal, return new conditions as-is - return newConditionsJSON - } - - return updatedJSON -} diff --git a/pkg/dao/cluster.go b/pkg/dao/cluster.go index efe2bd50..9f2c6f0d 100644 --- a/pkg/dao/cluster.go +++ b/pkg/dao/cluster.go @@ -12,9 +12,11 @@ import ( type ClusterDao interface { Get(ctx context.Context, id string) (*api.Cluster, error) + GetForUpdate(ctx context.Context, id string) (*api.Cluster, error) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) Save(ctx context.Context, cluster *api.Cluster) error + SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error Delete(ctx context.Context, id string) error FindByIDs(ctx context.Context, ids []string) (api.ClusterList, error) All(ctx context.Context) (api.ClusterList, error) @@ -39,6 +41,15 @@ func (d *sqlClusterDao) Get(ctx context.Context, id string) (*api.Cluster, error return &cluster, nil } +func (d *sqlClusterDao) GetForUpdate(ctx context.Context, id string) (*api.Cluster, error) { + g2 := (*d.sessionFactory).New(ctx) + var cluster api.Cluster + if err := g2.Clauses(clause.Locking{Strength: "UPDATE"}).Take(&cluster, "id = ?", id).Error; err != nil { + return nil, err + } + return &cluster, nil +} + func (d *sqlClusterDao) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { g2 := (*d.sessionFactory).New(ctx) if err := g2.Omit(clause.Associations).Create(cluster).Error; err != nil { @@ -83,6 +94,16 @@ func (d *sqlClusterDao) Save(ctx context.Context, cluster *api.Cluster) error { return nil } +func (d *sqlClusterDao) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { + g2 := (*d.sessionFactory).New(ctx) + result := g2.Model(&api.Cluster{}).Where("id = ?", id).Update("status_conditions", statusConditions) + if result.Error != nil { + db.MarkForRollback(ctx, result.Error) + return result.Error + } + return nil +} + func (d *sqlClusterDao) Delete(ctx context.Context, id string) error { g2 := (*d.sessionFactory).New(ctx) if err := g2.Omit(clause.Associations).Delete(&api.Cluster{Meta: api.Meta{ID: id}}).Error; err != nil { diff --git a/pkg/dao/mocks/cluster.go b/pkg/dao/mocks/cluster.go index 342ca121..1b02d55c 100644 --- a/pkg/dao/mocks/cluster.go +++ b/pkg/dao/mocks/cluster.go @@ -25,6 +25,10 @@ func (d *clusterDaoMock) Get(ctx context.Context, id string) (*api.Cluster, erro return nil, gorm.ErrRecordNotFound } +func (d *clusterDaoMock) GetForUpdate(ctx context.Context, id string) (*api.Cluster, error) { + return d.Get(ctx, id) +} + func (d *clusterDaoMock) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { d.clusters = append(d.clusters, cluster) return cluster, nil @@ -39,6 +43,16 @@ func (d *clusterDaoMock) Save(ctx context.Context, cluster *api.Cluster) error { return nil } +func (d *clusterDaoMock) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { + for _, c := range d.clusters { + if c.ID == id { + c.StatusConditions = statusConditions + return nil + } + } + return gorm.ErrRecordNotFound +} + func (d *clusterDaoMock) Delete(ctx context.Context, id string) error { return errors.NotImplemented("Cluster").AsError() } diff --git a/pkg/dao/mocks/node_pool.go b/pkg/dao/mocks/node_pool.go index 4229ef08..0c731af7 100644 --- a/pkg/dao/mocks/node_pool.go +++ b/pkg/dao/mocks/node_pool.go @@ -26,6 +26,20 @@ func (d *nodePoolDaoMock) Get(ctx context.Context, id string) (*api.NodePool, er return nil, gorm.ErrRecordNotFound } +func (d *nodePoolDaoMock) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) { + return d.Get(ctx, id) +} + +func (d *nodePoolDaoMock) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { + for _, np := range d.nodePools { + if np.ID == id { + np.StatusConditions = statusConditions + return nil + } + } + return gorm.ErrRecordNotFound +} + func (d *nodePoolDaoMock) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { d.nodePools = append(d.nodePools, nodePool) return nodePool, nil @@ -56,10 +70,23 @@ func (d *nodePoolDaoMock) FindByIDs(ctx context.Context, ids []string) (api.Node return nil, errors.NotImplemented("NodePool").AsError() } +func (d *nodePoolDaoMock) FindByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) { + return nil, errors.NotImplemented("NodePool").AsError() +} + func (d *nodePoolDaoMock) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error { return errors.NotImplemented("NodePool").AsError() } +func (d *nodePoolDaoMock) ExistsByOwner(ctx context.Context, ownerID string) (bool, error) { + for _, np := range d.nodePools { + if np.OwnerID == ownerID { + return true, nil + } + } + return false, nil +} + func (d *nodePoolDaoMock) All(ctx context.Context) (api.NodePoolList, error) { return d.nodePools, nil } diff --git a/pkg/dao/node_pool.go b/pkg/dao/node_pool.go index 362938bc..7a2f1041 100644 --- a/pkg/dao/node_pool.go +++ b/pkg/dao/node_pool.go @@ -14,14 +14,18 @@ import ( type NodePoolDao interface { Get(ctx context.Context, id string) (*api.NodePool, error) + GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) Save(ctx context.Context, nodePool *api.NodePool) error + SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error Delete(ctx context.Context, id string) error FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) + FindByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) SoftDeleteByOwner(ctx context.Context, ownerID string, t time.Time, deletedBy string) error UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error + ExistsByOwner(ctx context.Context, ownerID string) (bool, error) All(ctx context.Context) (api.NodePoolList, error) } @@ -44,6 +48,25 @@ func (d *sqlNodePoolDao) Get(ctx context.Context, id string) (*api.NodePool, err return &nodePool, nil } +func (d *sqlNodePoolDao) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) { + g2 := (*d.sessionFactory).New(ctx) + var nodePool api.NodePool + if err := g2.Clauses(clause.Locking{Strength: "UPDATE"}).Take(&nodePool, "id = ?", id).Error; err != nil { + return nil, err + } + return &nodePool, nil +} + +func (d *sqlNodePoolDao) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { + g2 := (*d.sessionFactory).New(ctx) + result := g2.Model(&api.NodePool{}).Where("id = ?", id).Update("status_conditions", statusConditions) + if result.Error != nil { + db.MarkForRollback(ctx, result.Error) + return result.Error + } + return nil +} + func (d *sqlNodePoolDao) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { g2 := (*d.sessionFactory).New(ctx) if err := g2.Omit(clause.Associations).Create(nodePool).Error; err != nil { @@ -131,6 +154,15 @@ func (d *sqlNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodeP return nodePools, nil } +func (d *sqlNodePoolDao) FindByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) { + g2 := (*d.sessionFactory).New(ctx) + var nodePools api.NodePoolList + if err := g2.Where("owner_id = ?", ownerID).Find(&nodePools).Error; err != nil { + return nil, err + } + return nodePools, nil +} + func (d *sqlNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error { g2 := (*d.sessionFactory).New(ctx) if len(updates) == 0 { @@ -149,6 +181,15 @@ func (d *sqlNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, update return nil } +func (d *sqlNodePoolDao) ExistsByOwner(ctx context.Context, ownerID string) (bool, error) { + g2 := (*d.sessionFactory).New(ctx) + var count int64 + if err := g2.Model(&api.NodePool{}).Where("owner_id = ?", ownerID).Limit(1).Count(&count).Error; err != nil { + return false, err + } + return count > 0, nil +} + func (d *sqlNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) { g2 := (*d.sessionFactory).New(ctx) nodePools := api.NodePoolList{} diff --git a/pkg/services/adapter_status.go b/pkg/services/adapter_status.go index f566909c..84c3e586 100644 --- a/pkg/services/adapter_status.go +++ b/pkg/services/adapter_status.go @@ -2,6 +2,8 @@ package services import ( "context" + "encoding/json" + "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" @@ -68,16 +70,6 @@ func (s *sqlAdapterStatusService) Replace( return adapterStatus, nil } -func (s *sqlAdapterStatusService) Upsert( - ctx context.Context, adapterStatus *api.AdapterStatus, -) (*api.AdapterStatus, *errors.ServiceError) { - adapterStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) - if err != nil { - return nil, handleCreateError("AdapterStatus", err) - } - return adapterStatus, nil -} - func (s *sqlAdapterStatusService) Delete(ctx context.Context, id string) *errors.ServiceError { if err := s.adapterStatusDao.Delete(ctx, id); err != nil { return handleDeleteError("AdapterStatus", errors.GeneralError("Unable to delete adapter status: %s", err)) @@ -126,3 +118,54 @@ func (s *sqlAdapterStatusService) All(ctx context.Context) (api.AdapterStatusLis } return statuses, nil } + +func (s *sqlAdapterStatusService) Upsert( + ctx context.Context, adapterStatus *api.AdapterStatus, +) (*api.AdapterStatus, *errors.ServiceError) { + existing, findErr := s.adapterStatusDao.FindByResourceAndAdapter( + ctx, adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter, + ) + if findErr != nil { + existing = nil + } + setConditionTransitionTimes(adapterStatus, existing) + result, err := s.adapterStatusDao.Upsert(ctx, adapterStatus, existing) + if err != nil { + return nil, handleCreateError("AdapterStatus", err) + } + return result, nil +} + +// setConditionTransitionTimes sets LastReportTime if unset and preserves condition +// LastTransitionTime for any condition whose status hasn't changed since the last report +// (Kubernetes condition semantic: LastTransitionTime only updates on status change). +func setConditionTransitionTimes(incoming *api.AdapterStatus, existing *api.AdapterStatus) { + if incoming.LastReportTime.IsZero() { + incoming.LastReportTime = time.Now() + } + if existing == nil || len(existing.Conditions) == 0 { + return + } + + var oldConds []api.AdapterCondition + if err := json.Unmarshal(existing.Conditions, &oldConds); err != nil { + return + } + var newConds []api.AdapterCondition + if len(incoming.Conditions) == 0 || json.Unmarshal(incoming.Conditions, &newConds) != nil { + return + } + + oldByType := make(map[string]api.AdapterCondition, len(oldConds)) + for _, c := range oldConds { + oldByType[c.Type] = c + } + for i := range newConds { + if old, ok := oldByType[newConds[i].Type]; ok && old.Status == newConds[i].Status { + newConds[i].LastTransitionTime = old.LastTransitionTime + } + } + if b, err := json.Marshal(newConds); err == nil { + incoming.Conditions = b + } +} diff --git a/pkg/services/aggregation.go b/pkg/services/aggregation.go index 07c4cd45..67c6ac26 100644 --- a/pkg/services/aggregation.go +++ b/pkg/services/aggregation.go @@ -24,11 +24,16 @@ const ( ConditionValidationErrorMissing = "missing" ) +// fixedConditionCount is the number of top-level conditions always present in resource status: +// Ready (deprecated), Reconciled, and Available. +const fixedConditionCount = 3 + // reasonMissingRequiredAdapters is the reason code for the Ready condition when one or more // required adapters have not yet reported Available=True at the current resource generation. const reasonMissingRequiredAdapters = "MissingRequiredAdapters" const reasonAllAdaptersReconciled = "All required adapters reported Available=True or Finalized=True " + "at the current generation" +const reasonWaitingForChildResources = "WaitingForChildResources" // ValidateMandatoryConditions checks if all mandatory conditions are present. // Format validation (empty type, duplicates, invalid status) is done in the Handler layer. @@ -91,6 +96,7 @@ type AggregateResourceStatusInput struct { PrevConditionsJSON []byte RequiredAdapters []string AdapterStatuses api.AdapterStatusList + HasChildResources bool } // AdapterObservedTime returns the adapter-reported observation instant used for ordering and aggregation. @@ -101,7 +107,7 @@ func AdapterObservedTime(as *api.AdapterStatus) time.Time { return as.LastReportTime } -// AggregateResourceStatus computes Ready, Available, and per-adapter conditions from stored adapter +// AggregateResourceStatus computes Reconciled, Available, and per-adapter conditions from stored adapter // rows and previous conditions. It does not use wall clock. // // The returned adapterConditions slice contains one entry per required adapter that has reported, @@ -120,6 +126,7 @@ func AggregateResourceStatus(ctx context.Context, in AggregateResourceStatusInpu prevReconciled, in.RequiredAdapters, reports, + in.HasChildResources, ) available = computeAvailable( in.RefTime, @@ -303,6 +310,7 @@ func computeReconciled( prev *api.ResourceCondition, required []string, byAdapter map[string]adapterAvailableSnapshot, + hasChildResources bool, ) api.ResourceCondition { allAtCurrent := true for _, name := range required { @@ -323,8 +331,12 @@ func computeReconciled( } } + // During deletion: even when all adapters have finalized, keep Reconciled=False + // until all child resources (node pools) have also been removed from the database. + allFinalizedButChildrenExist := deletedTime != nil && allAtCurrent && hasChildResources + status := api.ConditionFalse - if len(required) > 0 && allAtCurrent { + if len(required) > 0 && allAtCurrent && !allFinalizedButChildrenExist { status = api.ConditionTrue } @@ -342,11 +354,16 @@ func computeReconciled( var reason, message string if deletedTime != nil { - reason = reasonMissingRequiredAdapters - message = buildFinalizedFalseMessage(required, byAdapter, resourceGen) - if status == api.ConditionTrue { + switch { + case status == api.ConditionTrue: reason = reasonAllAdaptersReconciled message = reason + case allFinalizedButChildrenExist: + reason = reasonWaitingForChildResources + message = "Deletion in progress. All required adapters reported Finalized=True but child resources still exist" + default: + reason = reasonMissingRequiredAdapters + message = buildFinalizedFalseMessage(required, byAdapter, resourceGen) } } else { reason = reasonMissingRequiredAdapters @@ -717,3 +734,22 @@ func minTime(times []time.Time) time.Time { func strPtr(s string) *string { return &s } + +// allAdaptersFinalized checks if all required adapters have Finalized=True in their adapter_status conditions. +// Finalized is optional — if the condition is absent, it's treated as not finalized (same as False). +func allAdaptersFinalized(requiredAdapters []string, adapterStatuses api.AdapterStatusList) bool { + finalizedAdapters := make(map[string]struct{}) + + for _, adapterStatus := range adapterStatuses { + if adapterStatus.IsFinalized() { + finalizedAdapters[adapterStatus.Adapter] = struct{}{} + } + } + + for _, requiredAdapter := range requiredAdapters { + if _, exists := finalizedAdapters[requiredAdapter]; !exists { + return false + } + } + return true +} diff --git a/pkg/services/aggregation_test.go b/pkg/services/aggregation_test.go index 4b66c31d..64b9ece9 100644 --- a/pkg/services/aggregation_test.go +++ b/pkg/services/aggregation_test.go @@ -510,7 +510,7 @@ func TestComputeReconciled(t *testing.T) { t.Run("Type is Reconciled", func(t *testing.T) { t.Parallel() - cond := computeReconciled(1, aggTRef, nil, nil, nil, map[string]adapterAvailableSnapshot{}) + cond := computeReconciled(1, aggTRef, nil, nil, nil, map[string]adapterAvailableSnapshot{}, false) if cond.Type != api.ConditionTypeReconciled { t.Errorf("type got %v, want Reconciled", cond.Type) } @@ -518,7 +518,7 @@ func TestComputeReconciled(t *testing.T) { t.Run("empty required list → False", func(t *testing.T) { t.Parallel() - cond := computeReconciled(1, aggTRef, nil, nil, nil, map[string]adapterAvailableSnapshot{}) + cond := computeReconciled(1, aggTRef, nil, nil, nil, map[string]adapterAvailableSnapshot{}, false) if cond.Status != api.ConditionFalse { t.Errorf("got %v, want False", cond.Status) } @@ -531,7 +531,7 @@ func TestComputeReconciled(t *testing.T) { "a": snap(2, true, aggT1), "b": snap(2, true, aggT2), } - cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter, false) if cond.Status != api.ConditionTrue { t.Errorf("got %v, want True", cond.Status) } @@ -544,7 +544,7 @@ func TestComputeReconciled(t *testing.T) { "a": snap(2, true, aggT1), "b": snap(1, true, aggT2), } - cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter, false) if cond.Status != api.ConditionFalse { t.Errorf("got %v, want False", cond.Status) } @@ -556,7 +556,7 @@ func TestComputeReconciled(t *testing.T) { byAdapter := map[string]adapterAvailableSnapshot{ "a": snap(2, false, aggT1), } - cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter, false) if cond.Status != api.ConditionFalse { t.Errorf("got %v, want False", cond.Status) } @@ -564,7 +564,7 @@ func TestComputeReconciled(t *testing.T) { t.Run("ObservedGeneration always equals resourceGen", func(t *testing.T) { t.Parallel() - cond := computeReconciled(5, aggTRef, nil, nil, nil, map[string]adapterAvailableSnapshot{}) + cond := computeReconciled(5, aggTRef, nil, nil, nil, map[string]adapterAvailableSnapshot{}, false) if cond.ObservedGeneration != 5 { t.Errorf("ObservedGeneration got %d, want 5", cond.ObservedGeneration) } @@ -575,7 +575,7 @@ func TestComputeReconciled(t *testing.T) { required := []string{"a"} byAdapter := map[string]adapterAvailableSnapshot{"a": snap(2, true, aggT1)} prev := mkPrevReconciled(api.ConditionTrue, 1, aggT0, aggT0) - cond := computeReconciled(2, aggTRef, nil, prev, required, byAdapter) + cond := computeReconciled(2, aggTRef, nil, prev, required, byAdapter, false) if !cond.CreatedTime.Equal(aggT0) { t.Errorf("CreatedTime got %v, want prev.CreatedTime=%v", cond.CreatedTime, aggT0) } @@ -588,7 +588,7 @@ func TestComputeReconciled(t *testing.T) { "a": {observedGeneration: 2, availableTrue: true, finalizedTrue: false, observedTime: aggT1}, "b": {observedGeneration: 2, availableTrue: true, finalizedTrue: false, observedTime: aggT2}, } - cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter, false) if cond.Status != api.ConditionFalse { t.Errorf("got %v, want False", cond.Status) } @@ -601,7 +601,7 @@ func TestComputeReconciled(t *testing.T) { "a": {observedGeneration: 2, availableTrue: false, finalizedTrue: true, observedTime: aggT1}, "b": {observedGeneration: 2, availableTrue: false, finalizedTrue: true, observedTime: aggT2}, } - cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter, false) if cond.Status != api.ConditionTrue { t.Errorf("got %v, want True", cond.Status) } @@ -614,7 +614,7 @@ func TestComputeReconciled(t *testing.T) { "a": {observedGeneration: 2, availableTrue: false, finalizedTrue: true, observedTime: aggT1}, "b": {observedGeneration: 2, availableTrue: false, finalizedTrue: false, observedTime: aggT2}, } - cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter, false) if cond.Status != api.ConditionFalse { t.Errorf("got %v, want False", cond.Status) } @@ -626,7 +626,7 @@ func TestComputeReconciled(t *testing.T) { byAdapter := map[string]adapterAvailableSnapshot{ "a": {observedGeneration: 1, availableTrue: false, finalizedTrue: true, observedTime: aggT1}, } - cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter, false) if cond.Status != api.ConditionFalse { t.Errorf("got %v, want False (old gen)", cond.Status) } @@ -639,7 +639,7 @@ func TestComputeReconciled(t *testing.T) { byAdapter := map[string]adapterAvailableSnapshot{ "a": {observedGeneration: 2, availableTrue: true, finalizedTrue: false, observedTime: aggT1}, } - cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter, false) if cond.Status != api.ConditionFalse { t.Errorf("got %v, want False (Available=True irrelevant during deletion)", cond.Status) } @@ -651,11 +651,45 @@ func TestComputeReconciled(t *testing.T) { byAdapter := map[string]adapterAvailableSnapshot{ "a": {observedGeneration: 2, availableTrue: true, finalizedTrue: false, observedTime: aggT1}, } - cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter) + cond := computeReconciled(2, aggTRef, nil, nil, required, byAdapter, false) if cond.Status != api.ConditionTrue { t.Errorf("got %v, want True (normal lifecycle uses Available)", cond.Status) } }) + + name := "deletedTime set, all Finalized=True, hasChildResources=true" + + " → False with WaitingForChildResources" + t.Run(name, func(t *testing.T) { + t.Parallel() + required := []string{"a", "b"} + byAdapter := map[string]adapterAvailableSnapshot{ + "a": {observedGeneration: 2, availableTrue: false, finalizedTrue: true, observedTime: aggT1}, + "b": {observedGeneration: 2, availableTrue: false, finalizedTrue: true, observedTime: aggT2}, + } + cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter, true) + if cond.Status != api.ConditionFalse { + t.Errorf("got %v, want False (child resources still exist)", cond.Status) + } + if cond.Reason == nil || *cond.Reason != reasonWaitingForChildResources { + t.Errorf("Reason got %v, want %q", cond.Reason, reasonWaitingForChildResources) + } + }) + + t.Run("deletedTime set, all Finalized=True, hasChildResources=false → True", func(t *testing.T) { + t.Parallel() + required := []string{"a", "b"} + byAdapter := map[string]adapterAvailableSnapshot{ + "a": {observedGeneration: 2, availableTrue: false, finalizedTrue: true, observedTime: aggT1}, + "b": {observedGeneration: 2, availableTrue: false, finalizedTrue: true, observedTime: aggT2}, + } + cond := computeReconciled(2, aggTRef, deletedAt(aggT0), nil, required, byAdapter, false) + if cond.Status != api.ConditionTrue { + t.Errorf("got %v, want True (no child resources)", cond.Status) + } + if cond.Reason == nil || *cond.Reason != reasonAllAdaptersReconciled { + t.Errorf("Reason got %v, want %q", cond.Reason, reasonAllAdaptersReconciled) + } + }) } // --------------------------------------------------------------------------- @@ -949,6 +983,7 @@ func TestComputeAvailable(t *testing.T) { t.Errorf("LastTransitionTime got %v, want prev.LastTransitionTime=%v", cond.LastTransitionTime, aggT0) } }) + } // --------------------------------------------------------------------------- diff --git a/pkg/services/cluster.go b/pkg/services/cluster.go index 856c87aa..7cd8ed65 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -4,8 +4,6 @@ import ( "bytes" "context" "encoding/json" - stderrors "errors" - "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -13,7 +11,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" - "gorm.io/gorm" ) //go:generate mockgen-v0.6.0 -source=cluster.go -package=services -destination=cluster_mock.go @@ -24,17 +21,9 @@ type ClusterService interface { Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, *errors.ServiceError) SoftDelete(ctx context.Context, id string) (*api.Cluster, *errors.ServiceError) All(ctx context.Context) (api.ClusterList, *errors.ServiceError) - FindByIDs(ctx context.Context, ids []string) (api.ClusterList, *errors.ServiceError) - - // UpdateClusterStatusFromAdapters recomputes aggregated Ready and Available from stored adapter rows. UpdateClusterStatusFromAdapters(ctx context.Context, clusterID string) (*api.Cluster, *errors.ServiceError) - - // ProcessAdapterStatus validates mandatory conditions, applies discard rules, upserts adapter status, - // and triggers aggregation when appropriate. - ProcessAdapterStatus( - ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus, - ) (*api.AdapterStatus, *errors.ServiceError) + ProcessAdapterStatus(ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) // nolint:lll // idempotent functions for the control plane, but can also be called synchronously by any actor OnUpsert(ctx context.Context, id string) error @@ -142,7 +131,7 @@ func (s *sqlClusterService) SoftDelete(ctx context.Context, id string) (*api.Clu if err != nil { return nil, errors.GeneralError("Failed to fetch cascade-deleted nodepools: %s", err) } - if svcErr := batchUpdateNodePoolStatusesFromAdapters( + if svcErr := updateNodePoolStatusesForCascadeDelete( ctx, nodePools, s.nodePoolDao, @@ -199,7 +188,7 @@ func clusterRefTime(c *api.Cluster) time.Time { return c.CreatedTime } -// UpdateClusterStatusFromAdapters recomputes aggregated Ready, Available, and per-adapter conditions +// UpdateClusterStatusFromAdapters recomputes aggregated Reconciled, Available, and per-adapter conditions // from stored adapter rows and persists them to the cluster's status. func (s *sqlClusterService) UpdateClusterStatusFromAdapters( ctx context.Context, clusterID string, @@ -214,7 +203,25 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters( return nil, errors.GeneralError("Failed to get adapter statuses: %s", err) } + return s.recomputeAndSaveClusterStatus(ctx, cluster, adapterStatuses) +} + +// recomputeAndSaveClusterStatus aggregates adapter reports into cluster conditions and +// persists only when the result differs from the current stored value. +func (s *sqlClusterService) recomputeAndSaveClusterStatus( + ctx context.Context, cluster *api.Cluster, adapterStatuses api.AdapterStatusList, +) (*api.Cluster, *errors.ServiceError) { refTime := clusterRefTime(cluster) + + hasChildResources := false + if cluster.DeletedTime != nil { + exists, err := s.nodePoolDao.ExistsByOwner(ctx, cluster.ID) + if err != nil { + return nil, errors.GeneralError("Failed to check child node pools for status aggregation: %s", err) + } + hasChildResources = exists + } + reconciled, available, adapterConditions := AggregateResourceStatus(ctx, AggregateResourceStatusInput{ ResourceGeneration: cluster.Generation, RefTime: refTime, @@ -222,13 +229,14 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters( PrevConditionsJSON: cluster.StatusConditions, RequiredAdapters: s.adapterConfig.RequiredClusterAdapters(), AdapterStatuses: adapterStatuses, + HasChildResources: hasChildResources, }) // Ready mirrors Reconciled for backward compatibility (deprecated) ready := reconciled ready.Type = api.ConditionTypeReady - allConditions := make([]api.ResourceCondition, 0, 3+len(adapterConditions)) + allConditions := make([]api.ResourceCondition, 0, fixedConditionCount+len(adapterConditions)) allConditions = append(allConditions, ready, reconciled, available) allConditions = append(allConditions, adapterConditions...) @@ -241,71 +249,131 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters( return cluster, nil } - cluster.StatusConditions = conditionsJSON - - cluster, err = s.clusterDao.Replace(ctx, cluster) - if err != nil { + if err := s.clusterDao.SaveStatusConditions(ctx, cluster.ID, conditionsJSON); err != nil { return nil, handleUpdateError("Cluster", err) } + cluster.StatusConditions = conditionsJSON return cluster, nil } -// ProcessAdapterStatus applies discard rules, then mandatory validation and upsert. +// ProcessAdapterStatus validates mandatory conditions, applies discard rules, upserts adapter +// status, and triggers aggregation when appropriate. +// +// DB call budget (non-deleted happy path): +// 1. GetForUpdate — lock + fetch cluster +// 2. FindByResource — all adapter statuses (existing status found in-memory) +// 3. UpsertWithExisting — write adapter status +// 4. SaveStatusConditions — write updated cluster conditions (skipped when unchanged) func (s *sqlClusterService) ProcessAdapterStatus( ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus, ) (*api.AdapterStatus, *errors.ServiceError) { - cluster, err := s.clusterDao.Get(ctx, clusterID) + // 1. Acquire a row-level exclusive lock on the cluster for the duration of this + // transaction. Concurrent adapter status updates for the same cluster are + // serialized here: the second caller blocks until the first commits, ensuring + // the aggregation step always reads a fully up-to-date set of adapter statuses. + cluster, err := s.clusterDao.GetForUpdate(ctx, clusterID) if err != nil { return nil, handleGetError("Cluster", "id", clusterID, err) } - existingStatus, findErr := s.adapterStatusDao.FindByResourceAndAdapter( - ctx, "Cluster", clusterID, adapterStatus.Adapter, + allStatuses, err := s.adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) + if err != nil { + return nil, errors.GeneralError("Failed to get adapter statuses: %s", err) + } + + existingStatus := findAdapterStatusInList(allStatuses, adapterStatus.Adapter) + + conditions, triggerAggregation, svcErr := s.validateAndClassify( + ctx, clusterID, adapterStatus, cluster, existingStatus, ) - if findErr != nil && !stderrors.Is(findErr, gorm.ErrRecordNotFound) { - if !strings.Contains(findErr.Error(), errors.CodeNotFoundGeneric) { - return nil, errors.GeneralError("Failed to get adapter status: %s", findErr) + if svcErr != nil { + return nil, svcErr + } + if conditions == nil && !triggerAggregation { + return nil, nil + } + + adapterStatus.ResourceType = "Cluster" + adapterStatus.ResourceID = clusterID + setConditionTransitionTimes(adapterStatus, existingStatus) + + upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus, existingStatus) + if err != nil { + return nil, handleCreateError("AdapterStatus", err) + } + + // Build the post-upsert snapshot once and reuse it for both the hard-delete check + // and aggregation. Using the pre-upsert allStatuses for the hard-delete check would + // cause allAdaptersFinalized to return false when the current adapter is the last one + // needed to complete the Finalized=True quorum. + updatedStatuses := replaceAdapterStatusInList(allStatuses, upsertedStatus) + + if cluster.DeletedTime != nil { + hardDeleted, hdErr := s.tryHardDeleteCluster(ctx, cluster, conditions, updatedStatuses) + if hdErr != nil { + return nil, hdErr + } + if hardDeleted { + return upsertedStatus, nil } } + // 4. Re-aggregate using data already in memory. + if triggerAggregation { + if _, aggregateErr := s.recomputeAndSaveClusterStatus(ctx, cluster, updatedStatuses); aggregateErr != nil { + return nil, aggregateErr + } + } + + return upsertedStatus, nil +} + +// validateAndClassify performs all stateless validation and discard-rule checks on an incoming +// adapter status. Returns the parsed conditions and whether aggregation should be triggered. +// Returns (nil, false, nil) when the update should be silently discarded. +func (s *sqlClusterService) validateAndClassify( + ctx context.Context, + clusterID string, + adapterStatus *api.AdapterStatus, + cluster *api.Cluster, + existingStatus *api.AdapterStatus, +) ([]api.AdapterCondition, bool, *errors.ServiceError) { + log := logger.With(logger.WithClusterID(ctx, clusterID), logger.FieldAdapter, adapterStatus.Adapter) + if adapterStatus.ObservedGeneration > cluster.Generation { - logger.With(logger.WithClusterID(ctx, clusterID), logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: future generation") - return nil, nil + log.Debug("Discarding adapter status update: future generation") + return nil, false, nil } if existingStatus != nil && adapterStatus.ObservedGeneration < existingStatus.ObservedGeneration { - logger.With(logger.WithClusterID(ctx, clusterID), logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: stale generation") - return nil, nil + log.Debug("Discarding adapter status update: stale generation") + return nil, false, nil } incomingObs := AdapterObservedTime(adapterStatus) if incomingObs.IsZero() { - logger.With(logger.WithClusterID(ctx, clusterID), logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: zero observed time") - return nil, nil + log.Debug("Discarding adapter status update: zero observed time") + return nil, false, nil } if existingStatus != nil && adapterStatus.ObservedGeneration == existingStatus.ObservedGeneration { prevObs := AdapterObservedTime(existingStatus) if !prevObs.IsZero() && incomingObs.Before(prevObs) { - logger.With(logger.WithClusterID(ctx, clusterID), logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: stale observed time") - return nil, nil + log.Debug("Discarding adapter status update: stale observed time") + return nil, false, nil } } var conditions []api.AdapterCondition if len(adapterStatus.Conditions) > 0 { if errUnmarshal := json.Unmarshal(adapterStatus.Conditions, &conditions); errUnmarshal != nil { - return nil, errors.GeneralError("Failed to unmarshal adapter status conditions: %s", errUnmarshal) + return nil, false, errors.GeneralError("Failed to unmarshal adapter status conditions: %s", errUnmarshal) } } if errorType, conditionName := ValidateMandatoryConditions(conditions); errorType != "" { - return nil, errors.Validation( + return nil, false, errors.Validation( "missing mandatory condition '%s': all adapters must report Available, Applied, and Health", conditionName, ) @@ -321,20 +389,17 @@ func (s *sqlClusterService) ProcessAdapterStatus( cond.Status == api.AdapterConditionFalse || cond.Status == api.AdapterConditionUnknown if !isValidStatus { - return nil, errors.Validation( + return nil, false, errors.Validation( "condition '%s' has invalid status '%s': must be True, False, or Unknown", cond.Type, cond.Status, ) } if cond.Status != api.AdapterConditionTrue && cond.Status != api.AdapterConditionFalse { - // Status is Unknown — only valid on first report; discard subsequent reports. if existingStatus != nil { - logger.With(logger.WithClusterID(ctx, clusterID), logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: subsequent Unknown Available") - return nil, nil + log.Debug("Discarding adapter status update: subsequent Unknown Available") + return nil, false, nil } - // First report may carry Unknown; store it but do not aggregate from it. triggerAggregation = false break } @@ -343,19 +408,82 @@ func (s *sqlClusterService) ProcessAdapterStatus( break } - adapterStatus.ResourceType = "Cluster" - adapterStatus.ResourceID = clusterID + return conditions, triggerAggregation, nil +} + +// tryHardDeleteCluster checks whether all required adapters have reported Finalized=True for +// a soft-deleted cluster and, when there are no remaining node pools, permanently removes the +// cluster and all its adapter statuses. Returns true when the hard-delete was performed. +func (s *sqlClusterService) tryHardDeleteCluster( + ctx context.Context, + cluster *api.Cluster, + conditions []api.AdapterCondition, + allStatuses api.AdapterStatusList, +) (bool, *errors.ServiceError) { + if !incomingReportedFinalized(conditions) { + return false, nil + } - upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) + if !allAdaptersFinalized(s.adapterConfig.Required.Cluster, allStatuses) { + return false, nil + } + + hasNodePools, err := s.nodePoolDao.ExistsByOwner(ctx, cluster.ID) if err != nil { - return nil, handleCreateError("AdapterStatus", err) + return false, errors.GeneralError("Failed to check nodepools during hard-delete: %s", err) + } + if hasNodePools { + return false, nil } - if triggerAggregation { - if _, aggregateErr := s.UpdateClusterStatusFromAdapters(ctx, clusterID); aggregateErr != nil { - return nil, aggregateErr + if err := s.adapterStatusDao.DeleteByResource(ctx, "Cluster", cluster.ID); err != nil { + return false, errors.GeneralError("Failed to delete adapter statuses during hard-delete: %s", err) + } + if err := s.clusterDao.Delete(ctx, cluster.ID); err != nil { + return false, errors.GeneralError("Failed to hard-delete cluster: %s", err) + } + + logger.With(logger.WithClusterID(ctx, cluster.ID)). + Info("Hard-deleted cluster after all required adapters reported Finalized=True and no nodepools exist") + + return true, nil +} + +func findAdapterStatusInList(statuses api.AdapterStatusList, adapter string) *api.AdapterStatus { + for _, s := range statuses { + if s.Adapter == adapter { + return s } } + return nil +} - return upsertedStatus, nil +// replaceAdapterStatusInList returns a copy of the list with the entry for the given adapter +// replaced (or appended if not present). Used to build an up-to-date snapshot for aggregation +// after an upsert without re-querying. +func replaceAdapterStatusInList(statuses api.AdapterStatusList, updated *api.AdapterStatus) api.AdapterStatusList { + result := make(api.AdapterStatusList, 0, len(statuses)+1) + found := false + for _, s := range statuses { + if s.Adapter == updated.Adapter && s.ResourceType == updated.ResourceType && s.ResourceID == updated.ResourceID { + result = append(result, updated) + found = true + } else { + result = append(result, s) + } + } + if !found { + result = append(result, updated) + } + return result +} + +// incomingReportedFinalized returns true when the adapter conditions contain Finalized=True. +func incomingReportedFinalized(conditions []api.AdapterCondition) bool { + for _, cond := range conditions { + if cond.Type == api.ConditionTypeFinalized && cond.Status == api.AdapterConditionTrue { + return true + } + } + return false } diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go index 582f48bd..079ce5c7 100644 --- a/pkg/services/cluster_test.go +++ b/pkg/services/cluster_test.go @@ -50,6 +50,10 @@ func (d *mockClusterDao) Get(ctx context.Context, id string) (*api.Cluster, erro return nil, gorm.ErrRecordNotFound } +func (d *mockClusterDao) GetForUpdate(ctx context.Context, id string) (*api.Cluster, error) { + return d.Get(ctx, id) +} + func (d *mockClusterDao) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { if cluster.CreatedTime.IsZero() { now := time.Now() @@ -72,6 +76,15 @@ func (d *mockClusterDao) Save(ctx context.Context, cluster *api.Cluster) error { return nil } +func (d *mockClusterDao) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { + c, ok := d.clusters[id] + if !ok { + return gorm.ErrRecordNotFound + } + c.StatusConditions = statusConditions + return nil +} + func (d *mockClusterDao) Delete(ctx context.Context, id string) error { delete(d.clusters, id) return nil @@ -124,9 +137,11 @@ func (d *mockAdapterStatusDao) Replace(ctx context.Context, status *api.AdapterS return status, nil } -func (d *mockAdapterStatusDao) Upsert(ctx context.Context, status *api.AdapterStatus) (*api.AdapterStatus, error) { +func (d *mockAdapterStatusDao) Upsert( + ctx context.Context, status *api.AdapterStatus, existing *api.AdapterStatus, +) (*api.AdapterStatus, error) { key := status.ResourceType + ":" + status.ResourceID + ":" + status.Adapter - if existing, ok := d.statuses[key]; ok { + if existing != nil { isStoredFresherOrEqual := existing.ObservedGeneration > status.ObservedGeneration || (existing.ObservedGeneration == status.ObservedGeneration && !existing.LastReportTime.Before(status.LastReportTime)) @@ -151,6 +166,15 @@ func (d *mockAdapterStatusDao) Delete(ctx context.Context, id string) error { return nil } +func (d *mockAdapterStatusDao) DeleteByResource(ctx context.Context, resourceType, resourceID string) error { + for key, s := range d.statuses { + if s.ResourceType == resourceType && s.ResourceID == resourceID { + delete(d.statuses, key) + } + } + return nil +} + func (d *mockAdapterStatusDao) FindByResource( ctx context.Context, resourceType, resourceID string, @@ -299,7 +323,7 @@ func TestProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { CreatedTime: now, LastReportTime: now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _ = adapterStatusDao.Upsert(ctx, existingStatus, nil) // Now send another Unknown status report newAdapterStatus := &api.AdapterStatus{ @@ -633,7 +657,7 @@ func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *t CreatedTime: now, LastReportTime: now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _ = adapterStatusDao.Upsert(ctx, existingStatus, nil) // Now send another report with multiple conditions including Available=Unknown conditions := []api.AdapterCondition{ @@ -1379,3 +1403,129 @@ func TestClusterSoftDelete(t *testing.T) { g.Expect(postReady.ObservedGeneration).To(Equal(int32(2))) }) } + +func TestReconciled_DuringDeletion_ChildResources(t *testing.T) { + adapterConfig := &config.AdapterRequirementsConfig{ + Required: config.RequiredAdapters{ + Cluster: []string{"validation"}, + Nodepool: []string{"validation"}, + }, + } + + makeClusterWithDeletion := func(clusterID string, gen int32) *api.Cluster { + t := time.Now().UTC().Truncate(time.Microsecond) + return &api.Cluster{ + Meta: api.Meta{ID: clusterID}, + Generation: gen, + DeletedTime: &t, + } + } + + finalizedConditions := func(gen int32) []byte { + now := time.Now() + conds := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: now}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: now}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: now}, + {Type: api.ConditionTypeFinalized, Status: api.AdapterConditionTrue, LastTransitionTime: now}, + } + b, _ := json.Marshal(conds) + return b + } + + name := "all adapters finalized, nodepool still exists" + + " → Reconciled=False with WaitingForChildResources" + t.Run(name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + clusterID := "cluster-with-nodepools" + + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, adapterConfig) + + cluster := makeClusterWithDeletion(clusterID, 2) + clusterDao.clusters[clusterID] = cluster + + nodePoolDao.nodePools["np-1"] = &api.NodePool{Meta: api.Meta{ID: "np-1"}, OwnerID: clusterID} + + now := time.Now() + condJSON := finalizedConditions(2) + adapterStatusDao.statuses["Cluster:"+clusterID+":validation"] = &api.AdapterStatus{ + ResourceType: "Cluster", ResourceID: clusterID, Adapter: "validation", + ObservedGeneration: 2, Conditions: condJSON, + CreatedTime: now, LastReportTime: now, + } + + updated, svcErr := service.UpdateClusterStatusFromAdapters(ctx, clusterID) + g.Expect(svcErr).To(BeNil()) + + var conds []api.ResourceCondition + g.Expect(json.Unmarshal(updated.StatusConditions, &conds)).To(Succeed()) + + var reconciled, ready *api.ResourceCondition + for i := range conds { + switch conds[i].Type { + case api.ConditionTypeReconciled: + reconciled = &conds[i] + case api.ConditionTypeReady: + ready = &conds[i] + } + } + + g.Expect(reconciled).NotTo(BeNil()) + g.Expect(reconciled.Status).To(Equal(api.ConditionFalse)) + g.Expect(reconciled.Reason).NotTo(BeNil()) + g.Expect(*reconciled.Reason).To(Equal(reasonWaitingForChildResources)) + + g.Expect(ready).NotTo(BeNil()) + g.Expect(ready.Status).To(Equal(api.ConditionFalse)) + }) + + t.Run("all adapters finalized, no nodepools → Reconciled=True", func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + clusterID := "cluster-no-nodepools" + + clusterDao := newMockClusterDao() + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + service := NewClusterService(clusterDao, nodePoolDao, adapterStatusDao, adapterConfig) + + cluster := makeClusterWithDeletion(clusterID, 2) + clusterDao.clusters[clusterID] = cluster + + now := time.Now() + condJSON := finalizedConditions(2) + adapterStatusDao.statuses["Cluster:"+clusterID+":validation"] = &api.AdapterStatus{ + ResourceType: "Cluster", ResourceID: clusterID, Adapter: "validation", + ObservedGeneration: 2, Conditions: condJSON, + CreatedTime: now, LastReportTime: now, + } + + updated, svcErr := service.UpdateClusterStatusFromAdapters(ctx, clusterID) + g.Expect(svcErr).To(BeNil()) + + var conds []api.ResourceCondition + g.Expect(json.Unmarshal(updated.StatusConditions, &conds)).To(Succeed()) + + var reconciled, ready *api.ResourceCondition + for i := range conds { + switch conds[i].Type { + case api.ConditionTypeReconciled: + reconciled = &conds[i] + case api.ConditionTypeReady: + ready = &conds[i] + } + } + + g.Expect(reconciled).NotTo(BeNil()) + g.Expect(reconciled.Status).To(Equal(api.ConditionTrue)) + g.Expect(reconciled.Reason).NotTo(BeNil()) + g.Expect(*reconciled.Reason).To(Equal(reasonAllAdaptersReconciled)) + + g.Expect(ready).NotTo(BeNil()) + g.Expect(ready.Status).To(Equal(api.ConditionTrue)) + }) +} diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index 14438591..08c7f515 100644 --- a/pkg/services/node_pool.go +++ b/pkg/services/node_pool.go @@ -3,8 +3,6 @@ package services import ( "context" "encoding/json" - stderrors "errors" - "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -12,7 +10,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" - "gorm.io/gorm" ) //go:generate mockgen-v0.6.0 -source=node_pool.go -package=services -destination=node_pool_mock.go @@ -178,6 +175,9 @@ func nodePoolRefTime(np *api.NodePool) time.Time { return np.CreatedTime } +// UpdateNodePoolStatusFromAdapters is the public entry point for callers outside +// ProcessAdapterStatus (e.g. Create, Replace, SoftDelete) that don't already hold the node +// pool and adapter statuses. func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( ctx context.Context, nodePoolID string, ) (*api.NodePool, *errors.ServiceError) { @@ -190,60 +190,150 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( ) } +// recomputeAndSaveNodePoolStatus aggregates adapter reports into node pool conditions and +// persists only when the result differs from the current stored value. Accepts pre-fetched +// data to avoid redundant DB reads. +func (s *sqlNodePoolService) recomputeAndSaveNodePoolStatus( + ctx context.Context, nodePool *api.NodePool, adapterStatuses api.AdapterStatusList, +) (*api.NodePool, *errors.ServiceError) { + conditionsJSON, svcErr := computeNodePoolConditionsJSON( + ctx, nodePool, adapterStatuses, s.adapterConfig.RequiredNodePoolAdapters(), + ) + if svcErr != nil { + return nil, svcErr + } + if conditionsJSON == nil { + return nodePool, nil + } + + if err := s.nodePoolDao.SaveStatusConditions(ctx, nodePool.ID, conditionsJSON); err != nil { + return nil, handleUpdateError("NodePool", err) + } + + nodePool.StatusConditions = conditionsJSON + return nodePool, nil +} + +// ProcessAdapterStatus validates mandatory conditions, applies discard rules, upserts adapter +// status, and triggers aggregation when appropriate. +// +// DB call budget (non-deleted happy path): +// 1. GetForUpdate — lock + fetch node pool +// 2. FindByResource — all adapter statuses (existing status found in-memory) +// 3. Upsert — write adapter status +// 4. SaveStatusConditions — write updated node pool conditions (skipped when unchanged) func (s *sqlNodePoolService) ProcessAdapterStatus( ctx context.Context, nodePoolID string, adapterStatus *api.AdapterStatus, ) (*api.AdapterStatus, *errors.ServiceError) { - nodePool, err := s.nodePoolDao.Get(ctx, nodePoolID) + // 1. Acquire a row-level exclusive lock on the node pool for the duration of this + // transaction. Concurrent adapter status updates for the same node pool are + // serialized here: the second caller blocks until the first commits, ensuring + // the aggregation step always reads a fully up-to-date set of adapter statuses. + nodePool, err := s.nodePoolDao.GetForUpdate(ctx, nodePoolID) if err != nil { return nil, handleGetError("NodePool", "id", nodePoolID, err) } - existingStatus, findErr := s.adapterStatusDao.FindByResourceAndAdapter( - ctx, "NodePool", nodePoolID, adapterStatus.Adapter, + // 2. Fetch all adapter statuses for this node pool in one query. This replaces the + // individual FindByResourceAndAdapter and later FindByResource calls. + allStatuses, err := s.adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) + if err != nil { + return nil, errors.GeneralError("Failed to get adapter statuses: %s", err) + } + + existingStatus := findAdapterStatusInList(allStatuses, adapterStatus.Adapter) + + conditions, triggerAggregation, svcErr := s.validateAndClassifyNodePool( + ctx, nodePoolID, adapterStatus, nodePool, existingStatus, ) - if findErr != nil && !stderrors.Is(findErr, gorm.ErrRecordNotFound) { - if !strings.Contains(findErr.Error(), errors.CodeNotFoundGeneric) { - return nil, errors.GeneralError("Failed to get adapter status: %s", findErr) + if svcErr != nil { + return nil, svcErr + } + if conditions == nil && !triggerAggregation { + return nil, nil + } + + // 3. Upsert using the already-fetched existing status to skip a redundant lookup. + adapterStatus.ResourceType = "NodePool" + adapterStatus.ResourceID = nodePoolID + setConditionTransitionTimes(adapterStatus, existingStatus) + + upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus, existingStatus) + if err != nil { + return nil, handleCreateError("AdapterStatus", err) + } + + // Build the post-upsert snapshot once and reuse it for both the hard-delete check + // and aggregation. Using the pre-upsert allStatuses for the hard-delete check would + // cause allAdaptersFinalized to return false when the current adapter is the last one + // needed to complete the Finalized=True quorum. + updatedStatuses := replaceAdapterStatusInList(allStatuses, upsertedStatus) + + if nodePool.DeletedTime != nil { + hardDeleted, hdErr := s.tryHardDeleteNodePool(ctx, nodePoolID, conditions, updatedStatuses) + if hdErr != nil { + return nil, hdErr + } + if hardDeleted { + return upsertedStatus, nil } } + // 4. Re-aggregate using data already in memory. + if triggerAggregation { + if _, aggregateErr := s.recomputeAndSaveNodePoolStatus(ctx, nodePool, updatedStatuses); aggregateErr != nil { + return nil, aggregateErr + } + } + + return upsertedStatus, nil +} + +// validateAndClassifyNodePool performs all stateless validation and discard-rule checks on an +// incoming adapter status for a node pool. Returns the parsed conditions and whether aggregation +// should be triggered. Returns (nil, false, nil) when the update should be silently discarded. +func (s *sqlNodePoolService) validateAndClassifyNodePool( + ctx context.Context, + nodePoolID string, + adapterStatus *api.AdapterStatus, + nodePool *api.NodePool, + existingStatus *api.AdapterStatus, +) ([]api.AdapterCondition, bool, *errors.ServiceError) { + l := logger.With(ctx, logger.FieldNodePoolID, nodePoolID, logger.FieldAdapter, adapterStatus.Adapter) + if adapterStatus.ObservedGeneration > nodePool.Generation { - logger.With(ctx, logger.FieldNodePoolID, nodePoolID, logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: future generation") - return nil, nil + l.Debug("Discarding adapter status update: future generation") + return nil, false, nil } if existingStatus != nil && adapterStatus.ObservedGeneration < existingStatus.ObservedGeneration { - logger.With(ctx, logger.FieldNodePoolID, nodePoolID, logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: stale generation") - return nil, nil + l.Debug("Discarding adapter status update: stale generation") + return nil, false, nil } incomingObs := AdapterObservedTime(adapterStatus) if incomingObs.IsZero() { - logger.With(ctx, logger.FieldNodePoolID, nodePoolID, logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: zero observed time") - return nil, nil + l.Debug("Discarding adapter status update: zero observed time") + return nil, false, nil } if existingStatus != nil && adapterStatus.ObservedGeneration == existingStatus.ObservedGeneration { prevObs := AdapterObservedTime(existingStatus) if !prevObs.IsZero() && incomingObs.Before(prevObs) { - logger.With(ctx, logger.FieldNodePoolID, nodePoolID, logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: stale observed time") - return nil, nil + l.Debug("Discarding adapter status update: stale observed time") + return nil, false, nil } } var conditions []api.AdapterCondition if len(adapterStatus.Conditions) > 0 { if errUnmarshal := json.Unmarshal(adapterStatus.Conditions, &conditions); errUnmarshal != nil { - return nil, errors.GeneralError("Failed to unmarshal adapter status conditions: %s", errUnmarshal) + return nil, false, errors.GeneralError("Failed to unmarshal adapter status conditions: %s", errUnmarshal) } } if errorType, conditionName := ValidateMandatoryConditions(conditions); errorType != "" { - return nil, errors.Validation( + return nil, false, errors.Validation( "missing mandatory condition '%s': all adapters must report Available, Applied, and Health", conditionName, ) @@ -259,18 +349,16 @@ func (s *sqlNodePoolService) ProcessAdapterStatus( cond.Status == api.AdapterConditionFalse || cond.Status == api.AdapterConditionUnknown if !isValidStatus { - return nil, errors.Validation( + return nil, false, errors.Validation( "condition '%s' has invalid status '%s': must be True, False, or Unknown", cond.Type, cond.Status, ) } if cond.Status != api.AdapterConditionTrue && cond.Status != api.AdapterConditionFalse { - // Status is Unknown — only valid on first report; discard subsequent reports. if existingStatus != nil { - logger.With(ctx, logger.FieldNodePoolID, nodePoolID, logger.FieldAdapter, adapterStatus.Adapter). - Debug("Discarding adapter status update: subsequent Unknown Available") - return nil, nil + l.Debug("Discarding adapter status update: subsequent Unknown Available") + return nil, false, nil } triggerAggregation = false break @@ -280,19 +368,37 @@ func (s *sqlNodePoolService) ProcessAdapterStatus( break } - adapterStatus.ResourceType = "NodePool" - adapterStatus.ResourceID = nodePoolID + return conditions, triggerAggregation, nil +} - upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) - if err != nil { - return nil, handleCreateError("AdapterStatus", err) +// tryHardDeleteNodePool checks whether all required adapters have reported Finalized=True for +// a soft-deleted node pool and, when so, permanently removes the node pool and all its adapter +// statuses. Unlike clusters, node pools have no child resources to check. Returns true when the +// hard-delete was performed. +// +// Accepts the pre-fetched (post-upsert) adapter statuses list to avoid a redundant FindByResource +// call and to ensure the just-upserted status is visible to allAdaptersFinalized. +func (s *sqlNodePoolService) tryHardDeleteNodePool( + ctx context.Context, nodePoolID string, conditions []api.AdapterCondition, + allStatuses api.AdapterStatusList, +) (bool, *errors.ServiceError) { + if !incomingReportedFinalized(conditions) { + return false, nil } - if triggerAggregation { - if _, aggregateErr := s.UpdateNodePoolStatusFromAdapters(ctx, nodePoolID); aggregateErr != nil { - return nil, aggregateErr - } + if !allAdaptersFinalized(s.adapterConfig.Required.Nodepool, allStatuses) { + return false, nil } - return upsertedStatus, nil + if err := s.adapterStatusDao.DeleteByResource(ctx, "NodePool", nodePoolID); err != nil { + return false, errors.GeneralError("Failed to delete adapter statuses during hard-delete: %s", err) + } + if err := s.nodePoolDao.Delete(ctx, nodePoolID); err != nil { + return false, errors.GeneralError("Failed to hard-delete nodepool: %s", err) + } + + logger.With(ctx, logger.FieldNodePoolID, nodePoolID). + Info("Hard-deleted nodepool after all required adapters reported Finalized=True") + + return true, nil } diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index af4f4122..6031aacf 100644 --- a/pkg/services/node_pool_test.go +++ b/pkg/services/node_pool_test.go @@ -48,6 +48,18 @@ func (d *mockNodePoolDao) Get(ctx context.Context, id string) (*api.NodePool, er return nil, gorm.ErrRecordNotFound } +func (d *mockNodePoolDao) GetForUpdate(ctx context.Context, id string) (*api.NodePool, error) { + return d.Get(ctx, id) +} + +func (d *mockNodePoolDao) SaveStatusConditions(ctx context.Context, id string, statusConditions []byte) error { + if np, ok := d.nodePools[id]; ok { + np.StatusConditions = statusConditions + return nil + } + return gorm.ErrRecordNotFound +} + func (d *mockNodePoolDao) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { if nodePool.CreatedTime.IsZero() { now := time.Now() @@ -107,6 +119,16 @@ func (d *mockNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.Node return result, nil } +func (d *mockNodePoolDao) FindByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) { + var result api.NodePoolList + for _, np := range d.nodePools { + if np.OwnerID == ownerID { + result = append(result, np) + } + } + return result, nil +} + func (d *mockNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error { for id, statusConditions := range updates { if np, ok := d.nodePools[id]; ok { @@ -117,6 +139,15 @@ func (d *mockNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, updat return nil } +func (d *mockNodePoolDao) ExistsByOwner(ctx context.Context, ownerID string) (bool, error) { + for _, np := range d.nodePools { + if np.OwnerID == ownerID { + return true, nil + } + } + return false, nil +} + func (d *mockNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) { var result api.NodePoolList for _, np := range d.nodePools { @@ -225,7 +256,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { CreatedTime: now, LastReportTime: now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _ = adapterStatusDao.Upsert(ctx, existingStatus, nil) // Now send another Unknown status report newAdapterStatus := &api.AdapterStatus{ @@ -497,7 +528,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnkn CreatedTime: now, LastReportTime: now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _ = adapterStatusDao.Upsert(ctx, existingStatus, nil) // Now send another report with multiple conditions including Available=Unknown conditions := []api.AdapterCondition{ diff --git a/pkg/services/status_helpers.go b/pkg/services/status_helpers.go index dec07ed4..eda3d0a9 100644 --- a/pkg/services/status_helpers.go +++ b/pkg/services/status_helpers.go @@ -32,7 +32,7 @@ func computeNodePoolConditionsJSON( ready := reconciled ready.Type = api.ConditionTypeReady - allConditions := make([]api.ResourceCondition, 0, 3+len(adapterConditions)) + allConditions := make([]api.ResourceCondition, 0, fixedConditionCount+len(adapterConditions)) allConditions = append(allConditions, ready, reconciled, available) allConditions = append(allConditions, adapterConditions...) @@ -89,10 +89,10 @@ func updateNodePoolStatusFromAdapters( return nodePool, nil } -// batchUpdateNodePoolStatusesFromAdapters updates status conditions for multiple nodepools. -// It's fetching all adapter statuses in one query and persisting -// all changed nodepools via UpdateStatusConditionsByIDs. -func batchUpdateNodePoolStatusesFromAdapters( +// updateNodePoolStatusesForCascadeDelete recomputes and persists status conditions for a set of +// cascade-soft-deleted nodepools. Fetches all their adapter statuses in one query and writes +// only changed rows via UpdateStatusConditionsByIDs. +func updateNodePoolStatusesForCascadeDelete( ctx context.Context, nodePools []*api.NodePool, nodePoolDao dao.NodePoolDao, diff --git a/test/integration/clusters_test.go b/test/integration/clusters_test.go index 7176918f..52483587 100644 --- a/test/integration/clusters_test.go +++ b/test/integration/clusters_test.go @@ -1119,3 +1119,275 @@ func TestClusterSoftDelete(t *testing.T) { "nodepool generation should not be incremented on repeated delete") }) } + +func TestClusterHardDelete(t *testing.T) { + t.Run("given a soft-deleted cluster with no nodepools, when all required adapters report Finalized=True, then cluster is hard-deleted from DB", func(t *testing.T) { //nolint:lll + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + delResp, err := client.DeleteClusterByIdWithResponse(ctx, cluster.ID, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(delResp.StatusCode()).To(Equal(http.StatusAccepted)) + newGeneration := delResp.JSON202.Generation + Expect(newGeneration).To(Equal(cluster.Generation + 1)) + + requiredAdapters := []string{"validation", "dns", "pullsecret", "hypershift"} + dbSession := h.DBFactory.New(ctx) + + for _, adapter := range requiredAdapters { + statusInput := newAdapterStatusRequest( + adapter, + newGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ManifestWorkNotDiscovered"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("NamespaceNotDiscovered"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + { + Type: api.ConditionTypeFinalized, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("CleanupConfirmed"), + }, + }, + nil, + ) + statusResp, loopErr := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(loopErr).NotTo(HaveOccurred()) + Expect(statusResp.StatusCode()).To(Equal(http.StatusCreated)) + } + + var clusterCheck api.Cluster + dbErr := dbSession.First(&clusterCheck, "id = ?", cluster.ID).Error + Expect(dbErr).To(HaveOccurred(), "Cluster should be hard-deleted from DB") + Expect(dbErr.Error()).To(ContainSubstring("record not found")) + + var adapterStatuses []api.AdapterStatus + err = dbSession.Where("resource_type = ? AND resource_id = ?", "Cluster", cluster.ID).Find(&adapterStatuses).Error + Expect(err).NotTo(HaveOccurred()) + Expect(adapterStatuses).To(BeEmpty(), "Adapter statuses should be hard-deleted") + }) + + t.Run("given a soft-deleted cluster with nodepools, when a non-last adapter reports Finalized=True, then status is stored but cluster is not hard-deleted", func(t *testing.T) { //nolint:lll + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + nodePool, err := h.Factories.NewNodePools(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + dbSession := h.DBFactory.New(ctx) + err = dbSession.Model(nodePool).Update("owner_id", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + + delResp, err := client.DeleteClusterByIdWithResponse(ctx, cluster.ID, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(delResp.StatusCode()).To(Equal(http.StatusAccepted)) + newGeneration := delResp.JSON202.Generation + + statusInput := newAdapterStatusRequest( + "validation", + newGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ManifestWorkNotDiscovered"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("NamespaceNotDiscovered"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + { + Type: api.ConditionTypeFinalized, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("CleanupConfirmed"), + }, + }, + nil, + ) + statusResp, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(statusResp.StatusCode()).To(Equal(http.StatusCreated)) + + var adapterStatus api.AdapterStatus + err = dbSession.Where("resource_type = ? AND resource_id = ? AND adapter = ?", + "Cluster", cluster.ID, "validation").First(&adapterStatus).Error + Expect(err).NotTo(HaveOccurred()) + + var clusterCheck api.Cluster + err = dbSession.First(&clusterCheck, "id = ?", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + }) + + t.Run(`given a soft-deleted cluster with a nodepool, when cluster adapters, + report Finalized=True before nodepool adapters, cluster is not hard-deleted, + then once node pool adapters report Finalized=True as well, cluster is hard-deleted`, func(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + nodePool, err := h.Factories.NewNodePools(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + dbSession := h.DBFactory.New(ctx) + err = dbSession.Model(nodePool).Update("owner_id", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + + delResp, err := client.DeleteClusterByIdWithResponse(ctx, cluster.ID, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(delResp.StatusCode()).To(Equal(http.StatusAccepted)) + clusterNewGeneration := delResp.JSON202.Generation + + var nodePoolAfterDelete api.NodePool + err = dbSession.First(&nodePoolAfterDelete, "id = ?", nodePool.ID).Error + Expect(err).NotTo(HaveOccurred()) + nodePoolNewGeneration := nodePoolAfterDelete.Generation + Expect(nodePoolNewGeneration).To(Equal(nodePool.Generation + 1)) + + // STEP 1: Report all required cluster adapters with Finalized=True + // Nodepools still exist → cluster is not hard-deleted + clusterAdapters := []string{"validation", "dns", "pullsecret", "hypershift"} + for _, adapter := range clusterAdapters { + statusInput := newAdapterStatusRequest( + adapter, + clusterNewGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ManifestWorkNotDiscovered"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("NamespaceNotDiscovered"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + { + Type: api.ConditionTypeFinalized, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("CleanupConfirmed"), + }, + }, + nil, + ) + statusResp, loopErr := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(loopErr).NotTo(HaveOccurred()) + Expect(statusResp.StatusCode()).To(Equal(http.StatusCreated)) + } + + var clusterCheck api.Cluster + err = dbSession.First(&clusterCheck, "id = ?", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + Expect(clusterCheck.DeletedTime).NotTo(BeNil(), "Cluster should still be soft-deleted") + + // STEP 2: Report all required nodepool adapters with Finalized=True → nodepool is hard-deleted + nodePoolAdapters := []string{"validation", "hypershift"} + for _, adapter := range nodePoolAdapters { + statusInput := newAdapterStatusRequest( + adapter, + nodePoolNewGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ManifestWorkNotDiscovered"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("NamespaceNotDiscovered"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + { + Type: api.ConditionTypeFinalized, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("CleanupConfirmed"), + }, + }, + nil, + ) + _, loopErr := client.PostNodePoolStatusesWithResponse( + ctx, cluster.ID, nodePool.ID, + openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(loopErr).NotTo(HaveOccurred()) + } + + var nodePoolCheck api.NodePool + nodePoolErr := dbSession.First(&nodePoolCheck, "id = ?", nodePool.ID).Error + Expect(nodePoolErr).To(HaveOccurred()) + Expect(nodePoolErr.Error()).To(ContainSubstring("record not found")) + + // STEP 3: Re-report one cluster adapter with Finalized=True + // No nodepools remain → cluster is hard-deleted + statusInput := newAdapterStatusRequest( + "hypershift", + clusterNewGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ManifestWorkNotDiscovered"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("NamespaceNotDiscovered"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + { + Type: api.ConditionTypeFinalized, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("CleanupConfirmed"), + }, + }, + nil, + ) + finalResp, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(finalResp.StatusCode()).To(Equal(http.StatusCreated)) + + clusterErr := dbSession.First(&clusterCheck, "id = ?", cluster.ID).Error + Expect(clusterErr).To(HaveOccurred()) + Expect(clusterErr.Error()).To(ContainSubstring("record not found")) + + var adapterStatuses []api.AdapterStatus + err = dbSession.Where("resource_type = ? AND resource_id = ?", "Cluster", cluster.ID).Find(&adapterStatuses).Error + Expect(err).NotTo(HaveOccurred()) + Expect(adapterStatuses).To(BeEmpty()) + err = dbSession.Where("resource_type = ? AND resource_id = ?", "NodePool", nodePool.ID).Find(&adapterStatuses).Error + Expect(err).NotTo(HaveOccurred()) + Expect(adapterStatuses).To(BeEmpty()) + }) +} diff --git a/test/integration/node_pools_test.go b/test/integration/node_pools_test.go index e7225394..bba22a0b 100644 --- a/test/integration/node_pools_test.go +++ b/test/integration/node_pools_test.go @@ -707,3 +707,238 @@ func TestNodePoolSoftDelete(t *testing.T) { Expect(resp.StatusCode()).To(Equal(http.StatusNotFound)) }) } + +func TestNodePoolHardDelete(t *testing.T) { + t.Run("given a soft-deleted nodepool, when all required adapters report Finalized=True, then nodepool and its adapter statuses are hard-deleted from DB", func(t *testing.T) { //nolint:lll + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + nodePool, err := h.Factories.NewNodePools(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + dbSession := h.DBFactory.New(ctx) + err = dbSession.Model(nodePool).Update("owner_id", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + + delResp, err := client.DeleteNodePoolByIdWithResponse(ctx, cluster.ID, nodePool.ID, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(delResp.StatusCode()).To(Equal(http.StatusAccepted)) + newGeneration := delResp.JSON202.Generation + Expect(newGeneration).To(Equal(nodePool.Generation + 1)) + + requiredAdapters := []string{"validation", "hypershift"} + for _, adapter := range requiredAdapters { + statusInput := newAdapterStatusRequest( + adapter, + newGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ManifestWorkNotDiscovered"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("NamespaceNotDiscovered"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + { + Type: api.ConditionTypeFinalized, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("CleanupConfirmed"), + }, + }, + nil, + ) + statusResp, loopErr := client.PostNodePoolStatusesWithResponse( + ctx, cluster.ID, nodePool.ID, + openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(loopErr).NotTo(HaveOccurred()) + Expect(statusResp.StatusCode()).To(Equal(http.StatusCreated)) + } + + var nodePoolCheck api.NodePool + dbErr := dbSession.First(&nodePoolCheck, "id = ?", nodePool.ID).Error + Expect(dbErr).To(HaveOccurred(), "Nodepool should be hard-deleted from DB") + Expect(dbErr.Error()).To(ContainSubstring("record not found")) + + var adapterStatuses []api.AdapterStatus + err = dbSession.Where("resource_type = ? AND resource_id = ?", "NodePool", nodePool.ID).Find(&adapterStatuses).Error + Expect(err).NotTo(HaveOccurred()) + Expect(adapterStatuses).To(BeEmpty(), "Adapter statuses should be hard-deleted") + + var clusterCheck api.Cluster + err = dbSession.First(&clusterCheck, "id = ?", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + }) + + t.Run("given a soft-deleted nodepool, when adapters report Finalized=False, then nodepool remains in DB", func(t *testing.T) { //nolint:lll + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + nodePool, err := h.Factories.NewNodePools(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + dbSession := h.DBFactory.New(ctx) + err = dbSession.Model(nodePool).Update("owner_id", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + + delResp, err := client.DeleteNodePoolByIdWithResponse(ctx, cluster.ID, nodePool.ID, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(delResp.StatusCode()).To(Equal(http.StatusAccepted)) + newGeneration := delResp.JSON202.Generation + + statusInput := newAdapterStatusRequest( + "validation", + newGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("CleanupInProgress"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ResourcesStillExist"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + {Type: api.ConditionTypeFinalized, Status: openapi.AdapterConditionStatusFalse, Reason: util.PtrString("")}, + }, + nil, + ) + statusResp, err := client.PostNodePoolStatusesWithResponse( + ctx, cluster.ID, nodePool.ID, + openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(statusResp.StatusCode()).To(Equal(http.StatusCreated)) + + var nodePoolCheck api.NodePool + err = dbSession.First(&nodePoolCheck, "id = ?", nodePool.ID).Error + Expect(err).NotTo(HaveOccurred()) + Expect(nodePoolCheck.DeletedTime).NotTo(BeNil(), "Nodepool should still be soft-deleted") + }) + + t.Run("given a soft-deleted nodepool, when only one of the required adapters reports Finalized=True, then nodepool remains and partial adapter status is stored", func(t *testing.T) { //nolint:lll + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + nodePool, err := h.Factories.NewNodePools(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + dbSession := h.DBFactory.New(ctx) + err = dbSession.Model(nodePool).Update("owner_id", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + + delResp, err := client.DeleteNodePoolByIdWithResponse(ctx, cluster.ID, nodePool.ID, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(delResp.StatusCode()).To(Equal(http.StatusAccepted)) + newGeneration := delResp.JSON202.Generation + + statusInput := newAdapterStatusRequest( + "validation", + newGeneration, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("ManifestWorkNotDiscovered"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusFalse, + Reason: util.PtrString("NamespaceNotDiscovered"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + { + Type: api.ConditionTypeFinalized, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("CleanupConfirmed"), + }, + }, + nil, + ) + statusResp, err := client.PostNodePoolStatusesWithResponse( + ctx, cluster.ID, nodePool.ID, + openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(statusResp.StatusCode()).To(Equal(http.StatusCreated)) + + var nodePoolCheck api.NodePool + err = dbSession.First(&nodePoolCheck, "id = ?", nodePool.ID).Error + Expect(err).NotTo(HaveOccurred()) + + var adapterStatus api.AdapterStatus + err = dbSession.Where("resource_type = ? AND resource_id = ? AND adapter = ?", + "NodePool", nodePool.ID, "validation").First(&adapterStatus).Error + Expect(err).NotTo(HaveOccurred()) + Expect(adapterStatus.Adapter).To(Equal("validation"), "Adapter status should be stored") + }) + + t.Run("given a nodepool that is not soft-deleted, when all adapters report Finalized=True, then nodepool is not hard-deleted", func(t *testing.T) { //nolint:lll + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + nodePool, err := h.Factories.NewNodePools(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + dbSession := h.DBFactory.New(ctx) + err = dbSession.Model(nodePool).Update("owner_id", cluster.ID).Error + Expect(err).NotTo(HaveOccurred()) + + requiredAdapters := []string{"validation", "hypershift"} + for _, adapter := range requiredAdapters { + statusInput := newAdapterStatusRequest( + adapter, + nodePool.Generation, + []openapi.ConditionRequest{ + { + Type: api.ConditionTypeApplied, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("AppliedManifestWorkComplete"), + }, + { + Type: api.ConditionTypeAvailable, + Status: openapi.AdapterConditionStatusTrue, + Reason: util.PtrString("AllResourcesAvailable"), + }, + {Type: api.ConditionTypeHealth, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("Healthy")}, + {Type: api.ConditionTypeFinalized, Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("")}, + }, + nil, + ) + statusResp, loopErr := client.PostNodePoolStatusesWithResponse( + ctx, cluster.ID, nodePool.ID, + openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(loopErr).NotTo(HaveOccurred()) + Expect(statusResp.StatusCode()).To(Equal(http.StatusCreated)) + } + + var nodePoolCheck api.NodePool + err = dbSession.First(&nodePoolCheck, "id = ?", nodePool.ID).Error + Expect(err).NotTo(HaveOccurred()) + Expect(nodePoolCheck.DeletedTime).To(BeNil(), "Nodepool should not be soft-deleted") + + var adapterStatuses []api.AdapterStatus + err = dbSession.Where("resource_type = ? AND resource_id = ?", "NodePool", nodePool.ID).Find(&adapterStatuses).Error + Expect(err).NotTo(HaveOccurred()) + Expect(adapterStatuses).To(HaveLen(2)) + }) +} From 582e642a0b3dc860258c21bb20cc0a1de31dfb5d Mon Sep 17 00:00:00 2001 From: Martin Liptak Date: Thu, 30 Apr 2026 16:34:59 +0200 Subject: [PATCH 2/2] HYPERFLEET-854 - fix: changelog links --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64124af7..c2c0b07e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Hard deletion for Clusters and NodePools: resources and their adapter statuses are permanently removed from the database once all required adapters report `Finalized=True` and no child resources remain ([#HYPERFLEET-854](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/HYPERFLEET-854)) -- `Finalized` condition aggregation with `WaitingForChildResources` intermediate state when all adapters are finalized but child node pools still exist ([#HYPERFLEET-854](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/HYPERFLEET-854)) +- Hard deletion for Clusters and NodePools: resources and their adapter statuses are permanently removed from the database once all required adapters report `Finalized=True` and no child resources remain ([#119](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/119)) +- `Finalized` condition aggregation with `WaitingForChildResources` intermediate state when all adapters are finalized but child node pools still exist ([#119](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/119)) - Soft deletion for Clusters and NodePools with `deleted_time` and `deleted_by` fields for tracking deletion requests ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106)) - Aggregation logic for resource data ([#91](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/91)) - Version subcommand to CLI ([#84](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/84)) @@ -28,7 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Refactored `AdapterStatusDao.Upsert()` to accept a pre-fetched existing record, moving lookup and `LastTransitionTime` preservation logic to the service layer ([#HYPERFLEET-854](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/HYPERFLEET-854)) +- Refactored `AdapterStatusDao.Upsert()` to accept a pre-fetched existing record, moving lookup and `LastTransitionTime` preservation logic to the service layer ([#119](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/119)) - Refactored DAO methods to remove Unscoped calls for fetching Clusters and NodePools ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106)) - Bumped oapi-codegen version to fix missing `omitempty` on generated response objects ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106)) - Updated OpenAPI spec with examples for Cluster and NodePool schemas ([#106](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/106))