Skip to content

Commit 12333a0

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #112 from mliptak0/HYPERFLEET-994
HYPERFLEET-994 - fix: recompute cascade nodepool status on cluster soft-delete
2 parents a837b95 + d7afbfd commit 12333a0

12 files changed

Lines changed: 393 additions & 45 deletions

‎pkg/dao/adapter_status.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ type AdapterStatusDao interface {
2222
Upsert(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error)
2323
Delete(ctx context.Context, id string) error
2424
FindByResource(ctx context.Context, resourceType, resourceID string) (api.AdapterStatusList, error)
25+
FindByResourceIDs(ctx context.Context, resourceType string, resourceIDs []string) (api.AdapterStatusList, error)
2526
FindByResourcePaginated(
2627
ctx context.Context, resourceType, resourceID string, offset, limit int,
2728
) (api.AdapterStatusList, int64, error)
@@ -161,6 +162,21 @@ func (d *sqlAdapterStatusDao) FindByResource(
161162
return statuses, nil
162163
}
163164

165+
func (d *sqlAdapterStatusDao) FindByResourceIDs(
166+
ctx context.Context, resourceType string, resourceIDs []string,
167+
) (api.AdapterStatusList, error) {
168+
g2 := (*d.sessionFactory).New(ctx)
169+
statuses := api.AdapterStatusList{}
170+
if len(resourceIDs) == 0 {
171+
return statuses, nil
172+
}
173+
query := g2.Where("resource_type = ? AND resource_id IN ?", resourceType, resourceIDs)
174+
if err := query.Find(&statuses).Error; err != nil {
175+
return nil, err
176+
}
177+
return statuses, nil
178+
}
179+
164180
func (d *sqlAdapterStatusDao) FindByResourcePaginated(
165181
ctx context.Context, resourceType, resourceID string, offset, limit int,
166182
) (api.AdapterStatusList, int64, error) {

‎pkg/dao/mocks/node_pool.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,18 @@ func (d *nodePoolDaoMock) SoftDeleteByOwner(ctx context.Context, ownerID string,
4848
return errors.NotImplemented("NodePool").AsError()
4949
}
5050

51+
func (d *nodePoolDaoMock) FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) {
52+
return nil, errors.NotImplemented("NodePool").AsError()
53+
}
54+
5155
func (d *nodePoolDaoMock) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) {
5256
return nil, errors.NotImplemented("NodePool").AsError()
5357
}
5458

59+
func (d *nodePoolDaoMock) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error {
60+
return errors.NotImplemented("NodePool").AsError()
61+
}
62+
5563
func (d *nodePoolDaoMock) All(ctx context.Context) (api.NodePoolList, error) {
5664
return d.nodePools, nil
5765
}

‎pkg/dao/node_pool.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@ type NodePoolDao interface {
1919
Save(ctx context.Context, nodePool *api.NodePool) error
2020
Delete(ctx context.Context, id string) error
2121
FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error)
22+
FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error)
2223
SoftDeleteByOwner(ctx context.Context, ownerID string, t time.Time, deletedBy string) error
24+
UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error
2325
All(ctx context.Context) (api.NodePoolList, error)
2426
}
2527

@@ -111,6 +113,15 @@ func (d *sqlNodePoolDao) SoftDeleteByOwner(ctx context.Context, ownerID string,
111113
return nil
112114
}
113115

