From b4f62f2fc5dde4fa58f103ecbe6685cfc66c17f8 Mon Sep 17 00:00:00 2001 From: Markus Wieland Date: Tue, 31 Mar 2026 10:50:32 +0200 Subject: [PATCH 1/2] Add error handling to ReplaceAll --- internal/knowledge/db/db.go | 26 +++++++++------- internal/knowledge/db/db_test.go | 51 ++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/internal/knowledge/db/db.go b/internal/knowledge/db/db.go index 96a87e62a..498bac742 100644 --- a/internal/knowledge/db/db.go +++ b/internal/knowledge/db/db.go @@ -142,14 +142,16 @@ func (d *DB) SelectTimed(group string, i any, query string, args ...any) ([]any, func (d *DB) CreateTable(table ...*gorp.TableMap) error { tx, err := d.Begin() if err != nil { - slog.Error("failed to begin transaction", "error", err) - return tx.Rollback() + return fmt.Errorf("failed to begin transaction: %w", err) } for _, t := range table { slog.Info("creating table if exists", "table", t.TableName) sql := t.SqlForCreate(true) // true means to add IF NOT EXISTS if _, err := tx.Exec(sql); err != nil { - return tx.Rollback() + if rbErr := tx.Rollback(); rbErr != nil { + slog.Error("failed to rollback transaction", "error", rbErr) + } + return fmt.Errorf("failed to create table %s: %w", t.TableName, err) } } return tx.Commit() @@ -201,20 +203,22 @@ func ReplaceAll[T Table](db DB, objs ...T) error { tableName := model.TableName() tx, err := db.Begin() if err != nil { - slog.Error("failed to begin transaction", "error", err) - return tx.Rollback() + return fmt.Errorf("failed to begin transaction: %w", err) } if _, err = tx.Exec("DELETE FROM " + tableName); err != nil { - slog.Error("failed to delete old objects", "tableName", tableName, "error", err) - return tx.Rollback() + if rbErr := tx.Rollback(); rbErr != nil { + slog.Error("failed to rollback transaction", "error", rbErr) + } + return fmt.Errorf("failed to delete old objects from %s: %w", tableName, err) } if err = BulkInsert(tx, db, objs...); err != nil { - slog.Error("failed to insert new objects", "tableName", tableName, "error", err) - return tx.Rollback() + if rbErr := tx.Rollback(); rbErr != nil { + slog.Error("failed to rollback transaction", "error", rbErr) + } + return fmt.Errorf("failed to insert new objects into %s: %w", tableName, err) } if err = tx.Commit(); err != nil { - slog.Error("failed to commit transaction", "error", err) - return err + return fmt.Errorf("failed to commit transaction: %w", err) } return nil } diff --git a/internal/knowledge/db/db_test.go b/internal/knowledge/db/db_test.go index 5af31b1b6..72ff151ec 100644 --- a/internal/knowledge/db/db_test.go +++ b/internal/knowledge/db/db_test.go @@ -4,6 +4,7 @@ package db import ( + "strings" "testing" "time" @@ -119,6 +120,56 @@ func TestReplaceAll(t *testing.T) { } } +func TestReplaceAllWithDuplicateKeys(t *testing.T) { + dbEnv := testlibDB.SetupDBEnv(t) + db := DB{DbMap: dbEnv.DbMap} + defer dbEnv.Close() + + table := db.AddTable(MockTable{}) + if err := db.CreateTable(table); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + // Insert initial records. + for _, record := range []MockTable{{ID: 1, Name: "old"}} { + if err := db.Insert(&record); err != nil { + t.Fatalf("expected no error, got %v", err) + } + } + + // ReplaceAll with duplicate primary keys in the input. + dupes := []MockTable{ + {ID: 10, Name: "first"}, + {ID: 10, Name: "duplicate"}, + {ID: 20, Name: "other"}, + } + err := ReplaceAll(db, dupes...) + + // The insert should fail because of the duplicate key. + if err == nil { + t.Fatal("expected an error for duplicate keys, got nil") + } + if !strings.Contains(err.Error(), "failed to insert") { + t.Fatalf("expected insert error, got: %v", err) + } + + // The old data must still be intact because the transaction was rolled back. + var count int + if err := db.SelectOne(&count, "SELECT COUNT(*) FROM mock_table"); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if count != 1 { + t.Fatalf("expected 1 record (old data intact after rollback), got %d", count) + } + var name string + if err := db.SelectOne(&name, "SELECT name FROM mock_table WHERE id = 1"); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if name != "old" { + t.Fatalf("expected old data to be intact, got name=%q", name) + } +} + // Test all sorts of data types. type BulkMockTable struct { A int `db:"a,primarykey"` From 5a937619cd7bdfef30b052bc859589df0eb02516 Mon Sep 17 00:00:00 2001 From: Markus Wieland Date: Tue, 31 Mar 2026 11:18:52 +0200 Subject: [PATCH 2/2] Add initialization checks for syncers in Cinder, Limes, Manila, and Placement tests --- .../openstack/cinder/cinder_sync_test.go | 6 ++++ .../openstack/limes/limes_sync_test.go | 8 +++-- .../openstack/manila/manila_sync_test.go | 6 ++++ .../placement/placement_sync_test.go | 30 +++++++++++-------- 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/internal/knowledge/datasources/plugins/openstack/cinder/cinder_sync_test.go b/internal/knowledge/datasources/plugins/openstack/cinder/cinder_sync_test.go index 1cd9e592b..422a36575 100644 --- a/internal/knowledge/datasources/plugins/openstack/cinder/cinder_sync_test.go +++ b/internal/knowledge/datasources/plugins/openstack/cinder/cinder_sync_test.go @@ -55,6 +55,9 @@ func TestCinderSyncer_Sync(t *testing.T) { } ctx := t.Context() + if err := syncer.Init(ctx); err != nil { + t.Fatalf("failed to init cinder syncer: %v", err) + } _, err := syncer.Sync(ctx) if err != nil { t.Fatalf("expected no error, got %v", err) @@ -76,6 +79,9 @@ func TestCinderSyncer_SyncAllStoragePools(t *testing.T) { } ctx := t.Context() + if err := syncer.Init(ctx); err != nil { + t.Fatalf("failed to init cinder syncer: %v", err) + } n, err := syncer.SyncAllStoragePools(ctx) if err != nil { t.Fatalf("expected no error, got %v", err) diff --git a/internal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.go b/internal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.go index 43c92c6b8..06f705e1f 100644 --- a/internal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.go +++ b/internal/knowledge/datasources/plugins/openstack/limes/limes_sync_test.go @@ -125,16 +125,18 @@ func TestLimesSyncer_SyncCommitments(t *testing.T) { } mon := datasources.Monitor{} - k := &testlibKeystone.MockKeystoneClient{} conf := v1alpha1.LimesDatasource{Type: v1alpha1.LimesDatasourceTypeProjectCommitments} syncer := &LimesSyncer{ DB: testDB, Mon: mon, Conf: conf, - API: NewLimesAPI(mon, k, conf), + API: &mockLimesAPI{}, + } + + if err := syncer.Init(t.Context()); err != nil { + t.Fatalf("failed to init limes syncer: %v", err) } - syncer.API = &mockLimesAPI{} ctx := t.Context() n, err := syncer.SyncCommitments(ctx) diff --git a/internal/knowledge/datasources/plugins/openstack/manila/manila_sync_test.go b/internal/knowledge/datasources/plugins/openstack/manila/manila_sync_test.go index 7c2de76a4..95545a8f4 100644 --- a/internal/knowledge/datasources/plugins/openstack/manila/manila_sync_test.go +++ b/internal/knowledge/datasources/plugins/openstack/manila/manila_sync_test.go @@ -55,6 +55,9 @@ func TestManilaSyncer_Sync(t *testing.T) { } ctx := t.Context() + if err := syncer.Init(ctx); err != nil { + t.Fatalf("failed to init manila syncer: %v", err) + } _, err := syncer.Sync(ctx) if err != nil { t.Fatalf("expected no error, got %v", err) @@ -76,6 +79,9 @@ func TestManilaSyncer_SyncAllStoragePools(t *testing.T) { } ctx := t.Context() + if err := syncer.Init(ctx); err != nil { + t.Fatalf("failed to init manila syncer: %v", err) + } n, err := syncer.SyncAllStoragePools(ctx) if err != nil { t.Fatalf("expected no error, got %v", err) diff --git a/internal/knowledge/datasources/plugins/openstack/placement/placement_sync_test.go b/internal/knowledge/datasources/plugins/openstack/placement/placement_sync_test.go index bb01e553e..aa07c7956 100644 --- a/internal/knowledge/datasources/plugins/openstack/placement/placement_sync_test.go +++ b/internal/knowledge/datasources/plugins/openstack/placement/placement_sync_test.go @@ -55,18 +55,19 @@ func TestPlacementSyncer_Sync(t *testing.T) { testDB := db.DB{DbMap: dbEnv.DbMap} defer dbEnv.Close() mon := datasources.Monitor{} - pc := &testlibKeystone.MockKeystoneClient{} conf := v1alpha1.PlacementDatasource{Type: v1alpha1.PlacementDatasourceTypeResourceProviders} syncer := &PlacementSyncer{ DB: testDB, Mon: mon, Conf: conf, - API: NewPlacementAPI(mon, pc, conf), + API: &mockPlacementAPI{}, } - syncer.API = &mockPlacementAPI{} ctx := t.Context() + if err := syncer.Init(ctx); err != nil { + t.Fatalf("failed to init placement syncer: %v", err) + } _, err := syncer.Sync(ctx) if err != nil { t.Fatalf("expected no error, got %v", err) @@ -78,18 +79,19 @@ func TestPlacementSyncer_SyncResourceProviders(t *testing.T) { testDB := db.DB{DbMap: dbEnv.DbMap} defer dbEnv.Close() mon := datasources.Monitor{} - pc := &testlibKeystone.MockKeystoneClient{} conf := v1alpha1.PlacementDatasource{Type: v1alpha1.PlacementDatasourceTypeResourceProviders} syncer := &PlacementSyncer{ DB: testDB, Mon: mon, Conf: conf, - API: NewPlacementAPI(mon, pc, conf), + API: &mockPlacementAPI{}, } - syncer.API = &mockPlacementAPI{} ctx := t.Context() + if err := syncer.Init(ctx); err != nil { + t.Fatalf("failed to init placement syncer: %v", err) + } n, err := syncer.SyncResourceProviders(ctx) if err != nil { t.Fatalf("expected no error, got %v", err) @@ -104,7 +106,6 @@ func TestPlacementSyncer_SyncTraits(t *testing.T) { testDB := db.DB{DbMap: dbEnv.DbMap} defer dbEnv.Close() mon := datasources.Monitor{} - pc := &testlibKeystone.MockKeystoneClient{} conf := v1alpha1.PlacementDatasource{Type: v1alpha1.PlacementDatasourceTypeResourceProviderTraits} rps := []ResourceProvider{{UUID: "1", Name: "rp1"}} @@ -120,9 +121,12 @@ func TestPlacementSyncer_SyncTraits(t *testing.T) { DB: testDB, Mon: mon, Conf: conf, - API: NewPlacementAPI(mon, pc, conf), + API: &mockPlacementAPI{}, + } + + if err := syncer.Init(t.Context()); err != nil { + t.Fatalf("failed to init placement syncer: %v", err) } - syncer.API = &mockPlacementAPI{} ctx := t.Context() // First, we need to sync resource providers to have them in the DB. @@ -140,7 +144,6 @@ func TestPlacementSyncer_SyncInventoryUsages(t *testing.T) { testDB := db.DB{DbMap: dbEnv.DbMap} defer dbEnv.Close() mon := datasources.Monitor{} - pc := &testlibKeystone.MockKeystoneClient{} conf := v1alpha1.PlacementDatasource{Type: v1alpha1.PlacementDatasourceTypeResourceProviderInventoryUsages} rps := []ResourceProvider{{UUID: "1", Name: "rp1"}} @@ -156,9 +159,12 @@ func TestPlacementSyncer_SyncInventoryUsages(t *testing.T) { DB: testDB, Mon: mon, Conf: conf, - API: NewPlacementAPI(mon, pc, conf), + API: &mockPlacementAPI{}, + } + + if err := syncer.Init(t.Context()); err != nil { + t.Fatalf("failed to init placement syncer: %v", err) } - syncer.API = &mockPlacementAPI{} ctx := t.Context() n, err := syncer.SyncInventoryUsages(ctx)