diff --git a/providers/azure/internal/recommendations/consumption.go b/providers/azure/internal/recommendations/consumption.go index 36da7a5..bddac5b 100644 --- a/providers/azure/internal/recommendations/consumption.go +++ b/providers/azure/internal/recommendations/consumption.go @@ -300,7 +300,9 @@ func WithSkipLog(ctx context.Context) (context.Context, *SkipLog) { return context.WithValue(ctx, skipLogKey{}, l), l } -func recordSkip(ctx context.Context, service string, err error) { +// RecordSkip logs a dropped recommendation and records it in the context's +// SkipLog, if any. +func RecordSkip(ctx context.Context, service string, err error) { logging.Errorf("azure %s: skipping recommendation: %v", service, err) if l, ok := ctx.Value(skipLogKey{}).(*SkipLog); ok { l.mu.Lock() @@ -340,7 +342,7 @@ func (e *PartialResultError) Error() string { func ExtractConsumptionOrSkip(ctx context.Context, service string, rec armconsumption.ReservationRecommendationClassification) *ExtractedFields { f, err := ExtractConsumption(rec) if err != nil { - recordSkip(ctx, service, err) + RecordSkip(ctx, service, err) return nil } return f @@ -358,7 +360,7 @@ func AppendConsumptionVariants(ctx context.Context, service string, recs []commo if IsCollectionFailure(err) { return recs, fmt.Errorf("azure %s: %w", service, err) } - recordSkip(ctx, service, err) + RecordSkip(ctx, service, err) return recs, nil } return append(recs, variants...), nil diff --git a/providers/azure/pagination_cap_test.go b/providers/azure/pagination_cap_test.go index 51c78f9..1edcb7d 100644 --- a/providers/azure/pagination_cap_test.go +++ b/providers/azure/pagination_cap_test.go @@ -152,8 +152,8 @@ func TestGetRegions_PageCap(t *testing.T) { } func advisorPage(n int) armadvisor.RecommendationsClientListResponse { - field := "Microsoft.Compute/virtualMachines" - id := "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachines/vm" + string(rune('a'+n)) + field := "Microsoft.Subscriptions/subscriptions" + id := "/subscriptions/123/providers/Microsoft.Advisor/recommendations/rec" + string(rune('a'+n)) savings, currency := "1000.00", "USD" return armadvisor.RecommendationsClientListResponse{ResourceRecommendationBaseListResult: armadvisor.ResourceRecommendationBaseListResult{ Value: []*armadvisor.ResourceRecommendationBase{{ @@ -161,8 +161,12 @@ func advisorPage(n int) armadvisor.RecommendationsClientListResponse { Properties: &armadvisor.RecommendationProperties{ ImpactedField: &field, ExtendedProperties: map[string]*string{ - "annualSavingsAmount": &savings, - "savingsCurrency": ¤cy, + "annualSavingsAmount": &savings, + "savingsCurrency": ¤cy, + "reservedResourceType": strPtr("VirtualMachines"), + "term": strPtr("P1Y"), + "sku": strPtr("Standard_D2s_v3"), + "qty": strPtr("1"), }, }, }}, @@ -173,10 +177,10 @@ func TestCollectAdvisorRecommendations_PageCap(t *testing.T) { adapter := &RecommendationsClientAdapter{subscriptionID: "test-subscription"} params := common.RecommendationParams{} - t.Run("endless pager stops at the cap keeping partial results", func(t *testing.T) { + t.Run("endless pager stops at the cap with a truncation error and the partial results", func(t *testing.T) { pager := &fakePager[armadvisor.RecommendationsClientListResponse]{limit: 10000, page: advisorPage} recs, err := adapter.collectAdvisorRecommendations(context.Background(), params, pager) - require.NoError(t, err) + require.ErrorIs(t, err, errAdvisorTruncated) assert.Equal(t, maxAdvisorPages, pager.calls) assert.NotEmpty(t, recs) }) diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 083339e..62f0ded 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -3,6 +3,7 @@ package azure import ( "context" + "errors" "fmt" "math" "strconv" @@ -209,7 +210,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p advisorFn = r.getAdvisorRecommendations } goService(&advisorErr, func() { - advisorRecs, advisorErr = advisorFn(gctx, *params) + advisorRecs, advisorErr = collectAdvisorOrSkip(gctx, advisorFn, *params) }) // Wait for all goroutines. g.Wait() always returns nil because every @@ -245,7 +246,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", advisorRecs, advisorErr, false}) + serviceResult{"advisor", dropCoveredAdvisorSavingsPlans(advisorRecs, spRecs), advisorErr, false}) return withSkips(merged, err, skips) } @@ -263,6 +264,33 @@ func withSkips(recs []common.Recommendation, err error, skips *azrecs.SkipLog) ( return recs, nil } +// dropCoveredAdvisorSavingsPlans removes Advisor Savings Plan rows that the +// savings-plan client (Benefit Recommendations API) already covers, matched on +// (account, term). Advisor's row carries only a saving, no commitment amount, +// so the Benefit Recommendations row is the better record of the same advice. +// Matching on a key, not dropping all Advisor SP rows, keeps Advisor's advice +// when that client returns nothing for an account and term. +func dropCoveredAdvisorSavingsPlans(advisor, sp []common.Recommendation) []common.Recommendation { + type key struct{ account, term string } + covered := make(map[key]bool) + for _, r := range sp { + if r.CommitmentType == common.CommitmentSavingsPlan { + covered[key{r.Account, r.Term}] = true + } + } + if len(covered) == 0 { + return advisor + } + out := make([]common.Recommendation, 0, len(advisor)) + for _, r := range advisor { + if r.CommitmentType == common.CommitmentSavingsPlan && covered[key{r.Account, r.Term}] { + continue + } + out = append(out, r) + } + return out +} + // serviceResult bundles a per-service collection outcome for the deterministic // merge in mergeServiceResults. Extracted into a helper so GetRecommendations // stays under the cyclomatic-complexity gate after the post-Wait ctx.Err() @@ -370,9 +398,28 @@ type AdvisorPager interface { NextPage(ctx context.Context) (armadvisor.RecommendationsClientListResponse, error) } +// errAdvisorTruncated marks an Advisor walk that stopped before the last +// page (page cap or page error); the recommendations gathered so far are +// returned with it. +var errAdvisorTruncated = errors.New("azure advisor: recommendation list is incomplete") + +// collectAdvisorOrSkip runs the Advisor collection and reports a truncated +// walk through the context's skip log, keeping the recommendations gathered so +// far, so GetRecommendations surfaces it as a *azrecs.PartialResultError. +// Any other error is returned unchanged. +func collectAdvisorOrSkip(ctx context.Context, fn func(context.Context, common.RecommendationParams) ([]common.Recommendation, error), params common.RecommendationParams) ([]common.Recommendation, error) { + recs, err := fn(ctx, params) + if errors.Is(err, errAdvisorTruncated) { + azrecs.RecordSkip(ctx, "advisor", err) + return recs, nil + } + return recs, err +} + // collectAdvisorRecommendations walks the Advisor pager. A canceled context -// is an error; a page error or hitting the page cap keeps what was collected -// so far with a warning (the intentional partial-OK path). +// is an error. Hitting the page cap or a page error returns the +// recommendations collected so far together with errAdvisorTruncated, never a +// bare partial result. func (r *RecommendationsClientAdapter) collectAdvisorRecommendations(ctx context.Context, params common.RecommendationParams, pager AdvisorPager) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) for pageIdx := 0; pager.More(); pageIdx++ { @@ -380,19 +427,13 @@ func (r *RecommendationsClientAdapter) collectAdvisorRecommendations(ctx context return nil, fmt.Errorf("azure advisor: context canceled during pagination: %w", err) } if pageIdx >= maxAdvisorPages { - logging.Warnf("Azure Advisor pagination cap (%d pages) reached after %d recommendations (partial results returned)", maxAdvisorPages, len(recommendations)) - break + return recommendations, fmt.Errorf("%w: pagination cap (%d pages) reached after %d recommendations", errAdvisorTruncated, maxAdvisorPages, len(recommendations)) } page, err := pager.NextPage(ctx) if err != nil { - // Advisor partial-OK is intentional: per-service reservation results - // (compute, database, etc.) remain useful on their own when Advisor - // pagination fails mid-stream. Warnf with the accumulated count so - // operators can assess the completeness of the Advisor slice. - logging.Warnf("Azure Advisor pagination error after %d recommendations (partial results returned): %v", len(recommendations), err) - break + return recommendations, fmt.Errorf("%w: page error after %d recommendations: %v", errAdvisorTruncated, len(recommendations), err) } - recommendations = r.appendAdvisorPageRecs(params, page, recommendations) + recommendations = r.appendAdvisorPageRecs(ctx, params, page, recommendations) } return recommendations, nil @@ -402,6 +443,7 @@ func (r *RecommendationsClientAdapter) collectAdvisorRecommendations(ctx context // and appends them to the provided slice. Pulled out of getAdvisorRecommendations // to keep that function under the cyclomatic limit. func (r *RecommendationsClientAdapter) appendAdvisorPageRecs( + ctx context.Context, params common.RecommendationParams, page armadvisor.RecommendationsClientListResponse, recommendations []common.Recommendation, @@ -410,8 +452,7 @@ func (r *RecommendationsClientAdapter) appendAdvisorPageRecs( if advisorRec.Properties == nil { continue } - // Convert Azure Advisor recommendation to our common format - rec := r.convertAdvisorRecommendation(advisorRec) + rec := r.convertAdvisorRecommendation(ctx, advisorRec) if rec != nil && shouldIncludeService(params, rec.Service) { // Preserve Advisor-provided EstimatedSavings when both OnDemandCost // and CommitmentCost are unset (zero), since ExpandPaymentVariants @@ -448,14 +489,58 @@ func resolveAdvisorRegion(advisorRec *armadvisor.ResourceRecommendationBase) str return "" } -// convertAdvisorRecommendation converts an Azure Advisor recommendation to common format. -func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(advisorRec *armadvisor.ResourceRecommendationBase) *common.Recommendation { +// Advisor ExtendedProperties keys. The SDK types ExtendedProperties as a bare +// map[string]*string, so the keys are not in the SDK. annualSavingsAmount, +// savingsCurrency, term and lookbackPeriod are the names Microsoft's Advisor +// Resource Graph samples read from properties.extendedProperties +// (learn.microsoft.com/azure/advisor/advisor-azure-resource-graph); +// reservedResourceType, recommendationSubCategory, sku and qty are the keys +// this collector has always relied on. +const ( + advisorKeySavings = "annualSavingsAmount" + advisorKeyCurrency = "savingsCurrency" + advisorKeyTerm = "term" + advisorKeySKU = "sku" + advisorKeyQty = "qty" + advisorKeyRRT = "reservedResourceType" +) + +// advisorPurchaseKind classifies an Advisor recommendation. Only +// subscription-scoped reservation (reservedResourceType) and Savings Plan +// recommendations are commitment purchases; resource-level recommendations +// such as resize or shutdown are not, and are ignored (ok=false, no error). +// An unmapped reservedResourceType is ignored (debug log), not a skip. +func advisorPurchaseKind(ctx context.Context, ext map[string]*string) (common.CommitmentType, string, bool) { + service := serviceFromExtendedProperties(ext) + if service == "" { + if v := ext[advisorKeyRRT]; v != nil && *v != "" { + // Advisor routinely returns reservations for services this + // collector does not price; they are not ours to convert. + logging.Debugf("azure advisor: ignoring %s %q", advisorKeyRRT, *v) + } + return "", "", false + } + if v := ext["recommendationSubCategory"]; v != nil && strings.EqualFold(*v, "SavingsPlan") { + return common.CommitmentSavingsPlan, service, true + } + return common.CommitmentReservedInstance, service, true +} + +// convertAdvisorRecommendation converts an Azure Advisor commitment +// recommendation to common format. Payload problems are recorded in the +// context's skip log and yield nil. +// +// Payment option is left empty: the Advisor payload carries no payment-plan +// or price field, and Azure charges the same total for upfront and monthly +// billing, so ExpandPaymentVariants fans it out to both. CommitmentCost stays 0 +// and RecurringMonthlyCost nil (unknown) because the payload has no price. +func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(ctx context.Context, advisorRec *armadvisor.ResourceRecommendationBase) *common.Recommendation { if advisorRec.Properties == nil { return nil } - - service := extractServiceType(advisorRec) - if service == "" { + ext := advisorRec.Properties.ExtendedProperties + commitmentType, service, ok := advisorPurchaseKind(ctx, ext) + if !ok { return nil } @@ -463,36 +548,63 @@ func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(advisorRec * Provider: common.ProviderAzure, Service: common.ServiceType(service), Account: r.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Term: "1yr", - PaymentOption: "upfront", + CommitmentType: commitmentType, } - rec.Region = resolveAdvisorRegion(advisorRec) - if err := populateFromExtendedProperties(rec, advisorRec.Properties.ExtendedProperties); err != nil { - logging.Warnf("Azure Advisor %s recommendation in %q dropped: %v", service, rec.Region, err) + if err := populateFromExtendedProperties(rec, ext); err != nil { + azrecs.RecordSkip(ctx, "advisor", fmt.Errorf("%s recommendation in %q: %w", service, rec.Region, err)) return nil } return rec } -// populateFromExtendedProperties fills savings, SKU, term, and count from -// the Advisor recommendation's ExtendedProperties map. It returns an error -// when the savings figure is absent or unparsable, so the caller never ships -// a fabricated zero. +// populateFromExtendedProperties fills savings, SKU, term and count from the +// Advisor ExtendedProperties map and errors on anything absent or unknown: +// savings, a USD currency label and a known term are always required; a +// reservation also needs its SKU and a positive quantity. func populateFromExtendedProperties(rec *common.Recommendation, ext map[string]*string) error { - annualSavings, err := extFloat(ext, "annualSavingsAmount") + annualSavings, err := extFloat(ext, advisorKeySavings) if err != nil { return err } + if cur := extString(ext, advisorKeyCurrency, ""); !strings.EqualFold(cur, "USD") { + return fmt.Errorf("%s %q is not USD", advisorKeyCurrency, cur) + } rec.EstimatedSavings = annualSavings / 12 - rec.ResourceType = extString(ext, "sku", rec.ResourceType) - rec.Term = extString(ext, "term", rec.Term) - rec.Count = extInt(ext, "qty", rec.Count) + + if rec.Term, err = advisorTerm(ext); err != nil { + return err + } + if rec.CommitmentType == common.CommitmentSavingsPlan { + return nil + } + if rec.ResourceType = extString(ext, advisorKeySKU, ""); rec.ResourceType == "" { + return fmt.Errorf("%s is absent", advisorKeySKU) + } + qty, err := extInt(ext, advisorKeyQty) + if err != nil { + return err + } + rec.Count = qty return nil } +// advisorTerm maps the Advisor term ("P1Y"/"P3Y", or the already-normalised +// "1yr"/"3yr") to the common form and errors on a missing or unknown value. +func advisorTerm(ext map[string]*string) (string, error) { + switch t := extString(ext, advisorKeyTerm, ""); t { + case "P1Y", "1yr": + return "1yr", nil + case "P3Y", "3yr": + return "3yr", nil + case "": + return "", fmt.Errorf("%s is absent", advisorKeyTerm) + default: + return "", fmt.Errorf("unknown %s %q", advisorKeyTerm, t) + } +} + func extString(m map[string]*string, key, fallback string) string { if v, ok := m[key]; ok && v != nil && *v != "" { return *v @@ -518,45 +630,19 @@ func extFloat(m map[string]*string, key string) (float64, error) { return f, nil } -func extInt(m map[string]*string, key string, fallback int) int { - if v, ok := m[key]; ok && v != nil { - if n, err := strconv.Atoi(*v); err == nil { - return n - } - } - return fallback -} - -// extractServiceType determines the service type from an Advisor recommendation. -// First checks ImpactedField (resource-scoped), then falls back to -// ExtendedProperties (subscription-scoped reservations/savings plans). -func extractServiceType(rec *armadvisor.ResourceRecommendationBase) string { - if rec.Properties == nil { - return "" - } - if svc := serviceFromImpactedField(rec.Properties.ImpactedField); svc != "" { - return svc +func extInt(m map[string]*string, key string) (int, error) { + v, ok := m[key] + if !ok || v == nil { + return 0, fmt.Errorf("%s is absent", key) } - return serviceFromExtendedProperties(rec.Properties.ExtendedProperties) -} - -// serviceFromImpactedField maps Azure resource namespace to service type. -func serviceFromImpactedField(field *string) string { - if field == nil { - return "" + n, err := strconv.Atoi(*v) + if err != nil { + return 0, fmt.Errorf("%s %q is not an integer: %w", key, *v, err) } - f := *field - switch { - case strings.Contains(f, "Microsoft.Compute"): - return string(common.ServiceCompute) - case strings.Contains(f, "Microsoft.Sql"): - return string(common.ServiceRelationalDB) - case strings.Contains(f, "Microsoft.Cache"): - return string(common.ServiceCache) - case strings.Contains(f, "Microsoft.DBforMySQL"), strings.Contains(f, "Microsoft.DBforPostgreSQL"): - return string(common.ServiceRelationalDB) + if n <= 0 { + return 0, fmt.Errorf("%s %q is not positive", key, *v) } - return "" + return n, nil } // serviceFromExtendedProperties resolves service type for subscription-scoped @@ -577,7 +663,7 @@ func serviceFromExtendedProperties(ext map[string]*string) string { } if subcat, ok := ext["recommendationSubCategory"]; ok && subcat != nil { if strings.EqualFold(*subcat, "SavingsPlan") { - return string(common.ServiceCompute) + return string(common.ServiceSavingsPlansAll) } } return "" diff --git a/providers/azure/recommendations_advisor_test.go b/providers/azure/recommendations_advisor_test.go new file mode 100644 index 0000000..e0ffcd5 --- /dev/null +++ b/providers/azure/recommendations_advisor_test.go @@ -0,0 +1,181 @@ +package azure + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" +) + +// Fixture-based: payloads are shaped from armadvisor.ResourceRecommendationBase +// with the extendedProperties keys documented in advisorKey* (no live Azure +// Advisor data was available). + +func advisorRec(impacted string, ext map[string]*string) *armadvisor.ResourceRecommendationBase { + return &armadvisor.ResourceRecommendationBase{ + Properties: &armadvisor.RecommendationProperties{ImpactedField: &impacted, ExtendedProperties: ext}, + } +} + +func reservationExt(over map[string]*string) map[string]*string { + ext := map[string]*string{ + "reservedResourceType": strPtr("VirtualMachines"), + "annualSavingsAmount": strPtr("1200"), + "savingsCurrency": strPtr("USD"), + "term": strPtr("P3Y"), + "sku": strPtr("Standard_D2s_v3"), + "qty": strPtr("2"), + "region": strPtr("eastus"), + } + for k, v := range over { + if v == nil { + delete(ext, k) + } else { + ext[k] = v + } + } + return ext +} + +const subscriptionScope = "Microsoft.Subscriptions/subscriptions" + +func TestConvertAdvisor_DerivesFieldsFromPayload(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + + ri := a.convertAdvisorRecommendation(context.Background(), advisorRec(subscriptionScope, reservationExt(nil))) + require.NotNil(t, ri) + assert.Equal(t, common.CommitmentReservedInstance, ri.CommitmentType) + assert.Equal(t, "3yr", ri.Term, "term comes from the payload, not a 1yr default") + assert.Equal(t, "Standard_D2s_v3", ri.ResourceType) + assert.Equal(t, 2, ri.Count) + assert.Equal(t, 100.0, ri.EstimatedSavings) + assert.Empty(t, ri.PaymentOption, "the payload carries no payment plan; variants are fanned out later") + + sp := a.convertAdvisorRecommendation(context.Background(), advisorRec(subscriptionScope, map[string]*string{ + "recommendationSubCategory": strPtr("SavingsPlan"), + "annualSavingsAmount": strPtr("2400"), + "savingsCurrency": strPtr("USD"), + "term": strPtr("P1Y"), + })) + require.NotNil(t, sp) + assert.Equal(t, common.CommitmentSavingsPlan, sp.CommitmentType) + assert.Equal(t, common.ServiceSavingsPlansAll, sp.Service) + assert.Equal(t, "1yr", sp.Term) +} + +func TestConvertAdvisor_NonPurchaseRecommendationsAreIgnored(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + // Resize/shutdown: resource-scoped, no reservedResourceType. + ctx, skips := azrecs.WithSkipLog(context.Background()) + rightsize := advisorRec("Microsoft.Compute/virtualMachines", map[string]*string{ + "annualSavingsAmount": strPtr("1000"), "savingsCurrency": strPtr("USD"), + }) + assert.Nil(t, a.convertAdvisorRecommendation(ctx, rightsize)) + assert.Empty(t, skips.Entries(), "a non-purchase recommendation is not a skip") +} + +func TestConvertAdvisor_UnknownOrMissingValuesAreSkippedLoudly(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + for name, over := range map[string]map[string]*string{ + "unknown term": {"term": strPtr("P5Y")}, + "missing term": {"term": nil}, + "non-USD": {"savingsCurrency": strPtr("EUR")}, + "missing currency": {"savingsCurrency": nil}, + "missing sku": {"sku": nil}, + "missing qty": {"qty": nil}, + "zero qty": {"qty": strPtr("0")}, + "junk qty": {"qty": strPtr("two")}, + "missing savings": {"annualSavingsAmount": nil}, + } { + t.Run(name, func(t *testing.T) { + ctx, skips := azrecs.WithSkipLog(context.Background()) + assert.Nil(t, a.convertAdvisorRecommendation(ctx, advisorRec(subscriptionScope, reservationExt(over)))) + assert.Len(t, skips.Entries(), 1) + }) + } +} + +func TestAppendAdvisorPageRecs_VariantsKeepCostUnknown(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + page := armadvisor.RecommendationsClientListResponse{ResourceRecommendationBaseListResult: armadvisor.ResourceRecommendationBaseListResult{ + Value: []*armadvisor.ResourceRecommendationBase{advisorRec(subscriptionScope, reservationExt(nil))}, + }} + recs := a.appendAdvisorPageRecs(context.Background(), common.RecommendationParams{}, page, nil) + require.Len(t, recs, 2) + for i, payment := range []string{"upfront", "monthly"} { + assert.Equal(t, payment, recs[i].PaymentOption) + assert.Equal(t, 100.0, recs[i].EstimatedSavings) + assert.Nil(t, recs[i].RecurringMonthlyCost, "no price in the payload: unknown, not zero") + } +} + +func TestGetRecommendations_AdvisorTruncationIsSurfaced(t *testing.T) { + overrideServiceClientFns(t, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}) + a := &RecommendationsClientAdapter{cred: &mockAzureTokenCredential{}, subscriptionID: "sub"} + a.getAdvisorRecsFn = func(ctx context.Context, p common.RecommendationParams) ([]common.Recommendation, error) { + pager := &fakePager[armadvisor.RecommendationsClientListResponse]{limit: 10000, page: advisorPage} + return a.collectAdvisorRecommendations(ctx, p, pager) + } + recs, err := a.GetRecommendations(context.Background(), &common.RecommendationParams{}) + assert.NotEmpty(t, recs, "what was collected is kept") + var partial *azrecs.PartialResultError + require.True(t, errors.As(err, &partial), "a capped Advisor walk must not look complete: %v", err) + assert.Contains(t, partial.Skipped[0], "pagination cap") +} + +func TestGetRecommendations_AdvisorSavingsPlanDedupedAgainstBenefitRecs(t *testing.T) { + sp := func(account, term string, from string) common.Recommendation { + return common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute, + CommitmentType: common.CommitmentSavingsPlan, Account: account, Term: term, Region: from} + } + benefit := sp("sub", "1yr", "benefit") + run := func(spRecs []common.Recommendation, advisor []common.Recommendation) []common.Recommendation { + overrideServiceClientFns(t, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{recs: spRecs}) + a := &RecommendationsClientAdapter{cred: &mockAzureTokenCredential{}, subscriptionID: "sub", + getAdvisorRecsFn: func(context.Context, common.RecommendationParams) ([]common.Recommendation, error) { + return advisor, nil + }} + recs, err := a.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + return recs + } + advisorRI := common.Recommendation{CommitmentType: common.CommitmentReservedInstance, Account: "sub", Term: "1yr", Region: "advisor-ri"} + + got := run([]common.Recommendation{benefit}, []common.Recommendation{sp("sub", "1yr", "advisor"), sp("sub", "3yr", "advisor"), advisorRI}) + assert.ElementsMatch(t, []common.Recommendation{benefit, sp("sub", "3yr", "advisor"), advisorRI}, got, + "the covered (account, term) Advisor SP row is dropped; other terms and RI rows stay") + + got = run(nil, []common.Recommendation{sp("sub", "1yr", "advisor")}) + assert.Len(t, got, 1, "Advisor SP advice is kept when the benefit source returns nothing") +} + +func TestConvertAdvisor_UnmappedReservedResourceTypeIsIgnoredNotSkipped(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + ctx, skips := azrecs.WithSkipLog(context.Background()) + got := a.convertAdvisorRecommendation(ctx, advisorRec(subscriptionScope, reservationExt(map[string]*string{"reservedResourceType": strPtr("CosmosDb")}))) + assert.Nil(t, got) + assert.Empty(t, skips.Entries(), "a service we do not price must not make every subscription look partial") +} + +func TestConvertAdvisor_RejectsBadSavingsAmounts(t *testing.T) { + a := &RecommendationsClientAdapter{subscriptionID: "sub"} + for name, v := range map[string]*string{ + "nil": nil, "thousands separator": strPtr("1,234.00"), "empty": strPtr(""), + "NaN": strPtr("NaN"), "Inf": strPtr("Inf"), "-Inf": strPtr("-Inf"), "negative": strPtr("-1200"), + } { + t.Run(name, func(t *testing.T) { + ctx, skips := azrecs.WithSkipLog(context.Background()) + assert.Nil(t, a.convertAdvisorRecommendation(ctx, advisorRec(subscriptionScope, reservationExt(map[string]*string{"annualSavingsAmount": v})))) + assert.Len(t, skips.Entries(), 1) + }) + } + zero := a.convertAdvisorRecommendation(context.Background(), advisorRec(subscriptionScope, reservationExt(map[string]*string{"annualSavingsAmount": strPtr("0")}))) + require.NotNil(t, zero, "a real zero saving is kept") + assert.Equal(t, 0.0, zero.EstimatedSavings) +} diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index d049d1e..f2d7ad0 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -359,106 +359,6 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel "GetRecommendations must propagate the parent ctx error after g.Wait()") } -func TestExtractServiceType(t *testing.T) { - tests := []struct { - name string - impactedField string - expected string - }{ - { - name: "Microsoft.Compute returns Compute", - impactedField: "Microsoft.Compute/virtualMachines", - expected: string(common.ServiceCompute), - }, - { - name: "Microsoft.Sql returns RelationalDB", - impactedField: "Microsoft.Sql/servers", - expected: string(common.ServiceRelationalDB), - }, - { - name: "Microsoft.Cache returns Cache", - impactedField: "Microsoft.Cache/Redis", - expected: string(common.ServiceCache), - }, - { - name: "Microsoft.DBforMySQL returns RelationalDB", - impactedField: "Microsoft.DBforMySQL/servers", - expected: string(common.ServiceRelationalDB), - }, - { - name: "Microsoft.DBforPostgreSQL returns RelationalDB", - impactedField: "Microsoft.DBforPostgreSQL/servers", - expected: string(common.ServiceRelationalDB), - }, - { - name: "Unknown resource type returns empty", - impactedField: "Microsoft.Storage/storageAccounts", - expected: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - rec := &armadvisor.ResourceRecommendationBase{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &tt.impactedField, - }, - } - result := extractServiceType(rec) - assert.Equal(t, tt.expected, result) - }) - } -} - -func TestExtractServiceType_NilProperties(t *testing.T) { - // Test with nil properties - rec := &armadvisor.ResourceRecommendationBase{ - Properties: nil, - } - result := extractServiceType(rec) - assert.Equal(t, "", result) -} - -func TestExtractServiceType_NilImpactedField(t *testing.T) { - // Test with nil impacted field - rec := &armadvisor.ResourceRecommendationBase{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: nil, - }, - } - result := extractServiceType(rec) - assert.Equal(t, "", result) -} - -func TestConvertAdvisorRecommendation(t *testing.T) { - adapter := &RecommendationsClientAdapter{ - subscriptionID: "test-subscription", - } - - impactedField := "Microsoft.Compute/virtualMachines" - resourceID := "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachines/vm1" - - rec := &armadvisor.ResourceRecommendationBase{ - ID: &resourceID, - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &impactedField, - ExtendedProperties: map[string]*string{ - "annualSavingsAmount": strPtr("1000.00"), - "savingsCurrency": strPtr("USD"), - }, - }, - } - - result := adapter.convertAdvisorRecommendation(rec) - require.NotNil(t, result) - assert.Equal(t, common.ProviderAzure, result.Provider) - assert.Equal(t, common.ServiceCompute, result.Service) - assert.Equal(t, "test-subscription", result.Account) - assert.Equal(t, common.CommitmentReservedInstance, result.CommitmentType) - assert.Equal(t, "1yr", result.Term) - assert.Equal(t, "upfront", result.PaymentOption) -} - func TestConvertAdvisorRecommendation_NilProperties(t *testing.T) { adapter := &RecommendationsClientAdapter{ subscriptionID: "test-subscription", @@ -468,7 +368,7 @@ func TestConvertAdvisorRecommendation_NilProperties(t *testing.T) { Properties: nil, } - result := adapter.convertAdvisorRecommendation(rec) + result := adapter.convertAdvisorRecommendation(context.Background(), rec) assert.Nil(t, result) } @@ -484,50 +384,10 @@ func TestConvertAdvisorRecommendation_UnknownService(t *testing.T) { }, } - result := adapter.convertAdvisorRecommendation(rec) + result := adapter.convertAdvisorRecommendation(context.Background(), rec) assert.Nil(t, result) } -// A real zero saving must survive conversion, while an absent or unparsable -// annualSavingsAmount must drop the recommendation instead of shipping $0. -func TestConvertAdvisorRecommendation_ZeroVersusAbsentSavings(t *testing.T) { - adapter := &RecommendationsClientAdapter{subscriptionID: "test-subscription"} - impactedField := "Microsoft.Compute/virtualMachines" - mkRec := func(ext map[string]*string) *armadvisor.ResourceRecommendationBase { - return &armadvisor.ResourceRecommendationBase{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &impactedField, - ExtendedProperties: ext, - }, - } - } - - zero := adapter.convertAdvisorRecommendation(mkRec(map[string]*string{"annualSavingsAmount": strPtr("0")})) - require.NotNil(t, zero, "a real zero saving must be kept") - assert.Equal(t, 0.0, zero.EstimatedSavings) - - populated := adapter.convertAdvisorRecommendation(mkRec(map[string]*string{"annualSavingsAmount": strPtr("1200")})) - require.NotNil(t, populated) - assert.Equal(t, 100.0, populated.EstimatedSavings) - - for name, ext := range map[string]map[string]*string{ - "key absent": {"sku": strPtr("Standard_D2s_v3")}, - "nil value": {"annualSavingsAmount": nil}, - "nil map": nil, - "thousands separator": {"annualSavingsAmount": strPtr("1,234.00")}, - "empty string": {"annualSavingsAmount": strPtr("")}, - "NaN": {"annualSavingsAmount": strPtr("NaN")}, - "nan": {"annualSavingsAmount": strPtr("nan")}, - "Inf": {"annualSavingsAmount": strPtr("Inf")}, - "-Inf": {"annualSavingsAmount": strPtr("-Inf")}, - "negative": {"annualSavingsAmount": strPtr("-1200")}, - } { - t.Run(name, func(t *testing.T) { - assert.Nil(t, adapter.convertAdvisorRecommendation(mkRec(ext))) - }) - } -} - func strPtr(s string) *string { return &s } @@ -861,32 +721,3 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { assert.Contains(t, services, common.ServiceNoSQL, "cosmosdb recs must be present despite db error") assert.NotContains(t, services, common.ServiceRelationalDB, "db recs must be absent when db errors") } - -// The Advisor path must keep the pre-existing expansion: Advisor supplies only -// a monthly saving (no on-demand or commitment figure), which survives onto -// both variants with no recurring cost and no scaling applied. -func TestAppendAdvisorPageRecs_ExpansionUnchanged(t *testing.T) { - adapter := &RecommendationsClientAdapter{subscriptionID: "test-subscription"} - impactedField := "Microsoft.Compute/virtualMachines" - page := armadvisor.RecommendationsClientListResponse{ - ResourceRecommendationBaseListResult: armadvisor.ResourceRecommendationBaseListResult{ - Value: []*armadvisor.ResourceRecommendationBase{{ - Properties: &armadvisor.RecommendationProperties{ - ImpactedField: &impactedField, - ExtendedProperties: map[string]*string{"annualSavingsAmount": strPtr("1200"), "sku": strPtr("Standard_D2s_v3"), "term": strPtr("3yr"), "qty": strPtr("2")}, - }, - }}, - }, - } - - recs := adapter.appendAdvisorPageRecs(common.RecommendationParams{}, page, nil) - require.Len(t, recs, 2) - for i, payment := range []string{"upfront", "monthly"} { - assert.Equal(t, payment, recs[i].PaymentOption) - assert.Equal(t, 100.0, recs[i].EstimatedSavings) - assert.Zero(t, recs[i].OnDemandCost) - assert.Zero(t, recs[i].CommitmentCost) - assert.Zero(t, recs[i].SavingsPercentage) - assert.Nil(t, recs[i].RecurringMonthlyCost) - } -}