116+
func (d *sqlNodePoolDao) FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) {
117+
g2 := (*d.sessionFactory).New(ctx)
118+
var nodePools api.NodePoolList
119+
if err := g2.Where("owner_id = ? AND deleted_time IS NOT NULL", ownerID).Find(&nodePools).Error; err != nil {
120+
return nil, err
121+
}
122+
return nodePools, nil
123+
}
124+
114125
func (d *sqlNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) {
115126
g2 := (*d.sessionFactory).New(ctx)
116127
nodePools := api.NodePoolList{}
@@ -120,6 +131,24 @@ func (d *sqlNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodeP
120131
return nodePools, nil
121132
}
122133

134+
func (d *sqlNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error {
135+
g2 := (*d.sessionFactory).New(ctx)
136+
if len(updates) == 0 {
137+
return nil
138+
}
139+
140+
for id, statusConditions := range updates {
141+
result := g2.Model(&api.NodePool{}).
142+
Where("id = ?", id).
143+
Update("status_conditions", statusConditions)
144+
if result.Error != nil {
145+
db.MarkForRollback(ctx, result.Error)
146+
return result.Error
147+
}
148+
}
149+
return nil
150+
}
151+
123152
func (d *sqlNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) {
124153
g2 := (*d.sessionFactory).New(ctx)
125154
nodePools := api.NodePoolList{}

‎pkg/db/migrations/202604230001_add_nodepool_owner_deleted_index.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package migrations
2+
3+
// Migrations should NEVER use types from other packages. Types can change
4+
// and then migrations run on a _new_ database will fail or behave unexpectedly.
5+
// Instead of importing types, always re-create the type in the migration, as
6+
// is done here, even though the same type is defined in pkg/api
7+
8+
import (
9+
"gorm.io/gorm"
10+
11+
"github.com/go-gormigrate/gormigrate/v2"
12+
)
13+
14+
func addNodePoolOwnerDeletedIndex() *gormigrate.Migration {
15+
return &gormigrate.Migration{
16+
ID: "202604230001",
17+
Migrate: func(tx *gorm.DB) error {
18+
// Add composite index on (owner_id, deleted_time) for efficient queries
19+
// when fetching soft-deleted nodepools by owner.
20+
if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_node_pools_owner_deleted ON node_pools(owner_id, deleted_time);").Error; err != nil { //nolint:lll
21+
return err
22+
}
23+
return nil
24+
},
25+
}
26+
}

‎pkg/db/migrations/migration_structs.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ var MigrationList = []*gormigrate.Migration{
3333
addAdapterStatus(),
3434
addConditionsGinIndex(),
3535
addSoftDeleteSchema(),
36+
addNodePoolOwnerDeletedIndex(),
3637
}
3738

3839
// Model represents the base model struct. All entities will have this struct embedded.

‎pkg/services/cluster.go‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,19 +124,34 @@ func (s *sqlClusterService) SoftDelete(ctx context.Context, id string) (*api.Clu
124124
cluster.DeletedBy = &deletedBy
125125
cluster.Generation++
126126

127-
if err := s.clusterDao.Save(ctx, cluster); err != nil {
128-
return nil, handleSoftDeleteError("Cluster", err)
127+
if saveErr := s.clusterDao.Save(ctx, cluster); saveErr != nil {
128+
return nil, handleSoftDeleteError("Cluster", saveErr)
129129
}
130130

131-
if err := s.nodePoolDao.SoftDeleteByOwner(ctx, id, t, deletedBy); err != nil {
132-
return nil, handleSoftDeleteError("NodePool", err)
131+
if cascadeErr := s.nodePoolDao.SoftDeleteByOwner(ctx, id, t, deletedBy); cascadeErr != nil {
132+
return nil, handleSoftDeleteError("NodePool", cascadeErr)
133133
}
134134

135135
cluster, svcErr := s.UpdateClusterStatusFromAdapters(ctx, cluster.ID)
136136
if svcErr != nil {
137137
return nil, svcErr
138138
}
139139

140+
// Update status for all cascade-deleted nodepools so their Ready condition reflects the generation bump.
141+
nodePools, err := s.nodePoolDao.FindSoftDeletedByOwner(ctx, id)
142+
if err != nil {
143+
return nil, errors.GeneralError("Failed to fetch cascade-deleted nodepools: %s", err)
144+
}
145+
if svcErr := batchUpdateNodePoolStatusesFromAdapters(
146+
ctx,
147+
nodePools,
148+
s.nodePoolDao,
149+
s.adapterStatusDao,
150+
s.adapterConfig,
151+
); svcErr != nil {
152+
return nil, svcErr
153+
}
154+
140155
return cluster, nil
141156
}
142157

‎pkg/services/cluster_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,24 @@ func (d *mockAdapterStatusDao) FindByResource(
164164
return result, nil
165165
}
166166

167+
func (d *mockAdapterStatusDao) FindByResourceIDs(
168+
ctx context.Context,
169+
resourceType string,
170+
resourceIDs []string,
171+
) (api.AdapterStatusList, error) {
172+
var result api.AdapterStatusList
173+
resourceIDSet := make(map[string]bool, len(resourceIDs))
174+
for _, id := range resourceIDs {
175+
resourceIDSet[id] = true
176+
}
177+
for _, s := range d.statuses {
178+
if s.ResourceType == resourceType && resourceIDSet[s.ResourceID] {
179+
result = append(result, s)
180+
}
181+
}
182+
return result, nil
183+
}
184+
167185
func (d *mockAdapterStatusDao) FindByResourcePaginated(
168186
ctx context.Context,
169187
resourceType, resourceID string,

‎pkg/services/node_pool.go‎

Lines changed: 7 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package services
22

33
import (
4-
"bytes"
54
"context"
65
"encoding/json"
76
stderrors "errors"
@@ -182,46 +181,13 @@ func nodePoolRefTime(np *api.NodePool) time.Time {
182181
func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters(
183182
ctx context.Context, nodePoolID string,
184183
) (*api.NodePool, *errors.ServiceError) {
185-
nodePool, err := s.nodePoolDao.Get(ctx, nodePoolID)
186-
if err != nil {
187-
return nil, handleGetError("NodePool", "id", nodePoolID, err)
188-
}
189-
190-
adapterStatuses, err := s.adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID)
191-
if err != nil {
192-
return nil, errors.GeneralError("Failed to get adapter statuses: %s", err)
193-
}
194-
195-
refTime := nodePoolRefTime(nodePool)
196-
ready, available, adapterConditions := AggregateResourceStatus(ctx, AggregateResourceStatusInput{
197-
ResourceGeneration: nodePool.Generation,
198-
RefTime: refTime,
199-
PrevConditionsJSON: nodePool.StatusConditions,
200-
RequiredAdapters: s.adapterConfig.RequiredNodePoolAdapters(),
201-
AdapterStatuses: adapterStatuses,
202-
})
203-
204-
allConditions := make([]api.ResourceCondition, 0, 2+len(adapterConditions))
205-
allConditions = append(allConditions, ready, available)
206-
allConditions = append(allConditions, adapterConditions...)
207-
208-
conditionsJSON, err := json.Marshal(allConditions)
209-
if err != nil {
210-
return nil, errors.GeneralError("Failed to marshal conditions: %s", err)
211-
}
212-
213-
if bytes.Equal(nodePool.StatusConditions, conditionsJSON) {
214-
return nodePool, nil
215-
}
216-
217-
nodePool.StatusConditions = conditionsJSON
218-
219-
nodePool, err = s.nodePoolDao.Replace(ctx, nodePool)
220-
if err != nil {
221-
return nil, handleUpdateError("NodePool", err)
222-
}
223-
224-
return nodePool, nil
184+
return updateNodePoolStatusFromAdapters(
185+
ctx,
186+
nodePoolID,
187+
s.nodePoolDao,
188+
s.adapterStatusDao,
189+
s.adapterConfig,
190+
)
225191
}
226192

227193
func (s *sqlNodePoolService) ProcessAdapterStatus(

‎pkg/services/node_pool_test.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,16 @@ func (d *mockNodePoolDao) SoftDeleteByOwner(ctx context.Context, ownerID string,
8787
return nil
8888
}
8989

90+
func (d *mockNodePoolDao) FindSoftDeletedByOwner(ctx context.Context, ownerID string) (api.NodePoolList, error) {
91+
var result api.NodePoolList
92+
for _, np := range d.nodePools {
93+
if np.OwnerID == ownerID && np.DeletedTime != nil {
94+
result = append(result, np)
95+
}
96+
}
97+
return result, nil
98+
}
99+
90100
func (d *mockNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) {
91101
var result api.NodePoolList
92102
for _, id := range ids {
@@ -97,6 +107,16 @@ func (d *mockNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.Node
97107
return result, nil
98108
}
99109

110+
func (d *mockNodePoolDao) UpdateStatusConditionsByIDs(ctx context.Context, updates map[string][]byte) error {
111+
for id, statusConditions := range updates {
112+
if np, ok := d.nodePools[id]; ok {
113+
np.StatusConditions = statusConditions
114+
d.nodePools[id] = np
115+
}
116+
}
117+
return nil
118+
}
119+
100120
func (d *mockNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) {
101121
var result api.NodePoolList
102122
for _, np := range d.nodePools {

0 commit comments

Comments
 (0)