Skip to content

Commit 7a06be7

Browse files
committed
fix(drops): nil-map guard in DropSummary.Add + NoNUSignal category + test coverage (#875 CR)
- DropSummary.Add already had the lazy-init nil-map guard; add TestDropSummary_ZeroValue_Safe regression test to lock the behaviour - Add DropFamilyNoNUSignal ("family-nu-no-nu-signal") constant and FamilyDropCounts.NoNUSignal field for the case where AWS-rec NU sums to zero (unknown/unrecognised sizes) -- distinct from AlreadyAtTarget - Fix sizeRDSFamilyRecs: currentNU<=0 branch now sets NoNUSignal instead of incorrectly reusing AlreadyAtTarget - Propagate NoNUSignal in ApplyFamilyNUSizingRDS aggregation loop and in cmd/multi_service_helpers.go drops recording - Capture drops in all TestApplyFamilyNUSizingRDS subtests; assert AlreadyAtTarget==1 for the "at target" case, SizedToZero==1 for a new "floor(0)" test case, and zero total drops for passing-through cases
1 parent 9aa91a7 commit 7a06be7

5 files changed

Lines changed: 65 additions & 17 deletions

File tree

‎cmd/multi_service_helpers.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,7 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag
509509
var familyDrops recommendations.FamilyDropCounts
510510
sizedRDS, rest, familyDrops = recommendations.ApplyFamilyNUSizingRDS(recs, coverageMap, cfg.TargetCoverage)
511511
drops.Add(common.DropFamilyAlreadyAtTarget, familyDrops.AlreadyAtTarget)
512+
drops.Add(common.DropFamilyNoNUSignal, familyDrops.NoNUSignal)
512513
drops.Add(common.DropFamilySizedToZero, familyDrops.SizedToZero)
513514
}
514515
filteredRecs := applySizing(rest, cfg, cfg.Coverage, drops)

‎pkg/common/drop_summary.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ const (
1414
DropTargetAlreadyMet = "target-already-met"
1515
DropTargetSizedToZero = "target-sized-to-zero"
1616
DropFamilyAlreadyAtTarget = "family-nu-already-at-target"
17-
DropFamilySizedToZero = "family-nu-sized-to-zero"
17+
DropFamilyNoNUSignal = "family-nu-no-nu-signal"
18+
DropFamilySizedToZero = "family-nu-sized-to-zero"
1819
DropDuplicateDedup = "duplicate-dedup"
1920
)
2021

‎pkg/common/drop_summary_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -80,12 +80,13 @@ func TestDropSummary_FormatOneLine(t *testing.T) {
8080
d.Add(DropTargetAlreadyMet, 3)
8181
d.Add(DropTargetSizedToZero, 4)
8282
d.Add(DropFamilyAlreadyAtTarget, 5)
83-
d.Add(DropFamilySizedToZero, 6)
84-
d.Add(DropDuplicateDedup, 7)
83+
d.Add(DropFamilyNoNUSignal, 6)
84+
d.Add(DropFamilySizedToZero, 7)
85+
d.Add(DropDuplicateDedup, 8)
8586
},
86-
expected: "Dropped 28 recs: --include-extended-support=2, --min-pool-size=1, " +
87-
"duplicate-dedup=7, family-nu-already-at-target=5, family-nu-sized-to-zero=6, " +
88-
"target-already-met=3, target-sized-to-zero=4",
87+
expected: "Dropped 36 recs: --include-extended-support=2, --min-pool-size=1, " +
88+
"duplicate-dedup=8, family-nu-already-at-target=5, family-nu-no-nu-signal=6, " +
89+
"family-nu-sized-to-zero=7, target-already-met=3, target-sized-to-zero=4",
8990
},
9091
}
9192

