Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions providers/azure/internal/recommendations/consumption.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,8 @@ func TermYears(term string) (int, error) {
// ReservationPrice is the retail price of one reservation unit for a whole
// term (the Retail Prices API's priceType 'Reservation' row), in Currency.
type ReservationPrice struct {
Total float64
Currency string
Total float64
}

// PriceFunc resolves the term-total retail price of one reservation unit.
Expand All @@ -198,8 +198,8 @@ type priceKey struct {
}

type priceResult struct {
price float64
err error
price float64
}

// Pricer resolves reservation unit prices through lookup, once per distinct
Expand All @@ -208,9 +208,9 @@ type priceResult struct {
// catalog (see IsCollectionFailure) is never cached. It is meant
// to live for one GetRecommendations call and is not safe for concurrent use.
type Pricer struct {
service string
lookup PriceFunc
cache map[priceKey]priceResult
service string
}

// NewPricer returns a Pricer for service backed by lookup.
Expand Down Expand Up @@ -300,8 +300,8 @@ func ExpandConsumptionVariants(ctx context.Context, base common.Recommendation,
// SkipLog collects the recommendations a collection dropped. It is attached
// to the collection context by WithSkipLog and is safe for concurrent use.
type SkipLog struct {
mu sync.Mutex
entries []string
mu sync.Mutex
}

type skipLogKey struct{}
Expand Down
38 changes: 21 additions & 17 deletions providers/azure/internal/recommendations/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,18 @@ import (
// reservation recommendation services, normalised across both Legacy
// and Modern API response shapes.
type ExtractedFields struct {
Region string
ResourceType string
Count int
OnDemandCost float64
CommitmentCost float64
EstimatedSavings float64
Term string
// LookBackDays is the lookback window the cost fields were computed over;
// set only by ExtractConsumption (0 from plain Extract).
LookBackDays int
// RecurringMonthlyCost is the covered/effective cost over Azure's lookback
// window, set by plain Extract only (from TotalCostWithReservedInstances, or
// OnDemandCost - NetSavings when that is absent; nil, never a fabricated 0,
// when neither is available). ExtractConsumption always leaves it nil:
// ExpandConsumptionVariants sets it per payment variant (0 for upfront,
// Count x reservation price / term months for monthly).
RecurringMonthlyCost *float64

Region string
ResourceType string
Term string

// Scope is populated from the API response ("Shared" or a subscription ID)
// but is not yet threaded into the purchase body. All service clients
// currently send armreservations.AppliedScopeTypeShared, which is correct because
Expand All @@ -51,13 +53,15 @@ type ExtractedFields struct {
// ever requested. This field is retained for future wiring if single-
// subscription scoped recommendations are ever requested.
Scope string
// RecurringMonthlyCost is the covered/effective cost over Azure's lookback
// window, set by plain Extract only (from TotalCostWithReservedInstances, or
// OnDemandCost - NetSavings when that is absent; nil, never a fabricated 0,
// when neither is available). ExtractConsumption always leaves it nil:
// ExpandConsumptionVariants sets it per payment variant (0 for upfront,
// Count x reservation price / term months for monthly).
RecurringMonthlyCost *float64

Count int
OnDemandCost float64
CommitmentCost float64
EstimatedSavings float64

// LookBackDays is the lookback window the cost fields were computed over;
// set only by ExtractConsumption (0 from plain Extract).
LookBackDays int
}

// float64Ptr returns a pointer to the given float64 value. Used to
Expand Down
6 changes: 3 additions & 3 deletions providers/azure/mocks/consumption_fixtures.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ func AssertConsumptionUnitVariants(t *testing.T, recs []common.Recommendation, p
t.Helper()
require.Len(t, recs, 4, "two recommendations expand to four variants")
cases := []struct {
term string
upfront, monthly common.Recommendation
onDemand, total, days float64
unit, months float64
term string
}{
{recs[0], recs[1], fixture1yOnDemand, fixture1yTotal, fixtureLegacyWinDy, price1y, 12, "1yr"},
{recs[2], recs[3], fixture3yOnDemand, fixture3yTotal, fixtureModernWinDy, price3y, 36, "3yr"},
{"1yr", recs[0], recs[1], fixture1yOnDemand, fixture1yTotal, fixtureLegacyWinDy, price1y, 12},
{"3yr", recs[2], recs[3], fixture3yOnDemand, fixture3yTotal, fixtureModernWinDy, price3y, 36},
}
for i := range cases {
c := &cases[i]
Expand Down
8 changes: 4 additions & 4 deletions providers/azure/mocks/pricing_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ import (
// the lookup for one SKU fail while the others succeed. The rows are
// hand-built (synthetic prices, not a live Azure quote).
type PricingHTTP struct {
Items []map[string]any
FailMatch string
// Err, when set, is returned from every request (a transport failure).
Err error

mu sync.Mutex
calls int
FailMatch string
Items []map[string]any
calls int
mu sync.Mutex
}

// Do implements the HTTPClient interface the service clients accept.
Expand Down
11 changes: 6 additions & 5 deletions providers/azure/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,11 @@ func (r *realCredentialProvider) NewDefaultAzureCredential() (azcore.TokenCreden
// Provider implements the provider.Provider interface for Azure.
type Provider struct {
cred azcore.TokenCredential
credOnce sync.Once
credErr error
subscriptionID string
region string // Default region for operations
subscriptionsClient SubscriptionsClient
credProvider CredentialProvider
subscriptionID string
region string // Default region for operations

// accountsMu guards cachedAccounts. GetAccounts, GetServiceClient, and
// GetRecommendationsClient all resolve the subscription list on the hot
Expand All @@ -121,10 +120,12 @@ type Provider struct {
// singleflight call and gates the cache write, so an ARM fetch that
// started before an invalidation can neither publish its stale snapshot
// nor be joined by a caller that arrived after it.
accountsMu sync.RWMutex
accountsSF singleflight.Group
cachedAccounts []common.Account
accountsGen uint64
accountsSF singleflight.Group
accountsMu sync.RWMutex

credOnce sync.Once

// cache subsystem implementation (getOrFetchAccounts, fetchAccounts,
// cloneAccounts, InvalidateAccountsCache) lives in accounts_cache.go.
Expand Down
19 changes: 10 additions & 9 deletions providers/azure/recommendations.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ var (
// overridden per-instance in tests to avoid real ARM network calls.
type RecommendationsClientAdapter struct {
cred azcore.TokenCredential
subscriptionID string
getAdvisorRecsFn func(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error)
subscriptionID string
}

// NewRecommendationsClientAdapter builds a RecommendationsClientAdapter with
Expand Down Expand Up @@ -222,19 +222,19 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
return nil, err
}

merged, err := mergeServiceResults(serviceResult{"compute", computeRecs, computeErr, includeCompute},
serviceResult{"database", dbRecs, dbErr, includeDB},
serviceResult{"cache", cacheRecs, cacheErr, includeCache},
serviceResult{"cosmosdb", cosmosRecs, cosmosErr, includeCosmos},
serviceResult{"synapse", synapseRecs, synapseErr, includeSynapse},
merged, err := mergeServiceResults(serviceResult{err: computeErr, name: "compute", recs: computeRecs, attempted: includeCompute},
serviceResult{err: dbErr, name: "database", recs: dbRecs, attempted: includeDB},
serviceResult{err: cacheErr, name: "cache", recs: cacheRecs, attempted: includeCache},
serviceResult{err: cosmosErr, name: "cosmosdb", recs: cosmosRecs, attempted: includeCosmos},
serviceResult{err: synapseErr, name: "synapse", recs: synapseRecs, attempted: includeSynapse},
// The savingsplans client is a stub that unconditionally returns
// ([], nil) until the Benefits Recommendations API stabilizes (see
// services/savingsplans Client.GetRecommendations). Counting its
// built-in success as an attempted service would keep the
// all-attempted-failed guard from ever firing on a total provider
// failure (expired credential, subscription-wide throttle), so it is
// excluded from the guard until it makes real API calls.
serviceResult{"savingsplans", spRecs, spErr, false},
serviceResult{err: spErr, name: "savingsplans", recs: spRecs, attempted: false},
// The Advisor client is excluded from the all-attempted-failed guard
// for the same reason: getAdvisorRecommendations swallows pagination
// errors (the auth failure from an expired credential surfaces as a
Expand All @@ -244,7 +244,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
// credential failure -- the same hazard the savingsplans exclusion
// prevents. When getAdvisorRecommendations is changed to propagate
// hard errors, flip this flag back to true.
serviceResult{"advisor", dropCoveredAdvisorSavingsPlans(advisorRecs, spRecs), advisorErr, false})
serviceResult{err: advisorErr, name: "advisor", recs: dropCoveredAdvisorSavingsPlans(advisorRecs, spRecs), attempted: false})
return withSkips(merged, err, skips)
}

Expand Down Expand Up @@ -302,9 +302,10 @@ func (r *RecommendationsClientAdapter) advisorFn() func(context.Context, common.
// stays under the cyclomatic-complexity gate after the post-Wait ctx.Err()
// propagation was added.
type serviceResult struct {
err error
name string
recs []common.Recommendation
err error

// attempted records whether the service call was actually launched
// (i.e. not skipped by the params service filter). Skipped services
// carry nil recs and nil err, which is indistinguishable from a
Expand Down
10 changes: 6 additions & 4 deletions providers/azure/recommendations_multi_subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ var newSubscriptionRecommendationsClientFn = func(cred azcore.TokenCredential, s
// client so fan-out logs and error messages can identify which subscription
// a failure came from.
type subscriptionClient struct {
subscriptionID string
client provider.RecommendationsClient
subscriptionID string
}

// SubscriptionFailure records one subscription that could not be queried
// during an org-wide fan-out.
type SubscriptionFailure struct {
SubscriptionID string
Err error
SubscriptionID string
}

// ErrSubscriptionNotAccessible is the cause recorded for a subscription named
Expand Down Expand Up @@ -93,17 +93,19 @@ func AsPartialResult(err error) *PartialResultError {
// Failing the whole sweep on one transient subscription error would be worse
// than a partial result, which is why the successful data is still returned.
type PartialSubscriptionFailureError struct {
// Failed carries every subscription that errored, with its cause.
Failed []SubscriptionFailure

// Attempted is how many subscriptions the sweep was supposed to cover:
// the ones actually queried plus any named by AccountFilter that the
// principal cannot see (those are never queried, but they were asked
// for, so leaving them out of the denominator would under-report the
// gap this error exists to surface).
Attempted int

// Succeeded is how many returned a result. Always < Attempted and > 0:
// an all-failed sweep is a plain error, not a partial one.
Succeeded int
// Failed carries every subscription that errored, with its cause.
Failed []SubscriptionFailure
}

func (e *PartialSubscriptionFailureError) Error() string {
Expand Down
60 changes: 30 additions & 30 deletions providers/azure/recommendations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,12 +426,12 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) {

// Replicate the exact call order from GetRecommendations.
result, err := mergeServiceResults(
serviceResult{"compute", []common.Recommendation{computeRec}, nil, true},
serviceResult{"database", []common.Recommendation{dbRec}, nil, true},
serviceResult{"cache", []common.Recommendation{cacheRec}, nil, true},
serviceResult{"cosmosdb", []common.Recommendation{cosmosRec}, nil, true},
serviceResult{"savingsplans", []common.Recommendation{spRec}, nil, true},
serviceResult{"advisor", []common.Recommendation{advisorRec}, nil, true},
serviceResult{err: nil, name: "compute", recs: []common.Recommendation{computeRec}, attempted: true},
serviceResult{err: nil, name: "database", recs: []common.Recommendation{dbRec}, attempted: true},
serviceResult{err: nil, name: "cache", recs: []common.Recommendation{cacheRec}, attempted: true},
serviceResult{err: nil, name: "cosmosdb", recs: []common.Recommendation{cosmosRec}, attempted: true},
serviceResult{err: nil, name: "savingsplans", recs: []common.Recommendation{spRec}, attempted: true},
serviceResult{err: nil, name: "advisor", recs: []common.Recommendation{advisorRec}, attempted: true},
)

require.NoError(t, err, "all-success merge must not error")
Expand Down Expand Up @@ -464,12 +464,12 @@ func TestMergeServiceResults_AllAttemptedFailed(t *testing.T) {
authErr := errors.New("DefaultAzureCredential: federated token expired")

recs, err := mergeServiceResults(
serviceResult{"compute", nil, authErr, true},
serviceResult{"database", nil, authErr, true},
serviceResult{"cache", nil, authErr, true},
serviceResult{"cosmosdb", nil, authErr, true},
serviceResult{"savingsplans", nil, nil, false},
serviceResult{"advisor", nil, nil, false},
serviceResult{err: authErr, name: "compute", recs: nil, attempted: true},
serviceResult{err: authErr, name: "database", recs: nil, attempted: true},
serviceResult{err: authErr, name: "cache", recs: nil, attempted: true},
serviceResult{err: authErr, name: "cosmosdb", recs: nil, attempted: true},
serviceResult{err: nil, name: "savingsplans", recs: nil, attempted: false},
serviceResult{err: nil, name: "advisor", recs: nil, attempted: false},
)

require.Error(t, err, "all-attempted-failed merge must fail loud, not return (empty, nil)")
Expand All @@ -490,12 +490,12 @@ func TestMergeServiceResults_SkippedServicesDoNotMaskTotalFailure(t *testing.T)
throttleErr := errors.New("429 too many requests")

_, err := mergeServiceResults(
serviceResult{"compute", nil, throttleErr, true},
serviceResult{"database", nil, nil, false},
serviceResult{"cache", nil, nil, false},
serviceResult{"cosmosdb", nil, nil, false},
serviceResult{"savingsplans", nil, nil, false},
serviceResult{"advisor", nil, nil, false},
serviceResult{err: throttleErr, name: "compute", recs: nil, attempted: true},
serviceResult{err: nil, name: "database", recs: nil, attempted: false},
serviceResult{err: nil, name: "cache", recs: nil, attempted: false},
serviceResult{err: nil, name: "cosmosdb", recs: nil, attempted: false},
serviceResult{err: nil, name: "savingsplans", recs: nil, attempted: false},
serviceResult{err: nil, name: "advisor", recs: nil, attempted: false},
)

require.Error(t, err, "skipped services must not count as successes in the all-failed guard")
Expand All @@ -511,12 +511,12 @@ func TestMergeServiceResults_PartialFailureStillSucceeds(t *testing.T) {
computeRec := common.Recommendation{Service: common.ServiceCompute, Provider: common.ProviderAzure}

recs, err := mergeServiceResults(
serviceResult{"compute", []common.Recommendation{computeRec}, nil, true},
serviceResult{"database", nil, svcErr, true},
serviceResult{"cache", nil, svcErr, true},
serviceResult{"cosmosdb", nil, svcErr, true},
serviceResult{"savingsplans", nil, nil, false},
serviceResult{"advisor", nil, nil, false},
serviceResult{err: nil, name: "compute", recs: []common.Recommendation{computeRec}, attempted: true},
serviceResult{err: svcErr, name: "database", recs: nil, attempted: true},
serviceResult{err: svcErr, name: "cache", recs: nil, attempted: true},
serviceResult{err: svcErr, name: "cosmosdb", recs: nil, attempted: true},
serviceResult{err: nil, name: "savingsplans", recs: nil, attempted: false},
serviceResult{err: nil, name: "advisor", recs: nil, attempted: false},
)

require.NoError(t, err, "partial failure must still return the successful services' recs")
Expand All @@ -537,12 +537,12 @@ func TestMergeServiceResults_StubsDoNotMaskTotalFailure(t *testing.T) {
authErr := errors.New("DefaultAzureCredential: federated token expired")

recs, err := mergeServiceResults(
serviceResult{"compute", nil, authErr, true},
serviceResult{"database", nil, authErr, true},
serviceResult{"cache", nil, authErr, true},
serviceResult{"cosmosdb", nil, authErr, true},
serviceResult{"savingsplans", []common.Recommendation{}, nil, false},
serviceResult{"advisor", []common.Recommendation{}, nil, false},
serviceResult{err: authErr, name: "compute", recs: nil, attempted: true},
serviceResult{err: authErr, name: "database", recs: nil, attempted: true},
serviceResult{err: authErr, name: "cache", recs: nil, attempted: true},
serviceResult{err: authErr, name: "cosmosdb", recs: nil, attempted: true},
serviceResult{err: nil, name: "savingsplans", recs: []common.Recommendation{}, attempted: false},
serviceResult{err: nil, name: "advisor", recs: []common.Recommendation{}, attempted: false},
)

require.Error(t, err, "stubs that swallow errors must not mask a total provider failure")
Expand Down
17 changes: 13 additions & 4 deletions providers/azure/services/cache/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,23 @@ type RedisCachesPager interface {
// Client handles Azure Cache for Redis Reserved Capacity.
type Client struct {
cred azcore.TokenCredential
subscriptionID string
region string
httpClient HTTPClient
recommendationsPager RecommendationsPager
reservationsPager ReservationsDetailsPager
redisCachesPager RedisCachesPager

// Lazy SKU catalog cache. armredis exposes no per-region "list all
// possible SKUs" surface, so we derive shard counts from the existing
// caches in the subscription (NewListBySubscriptionPager). Fetched
// ONCE per client lifetime; subsequent converter calls in the same
// GetRecommendations run hit the in-memory map. A failed fetch leaves
// skuCacheMap nil and converters fall back to Shards=0 with a WARN
// log — the conversion itself does NOT fail.
skuCacheMap map[string]redisSKUEntry

subscriptionID string
region string

// Lazy SKU catalog cache. armredis exposes no per-region "list all
// possible SKUs" surface, so we derive shard counts from the existing
// caches in the subscription (NewListBySubscriptionPager). Fetched
Expand All @@ -85,7 +95,6 @@ type Client struct {
// skuCacheMap nil and converters fall back to Shards=0 with a WARN
// log — the conversion itself does NOT fail.
skuCacheOnce sync.Once
skuCacheMap map[string]redisSKUEntry
}

// NewClient creates a new Azure Cache client.
Expand Down Expand Up @@ -541,9 +550,9 @@ func (c *Client) getCommonSKUs() []string {

// RedisPricing contains pricing information for Redis Cache.
type RedisPricing struct {
Currency string
HourlyRate float64
ReservationPrice float64
Currency string
}

func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYears int) (*RedisPricing, error) {
Expand Down
Loading
Loading