From 9b286f3622abcd3db67fbca512941e4e5df8e598 Mon Sep 17 00:00:00 2001 From: Malte Viering Date: Tue, 31 Mar 2026 11:15:11 +0200 Subject: [PATCH 1/3] Remove duplicated nova servers --- .../plugins/openstack/nova/nova_api.go | 10 ++- .../plugins/openstack/nova/nova_api_test.go | 86 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go b/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go index ddaef8aa4..7b9a3a3e8 100644 --- a/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go +++ b/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go @@ -95,6 +95,7 @@ func (api *novaAPI) GetAllServers(ctx context.Context) ([]Server, error) { initialURL := api.sc.Endpoint + "servers/detail?all_tenants=true" var nextURL = &initialURL var allServers []Server + seen := make(map[string]bool) for nextURL != nil { req, err := http.NewRequestWithContext(ctx, http.MethodGet, *nextURL, http.NoBody) @@ -122,7 +123,14 @@ func (api *novaAPI) GetAllServers(ctx context.Context) ([]Server, error) { if err != nil { return nil, err } - allServers = append(allServers, list.Servers...) + for _, s := range list.Servers { + if seen[s.ID] { + slog.Warn("skipping duplicate server", "id", s.ID) + continue + } + seen[s.ID] = true + allServers = append(allServers, s) + } nextURL = nil for _, link := range list.Links { if link.Rel == "next" { diff --git a/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go b/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go index 80c95a7d1..734ab8088 100644 --- a/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go +++ b/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -122,6 +123,91 @@ func TestNovaAPI_GetAllServers(t *testing.T) { } } +func TestNovaAPI_GetAllServers_DeduplicatesServers(t *testing.T) { + tests := []struct { + name string + responses []string + expectedCount int + expectedServers []string + }{ + { + name: "duplicates within same page", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "server1", "flavor": {"id": "1"}}, + {"id": "bbb", "name": "server2", "flavor": {"id": "1"}}, + {"id": "aaa", "name": "server1-dup", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedServers: []string{"aaa", "bbb"}, + }, + { + name: "duplicates across pages", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "server1", "flavor": {"id": "1"}} + ], "servers_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"servers": [ + {"id": "aaa", "name": "server1-dup", "flavor": {"id": "1"}}, + {"id": "bbb", "name": "server2", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedServers: []string{"aaa", "bbb"}, + }, + { + name: "no duplicates", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "server1", "flavor": {"id": "1"}}, + {"id": "bbb", "name": "server2", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedServers: []string{"aaa", "bbb"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + callCount := 0 + handler := func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte(tt.responses[callCount])); err != nil { + t.Fatalf("failed to write response: %v", err) + } + callCount++ + } + srv, k := setupNovaMockServer(handler) + defer srv.Close() + + // Patch NEXT_URL placeholder with actual server URL. + for i := range tt.responses { + tt.responses[i] = strings.ReplaceAll(tt.responses[i], "NEXT_URL", srv.URL+"/servers/detail?page=2") + } + + api := NewNovaAPI(datasources.Monitor{}, k, v1alpha1.NovaDatasource{Type: v1alpha1.NovaDatasourceTypeServers}).(*novaAPI) + if err := api.Init(t.Context()); err != nil { + t.Fatalf("failed to init nova api: %v", err) + } + + servers, err := api.GetAllServers(t.Context()) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(servers) != tt.expectedCount { + t.Fatalf("expected %d servers, got %d", tt.expectedCount, len(servers)) + } + for i, id := range tt.expectedServers { + if servers[i].ID != id { + t.Fatalf("expected server[%d].ID = %s, got %s", i, id, servers[i].ID) + } + } + }) + } +} + func TestNovaAPI_GetAllHypervisors(t *testing.T) { handler := func(w http.ResponseWriter, r *http.Request) { // changes-since is not supported by the hypervisor api so From dad1f819eff9aaa1e788c0f036dc808c0bdd1a9e Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Tue, 31 Mar 2026 11:23:09 +0200 Subject: [PATCH 2/3] Use ideomatic/safe way to check seen servers --- .../datasources/plugins/openstack/nova/nova_api.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go b/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go index 7b9a3a3e8..98175ae2d 100644 --- a/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go +++ b/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go @@ -95,7 +95,7 @@ func (api *novaAPI) GetAllServers(ctx context.Context) ([]Server, error) { initialURL := api.sc.Endpoint + "servers/detail?all_tenants=true" var nextURL = &initialURL var allServers []Server - seen := make(map[string]bool) + seen := make(map[string]struct{}) for nextURL != nil { req, err := http.NewRequestWithContext(ctx, http.MethodGet, *nextURL, http.NoBody) @@ -124,11 +124,11 @@ func (api *novaAPI) GetAllServers(ctx context.Context) ([]Server, error) { return nil, err } for _, s := range list.Servers { - if seen[s.ID] { + if _, ok := seen[s.ID]; ok { slog.Warn("skipping duplicate server", "id", s.ID) continue } - seen[s.ID] = true + seen[s.ID] = struct{}{} allServers = append(allServers, s) } nextURL = nil From 800f9a6cb3d40f2ffd452d01038345543d9e94c4 Mon Sep 17 00:00:00 2001 From: Malte Viering Date: Tue, 31 Mar 2026 11:37:54 +0200 Subject: [PATCH 3/3] Add it to other ep as well --- .../plugins/openstack/nova/nova_api.go | 30 +- .../plugins/openstack/nova/nova_api_test.go | 306 +++++++++++++++++- 2 files changed, 332 insertions(+), 4 deletions(-) diff --git a/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go b/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go index 98175ae2d..2f41c54a6 100644 --- a/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go +++ b/internal/knowledge/datasources/plugins/openstack/nova/nova_api.go @@ -163,6 +163,7 @@ func (api *novaAPI) GetDeletedServers(ctx context.Context, since time.Time) ([]D initialURL := api.sc.Endpoint + "servers/detail?status=DELETED&all_tenants=true&changes-since=" + url.QueryEscape(since.Format(time.RFC3339)) var nextURL = &initialURL var deletedServers []DeletedServer + seen := make(map[string]struct{}) for nextURL != nil { req, err := http.NewRequestWithContext(ctx, http.MethodGet, *nextURL, http.NoBody) @@ -190,7 +191,14 @@ func (api *novaAPI) GetDeletedServers(ctx context.Context, since time.Time) ([]D if err != nil { return nil, err } - deletedServers = append(deletedServers, list.Servers...) + for _, s := range list.Servers { + if _, ok := seen[s.ID]; ok { + slog.Warn("skipping duplicate deleted server", "id", s.ID) + continue + } + seen[s.ID] = struct{}{} + deletedServers = append(deletedServers, s) + } nextURL = nil for _, link := range list.Links { if link.Rel == "next" { @@ -219,6 +227,7 @@ func (api *novaAPI) GetAllHypervisors(ctx context.Context) ([]Hypervisor, error) initialURL := api.sc.Endpoint + "os-hypervisors/detail" var nextURL = &initialURL var hypervisors []Hypervisor + seen := make(map[string]struct{}) for nextURL != nil { req, err := http.NewRequestWithContext(ctx, http.MethodGet, *nextURL, http.NoBody) if err != nil { @@ -245,7 +254,14 @@ func (api *novaAPI) GetAllHypervisors(ctx context.Context) ([]Hypervisor, error) if err != nil { return nil, err } - hypervisors = append(hypervisors, list.Hypervisors...) + for _, h := range list.Hypervisors { + if _, ok := seen[h.ID]; ok { + slog.Warn("skipping duplicate hypervisor", "id", h.ID) + continue + } + seen[h.ID] = struct{}{} + hypervisors = append(hypervisors, h) + } nextURL = nil for _, link := range list.Links { if link.Rel == "next" { @@ -309,6 +325,7 @@ func (api *novaAPI) GetAllMigrations(ctx context.Context) ([]Migration, error) { initialURL := api.sc.Endpoint + "os-migrations" var nextURL = &initialURL var migrations []Migration + seen := make(map[string]struct{}) for nextURL != nil { req, err := http.NewRequestWithContext(ctx, http.MethodGet, *nextURL, http.NoBody) if err != nil { @@ -336,6 +353,14 @@ func (api *novaAPI) GetAllMigrations(ctx context.Context) ([]Migration, error) { if err != nil { return nil, err } + for _, m := range list.Migrations { + if _, ok := seen[m.UUID]; ok { + slog.Warn("skipping duplicate migration", "uuid", m.UUID) + continue + } + seen[m.UUID] = struct{}{} + migrations = append(migrations, m) + } nextURL = nil for _, link := range list.Links { if link.Rel == "next" { @@ -343,7 +368,6 @@ func (api *novaAPI) GetAllMigrations(ctx context.Context) ([]Migration, error) { break } } - migrations = append(migrations, list.Migrations...) } slog.Info("fetched", "label", label, "count", len(migrations)) return migrations, nil diff --git a/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go b/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go index 734ab8088..49f0af4b4 100644 --- a/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go +++ b/internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go @@ -157,7 +157,7 @@ func TestNovaAPI_GetAllServers_DeduplicatesServers(t *testing.T) { expectedServers: []string{"aaa", "bbb"}, }, { - name: "no duplicates", + name: "no duplicates single page", responses: []string{ `{"servers": [ {"id": "aaa", "name": "server1", "flavor": {"id": "1"}}, @@ -167,6 +167,19 @@ func TestNovaAPI_GetAllServers_DeduplicatesServers(t *testing.T) { expectedCount: 2, expectedServers: []string{"aaa", "bbb"}, }, + { + name: "no duplicates across pages", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "server1", "flavor": {"id": "1"}} + ], "servers_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"servers": [ + {"id": "bbb", "name": "server2", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedServers: []string{"aaa", "bbb"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -330,3 +343,294 @@ func TestNovaAPI_GetAllMigrations(t *testing.T) { t.Errorf("unexpected migration data: %+v", migrations[0]) } } + +func TestNovaAPI_GetDeletedServers_DeduplicatesServers(t *testing.T) { + tests := []struct { + name string + responses []string + expectedCount int + expectedIDs []string + }{ + { + name: "duplicates within same page", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "s1", "status": "DELETED", "flavor": {"id": "1"}}, + {"id": "bbb", "name": "s2", "status": "DELETED", "flavor": {"id": "1"}}, + {"id": "aaa", "name": "s1-dup", "status": "DELETED", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + { + name: "duplicates across pages", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "s1", "status": "DELETED", "flavor": {"id": "1"}} + ], "servers_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"servers": [ + {"id": "aaa", "name": "s1-dup", "status": "DELETED", "flavor": {"id": "1"}}, + {"id": "bbb", "name": "s2", "status": "DELETED", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + { + name: "no duplicates single page", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "s1", "status": "DELETED", "flavor": {"id": "1"}}, + {"id": "bbb", "name": "s2", "status": "DELETED", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + { + name: "no duplicates across pages", + responses: []string{ + `{"servers": [ + {"id": "aaa", "name": "s1", "status": "DELETED", "flavor": {"id": "1"}} + ], "servers_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"servers": [ + {"id": "bbb", "name": "s2", "status": "DELETED", "flavor": {"id": "1"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + callCount := 0 + handler := func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte(tt.responses[callCount])); err != nil { + t.Fatalf("failed to write response: %v", err) + } + callCount++ + } + srv, k := setupNovaMockServer(handler) + defer srv.Close() + + for i := range tt.responses { + tt.responses[i] = strings.ReplaceAll(tt.responses[i], "NEXT_URL", srv.URL+"/servers/detail?page=2") + } + + api := NewNovaAPI(datasources.Monitor{}, k, v1alpha1.NovaDatasource{}).(*novaAPI) + if err := api.Init(t.Context()); err != nil { + t.Fatalf("failed to init nova api: %v", err) + } + + servers, err := api.GetDeletedServers(t.Context(), time.Now().Add(-6*time.Hour)) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(servers) != tt.expectedCount { + t.Fatalf("expected %d servers, got %d", tt.expectedCount, len(servers)) + } + for i, id := range tt.expectedIDs { + if servers[i].ID != id { + t.Fatalf("expected server[%d].ID = %s, got %s", i, id, servers[i].ID) + } + } + }) + } +} + +func TestNovaAPI_GetAllHypervisors_DeduplicatesHypervisors(t *testing.T) { + tests := []struct { + name string + responses []string + expectedCount int + expectedIDs []string + }{ + { + name: "duplicates within same page", + responses: []string{ + `{"hypervisors": [ + {"id": "aaa", "hypervisor_hostname": "h1", "cpu_info": {}, "service": {"id": "s1", "host": "h1"}}, + {"id": "bbb", "hypervisor_hostname": "h2", "cpu_info": {}, "service": {"id": "s2", "host": "h2"}}, + {"id": "aaa", "hypervisor_hostname": "h1-dup", "cpu_info": {}, "service": {"id": "s1", "host": "h1"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + { + name: "duplicates across pages", + responses: []string{ + `{"hypervisors": [ + {"id": "aaa", "hypervisor_hostname": "h1", "cpu_info": {}, "service": {"id": "s1", "host": "h1"}} + ], "hypervisors_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"hypervisors": [ + {"id": "aaa", "hypervisor_hostname": "h1-dup", "cpu_info": {}, "service": {"id": "s1", "host": "h1"}}, + {"id": "bbb", "hypervisor_hostname": "h2", "cpu_info": {}, "service": {"id": "s2", "host": "h2"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + { + name: "no duplicates single page", + responses: []string{ + `{"hypervisors": [ + {"id": "aaa", "hypervisor_hostname": "h1", "cpu_info": {}, "service": {"id": "s1", "host": "h1"}}, + {"id": "bbb", "hypervisor_hostname": "h2", "cpu_info": {}, "service": {"id": "s2", "host": "h2"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + { + name: "no duplicates across pages", + responses: []string{ + `{"hypervisors": [ + {"id": "aaa", "hypervisor_hostname": "h1", "cpu_info": {}, "service": {"id": "s1", "host": "h1"}} + ], "hypervisors_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"hypervisors": [ + {"id": "bbb", "hypervisor_hostname": "h2", "cpu_info": {}, "service": {"id": "s2", "host": "h2"}} + ]}`, + }, + expectedCount: 2, + expectedIDs: []string{"aaa", "bbb"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + callCount := 0 + handler := func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte(tt.responses[callCount])); err != nil { + t.Fatalf("failed to write response: %v", err) + } + callCount++ + } + srv, k := setupNovaMockServer(handler) + defer srv.Close() + + for i := range tt.responses { + tt.responses[i] = strings.ReplaceAll(tt.responses[i], "NEXT_URL", srv.URL+"/os-hypervisors/detail?page=2") + } + + api := NewNovaAPI(datasources.Monitor{}, k, v1alpha1.NovaDatasource{}).(*novaAPI) + if err := api.Init(t.Context()); err != nil { + t.Fatalf("failed to init nova api: %v", err) + } + + hypervisors, err := api.GetAllHypervisors(t.Context()) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(hypervisors) != tt.expectedCount { + t.Fatalf("expected %d hypervisors, got %d", tt.expectedCount, len(hypervisors)) + } + for i, id := range tt.expectedIDs { + if hypervisors[i].ID != id { + t.Fatalf("expected hypervisor[%d].ID = %s, got %s", i, id, hypervisors[i].ID) + } + } + }) + } +} + +func TestNovaAPI_GetAllMigrations_DeduplicatesMigrations(t *testing.T) { + tests := []struct { + name string + responses []string + expectedCount int + expectedUUIDs []string + }{ + { + name: "duplicates within same page", + responses: []string{ + `{"migrations": [ + {"id": 1, "uuid": "aaa", "status": "completed"}, + {"id": 2, "uuid": "bbb", "status": "completed"}, + {"id": 1, "uuid": "aaa", "status": "completed"} + ]}`, + }, + expectedCount: 2, + expectedUUIDs: []string{"aaa", "bbb"}, + }, + { + name: "duplicates across pages", + responses: []string{ + `{"migrations": [ + {"id": 1, "uuid": "aaa", "status": "completed"} + ], "migrations_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"migrations": [ + {"id": 1, "uuid": "aaa", "status": "completed"}, + {"id": 2, "uuid": "bbb", "status": "completed"} + ]}`, + }, + expectedCount: 2, + expectedUUIDs: []string{"aaa", "bbb"}, + }, + { + name: "no duplicates single page", + responses: []string{ + `{"migrations": [ + {"id": 1, "uuid": "aaa", "status": "completed"}, + {"id": 2, "uuid": "bbb", "status": "completed"} + ]}`, + }, + expectedCount: 2, + expectedUUIDs: []string{"aaa", "bbb"}, + }, + { + name: "no duplicates across pages", + responses: []string{ + `{"migrations": [ + {"id": 1, "uuid": "aaa", "status": "completed"} + ], "migrations_links": [{"rel": "next", "href": "NEXT_URL"}]}`, + `{"migrations": [ + {"id": 2, "uuid": "bbb", "status": "completed"} + ]}`, + }, + expectedCount: 2, + expectedUUIDs: []string{"aaa", "bbb"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + callCount := 0 + handler := func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte(tt.responses[callCount])); err != nil { + t.Fatalf("failed to write response: %v", err) + } + callCount++ + } + srv, k := setupNovaMockServer(handler) + defer srv.Close() + + for i := range tt.responses { + tt.responses[i] = strings.ReplaceAll(tt.responses[i], "NEXT_URL", srv.URL+"/os-migrations?page=2") + } + + api := NewNovaAPI(datasources.Monitor{}, k, v1alpha1.NovaDatasource{}).(*novaAPI) + if err := api.Init(t.Context()); err != nil { + t.Fatalf("failed to init nova api: %v", err) + } + + migrations, err := api.GetAllMigrations(t.Context()) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(migrations) != tt.expectedCount { + t.Fatalf("expected %d migrations, got %d", tt.expectedCount, len(migrations)) + } + for i, uuid := range tt.expectedUUIDs { + if migrations[i].UUID != uuid { + t.Fatalf("expected migration[%d].UUID = %s, got %s", i, uuid, migrations[i].UUID) + } + } + }) + } +}