‎providers/aws/recommendations/family_nu.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,11 @@ type FamilyDropCounts struct {
138138
// AlreadyAtTarget is the number of recs from families where the
139139
// existing coverage already meets or exceeds the target (gap <= 0).
140140
AlreadyAtTarget int
141+
// NoNUSignal is the number of recs dropped because the family's
142+
// AWS-recommended counts summed to zero NU (e.g. all recs at
143+
// unknown/unrecognised sizes), so there is no scalable NU to apply
144+
// the family target against. This is distinct from AlreadyAtTarget.
145+
NoNUSignal int
141146
// SizedToZero is the number of recs dropped because the family-wide
142147
// scale factor produced a floor(0) count for that rec's size.
143148
SizedToZero int
@@ -182,6 +187,7 @@ func ApplyFamilyNUSizingRDS(
182187
sized, familyDrops := sizeRDSFamilyRecs(recs, indices, familyCov[fk], targetPct)
183188
sizedRDS = append(sizedRDS, sized...)
184189
drops.AlreadyAtTarget += familyDrops.AlreadyAtTarget
190+
drops.NoNUSignal += familyDrops.NoNUSignal
185191
drops.SizedToZero += familyDrops.SizedToZero
186192
}
187193
return sizedRDS, nonRDS, drops
@@ -254,8 +260,9 @@ func sizeRDSFamilyRecs(
254260
currentNU += float64(recs[i].Count) * rdsInstanceNUFromType(recs[i].ResourceType)
255261
}
256262
if currentNU <= 0 {
257-
// Recs sum to zero NU — nothing to scale.
258-
drops.AlreadyAtTarget = len(indices)
263+
// Recs sum to zero NU — family lookup returned no scalable signal.
264+
// This is not the same as "already at target"; record separately.
265+
drops.NoNUSignal = len(indices)
259266
return nil, drops
260267
}
261268
scale := targetNU / currentNU

‎providers/aws/recommendations/family_nu_test.go‎

Lines changed: 47 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -124,12 +124,13 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
124124
Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"},
125125
},
126126
}
127-
sized, nonRDS, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
127+
sized, nonRDS, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
128128
require.Len(t, sized, 1, "RDS rec kept (target NU > 0)")
129129
assert.Empty(t, nonRDS, "no non-RDS recs in this fixture")
130130
assert.Equal(t, 15, sized[0].Count, "AWS-rec NU already matches target → count preserved")
131131
// Costs unchanged (ratio = 1)
132132
assert.InDelta(t, 1500.0, sized[0].CommitmentCost, 0.01)
133+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected")
133134
})
134135

135136
t.Run("RecurringMonthlyCost scales with count when populated", func(t *testing.T) {
@@ -155,13 +156,14 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
155156
Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"},
156157
},
157158
}
158-
sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
159+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
159160
require.Len(t, sized, 1)
160161
// Scale 32/20 = 1.6 → newCount = 8 → monthly = 100 × 8/5 = 160
161162
assert.Equal(t, 8, sized[0].Count)
162163
require.NotNil(t, sized[0].RecurringMonthlyCost)
163164
assert.InDelta(t, 160.0, *sized[0].RecurringMonthlyCost, 0.001, "monthly fee scales by 8/5 alongside other costs")
164165
assert.Equal(t, 100.0, monthly, "original target should not be mutated (new pointer)")
166+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected")
165167
})
166168

167169
t.Run("AWS rec under-recommends → counts scale up", func(t *testing.T) {
@@ -186,10 +188,11 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
186188
Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"},
187189
},
188190
}
189-
sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
191+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
190192
require.Len(t, sized, 1)
191193
assert.Equal(t, 8, sized[0].Count, "scale 32/20 = 1.6 × 5 = 8 RIs to deliver 32 NU at 80% target")
192194
assert.InDelta(t, 5000*8.0/5.0, sized[0].CommitmentCost, 0.01, "CommitmentCost scales by 8/5")
195+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected")
193196
})
194197

195198
t.Run("AWS rec over-recommends → counts scale down", func(t *testing.T) {
@@ -212,9 +215,10 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
212215
Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "multi-az"},
213216
},
214217
}
215-
sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
218+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
216219
require.Len(t, sized, 1)
217220
assert.Equal(t, 8, sized[0].Count, "AWS over-proposed; scale down to family target")
221+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected")
218222
})
219223

