فهرست منبع

fix: cover LbAllocation.Adjustment and simplify summary rebuild

Addresses two further Copilot review comments on PR #3735:

1. pkg/costmodel/currency_helper.go -- LbAllocation.Adjustment was not
   being converted. core/pkg/opencost/allocation.go defines it with
   `json:"adjustment"` so it appears in API responses; leaving it in
   USD produced the exact mixed-currency failure mode the helper is
   meant to prevent. Converting it alongside LbAllocation.Cost. Test
   fixture and assertion updated.

2. pkg/costmodel/aggregation.go -- ComputeAllocationHandlerSummary was
   building the summary set twice on any non-USD request: once from
   the unconverted AllocationSetRange, then rebuilding after conversion.
   Moved the conversion ahead of summary construction so it happens
   once. Also corrected the stale "partially mutated" comment: after
   the earlier GetRate-probe refactor, the helper returns an error
   only before any mutation, so the pre-fix worry about partial
   mutations no longer applies.

Signed-off-by: Warwick Peatey <warwick@automatic.systems>
Assisted-by: Claude Code
Warwick Peatey 1 ماه پیش
والد
کامیت
8faaab6607
3فایلهای تغییر یافته به همراه19 افزوده شده و 24 حذف شده
  1. 12 22
      pkg/costmodel/aggregation.go
  2. 3 1
      pkg/costmodel/currency_helper.go
  3. 4 1
      pkg/costmodel/currency_helper_test.go

+ 12 - 22
pkg/costmodel/aggregation.go

@@ -157,35 +157,25 @@ func (a *Accesses) ComputeAllocationHandlerSummary(w http.ResponseWriter, r *htt
 		}
 		}
 	}
 	}
 
 
-	sasl := []*opencost.SummaryAllocationSet{}
-	for _, as := range asr.Slice() {
-		sas := opencost.NewSummaryAllocationSet(as, nil, nil, false, false)
-		sasl = append(sasl, sas)
-	}
-	sasr := opencost.NewSummaryAllocationSetRange(sasl...)
-
-	// Extract currency parameter and convert if needed. Currency conversion
-	// is best-effort: individual field failures are logged at source and
-	// leave USD values in place. A non-nil error from the helper means the
-	// upfront rate lookup failed and no mutation occurred.
+	// Convert the underlying AllocationSetRange *before* building the
+	// summary so we don't pay for two summary constructions on non-USD
+	// requests. The helper returns a non-nil error only on the upfront
+	// rate-lookup failure, which happens before any mutation, so on
+	// error we proceed with untouched USD data.
 	currency := strings.ToUpper(strings.TrimSpace(qp.Get("currency", "USD")))
 	currency := strings.ToUpper(strings.TrimSpace(qp.Get("currency", "USD")))
 	if currency != "USD" && a.CurrencyConverter != nil {
 	if currency != "USD" && a.CurrencyConverter != nil {
 		if err = ConvertAllocationSetRange(asr, a.CurrencyConverter, currency); err != nil {
 		if err = ConvertAllocationSetRange(asr, a.CurrencyConverter, currency); err != nil {
 			log.Warnf("Currency conversion failed for currency %s: %v", currency, err)
 			log.Warnf("Currency conversion failed for currency %s: %v", currency, err)
 		}
 		}
-		// Always rebuild the summary: the helper may have partially
-		// mutated the underlying sets even when it returns an error
-		// (per-field best-effort), so the pre-conversion summary would
-		// otherwise be inconsistent with the (possibly partly converted)
-		// data.
-		sasl = sasl[:0]
-		for _, as := range asr.Slice() {
-			sas := opencost.NewSummaryAllocationSet(as, nil, nil, false, false)
-			sasl = append(sasl, sas)
-		}
-		sasr = opencost.NewSummaryAllocationSetRange(sasl...)
 	}
 	}
 
 
+	sasl := []*opencost.SummaryAllocationSet{}
+	for _, as := range asr.Slice() {
+		sas := opencost.NewSummaryAllocationSet(as, nil, nil, false, false)
+		sasl = append(sasl, sas)
+	}
+	sasr := opencost.NewSummaryAllocationSetRange(sasl...)
+
 	WriteData(w, sasr, nil)
 	WriteData(w, sasr, nil)
 }
 }
 
 

+ 3 - 1
pkg/costmodel/currency_helper.go

@@ -128,7 +128,9 @@ func convertAllocationFields(alloc *opencost.Allocation, converter currency.Conv
 		if lb == nil {
 		if lb == nil {
 			continue
 			continue
 		}
 		}
-		lb.Cost = tryConvert(converter, lb.Cost, targetCurrency, "Allocation.LoadBalancers."+lbKey+".Cost")
+		lbCtx := "Allocation.LoadBalancers." + lbKey
+		lb.Cost = tryConvert(converter, lb.Cost, targetCurrency, lbCtx+".Cost")
+		lb.Adjustment = tryConvert(converter, lb.Adjustment, targetCurrency, lbCtx+".Adjustment")
 		alloc.LoadBalancers[lbKey] = lb
 		alloc.LoadBalancers[lbKey] = lb
 	}
 	}
 
 

+ 4 - 1
pkg/costmodel/currency_helper_test.go

@@ -72,7 +72,7 @@ func newFullAllocation() *opencost.Allocation {
 			opencost.PVKey{Cluster: "c1", Name: "pv-a"}: {Cost: 100, Adjustment: 5, ByteHours: 1},
 			opencost.PVKey{Cluster: "c1", Name: "pv-a"}: {Cost: 100, Adjustment: 5, ByteHours: 1},
 		},
 		},
 		LoadBalancers: opencost.LbAllocations{
 		LoadBalancers: opencost.LbAllocations{
-			"lb-a": {Service: "svc-a", Cost: 80},
+			"lb-a": {Service: "svc-a", Cost: 80, Adjustment: 3},
 		},
 		},
 		SharedCostBreakdown: opencost.SharedCostBreakdowns{
 		SharedCostBreakdown: opencost.SharedCostBreakdowns{
 			"shared-a": {
 			"shared-a": {
@@ -187,6 +187,9 @@ func TestConvertAllocation_NestedStructures(t *testing.T) {
 	if lb.Cost != 160 {
 	if lb.Cost != 160 {
 		t.Errorf("LB.Cost: got %v want 160", lb.Cost)
 		t.Errorf("LB.Cost: got %v want 160", lb.Cost)
 	}
 	}
+	if lb.Adjustment != 6 {
+		t.Errorf("LB.Adjustment: got %v want 6", lb.Adjustment)
+	}
 	if lb.Service != "svc-a" {
 	if lb.Service != "svc-a" {
 		t.Errorf("LB.Service must not be touched: got %q", lb.Service)
 		t.Errorf("LB.Service must not be touched: got %q", lb.Service)
 	}
 	}