From 4f545584dd1a56883cb531015875528bec60cbac Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 03:06:08 +0200 Subject: [PATCH 1/2] fix(cli): re-derive count-linear projections after Count changes ApplyInstanceLimit, ApplyCountOverride and the duplicate-RI dedupe changed Count and rescaled the money fields but left ProjectedCoverage and ProjectedUtilization at the values target-coverage sizing wrote for the old Count (target-coverage 80, Count 100 capped to 10 still reported 80% coverage; the correct figure is 8%). The Redshift NumberOfNodes mirrored from Count at parse also stayed stale after any sizing or cap. Add refreshCountLinear (mirrors the shared sizer's projection formulas, zero projections stay zero, Savings Plans and rows without a finite positive usage signal are untouched, RDS ProjectedCoverage is a family aggregate and is left alone) and syncDataWarehouseNodes (copy-on-write, only when the provider mirrors Count), wire them into the three Count-changing paths and the three shared-sizing wrappers. resizeAfterExclusion is not wired: it runs before sizing, when the projections are still zero. Refs #1845 Co-Authored-By: Claude Opus 5.5 --- cmd/helpers.go | 96 ++++++++++++- cmd/helpers_count_linear_test.go | 237 +++++++++++++++++++++++++++++++ cmd/helpers_count_override.go | 6 +- cmd/multi_service_helpers.go | 3 +- 4 files changed, 335 insertions(+), 7 deletions(-) create mode 100644 cmd/helpers_count_linear_test.go diff --git a/cmd/helpers.go b/cmd/helpers.go index 301c93d65..20545ab40 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "log" + "math" "os" "strings" "sync" @@ -137,20 +138,20 @@ func CalculateTotalInstances(recs []common.Recommendation) int { // ApplyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the // logging sink. Substantive documentation lives on recfilter.ApplyCoverage. func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation { - return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, nil) + return mapRecs(recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, nil), syncDataWarehouseNodes) } // applyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the // logging sink. Substantive documentation lives on recfilter.ApplyCoverage. func applyCoverage(recs []common.Recommendation, coverage float64, drops *common.DropSummary) []common.Recommendation { - return recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, drops) + return mapRecs(recfilter.ApplyCoverage(recs, coverage, AppLogger.Printf, drops), syncDataWarehouseNodes) } // ApplyTargetCoverage delegates to recfilter.ApplyTargetCoverage, wiring // AppLogger as the logging sink. Substantive documentation (the RI/SP sizing // formulas and the #338 flag-name history) lives on recfilter.ApplyTargetCoverage. func ApplyTargetCoverage(recs []common.Recommendation, targetPct float64, drops *common.DropSummary) []common.Recommendation { - return recfilter.ApplyTargetCoverage(recs, targetPct, AppLogger.Printf, drops) + return mapRecs(recfilter.ApplyTargetCoverage(recs, targetPct, AppLogger.Printf, drops), syncDataWarehouseNodes) } // applySizing chooses target-coverage or coverage sizing. @@ -187,7 +188,9 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro // alone leaves the row claiming the savings of instances the run will not // buy. The run summary and the purchase report then overstate the benefit // of a capped run, which is the wrong direction to be wrong in on a money -// path (#1830). +// path (#1830). The count-linear fields that are not money (ProjectedCoverage, +// ProjectedUtilization, Redshift NumberOfNodes) are re-derived from the new +// Count by refreshCountLinear (#1845). func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []common.Recommendation { if maxInstances <= 0 { return recs @@ -210,6 +213,7 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm if rec.Count > remaining { adjusted = common.ScaleRecommendationCosts(rec, float64(remaining)/float64(rec.Count)) adjusted.Count = remaining + adjusted = refreshCountLinear(adjusted) } result = append(result, adjusted) // Only a positive Count consumes budget. Subtracting a non-positive @@ -222,6 +226,90 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm return result } +// refreshCountLinear re-derives the count-linear, non-money fields after a +// caller outside the shared sizing code changed rec.Count (#1845): the +// projections the target-coverage sizer wrote, and the Redshift node count +// mirrored from Count at parse. It is idempotent on a row whose Count did not +// change, so callers may apply it to every row of a slice. +func refreshCountLinear(rec common.Recommendation) common.Recommendation { + return syncDataWarehouseNodes(rederiveProjections(rec)) +} + +// mapRecs applies fn to every rec into a new slice, keeping a nil input nil. +func mapRecs(recs []common.Recommendation, fn func(common.Recommendation) common.Recommendation) []common.Recommendation { + if recs == nil { + return nil + } + out := make([]common.Recommendation, len(recs)) + for i := range recs { + out[i] = fn(recs[i]) + } + return out +} + +// rederiveProjections mirrors the projection formulas of the shared sizer +// (recfilter applyTargetCoverageRI) for the rec's current Count. A projection +// that is zero ("not computed") stays zero. Savings Plans, rows without a +// usage signal (zero, negative, NaN or infinite avg) and rows without a positive +// Count are returned unchanged. +// RDS ProjectedCoverage is a family aggregate set by family-NU sizing, which a +// per-row formula would corrupt, so only RDS ProjectedUtilization is refreshed. +func rederiveProjections(rec common.Recommendation) common.Recommendation { + avg := rec.AverageInstancesUsedPerHour + if common.IsSavingsPlan(rec.Service) || rec.Count <= 0 || !(avg > 0) || math.IsInf(avg, 1) { + return rec + } + n := float64(rec.Count) + if rec.ProjectedUtilization != 0 { + rec.ProjectedUtilization = math.Min(100, avg/n*100) + } + if rec.ProjectedCoverage != 0 && rec.ExistingCoverageKnown && rec.Service != common.ServiceRDS { + rec.ProjectedCoverage = math.Min(100, rec.ExistingCoveragePct+n/avg*100) + } + return rec +} + +// syncDataWarehouseNodes re-syncs a mirrored NumberOfNodes (and the +// single-node/multi-node ClusterType derived from it) with rec.Count. The +// details are copied, never written through the shared pointer. A zero +// NumberOfNodes means the provider does not mirror Count and stays zero. +func syncDataWarehouseNodes(rec common.Recommendation) common.Recommendation { + if rec.Count <= 0 { + return rec + } + d, isPtr, ok := dataWarehouseDetails(rec.Details) + if !ok || d.NumberOfNodes <= 0 || d.NumberOfNodes == rec.Count { + return rec + } + d.NumberOfNodes = rec.Count + if d.ClusterType == "single-node" || d.ClusterType == "multi-node" { + d.ClusterType = "multi-node" + if rec.Count == 1 { + d.ClusterType = "single-node" + } + } + if isPtr { + rec.Details = &d + } else { + rec.Details = d + } + return rec +} + +// dataWarehouseDetails returns a copy of a pointer or value DataWarehouseDetails. +func dataWarehouseDetails(d common.ServiceDetails) (cp common.DataWarehouseDetails, isPtr, ok bool) { + switch v := d.(type) { + case *common.DataWarehouseDetails: + if v == nil { + return cp, false, false + } + return *v, true, true + case common.DataWarehouseDetails: + return v, false, true + } + return cp, false, false +} + // ConfirmPurchase requires terminal input; totalSavings is monthly savings, not purchase cost. func ConfirmPurchase(totalInstances int, totalSavings float64) bool { if !term.IsTerminal(int(os.Stdin.Fd())) { //nolint:gosec // G115: uintptr->int for file descriptor; FD values are always small positive integers diff --git a/cmd/helpers_count_linear_test.go b/cmd/helpers_count_linear_test.go new file mode 100644 index 000000000..e7f649ae2 --- /dev/null +++ b/cmd/helpers_count_linear_test.go @@ -0,0 +1,237 @@ +package main + +import ( + "context" + "math" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func riRec(count int, avg, existing, cov, util float64) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceEC2, CommitmentType: common.CommitmentReservedInstance, Region: "us-east-1", ResourceType: "m5.large", + Count: count, AverageInstancesUsedPerHour: avg, ExistingCoveragePct: existing, + ExistingCoverageKnown: true, ProjectedCoverage: cov, ProjectedUtilization: util, + EstimatedSavings: 1000, CommitmentCost: 2000, OnDemandCost: 3000, + } +} + +func TestRederiveProjections(t *testing.T) { + rds := riRec(10, 125, 10, 55, 100) + rds.Service = common.ServiceRDS + noKnown := riRec(10, 125, 0, 80, 100) + noKnown.ExistingCoverageKnown = false + sp := riRec(10, 125, 0, 0, 90) + sp.Service = common.ServiceSavingsPlansCompute + + tests := []struct { + name string + rec common.Recommendation + cov, util float64 + }{ + {"issue scenario: cap 100 -> 10", riRec(10, 125, 0, 80, 100), 8.0, 100}, + {"unclamped util, clamped cov (hand-computed)", riRec(10, 4, 0, 50, 100), 100, 40}, + {"unclamped cov with existing (hand-computed)", riRec(10, 125, 10, 80, 100), 18.0, 100}, + {"zero projections stay zero", riRec(10, 125, 10, 0, 0), 0, 0}, + {"only util computed stays so", riRec(10, 4, 0, 0, 100), 0, 40}, + {"avg zero untouched", riRec(10, 0, 0, 80, 100), 80, 100}, + {"count zero untouched", riRec(0, 125, 0, 80, 100), 80, 100}, + {"existing coverage unknown: cov untouched", noKnown, 80, 100}, + {"RDS: cov untouched (family aggregate), util refreshed", rds, 55, 100}, + {"SP untouched", sp, 0, 90}, + {"NaN avg untouched", riRec(10, math.NaN(), 0, 80, 100), 80, 100}, + {"+Inf avg untouched", riRec(10, math.Inf(1), 0, 80, 100), 80, 100}, + {"negative avg untouched", riRec(10, -3, 0, 80, 100), 80, 100}, + {"negative count untouched", riRec(-5, 125, 0, 80, 100), 80, 100}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := rederiveProjections(tt.rec) + assert.InDelta(t, tt.cov, got.ProjectedCoverage, 1e-9) + assert.InDelta(t, tt.util, got.ProjectedUtilization, 1e-9) + }) + } +} + +func TestSyncDataWarehouseNodes(t *testing.T) { + ptr := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"} + rec := common.Recommendation{Service: common.ServiceRedshift, Count: 3, Details: ptr} + + got := syncDataWarehouseNodes(rec) + assert.Equal(t, 3, got.Details.(*common.DataWarehouseDetails).NumberOfNodes) + assert.Equal(t, 10, ptr.NumberOfNodes, "the shared pointer must not be written through") + + rec.Count = 1 + assert.Equal(t, "single-node", syncDataWarehouseNodes(rec).Details.(*common.DataWarehouseDetails).ClusterType) + + val := common.Recommendation{Count: 2, Details: common.DataWarehouseDetails{NumberOfNodes: 1, ClusterType: "single-node"}} + d := syncDataWarehouseNodes(val).Details.(common.DataWarehouseDetails) + assert.Equal(t, 2, d.NumberOfNodes) + assert.Equal(t, "multi-node", d.ClusterType) + + azure := common.Recommendation{Count: 4, Details: common.DataWarehouseDetails{ClusterType: "dedicated-sql-pool"}} + assert.Equal(t, azure, syncDataWarehouseNodes(azure), "NumberOfNodes 0 means not mirrored") + custom := common.Recommendation{Count: 4, Details: common.DataWarehouseDetails{NumberOfNodes: 9, ClusterType: "dedicated-sql-pool"}} + assert.Equal(t, "dedicated-sql-pool", syncDataWarehouseNodes(custom).Details.(common.DataWarehouseDetails).ClusterType) + + same := common.Recommendation{Count: 10, Details: ptr} + assert.Same(t, ptr, syncDataWarehouseNodes(same).Details, "equal values keep pointer identity") + var nilPtr *common.DataWarehouseDetails + assert.NotPanics(t, func() { syncDataWarehouseNodes(common.Recommendation{Count: 2, Details: nilPtr}) }) + zero := common.Recommendation{Count: 0, Details: ptr} + assert.Same(t, ptr, syncDataWarehouseNodes(zero).Details) + other := common.Recommendation{Count: 2, Details: &common.DatabaseDetails{Engine: "mysql"}} + assert.Equal(t, other, syncDataWarehouseNodes(other)) +} + +// The coverage mirror must match what the shared sizer writes. +func TestRefreshCountLinearMatchesSizerOutput(t *testing.T) { + in := make([]common.Recommendation, 0, 4) + for _, p := range []struct{ avg, existing float64 }{{125, 0}, {40.5, 30}, {400, 79.5}, {33.3, 10}} { + in = append(in, riRec(50, p.avg, p.existing, 0, 0)) + } + out := ApplyTargetCoverage(in, 80, nil) + require.Len(t, out, len(in)) + for i := range out { + require.NotZero(t, out[i].ProjectedCoverage) + assert.Equal(t, out[i], refreshCountLinear(out[i]), "pool %d drifted", i) + } +} + +// Issue #1845: the cap changes Count, so the projections must follow. +func TestApplyInstanceLimitRederivesProjections(t *testing.T) { + capped := riRec(100, 125, 0, 80, 100) + untouched := riRec(5, 10, 0, 40, 100) + untouched.ResourceType = "c5.large" + zero := riRec(100, 125, 0, 0, 0) + zero.ResourceType = "r5.large" + got := ApplyInstanceLimit([]common.Recommendation{untouched, capped, zero}, 10) + require.Len(t, got, 2) + assert.Equal(t, untouched, got[0], "a row the cap does not truncate is byte-identical") + assert.Equal(t, 5, got[1].Count) + // remaining budget 5 after the first row: cov = 5/125*100 = 4.0 + assert.InDelta(t, 4.0, got[1].ProjectedCoverage, 1e-9) + + got = ApplyInstanceLimit([]common.Recommendation{capped}, 10) + assert.InDelta(t, 8.0, got[0].ProjectedCoverage, 1e-9) + assert.InDelta(t, 100, got[0].ProjectedUtilization, 1e-9) + + got = ApplyInstanceLimit([]common.Recommendation{zero}, 10) + assert.Zero(t, got[0].ProjectedCoverage) + assert.Zero(t, got[0].ProjectedUtilization) + + noSignal := riRec(100, 0, 0, 80, 100) + assert.Equal(t, 80.0, ApplyInstanceLimit([]common.Recommendation{noSignal}, 10)[0].ProjectedCoverage) +} + +func TestApplyInstanceLimitSyncsRedshiftNodes(t *testing.T) { + src := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"} + rec := common.Recommendation{Service: common.ServiceRedshift, Count: 10, Details: src} + got := ApplyInstanceLimit([]common.Recommendation{rec}, 3) + assert.Equal(t, 3, got[0].Details.(*common.DataWarehouseDetails).NumberOfNodes) + assert.Equal(t, 10, src.NumberOfNodes) +} + +func TestApplyCountOverrideRederivesProjections(t *testing.T) { + got := ApplyCountOverride([]common.Recommendation{riRec(100, 125, 0, 80, 100)}, 200) + assert.InDelta(t, 100, got[0].ProjectedCoverage, 1e-9) // 200/125*100 clamps + assert.InDelta(t, 62.5, got[0].ProjectedUtilization, 1e-9) + + rds := riRec(100, 125, 0, 55, 100) + rds.Service = common.ServiceRDS + got = ApplyCountOverride([]common.Recommendation{rds}, 10) + assert.Equal(t, 55.0, got[0].ProjectedCoverage, "RDS family coverage survives") +} + +func TestAdjustRecsForDuplicatesRederivesProjections(t *testing.T) { + ctx := context.Background() + client := &MockServiceClient{} + client.On("GetExistingCommitments", ctx).Return([]common.Commitment{ + {ResourceType: "m5.large", Region: "us-east-1", Count: 60, State: "active", StartDate: time.Now().Add(-time.Hour)}, + }, nil) + got, err := adjustRecsForDuplicates(ctx, []common.Recommendation{riRec(100, 125, 0, 80, 100)}, client) + require.NoError(t, err) + require.Len(t, got, 1) + require.Equal(t, 40, got[0].Count) + assert.InDelta(t, 32.0, got[0].ProjectedCoverage, 1e-9) // 40/125*100 +} + +// Fixture-based: sizer output flows through the run-wide cap unchanged in shape. +func TestTargetCoverageThenGlobalCapAgree(t *testing.T) { + sized := ApplyTargetCoverage([]common.Recommendation{riRec(300, 125, 0, 0, 0)}, 80, nil) + require.Len(t, sized, 1) + require.Equal(t, 100, sized[0].Count) + require.InDelta(t, 80.0, sized[0].ProjectedCoverage, 1e-9) + got := applyGlobalInstanceLimit(sized, Config{MaxInstances: 10}, rankBySavingsPercentage, common.NewDropSummary()) + require.Len(t, got, 1) + assert.Equal(t, 10, got[0].Count) + assert.InDelta(t, 8.0, got[0].ProjectedCoverage, 1e-9) +} + +func TestTargetCoverageSyncsRedshiftNodes(t *testing.T) { + src := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"} + rec := riRec(10, 5, 0, 0, 0) + rec.Service = common.ServiceRedshift + rec.Details = src + got := ApplyTargetCoverage([]common.Recommendation{rec}, 80, nil) + require.Len(t, got, 1) + assert.Equal(t, 4, got[0].Count) + assert.Equal(t, 4, got[0].Details.(*common.DataWarehouseDetails).NumberOfNodes) + assert.Equal(t, 10, src.NumberOfNodes) +} + +func rdsFamilyRec(count int, avg, cov, util float64) common.Recommendation { + r := riRec(count, avg, 0, cov, util) + r.Service = common.ServiceRDS + return r +} + +func TestRDSCoverageSurvivesCapAndDedupe(t *testing.T) { + got := ApplyInstanceLimit([]common.Recommendation{rdsFamilyRec(100, 40, 55, 40)}, 10) + assert.Equal(t, 55.0, got[0].ProjectedCoverage, "family coverage untouched by the cap") + assert.InDelta(t, 100, got[0].ProjectedUtilization, 1e-9) // 40/10 clamps + + ctx := context.Background() + client := &MockServiceClient{} + client.On("GetExistingCommitments", ctx).Return([]common.Commitment{ + {ResourceType: "m5.large", Region: "us-east-1", Count: 60, State: "active", StartDate: time.Now().Add(-time.Hour)}, + }, nil) + dd := checkDuplicates(ctx, []common.Recommendation{rdsFamilyRec(100, 20, 55, 20)}, client, false, common.NewDropSummary()) + require.Len(t, dd, 1) + require.Equal(t, 40, dd[0].Count) + assert.Equal(t, 55.0, dd[0].ProjectedCoverage) + assert.InDelta(t, 50, dd[0].ProjectedUtilization, 1e-9) // 20/40 +} + +func TestCheckDuplicatesRederivesProjections(t *testing.T) { + ctx := context.Background() + client := &MockServiceClient{} + client.On("GetExistingCommitments", ctx).Return([]common.Commitment{ + {ResourceType: "m5.large", Region: "us-east-1", Count: 60, State: "active", StartDate: time.Now().Add(-time.Hour)}, + }, nil) + got := checkDuplicates(ctx, []common.Recommendation{riRec(100, 125, 0, 80, 100)}, client, false, common.NewDropSummary()) + require.Len(t, got, 1) + require.Equal(t, 40, got[0].Count) + assert.InDelta(t, 32.0, got[0].ProjectedCoverage, 1e-9) +} + +func TestApplyCoverageSyncsRedshiftNodes(t *testing.T) { + src := &common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 10, ClusterType: "multi-node"} + rec := riRec(10, 5, 0, 0, 0) + rec.Service = common.ServiceRedshift + rec.Details = src + for name, got := range map[string][]common.Recommendation{ + "ApplyCoverage": ApplyCoverage([]common.Recommendation{rec}, 40), + "applyCoverage": applyCoverage([]common.Recommendation{rec}, 40, common.NewDropSummary()), + } { + require.Len(t, got, 1, name) + assert.Equal(t, 4, got[0].Count, name) + assert.Equal(t, 4, got[0].Details.(*common.DataWarehouseDetails).NumberOfNodes, name) + } + assert.Equal(t, 10, src.NumberOfNodes) + assert.Nil(t, mapRecs(nil, syncDataWarehouseNodes), "nil stays nil") +} diff --git a/cmd/helpers_count_override.go b/cmd/helpers_count_override.go index 3a4d8f5d1..c927cc462 100644 --- a/cmd/helpers_count_override.go +++ b/cmd/helpers_count_override.go @@ -13,8 +13,9 @@ import ( // alone leaves the row claiming the money of a quantity nobody will buy // (#1844). The scaling goes through common.ScaleRecommendationCosts, the same // helper ApplyInstanceLimit and the coverage paths use, so the arithmetic -// cannot drift between the flags. ProjectedCoverage / ProjectedUtilization are -// count-linear but not re-derived here, exactly as after a cap (#1845). +// cannot drift between the flags. ProjectedCoverage / ProjectedUtilization and +// a Redshift NumberOfNodes are count-linear, so refreshCountLinear re-derives +// them from the new Count (#1845). // // Savings Plans are left entirely alone, Count included. The flag is // documented as an override for "all selected RIs", an SP commitment is @@ -55,6 +56,7 @@ func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []com default: result[i] = common.ScaleRecommendationCosts(rec, float64(overrideCount)/float64(rec.Count)) result[i].Count = int(overrideCount) + result[i] = refreshCountLinear(result[i]) if int(overrideCount) > evidencedCount(rec) { extrapolated++ } diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 3a9f8d0b7..853b007f3 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -270,6 +270,7 @@ func adjustRecsForDuplicates(ctx context.Context, recs []common.Recommendation, if err != nil { return recs, err // Return original recommendations with error } + adjustedRecs = mapRecs(adjustedRecs, refreshCountLinear) originalInstances := CalculateTotalInstances(recs) adjustedInstances := CalculateTotalInstances(adjustedRecs) @@ -826,7 +827,7 @@ func checkDuplicates( AppLogger.Printf(" 🔍 Adjusted recommendations: %d instances → %d instances to avoid duplicate purchases\n", originalInstances, adjustedInstances) } drops.Add(common.DropDuplicateDedup, len(dedupedOut)) - filteredRecs = adjustedRecs + filteredRecs = mapRecs(adjustedRecs, refreshCountLinear) } return filteredRecs From b4ecbcc42a2259bc69d31512d2054fb0a6c4028d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 03:47:02 +0200 Subject: [PATCH 2/2] fix(cli): restrict node sync to Redshift recs syncDataWarehouseNodes rewrote NumberOfNodes on any rec carrying DataWarehouseDetails, but only the AWS Redshift parser mirrors Count into that field; other users of the type keep their own value. Gate the sync on the Redshift service. The NumberOfNodes > 0 guard is dropped: the service check covers the Azure case it existed for. Co-Authored-By: Claude Opus 5.5 --- cmd/helpers.go | 13 +++++++------ cmd/helpers_count_linear_test.go | 10 +++++----- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index 20545ab40..a5d809fea 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -269,16 +269,17 @@ func rederiveProjections(rec common.Recommendation) common.Recommendation { return rec } -// syncDataWarehouseNodes re-syncs a mirrored NumberOfNodes (and the -// single-node/multi-node ClusterType derived from it) with rec.Count. The -// details are copied, never written through the shared pointer. A zero -// NumberOfNodes means the provider does not mirror Count and stays zero. +// syncDataWarehouseNodes re-syncs NumberOfNodes (and the single-node/multi-node +// ClusterType derived from it) with rec.Count for AWS Redshift recs, the only +// parser that mirrors Count into NumberOfNodes; other users of +// DataWarehouseDetails keep their value. The details are copied, never written +// through the shared pointer. func syncDataWarehouseNodes(rec common.Recommendation) common.Recommendation { - if rec.Count <= 0 { + if rec.Service != common.ServiceRedshift || rec.Count <= 0 { return rec } d, isPtr, ok := dataWarehouseDetails(rec.Details) - if !ok || d.NumberOfNodes <= 0 || d.NumberOfNodes == rec.Count { + if !ok || d.NumberOfNodes == rec.Count { return rec } d.NumberOfNodes = rec.Count diff --git a/cmd/helpers_count_linear_test.go b/cmd/helpers_count_linear_test.go index e7f649ae2..33172a0ff 100644 --- a/cmd/helpers_count_linear_test.go +++ b/cmd/helpers_count_linear_test.go @@ -68,15 +68,15 @@ func TestSyncDataWarehouseNodes(t *testing.T) { rec.Count = 1 assert.Equal(t, "single-node", syncDataWarehouseNodes(rec).Details.(*common.DataWarehouseDetails).ClusterType) - val := common.Recommendation{Count: 2, Details: common.DataWarehouseDetails{NumberOfNodes: 1, ClusterType: "single-node"}} + val := common.Recommendation{Service: common.ServiceRedshift, Count: 2, Details: common.DataWarehouseDetails{NumberOfNodes: 1, ClusterType: "single-node"}} d := syncDataWarehouseNodes(val).Details.(common.DataWarehouseDetails) assert.Equal(t, 2, d.NumberOfNodes) assert.Equal(t, "multi-node", d.ClusterType) - azure := common.Recommendation{Count: 4, Details: common.DataWarehouseDetails{ClusterType: "dedicated-sql-pool"}} - assert.Equal(t, azure, syncDataWarehouseNodes(azure), "NumberOfNodes 0 means not mirrored") - custom := common.Recommendation{Count: 4, Details: common.DataWarehouseDetails{NumberOfNodes: 9, ClusterType: "dedicated-sql-pool"}} - assert.Equal(t, "dedicated-sql-pool", syncDataWarehouseNodes(custom).Details.(common.DataWarehouseDetails).ClusterType) + azure := common.Recommendation{Service: common.ServiceDataWarehouse, Count: 4, Details: common.DataWarehouseDetails{ClusterType: "dedicated-sql-pool"}} + assert.Equal(t, azure, syncDataWarehouseNodes(azure), "non-Redshift recs are never touched") + custom := common.Recommendation{Service: common.ServiceDataWarehouse, Count: 4, Details: common.DataWarehouseDetails{NumberOfNodes: 9, ClusterType: "single-node"}} + assert.Equal(t, custom, syncDataWarehouseNodes(custom), "non-Redshift NumberOfNodes stays 9") same := common.Recommendation{Count: 10, Details: ptr} assert.Same(t, ptr, syncDataWarehouseNodes(same).Details, "equal values keep pointer identity")