From e0b8ab2f04c20606c8ef2e16f94d97d57c765c0c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 17:53:26 +0200 Subject: [PATCH 1/3] build(deps): bump providers/aws to pick up the expiry lookback credit Providers/aws 512e42fe adds PoolExpiryOptions and the WithCoverageOptions expiry adjustment (go#449). The pkg pin is unchanged. The bump also pulls newer EC2 scope validation errors, so the zonal AZ refusal test now matches the lowercase "availability zone" wording. Refs #2235 Co-Authored-By: Claude Opus 5.5 --- cmd/ec2_zonal_az_test.go | 3 ++- go.mod | 2 +- go.sum | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cmd/ec2_zonal_az_test.go b/cmd/ec2_zonal_az_test.go index abb8d4207..f74f62d78 100644 --- a/cmd/ec2_zonal_az_test.go +++ b/cmd/ec2_zonal_az_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "path/filepath" + "strings" "sync" "testing" @@ -90,7 +91,7 @@ func TestPurchaseSingleRec_RefusesInconsistentEC2ScopeBeforeAnyAWSCall(t *testin result, recorder := purchaseEC2WithRecorder(t, rec) require.Error(t, result.Error) - assert.Contains(t, result.Error.Error(), "AvailabilityZone") + assert.Contains(t, strings.ToLower(result.Error.Error()), "availability zone") assert.Empty(t, recorder.actions, "no AWS call may precede the refusal") }) } diff --git a/go.mod b/go.mod index 7fb6388f5..6c7b481cc 100644 --- a/go.mod +++ b/go.mod @@ -83,7 +83,7 @@ require ( require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0 github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016175124-570d168c6bc7 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261013212804-9b35837e78d1 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261016165653-512e42fe37c6 github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261013212804-9b35837e78d1 github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261013212804-9b35837e78d1 github.com/aws/aws-sdk-go-v2/service/organizations v1.45.3 diff --git a/go.sum b/go.sum index 9ae81a29c..83477ee9e 100644 --- a/go.sum +++ b/go.sum @@ -80,8 +80,8 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapp github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016175124-570d168c6bc7 h1:mAvqQSMkTuw4P8m8o+/wdyoxGE9L6eiEzZFanGXqYcI= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016175124-570d168c6bc7/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261013212804-9b35837e78d1 h1:c4JuGObXDFP+oqC7CD+0Sn9ljzlUciVikW7iMDCNNa4= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261013212804-9b35837e78d1/go.mod h1:no7i/7i+W37+qhEOVU8F/0dP5FhO3wtzvKIN0+tvL3A= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261016165653-512e42fe37c6 h1:nxcsaVj7BKH0/I9wiTvraQPivKABf/GzE7Ecwm47u9U= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261016165653-512e42fe37c6/go.mod h1:uOUuTVRjTm0Fhu/Vra2M2NES0w4pvZUSfZLsGyCd18s= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261013212804-9b35837e78d1 h1:aYYu7JDJQcoriUKg/m+IgTJruc2ugfBLXafAC2LQy00= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261013212804-9b35837e78d1/go.mod h1:8vqO2ZlDzjX/Z4bESZVp/IenhTifkySeF0VJwQKihLA= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261013212804-9b35837e78d1 h1:LPI8x3lOnrvr4w3oX8cHsCLoXuCSLSUhcHbe4WCMUSQ= From 3082f64cbdb83b135c28dd2d3ae942df2fde8c06 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 17:53:32 +0200 Subject: [PATCH 2/3] fix(aws): credit replacement RIs bought inside the coverage lookback --target-coverage with --rebuy-window-days now calls the WithCoverageOptions expiry adjustment, passing the same lookback the coverage map was fetched with, so a re-run does not re-buy replacement RIs already bought. Fixes #2235 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +++ cmd/multi_service_helpers.go | 9 ++++--- cmd/reservation_expiry_command_test.go | 27 ++++++++++++++++++-- cmd/reservation_expiry_test.go | 35 ++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68d8361c5..b97c3701a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `--target-coverage` with `--rebuy-window-days` no longer buys replacement EC2 and + other non-RDS RIs again when they were already bought inside the coverage + lookback: the expiring RIs are netted against those replacements (#2235). - Recommendations with no account ID or name are now reported in the drop summary when an account filter (`--include-accounts` or `--exclude-accounts`) removes them (#1613). diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index ec1f8acc8..a8bb73d9a 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -577,8 +577,9 @@ func applyRegionFilters( // // expiringCommitments (when non-empty and cfg.RebuyWindowDays > 0) reduces // ExistingCoveragePct further by the share of pool demand attributable to -// RIs expiring within the window, so --target-coverage recommends -// replacements before the cliff. Doesn't run if either input is empty. +// RIs expiring within the window, net of replacement RIs started inside the +// coverage lookback (so a re-run does not buy them again), so --target-coverage +// recommends replacements before the cliff. Doesn't run if either input is empty. // RDS recs sized by the family-NU pass (target-coverage mode) ignore that // per-rec adjustment and instead get the same commitments and window through // ApplyFamilyNUSizingRDSWithExpiryOptions, which deducts expiring normalized units @@ -592,7 +593,9 @@ func applyRegionFilters( func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverageMap recommendations.PoolCoverageMap, expiringCommitments []common.Commitment, drops *common.DropSummary) []common.Recommendation { recommendations.ApplyCoverageMapToRecommendations(recs, coverageMap) if cfg.RebuyWindowDays > 0 && len(expiringCommitments) > 0 { - n, missingDemand := recommendations.AdjustExistingCoverageForExpiringCommitmentsWithCoverage(recs, expiringCommitments, cfg.RebuyWindowDays, coverageMap) + n, missingDemand := recommendations.AdjustExistingCoverageForExpiringCommitmentsWithCoverageOptions( + recs, expiringCommitments, coverageMap, + recommendations.PoolExpiryOptions{WindowDays: cfg.RebuyWindowDays, CoverageLookbackDays: coverageLookbackDays(cfg)}) if n > 0 { AppLogger.Printf(" ⏰ Treating %d recs as partially uncovered (RIs expiring within %d days)\n", n, cfg.RebuyWindowDays) } diff --git a/cmd/reservation_expiry_command_test.go b/cmd/reservation_expiry_command_test.go index 0306cf900..8a9f795ea 100644 --- a/cmd/reservation_expiry_command_test.go +++ b/cmd/reservation_expiry_command_test.go @@ -23,6 +23,7 @@ type reservationExpiryScenario struct { rows, expiry, endDays, count int average, demand, target, existing float64 missing, filtered bool + replacedDays, replacedCount int } func reservationExpiryCase(name string) reservationExpiryScenario { @@ -31,6 +32,11 @@ func reservationExpiryCase(name string) reservationExpiryScenario { case "pool-demand": case "outside-window": s.endDays, s.count, s.existing = 180, 6, 60 + case "replaced-inside-lookback": + // Replacements bought 5 days ago are inside the 30-day coverage + // lookback, so the expiring RIs net to zero and the run must not + // re-buy them (cli#2235). + s.count, s.existing, s.replacedDays, s.replacedCount = 6, 60, 5, 18 case "missing-demand-two-each-six-total": s.average, s.demand, s.count, s.existing, s.missing = 10, 0, 2, 60, true case "filtered-subset": @@ -47,7 +53,7 @@ func reservationExpiryCase(name string) reservationExpiryScenario { } func TestReservationExpiryCommand(t *testing.T) { - for _, name := range []string{"pool-demand", "outside-window", "missing-demand-two-each-six-total", "filtered-subset", "exact-boundary", "below-boundary"} { + for _, name := range []string{"pool-demand", "outside-window", "replaced-inside-lookback", "missing-demand-two-each-six-total", "filtered-subset", "exact-boundary", "below-boundary"} { t.Run(name, func(t *testing.T) { runCompletenessScenario(t, "ec2", "explicit", name) }) } } @@ -189,7 +195,7 @@ func (p *completenessProxy) reservationExpiryResponse(host, op string, body []by case "DescribeInstanceTypes": return 200, "text/xml", `m5.large28192` case "DescribeReservedInstances": - return 200, "text/xml", fmt.Sprintf(`expiry-fixturem5.large%dactive%s%s`, s.expiry, time.Now().AddDate(-1, 0, 0).UTC().Format(time.RFC3339), time.Now().AddDate(0, 0, s.endDays).UTC().Format(time.RFC3339)) + return 200, "text/xml", reservationExpiryReservedInstances(s) } } if host == "rds.us-east-1.amazonaws.com" && (op == "DescribeDBInstances" || op == "DescribeDBMajorEngineVersions") { @@ -208,6 +214,23 @@ func (p *completenessProxy) reservationExpiryResponse(host, op string, body []by return 403, "text/plain", "fixture rejected operation" } +// reservationExpiryReservedInstances renders the owned EC2 RIs the fixture +// reports: the expiring RI that --rebuy-window-days acts on, plus a +// replacement bought inside the coverage lookback when the scenario asks for +// one. +func reservationExpiryReservedInstances(s reservationExpiryScenario) string { + item := func(id string, count int, start, end time.Time) string { + return fmt.Sprintf(`%sm5.large%dactive%s%s`, + id, count, start.UTC().Format(time.RFC3339), end.UTC().Format(time.RFC3339)) + } + now := time.Now() + items := item("expiry-fixture", s.expiry, now.AddDate(-1, 0, 0), now.AddDate(0, 0, s.endDays)) + if s.replacedCount > 0 { + items += item("replacement-fixture", s.replacedCount, now.AddDate(0, 0, -s.replacedDays), now.AddDate(0, 0, 400)) + } + return fmt.Sprintf(`%s`, items) +} + func (p *completenessProxy) reservationExpiryCoverage(body []byte) string { var request struct { TimePeriod map[string]string diff --git a/cmd/reservation_expiry_test.go b/cmd/reservation_expiry_test.go index e469dbbc3..b17174e58 100644 --- a/cmd/reservation_expiry_test.go +++ b/cmd/reservation_expiry_test.go @@ -50,6 +50,41 @@ func TestReservationExpirySizing(t *testing.T) { } } +// A --target-coverage run must not re-buy replacements already bought inside +// the coverage lookback (cli#2235): the lagged coverage average only partly +// holds them, so the expiring RIs are netted against the replacements. +func TestReservationExpiryCreditsReplacementsInsideLookback(t *testing.T) { + coverage := recommendations.PoolCoverageMap{"us-east-1:m5.large": {Pct: 60, AvgInstancesPerHour: 90}} + replacement := func(startDaysAgo int) common.Commitment { + c := reservationExpiryCommitments(18, 400)[0] + c.StartDate = time.Now().AddDate(0, 0, -startDaysAgo) + return c + } + for _, tc := range []struct { + name string + lookbackDays int + startDaysAgo int + wantCount int + wantExisting float64 + }{ + {"replaced-inside-lookback", 30, 5, 6, 60}, + {"default-lookback-when-unset", 0, 5, 6, 60}, + {"replaced-outside-lookback", 30, 45, 12, 40}, + {"lookback-narrower-than-purchase", 3, 5, 12, 40}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := Config{TargetCoverage: 80, RebuyWindowDays: 30, CoverageLookbackDays: tc.lookbackDays} + commitments := append(reservationExpiryCommitments(18, 15), replacement(tc.startDaysAgo)) + got := applyCoverageAndOverrides(reservationExpiryRecommendations([]float64{30, 30, 30}), cfg, coverage, commitments, nil) + require.Len(t, got, 3) + for _, rec := range got { + assert.Equal(t, tc.wantCount, rec.Count) + assert.InDelta(t, tc.wantExisting, rec.ExistingCoveragePct, 1e-12) + } + }) + } +} + func TestReservationExpiryMissingDemand(t *testing.T) { for _, tc := range []struct { name string From 59b42a95acf25d37b94dab9f07134324cce274c8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 18:13:38 +0200 Subject: [PATCH 3/3] fix(aws): align the Savings Plans commitment precheck with the library's rule The library now formats the hourly commitment through spcommitment.Normalize: 0.001 to 1e6, truncated to 5 decimals. The CLI precheck still mirrored the old cents rounding and rejected 0.001 to 0.004. Mirror the new bounds (and reject NaN/Inf) so a dry run and the confirmation prompt still fail before any purchase starts. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++++ cmd/savings_plan_purchase.go | 23 ++++++++++++++--------- cmd/savings_plan_purchase_test.go | 16 +++++++--------- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b97c3701a..3623a02b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- Savings Plans purchases now send the hourly commitment truncated to 5 decimals + (at least 2 shown) instead of rounded to cents, and accept amounts from 0.001 + to 1,000,000, so a 0.001 to 0.004 commitment is no longer rejected (library + bump, #2235). - `--target-coverage` with `--rebuy-window-days` no longer buys replacement EC2 and other non-RDS RIs again when they were already bought inside the coverage lookback: the expiring RIs are netted against those replacements (#2235). diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go index 1d37727b1..7ad0e28aa 100644 --- a/cmd/savings_plan_purchase.go +++ b/cmd/savings_plan_purchase.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "math" "strconv" "strings" "time" @@ -17,11 +18,15 @@ import ( // regions and are not supported for Savings Plan purchases. const spAPIRegion = "us-east-1" -// spCommitmentFormat is how the library formats HourlyCommitment for the -// CreateSavingsPlan call (savingsplans/client.go). A positive value that -// formats as "0.00" would be sent as a zero commitment, so preconditions -// reject it. -const spCommitmentFormat = "%.2f" +// Hourly commitment bounds accepted by the library's CreateSavingsPlan path. +// Cross-repo mirror: providers/aws/internal/spcommitment.Normalize in +// cloud-commitments-go is the source of truth (it also truncates the sent +// amount to 5 decimals). The CLI repeats the bounds so a dry run and the +// confirmation prompt reject a bad amount before any purchase starts. +const ( + spMinHourlyCommitment = 0.001 + spMaxHourlyCommitment = 1000000.0 +) // savingsPlanRowKey identifies one Savings Plan purchase. Two rows with the // same key buy the same plan twice. @@ -146,11 +151,11 @@ func validateSavingsPlanPreconditions(rec common.Recommendation) error { if d.PlanType == "" { return fmt.Errorf("%s recommendation is missing the plan type", rec.Service) } - if d.HourlyCommitment <= 0 { - return fmt.Errorf("%s recommendation has no positive hourly commitment (got %v)", rec.Service, d.HourlyCommitment) + if math.IsNaN(d.HourlyCommitment) || math.IsInf(d.HourlyCommitment, 0) || d.HourlyCommitment <= 0 { + return fmt.Errorf("%s recommendation has no positive finite hourly commitment (got %v)", rec.Service, d.HourlyCommitment) } - if fmt.Sprintf(spCommitmentFormat, d.HourlyCommitment) == "0.00" { - return fmt.Errorf("%s recommendation's hourly commitment %v would be sent as 0.00; it must be at least 0.005", rec.Service, d.HourlyCommitment) + if d.HourlyCommitment < spMinHourlyCommitment || d.HourlyCommitment > spMaxHourlyCommitment { + return fmt.Errorf("%s recommendation's hourly commitment %v must be between %g and %g", rec.Service, d.HourlyCommitment, spMinHourlyCommitment, spMaxHourlyCommitment) } if rec.Service == common.ServiceSavingsPlansEC2Instance { return validateEC2InstanceSPOffering(d) diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index af4f05e68..998bf24f1 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/csv" "io" + "math" "net/http" "os" "path/filepath" @@ -298,21 +299,18 @@ func TestValidatePurchasePreconditions(t *testing.T) { } } -// TestValidatePurchasePreconditions_RejectsCommitmentThatRoundsToZero covers -// issue #2163: the library sends the hourly commitment as "%.2f", so a positive -// value below 0.005 would reach CreateSavingsPlan as "0.00". -func TestValidatePurchasePreconditions_RejectsCommitmentThatRoundsToZero(t *testing.T) { +// TestValidatePurchasePreconditions_CommitmentBounds mirrors the library's +// spcommitment.Normalize bounds (0.001..1e6, positive, finite). +func TestValidatePurchasePreconditions_CommitmentBounds(t *testing.T) { withCommitment := func(c float64) common.Recommendation { rec := parserShapedSP() rec.Details = &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: c} return rec } - for _, c := range []float64{0.001, 0.004, 0.00499} { - err := validatePurchasePreconditions(withCommitment(c), "", defaultTestAccount) - require.Error(t, err, "commitment %v rounds to 0.00 and must be rejected", c) - assert.Contains(t, err.Error(), "0.00", "commitment %v", c) + for _, c := range []float64{0.0009, 0.00099, 0, -1, 1000000.1, math.NaN(), math.Inf(1)} { + assert.Error(t, validatePurchasePreconditions(withCommitment(c), "", defaultTestAccount), "commitment %v", c) } - for _, c := range []float64{0.005, 0.01, 1.5} { + for _, c := range []float64{0.001, 0.004, 0.005, 1.239, 1.2345678, 1000000} { assert.NoError(t, validatePurchasePreconditions(withCommitment(c), "", defaultTestAccount), "commitment %v", c) } }