diff --git a/internal/scheduler/advisor_pricing_outage_integration_test.go b/internal/scheduler/advisor_pricing_outage_integration_test.go index d0e925a36..97433b339 100644 --- a/internal/scheduler/advisor_pricing_outage_integration_test.go +++ b/internal/scheduler/advisor_pricing_outage_integration_test.go @@ -16,7 +16,10 @@ import ( "github.com/stretchr/testify/require" ) -// Skip reasons as the go#663 Advisor pricing records them (see pricingOutageReasons). +// The kind the library records for each skip below: a failed price lookup. +var advisorOutageKinds = []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable, azureprovider.SkipPriceLookupUnavailable} + +// Skip reasons as the go#663 Advisor pricing records them. var advisorOutageSkips = []string{ `advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, `advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, @@ -76,13 +79,13 @@ func TestAzureAdvisorPricingOutage_HoldsEviction(t *testing.T) { t.Run("outage keeps prior rows", func(t *testing.T) { store := newStore(t) - collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds}) require.Contains(t, storedAzureIDs(t, store), "prev-advisor-row") }) t.Run("later good cycle replaces them", func(t *testing.T) { store := newStore(t) - collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds}) got := collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil) require.NotEmpty(t, got) ids := storedAzureIDs(t, store) @@ -113,7 +116,7 @@ func TestAzureAdvisorPricingOutage_FreshnessListsHeldAccount(t *testing.T) { acct := config.CloudAccount{ID: uuid.NewString(), Name: "host", Provider: "azure", ExternalID: "sub-a", AzureSubscriptionID: "sub-a", Enabled: false} require.NoError(t, store.CreateCloudAccount(ctx, &acct)) - collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds}) fresh, err := store.GetRecommendationsFreshness(ctx) require.NoError(t, err) require.NotNil(t, fresh.LastCollectionPartial) diff --git a/internal/scheduler/azure_hold_carry_integration_test.go b/internal/scheduler/azure_hold_carry_integration_test.go index bf34803ef..2cd4cd334 100644 --- a/internal/scheduler/azure_hold_carry_integration_test.go +++ b/internal/scheduler/azure_hold_carry_integration_test.go @@ -162,7 +162,7 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) { store := config.NewPostgresStore(container.DB) s := &Scheduler{config: store} seedAdvisorRow(t, store) - collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips}) + collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds}) s.persistCollection(ctx, nil, nil, map[string]string{"azure": "boom"}, nil) fresh, err := store.GetRecommendationsFreshness(ctx) require.NoError(t, err) diff --git a/internal/scheduler/partial_skip_test.go b/internal/scheduler/partial_skip_test.go index e56da1b4d..9568b8ea0 100644 --- a/internal/scheduler/partial_skip_test.go +++ b/internal/scheduler/partial_skip_test.go @@ -21,6 +21,23 @@ type skipErr struct { reasons []string } +// kindSkipErr is skipErr that also reports a kind per skip reason, as Azure's +// PartialResultError does (cloud-commitments-go#670). +type kindSkipErr struct { + skipErr + kinds []azureprovider.SkipKind +} + +func (e *kindSkipErr) SkipKinds() []azureprovider.SkipKind { return e.kinds } + +// outageSkip is an Advisor skip whose price lookup did not complete, with a +// reason text that matches no prefix the scheduler ever knew: the hold reads +// the kind, never the text. +func outageSkip(reason string) *kindSkipErr { + return &kindSkipErr{skipErr: skipErr{partial: true, count: 1, reasons: []string{reason}}, + kinds: []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable}} +} + func (e *skipErr) Error() string { return e.msg } func (e *skipErr) Partial() bool { return e.partial } func (e *skipErr) SkippedCount() int { return e.count } @@ -126,28 +143,31 @@ func TestAzureFailureWithoutSkips_RecordsNoSkippedCount(t *testing.T) { assert.True(t, gap.holdsEviction()) } -func TestHasPricingOutageSkip(t *testing.T) { +func TestCountPricingOutageSkips(t *testing.T) { + const ( + outage = azureprovider.SkipPriceLookupUnavailable + unpriced = azureprovider.SkipUnpriceable + incomplete = azureprovider.SkipSourceIncomplete + ) for name, tc := range map[string]struct { - reasons []string - want bool + kinds []azureprovider.SkipKind + want int }{ - "first failed row": {[]string{"advisor: azure advisor: advisor compute reservation price: get: retail prices fetch failed"}, true}, - "later rows": {[]string{"advisor: non-USD", "advisor: price lookup unavailable: x"}, true}, - "invalid recommendation": {[]string{"advisor: row is in EUR, not USD", "computeengine us-east1: non-USD"}, false}, - "mid-string mention": {[]string{"computeengine: price lookup unavailable: x"}, false}, - "truncated walk is not an outage": {[]string{"advisor: azure advisor: recommendation list is incomplete"}, false}, - "truncated walk plus outage": {[]string{"advisor: azure advisor: recommendation list is incomplete", "advisor: azure advisor: get: retail prices fetch failed"}, true}, - "prefix mid-string": {[]string{"sub-1: advisor: price lookup unavailable: x"}, false}, - "none": {nil, false}, + "a failed price lookup": {[]azureprovider.SkipKind{outage}, 1}, + "later rows of the same call": {[]azureprovider.SkipKind{unpriced, outage, outage}, 2}, + "invalid recommendations only": {[]azureprovider.SkipKind{unpriced, unpriced}, 0}, + "a truncated walk is not an outage": {[]azureprovider.SkipKind{incomplete}, 0}, + "a truncated walk plus an outage": {[]azureprovider.SkipKind{incomplete, outage}, 1}, + "an unrecognized kind is not an outage": {[]azureprovider.SkipKind{azureprovider.SkipKind("a_kind_added_later")}, 0}, + "none": {nil, 0}, } { - t.Run(name, func(t *testing.T) { assert.Equal(t, tc.want, hasPricingOutageSkip(tc.reasons)) }) + t.Run(name, func(t *testing.T) { assert.Equal(t, tc.want, countPricingOutageSkips(tc.kinds)) }) } } // A pricing outage holds eviction; ordinary invalid-row skips do not. func TestTolerateSkipped_PricingOutageHoldsEviction(t *testing.T) { - gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, - reasons: []string{"advisor: price lookup unavailable: x"}}) + gap, err := tolerateIncompleteSweep("azure", outageSkip("advisor: price lookup unavailable: x")) require.NoError(t, err) assert.True(t, gap.holdsEviction()) assert.Equal(t, 1, gap.skippedRecommendations) @@ -162,11 +182,11 @@ func TestTolerateSkipped_PricingOutageHoldsEviction(t *testing.T) { func TestPricingOutageGap_FanOutRecordsHeldAccounts(t *testing.T) { accounts := []config.CloudAccount{{ID: "held", Name: "held"}, {ID: "plain", Name: "plain"}} fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) { - reason := "advisor: non-USD" + var skip error = &skipErr{partial: true, count: 1, reasons: []string{"advisor: non-USD"}} if acct.ID == "held" { - reason = "advisor: price lookup unavailable: x" + skip = outageSkip("advisor: price lookup unavailable: x") } - gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, reasons: []string{reason}}) + gap, err := tolerateIncompleteSweep("azure", skip) return []config.RecommendationRecord{{ID: acct.ID}}, gap, err } @@ -180,3 +200,48 @@ func TestPricingOutageGap_FanOutRecordsHeldAccounts(t *testing.T) { assert.Equal(t, []string{"held"}, outcome.IncompleteAccountIDs) assert.Equal(t, []string{"plain"}, outcome.SucceededAccountIDs) } + +// The hold decision reads only the typed kind. An outage skip whose reason text +// matches no known prefix still holds, and a skip whose text looks like an +// outage but is not of that kind does not: the library can reword its reasons +// without silently stopping or starting the hold. +func TestTolerateSkipped_HoldReadsTheKindNotTheText(t *testing.T) { + gap, err := tolerateIncompleteSweep("azure", outageSkip("the retail catalog said no")) + require.NoError(t, err) + assert.True(t, gap.holdsEviction(), "an outage with unfamiliar text still holds") + assert.True(t, gap.pricingUnavailable) + + lookalike := &kindSkipErr{skipErr: skipErr{partial: true, count: 1, reasons: []string{"advisor: price lookup unavailable: x"}}, + kinds: []azureprovider.SkipKind{azureprovider.SkipUnpriceable}} + gap, err = tolerateIncompleteSweep("azure", lookalike) + require.NoError(t, err) + assert.False(t, gap.holdsEviction(), "outage-looking text of another kind does not hold") + + // Kinds beyond the skips name no skip: the library ignores them, so they + // must not count as an outage. + extra := &kindSkipErr{skipErr: skipErr{partial: true, count: 1, reasons: []string{"a"}}, + kinds: []azureprovider.SkipKind{azureprovider.SkipUnpriceable, azureprovider.SkipPriceLookupUnavailable}} + gap, err = tolerateIncompleteSweep("azure", extra) + require.NoError(t, err) + assert.False(t, gap.holdsEviction(), "a kind with no skip behind it never holds") + + short := &kindSkipErr{skipErr: skipErr{partial: true, count: 2, reasons: []string{"a", "b"}}, + kinds: []azureprovider.SkipKind{azureprovider.SkipUnpriceable}} + gap, err = tolerateIncompleteSweep("azure", short) + require.NoError(t, err) + assert.False(t, gap.holdsEviction(), "a skip with no kind reads as unpriceable, never as an outage") +} + +// The same through the multi-subscription failure that also lost a subscription. +func TestGapFromPartialSubscriptions_HoldReadsTheKindNotTheText(t *testing.T) { + failed := []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("403 authorization failed")}} + gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailureWithKinds(2, failed, + []string{"the retail catalog said no"}, []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable})) + require.NoError(t, err) + assert.True(t, gap.forAccount().withHeld("acct-1").pricingUnavailable) + + gap, err = tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed, + []string{"advisor: price lookup unavailable: x"})) + require.NoError(t, err) + assert.False(t, gap.forAccount().withHeld("acct-1").pricingUnavailable, "no kinds recorded: the text alone never holds") +} diff --git a/internal/scheduler/partial_sweep.go b/internal/scheduler/partial_sweep.go index a510d0bbc..a79bc818b 100644 --- a/internal/scheduler/partial_sweep.go +++ b/internal/scheduler/partial_sweep.go @@ -41,7 +41,7 @@ type sweepGap struct { skippedOnly bool reasons map[string]int // pricingUnavailable is set when a skip meant the price could not be - // fetched, so the previous cycle's rows are held (see hasPricingOutageSkip). + // fetched, so the previous cycle's rows are held (see recordSkips). pricingUnavailable bool // heldAccountIDs are the platform account UUIDs whose rows are held for // that reason; the collector that knows the account fills them in. @@ -66,6 +66,13 @@ type partialResult interface { SkipReasons() []string } +// skipKinder is implemented by Azure's PartialResultError: one kind per skip +// reason, index-aligned (cloud-commitments-go#670). A provider without it, such +// as GCP, reads every skip as azureprovider.SkipUnpriceable. +type skipKinder interface { + SkipKinds() []azureprovider.SkipKind +} + // tolerateSkippedRecommendations keeps the recommendations returned alongside // a Partial() error and records how many were skipped. Any other error is // returned unchanged. @@ -76,33 +83,31 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap, } logging.Warnf("%s recommendations partial, keeping the rest: %v", providerName, err) g := &sweepGap{reasons: map[string]int{}} - g.recordSkips(p.SkippedCount(), p.SkipReasons()) + var kinds []azureprovider.SkipKind + if k, ok := p.(skipKinder); ok { + kinds = k.SkipKinds() + } + g.recordSkips(p.SkippedCount(), kinds) g.skippedOnly = !g.pricingUnavailable return g, nil } -// trimSubscriptionPrefixes drops the ": " that the Azure -// multi-subscription client puts before each skip it aggregates -// (recommendations_multi_subscription.go:399 at 61111428), so the reasons match -// the same prefixes as a single-subscription sweep. Only a leading segment -// followed by an Advisor skip is removed, so unprefixed reasons pass through. -func trimSubscriptionPrefixes(reasons []string) []string { - out := make([]string, len(reasons)) - for i, reason := range reasons { - out[i] = reason - if sub, rest, ok := strings.Cut(reason, ": "); ok && sub != "" && !strings.ContainsAny(sub, " :") && strings.HasPrefix(rest, "advisor: ") { - out[i] = rest - } - } - return out -} - -// recordSkips adds dropped recommendations to the gap. A skip that means "the -// price could not be fetched" says nothing about the row being invalid, so its -// previous-cycle row must survive: it marks the gap pricingUnavailable. -func (g *sweepGap) recordSkips(count int, reasons []string) { +// recordSkips adds dropped recommendations to the gap. A skip of kind +// SkipPriceLookupUnavailable means "the price could not be fetched", which says +// nothing about the row being invalid, so its previous-cycle row must survive: +// it marks the gap pricingUnavailable. The decision reads only the typed kinds, +// never the reason text, so a wording change in the library cannot silently +// stop holding rows. A truncated walk (SkipSourceIncomplete) and an unusable +// row (SkipUnpriceable) do not hold: holding on a walk that is always +// truncated would keep that account's stale rows forever. +func (g *sweepGap) recordSkips(count int, kinds []azureprovider.SkipKind) { g.skippedRecommendations += count - if outage := countPricingOutageSkips(trimSubscriptionPrefixes(reasons)); outage > 0 { + // Kinds are index-aligned with the skips, and the library ignores extra + // ones, so only the first count can name a skip. + if len(kinds) > count { + kinds = kinds[:count] + } + if outage := countPricingOutageSkips(kinds); outage > 0 { g.pricingUnavailable = true g.reasons[config.PartialReasonPricingUnavailable] += outage } @@ -119,42 +124,12 @@ func (g *sweepGap) withHeld(accountID string) *sweepGap { return out } -// Skip-reason prefixes cloud-commitments-go records when Azure Advisor rows -// cannot be priced because the Retail Prices catalog is unreachable -// (providers/azure/recommendations_advisor_pricing.go, priceAdvisorReservations, -// go#663). The library exports no constant for them: RecordSkip stores -// ": " strings. The first failed row is recorded with the -// collection-failure wrapper "azure advisor: "; every later row of the call -// gets "price lookup unavailable: ". -var pricingOutageSkipPrefixes = []string{ - "advisor: price lookup unavailable: ", - "advisor: azure advisor: ", -} - -// advisorTruncatedSkipPrefix starts the skip recorded for an Advisor walk that -// hit the page cap or a page error (collectAdvisorOrSkip, -// providers/azure/recommendations.go:471 at 61111428). It shares the -// "advisor: azure advisor: " prefix but is not a pricing outage: a walk over -// the page cap is always truncated, so holding eviction would keep that -// account's stale rows forever. -const advisorTruncatedSkipPrefix = "advisor: azure advisor: recommendation list is incomplete" - -// hasPricingOutageSkip reports whether any skip reason is a pricing outage. -func hasPricingOutageSkip(reasons []string) bool { - return countPricingOutageSkips(reasons) > 0 -} - -func countPricingOutageSkips(reasons []string) int { +// countPricingOutageSkips counts the skips whose price lookup did not complete. +func countPricingOutageSkips(kinds []azureprovider.SkipKind) int { n := 0 - for _, reason := range reasons { - if strings.HasPrefix(reason, advisorTruncatedSkipPrefix) { - continue - } - for _, prefix := range pricingOutageSkipPrefixes { - if strings.HasPrefix(reason, prefix) { - n++ - break - } + for _, kind := range kinds { + if kind == azureprovider.SkipPriceLookupUnavailable { + n++ } } return n @@ -187,7 +162,7 @@ func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureErro // user warning only and does not affect eviction (skippedOnly stays false), // but a pricing outage among them holds the account like a skips-only one. if sk := p.Skips(); sk != nil { - g.recordSkips(sk.SkippedCount(), sk.SkipReasons()) + g.recordSkips(sk.SkippedCount(), sk.SkipKinds()) } for _, f := range p.Failed { g.reasons[classifySweepCause(f.Err)]++ diff --git a/internal/scheduler/partial_sweep_test.go b/internal/scheduler/partial_sweep_test.go index 62d48be62..ed4ee02e2 100644 --- a/internal/scheduler/partial_sweep_test.go +++ b/internal/scheduler/partial_sweep_test.go @@ -292,7 +292,8 @@ func TestGapFromPartialSubscriptions_PricingOutageSkipsHoldAccount(t *testing.T) for i, r := range outage { prefixed[i] = "0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: " + r } - gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed, prefixed)) + gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailureWithKinds(2, failed, prefixed, + []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable, azureprovider.SkipPriceLookupUnavailable})) require.NoError(t, err) held := gap.forAccount().withHeld("acct-1") @@ -312,7 +313,8 @@ func TestTolerateIncompleteSweep_PrefixedPricingOutageSkipsHoldAccount(t *testin `0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, `0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`, } - gap, err := tolerateIncompleteSweep("azure", &azureprovider.PartialResultError{Skipped: skips}) + gap, err := tolerateIncompleteSweep("azure", &azureprovider.PartialResultError{Skipped: skips, + Kinds: []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable, azureprovider.SkipPriceLookupUnavailable}}) require.NoError(t, err) held := gap.forAccount().withHeld("acct-1")