Fix overhead addition in (*Node).add() (#2246) (#2249)

Before this change, observed overheads after .add() could exceed 1. See
the new comments for an explanation.

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

pkg/kubecost/asset.go

@@ -2160,6 +2160,13 @@ func (n *Node) add(that *Node) {
 		n.RAMBreakdown.User = (n.RAMBreakdown.User*n.RAMCost + that.RAMBreakdown.User*that.RAMCost) / totalRAMCost
 	}
 
+	// These calculations have to happen before the mutable fields of n they
+	// depend on (cpu cost, ram cost) are mutated with post-add totals.
+	if n.Overhead != nil && that.Overhead != nil {
+		n.Overhead.RamOverheadFraction = (n.Overhead.RamOverheadFraction*n.RAMCost + that.Overhead.RamOverheadFraction*that.RAMCost) / totalRAMCost
+		n.Overhead.CpuOverheadFraction = (n.Overhead.CpuOverheadFraction*n.CPUCost + that.Overhead.CpuOverheadFraction*that.CPUCost) / totalCPUCost
+	}
+
 	n.CPUCoreHours += that.CPUCoreHours
 	n.RAMByteHours += that.RAMByteHours
 	n.GPUHours += that.GPUHours
@@ -2169,10 +2176,9 @@ func (n *Node) add(that *Node) {
 	n.RAMCost += that.RAMCost
 	n.Adjustment += that.Adjustment
 
-	if n.Overhead != nil && that.Overhead != nil {
-
-		n.Overhead.RamOverheadFraction = (n.Overhead.RamOverheadFraction*n.RAMCost + that.Overhead.RamOverheadFraction*that.RAMCost) / totalRAMCost
-		n.Overhead.CpuOverheadFraction = (n.Overhead.CpuOverheadFraction*n.CPUCost + that.Overhead.CpuOverheadFraction*that.CPUCost) / totalCPUCost
+	// The cost-weighted overhead is calculated after the node is totaled
+	// because the cost-weighted overhead is based on post-add data.
+	if n.Overhead != nil {
 		n.Overhead.OverheadCostFraction = ((n.Overhead.CpuOverheadFraction * n.CPUCost) + (n.Overhead.RamOverheadFraction * n.RAMCost)) / n.TotalCost()
 	}
 }

pkg/kubecost/asset_test.go

@@ -384,6 +384,11 @@ func TestNode_Add(t *testing.T) {
 		Other:  0.0,
 	}
 	node1.SetAdjustment(1.6)
+	node1.Overhead = &NodeOverhead{
+		CpuOverheadFraction:  1,
+		RamOverheadFraction:  1,
+		OverheadCostFraction: 1,
+	}
 
 	node2 := NewNode("node2", "cluster1", "node2", *windows[0].start, *windows[0].end, windows[0])
 	node2.CPUCoreHours = 1.0 * hours
@@ -406,6 +411,11 @@ func TestNode_Add(t *testing.T) {
 		Other:  0.05,
 	}
 	node2.SetAdjustment(1.0)
+	node2.Overhead = &NodeOverhead{
+		CpuOverheadFraction:  0.6,
+		RamOverheadFraction:  0.75,
+		OverheadCostFraction: 0.7,
+	}
 
 	nodeT := node1.Add(node2).(*Node)
 
@@ -434,6 +444,19 @@ func TestNode_Add(t *testing.T) {
 	if nodeT.RAMBytes() != 4.0*gb {
 		t.Fatalf("Node.Add: expected %f; got %f", 4.0*gb, nodeT.RAMBytes())
 	}
+	if o := nodeT.Overhead; o == nil {
+		t.Errorf("Node.Add (1 + 2): expected overhead to be non-nil")
+	} else {
+		if o.CpuOverheadFraction < 0 || o.CpuOverheadFraction > 1 {
+			t.Errorf("CPU overhead must be within [0, 1], is: %f", o.CpuOverheadFraction)
+		}
+		if o.RamOverheadFraction < 0 || o.RamOverheadFraction > 1 {
+			t.Errorf("RAM overhead must be within [0, 1], is: %f", o.RamOverheadFraction)
+		}
+		if o.OverheadCostFraction < 0 || o.OverheadCostFraction > 1 {
+			t.Errorf("Cost-weighted overhead must be within [0, 1], is: %f", o.OverheadCostFraction)
+		}
+	}
 
 	// Check that the original assets are unchanged
 	if !util.IsApproximately(node1.TotalCost(), 10.0) {