Merge pull request #2047 from avrodrigues5/CORE-370

fix the unmounted allocation caused by the offset duration leading to an unmounted PVC coefficient entry

pkg/costmodel/allocation.go

@@ -662,7 +662,7 @@ func (cm *CostModel) computeAllocation(start, end time.Time, resolution time.Dur
 	// split appropriately among each pod's container allocation.
 	podPVCMap := map[podKey][]*pvc{}
 	buildPodPVCMap(podPVCMap, pvMap, pvcMap, podMap, resPodPVCAllocation, podUIDKeyMap, ingestPodUID)
-	applyPVCsToPods(window, podMap, podPVCMap, pvcMap)
+	applyPVCsToPods(window, podMap, podPVCMap, pvcMap, resolution)
 
 	// Identify PVCs without pods and add pv costs to the unmounted Allocation for the pvc's cluster
 	applyUnmountedPVCs(window, podMap, pvcMap)

pkg/costmodel/allocation_helpers.go

@@ -1944,7 +1944,8 @@ func buildPodPVCMap(podPVCMap map[podKey][]*pvc, pvMap map[pvKey]*pv, pvcMap map
 		}
 	}
 }
-func applyPVCsToPods(window kubecost.Window, podMap map[podKey]*pod, podPVCMap map[podKey][]*pvc, pvcMap map[pvcKey]*pvc) {
+
+func applyPVCsToPods(window kubecost.Window, podMap map[podKey]*pod, podPVCMap map[podKey][]*pvc, pvcMap map[pvcKey]*pvc, resolution time.Duration) {
 	// Because PVCs can be shared among pods, the respective pv cost
 	// needs to be evenly distributed to those pods based on time
 	// running, as well as the amount of time the pvc was shared.
@@ -1994,7 +1995,7 @@ func applyPVCsToPods(window kubecost.Window, podMap map[podKey]*pod, podPVCMap m
 		}
 
 		// Determine coefficients for each pvc-pod relation.
-		sharedPVCCostCoefficients := getPVCCostCoefficients(intervals, pvc)
+		sharedPVCCostCoefficients := getPVCCostCoefficients(intervals, pvc, resolution)
 
 		// Distribute pvc costs to Allocations
 		for thisPodKey, coeffComponents := range sharedPVCCostCoefficients {

pkg/costmodel/intervals.go

@@ -79,20 +79,20 @@ func getIntervalPointsFromWindows(windows map[podKey]kubecost.Window) IntervalPo
 // getPVCCostCoefficients gets a coefficient which represents the scale
 // factor that each PVC in a pvcIntervalMap and corresponding slice of
 // IntervalPoints intervals uses to calculate a cost for that PVC's PV.
-func getPVCCostCoefficients(intervals IntervalPoints, thisPVC *pvc) map[podKey][]CoefficientComponent {
+func getPVCCostCoefficients(intervals IntervalPoints, thisPVC *pvc, resolution time.Duration) map[podKey][]CoefficientComponent {
 	// pvcCostCoefficientMap has a format such that the individual coefficient
 	// components are preserved for testing purposes.
 	pvcCostCoefficientMap := make(map[podKey][]CoefficientComponent)
 
-	pvcWindow := kubecost.NewWindow(&thisPVC.Start, &thisPVC.End)
-
+	// Reset the start time by the offset as well. so that offset is not used in coefficient calculation!
+	startTime := thisPVC.Start.Add(resolution)
+	pvcWindow := kubecost.NewWindow(&startTime, &thisPVC.End)
 	unmountedKey := getUnmountedPodKey(thisPVC.Cluster)
 
 	var void struct{}
 	activeKeys := map[podKey]struct{}{}
 
-	currentTime := thisPVC.Start
-
+	currentTime := startTime
 	// For each interval i.e. for any time a pod-PVC relation ends or starts...
 	for _, point := range intervals {
 		// If the current point happens at a later time than the previous point
@@ -142,7 +142,6 @@ func getPVCCostCoefficients(intervals IntervalPoints, thisPVC *pvc) map[podKey][
 			},
 		)
 	}
-
 	return pvcCostCoefficientMap
 }
 

pkg/costmodel/intervals_test.go

@@ -164,11 +164,24 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 	pod4Key := newPodKey("cluster1", "namespace1", "pod4")
 	ummountedPodKey := newPodKey("cluster1", kubecost.UnmountedSuffix, kubecost.UnmountedSuffix)
 
+	zeroDuration, _ := time.ParseDuration("0m0s")
+	fiveMinOffset, _ := time.ParseDuration("5m")
+	// Reflects the pvc that is offset by duration, the actual case that happens in allocation workflow.
+	// before core-370, the offset was causing a unmounted shared pvc coefficient map for the duration of the offset.
+	pvcWithDurationOffset := &pvc{
+		Bytes:     0,
+		Name:      "pvc1",
+		Cluster:   "cluster1",
+		Namespace: "namespace1",
+		Start:     time.Date(2021, 2, 19, 8, 0, 0, 0, time.UTC).Add(-fiveMinOffset),
+		End:       time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC),
+	}
 	cases := []struct {
 		name           string
 		pvc            *pvc
 		pvcIntervalMap map[podKey]kubecost.Window
 		intervals      []IntervalPoint
+		resolution     time.Duration
 		expected       map[podKey][]CoefficientComponent
 	}{
 		{
@@ -184,6 +197,7 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod3Key),
 				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod1Key),
 			},
+			resolution: zeroDuration,
 			expected: map[podKey][]CoefficientComponent{
 				pod1Key: {
 					{0.5, 0.25},
@@ -212,6 +226,7 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 				NewIntervalPoint(time.Date(2021, 2, 19, 8, 30, 0, 0, time.UTC), "end", pod1Key),
 				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod2Key),
 			},
+			resolution: zeroDuration,
 			expected: map[podKey][]CoefficientComponent{
 				pod1Key: {
 					{1.0, 0.5},
@@ -230,6 +245,7 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod1Key),
 				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod2Key),
 			},
