From 8e86b019076c6eb1c9944d44fca66071f813488b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 20:54:21 +0200 Subject: [PATCH] perf(azure database): batched per-server AZConfig lookup, closes #149 Replace the per-server individual AZConfig/Deployment lookup with a batched approach that calls the API once per server list page, reducing API call volume proportionally to the number of SQL servers. Adds MockSQLServersPager and MockSQLManagedInstancesPager test helpers with CallCount tracking. Resolves conflict by including main's TestReservationResourceTypeSQLDB_IsCanonical (H2b regression test for SQLDatabases filter canonical value) alongside the new batched-lookup tests. --- providers/azure/services/database/client.go | 212 ++++++++++++++- .../azure/services/database/client_test.go | 249 ++++++++++++++++++ 2 files changed, 449 insertions(+), 12 deletions(-) diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index cf821009c..50df7c5db 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -70,15 +70,29 @@ type CapabilitiesClient interface { ListByLocation(ctx context.Context, locationName string, options *armsql.CapabilitiesClientListByLocationOptions) (armsql.CapabilitiesClientListByLocationResponse, error) } +// SQLServersPager interface for listing SQL servers (enables mocking). +type SQLServersPager interface { + More() bool + NextPage(ctx context.Context) (armsql.ServersClientListResponse, error) +} + +// SQLManagedInstancesPager interface for listing SQL managed instances (enables mocking). +type SQLManagedInstancesPager interface { + More() bool + NextPage(ctx context.Context) (armsql.ManagedInstancesClientListResponse, error) +} + // DatabaseClient handles Azure SQL Database Reserved Capacity type DatabaseClient struct { - cred azcore.TokenCredential - subscriptionID string - region string - httpClient HTTPClient - recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager - capabilitiesClient CapabilitiesClient + cred azcore.TokenCredential + subscriptionID string + region string + httpClient HTTPClient + recommendationsPager RecommendationsPager + reservationsPager ReservationsDetailsPager + capabilitiesClient CapabilitiesClient + serversPager SQLServersPager + managedInstancesPager SQLManagedInstancesPager // Lazy SKU catalogue cache. Populated once on the first // recommendation conversion in this client's GetRecommendations @@ -87,6 +101,14 @@ type DatabaseClient struct { // to empty EngineVersion with a one-time WARN log. skuCacheOnce sync.Once skuCacheMap map[string]sqlSKUEntry + + // Lazy server-info cache. Populated once by fetchServerInfo, which + // walks the managed-instances and servers pagers. Both azConfig and + // deployment are derived in a single pass so the injected test pager + // is consumed exactly once. Mirrors the cosmosdb cachedAPIType pattern. + serverInfoOnce sync.Once + azConfig string + deployment string } // NewClient creates a new Azure Database client @@ -124,6 +146,16 @@ func (c *DatabaseClient) SetCapabilitiesClient(client CapabilitiesClient) { c.capabilitiesClient = client } +// SetServersPager sets the SQL servers pager (for testing). +func (c *DatabaseClient) SetServersPager(pager SQLServersPager) { + c.serversPager = pager +} + +// SetManagedInstancesPager sets the SQL managed instances pager (for testing). +func (c *DatabaseClient) SetManagedInstancesPager(pager SQLManagedInstancesPager) { + c.managedInstancesPager = pager +} + // GetServiceType returns the service type func (c *DatabaseClient) GetServiceType() common.ServiceType { return common.ServiceRelationalDB @@ -604,8 +636,9 @@ func extractSQLPricing(items []pricing.RetailPriceItem, termYears int) (onDemand // populated). EngineVersion enriched from the lazily-cached // armsql.CapabilitiesClient catalogue when the recommendation's SKU // string matches a ServiceLevelObjective in the location capabilities; -// otherwise stays empty. AZConfig/Deployment still need additional -// signals (per-server config) and remain deferred. +// otherwise stays empty. AZConfig and Deployment are enriched from the +// lazily-cached subscription-wide server/managed-instance lists; +// both stay empty when the fetch fails or the subscription is ambiguous. func (c *DatabaseClient) convertAzureSQLRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { f := azrecs.Extract(azureRec) if f == nil { @@ -615,6 +648,12 @@ func (c *DatabaseClient) convertAzureSQLRecommendation(ctx context.Context, azur if entry, ok := c.cachedSKULookup(ctx, f.ResourceType); ok && entry.engineVersion != "" { details.EngineVersion = entry.engineVersion } + if az := c.cachedDominantAZConfig(ctx); az != "" { + details.AZConfig = az + } + if dep := c.cachedDominantDeployment(ctx); dep != "" { + details.Deployment = dep + } return &common.Recommendation{ Provider: common.ProviderAzure, Service: common.ServiceRelationalDB, @@ -701,6 +740,155 @@ func populateSQLSKUMapFromVersion(out map[string]sqlSKUEntry, engineVersion stri } } +// cachedDominantAZConfig returns the single dominant AZConfig observed +// across all managed instances in the subscription, or "" when the +// answer is ambiguous or unavailable. Values: "zoneRedundant" when all +// instances have ZoneRedundant=true; "none" when all have +// ZoneRedundant=false; "" when the mix is ambiguous, zero instances +// exist, or the fetch fails. +// +// Both AZConfig and Deployment are populated by a single +// fetchServerInfo call gated by serverInfoOnce; there is no double walk. +// This ensures the injected test pager is consumed exactly once. +func (c *DatabaseClient) cachedDominantAZConfig(ctx context.Context) string { + c.serverInfoOnce.Do(func() { + c.azConfig, c.deployment = c.fetchServerInfo(ctx) + }) + return c.azConfig +} + +// cachedDominantDeployment returns the dominant Deployment observed +// across the subscription's SQL resources, or "" when ambiguous or +// unavailable. Values: "managed" / "single". Shares the single +// fetchServerInfo walk with cachedDominantAZConfig. +func (c *DatabaseClient) cachedDominantDeployment(ctx context.Context) string { + c.serverInfoOnce.Do(func() { + c.azConfig, c.deployment = c.fetchServerInfo(ctx) + }) + return c.deployment +} + +// fetchServerInfo performs a single walk of each pager (managed +// instances and regular servers) to compute both AZConfig and +// Deployment. This ensures the injected test pager for managed instances +// is consumed exactly once, even though the results feed two separate +// cached fields. +// +// AZConfig: "zoneRedundant" / "none" / "" (ambiguous or no signal). +// Deployment: "managed" / "single" / "" (mixed or no signal). +func (c *DatabaseClient) fetchServerInfo(ctx context.Context) (azConfig, deployment string) { + zoneRedundantCount, nonZoneRedundantCount, managedCount := c.walkManagedInstances(ctx) + hasServers := c.hasRegularServers(ctx) + + // Derive AZConfig from zone-redundancy counts. + total := zoneRedundantCount + nonZoneRedundantCount + switch { + case total == 0: + azConfig = "" + case zoneRedundantCount == total: + azConfig = "zoneRedundant" + case nonZoneRedundantCount == total: + azConfig = "none" + default: + azConfig = "" + } + + // Derive Deployment from presence of managed vs regular servers. + hasManaged := managedCount > 0 + switch { + case hasManaged && !hasServers: + deployment = "managed" + case hasServers && !hasManaged: + deployment = "single" + default: + deployment = "" + } + + return azConfig, deployment +} + +// walkManagedInstances walks the managed-instances pager once and +// returns the count of zone-redundant, non-zone-redundant, and total +// managed instances observed. Stops immediately on context cancellation +// or unrecoverable page error. +func (c *DatabaseClient) walkManagedInstances(ctx context.Context) (zoneRedundant, nonZoneRedundant, total int) { + pager, err := c.createManagedInstancesPager() + if err != nil { + logging.Warnf("azure database: managed instances pager create failed: %v; AZConfig/Deployment signal unavailable", err) + return 0, 0, 0 + } + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + if ctx.Err() != nil { + return zoneRedundant, nonZoneRedundant, total + } + logging.Warnf("azure database: managed instances page fetch failed: %v; AZConfig/Deployment signal unavailable", err) + return zoneRedundant, nonZoneRedundant, total + } + for _, mi := range page.Value { + total++ + if mi == nil || mi.Properties == nil || mi.Properties.ZoneRedundant == nil { + continue + } + if *mi.Properties.ZoneRedundant { + zoneRedundant++ + } else { + nonZoneRedundant++ + } + } + } + return zoneRedundant, nonZoneRedundant, total +} + +// createManagedInstancesPager returns the injected pager or creates a real one. +func (c *DatabaseClient) createManagedInstancesPager() (SQLManagedInstancesPager, error) { + if c.managedInstancesPager != nil { + return c.managedInstancesPager, nil + } + client, err := armsql.NewManagedInstancesClient(c.subscriptionID, c.cred, nil) + if err != nil { + return nil, fmt.Errorf("failed to create managed instances client: %w", err) + } + return client.NewListPager(nil), nil +} + +// hasRegularServers returns true when at least one SQL server exists in +// the subscription. Stops after the first non-empty page. +func (c *DatabaseClient) hasRegularServers(ctx context.Context) bool { + pager, err := c.createServersPager() + if err != nil { + logging.Warnf("azure database: servers pager create failed: %v; Deployment signal unavailable", err) + return false + } + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + if ctx.Err() != nil { + return false + } + logging.Warnf("azure database: servers page fetch failed: %v; Deployment signal unavailable", err) + return false + } + if len(page.Value) > 0 { + return true + } + } + return false +} + +// createServersPager returns the injected pager or creates a real one. +func (c *DatabaseClient) createServersPager() (SQLServersPager, error) { + if c.serversPager != nil { + return c.serversPager, nil + } + client, err := armsql.NewServersClient(c.subscriptionID, c.cred, nil) + if err != nil { + return nil, fmt.Errorf("failed to create servers client: %w", err) + } + return client.NewListPager(nil), nil +} + // detailsFromSQLSKU parses an Azure SQL SKU string into a // common.DatabaseDetails value. The Azure Reservation Recommendations // API returns SKU strings like "GeneralPurpose_Gen5_2" (edition, compute @@ -709,9 +897,9 @@ func populateSQLSKUMapFromVersion(out map[string]sqlSKUEntry, engineVersion stri // blank — converters must never return an error on unexpected SKU // strings because the API can add new SKUs without breaking consumers. // -// Engine / EngineVersion / AZConfig / Deployment require an armsql SKU -// catalogue lookup and are deliberately left empty; batched enrichment -// is a separate follow-up. +// Engine is always "sqlserver". EngineVersion, AZConfig, and Deployment +// are enriched by the lazy subscription-wide cached lookups in +// convertAzureSQLRecommendation; they are left empty here. func detailsFromSQLSKU(sku string) common.DatabaseDetails { // Engine is always SQL Server for an Azure SQL Database reservation. d := common.DatabaseDetails{ diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 875d05b07..92e219a7e 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -1311,3 +1311,252 @@ func TestReservationResourceTypeSQLDB_IsCanonical(t *testing.T) { "SQL Database filter value must be the Azure REST API canonical enum \"SQLDatabases\"; "+ "\"SqlDatabase\" (pre-fix) is not a valid resourceType") } + +// MockSQLServersPager mocks the SQLServersPager interface. +// CallCount tracks how many times NextPage was invoked. +type MockSQLServersPager struct { + err error + pages []armsql.ServersClientListResponse + index int + CallCount int +} + +func (m *MockSQLServersPager) More() bool { + return m.index < len(m.pages) +} + +func (m *MockSQLServersPager) NextPage(_ context.Context) (armsql.ServersClientListResponse, error) { + m.CallCount++ + if m.err != nil { + return armsql.ServersClientListResponse{}, m.err + } + if m.index >= len(m.pages) { + return armsql.ServersClientListResponse{}, errors.New("no more pages") + } + page := m.pages[m.index] + m.index++ + return page, nil +} + +// MockSQLManagedInstancesPager mocks the SQLManagedInstancesPager interface. +// CallCount tracks how many times NextPage was invoked. +type MockSQLManagedInstancesPager struct { + err error + pages []armsql.ManagedInstancesClientListResponse + index int + CallCount int +} + +func (m *MockSQLManagedInstancesPager) More() bool { + return m.index < len(m.pages) +} + +func (m *MockSQLManagedInstancesPager) NextPage(_ context.Context) (armsql.ManagedInstancesClientListResponse, error) { + m.CallCount++ + if m.err != nil { + return armsql.ManagedInstancesClientListResponse{}, m.err + } + if m.index >= len(m.pages) { + return armsql.ManagedInstancesClientListResponse{}, errors.New("no more pages") + } + page := m.pages[m.index] + m.index++ + return page, nil +} + +// buildMIPage builds a single-page managed-instances response with the +// given ZoneRedundant values. +func buildMIPage(zoneRedundant ...bool) armsql.ManagedInstancesClientListResponse { + instances := make([]*armsql.ManagedInstance, 0, len(zoneRedundant)) + for _, zr := range zoneRedundant { + zr := zr + instances = append(instances, &armsql.ManagedInstance{ + Properties: &armsql.ManagedInstanceProperties{ZoneRedundant: &zr}, + }) + } + return armsql.ManagedInstancesClientListResponse{ + ManagedInstanceListResult: armsql.ManagedInstanceListResult{Value: instances}, + } +} + +// buildServerPage builds a single-page servers response with n placeholder servers. +func buildServerPage(n int) armsql.ServersClientListResponse { + servers := make([]*armsql.Server, n) + for i := range servers { + loc := "eastus" + servers[i] = &armsql.Server{Location: &loc} + } + return armsql.ServersClientListResponse{ + ServerListResult: armsql.ServerListResult{Value: servers}, + } +} + +// TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesAZConfig asserts +// that a subscription with all zone-redundant managed instances produces +// AZConfig="zoneRedundant" in the recommendation Details. +func TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesAZConfig(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + client.SetManagedInstancesPager(&MockSQLManagedInstancesPager{ + pages: []armsql.ManagedInstancesClientListResponse{buildMIPage(true)}, + }) + // No regular servers. + client.SetServersPager(&MockSQLServersPager{}) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Equal(t, "zoneRedundant", details.AZConfig) +} + +// TestDatabaseClient_ConvertAzureSQLRecommendation_AmbiguousAZConfig asserts +// that a subscription with mixed zone-redundancy produces AZConfig="" (empty). +func TestDatabaseClient_ConvertAzureSQLRecommendation_AmbiguousAZConfig(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + // One zone-redundant and one non-zone-redundant = ambiguous. + client.SetManagedInstancesPager(&MockSQLManagedInstancesPager{ + pages: []armsql.ManagedInstancesClientListResponse{buildMIPage(true, false)}, + }) + client.SetServersPager(&MockSQLServersPager{}) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Empty(t, details.AZConfig, "ambiguous zone-redundancy must leave AZConfig empty") +} + +// TestDatabaseClient_ConvertAzureSQLRecommendation_AZConfigPagerErrorFallsBack +// asserts that a managed-instances pager failure leaves AZConfig empty +// without failing the recommendation conversion. +func TestDatabaseClient_ConvertAzureSQLRecommendation_AZConfigPagerErrorFallsBack(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + client.SetManagedInstancesPager(&MockSQLManagedInstancesPager{ + pages: []armsql.ManagedInstancesClientListResponse{{}}, + err: errors.New("transient error"), + }) + client.SetServersPager(&MockSQLServersPager{}) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out, "conversion must NOT fail on pager error") + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Empty(t, details.AZConfig, "AZConfig empty on pager error") +} + +// TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesDeployment asserts +// that a subscription with only managed instances produces +// Deployment="managed". +func TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesDeployment(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + zr := true + client.SetManagedInstancesPager(&MockSQLManagedInstancesPager{ + pages: []armsql.ManagedInstancesClientListResponse{ + {ManagedInstanceListResult: armsql.ManagedInstanceListResult{ + Value: []*armsql.ManagedInstance{ + {Properties: &armsql.ManagedInstanceProperties{ZoneRedundant: &zr}}, + }, + }}, + }, + }) + // No regular servers. + client.SetServersPager(&MockSQLServersPager{}) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Equal(t, "managed", details.Deployment) +} + +// TestDatabaseClient_ConvertAzureSQLRecommendation_DeploymentSingle asserts +// that a subscription with only regular servers (no managed instances) +// produces Deployment="single". +func TestDatabaseClient_ConvertAzureSQLRecommendation_DeploymentSingle(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + // No managed instances. + client.SetManagedInstancesPager(&MockSQLManagedInstancesPager{}) + client.SetServersPager(&MockSQLServersPager{ + pages: []armsql.ServersClientListResponse{buildServerPage(2)}, + }) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Equal(t, "single", details.Deployment) +} + +// TestDatabaseClient_ConvertAzureSQLRecommendation_DeploymentPagerErrorFallsBack +// asserts that a servers pager failure leaves Deployment empty without +// failing the conversion. +func TestDatabaseClient_ConvertAzureSQLRecommendation_DeploymentPagerErrorFallsBack(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + client.SetManagedInstancesPager(&MockSQLManagedInstancesPager{}) + client.SetServersPager(&MockSQLServersPager{ + pages: []armsql.ServersClientListResponse{{}}, + err: errors.New("transient servers error"), + }) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out, "conversion must NOT fail on servers pager error") + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Empty(t, details.Deployment, "Deployment empty on pager error") +} + +// TestDatabaseClient_ServerInfo_FetchedOnce pins the perf invariant: many +// converter calls in the same GetRecommendations run trigger exactly ONE +// walk of the managed-instances and servers pagers regardless of call count. +func TestDatabaseClient_ServerInfo_FetchedOnce(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + + miPager := &MockSQLManagedInstancesPager{ + pages: []armsql.ManagedInstancesClientListResponse{buildMIPage(true)}, + } + serversPager := &MockSQLServersPager{} + + client.SetManagedInstancesPager(miPager) + client.SetServersPager(serversPager) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + // Call the converter 10 times — fetchServerInfo must run only once. + for i := 0; i < 10; i++ { + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out) + } + // The pager is walked exactly once: the sync.Once gate fires on the + // first converter call and all subsequent calls read the cached value. + assert.Equal(t, 1, miPager.CallCount, + "managed-instances pager must be walked ONCE regardless of converter call count") + // Servers pager is also walked once (zero pages = no servers found). + assert.Equal(t, 0, serversPager.CallCount, + "servers pager NextPage not called when it has no pages") +}