diff --git a/CHANGELOG.md b/CHANGELOG.md index 866981b9..3ec3aaf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,6 +309,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **Breaking (source):** `managedredis.ReservationsDetailsPager` and `(*managedredis.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#699). - **`azure` Redis:** `managedredis.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Redis reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard (#699). An incomplete live reservation row (missing SKU, location, quantity or purchase date, an unattributable scope, or an expiry falling today) now fails the lookup with an error instead of being skipped, as `azure` compute already does. +- **Breaking (source):** `cosmosdb.ReservationsDetailsPager` and `(*cosmosdb.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#705). +- **`azure` Cosmos DB:** `cosmosdb.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Cosmos DB reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). +- **Breaking (source):** `synapse.ReservationsDetailsPager` and `(*synapse.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#705). +- **`azure` Synapse:** `synapse.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Synapse reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). +- **Breaking (source):** `database.ReservationsDetailsPager` and `(*database.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#705). +- **`azure` SQL Database:** `database.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a SQL Database reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. Held reservations carry engine `sqlserver` and the subscription's dominant zone-redundancy setting, as recommendations do. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). - **`aws` RI recommendations:** a fractional Cost Explorer quantity now rounds down instead of to the nearest integer, so a purchase never exceeds CE's quantity. The upfront, recurring monthly cost, monthly savings and savings diff --git a/pkg/common/types.go b/pkg/common/types.go index 942f2f60..d6e53ad5 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -527,12 +527,12 @@ const ( // Commitment represents an existing commitment (RI/SP/CUD/etc). // -// Deployment is RDS-specific (Multi-AZ vs Single-AZ); it stays empty for -// services that don't have a deployment dimension (EC2, ElastiCache, -// etc). When populated, it carries the same vocabulary as -// DatabaseDetails.AZConfig on Recommendation ("single-az" / "multi-az") -// so pool-key matching can collapse both sides via normaliseDeployment -// in the recommendations package. Without this field, RDS expiry +// Deployment is set for AWS RDS and Azure SQL Database and stays empty for +// services without a deployment dimension (EC2, ElastiCache, etc). When +// populated, it carries the same vocabulary as DatabaseDetails.AZConfig on +// Recommendation ("single-az" / "multi-az" on AWS, "zoneRedundant" / "none" +// on Azure) so pool-key matching can collapse both sides via +// normaliseDeployment in the recommendations package. Without this field, RDS expiry // adjustments would silently miss because Recommendation lookup keys // are deployment-aware while commitment keys defaulted to empty. type Commitment struct { @@ -546,7 +546,7 @@ type Commitment struct { Region string `json:"region"` ResourceType string `json:"resource_type"` Engine string `json:"engine,omitempty"` // Database engine for RDS/ElastiCache (e.g., "mysql", "aurora-postgresql") - Deployment string `json:"deployment,omitempty"` // RDS Multi-AZ vs Single-AZ; empty for non-RDS + Deployment string `json:"deployment,omitempty"` // RDS Multi-AZ vs Single-AZ, Azure SQL zone redundancy; empty otherwise State CommitmentState `json:"state"` // EC2 reserved-instance dimensions. Empty means unknown or a wildcard. diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 7bb13c37..ab1d7f6b 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -13,6 +13,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -469,8 +470,6 @@ func TestAzureClients_GetExistingCommitments_PagerConstructionError(t *testing.T client provider.ServiceClient }{ {"cache", NewCacheClient(nil, "test-subscription", "eastus")}, - {"cosmosdb", NewCosmosDBClient(nil, "test-subscription", "eastus")}, - {"database", NewDatabaseClient(nil, "test-subscription", "eastus")}, {"search", NewSearchClient(nil, "test-subscription", "eastus")}, } { t.Run(tc.name, func(t *testing.T) { @@ -485,6 +484,36 @@ func TestAzureClients_GetExistingCommitments_PagerConstructionError(t *testing.T } } +// Held reservations come from the Reservations API, so a client-construction +// failure of the default inventory factories surfaces under the shared +// ": list applied reservations" prefix. +func TestAzureClients_ReservationInventory_DefaultFactoryError(t *testing.T) { + // Keep this test sequential: the SDK reads this process-wide configuration. + originalCloud := cloud.AzurePublic + t.Cleanup(func() { cloud.AzurePublic = originalCloud }) + cloud.AzurePublic = cloud.Configuration{} + + _, sdkErr := armreservations.NewAzureReservationAPIClient(nil, nil) + require.Error(t, sdkErr) + + for _, tc := range []struct { + name string + client provider.ServiceClient + }{ + {"cosmosdb", NewCosmosDBClient(nil, "test-subscription", "eastus")}, + {"database", NewDatabaseClient(nil, "test-subscription", "eastus")}, + {"synapse", NewSynapseClient(nil, "test-subscription", "eastus")}, + } { + t.Run(tc.name, func(t *testing.T) { + commitments, err := tc.client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Nil(t, commitments) + assert.Contains(t, err.Error(), tc.name+": list applied reservations: ") + assert.Contains(t, err.Error(), sdkErr.Error()) + }) + } +} + func TestAzureProvider_GetRecommendationsClient_NotConfigured(t *testing.T) { t.Parallel() // Test GetRecommendationsClient when Azure is not configured. Inject a diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 78e290b9..932a8627 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -294,24 +294,8 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen factories = *c.inventoryFactories } - responses, err := reservations.ListAppliedReservations(ctx, c.subscriptionID, factories, maxReservationsPages) - if err != nil { - return nil, fmt.Errorf("compute: list applied reservations: %w", err) - } - - now := time.Now() - commitments := make([]common.Commitment, 0, len(responses)) - for _, r := range responses { - commitment, err := reservations.StrictCommitment(r, c.subscriptionID, common.ServiceCompute, - armreservations.ReservedResourceTypeVirtualMachines, reservations.StrictLabels{Service: "compute", Resource: "VM"}, now) - if err != nil { - return nil, err - } - if commitment != nil { - commitments = append(commitments, *commitment) - } - } - return commitments, nil + return reservations.StrictCommitments(ctx, c.subscriptionID, factories, maxReservationsPages, common.ServiceCompute, + armreservations.ReservedResourceTypeVirtualMachines, reservations.StrictLabels{Service: "compute", Resource: "VM"}) } // providerRegistrationState is the JSON shape returned by the ARM providers API. diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 66eb07e7..377374b3 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -53,12 +53,6 @@ type RecommendationsPager interface { NextPage(ctx context.Context) (armconsumption.ReservationRecommendationsClientListResponse, error) } -// ReservationsDetailsPager interface for reservations details pager (enables mocking). -type ReservationsDetailsPager interface { - More() bool - NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) -} - // CosmosAccountsPager interface for Cosmos DB accounts pager (enables mocking). type CosmosAccountsPager interface { More() bool @@ -70,7 +64,7 @@ type Client struct { cred azcore.TokenCredential httpClient HTTPClient recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager + inventoryFactories *reservations.InventoryFactories cosmosAccountsPager CosmosAccountsPager subscriptionID string region string @@ -118,9 +112,18 @@ func (c *Client) SetRecommendationsPager(pager RecommendationsPager) { c.recommendationsPager = pager } -// SetReservationsPager sets the reservations pager (for testing). -func (c *Client) SetReservationsPager(pager ReservationsDetailsPager) { - c.reservationsPager = pager +// SetInventoryFactories injects reservation-inventory factories (for testing). +func (c *Client) SetInventoryFactories(f *reservations.InventoryFactories) { + c.inventoryFactories = f +} + +// factories returns the injected inventory factories, or the real SDK-backed +// ones when none were injected (production only calls NewClient). +func (c *Client) factories() reservations.InventoryFactories { + if c.inventoryFactories != nil { + return *c.inventoryFactories + } + return reservations.DefaultInventoryFactories(c.cred) } // SetCosmosAccountsPager sets the Cosmos DB accounts pager (for testing). @@ -212,91 +215,14 @@ func (c *Client) appendPage(ctx context.Context, pricer *azrecs.Pricer, recs []c return recs, nil } -// GetExistingCommitments retrieves existing Cosmos DB reserved capacity using Azure Resource Graph. +// GetExistingCommitments retrieves the Cosmos DB reservations that apply to +// this subscription from the Reservations API, which carries the purchase +// date, quantity, location and lifecycle state the duplicate-purchase guard +// needs (issue #705). Any failure or incomplete live row is returned as an +// error rather than as an empty or partial inventory. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - pager, err := c.createReservationsPager() - if err != nil { - return nil, fmt.Errorf("cosmosdb: create reservations pager: %w", err) - } - - return c.collectCosmosReservations(ctx, pager) -} - -// createReservationsPager creates a pager for listing reservations. -func (c *Client) createReservationsPager() (ReservationsDetailsPager, error) { - // Use injected pager if available (for testing) - if c.reservationsPager != nil { - return c.reservationsPager, nil - } - - client, err := armconsumption.NewReservationsDetailsClient(c.cred, nil) - if err != nil { - return nil, err - } - - scope := fmt.Sprintf("subscriptions/%s", c.subscriptionID) - return client.NewListPager(scope, &armconsumption.ReservationsDetailsClientListOptions{}), nil -} - -// collectCosmosReservations collects Cosmos DB reservations from the pager. -// Returns an error on first pagination failure so callers can't silently act -// on a partial list — see the compute client for the full rationale. -func (c *Client) collectCosmosReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - for pageIdx := 0; pager.More(); pageIdx++ { - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("context canceled during pagination: %w", err) - } - if pageIdx >= maxReservationsPages { - return nil, fmt.Errorf("cosmosdb: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - return nil, fmt.Errorf("cosmosdb: list reservations: %w", err) - } - - for _, detail := range page.Value { - if commitment := c.convertCosmosReservation(detail); commitment != nil { - commitments = append(commitments, *commitment) - } - } - } - - // ReservationsDetails is a daily usage API: one row per reservation per - // usage day. Without this, a single reservation held for N days would - // surface as N commitments sharing one CommitmentID (issue #73). - return reservations.DedupeCommitmentsByID(commitments), nil -} - -// convertCosmosReservation converts a reservation detail to a commitment if it's a Cosmos DB reservation. -func (c *Client) convertCosmosReservation(detail *armconsumption.ReservationDetail) *common.Commitment { - if detail.Properties == nil { - return nil - } - - props := detail.Properties - if props.SKUName == nil || !strings.Contains(strings.ToLower(*props.SKUName), "cosmos") { - return nil - } - - commitment := &common.Commitment{ - Provider: common.ProviderAzure, - Account: c.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Service: common.ServiceNoSQL, - Region: c.region, - State: common.CommitmentStateActive, - } - - if props.ReservationID != nil { - commitment.CommitmentID = *props.ReservationID - } - if props.SKUName != nil { - commitment.ResourceType = *props.SKUName - } - - return commitment + return reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceNoSQL, + armreservations.ReservedResourceTypeCosmosDb, reservations.StrictLabels{Service: "cosmosdb", Resource: "Cosmos DB"}) } // PurchaseCommitment purchases Cosmos DB reserved capacity using the two-step diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 06a846bf..981afdc9 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -15,12 +15,15 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // MockRecommendationsPager mocks the RecommendationsPager interface. @@ -49,32 +52,6 @@ func (m *MockRecommendationsPager) NextPage(ctx context.Context) (armconsumption return page, nil } -// MockReservationsDetailsPager mocks the ReservationsDetailsPager interface. -type MockReservationsDetailsPager struct { - pages []armconsumption.ReservationsDetailsClientListResponse - index int - err error -} - -func (m *MockReservationsDetailsPager) More() bool { - if m.err != nil { - return true // Return true to allow error to be returned on NextPage - } - return m.index < len(m.pages) -} - -func (m *MockReservationsDetailsPager) NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - if m.err != nil { - return armconsumption.ReservationsDetailsClientListResponse{}, m.err - } - if m.index >= len(m.pages) { - return armconsumption.ReservationsDetailsClientListResponse{}, errors.New("no more pages") - } - page := m.pages[m.index] - m.index++ - return page, nil -} - // MockCosmosAccountsPager mocks the CosmosAccountsPager interface. type MockCosmosAccountsPager struct { pages []armcosmos.DatabaseAccountsClientListResponse @@ -569,19 +546,6 @@ func TestCosmosDBClient_GetOfferingDetails_RequestsSKUFilter(t *testing.T) { mockHTTP.AssertExpectations(t) } -func TestCosmosDBClient_GetExistingCommitments_Empty(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Mock pager returns no pages — the empty-subscription case. - client.SetReservationsPager(&MockReservationsDetailsPager{}) - - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) -} - func TestCosmosPricingStructure(t *testing.T) { t.Parallel() pricing := CosmosPricing{ @@ -665,153 +629,113 @@ func TestCosmosDBClient_GetRecommendations_MultiplePages(t *testing.T) { assert.Equal(t, 2, mockPager.index) // Verify both pages were consumed } -func TestCosmosDBClient_GetExistingCommitments_WithMockPager(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "sql-db-standard" // Does NOT contain "cosmos" so won't be included +// factoriesRows injects one order holding rows (and err on the page fetch). +func factoriesRows(err error, rows ...*armreservations.ReservationResponse) *reservations.InventoryFactories { + return factoriesFor(func() reservations.OrderReservationsPager { return &reservationstest.OrderPager{Rows: rows, Err: err} }) +} - // Create mock pager with test data - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - }, - }, - }, - }, - }, +// factoriesFor injects one reservation order whose pager is newPager(). +func factoriesFor(newPager func() reservations.OrderReservationsPager) *reservations.InventoryFactories { + return &reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return reservationstest.AppliedLister{}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return newPager() }, } - client.SetReservationsPager(mockPager) +} + +func cosmosRow(id, sku, location string, qty int32) *armreservations.ReservationResponse { + return reservationstest.LiveRow(armreservations.ReservedResourceTypeCosmosDb, "sub", id, sku, location, qty) +} - commitments, err := client.GetExistingCommitments(ctx) +func TestGetExistingCommitments_Empty(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(nil)) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) - assert.NotNil(t, commitments) - // The SKU name doesn't contain "cosmos" so won't be included assert.Empty(t, commitments) } -func TestCosmosDBClient_GetExistingCommitments_CosmosCommitments(t *testing.T) { +func TestGetExistingCommitments_CosmosRowConverted(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "cosmos-db-standard" // Contains "cosmos" - - // Create mock pager with cosmos commitment - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - }, - }, - }, - }, - }, - } - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(nil, cosmosRow("res-123", "Cosmos_DB_Standard", "westus2", 3))) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) require.Len(t, commitments, 1) - assert.Equal(t, reservationID, commitments[0].CommitmentID) - assert.Equal(t, skuName, commitments[0].ResourceType) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-123", commitments[0].CommitmentID) + assert.Equal(t, "Cosmos_DB_Standard", commitments[0].ResourceType) + assert.Equal(t, "westus2", commitments[0].Region, "reservation location must win over the client region") + assert.Equal(t, 3, commitments[0].Count) + assert.Equal(t, common.CommitmentStateActive, commitments[0].State) assert.Equal(t, common.ServiceNoSQL, commitments[0].Service) + assert.Empty(t, commitments[0].Engine, "NoSQLDetails keys carry no engine") + assert.Empty(t, commitments[0].Deployment, "NoSQLDetails keys carry no deployment") } -// TestCosmosDBClient_GetExistingCommitments_DedupesDailyUsageRows is the -// regression test for #73: armconsumption.ReservationsDetails is a daily -// usage API, one row per reservation per usage day. A pager returning three -// rows for the same ReservationID must surface as a single commitment. -func TestCosmosDBClient_GetExistingCommitments_DedupesDailyUsageRows(t *testing.T) { +func TestGetExistingCommitments_NonCosmosFiltered(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "cosmos-db-standard" - - dailyRow := &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - } - - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{dailyRow, dailyRow, dailyRow}, - }, - }, - }, - } - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) + c := NewClient(nil, "sub", "eastus") + sqlRow := cosmosRow("res-1", "Standard_S0", "eastus", 1) + sqlType := armreservations.ReservedResourceTypeSQLDatabases + sqlRow.Properties.ReservedResourceType = &sqlType + c.SetInventoryFactories(factoriesRows(nil, sqlRow, cosmosRow("res-2", "Cosmos_DB_Standard", "eastus", 1))) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) - require.Len(t, commitments, 1, "three daily rows for one reservation must collapse to one commitment (issue #73)") - assert.Equal(t, reservationID, commitments[0].CommitmentID) + require.Len(t, commitments, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-2", commitments[0].CommitmentID) } -func TestCosmosDBClient_GetExistingCommitments_PagerError(t *testing.T) { +// A row the guard cannot classify may be the reservation bought an hour ago, +// so it fails closed instead of being dropped. +func TestGetExistingCommitments_NilPropertiesFailsClosed(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - mockPager := &MockReservationsDetailsPager{ - err: errors.New("API error"), - } - client.SetReservationsPager(mockPager) - - // Pagination errors must propagate — partial lists are unsafe for the - // purchase flow (duplicate-purchase risk). See the compute client. - commitments, err := client.GetExistingCommitments(ctx) + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(nil, &armreservations.ReservationResponse{})) + _, err := c.GetExistingCommitments(context.Background()) require.Error(t, err) - assert.Contains(t, err.Error(), "cosmosdb: list reservations") - assert.Nil(t, commitments) } -func TestCosmosDBClient_GetExistingCommitments_NilProperties(t *testing.T) { +func TestGetExistingCommitments_PagerError(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(errors.New("api error"))) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "api error") +} - // Create mock pager with nil properties - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: nil, // Nil properties should be skipped - }, - }, - }, - }, +func TestGetExistingCommitments_ListerError(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: errors.New("boom")}, nil }, - } - client.SetReservationsPager(mockPager) + NewOrderPager: func(string) reservations.OrderReservationsPager { return &reservationstest.OrderPager{} }, + }) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err, "a failed inventory lookup must not masquerade as an empty list") + assert.Contains(t, err.Error(), "cosmosdb: list applied reservations") +} - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) +func TestNewClient_DefaultInventoryFactories(t *testing.T) { + t.Parallel() + for name, c := range map[string]*Client{ + "NewClient": NewClient(nil, "sub", "eastus"), + "NewClientWithHTTP": NewClientWithHTTP(nil, "sub", "eastus", nil), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + require.Nil(t, c.inventoryFactories, "production wiring injects nothing") + f := c.factories() + assert.NotNil(t, f.NewAppliedLister) + assert.NotNil(t, f.NewOrderPager) + }) + } + injected := &reservations.InventoryFactories{} + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(injected) + assert.Equal(t, *injected, c.factories()) } func TestCosmosDBClient_GetValidResourceTypes_WithMockPager(t *testing.T) { @@ -1021,10 +945,10 @@ func TestCosmosDBClient_SetterMethods(t *testing.T) { client.SetRecommendationsPager(mockRecPager) assert.Equal(t, mockRecPager, client.recommendationsPager) - // Test SetReservationsPager - mockResPager := &MockReservationsDetailsPager{} - client.SetReservationsPager(mockResPager) - assert.Equal(t, mockResPager, client.reservationsPager) + // Test SetInventoryFactories + factories := &reservations.InventoryFactories{} + client.SetInventoryFactories(factories) + assert.Same(t, factories, client.inventoryFactories) // Test SetCosmosAccountsPager mockAccountsPager := &MockCosmosAccountsPager{} diff --git a/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go b/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go new file mode 100644 index 00000000..60031660 --- /dev/null +++ b/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go @@ -0,0 +1,160 @@ +package cosmosdb + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" +) + +// Issue #705: a Cosmos DB reservation CUDly (or anyone) already holds must +// reduce the matching recommendation. Fixture dates are UTC calendar dates, +// because the Reservations API returns date-only values. +var ( + fixtureToday = time.Now().UTC().Truncate(24 * time.Hour) + fixturePast = fixtureToday.AddDate(0, 0, -30) +) + +const cosmosSKU = "CosmosDB_RU_1000" + +func testPricer() *recommendations.Pricer { + return recommendations.NewPricer("cosmosdb", func(context.Context, string, string, int) (recommendations.ReservationPrice, error) { + return recommendations.ReservationPrice{Total: 1000, Currency: "USD"}, nil + }) +} + +// upfrontRec runs the real converter and variant expansion for a count-2 +// P1Y recommendation and returns its upfront variant. +func upfrontRec(t *testing.T, c *Client) common.Recommendation { + t.Helper() + raw := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), mocks.WithTerm("P1Y"), mocks.WithQuantity(2), + mocks.WithNormalizedSize(cosmosSKU), mocks.WithCosts(1000, 700, 300)) + recs, err := c.appendPage(context.Background(), testPricer(), nil, + []armconsumption.ReservationRecommendationClassification{raw}) + require.NoError(t, err) + require.Len(t, recs, 2) + upfront := recs[0] + require.Equal(t, "upfront", upfront.PaymentOption) + require.Equal(t, 2, upfront.Count) + // The dedupe key reads engine and deployment from the details; NoSQL + // recommendations carry neither, so held commitments must carry neither. + require.Empty(t, common.EngineFromDetails(upfront.Details)) + require.Empty(t, common.DeploymentFromDetails(upfront.Details)) + return upfront +} + +// purchasedRow buys rec through a capturing transport and builds the held +// reservation from the request body CUDly actually sent. region is the +// recommending client's, so the location assertion can fail. +func purchasedRow(t *testing.T, region string, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { + t.Helper() + capture := &reservationstest.Capture{} + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", region, capture) + _, err := buyer.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + var body struct { + Location string `json:"location"` + SKU struct { + Name string `json:"name"` + } `json:"sku"` + Properties struct { + ReservedResourceType string `json:"reservedResourceType"` + Quantity int32 `json:"quantity"` + } `json:"properties"` + } + require.NoError(t, json.Unmarshal(capture.BodyBytes, &body)) + require.Equal(t, rec.Region, body.Location) + require.Equal(t, rec.Count, int(body.Properties.Quantity)) + return reservationstest.LiveRow(armreservations.ReservedResourceType(body.Properties.ReservedResourceType), + "sub", "held", body.SKU.Name, body.Location, qty) +} + +func adjust(t *testing.T, c *Client, recs ...common.Recommendation) ([]common.Recommendation, error) { + t.Helper() + passed, _, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, c) + return passed, err +} + +func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count) + assert.InDelta(t, upfront.CommitmentCost/2, passed[0].CommitmentCost, 0.001, "money fields scale with the count") +} + +func TestHeldReservation_FullHoldFiltersRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 2))) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + assert.Empty(t, passed) +} + +func TestHeldReservation_Table(t *testing.T) { + t.Parallel() + state := func(s armreservations.ProvisioningState) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ProvisioningState = &s } + } + expiry := func(d time.Time) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ExpiryDate = &d } + } + cases := []struct { + name string + mutate func(*armreservations.ReservationResponse) + want int + wantErr string // non-empty: the lookup must fail with this text + }{ + {"shared, this subscription", func(*armreservations.ReservationResponse) {}, 1, ""}, + {"creating", state(armreservations.ProvisioningStateCreating), 1, ""}, + {"shared, billed to another subscription", func(r *armreservations.ReservationResponse) { + r.Properties.BillingScopeID = mocks.StringPtr("/subscriptions/other") + }, 2, ""}, + {"canceled", state(armreservations.ProvisioningStateCancelled), 2, ""}, + {"expired state", state(armreservations.ProvisioningStateExpired), 2, ""}, + {"succeeded past expiry", expiry(fixturePast), 2, ""}, + {"other region", func(r *armreservations.ReservationResponse) { r.Location = mocks.StringPtr("westus") }, 2, ""}, + {"other sku", func(r *armreservations.ReservationResponse) { r.SKU.Name = mocks.StringPtr("CosmosDB_RU_5000") }, 2, ""}, + {"missing quantity", func(r *armreservations.ReservationResponse) { r.Properties.Quantity = nil }, 0, "quantity"}, + {"expires today", expiry(fixtureToday), 0, "expiryDate"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + row := purchasedRow(t, c.region, upfront, 1) + tc.mutate(row) + c.SetInventoryFactories(factoriesRows(nil, row)) + passed, err := adjust(t, c, upfront) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, tc.want, passed[0].Count) + }) + } +} diff --git a/providers/azure/services/cosmosdb/pagination_cap_test.go b/providers/azure/services/cosmosdb/pagination_cap_test.go index a8a31f78..c4011849 100644 --- a/providers/azure/services/cosmosdb/pagination_cap_test.go +++ b/providers/azure/services/cosmosdb/pagination_cap_test.go @@ -6,6 +6,10 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // countingAccountsPager reports More() until limit pages were requested, so a @@ -71,3 +75,58 @@ func TestFetchDominantAPIType_ExactlyCapPagesWithPartialLastPageSucceeds(t *test assert.Equal(t, "mongodb", client.fetchDominantAPIType(context.Background())) assert.Equal(t, maxAccountsPages, pager.calls) } + +func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { + t.Parallel() + pager := &reservationstest.CountingPager{Limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap") + assert.Equal(t, maxReservationsPages, pager.Calls) +} + +func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + pager := &reservationstest.CountingPager{Limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, pager.Calls) +} + +func TestGetExistingCommitments_CapIsExact(t *testing.T) { + t.Parallel() + atCap := &reservationstest.CountingPager{Limit: maxReservationsPages} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return atCap })) + _, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err, "exactly cap pages must succeed") + assert.Equal(t, maxReservationsPages, atCap.Calls) + + overCap := &reservationstest.CountingPager{Limit: maxReservationsPages + 1} + client = NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) + _, err = client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Equal(t, maxReservationsPages, overCap.Calls) +} + +func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pager := &reservationstest.CancelOnSecondCallPager{Cancel: cancel} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 2, pager.Calls) +} diff --git a/providers/azure/services/cosmosdb/slot_wiring_test.go b/providers/azure/services/cosmosdb/slot_wiring_test.go index 2ce02a41..a4cc506d 100644 --- a/providers/azure/services/cosmosdb/slot_wiring_test.go +++ b/providers/azure/services/cosmosdb/slot_wiring_test.go @@ -9,8 +9,11 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot/slottest" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // onePager yields a single empty page and asserts the slot is held while @@ -28,6 +31,14 @@ func (p *onePager[T]) NextPage(context.Context) (T, error) { return zero, nil } +// slotLister reports one order so the order pager is exercised too. +type slotLister struct{ p *slottest.Probe } + +func (l slotLister) GetAppliedReservationList(ctx context.Context, sub string, o *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + l.p.Check() + return reservationstest.AppliedLister{}.GetAppliedReservationList(ctx, sub, o) +} + type probeHTTP struct{ probe *slottest.Probe } func (h probeHTTP) Do(*http.Request) (*http.Response, error) { @@ -43,7 +54,12 @@ func TestClientCallsHoldSharedSlot(t *testing.T) { _, _ = c.GetRecommendations(ctx, nil) }, "GetExistingCommitments": func(ctx context.Context, p *slottest.Probe, c *Client) { - c.SetReservationsPager(&onePager[armconsumption.ReservationsDetailsClientListResponse]{probe: p}) + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return slotLister{p}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &onePager[armreservations.ReservationClientListResponse]{probe: p} + }, + }) _, _ = c.GetExistingCommitments(ctx) }, "GetValidResourceTypes": func(ctx context.Context, p *slottest.Probe, c *Client) { diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index da0b56c1..346772c3 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -34,6 +34,10 @@ const reservationResourceTypeSQLDB = "SQLDatabases" // maxRecsPages caps Consumption API recommendation pagination. const maxRecsPages = 10 +// sqlServerEngine is the engine every Azure SQL Database recommendation and +// held reservation carries. +const sqlServerEngine = "sqlserver" + // maxReservationsPages caps reservation-detail pagination. const maxReservationsPages = 50 @@ -60,12 +64,6 @@ type RecommendationsPager interface { NextPage(ctx context.Context) (armconsumption.ReservationRecommendationsClientListResponse, error) } -// ReservationsDetailsPager interface for reservations details pager (enables mocking). -type ReservationsDetailsPager interface { - More() bool - NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) -} - // CapabilitiesClient interface for SQL capabilities (enables mocking). type CapabilitiesClient interface { ListByLocation(ctx context.Context, locationName string, options *armsql.CapabilitiesClientListByLocationOptions) (armsql.CapabilitiesClientListByLocationResponse, error) @@ -96,7 +94,7 @@ type Client struct { cred azcore.TokenCredential httpClient HTTPClient recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager + inventoryFactories *reservations.InventoryFactories capabilitiesClient CapabilitiesClient serversPager SQLServersPager managedInstancesPager SQLManagedInstancesPager @@ -117,8 +115,12 @@ type Client struct { // Only a complete walk is cached (serverInfoLoaded); a failed or // canceled walk is retried, but not before serverInfoRetryAfter so a // persistently failing subscription is not re-walked per recommendation. - azConfig string - deployment string + azConfig string + deployment string + // recAZConfig is the AZConfig the recommendation path last stamped, so + // held reservations are keyed identically even if a later walk answers + // differently. Guarded by serverInfoMu. + recAZConfig string serverInfoRetryDelay time.Duration // Lazy SKU catalog cache. Populated once on the first @@ -136,6 +138,7 @@ type Client struct { // persistently failing subscription is not re-walked per recommendation. serverInfoMu sync.Mutex serverInfoLoaded bool + recAZConfigSet bool } // serverInfoRetryDelayDefault is the minimum wait before re-walking the @@ -174,9 +177,18 @@ func (c *Client) SetRecommendationsPager(pager RecommendationsPager) { c.recommendationsPager = pager } -// SetReservationsPager sets the reservations pager (for testing). -func (c *Client) SetReservationsPager(pager ReservationsDetailsPager) { - c.reservationsPager = pager +// SetInventoryFactories injects reservation-inventory factories (for testing). +func (c *Client) SetInventoryFactories(f *reservations.InventoryFactories) { + c.inventoryFactories = f +} + +// factories returns the injected inventory factories, or the real SDK-backed +// ones when none were injected (production only calls NewClient). +func (c *Client) factories() reservations.InventoryFactories { + if c.inventoryFactories != nil { + return *c.inventoryFactories + } + return reservations.DefaultInventoryFactories(c.cred) } // SetCapabilitiesClient sets the capabilities client (for testing). @@ -268,91 +280,34 @@ func (c *Client) appendPage(ctx context.Context, pricer *azrecs.Pricer, recs []c return recs, nil } -// GetExistingCommitments retrieves existing SQL Database reserved capacity using Azure Resource Graph. +// GetExistingCommitments retrieves the SQL Database reservations that apply to +// this subscription from the Reservations API, which carries the purchase +// date, quantity, location and lifecycle state the duplicate-purchase guard +// needs (issue #705). Any failure or incomplete live row is returned as an +// error rather than as an empty or partial inventory. +// +// The dedupe key includes engine and deployment, and recommendations carry +// Engine "sqlserver" and the subscription's dominant AZConfig, so held +// reservations are stamped with the same values. The server walk is skipped +// when nothing is held. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - pager, err := c.createReservationsPager() - if err != nil { - return nil, fmt.Errorf("database: create reservations pager: %w", err) + commitments, err := reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceRelationalDB, + armreservations.ReservedResourceTypeSQLDatabases, reservations.StrictLabels{Service: "database", Resource: "SQL"}) + if err != nil || len(commitments) == 0 { + return commitments, err } - - return c.collectSQLReservations(ctx, pager) -} - -// createReservationsPager creates a pager for listing reservations. -func (c *Client) createReservationsPager() (ReservationsDetailsPager, error) { - // Use injected pager if available (for testing) - if c.reservationsPager != nil { - return c.reservationsPager, nil + az, ok := c.recommendationAZConfig() + if !ok { + az = c.cachedDominantAZConfig(ctx) } - - client, err := armconsumption.NewReservationsDetailsClient(c.cred, nil) - if err != nil { - return nil, err + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("database: enrich held reservations: %w", err) } - - scope := fmt.Sprintf("subscriptions/%s", c.subscriptionID) - return client.NewListPager(scope, &armconsumption.ReservationsDetailsClientListOptions{}), nil -} - -// collectSQLReservations collects SQL Database reservations from the pager. -// Returns an error on first pagination failure so callers can't silently act -// on a partial list — see the compute client for the full rationale. -func (c *Client) collectSQLReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - for pageIdx := 0; pager.More(); pageIdx++ { - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("context canceled during pagination: %w", err) - } - if pageIdx >= maxReservationsPages { - return nil, fmt.Errorf("database: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - return nil, fmt.Errorf("database: list reservations: %w", err) - } - - for _, detail := range page.Value { - if commitment := c.convertSQLReservation(detail); commitment != nil { - commitments = append(commitments, *commitment) - } - } - } - - // ReservationsDetails is a daily usage API: one row per reservation per - // usage day. Without this, a single reservation held for N days would - // surface as N commitments sharing one CommitmentID (issue #73). - return reservations.DedupeCommitmentsByID(commitments), nil -} - -// convertSQLReservation converts a reservation detail to a commitment if it's a SQL reservation. -func (c *Client) convertSQLReservation(detail *armconsumption.ReservationDetail) *common.Commitment { - if detail.Properties == nil { - return nil - } - - props := detail.Properties - if props.SKUName == nil || !strings.Contains(strings.ToLower(*props.SKUName), "sql") { - return nil - } - - commitment := &common.Commitment{ - Provider: common.ProviderAzure, - Account: c.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Service: common.ServiceRelationalDB, - Region: c.region, - State: common.CommitmentStateActive, - } - - if props.ReservationID != nil { - commitment.CommitmentID = *props.ReservationID - } - if props.SKUName != nil { - commitment.ResourceType = *props.SKUName + for i := range commitments { + commitments[i].Engine = sqlServerEngine + commitments[i].Deployment = az } - - return commitment + return commitments, nil } // PurchaseCommitment purchases SQL Database reserved capacity using the two-step @@ -674,7 +629,9 @@ func (c *Client) convertAzureSQLRecommendation(ctx context.Context, azureRec arm if entry, ok := c.cachedSKULookup(ctx, f.ResourceType); ok && entry.engineVersion != "" { details.EngineVersion = entry.engineVersion } - if az := c.cachedDominantAZConfig(ctx); az != "" { + az := c.cachedDominantAZConfig(ctx) + c.recordRecommendationAZConfig(az) + if az != "" { details.AZConfig = az } if dep := c.cachedDominantDeployment(ctx); dep != "" { @@ -783,6 +740,18 @@ func (c *Client) cachedDominantAZConfig(ctx context.Context) string { return az } +func (c *Client) recordRecommendationAZConfig(az string) { + c.serverInfoMu.Lock() + defer c.serverInfoMu.Unlock() + c.recAZConfig, c.recAZConfigSet = az, true +} + +func (c *Client) recommendationAZConfig() (string, bool) { + c.serverInfoMu.Lock() + defer c.serverInfoMu.Unlock() + return c.recAZConfig, c.recAZConfigSet +} + // cachedDominantDeployment returns the dominant Deployment observed // across the subscription's SQL resources, or "" when ambiguous or // unavailable. Values: "managed" / "single". Shares the single @@ -995,7 +964,7 @@ func (c *Client) createServersPager() (SQLServersPager, error) { func detailsFromSQLSKU(sku string) *common.DatabaseDetails { // Engine is always SQL Server for an Azure SQL Database reservation. return &common.DatabaseDetails{ - Engine: "sqlserver", + Engine: sqlServerEngine, InstanceClass: sku, } } diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index bbf7ee93..a384dc20 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -22,6 +22,8 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" azpricing "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/pricing" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // MockRecommendationsPager mocks the RecommendationsPager interface. @@ -43,29 +45,6 @@ func (m *MockRecommendationsPager) NextPage(ctx context.Context) (armconsumption return page, nil } -// MockReservationsDetailsPager mocks the ReservationsDetailsPager interface. -type MockReservationsDetailsPager struct { - pages []armconsumption.ReservationsDetailsClientListResponse - index int - err error -} - -func (m *MockReservationsDetailsPager) More() bool { - return m.index < len(m.pages) -} - -func (m *MockReservationsDetailsPager) NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - if m.err != nil { - return armconsumption.ReservationsDetailsClientListResponse{}, m.err - } - if m.index >= len(m.pages) { - return armconsumption.ReservationsDetailsClientListResponse{}, errors.New("no more pages") - } - page := m.pages[m.index] - m.index++ - return page, nil -} - // MockCapabilitiesClient mocks the CapabilitiesClient interface. // CallCount tracks how many times ListByLocation was invoked — used by // the cachedSKULookup-fetched-once test to pin the perf invariant. @@ -618,20 +597,6 @@ func TestDatabaseClient_GetOfferingDetails_ReservationAcrossPages(t *testing.T) } } -func TestDatabaseClient_GetExistingCommitments_Empty(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Mock pager returns no pages — the empty-subscription case. See the - // cache client for the full rationale. - client.SetReservationsPager(&MockReservationsDetailsPager{}) - - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) -} - func TestDatabaseClient_GetRecommendations_WithMockPager(t *testing.T) { t.Parallel() ctx := context.Background() @@ -683,167 +648,170 @@ func TestDatabaseClient_GetRecommendations_MultiplePages(t *testing.T) { assert.Empty(t, recs) } -func TestDatabaseClient_GetExistingCommitments_WithMockPager(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "sql-standard-s0" +// factoriesRows injects one order holding rows (and err on the page fetch). +func factoriesRows(err error, rows ...*armreservations.ReservationResponse) *reservations.InventoryFactories { + return factoriesFor(func() reservations.OrderReservationsPager { return &reservationstest.OrderPager{Rows: rows, Err: err} }) +} - // Create mock pager with SQL commitment - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - }, - }, - }, - }, - }, +// factoriesFor injects one reservation order whose pager is newPager(). +func factoriesFor(newPager func() reservations.OrderReservationsPager) *reservations.InventoryFactories { + return &reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return reservationstest.AppliedLister{}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return newPager() }, } +} - client.SetReservationsPager(mockPager) +func sqlRow(id, sku, location string, qty int32) *armreservations.ReservationResponse { + return reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDatabases, "sub", id, sku, location, qty) +} - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - require.Len(t, commitments, 1) - assert.Equal(t, reservationID, commitments[0].CommitmentID) - assert.Equal(t, skuName, commitments[0].ResourceType) - assert.Equal(t, common.ServiceRelationalDB, commitments[0].Service) +// inventoryClient returns a client with empty server pagers injected, so no +// inventory test can reach the network, and the pagers for call counting. +func inventoryClient(rows ...*armreservations.ReservationResponse) (*Client, *MockSQLManagedInstancesPager, *MockSQLServersPager) { + c := NewClient(nil, "sub", "eastus") + mi, servers := &MockSQLManagedInstancesPager{}, &MockSQLServersPager{} + c.SetManagedInstancesPager(mi) + c.SetServersPager(servers) + c.SetInventoryFactories(factoriesRows(nil, rows...)) + return c, mi, servers } -// TestDatabaseClient_GetExistingCommitments_DedupesDailyUsageRows is the -// regression test for #73: armconsumption.ReservationsDetails is a daily -// usage API, one row per reservation per usage day. A pager returning three -// rows for the same ReservationID must surface as a single commitment. -func TestDatabaseClient_GetExistingCommitments_DedupesDailyUsageRows(t *testing.T) { +// An empty inventory must not walk the subscription's SQL servers. +func TestGetExistingCommitments_EmptySkipsServerWalk(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "sql-standard-s0" - - dailyRow := &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - } - - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{dailyRow, dailyRow, dailyRow}, - }, - }, - }, - } - - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) + c, mi, servers := inventoryClient() + mi.pages = []armsql.ManagedInstancesClientListResponse{buildMIPage(true)} + servers.pages = []armsql.ServersClientListResponse{buildServerPage(1)} + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) - require.Len(t, commitments, 1, "three daily rows for one reservation must collapse to one commitment (issue #73)") - assert.Equal(t, reservationID, commitments[0].CommitmentID) + assert.Empty(t, commitments) + assert.Zero(t, mi.CallCount) + assert.Zero(t, servers.CallCount) } -func TestDatabaseClient_GetExistingCommitments_FilterNonSQL(t *testing.T) { +func TestGetExistingCommitments_SQLRowConverted(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Test that non-SQL SKUs are filtered out - nonSQLSKU := "redis-premium-p1" - sqlSKU := "sql-premium-p2" - reservationID1 := "test-reservation-1" - reservationID2 := "test-reservation-2" + c, _, _ := inventoryClient(sqlRow("res-123", "GP_Gen5_2", "westus2", 3)) + commitments, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, commitments, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-123", commitments[0].CommitmentID) + assert.Equal(t, "GP_Gen5_2", commitments[0].ResourceType) + assert.Equal(t, "westus2", commitments[0].Region, "reservation location must win over the client region") + assert.Equal(t, 3, commitments[0].Count) + assert.Equal(t, common.CommitmentStateActive, commitments[0].State) + assert.Equal(t, common.ServiceRelationalDB, commitments[0].Service) + assert.Equal(t, "sqlserver", commitments[0].Engine) +} - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID1, - SKUName: &nonSQLSKU, - }, - }, - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID2, - SKUName: &sqlSKU, - }, - }, - }, - }, - }, - }, +func TestGetExistingCommitments_DeploymentStamp(t *testing.T) { + t.Parallel() + for name, tc := range map[string]struct { + pages []armsql.ManagedInstancesClientListResponse + want string + }{ + "no managed instances": {nil, ""}, + "all zone-redundant": {[]armsql.ManagedInstancesClientListResponse{buildMIPage(true, true)}, "zoneRedundant"}, + "none zone-redundant": {[]armsql.ManagedInstancesClientListResponse{buildMIPage(false)}, "none"}, + "mixed fleet is unknown": {[]armsql.ManagedInstancesClientListResponse{buildMIPage(true, false)}, ""}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + c, mi, _ := inventoryClient(sqlRow("a", "GP_Gen5_2", "eastus", 1), sqlRow("b", "GP_Gen5_4", "eastus", 1)) + mi.pages = tc.pages + commitments, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, commitments, 2) + for _, commitment := range commitments { + assert.Equal(t, tc.want, commitment.Deployment) + } + }) } +} - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) +func TestGetExistingCommitments_NonSQLFiltered(t *testing.T) { + t.Parallel() + redisRow := sqlRow("res-1", "Premium_P1", "eastus", 1) + redisType := armreservations.ReservedResourceTypeRedisCache + redisRow.Properties.ReservedResourceType = &redisType + c, _, _ := inventoryClient(redisRow, sqlRow("res-2", "GP_Gen5_2", "eastus", 1)) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) require.Len(t, commitments, 1) - assert.Equal(t, reservationID2, commitments[0].CommitmentID) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-2", commitments[0].CommitmentID) } -func TestDatabaseClient_GetExistingCommitments_NilProperties(t *testing.T) { +// A row the guard cannot classify may be the reservation bought an hour ago, +// so it fails closed instead of being dropped, without walking servers. +func TestGetExistingCommitments_NilPropertiesFailsClosed(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Test that nil properties are handled gracefully - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: nil, - }, - }, - }, - }, - }, - } + c, mi, servers := inventoryClient(&armreservations.ReservationResponse{}) + mi.pages = []armsql.ManagedInstancesClientListResponse{buildMIPage(true)} + servers.pages = []armsql.ServersClientListResponse{buildServerPage(1)} + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Zero(t, mi.CallCount) + assert.Zero(t, servers.CallCount) +} - client.SetReservationsPager(mockPager) +// A cancellation during the enrichment walk must surface, not return +// inventory stamped with a blank deployment. +func TestGetExistingCommitments_CancelDuringEnrichmentWalk(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c, _, servers := inventoryClient(sqlRow("res-1", "GP_Gen5_2", "eastus", 1)) + c.SetManagedInstancesPager(&cancelAfterPagesMIPager{ + MockSQLManagedInstancesPager: MockSQLManagedInstancesPager{pages: []armsql.ManagedInstancesClientListResponse{buildMIPage(true)}}, + cancel: cancel, + }) + servers.pages = []armsql.ServersClientListResponse{buildServerPage(1)} + commitments, err := c.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Nil(t, commitments) +} - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) +func TestGetExistingCommitments_PagerError(t *testing.T) { + t.Parallel() + c, _, _ := inventoryClient() + c.SetInventoryFactories(factoriesRows(errors.New("api error"))) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "api error") } -func TestDatabaseClient_GetExistingCommitments_PagerError(t *testing.T) { +func TestGetExistingCommitments_ListerError(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") + c, _, _ := inventoryClient() + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: errors.New("boom")}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return &reservationstest.OrderPager{} }, + }) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err, "a failed inventory lookup must not masquerade as an empty list") + assert.Contains(t, err.Error(), "database: list applied reservations") +} - // Pagination errors must propagate — returning a partial commitment list - // silently is unsafe for the purchase flow (it could trigger duplicate - // purchases for reservations we didn't see). - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{{}}, - err: errors.New("API error"), +func TestNewClient_DefaultInventoryFactories(t *testing.T) { + t.Parallel() + for name, c := range map[string]*Client{ + "NewClient": NewClient(nil, "sub", "eastus"), + "NewClientWithHTTP": NewClientWithHTTP(nil, "sub", "eastus", nil), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + require.Nil(t, c.inventoryFactories, "production wiring injects nothing") + f := c.factories() + assert.NotNil(t, f.NewAppliedLister) + assert.NotNil(t, f.NewOrderPager) + }) } - - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "database: list reservations") - assert.Nil(t, commitments) + injected := &reservations.InventoryFactories{} + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(injected) + assert.Equal(t, *injected, c.factories()) } func TestDatabaseClient_GetValidResourceTypes_WithMock(t *testing.T) { @@ -959,10 +927,10 @@ func TestDatabaseClient_SetterMethods(t *testing.T) { client.SetRecommendationsPager(mockRecPager) assert.Equal(t, mockRecPager, client.recommendationsPager) - // Test SetReservationsPager - mockResPager := &MockReservationsDetailsPager{} - client.SetReservationsPager(mockResPager) - assert.Equal(t, mockResPager, client.reservationsPager) + // Test SetInventoryFactories + factories := &reservations.InventoryFactories{} + client.SetInventoryFactories(factories) + assert.Same(t, factories, client.inventoryFactories) // Test SetCapabilitiesClient mockCapClient := &MockCapabilitiesClient{} diff --git a/providers/azure/services/database/held_reservation_dedupe_test.go b/providers/azure/services/database/held_reservation_dedupe_test.go new file mode 100644 index 00000000..6a109c3d --- /dev/null +++ b/providers/azure/services/database/held_reservation_dedupe_test.go @@ -0,0 +1,241 @@ +package database + +import ( + "context" + "encoding/json" + "errors" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" +) + +// Issue #705: a SQL Database reservation CUDly (or anyone) already holds must +// reduce the matching recommendation. Fixture dates are UTC calendar dates, +// because the Reservations API returns date-only values. +var ( + fixtureToday = time.Now().UTC().Truncate(24 * time.Hour) + fixturePast = fixtureToday.AddDate(0, 0, -30) +) + +const sqlSKU = "GP_Gen5_2" + +func testPricer() *recommendations.Pricer { + return recommendations.NewPricer("database", func(context.Context, string, string, int) (recommendations.ReservationPrice, error) { + return recommendations.ReservationPrice{Total: 1000, Currency: "USD"}, nil + }) +} + +// fleetClient returns a client whose subscription has the given managed +// instances (one ZoneRedundant flag each); the same client serves the +// recommendation and the held-reservation lookup, as in production. +func fleetClient(zoneRedundant ...bool) *Client { + c := NewClient(nil, "sub", "eastus") + c.SetCapabilitiesClient(&MockCapabilitiesClient{}) + mi := &MockSQLManagedInstancesPager{} + if len(zoneRedundant) > 0 { + mi.pages = []armsql.ManagedInstancesClientListResponse{buildMIPage(zoneRedundant...)} + } + c.SetManagedInstancesPager(mi) + c.SetServersPager(&MockSQLServersPager{}) + return c +} + +// upfrontRec runs the real converter (including the AZConfig enrichment) and +// variant expansion for a count-2 P1Y recommendation and returns its upfront +// variant. +func upfrontRec(t *testing.T, c *Client) common.Recommendation { + t.Helper() + raw := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), mocks.WithTerm("P1Y"), mocks.WithQuantity(2), + mocks.WithNormalizedSize(sqlSKU), mocks.WithCosts(1000, 700, 300)) + recs, err := c.appendPage(context.Background(), testPricer(), nil, + []armconsumption.ReservationRecommendationClassification{raw}) + require.NoError(t, err) + require.Len(t, recs, 2) + upfront := recs[0] + require.Equal(t, "upfront", upfront.PaymentOption) + require.Equal(t, 2, upfront.Count) + require.Equal(t, "sqlserver", common.EngineFromDetails(upfront.Details)) + return upfront +} + +// purchasedRow buys rec through a capturing transport and builds the held +// reservation from the request body CUDly actually sent. +func purchasedRow(t *testing.T, region string, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { + t.Helper() + capture := &reservationstest.Capture{} + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", region, capture) + _, err := buyer.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + var body struct { + Location string `json:"location"` + SKU struct { + Name string `json:"name"` + } `json:"sku"` + Properties struct { + ReservedResourceType string `json:"reservedResourceType"` + Quantity int32 `json:"quantity"` + } `json:"properties"` + } + require.NoError(t, json.Unmarshal(capture.BodyBytes, &body)) + require.Equal(t, rec.Region, body.Location) + require.Equal(t, rec.Count, int(body.Properties.Quantity)) + return reservationstest.LiveRow(armreservations.ReservedResourceType(body.Properties.ReservedResourceType), + "sub", "held", body.SKU.Name, body.Location, qty) +} + +func adjust(t *testing.T, c *Client, recs ...common.Recommendation) ([]common.Recommendation, error) { + t.Helper() + passed, _, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, c) + return passed, err +} + +func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { + t.Parallel() + c := fleetClient() + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count) + assert.InDelta(t, upfront.CommitmentCost/2, passed[0].CommitmentCost, 0.001, "money fields scale with the count") +} + +func TestHeldReservation_FullHoldFiltersRecommendation(t *testing.T) { + t.Parallel() + c := fleetClient() + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 2))) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + assert.Empty(t, passed) +} + +func TestHeldReservation_Table(t *testing.T) { + t.Parallel() + state := func(s armreservations.ProvisioningState) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ProvisioningState = &s } + } + expiry := func(d time.Time) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ExpiryDate = &d } + } + cases := []struct { + name string + mutate func(*armreservations.ReservationResponse) + want int + wantErr string // non-empty: the lookup must fail with this text + }{ + {"shared, this subscription", func(*armreservations.ReservationResponse) {}, 1, ""}, + {"creating", state(armreservations.ProvisioningStateCreating), 1, ""}, + {"shared, billed to another subscription", func(r *armreservations.ReservationResponse) { + r.Properties.BillingScopeID = mocks.StringPtr("/subscriptions/other") + }, 2, ""}, + {"canceled", state(armreservations.ProvisioningStateCancelled), 2, ""}, + {"expired state", state(armreservations.ProvisioningStateExpired), 2, ""}, + {"succeeded past expiry", expiry(fixturePast), 2, ""}, + {"other region", func(r *armreservations.ReservationResponse) { r.Location = mocks.StringPtr("westus") }, 2, ""}, + {"other sku", func(r *armreservations.ReservationResponse) { r.SKU.Name = mocks.StringPtr("GP_Gen5_4") }, 2, ""}, + {"missing quantity", func(r *armreservations.ReservationResponse) { r.Properties.Quantity = nil }, 0, "quantity"}, + {"expires today", expiry(fixtureToday), 0, "expiryDate"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + c := fleetClient() + upfront := upfrontRec(t, c) + row := purchasedRow(t, c.region, upfront, 1) + tc.mutate(row) + c.SetInventoryFactories(factoriesRows(nil, row)) + passed, err := adjust(t, c, upfront) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, tc.want, passed[0].Count) + }) + } +} + +// The key carries the subscription's dominant zone-redundancy, which the held +// reservation must match exactly or the guard fails open. +func TestHeldReservation_FleetShapes(t *testing.T) { + t.Parallel() + for name, tc := range map[string]struct { + fleet []bool + miErr error + az string + }{ + "no managed instances": {nil, nil, ""}, + "all zone-redundant": {[]bool{true, true}, nil, "zoneRedundant"}, + "none zone-redundant": {[]bool{false}, nil, "none"}, + "mixed fleet": {[]bool{true, false}, nil, ""}, + "managed-instance walk fails": {nil, errors.New("mi list failed"), ""}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + c := fleetClient(tc.fleet...) + var failing *MockSQLManagedInstancesPager + if tc.miErr != nil { + failing = &MockSQLManagedInstancesPager{err: tc.miErr, pages: []armsql.ManagedInstancesClientListResponse{buildMIPage(true)}} + c.SetManagedInstancesPager(failing) + } + upfront := upfrontRec(t, c) + require.Equal(t, tc.az, common.DeploymentFromDetails(upfront.Details), "the rec key's deployment") + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + held, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, held, 1) + assert.Equal(t, tc.az, held[0].Deployment) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count, "the hold must match the key whatever the fleet shape") + if failing != nil { + assert.Positive(t, failing.CallCount, "the failing walk must actually run") + } + }) + } +} + +// A walk that fails on the recommendation call and succeeds on the inventory +// call must not give the held reservation a different deployment than the key +// the recommendation already carries. +func TestHeldReservation_UsesRecommendationsAZSnapshot(t *testing.T) { + t.Parallel() + c := fleetClient() + c.SetManagedInstancesPager(&flakyMIPager{failures: 1}) + c.serverInfoRetryDelay = 10 * time.Millisecond + upfront := upfrontRec(t, c) + require.Empty(t, common.DeploymentFromDetails(upfront.Details), "the rec was keyed while the walk failed") + time.Sleep(30 * time.Millisecond) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + held, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, held, 1) + assert.Empty(t, held[0].Deployment, "the held reservation must reuse the rec's snapshot") + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count) +} diff --git a/providers/azure/services/database/pagination_cap_test.go b/providers/azure/services/database/pagination_cap_test.go index 9f8e28a9..3949fcc9 100644 --- a/providers/azure/services/database/pagination_cap_test.go +++ b/providers/azure/services/database/pagination_cap_test.go @@ -6,6 +6,10 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // countingMIPager and countingServersPager report More() until limit pages @@ -178,3 +182,58 @@ func TestHasRegularServers_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing assert.True(t, complete) assert.Equal(t, maxSQLListPages, pager.calls) } + +func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { + t.Parallel() + pager := &reservationstest.CountingPager{Limit: 1000} + client, _, _ := inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap") + assert.Equal(t, maxReservationsPages, pager.Calls) +} + +func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + pager := &reservationstest.CountingPager{Limit: 1000} + client, _, _ := inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, pager.Calls) +} + +func TestGetExistingCommitments_CapIsExact(t *testing.T) { + t.Parallel() + atCap := &reservationstest.CountingPager{Limit: maxReservationsPages} + client, _, _ := inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return atCap })) + _, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err, "exactly cap pages must succeed") + assert.Equal(t, maxReservationsPages, atCap.Calls) + + overCap := &reservationstest.CountingPager{Limit: maxReservationsPages + 1} + client, _, _ = inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) + _, err = client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Equal(t, maxReservationsPages, overCap.Calls) +} + +func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pager := &reservationstest.CancelOnSecondCallPager{Cancel: cancel} + client, _, _ := inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 2, pager.Calls) +} diff --git a/providers/azure/services/database/slot_wiring_test.go b/providers/azure/services/database/slot_wiring_test.go index e197b808..775a8c62 100644 --- a/providers/azure/services/database/slot_wiring_test.go +++ b/providers/azure/services/database/slot_wiring_test.go @@ -8,9 +8,12 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot/slottest" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // onePager yields a single empty page and asserts the slot is held while @@ -28,6 +31,14 @@ func (p *onePager[T]) NextPage(context.Context) (T, error) { return zero, nil } +// slotLister reports one order so the order pager is exercised too. +type slotLister struct{ probe *slottest.Probe } + +func (l slotLister) GetAppliedReservationList(ctx context.Context, sub string, o *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + l.probe.Check() + return reservationstest.AppliedLister{}.GetAppliedReservationList(ctx, sub, o) +} + type probeCaps struct{ probe *slottest.Probe } func (c probeCaps) ListByLocation(context.Context, string, *armsql.CapabilitiesClientListByLocationOptions) (armsql.CapabilitiesClientListByLocationResponse, error) { @@ -50,7 +61,14 @@ func TestClientCallsHoldSharedSlot(t *testing.T) { _, _ = c.GetRecommendations(ctx, nil) }, "GetExistingCommitments": func(ctx context.Context, p *slottest.Probe, c *Client) { - c.SetReservationsPager(&onePager[armconsumption.ReservationsDetailsClientListResponse]{probe: p}) + c.SetManagedInstancesPager(&MockSQLManagedInstancesPager{}) + c.SetServersPager(&MockSQLServersPager{}) + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return slotLister{p}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &onePager[armreservations.ReservationClientListResponse]{probe: p} + }, + }) _, _ = c.GetExistingCommitments(ctx) }, "GetValidResourceTypes": func(ctx context.Context, p *slottest.Probe, c *Client) { diff --git a/providers/azure/services/internal/reservations/reservationstest/capture.go b/providers/azure/services/internal/reservations/reservationstest/capture.go index 43f671bf..7cb3c147 100644 --- a/providers/azure/services/internal/reservations/reservationstest/capture.go +++ b/providers/azure/services/internal/reservations/reservationstest/capture.go @@ -1,5 +1,6 @@ // Package reservationstest provides a fake ARM transport for asserting the -// purchase request bodies the Azure service clients send. +// purchase request bodies the Azure service clients send, and fakes for the +// reservation inventory path (live rows, applied lister, order pager). package reservationstest import ( diff --git a/providers/azure/services/internal/reservations/reservationstest/inventory.go b/providers/azure/services/internal/reservations/reservationstest/inventory.go new file mode 100644 index 00000000..352a3cb6 --- /dev/null +++ b/providers/azure/services/internal/reservations/reservationstest/inventory.go @@ -0,0 +1,102 @@ +package reservationstest + +import ( + "context" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" +) + +// OrderPath is the reservation order ID the fakes below serve. +const OrderPath = "/providers/Microsoft.Capacity/reservationOrders/order-1" + +// LiveRow is a complete live reservation of resourceType: Shared scope billed +// to subscriptionID, Succeeded, purchased today and expiring next year. +func LiveRow(resourceType armreservations.ReservedResourceType, subscriptionID, id, sku, location string, qty int32) *armreservations.ReservationResponse { + shared := armreservations.AppliedScopeTypeShared + succeeded := armreservations.ProvisioningStateSucceeded + purchased := time.Now().UTC().Truncate(24 * time.Hour) + expiry := purchased.AddDate(1, 0, 0) + billing := "/subscriptions/" + subscriptionID + path := OrderPath + "/reservations/" + id + return &armreservations.ReservationResponse{ + ID: &path, + Location: &location, + SKU: &armreservations.SKUName{Name: &sku}, + Properties: &armreservations.Properties{ + ReservedResourceType: &resourceType, + AppliedScopeType: &shared, + BillingScopeID: &billing, + ProvisioningState: &succeeded, + Quantity: &qty, + PurchaseDate: &purchased, + ExpiryDate: &expiry, + }, + } +} + +// AppliedLister serves OrderPath as the only applied order, or Err. +type AppliedLister struct { + Err error +} + +func (l AppliedLister) GetAppliedReservationList(context.Context, string, *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + if l.Err != nil { + return armreservations.AzureReservationAPIClientGetAppliedReservationListResponse{}, l.Err + } + order := OrderPath + return armreservations.AzureReservationAPIClientGetAppliedReservationListResponse{ + AppliedReservations: armreservations.AppliedReservations{ + Properties: &armreservations.AppliedReservationsProperties{ + ReservationOrderIDs: &armreservations.AppliedReservationList{Value: []*string{&order}}, + }, + }, + }, nil +} + +// OrderPager is a single-page pager over Rows, or Err on the page fetch. +type OrderPager struct { + Err error + Rows []*armreservations.ReservationResponse + done bool +} + +func (p *OrderPager) More() bool { return !p.done } + +func (p *OrderPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + p.done = true + if p.Err != nil { + return armreservations.ReservationClientListResponse{}, p.Err + } + return armreservations.ReservationClientListResponse{ReservationList: armreservations.ReservationList{Value: p.Rows}}, nil +} + +// CountingPager reports More until Limit pages were requested, so a missing +// pagination cap shows up as Calls == Limit instead of a hung test. +type CountingPager struct { + Calls, Limit int +} + +func (p *CountingPager) More() bool { return p.Calls < p.Limit } + +func (p *CountingPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + p.Calls++ + return armreservations.ReservationClientListResponse{}, nil +} + +// CancelOnSecondCallPager never ends and calls Cancel while serving its +// second page. +type CancelOnSecondCallPager struct { + Cancel context.CancelFunc + Calls int +} + +func (p *CancelOnSecondCallPager) More() bool { return true } + +func (p *CancelOnSecondCallPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + p.Calls++ + if p.Calls == 2 { + p.Cancel() + } + return armreservations.ReservationClientListResponse{}, nil +} diff --git a/providers/azure/services/internal/reservations/strict.go b/providers/azure/services/internal/reservations/strict.go index bb178b9f..d9ad349f 100644 --- a/providers/azure/services/internal/reservations/strict.go +++ b/providers/azure/services/internal/reservations/strict.go @@ -1,6 +1,7 @@ package reservations import ( + "context" "fmt" "log" "strings" @@ -19,6 +20,28 @@ type StrictLabels struct { Resource string } +// StrictCommitments lists the reservations applied to subscriptionID and +// converts each with StrictCommitment, so any lister failure or incomplete +// live row fails the whole inventory instead of yielding a partial one. +func StrictCommitments(ctx context.Context, subscriptionID string, f InventoryFactories, maxPages int, service common.ServiceType, wantType armreservations.ReservedResourceType, labels StrictLabels) ([]common.Commitment, error) { + responses, err := ListAppliedReservations(ctx, subscriptionID, f, maxPages) + if err != nil { + return nil, fmt.Errorf("%s: list applied reservations: %w", labels.Service, err) + } + now := time.Now() + commitments := make([]common.Commitment, 0, len(responses)) + for _, r := range responses { + commitment, err := StrictCommitment(r, subscriptionID, service, wantType, labels, now) + if err != nil { + return nil, err + } + if commitment != nil { + commitments = append(commitments, *commitment) + } + } + return commitments, nil +} + // StrictCommitment validates and converts one reservation, failing closed // instead of dropping or zero-counting incomplete inventory. It returns a nil // commitment for reservations of another type. diff --git a/providers/azure/services/internal/reservations/strict_commitments_test.go b/providers/azure/services/internal/reservations/strict_commitments_test.go new file mode 100644 index 00000000..320aa968 --- /dev/null +++ b/providers/azure/services/internal/reservations/strict_commitments_test.go @@ -0,0 +1,78 @@ +package reservations_test + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" +) + +const strictSub = "sub-a" + +var strictLabels = reservations.StrictLabels{Service: "svc", Resource: "Redis"} + +func strictFactories(listErr error, rows ...*armreservations.ReservationResponse) reservations.InventoryFactories { + return reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: listErr}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &reservationstest.OrderPager{Rows: rows} + }, + } +} + +func strictList(f reservations.InventoryFactories) ([]common.Commitment, error) { + return reservations.StrictCommitments(context.Background(), strictSub, f, 5, common.ServiceMemoryDB, + armreservations.ReservedResourceTypeRedisCache, strictLabels) +} + +func TestStrictCommitments_ListErrorKeepsPrefixAndCause(t *testing.T) { + t.Parallel() + cause := errors.New("boom") + got, err := strictList(strictFactories(cause)) + assert.Nil(t, got) + require.ErrorIs(t, err, cause) + assert.Contains(t, err.Error(), "svc: list applied reservations: ") +} + +func TestStrictCommitments_IgnoresOtherTypes(t *testing.T) { + t.Parallel() + got, err := strictList(strictFactories(nil, + reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDatabases, strictSub, "sql", "S0", "eastus", 1), + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "redis", "P1", "eastus", 1))) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/redis", got[0].CommitmentID) +} + +func TestStrictCommitments_IncompleteLiveRowFailsClosed(t *testing.T) { + t.Parallel() + row := reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "redis", "P1", "eastus", 1) + row.Properties.Quantity = nil + got, err := strictList(strictFactories(nil, row)) + assert.Nil(t, got) + require.ErrorContains(t, err, "quantity") +} + +func TestStrictCommitments_ReturnsConvertedFields(t *testing.T) { + t.Parallel() + got, err := strictList(strictFactories(nil, + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "a", "P1", "westus2", 3), + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "b", "P2", "eastus", 1))) + require.NoError(t, err) + require.Len(t, got, 2) + assert.Equal(t, "P1", got[0].ResourceType) + assert.Equal(t, "westus2", got[0].Region) + assert.Equal(t, 3, got[0].Count) + assert.Equal(t, common.ServiceMemoryDB, got[0].Service) + assert.Equal(t, common.CommitmentStateActive, got[0].State) + assert.Equal(t, "P2", got[1].ResourceType) +} diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index c6298322..545a0bce 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -204,25 +204,15 @@ func (c *Client) appendPage(ctx context.Context, pricer *recommendations.Pricer, // guard needs (issue #699). Like compute, any failure or incomplete live row // is returned as an error rather than as an empty or partial inventory. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - responses, err := reservations.ListAppliedReservations(ctx, c.subscriptionID, c.factories(), maxReservationsPages) + commitments, err := reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceMemoryDB, + armreservations.ReservedResourceTypeRedisCache, reservations.StrictLabels{Service: "managedredis", Resource: "Redis"}) if err != nil { - return nil, fmt.Errorf("managedredis: list applied reservations: %w", err) + return nil, err } - - now := time.Now() - commitments := make([]common.Commitment, 0, len(responses)) - for _, r := range responses { - commitment, err := reservations.StrictCommitment(r, c.subscriptionID, common.ServiceMemoryDB, - armreservations.ReservedResourceTypeRedisCache, reservations.StrictLabels{Service: "managedredis", Resource: "Redis"}, now) - if err != nil { - return nil, err - } - if commitment != nil { - // The dedupe key includes the engine and recommendations carry - // CacheDetails.Engine "redis", so held reservations must too. - commitment.Engine = Engine - commitments = append(commitments, *commitment) - } + // The dedupe key includes the engine and recommendations carry + // CacheDetails.Engine "redis", so held reservations must too. + for i := range commitments { + commitments[i].Engine = Engine } return commitments, nil } diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index 0d79c14f..3f422a77 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -49,18 +49,12 @@ type RecommendationsPager interface { NextPage(ctx context.Context) (armconsumption.ReservationRecommendationsClientListResponse, error) } -// ReservationsDetailsPager interface for reservations details pager (enables mocking). -type ReservationsDetailsPager interface { - More() bool - NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) -} - // Client handles Azure Synapse Analytics Reserved Capacity. type Client struct { cred azcore.TokenCredential httpClient HTTPClient recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager + inventoryFactories *reservations.InventoryFactories subscriptionID string region string } @@ -97,9 +91,18 @@ func (c *Client) SetRecommendationsPager(pager RecommendationsPager) { c.recommendationsPager = pager } -// SetReservationsPager sets the reservations pager (for testing). -func (c *Client) SetReservationsPager(pager ReservationsDetailsPager) { - c.reservationsPager = pager +// SetInventoryFactories injects reservation-inventory factories (for testing). +func (c *Client) SetInventoryFactories(f *reservations.InventoryFactories) { + c.inventoryFactories = f +} + +// factories returns the injected inventory factories, or the real SDK-backed +// ones when none were injected (production only calls NewClient). +func (c *Client) factories() reservations.InventoryFactories { + if c.inventoryFactories != nil { + return *c.inventoryFactories + } + return reservations.DefaultInventoryFactories(c.cred) } // GetServiceType returns the service type. @@ -194,93 +197,19 @@ func (c *Client) appendRegionRecommendations(ctx context.Context, pricer *recomm return recs, nil } -// GetExistingCommitments retrieves existing Synapse reserved capacity -// commitments from the Azure Consumption API. +// GetExistingCommitments retrieves the Synapse reservations that apply to this +// subscription from the Reservations API, which carries the purchase date, +// quantity, location and lifecycle state the duplicate-purchase guard needs +// (issue #705). Any failure or incomplete live row is returned as an error +// rather than as an empty or partial inventory. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - pager, err := c.createReservationsPager() - if err != nil { - return nil, fmt.Errorf("synapse: create reservations pager: %w", err) - } - - return c.collectSynapseReservations(ctx, pager) -} - -func (c *Client) createReservationsPager() (ReservationsDetailsPager, error) { - if c.reservationsPager != nil { - return c.reservationsPager, nil - } - client, err := armconsumption.NewReservationsDetailsClient(c.cred, nil) - if err != nil { - return nil, err - } - scope := fmt.Sprintf("subscriptions/%s", c.subscriptionID) - return client.NewListPager(scope, &armconsumption.ReservationsDetailsClientListOptions{}), nil -} - -func (c *Client) collectSynapseReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - for pageIdx := 0; pager.More(); pageIdx++ { - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("context canceled during pagination: %w", err) - } - if pageIdx >= maxReservationsPages { - return nil, fmt.Errorf("synapse: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - return nil, fmt.Errorf("synapse: list reservations: %w", err) - } - for _, detail := range page.Value { - if commitment := c.convertSynapseReservation(detail); commitment != nil { - commitments = append(commitments, *commitment) - } - } - } - - // ReservationsDetails is a daily usage API: one row per reservation per - // usage day. Without this, a single reservation held for N days would - // surface as N commitments sharing one CommitmentID (issue #73). - return reservations.DedupeCommitmentsByID(commitments), nil -} - -// convertSynapseReservation converts a reservation detail to a Commitment if -// it is a Synapse SQL Pool or Spark reservation. Identification relies on the -// SKU name containing a Synapse-specific prefix ("DW" for Dedicated SQL Pools -// or "SCU" for Spark Compute Units). -func (c *Client) convertSynapseReservation(detail *armconsumption.ReservationDetail) *common.Commitment { - if detail == nil || detail.Properties == nil { - return nil - } - props := detail.Properties - if props.SKUName == nil { - return nil - } - skuLower := strings.ToLower(*props.SKUName) - if !strings.HasPrefix(skuLower, "dw") && - !strings.HasPrefix(skuLower, "scu") && - !strings.Contains(skuLower, "synapse") { - return nil - } - - commitment := &common.Commitment{ - Provider: common.ProviderAzure, - Account: c.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Service: common.ServiceDataWarehouse, - Region: c.region, - State: common.CommitmentStateActive, - } - if props.ReservationID != nil { - commitment.CommitmentID = *props.ReservationID - } - commitment.ResourceType = *props.SKUName - return commitment + return reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceDataWarehouse, + armreservations.ReservedResourceTypeSQLDataWarehouse, reservations.StrictLabels{Service: "synapse", Resource: "Synapse"}) } // PurchaseCommitment purchases Synapse reserved capacity via the Azure // Reservations API two-step flow (calculatePrice -> purchase). The reserved -// resource type is "SqlDW" which covers Dedicated SQL Pool DWU reservations. +// resource type is "SqlDataWarehouse" which covers Dedicated SQL Pool DWU reservations. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 9b1c85bc..0804e14e 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -23,6 +23,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // ---- credential mock ------------------------------------------------------- @@ -78,28 +79,6 @@ func (e *errorRecommendationsPager) NextPage(_ context.Context) (armconsumption. return armconsumption.ReservationRecommendationsClientListResponse{}, errors.New("API error") } -type fakeReservationsPager struct { - pages []armconsumption.ReservationsDetailsClientListResponse - index int - err error -} - -func (m *fakeReservationsPager) More() bool { - return m.index < len(m.pages) -} - -func (m *fakeReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - if m.err != nil { - return armconsumption.ReservationsDetailsClientListResponse{}, m.err - } - if m.index >= len(m.pages) { - return armconsumption.ReservationsDetailsClientListResponse{}, errors.New("no more pages") - } - page := m.pages[m.index] - m.index++ - return page, nil -} - // ---- helpers --------------------------------------------------------------- func newTestClient() *Client { @@ -328,117 +307,114 @@ func TestGetRecommendations_modernShape(t *testing.T) { // ---- GetExistingCommitments ----------------------------------------------- +// factoriesRows injects one order holding rows (and err on the page fetch). +func factoriesRows(err error, rows ...*armreservations.ReservationResponse) *reservations.InventoryFactories { + return factoriesFor(func() reservations.OrderReservationsPager { return &reservationstest.OrderPager{Rows: rows, Err: err} }) +} + +// factoriesFor injects one reservation order whose pager is newPager(). +func factoriesFor(newPager func() reservations.OrderReservationsPager) *reservations.InventoryFactories { + return &reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return reservationstest.AppliedLister{}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return newPager() }, + } +} + +func synapseRow(id, sku, location string, qty int32) *armreservations.ReservationResponse { + return reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDataWarehouse, "sub-123", id, sku, location, qty) +} + func TestGetExistingCommitments_empty(t *testing.T) { t.Parallel() c := newTestClient() - c.SetReservationsPager(&fakeReservationsPager{}) - + c.SetInventoryFactories(factoriesRows(nil)) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) assert.Empty(t, commitments) } -func TestGetExistingCommitments_synapseSKU(t *testing.T) { +func TestGetExistingCommitments_synapseRowConverted(t *testing.T) { t.Parallel() c := newTestClient() - - skuName := "DW1000c" - reservationID := "synapse-res-123" - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &skuName, - ReservationID: &reservationID, - }, - }, - }, - }, - }, - }, - }) - + c.SetInventoryFactories(factoriesRows(nil, synapseRow("res-123", "DW1000c", "westus2", 3))) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) require.Len(t, commitments, 1) - assert.Equal(t, common.ServiceDataWarehouse, commitments[0].Service) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-123", commitments[0].CommitmentID) assert.Equal(t, "DW1000c", commitments[0].ResourceType) - assert.Equal(t, "synapse-res-123", commitments[0].CommitmentID) - assert.Equal(t, common.CommitmentReservedInstance, commitments[0].CommitmentType) + assert.Equal(t, "westus2", commitments[0].Region, "reservation location must win over the client region") + assert.Equal(t, 3, commitments[0].Count) + assert.Equal(t, common.CommitmentStateActive, commitments[0].State) + assert.Equal(t, common.ServiceDataWarehouse, commitments[0].Service) assert.Equal(t, common.ProviderAzure, commitments[0].Provider) + assert.Empty(t, commitments[0].Engine, "DataWarehouseDetails keys carry no engine") + assert.Empty(t, commitments[0].Deployment, "DataWarehouseDetails keys carry no deployment") } -// TestGetExistingCommitments_dedupesDailyUsageRows is the regression test -// for #73: armconsumption.ReservationsDetails is a daily usage API, one row -// per reservation per usage day. A pager returning three rows for the same -// ReservationID must surface as a single commitment. -func TestGetExistingCommitments_dedupesDailyUsageRows(t *testing.T) { +func TestGetExistingCommitments_nonSynapseFiltered(t *testing.T) { t.Parallel() c := newTestClient() - - skuName := "DW1000c" - reservationID := "synapse-res-123" - dailyRow := &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &skuName, - ReservationID: &reservationID, - }, - } - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{dailyRow, dailyRow, dailyRow}, - }, - }, - }, - }) - + sqlRow := synapseRow("res-1", "Standard_S0", "eastus", 1) + sqlType := armreservations.ReservedResourceTypeSQLDatabases + sqlRow.Properties.ReservedResourceType = &sqlType + c.SetInventoryFactories(factoriesRows(nil, sqlRow, synapseRow("res-2", "DW1000c", "eastus", 1))) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) - require.Len(t, commitments, 1, "three daily rows for one reservation must collapse to one commitment (issue #73)") - assert.Equal(t, "synapse-res-123", commitments[0].CommitmentID) + require.Len(t, commitments, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-2", commitments[0].CommitmentID) } -func TestGetExistingCommitments_filterNonSynapse(t *testing.T) { +// A row the guard cannot classify may be the reservation bought an hour ago, +// so it fails closed instead of being dropped. +func TestGetExistingCommitments_nilPropertiesFailsClosed(t *testing.T) { t.Parallel() c := newTestClient() - - vmSKU := "Standard_D2s_v3" - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &vmSKU, - }, - }, - }, - }, - }, - }, - }) - - commitments, err := c.GetExistingCommitments(context.Background()) - require.NoError(t, err) - assert.Empty(t, commitments, "non-Synapse SKU should be filtered out") + c.SetInventoryFactories(factoriesRows(nil, &armreservations.ReservationResponse{})) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) } func TestGetExistingCommitments_pagerError(t *testing.T) { t.Parallel() c := newTestClient() - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{{}}, - err: errors.New("pager error"), - }) + c.SetInventoryFactories(factoriesRows(errors.New("api error"))) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "api error") +} +func TestGetExistingCommitments_listerError(t *testing.T) { + t.Parallel() + c := newTestClient() + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: errors.New("boom")}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return &reservationstest.OrderPager{} }, + }) _, err := c.GetExistingCommitments(context.Background()) - assert.Error(t, err) + require.Error(t, err, "a failed inventory lookup must not masquerade as an empty list") + assert.Contains(t, err.Error(), "synapse: list applied reservations") +} + +func TestNewClient_DefaultInventoryFactories(t *testing.T) { + t.Parallel() + for name, c := range map[string]*Client{ + "NewClient": NewClient(nil, "sub", "eastus"), + "NewClientWithHTTP": NewClientWithHTTP(nil, "sub", "eastus", nil), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + require.Nil(t, c.inventoryFactories, "production wiring injects nothing") + f := c.factories() + assert.NotNil(t, f.NewAppliedLister) + assert.NotNil(t, f.NewOrderPager) + }) + } + injected := &reservations.InventoryFactories{} + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(injected) + assert.Equal(t, *injected, c.factories()) } // ---- ValidateOffering ----------------------------------------------------- @@ -822,82 +798,6 @@ func TestPurchaseCommitment_tokenError(t *testing.T) { assert.False(t, result.Success) } -// ---- convertSynapseReservation -------------------------------------------- - -func TestConvertSynapseReservation_nil(t *testing.T) { - t.Parallel() - c := newTestClient() - assert.Nil(t, c.convertSynapseReservation(nil)) -} - -func TestConvertSynapseReservation_nilProperties(t *testing.T) { - t.Parallel() - c := newTestClient() - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{})) -} - -func TestConvertSynapseReservation_nonSynapseSKU(t *testing.T) { - t.Parallel() - c := newTestClient() - sku := "Premium_P1" - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{SKUName: &sku}, - })) -} - -func TestConvertSynapseReservation_dwSKU(t *testing.T) { - t.Parallel() - c := newTestClient() - sku := "DW3000c" - resID := "res-abc" - commitment := c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &sku, - ReservationID: &resID, - }, - }) - require.NotNil(t, commitment) - assert.Equal(t, "DW3000c", commitment.ResourceType) - assert.Equal(t, "res-abc", commitment.CommitmentID) - assert.Equal(t, common.ServiceDataWarehouse, commitment.Service) -} - -func TestConvertSynapseReservation_scuPrefix(t *testing.T) { - t.Parallel() - c := newTestClient() - // SCU prefix (Spark Compute Units) should be classified as Synapse. - sku := "SCU_Standard" - resID := "res-scu" - commitment := c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &sku, - ReservationID: &resID, - }, - }) - require.NotNil(t, commitment) - assert.Equal(t, "SCU_Standard", commitment.ResourceType) -} - -func TestConvertSynapseReservation_scuSubstringNotMatched(t *testing.T) { - t.Parallel() - c := newTestClient() - // Non-Synapse SKU that merely contains "scu" as a substring must not - // be misclassified as a Synapse reservation. Guards against the prior - // substring-match false-positive. - sku := "rescue_premium" - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{SKUName: &sku}, - })) -} - -func TestConvertSynapseReservation_nilSKUName(t *testing.T) { - t.Parallel() - c := newTestClient() - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{}, - })) -} - // ---- public reservation selection ----------------------------------------- func TestGetOfferingDetails_ReservationSelection(t *testing.T) { diff --git a/providers/azure/services/synapse/held_reservation_dedupe_test.go b/providers/azure/services/synapse/held_reservation_dedupe_test.go new file mode 100644 index 00000000..27c84914 --- /dev/null +++ b/providers/azure/services/synapse/held_reservation_dedupe_test.go @@ -0,0 +1,160 @@ +package synapse + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" +) + +// Issue #705: a Synapse reservation CUDly (or anyone) already holds must +// reduce the matching recommendation. Fixture dates are UTC calendar dates, +// because the Reservations API returns date-only values. +var ( + fixtureToday = time.Now().UTC().Truncate(24 * time.Hour) + fixturePast = fixtureToday.AddDate(0, 0, -30) +) + +const synapseSKU = "DW1000c" + +func testPricer() *recommendations.Pricer { + return recommendations.NewPricer("synapse", func(context.Context, string, string, int) (recommendations.ReservationPrice, error) { + return recommendations.ReservationPrice{Total: 1000, Currency: "USD"}, nil + }) +} + +// upfrontRec runs the real converter and variant expansion for a count-2 +// P1Y recommendation and returns its upfront variant. +func upfrontRec(t *testing.T, c *Client) common.Recommendation { + t.Helper() + raw := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), mocks.WithTerm("P1Y"), mocks.WithQuantity(2), + mocks.WithNormalizedSize(synapseSKU), mocks.WithCosts(1000, 700, 300)) + recs, err := c.appendRegionRecommendations(context.Background(), testPricer(), nil, + []armconsumption.ReservationRecommendationClassification{raw}) + require.NoError(t, err) + require.Len(t, recs, 2) + upfront := recs[0] + require.Equal(t, "upfront", upfront.PaymentOption) + require.Equal(t, 2, upfront.Count) + // The dedupe key reads engine and deployment from the details; data-warehouse + // recommendations carry neither, so held commitments must carry neither. + require.Empty(t, common.EngineFromDetails(upfront.Details)) + require.Empty(t, common.DeploymentFromDetails(upfront.Details)) + return upfront +} + +// purchasedRow buys rec through a capturing transport and builds the held +// reservation from the request body CUDly actually sent. region is the +// recommending client's, so the location assertion can fail. +func purchasedRow(t *testing.T, region string, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { + t.Helper() + capture := &reservationstest.Capture{} + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", region, capture) + _, err := buyer.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + var body struct { + Location string `json:"location"` + SKU struct { + Name string `json:"name"` + } `json:"sku"` + Properties struct { + ReservedResourceType string `json:"reservedResourceType"` + Quantity int32 `json:"quantity"` + } `json:"properties"` + } + require.NoError(t, json.Unmarshal(capture.BodyBytes, &body)) + require.Equal(t, rec.Region, body.Location) + require.Equal(t, rec.Count, int(body.Properties.Quantity)) + return reservationstest.LiveRow(armreservations.ReservedResourceType(body.Properties.ReservedResourceType), + "sub", "held", body.SKU.Name, body.Location, qty) +} + +func adjust(t *testing.T, c *Client, recs ...common.Recommendation) ([]common.Recommendation, error) { + t.Helper() + passed, _, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, c) + return passed, err +} + +func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count) + assert.InDelta(t, upfront.CommitmentCost/2, passed[0].CommitmentCost, 0.001, "money fields scale with the count") +} + +func TestHeldReservation_FullHoldFiltersRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 2))) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + assert.Empty(t, passed) +} + +func TestHeldReservation_Table(t *testing.T) { + t.Parallel() + state := func(s armreservations.ProvisioningState) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ProvisioningState = &s } + } + expiry := func(d time.Time) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ExpiryDate = &d } + } + cases := []struct { + name string + mutate func(*armreservations.ReservationResponse) + want int + wantErr string // non-empty: the lookup must fail with this text + }{ + {"shared, this subscription", func(*armreservations.ReservationResponse) {}, 1, ""}, + {"creating", state(armreservations.ProvisioningStateCreating), 1, ""}, + {"shared, billed to another subscription", func(r *armreservations.ReservationResponse) { + r.Properties.BillingScopeID = mocks.StringPtr("/subscriptions/other") + }, 2, ""}, + {"canceled", state(armreservations.ProvisioningStateCancelled), 2, ""}, + {"expired state", state(armreservations.ProvisioningStateExpired), 2, ""}, + {"succeeded past expiry", expiry(fixturePast), 2, ""}, + {"other region", func(r *armreservations.ReservationResponse) { r.Location = mocks.StringPtr("westus") }, 2, ""}, + {"other sku", func(r *armreservations.ReservationResponse) { r.SKU.Name = mocks.StringPtr("DW2000c") }, 2, ""}, + {"missing quantity", func(r *armreservations.ReservationResponse) { r.Properties.Quantity = nil }, 0, "quantity"}, + {"expires today", expiry(fixtureToday), 0, "expiryDate"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + row := purchasedRow(t, c.region, upfront, 1) + tc.mutate(row) + c.SetInventoryFactories(factoriesRows(nil, row)) + passed, err := adjust(t, c, upfront) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, tc.want, passed[0].Count) + }) + } +} diff --git a/providers/azure/services/synapse/pagination_cap_test.go b/providers/azure/services/synapse/pagination_cap_test.go index 9ed15fa3..128d1171 100644 --- a/providers/azure/services/synapse/pagination_cap_test.go +++ b/providers/azure/services/synapse/pagination_cap_test.go @@ -6,8 +6,11 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -24,16 +27,6 @@ func (p *countingRecommendationsPager) NextPage(_ context.Context) (armconsumpti return armconsumption.ReservationRecommendationsClientListResponse{}, nil } -type countingReservationsPager struct { - calls, limit int -} - -func (p *countingReservationsPager) More() bool { return p.calls < p.limit } -func (p *countingReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - p.calls++ - return armconsumption.ReservationsDetailsClientListResponse{}, nil -} - func TestGetRecommendations_PageCapReturnsError(t *testing.T) { t.Parallel() pager := &countingRecommendationsPager{limit: 1000} @@ -59,31 +52,6 @@ func TestGetRecommendations_CancelledContextReturnsContextError(t *testing.T) { assert.Zero(t, pager.calls) } -func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { - t.Parallel() - pager := &countingReservationsPager{limit: 1000} - client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) - - _, err := client.GetExistingCommitments(context.Background()) - require.Error(t, err) - assert.Contains(t, err.Error(), "pagination cap") - assert.Equal(t, maxReservationsPages, pager.calls) -} - -func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - cancel() - pager := &countingReservationsPager{limit: 1000} - client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) - - _, err := client.GetExistingCommitments(ctx) - require.ErrorIs(t, err, context.Canceled) - assert.Zero(t, pager.calls) -} - // finalPartialRecommendationsPager serves exactly pages pages: full pages of // two recommendations, then a last page of one with no next link. type finalPartialRecommendationsPager struct { @@ -111,49 +79,99 @@ func (p *finalPartialRecommendationsPager) NextPage(_ context.Context) (armconsu }, nil } +func TestGetRecommendations_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing.T) { + t.Parallel() + pager := &finalPartialRecommendationsPager{pages: maxRecsPages} + client := newPricedTestClient() + client.SetRecommendationsPager(pager) + + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + // Each recommendation expands to an upfront and a monthly variant. + assert.Len(t, recs, 2*(2*(maxRecsPages-1)+1)) + assert.Equal(t, maxRecsPages, pager.calls) +} + +func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { + t.Parallel() + pager := &reservationstest.CountingPager{Limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap") + assert.Equal(t, maxReservationsPages, pager.Calls) +} + +func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + pager := &reservationstest.CountingPager{Limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, pager.Calls) +} + +func TestGetExistingCommitments_CapIsExact(t *testing.T) { + t.Parallel() + atCap := &reservationstest.CountingPager{Limit: maxReservationsPages} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return atCap })) + _, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err, "exactly cap pages must succeed") + assert.Equal(t, maxReservationsPages, atCap.Calls) + + overCap := &reservationstest.CountingPager{Limit: maxReservationsPages + 1} + client = NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) + _, err = client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Equal(t, maxReservationsPages, overCap.Calls) +} + +func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pager := &reservationstest.CancelOnSecondCallPager{Cancel: cancel} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 2, pager.Calls) +} + // finalPartialReservationsPager serves exactly pages pages: full pages of two -// reservations, then a last page of one with no next link. +// reservations, then a last page of one. type finalPartialReservationsPager struct { calls, pages int } func (p *finalPartialReservationsPager) More() bool { return p.calls < p.pages } -func (p *finalPartialReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { +func (p *finalPartialReservationsPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { n := 2 if p.calls == p.pages-1 { n = 1 } - var details []*armconsumption.ReservationDetail + var rows []*armreservations.ReservationResponse for i := 0; i < n; i++ { - sku, id := "DW1000c", fmt.Sprintf("res-%d-%d", p.calls, i) - details = append(details, &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{SKUName: &sku, ReservationID: &id}, - }) + rows = append(rows, synapseRow(fmt.Sprintf("res-%d-%d", p.calls, i), "DW1000c", "eastus", 1)) } p.calls++ - return armconsumption.ReservationsDetailsClientListResponse{ - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{Value: details}, - }, nil -} - -func TestGetRecommendations_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing.T) { - t.Parallel() - pager := &finalPartialRecommendationsPager{pages: maxRecsPages} - client := newPricedTestClient() - client.SetRecommendationsPager(pager) - - recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) - require.NoError(t, err) - // Each recommendation expands to an upfront and a monthly variant. - assert.Len(t, recs, 2*(2*(maxRecsPages-1)+1)) - assert.Equal(t, maxRecsPages, pager.calls) + return armreservations.ReservationClientListResponse{ReservationList: armreservations.ReservationList{Value: rows}}, nil } func TestGetExistingCommitments_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing.T) { t.Parallel() pager := &finalPartialReservationsPager{pages: maxReservationsPages} - client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) + client := newTestClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) got, err := client.GetExistingCommitments(context.Background()) require.NoError(t, err) diff --git a/providers/azure/services/synapse/slot_wiring_test.go b/providers/azure/services/synapse/slot_wiring_test.go index 77d93e08..235f10a3 100644 --- a/providers/azure/services/synapse/slot_wiring_test.go +++ b/providers/azure/services/synapse/slot_wiring_test.go @@ -8,8 +8,11 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot/slottest" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // slotPager is a one-page fake pager whose NextPage reports whether the shared @@ -27,6 +30,14 @@ func (s *slotPager[R]) NextPage(context.Context) (R, error) { return z, nil } +// slotLister reports one order so the order pager is exercised too. +type slotLister struct{ p *slottest.Probe } + +func (l slotLister) GetAppliedReservationList(ctx context.Context, sub string, o *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + l.p.Check() + return reservationstest.AppliedLister{}.GetAppliedReservationList(ctx, sub, o) +} + type slotHTTP struct{ p *slottest.Probe } func (h slotHTTP) Do(*http.Request) (*http.Response, error) { @@ -39,8 +50,13 @@ func TestSlotHeldDuringCalls(t *testing.T) { recs := func(p *slottest.Probe) *slotPager[armconsumption.ReservationRecommendationsClientListResponse] { return &slotPager[armconsumption.ReservationRecommendationsClientListResponse]{p: p} } - resv := func(p *slottest.Probe) *slotPager[armconsumption.ReservationsDetailsClientListResponse] { - return &slotPager[armconsumption.ReservationsDetailsClientListResponse]{p: p} + resv := func(p *slottest.Probe) *reservations.InventoryFactories { + return &reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return slotLister{p}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &slotPager[armreservations.ReservationClientListResponse]{p: p} + }, + } } cases := []struct { name string @@ -50,7 +66,7 @@ func TestSlotHeldDuringCalls(t *testing.T) { _, _ = (&Client{recommendationsPager: recs(p)}).GetRecommendations(ctx, nil) }}, {"reservations", func(ctx context.Context, p *slottest.Probe) { - _, _ = (&Client{reservationsPager: resv(p)}).GetExistingCommitments(ctx) + _, _ = (&Client{inventoryFactories: resv(p)}).GetExistingCommitments(ctx) }}, {"retail pricing", func(ctx context.Context, p *slottest.Probe) { _, _ = (&Client{httpClient: slotHTTP{p}}).getSynapsePricing(ctx, "DW100c", "eastus", 1)