diff --git a/CHANGELOG.md b/CHANGELOG.md index 933ded6b0..430f2b3ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `--max-instances` drop summary: a zero-Count recommendation swept out by the + `--min-count` floor no longer drives the tally negative (which panicked in + `DropSummary.Add`), and removed zero-Count entries are now counted and named + (#1742). - `--max-instances` and `--min-count` no longer apply to Savings Plans, whose Count of 1 is not an instance count: a Savings Plan neither uses up the instance cap nor is dropped by the floor (#1613). diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 56848e503..bf1a39ccc 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -497,26 +497,32 @@ func reportInstanceLimit(before, after []common.Recommendation, belowMinCount, t maxInstances, len(before), totalBefore) AppLogger.Printf(" Keeping the %s. The following are reduced or dropped:\n", rule) + // after is a prefix of before, so membership is positional: every entry past + // len(after) was removed, whatever its Count (a zero-Count entry equals its + // own "kept" count of 0 and must still be counted as removed). reduced, dropped := 0, 0 for i := range before { rec := before[i] - kept := 0 - if i < len(after) { - kept = after[i].Count - } - switch { - case kept == rec.Count: + if i >= len(after) { + dropped++ + AppLogger.Printf(" • dropped: %s %s %s (%d instances)\n", rec.Service, rec.Region, rec.ResourceType, rec.Count) continue - case kept > 0: + } + if kept := after[i].Count; kept != rec.Count { reduced++ AppLogger.Printf(" • reduced: %s %s %s %d → %d instances\n", rec.Service, rec.Region, rec.ResourceType, rec.Count, kept) - default: - dropped++ - AppLogger.Printf(" • dropped: %s %s %s (%d instances)\n", rec.Service, rec.Region, rec.ResourceType, rec.Count) } } - drops.Add(common.DropMaxInstances, dropped-belowMinCount) + // The floor only removes entries from the tail of what the cap kept, so + // they are a subset of the removed entries. DropSummary.Add panics on a + // negative count; surface the broken invariant with context instead. + budgetDropped := dropped - belowMinCount + if budgetDropped < 0 { + panic(fmt.Sprintf("reportInstanceLimit: %d recommendations attributed to --min-count-after-cap but only %d were removed (before=%d, after=%d)", + belowMinCount, dropped, len(before), len(after))) + } + drops.Add(common.DropMaxInstances, budgetDropped) AppLogger.Printf(" Proceeding with %d instances across %d recommendations (%d reduced, %d dropped).\n", CalculateTotalInstances(after), len(after), reduced, dropped) } diff --git a/cmd/multi_service_drop_attribution_test.go b/cmd/multi_service_drop_attribution_test.go new file mode 100644 index 000000000..9a552e6a7 --- /dev/null +++ b/cmd/multi_service_drop_attribution_test.go @@ -0,0 +1,61 @@ +package main + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func dropRec(rt string, count int) common.Recommendation { + return common.Recommendation{Service: common.ServiceEC2, Region: "us-east-1", ResourceType: rt, Count: count} +} + +// A zero-Count entry swept out by the --min-count floor (behind a truncated +// tail) used to be uncounted as a budget drop (kept == rec.Count == 0) yet +// subtracted as a floor drop, driving the --max-instances tally to -1 and +// panicking in DropSummary.Add (#1742). +func TestApplyGlobalInstanceLimitZeroCountTailDoesNotPanic(t *testing.T) { + drops := common.NewDropSummary() + recs := []common.Recommendation{dropRec("m5.large", 4), dropRec("c5.large", 0), dropRec("m5.xlarge", 6)} + + var got []common.Recommendation + require.NotPanics(t, func() { + got = applyGlobalInstanceLimit(recs, Config{MaxInstances: 7, MinCount: 4}, rankBySavingsPercentage, drops) + }) + require.Len(t, got, 1) + assert.Equal(t, "m5.large", got[0].ResourceType) + // The truncated m5.xlarge and the zero-Count c5.large are both floor + // drops, counted once each and none attributed to the budget. + assert.Equal(t, 2, drops.Total()) + assert.Contains(t, drops.FormatOneLine(), "min-count-after-cap=2") + assert.NotContains(t, drops.FormatOneLine(), "--max-instances") +} + +// A zero-Count entry the budget left out is a removed recommendation and is +// attributed to --max-instances, not skipped by the kept == rec.Count check. +func TestReportInstanceLimitCountsZeroCountBudgetDrop(t *testing.T) { + before := []common.Recommendation{dropRec("m5.large", 6), dropRec("m5.xlarge", 6), dropRec("c5.large", 0)} + after := ApplyInstanceLimit(before, 6) + require.Len(t, after, 1) + + drops := common.NewDropSummary() + out := captureAppOutput(t, func() { + reportInstanceLimit(before, after, 0, CalculateTotalInstances(before), 6, rankBySavingsPercentage, drops) + }) + assert.Equal(t, 2, drops.Total()) + assert.Contains(t, out, "c5.large") + assert.Contains(t, out, "2 dropped") +} + +// More floor drops than removed entries is an impossible state and must fail +// loudly with context rather than clamp or hit DropSummary.Add's bare panic. +func TestReportInstanceLimitImpossibleAttributionPanicsWithContext(t *testing.T) { + before := []common.Recommendation{dropRec("m5.large", 6)} + assert.PanicsWithValue(t, + "reportInstanceLimit: 2 recommendations attributed to --min-count-after-cap but only 0 were removed (before=1, after=1)", + func() { + reportInstanceLimit(before, before, 2, 6, 6, rankBySavingsPercentage, common.NewDropSummary()) + }) +}