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") +}