220224
t.Run("family at-or-above target → all recs dropped", func(t *testing.T) {
@@ -234,8 +238,38 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
234238
Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"},
235239
},
236240
}
237-
sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
241+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
238242
assert.Empty(t, sized, "family already covered → drop all recs")
243+
assert.Equal(t, 1, drops.AlreadyAtTarget, "one rec dropped as family-at-target")
244+
assert.Equal(t, 0, drops.NoNUSignal+drops.SizedToZero, "no other drop categories")
245+
})
246+
247+
t.Run("scale produces floor(0) for rec → SizedToZero drop", func(t *testing.T) {
248+
// Family db.r7g Aurora MySQL us-east-1; cov=0, target=80 → target_NU=32.
249+
// AWS rec: 1 × db.r7g.xlarge = 8 NU → scale = 32/8 = 4 — wait, that
250+
// would scale up. Use a scenario where scale < 1 and count = 1:
251+
// TotalNU = 10*4 = 40, existing=79% → gap=1 → targetNU=0.4.
252+
// AWS rec: 1 × db.r7g.large = 4 NU → scale = 0.4/4 = 0.1 → floor(0.1) = 0.
253+
cov := PoolCoverageMap{
254+
rdsPoolKey("us-east-1", "db.r7g.large", "Aurora MySQL", "Single-AZ"): {
255+
Pct: 79.0, AvgInstancesPerHour: 10,
256+
},
257+
}
258+
recs := []common.Recommendation{
259+
{
260+
Service: common.ServiceRDS,
261+
CommitmentType: common.CommitmentReservedInstance,
262+
Region: "us-east-1",
263+
ResourceType: "db.r7g.large",
264+
Count: 1,
265+
CommitmentCost: 1000,
266+
Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"},
267+
},
268+
}
269+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
270+
assert.Empty(t, sized, "rec scaled to zero → dropped")
271+
assert.Equal(t, 1, drops.SizedToZero, "rec dropped because floor(scale*count)==0")
272+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal, "no other drop categories")
239273
})
240274

241275
t.Run("no coverage signal → recs pass through unchanged", func(t *testing.T) {
@@ -251,9 +285,10 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
251285
Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"},
252286
},
253287
}
254-
sized, _, _ := ApplyFamilyNUSizingRDS(recs, PoolCoverageMap{}, 80)
288+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, PoolCoverageMap{}, 80)
255289
require.Len(t, sized, 1, "rec preserved as-is when no family-NU signal")
256290
assert.Equal(t, 5, sized[0].Count)
291+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops when coverage map is empty")
257292
})
258293

259294
t.Run("non-RDS recs flow through nonRDS partition", func(t *testing.T) {
@@ -276,11 +311,12 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
276311
Pct: 0.0, AvgInstancesPerHour: 10,
277312
},
278313
}
279-
sized, nonRDS, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
314+
sized, nonRDS, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
280315
require.Len(t, sized, 1, "only the RDS rec went through family-NU")
281316
require.Len(t, nonRDS, 2, "EC2 + SP recs left for per-pool sizing")
282317
assert.Equal(t, common.ServiceEC2, nonRDS[0].Service)
283318
assert.Equal(t, common.ServiceSavingsPlans, nonRDS[1].Service)
319+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected")
284320
})
285321

286322
t.Run("ProjectedCoverage is cumulative across recs in a family", func(t *testing.T) {
@@ -322,7 +358,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
322358
Details: &common.DatabaseDetails{Engine: "aurora-postgresql", AZConfig: "single-az"},
323359
},
324360
}
325-
sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
361+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
326362
require.Len(t, sized, 2, "both recs kept after scaling")
327363
// Both recs see the SAME cumulative projection.
328364
assert.InDelta(t, sized[0].ProjectedCoverage, sized[1].ProjectedCoverage, 0.001,
@@ -334,6 +370,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
334370
// Both rec.Count values reflect the scale-down.
335371
assert.Equal(t, 4, sized[0].Count, "prod scaled 5→4")
336372
assert.Equal(t, 2, sized[1].Count, "staging scaled 3→2")
373+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected")
337374
})
338375

339376
t.Run("multiple sizes in same family scale together", func(t *testing.T) {
@@ -360,9 +397,10 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) {
360397
Details: &common.DatabaseDetails{Engine: "aurora-mysql", AZConfig: "single-az"},
361398
},
362399
}
363-
sized, _, _ := ApplyFamilyNUSizingRDS(recs, cov, 80)
400+
sized, _, drops := ApplyFamilyNUSizingRDS(recs, cov, 80)
364401
require.Len(t, sized, 1)
365402
assert.Equal(t, 12, sized[0].Count, "family-NU need includes .xlarge demand even though AWS rec is at .large")
403+
assert.Equal(t, 0, drops.AlreadyAtTarget+drops.NoNUSignal+drops.SizedToZero, "no drops expected")
366404
})
367405
}
368406

0 commit comments

Comments
 (0)