From 9e3f9eb5128a7b65861f8f3104ac8c73d2b39f11 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 21:03:39 +0200 Subject: [PATCH 1/2] fix(rds): deduct expiring RIs from family coverage in target-coverage sizing applyCoverageAndOverrides adjusted per-rec coverage for RIs expiring inside --rebuy-window-days but passed the original coverage map to the RDS family-NU pass, which dropped the family at gap <= 0. Call ApplyFamilyNUSizingRDSWithExpiry with the existing commitments and the rebuy window, and log FamilyExpiryReport.Unusable so an expiring RI that could not be deducted is never read as "no expiry". RDS counts and costs deliberately increase in the expiring-RI case. Non-RDS and EC2 paths and the nil-commitments caller are unchanged. Add an rds-expiry mode to the recorded-HTTP completeness proxy with command-level scenarios (already-at-target, below-target, large RI vs xlarge usage, multi-size with linked accounts, partial inventory, unusable RIs) and labeled controls. Closes #2132 Co-Authored-By: Claude Sonnet 5.5 --- cmd/multi_service_helpers.go | 26 +- cmd/rds_family_expiry_command_test.go | 383 ++++++++++++++++++ cmd/recommendation_completeness_proxy_test.go | 11 +- cmd/recommendation_completeness_test.go | 11 +- 4 files changed, 427 insertions(+), 4 deletions(-) create mode 100644 cmd/rds_family_expiry_command_test.go diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index c420bfa7d..b767abc56 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -469,6 +469,8 @@ func processRegionRecommendations( // fetch expiring commitments — expiry-aware sizing only runs via the // main pipeline in fetchAndFilterRegionRecs, which has access to the // regional service client at the right moment. Drop tracking skipped (nil). + // nil commitments: no expiry deduction, so family sizing is identical to + // ApplyFamilyNUSizingRDS (documented on applyCoverageAndOverrides). filteredRecs := applyCoverageAndOverrides(recs, cfg, coverageMap, nil, nil) result.recommendations = filteredRecs @@ -616,6 +618,12 @@ func applyRegionFilters( // 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. +// 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 +// ApplyFamilyNUSizingRDSWithExpiry, which deducts expiring normalized units +// from the family's coverage; this deliberately raises RDS counts and costs +// when RIs are about to expire (cli#2132). Callers passing nil commitments +// get the plain ApplyFamilyNUSizingRDS behavior. // // drops accumulates per-reason drop counts for the end-of-run summary. // Pass nil to skip tracking. @@ -640,7 +648,10 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag rest := recs if cfg.TargetCoverage > 0 { var familyDrops recommendations.FamilyDropCounts - sizedRDS, rest, familyDrops = recommendations.ApplyFamilyNUSizingRDS(recs, coverageMap, cfg.TargetCoverage) + var expiry recommendations.FamilyExpiryReport + sizedRDS, rest, familyDrops, expiry = recommendations.ApplyFamilyNUSizingRDSWithExpiry( + recs, coverageMap, cfg.TargetCoverage, expiringCommitments, cfg.RebuyWindowDays) + logFamilyExpiry(expiry, cfg.RebuyWindowDays) drops.Add(common.DropFamilyAlreadyAtTarget, familyDrops.AlreadyAtTarget) drops.Add(common.DropFamilyNoNUSignal, familyDrops.NoNUSignal) drops.Add(common.DropFamilySizedToZero, familyDrops.SizedToZero) @@ -662,6 +673,19 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag return filteredRecs } +// logFamilyExpiry reports how rebuy-window expiry was folded into the RDS +// family-NU sizing. Unusable RIs (unparsable instance class, or an engine +// spelled differently from the coverage report) were NOT deducted, so they are +// warned about rather than read as "no expiry". +func logFamilyExpiry(expiry recommendations.FamilyExpiryReport, windowDays int) { + if expiry.ExpiringNU > 0 { + AppLogger.Printf(" ⏰ RDS family sizing: deducting %.0f normalized units of RIs expiring within %d days from family coverage\n", expiry.ExpiringNU, windowDays) + } + if expiry.Unusable > 0 { + AppLogger.Printf(" ⚠️ %d RDS reserved instances expiring within %d days could not be applied to family sizing (unparsable instance class or no matching coverage family); their expiry was NOT deducted\n", expiry.Unusable, windowDays) + } +} + // checkDuplicates adjusts recommendations against already-owned RIs so the run // does not double-purchase existing capacity. // diff --git a/cmd/rds_family_expiry_command_test.go b/cmd/rds_family_expiry_command_test.go new file mode 100644 index 000000000..88c946834 --- /dev/null +++ b/cmd/rds_family_expiry_command_test.go @@ -0,0 +1,383 @@ +package main + +// Command-level regression tests for cli#2132: with --target-coverage and +// --rebuy-window-days, RDS family-NU sizing must deduct RIs that expire inside +// the window from the family's existing coverage. The harness drives the real +// root command against a recorded-HTTP proxy (Cost Explorer, RDS and +// Organizations responses are synthetic fixtures), dry-run only: no purchase +// operation is permitted. +// +// Every scenario below that is not labeled "control" fails on the commit +// before the fix (the family was sized against the un-adjusted coverage). + +import ( + "bytes" + "encoding/csv" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "os" + "strconv" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +const rdsExpiryService = "rds-expiry" + +// rdsExpiryUsage is one Cost Explorer coverage row: average running instances +// per hour of one class and the share of those hours covered by RIs. +type rdsExpiryUsage struct { + engine, class string + avg, pct float64 +} + +// rdsExpiryRI is one DescribeReservedDBInstances entry. +type rdsExpiryRI struct { + class, engine string + multiAZ bool + count int + endDays int +} + +// rdsExpiryRec is one Cost Explorer purchase recommendation row (one linked +// account, one class). The fixture prices every recommended instance at 100 +// upfront, 100 monthly savings and 10 recurring, so costs scale with count. +type rdsExpiryRec struct { + account int + class string + count int +} + +type rdsExpiryScenario struct { + usage []rdsExpiryUsage + ris []rdsExpiryRI + recs []rdsExpiryRec + // want is the expected Count per rec after sizing (0 = the rec is absent + // from the CSV). existing is the expected family ExistingCoverage column + // for the surviving rows. unusable is the expected number of expiring RIs + // reported as not applied to family sizing. + want []int + existing float64 + unusable int + // parent documents the count the commit before the fix produced. + parent []int +} + +const rdsExpiryTarget = 80 + +var rdsExpiryAccountIDs = func() []string { + ids := make([]string, 4) + for i := range ids { + ids[i] = strings.Repeat(strconv.Itoa(i+1), 12) + } + return ids +}() + +func rdsExpiryCase(name string) rdsExpiryScenario { + mysql := func(class string, avg, pct float64) rdsExpiryUsage { + return rdsExpiryUsage{engine: "MySQL", class: class, avg: avg, pct: pct} + } + ri := func(class string, count, endDays int) rdsExpiryRI { + return rdsExpiryRI{class: class, engine: "mysql", count: count, endDays: endDays} + } + one := func(class string, count int) []rdsExpiryRec { return []rdsExpiryRec{{0, class, count}} } + switch name { + case "already-at-target": + // 80% covered by 16 large RIs, 3 of them expiring: the family looks at + // target, so the parent drops it. After expiry it is 65% covered and + // the 15% gap (12 NU) buys exactly the 3 expiring RIs back. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 80)}, + ris: []rdsExpiryRI{ri("db.r6g.large", 13, 200), ri("db.r6g.large", 3, 15)}, + recs: one("db.r6g.large", 8), want: []int{3}, existing: 65, parent: []int{0}, + } + case "below-target": + // 40% covered by 8 large RIs, 2 expiring: post-expiry 30%, so the gap + // is 50% (40 NU) = 10 large instead of the parent's 8. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 40)}, + ris: []rdsExpiryRI{ri("db.r6g.large", 6, 200), ri("db.r6g.large", 2, 15)}, + recs: one("db.r6g.large", 8), want: []int{10}, existing: 30, parent: []int{8}, + } + case "large-ri-xlarge-usage": + // Cost Explorer reports coverage under the running size (xlarge) while + // the expiring RIs are large: 4 large = 16 NU expire, so the family + // drops from 80% to 60% and buys 2 xlarge (16 NU). + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.xlarge", 10, 80)}, + ris: []rdsExpiryRI{ri("db.r6g.large", 12, 200), ri("db.r6g.large", 4, 15)}, + recs: one("db.r6g.xlarge", 4), want: []int{2}, existing: 60, parent: []int{0}, + } + case "multi-size-linked-accounts": + // Two sizes and three linked-account rows in one family: the 16 expiring + // NU are deducted once (family 40% -> 20%), so the 60% gap (48 NU) splits + // 2 + 4 + 2. A double deduction would buy 2 + 5 + 2; no deduction, 1 + 2 + 1. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.xlarge", 5, 40), mysql("db.r6g.large", 10, 40)}, + ris: []rdsExpiryRI{ri("db.r6g.large", 4, 200), ri("db.r6g.large", 4, 15)}, + recs: []rdsExpiryRec{ + {0, "db.r6g.xlarge", 4}, {1, "db.r6g.large", 8}, {2, "db.r6g.xlarge", 4}, + }, + want: []int{2, 4, 2}, existing: 20, parent: []int{1, 2, 1}, + } + case "partial-inventory": + // Cost Explorer covers 64 NU org-wide but the single-account inventory + // lists only the 4 expiring large RIs (16 NU); the other accounts' RIs + // are missing. The deduction is capped at the 16 expiring NU (80% -> + // 60%, buy 4 large), never the whole supplied-vs-covered difference. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 80)}, + ris: []rdsExpiryRI{ri("db.r6g.large", 4, 15)}, + recs: one("db.r6g.large", 8), want: []int{4}, existing: 60, parent: []int{0}, + } + case "unusable-expiring-warned": + // Expiring RIs the family pass cannot use (multi-AZ with no coverage + // family, and an unparsable class) must be reported, not read as "no + // expiry". Counts are unchanged from the parent; only the warning is new. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 40)}, + ris: []rdsExpiryRI{ + ri("db.r6g.large", 8, 200), + {class: "db.r6g.large", engine: "mysql", multiAZ: true, count: 2, endDays: 15}, + ri("db.t3.weird", 1, 15), + }, + recs: one("db.r6g.large", 8), want: []int{8}, existing: 40, unusable: 2, parent: []int{8}, + } + case "control-outside-window": + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 40)}, + ris: []rdsExpiryRI{ri("db.r6g.large", 8, 180)}, + recs: one("db.r6g.large", 8), want: []int{8}, existing: 40, parent: []int{8}, + } + case "control-engine-isolation": + // The expiring RIs belong to a PostgreSQL family; the MySQL family must + // not be touched. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 40), {engine: "PostgreSQL", class: "db.r6g.large", avg: 20, pct: 80}}, + ris: []rdsExpiryRI{{class: "db.r6g.large", engine: "postgres", count: 16, endDays: 15}}, + recs: one("db.r6g.large", 8), want: []int{8}, existing: 40, parent: []int{8}, + } + } + panic("unknown rds expiry scenario: " + name) +} + +func TestRDSFamilyExpiryCommand(t *testing.T) { + for _, name := range []string{ + "already-at-target", "below-target", "large-ri-xlarge-usage", "multi-size-linked-accounts", + "partial-inventory", "unusable-expiring-warned", "control-outside-window", "control-engine-isolation", + } { + t.Run(name, func(t *testing.T) { runCompletenessScenario(t, rdsExpiryService, "explicit", name) }) + } +} + +func rdsExpiryArgs(string) []string { + return []string{"--payment", "partial-upfront", "--target-coverage", strconv.Itoa(rdsExpiryTarget), "--rebuy-window-days", "30"} +} + +func assertRDSExpiryCSV(t *testing.T, output, name, logs string) { + t.Helper() + s := rdsExpiryCase(name) + data, err := os.ReadFile(output) + require.NoError(t, err, logs) + rows, err := csv.NewReader(bytes.NewReader(data)).ReadAll() + require.NoError(t, err) + t.Logf("actual synthetic RDS expiry CSV scenario=%s rows=%v", name, rows) + for _, failure := range []string{"Failed to fetch", "incomplete AWS", "Could not fetch", "failed to fetch", "WARNING:", "catalog fetch"} { + require.NotContains(t, logs, failure) + } + if s.unusable > 0 { + require.Contains(t, logs, fmt.Sprintf("%d RDS reserved instances expiring within 30 days could not be applied to family sizing", s.unusable), logs) + } else { + require.NotContains(t, logs, "could not be applied to family sizing", logs) + } + byAccount := make(map[string]map[string]string) + total := 0 + for _, row := range rows[1 : len(rows)-1] { + columns := make(map[string]string) + for i, header := range rows[0] { + columns[header] = row[i] + } + require.NotContains(t, byAccount, columns["Account"], "duplicate account row") + byAccount[columns["Account"]] = columns + } + for i, rec := range s.recs { + columns, present := byAccount[rdsExpiryAccountIDs[rec.account]] + count := s.want[i] + if count == 0 { + require.False(t, present, "scenario=%s rec %d must be dropped (parent behavior); rows=%v", name, i, rows) + continue + } + require.True(t, present, "scenario=%s rec %d missing; rows=%v\nlogs=%s", name, i, rows, logs) + want := map[string]string{ + "Service": "rds", "Region": "us-east-1", "ResourceType": rec.class, "Term": "1yr", "PaymentOption": "partial-upfront", + "Count": strconv.Itoa(count), "ExistingCoverage": fmt.Sprintf("%.1f", s.existing), + "UpfrontPayment": fmt.Sprintf("%.2f", float64(count)*100), "EstimatedSavings": fmt.Sprintf("%.2f", float64(count)*100), + "RecurringMonthlyCost": fmt.Sprintf("%.2f", float64(count)*10), "Success": "true", "Error": "", + } + for field, value := range want { + require.Equal(t, value, columns[field], "scenario=%s rec=%d field=%s (parent count %d)", name, i, field, s.parent[i]) + } + total += count + } + require.Len(t, byAccount, countNonZero(s.want), "unexpected extra rows: %v", rows) + if total > 0 { + last := rows[len(rows)-1] + require.Equal(t, "TOTAL", last[0]) + for i, header := range rows[0] { + switch header { + case "Count": + require.Equal(t, strconv.Itoa(total), last[i]) + case "UpfrontPayment", "EstimatedSavings": + require.Equal(t, fmt.Sprintf("%.2f", float64(total)*100), last[i]) + case "RecurringMonthlyCost": + require.Equal(t, fmt.Sprintf("%.2f", float64(total)*10), last[i]) + } + } + } +} + +func countNonZero(values []int) int { + n := 0 + for _, v := range values { + if v != 0 { + n++ + } + } + return n +} + +func (p *completenessProxy) respondRDSExpiry(req *http.Request, body []byte) *http.Response { + p.mu.Lock() + defer p.mu.Unlock() + op := req.Header.Get("X-Amz-Target") + values, err := url.ParseQuery(string(body)) + if op == "" { + require.NoError(p.t, err) + op = values.Get("Action") + } + p.requests[req.Host+"/"+op]++ + if req.Method != http.MethodPost || req.URL.Path != "/" || req.URL.RawQuery != "" { + p.t.Errorf("unexpected rds expiry request %s %s", req.Method, req.URL) + } + status, contentType, payload := p.rdsExpiryResponse(req.Host, op, body, values) + return &http.Response{StatusCode: status, ProtoMajor: 1, ProtoMinor: 1, + Header: http.Header{"Content-Type": []string{contentType}}, + Body: io.NopCloser(strings.NewReader(payload)), ContentLength: int64(len(payload))} +} + +func (p *completenessProxy) rdsExpiryResponse(host, op string, body []byte, values url.Values) (int, string, string) { + s := rdsExpiryCase(p.details) + switch { + case host == "ce.us-east-1.amazonaws.com" && op == "AWSInsightsIndexService.GetReservationPurchaseRecommendation": + var request map[string]any + require.NoError(p.t, json.Unmarshal(body, &request)) + require.Equal(p.t, "Amazon Relational Database Service", request["Service"]) + details := make([]map[string]any, 0, len(s.recs)) + for _, rec := range s.recs { + details = append(details, map[string]any{ + "AccountId": rdsExpiryAccountIDs[rec.account], + "RecommendedNumberOfInstancesToPurchase": strconv.Itoa(rec.count), "AverageNumberOfInstancesUsedPerHour": "1", + "UpfrontCost": strconv.Itoa(rec.count * 100), "EstimatedMonthlyOnDemandCost": strconv.Itoa(rec.count * 200), + "EstimatedMonthlySavingsAmount": strconv.Itoa(rec.count * 100), "RecurringStandardMonthlyCost": strconv.Itoa(rec.count * 10), + "InstanceDetails": map[string]any{"RDSInstanceDetails": map[string]string{ + "InstanceType": rec.class, "Region": "us-east-1", "DatabaseEngine": "MySQL", "DeploymentOption": "Single-AZ", + }}, + }) + } + encoded, err := json.Marshal(map[string]any{"Recommendations": []any{map[string]any{"RecommendationDetails": details}}}) + require.NoError(p.t, err) + return 200, "application/x-amz-json-1.1", string(encoded) + case host == "ce.us-east-1.amazonaws.com" && op == "AWSInsightsIndexService.GetReservationCoverage": + return 200, "application/x-amz-json-1.1", p.rdsExpiryCoverage(s, body) + case host == "organizations.us-east-1.amazonaws.com" && op == "AWSOrganizationsV20161128.DescribeAccount": + var request map[string]string + require.NoError(p.t, json.Unmarshal(body, &request)) + require.Contains(p.t, rdsExpiryAccountIDs, request["AccountId"]) + p.expiryAccounts = append(p.expiryAccounts, request["AccountId"]) + return 200, "application/x-amz-json-1.1", fmt.Sprintf(`{"Account":{"Id":%q,"Name":"rds-expiry-account"}}`, request["AccountId"]) + case host == "rds.us-east-1.amazonaws.com" && op == "DescribeReservedDBInstances": + require.Equal(p.t, "2014-10-31", values.Get("Version")) + return 200, "text/xml", rdsExpiryInventory(s) + } + p.t.Errorf("unexpected rds expiry operation %s %s", host, op) + return 403, "text/plain", "fixture rejected operation" +} + +func rdsExpiryInventory(s rdsExpiryScenario) string { + var items strings.Builder + for i, ri := range s.ris { + // A one-year reservation ending endDays from now. + start := time.Now().AddDate(0, 0, ri.endDays-365).UTC().Format(time.RFC3339) + fmt.Fprintf(&items, `rds-expiry-%d%s%s%t%dactive%s31536000`, + i, ri.class, ri.engine, ri.multiAZ, ri.count, start) + } + return `` + + items.String() + `` +} + +func (p *completenessProxy) rdsExpiryCoverage(s rdsExpiryScenario, body []byte) string { + var request struct { + GroupBy []map[string]string + Filter struct { + And []struct { + Dimensions struct { + Key string + Values []string + } + } + } + } + require.NoError(p.t, json.Unmarshal(body, &request)) + filters := make(map[string][]string) + for _, expression := range request.Filter.And { + filters[expression.Dimensions.Key] = expression.Dimensions.Values + } + require.Len(p.t, filters["SERVICE"], 1) + service, engine := filters["SERVICE"][0], "" + key := service + if service == "Amazon Relational Database Service" { + require.Len(p.t, filters["DATABASE_ENGINE"], 1) + engine = filters["DATABASE_ENGINE"][0] + key += "/" + engine + } + p.expiryCoverage = append(p.expiryCoverage, key) + if engine == "" { + return `{"CoveragesByTime":[]}` + } + var groups []any + for _, usage := range s.usage { + if usage.engine != engine { + continue + } + groups = append(groups, map[string]any{ + "Attributes": map[string]string{"instanceType": usage.class, "deploymentOption": "Single-AZ"}, + "Coverage": map[string]any{"CoverageHours": map[string]string{ + "CoverageHoursPercentage": strconv.FormatFloat(usage.pct, 'g', -1, 64), + "TotalRunningHours": strconv.FormatFloat(usage.avg*720, 'g', -1, 64), + }}, + }) + } + if groups == nil { + return `{"CoveragesByTime":[]}` + } + payload, err := json.Marshal(map[string]any{"CoveragesByTime": []any{map[string]any{"Groups": groups}}}) + require.NoError(p.t, err) + return string(payload) +} + +func (p *completenessProxy) assertRDSExpiryRequests(t *testing.T) { + t.Helper() + p.handlers.Wait() + for op, n := range p.requests { + require.NotContains(t, op, "PurchaseReservedDBInstancesOffering", "dry run must not purchase") + t.Logf("rds expiry op %s x%d", op, n) + } + require.Equal(t, 1, p.requests["ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationPurchaseRecommendation"]) + require.Equal(t, 12, p.requests["ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage"]) + require.Positive(t, p.requests["rds.us-east-1.amazonaws.com/DescribeReservedDBInstances"]) +} diff --git a/cmd/recommendation_completeness_proxy_test.go b/cmd/recommendation_completeness_proxy_test.go index d4926dace..8c9359a8a 100644 --- a/cmd/recommendation_completeness_proxy_test.go +++ b/cmd/recommendation_completeness_proxy_test.go @@ -62,7 +62,7 @@ func newCompletenessProxy(t *testing.T, dir, regions, details, service string) ( KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, } - if service == "ec2" { + if service == "ec2" || service == rdsExpiryService { template.DNSNames = append(template.DNSNames, "organizations.us-east-1.amazonaws.com") } der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) @@ -84,7 +84,7 @@ func (p *completenessProxy) serveConnect(w http.ResponseWriter, r *http.Request) p.handlers.Add(1) defer p.handlers.Done() allowed := r.Host == "ce.us-east-1.amazonaws.com:443" || r.Host == "ec2.us-east-1.amazonaws.com:443" || r.Host == "rds.us-east-1.amazonaws.com:443" || r.Host == "savingsplans.amazonaws.com:443" - allowed = allowed || (p.service == "ec2" && r.Host == "organizations.us-east-1.amazonaws.com:443") + allowed = allowed || ((p.service == "ec2" || p.service == rdsExpiryService) && r.Host == "organizations.us-east-1.amazonaws.com:443") if r.Method != http.MethodConnect || !allowed { p.t.Errorf("unexpected CONNECT %s %s", r.Method, r.Host) http.Error(w, "forbidden", http.StatusForbidden) @@ -142,6 +142,9 @@ func (p *completenessProxy) respond(req *http.Request) *http.Response { if p.service == "ec2" { return p.respondReservationExpiry(req, body) } + if p.service == rdsExpiryService { + return p.respondRDSExpiry(req, body) + } op := req.Header.Get("X-Amz-Target") if req.Host == "savingsplans.amazonaws.com" && op == "" { op = strings.TrimPrefix(req.URL.Path, "/") @@ -253,6 +256,10 @@ func (p *completenessProxy) assertRequests(t *testing.T) { p.assertReservationExpiryRequests(t) return } + if p.service == rdsExpiryService { + p.assertRDSExpiryRequests(t) + return + } if p.service != "rds" { p.assertSPRequests(t) return diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index 6ca2ccebb..1f3b8a6c2 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -76,6 +76,8 @@ func runCompletenessScenario(t *testing.T, service, regionMode, details string) switch service { case "ec2": assertReservationExpiryCSV(t, output, details, logs) + case rdsExpiryService: + assertRDSExpiryCSV(t, output, details, logs) case "rds": assertCompletenessDiagnostics(t, regionMode, details, stdout.String(), stderr.String()) assertCompletenessCSV(t, output, details, logs) @@ -94,7 +96,11 @@ func runCompletenessCommandChild(t *testing.T) { transport.TLSClientConfig = &tls.Config{RootCAs: roots, MinVersion: tls.VersionTLS12} http.DefaultTransport = transport t.Cleanup(transport.CloseIdleConnections) - args := []string{"--services", os.Getenv("CUDLY_COMPLETENESS_SERVICE"), "--term", "1", "--include-extended-support", "--coverage", "100", + services := os.Getenv("CUDLY_COMPLETENESS_SERVICE") + if services == rdsExpiryService { + services = "rds" + } + args := []string{"--services", services, "--term", "1", "--include-extended-support", "--coverage", "100", "--output", os.Getenv("CUDLY_COMPLETENESS_OUTPUT"), "--audit-log", os.Getenv("CUDLY_COMPLETENESS_AUDIT")} if os.Getenv("CUDLY_COMPLETENESS_REGIONS") == "explicit" { args = append(args, "--regions", "us-east-1") @@ -102,6 +108,9 @@ func runCompletenessCommandChild(t *testing.T) { if os.Getenv("CUDLY_COMPLETENESS_SERVICE") == "ec2" { args = append(args, reservationExpiryArgs(os.Getenv("CUDLY_COMPLETENESS_DETAILS"))...) } + if os.Getenv("CUDLY_COMPLETENESS_SERVICE") == rdsExpiryService { + args = append(args, rdsExpiryArgs(os.Getenv("CUDLY_COMPLETENESS_DETAILS"))...) + } rootCmd.SetArgs(args) require.NoError(t, rootCmd.Execute()) } From f4eb6a01f2977191d5d4b990783857ba03b26793 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 22:05:15 +0200 Subject: [PATCH 2/2] fix(rds): credit replacement RIs in family expiry sizing (#2132 gate F1) Bump cloud-commitments-go providers/aws and pkg to 218912bb (go#347) and call ApplyFamilyNUSizingRDSWithExpiryOptions with CoverageLookbackDays set to the same value given to GetRICoverageMap (coverageLookbackDays), so RIs bought inside the lagged coverage window are not bought again on a re-run. The log line now reports the expiring and credited replacement NU instead of claiming a deduction. Adds the gate F1 re-run scenario and an outside-lookback control to TestRDSFamilyExpiryCommand. Adapts the archera client to the opaque insurance.Secret key and its tests to pkg no longer echoing rejected values in decode errors (both come with the pkg bump). Co-Authored-By: Claude Sonnet 5.5 --- cmd/archera_comparison.go | 2 +- cmd/archera_comparison_key_test.go | 5 +++ cmd/archera_comparison_test.go | 9 ++++-- cmd/multi_service.go | 14 +++++--- cmd/multi_service_helpers.go | 21 +++++++----- cmd/rds_family_expiry_command_test.go | 46 +++++++++++++++++++++++++++ go.mod | 4 +-- go.sum | 8 ++--- 8 files changed, 88 insertions(+), 21 deletions(-) diff --git a/cmd/archera_comparison.go b/cmd/archera_comparison.go index a70d35627..047672887 100644 --- a/cmd/archera_comparison.go +++ b/cmd/archera_comparison.go @@ -81,7 +81,7 @@ func runArcheraComparison(cmd *cobra.Command, _ []string) error { // Config and client stay local to this call; never store them in a // package variable or struct. - client, err := insurance.NewClient(insurance.Config{APIKey: key, OrgID: orgID}, archeraHTTPClient) + client, err := insurance.NewClient(insurance.Config{APIKey: insurance.NewSecret(key), OrgID: orgID}, archeraHTTPClient) if err != nil { return err } diff --git a/cmd/archera_comparison_key_test.go b/cmd/archera_comparison_key_test.go index 586af8194..6aee40537 100644 --- a/cmd/archera_comparison_key_test.go +++ b/cmd/archera_comparison_key_test.go @@ -145,6 +145,11 @@ func TestArcheraLongKeyVisibleDisguisesAreWithheld(t *testing.T) { } for name, echo := range echoes { forArcheraErrorPaths(t, echo, []int{0}, func(path, got string) { + if path == "decode" { + // pkg/insurance no longer echoes vendor values in decode errors. + assert.Empty(t, archeraReadableKeyFragment(got), "%s %s: %q", path, name, got) + return + } assert.Contains(t, got, archeraWithheld, "%s %s: %q", path, name, got) }) } diff --git a/cmd/archera_comparison_test.go b/cmd/archera_comparison_test.go index d8666eebf..01a141627 100644 --- a/cmd/archera_comparison_test.go +++ b/cmd/archera_comparison_test.go @@ -497,7 +497,8 @@ func TestArcheraErrorsMaskObfuscatedKey(t *testing.T) { out, errOut, err := runArchera(t, fullEnv()) require.Error(t, err) assert.NotContains(t, out+errOut+err.Error(), k) - assert.Contains(t, err.Error(), "[redacted]") + // pkg/insurance no longer echoes the rejected value at all. + assert.Contains(t, err.Error(), "not shown") }) t.Run("plain key as enum value", func(t *testing.T) { body := strings.Replace(archeraComparisonFixture(), `"payment_option":"no_upfront"`, `"payment_option":"`+k+`"`, 1) @@ -567,7 +568,11 @@ func TestArcheraErrorsWithholdKeyHiddenBehindSurvivingCharacters(t *testing.T) { out, errOut, err := runArchera(t, fullEnv()) require.Error(t, err) assert.NotContains(t, out+errOut, k) - checkMsg(t, err.Error()) + // pkg/insurance no longer echoes the rejected value, so there is + // nothing to withhold; the key must still not appear. + assert.NotContains(t, err.Error(), k) + assertArcheraNoControlChars(t, err.Error()) + assert.LessOrEqual(t, len(err.Error()), archeraMaxErrorSize) }) for kind, wrapped := range map[string]error{ "transport": errors.New(msg), diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 33b46cfc7..2085869b6 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -51,10 +51,7 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro // not subject to the fail-closed behavior below. return nil, nil } - lookbackDays := cfg.CoverageLookbackDays - if lookbackDays <= 0 { - lookbackDays = 30 - } + lookbackDays := coverageLookbackDays(cfg) regions := cfg.Regions if len(regions) == 0 { allRegions, err := getAllAWSRegions(ctx, awsCfg) @@ -1059,3 +1056,12 @@ func exitOnExclusionError(err error) { log.Fatalf("Cannot apply extended-support exclusion: %v", err) } } + +// coverageLookbackDays is the Cost Explorer coverage lookback: the one value +// given to GetRICoverageMap and to the RDS family sizing, which must agree. +func coverageLookbackDays(cfg Config) int { + if cfg.CoverageLookbackDays <= 0 { + return 30 + } + return cfg.CoverageLookbackDays +} diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index b767abc56..19b69e469 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -620,9 +620,10 @@ func applyRegionFilters( // 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 -// ApplyFamilyNUSizingRDSWithExpiry, which deducts expiring normalized units -// from the family's coverage; this deliberately raises RDS counts and costs -// when RIs are about to expire (cli#2132). Callers passing nil commitments +// ApplyFamilyNUSizingRDSWithExpiryOptions, which deducts expiring normalized units +// from the family's coverage and credits replacement RIs bought inside the +// coverage lookback (so a re-run does not buy them again); this deliberately +// raises RDS counts and costs when RIs are about to expire (cli#2132). Callers passing nil commitments // get the plain ApplyFamilyNUSizingRDS behavior. // // drops accumulates per-reason drop counts for the end-of-run summary. @@ -649,9 +650,11 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag if cfg.TargetCoverage > 0 { var familyDrops recommendations.FamilyDropCounts var expiry recommendations.FamilyExpiryReport - sizedRDS, rest, familyDrops, expiry = recommendations.ApplyFamilyNUSizingRDSWithExpiry( - recs, coverageMap, cfg.TargetCoverage, expiringCommitments, cfg.RebuyWindowDays) - logFamilyExpiry(expiry, cfg.RebuyWindowDays) + lookbackDays := coverageLookbackDays(cfg) + sizedRDS, rest, familyDrops, expiry = recommendations.ApplyFamilyNUSizingRDSWithExpiryOptions( + recs, coverageMap, cfg.TargetCoverage, expiringCommitments, + recommendations.FamilyExpiryOptions{WindowDays: cfg.RebuyWindowDays, CoverageLookbackDays: lookbackDays}) + logFamilyExpiry(expiry, cfg.RebuyWindowDays, lookbackDays) drops.Add(common.DropFamilyAlreadyAtTarget, familyDrops.AlreadyAtTarget) drops.Add(common.DropFamilyNoNUSignal, familyDrops.NoNUSignal) drops.Add(common.DropFamilySizedToZero, familyDrops.SizedToZero) @@ -677,9 +680,11 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag // family-NU sizing. Unusable RIs (unparsable instance class, or an engine // spelled differently from the coverage report) were NOT deducted, so they are // warned about rather than read as "no expiry". -func logFamilyExpiry(expiry recommendations.FamilyExpiryReport, windowDays int) { +func logFamilyExpiry(expiry recommendations.FamilyExpiryReport, windowDays, lookbackDays int) { if expiry.ExpiringNU > 0 { - AppLogger.Printf(" ⏰ RDS family sizing: deducting %.0f normalized units of RIs expiring within %d days from family coverage\n", expiry.ExpiringNU, windowDays) + // The exact deduction is capped by the covered and active NU per family + // (go#342); report the inputs rather than claim a net figure. + AppLogger.Printf(" ⏰ RDS family sizing: %.0f normalized units of RIs expiring within %d days considered against family coverage; %.0f units of replacement RIs started in the last %d days credited back\n", expiry.ExpiringNU, windowDays, expiry.ReplacementNU, lookbackDays) } if expiry.Unusable > 0 { AppLogger.Printf(" ⚠️ %d RDS reserved instances expiring within %d days could not be applied to family sizing (unparsable instance class or no matching coverage family); their expiry was NOT deducted\n", expiry.Unusable, windowDays) diff --git a/cmd/rds_family_expiry_command_test.go b/cmd/rds_family_expiry_command_test.go index 88c946834..77ef1d4cc 100644 --- a/cmd/rds_family_expiry_command_test.go +++ b/cmd/rds_family_expiry_command_test.go @@ -42,6 +42,8 @@ type rdsExpiryRI struct { multiAZ bool count int endDays int + // startedDaysAgo overrides the derived start (one year before endDays). + startedDaysAgo int } // rdsExpiryRec is one Cost Explorer purchase recommendation row (one linked @@ -64,6 +66,8 @@ type rdsExpiryScenario struct { want []int existing float64 unusable int + // logLine, when set, must appear in the run log. + logLine string // parent documents the count the commit before the fix produced. parent []int } @@ -162,6 +166,36 @@ func rdsExpiryCase(name string) rdsExpiryScenario { ris: []rdsExpiryRI{{class: "db.r6g.large", engine: "postgres", count: 16, endDays: 15}}, recs: one("db.r6g.large", 8), want: []int{8}, existing: 40, parent: []int{8}, } + case "rerun-replacement-bought": + // Re-run after the CLI already bought the replacements (cli#2175 gate F1). + // Lagged coverage is 82.5% (66 NU): 12 large owned by another account (not + // in the inventory), the 4 expiring, and part of the 4 replacements bought + // 2 days ago (outside the 24h idempotency window). True post-expiry + // coverage is 12 + 4 replacements = 16 large = 64 NU = 80%, so nothing is + // bought. Before the lookback-aware call the lagged coverage lost the 16 + // expiring NU and 3 large were bought again. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 82.5)}, + ris: []rdsExpiryRI{ + ri("db.r6g.large", 4, 15), + {class: "db.r6g.large", engine: "mysql", count: 4, endDays: 363, startedDaysAgo: 2}, + }, + recs: one("db.r6g.large", 8), want: []int{0}, parent: []int{3}, + logLine: "16 normalized units of RIs expiring within 30 days considered against family coverage; 16 units of replacement RIs started in the last 30 days credited back", + } + case "control-replacement-outside-lookback": + // Same inventory but the replacements started 45 days ago, outside the + // 30-day coverage lookback, so they are already inside the coverage + // figure and must not be credited again: 66 - 16 = 50 NU (62.5%), the + // 17.5% gap (14 NU) buys 3 large. + return rdsExpiryScenario{ + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 82.5)}, + ris: []rdsExpiryRI{ + ri("db.r6g.large", 4, 15), + {class: "db.r6g.large", engine: "mysql", count: 4, endDays: 320, startedDaysAgo: 45}, + }, + recs: one("db.r6g.large", 8), want: []int{3}, existing: 62.5, parent: []int{3}, + } } panic("unknown rds expiry scenario: " + name) } @@ -170,6 +204,7 @@ func TestRDSFamilyExpiryCommand(t *testing.T) { for _, name := range []string{ "already-at-target", "below-target", "large-ri-xlarge-usage", "multi-size-linked-accounts", "partial-inventory", "unusable-expiring-warned", "control-outside-window", "control-engine-isolation", + "rerun-replacement-bought", "control-replacement-outside-lookback", } { t.Run(name, func(t *testing.T) { runCompletenessScenario(t, rdsExpiryService, "explicit", name) }) } @@ -182,7 +217,15 @@ func rdsExpiryArgs(string) []string { func assertRDSExpiryCSV(t *testing.T, output, name, logs string) { t.Helper() s := rdsExpiryCase(name) + if s.logLine != "" { + require.Contains(t, logs, s.logLine) + } data, err := os.ReadFile(output) + if countNonZero(s.want) == 0 { + require.ErrorIs(t, err, os.ErrNotExist, "nothing to buy must write no CSV; logs=%s", logs) + require.Contains(t, logs, "0 recommendations selected", logs) + return + } require.NoError(t, err, logs) rows, err := csv.NewReader(bytes.NewReader(data)).ReadAll() require.NoError(t, err) @@ -313,6 +356,9 @@ func rdsExpiryInventory(s rdsExpiryScenario) string { for i, ri := range s.ris { // A one-year reservation ending endDays from now. start := time.Now().AddDate(0, 0, ri.endDays-365).UTC().Format(time.RFC3339) + if ri.startedDaysAgo > 0 { + start = time.Now().AddDate(0, 0, -ri.startedDaysAgo).UTC().Format(time.RFC3339) + } fmt.Fprintf(&items, `rds-expiry-%d%s%s%t%dactive%s31536000`, i, ri.class, ri.engine, ri.multiAZ, ri.count, start) } diff --git a/go.mod b/go.mod index 377a8594a..8389b9b41 100644 --- a/go.mod +++ b/go.mod @@ -82,8 +82,8 @@ 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-20261009181525-30bf383795db - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012180926-2c1e87b723ab + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012195142-218912bb8cb9 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012195142-218912bb8cb9 github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007133317-58c25f04c49b github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261006205158-7ff8c1aee1bb github.com/aws/aws-sdk-go-v2/service/organizations v1.45.3 diff --git a/go.sum b/go.sum index ee996a239..f857c05f1 100644 --- a/go.sum +++ b/go.sum @@ -78,10 +78,10 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= 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-20261009181525-30bf383795db h1:d/eTZP31KewqE049NlQM3nZIUygRudHJ25Im2ibJk58= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261009181525-30bf383795db/go.mod h1:8RTmzPazA8IeVZfn7wNaU7ILx0VK5eHZQozO21Mvtq8= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012180926-2c1e87b723ab h1:E6cZ5WllhWGR8EgONN8MwWaGD3qLCCwYIXWgIysnfYU= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012180926-2c1e87b723ab/go.mod h1:C0ildZeRds0aRvDZp49MLY0kFWh/4FiM4lPWWWd3HFA= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012195142-218912bb8cb9 h1:b1AVTF1yVwPgWo9/6SlNVO460EMqWTsLZ2BSKcrwfjA= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012195142-218912bb8cb9/go.mod h1:8RTmzPazA8IeVZfn7wNaU7ILx0VK5eHZQozO21Mvtq8= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012195142-218912bb8cb9 h1:We82Ai0sL37/PTC14qIJub8B+d06qWHFnIco5XhVX2U= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012195142-218912bb8cb9/go.mod h1:fiu6WQmUGRdViiOf45QV235Cowq8j0yn8ahGWCGEFn8= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007133317-58c25f04c49b h1:yurugmfdZjf+dqjfqfDyfoBR3IAiFUpMKQ8URnZnJzQ= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007133317-58c25f04c49b/go.mod h1:iIvPuWLWd+SXqLHAQNdodYMUAAcq7PnwfScXWgARt5A= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261006205158-7ff8c1aee1bb h1:AJ4nKDtttjiTDemo9PDp2esoc35ymVaiCSr0zIewwpY=