+			resolution: zeroDuration,
 			expected: map[podKey][]CoefficientComponent{
 				pod1Key: {
 					{0.5, 0.5},
@@ -249,6 +265,7 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 				NewIntervalPoint(time.Date(2021, 2, 19, 8, 0, 0, 0, time.UTC), "start", pod1Key),
 				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod1Key),
 			},
+			resolution: zeroDuration,
 			expected: map[podKey][]CoefficientComponent{
 				pod1Key: {
 					{1.0, 1.0},
@@ -264,6 +281,7 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 				NewIntervalPoint(time.Date(2021, 2, 19, 8, 45, 0, 0, time.UTC), "start", pod2Key),
 				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod2Key),
 			},
+			resolution: zeroDuration,
 			expected: map[podKey][]CoefficientComponent{
 				pod1Key: {
 					{1.0, 0.25},
@@ -283,6 +301,7 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 				NewIntervalPoint(time.Date(2021, 2, 19, 8, 15, 0, 0, time.UTC), "start", pod1Key),
 				NewIntervalPoint(time.Date(2021, 2, 19, 8, 45, 0, 0, time.UTC), "end", pod1Key),
 			},
+			resolution: zeroDuration,
 			expected: map[podKey][]CoefficientComponent{
 				pod1Key: {
 					{1.0, 0.5},
@@ -293,11 +312,31 @@ func TestGetPVCCostCoefficients(t *testing.T) {
 				},
 			},
 		},
+		// Test case to ensure the offset doesnt cause any unmounted
+		{
+			name: "pvcMap with duration offset not causing any unmounted entry in sharedPV Coefficient",
+			pvc:  pvcWithDurationOffset,
+			intervals: []IntervalPoint{
+				NewIntervalPoint(time.Date(2021, 2, 19, 8, 0, 0, 0, time.UTC), "start", pod1Key),
+				NewIntervalPoint(time.Date(2021, 2, 19, 8, 30, 0, 0, time.UTC), "start", pod2Key),
+				NewIntervalPoint(time.Date(2021, 2, 19, 8, 30, 0, 0, time.UTC), "end", pod1Key),
+				NewIntervalPoint(time.Date(2021, 2, 19, 9, 0, 0, 0, time.UTC), "end", pod2Key),
+			},
+			resolution: fiveMinOffset,
+			expected: map[podKey][]CoefficientComponent{
+				pod1Key: {
+					{1.0, 0.5},
+				},
+				pod2Key: {
+					{1.0, 0.5},
+				},
+			},
+		},
 	}
 
 	for _, testCase := range cases {
 		t.Run(testCase.name, func(t *testing.T) {
-			result := getPVCCostCoefficients(testCase.intervals, testCase.pvc)
+			result := getPVCCostCoefficients(testCase.intervals, testCase.pvc, testCase.resolution)
 
 			if !reflect.DeepEqual(result, testCase.expected) {
 				t.Errorf("getPVCCostCoefficients test failed: %s: Got %+v but expected %+v", testCase.name, result, testCase.expected)