Fix idle allocation bug for windows < 1h; improve DurationOffset string conversion

pkg/costmodel/aggregation.go

@@ -204,23 +204,14 @@ func (a *Accesses) ComputeIdleCoefficient(costData map[string]*CostData, cli pro
 
 	key := fmt.Sprintf("%s:%s", windowString, offset)
 	if data, valid := a.ClusterCostsCache.Get(key); valid {
-
 		clusterCosts = data.(map[string]*ClusterCosts)
-
-		log.Infof("AggAPI: ComputeIdleCoefficient: ClusterCostsCache is valid")
-
 	} else {
 		clusterCosts, err = a.ComputeClusterCosts(cli, cp, windowString, offset, false)
 		if err != nil {
 			return nil, err
 		}
-
-		log.Infof("AggAPI: ComputeIdleCoefficient: ClusterCostsCache is not valid: ComputeClusterCosts")
-
 	}
 
-	log.Infof("AggAPI: ComputeIdleCoefficient: clusterCosts: %+v", clusterCosts)
-
 	measureTime(profileStart, profileThreshold, profileName)
 
 	for cid, costs := range clusterCosts {
@@ -301,8 +292,6 @@ func AggregateCostData(costData map[string]*CostData, field string, subfields []
 	rate := opts.Rate
 	sr := opts.SharedResourceInfo
 
-	log.Infof("AggAPI: AggregateCostData: idleCoefficients: %+v", idleCoefficients)
-
 	resolutionHours := 1.0
 	if opts.ResolutionHours > 0.0 {
 		resolutionHours = opts.ResolutionHours
@@ -1402,22 +1391,38 @@ func (a *Accesses) ComputeAggregateCostModel(promClient prometheusClient.Client,
 
 	idleCoefficients := make(map[string]float64)
 	if allocateIdle {
-		duration, offset := window.DurationOffsetStrings()
-
-		idleDurationCalcHours := window.Hours()
-		if window.Hours() < 1 {
-			idleDurationCalcHours = 1
+		dur, off, err := window.DurationOffset()
+		if err != nil {
+			log.Errorf("ComputeAggregateCostModel: error computing idle coefficient: illegal window: %s (%s)", window, err)
+			return nil, "", err
 		}
-		duration = fmt.Sprintf("%dh", int(idleDurationCalcHours))
 
-		if a.ThanosClient != nil {
-			offset = thanos.Offset()
-			log.Infof("ComputeAggregateCostModel: setting offset to %s", offset)
+		if a.ThanosClient != nil && off < thanos.OffsetDuration() {
+			// Determine difference between the Thanos offset and the requested
+			// offset; e.g. off=1h, thanosOffsetDuration=3h => diff=2h
+			diff := thanos.OffsetDuration() - off
+
+			// Reduce duration by difference and increase offset by difference
+			// e.g. 24h offset 0h => 21h offset 3h
+			dur = dur - diff
+			off = thanos.OffsetDuration()
+
+			log.Infof("ComputeAggregateCostModel: setting duration, offset to %s, %s due to Thanos", dur, off)
+
+			// Idle computation cannot be fulfilled for some windows, specifically
+			// those with sum(duration, offset) < Thanos offset, because there is
+			// no data within that window.
+			if dur <= 0 {
+				return nil, "", fmt.Errorf("requested idle coefficients from Thanos for illegal duration, offset: %s, %s (original window %s)", dur, off, window)
+			}
 		}
 
-		idleCoefficients, err = a.ComputeIdleCoefficient(costData, promClient, a.CloudProvider, discount, customDiscount, duration, offset)
+		// Convert to Prometheus-compatible strings
+		durStr, offStr := util.DurationOffsetStrings(dur, off)
+
+		idleCoefficients, err = a.ComputeIdleCoefficient(costData, promClient, a.CloudProvider, discount, customDiscount, durStr, offStr)
 		if err != nil {
-			log.Errorf("ComputeAggregateCostModel: error computing idle coefficient: duration=%s, offset=%s, err=%s", duration, offset, err)
+			log.Errorf("ComputeAggregateCostModel: error computing idle coefficient: duration=%s, offset=%s, err=%s", durStr, offStr, err)
 			return nil, "", err
 		}
 	}

pkg/kubecost/window.go

@@ -7,6 +7,8 @@ import (
 	"regexp"
 	"strconv"
 	"time"
+
+	"github.com/kubecost/cost-model/pkg/util"
 )
 
 const (
@@ -480,33 +482,16 @@ func (w Window) DurationOffset() (time.Duration, time.Duration, error) {
 	return duration, offset, nil
 }
 
-// DurationOffsetStrings returns formatted strings representing the duration and
-// offset of the window in terms of minutes; e.g. ("30m", "1m")
-// TODO check for nils in Window
+// DurationOffsetStrings returns formatted, Prometheus-compatible strings representing
+// the duration and offset of the window in terms of days, hours, minutes, or seconds;
+// e.g. ("7d", "1441m", "30m", "1s", "")
 func (w Window) DurationOffsetStrings() (string, string) {
-	durMins := int(w.Duration().Minutes())
-
-	offStr := ""
-	if w.End() != nil {
-		offMins := int(time.Now().Sub(*w.End()).Minutes())
-		if offMins > 1 {
-			offStr = fmt.Sprintf("%dm", int(offMins))
-		} else if offMins < -1 {
-			durMins += offMins
-		}
-	}
-
-	// default to formatting in terms of minutes
-	durStr := fmt.Sprintf("%dm", durMins)
-	if (durMins >= minutesPerDay) && (durMins%minutesPerDay == 0) {
-		// convert to days
-		durStr = fmt.Sprintf("%dd", durMins/minutesPerDay)
-	} else if (durMins >= minutesPerHour) && (durMins%minutesPerHour == 0) {
-		// convert to hours
-		durStr = fmt.Sprintf("%dh", durMins/minutesPerHour)
+	dur, off, err := w.DurationOffset()
+	if err != nil {
+		return "", ""
 	}
 
-	return durStr, offStr
+	return util.DurationOffsetStrings(dur, off)
 }
 
 type BoundaryError struct {

pkg/util/time.go

@@ -7,9 +7,21 @@ import (
 )
 
 const (
+	// SecsPerMin expresses the amount of seconds in a minute
+	SecsPerMin = 60.0
+
+	// SecsPerHour expresses the amount of seconds in a minute
+	SecsPerHour = 3600.0
+
+	// SecsPerDay expressed the amount of seconds in a day
+	SecsPerDay = 86400.0
+
 	// MinsPerHour expresses the amount of minutes in an hour
 	MinsPerHour = 60.0
 
+	// MinsPerDay expresses the amount of minutes in a day
+	MinsPerDay = 1440.0
+
 	// HoursPerDay expresses the amount of hours in a day
 	HoursPerDay = 24.0
 
@@ -20,6 +32,48 @@ const (
 	DaysPerMonth = 30.42
 )
 
+// DurationOffsetStrings converts a (duration, offset) pair to Prometheus-
+// compatible strings in terms of days, hours, minutes, or seconds.
+func DurationOffsetStrings(duration, offset time.Duration) (string, string) {
+	durSecs := int64(duration.Seconds())
+	offSecs := int64(offset.Seconds())
+
+	durStr := ""
+	if durSecs > 0 {
+		if durSecs%SecsPerDay == 0 {
+			// convert to days
+			durStr = fmt.Sprintf("%dd", durSecs/SecsPerDay)
+		} else if durSecs%SecsPerHour == 0 {
+			// convert to hours
+			durStr = fmt.Sprintf("%dh", durSecs/SecsPerHour)
+		} else if durSecs%SecsPerMin == 0 {
+			// convert to mins
+			durStr = fmt.Sprintf("%dm", durSecs/SecsPerMin)
+		} else if durSecs > 0 {
+			// default to mins, as long as duration is positive
+			durStr = fmt.Sprintf("%ds", durSecs)
+		}
+	}
+
+	offStr := ""
+	if offSecs > 0 {
+		if offSecs%SecsPerDay == 0 {
+			// convert to days
+			offStr = fmt.Sprintf("%dd", offSecs/SecsPerDay)
+		} else if offSecs%SecsPerHour == 0 {
+			// convert to hours
+			offStr = fmt.Sprintf("%dh", offSecs/SecsPerHour)
+		} else if offSecs%SecsPerMin == 0 {
+			// convert to mins
+			offStr = fmt.Sprintf("%dm", offSecs/SecsPerMin)
+		} else if offSecs > 0 {
+			// default to mins, as long as offation is positive
+			offStr = fmt.Sprintf("%ds", offSecs)
+		}
+	}
+	return durStr, offStr
+}
+
 // ParseDuration converts a Prometheus-style duration string into a Duration
 func ParseDuration(duration string) (*time.Duration, error) {
 	unitStr := duration[len(duration)-1:]