Merge branch 'develop' into sean/add-annotations

pkg/cloud/awsprovider.go

@@ -265,7 +265,7 @@ var locationToRegion = map[string]string{
 	"EU (Stockholm)":             "eu-north-1",
 	"South America (Sao Paulo)":  "sa-east-1",
 	"AWS GovCloud (US-East)":     "us-gov-east-1",
-	"AWS GovCloud (US)":          "us-gov-west-1",
+	"AWS GovCloud (US-West)":     "us-gov-west-1",
 }
 
 var regionToBillingRegionCode = map[string]string{

pkg/costmodel/aggregation.go

@@ -1440,22 +1440,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/costmodel/costmodel.go

@@ -178,10 +178,10 @@ const (
 		)
 	) by (namespace,container_name,pod_name,node,cluster_id)
 	* on (pod_name, namespace, cluster_id) group_left(container) label_replace(avg(avg_over_time(kube_pod_status_phase{phase="Running"}[%s] %s)) by (pod,namespace,cluster_id), "pod_name","$1","pod","(.+)")`
-	queryPVRequestsStr = `avg(avg(kube_persistentvolumeclaim_info{volumename != ""}) by (persistentvolumeclaim, storageclass, namespace, volumename, cluster_id)
+	queryPVRequestsStr = `avg(avg(kube_persistentvolumeclaim_info{volumename != ""}) by (persistentvolumeclaim, storageclass, namespace, volumename, cluster_id, kubernetes_node)
 	*
-	on (persistentvolumeclaim, namespace, cluster_id) group_right(storageclass, volumename)
-	sum(kube_persistentvolumeclaim_resource_requests_storage_bytes{volumename != ""}) by (persistentvolumeclaim, namespace, cluster_id, kubernetes_name)) by (persistentvolumeclaim, storageclass, namespace, volumename, cluster_id)`
+	on (persistentvolumeclaim, namespace, cluster_id, kubernetes_node) group_right(storageclass, volumename)
+	sum(kube_persistentvolumeclaim_resource_requests_storage_bytes{}) by (persistentvolumeclaim, namespace, cluster_id, kubernetes_node, kubernetes_name)) by (persistentvolumeclaim, storageclass, namespace, cluster_id, volumename, kubernetes_node)`
 	// queryRAMAllocationByteHours yields the total byte-hour RAM allocation over the given
 	// window, aggregated by container.
 	//  [line 3]  sum_over_time(each byte) = [byte*scrape] by metric

pkg/costmodel/metrics.go

@@ -13,6 +13,7 @@ import (
 	"github.com/kubecost/cost-model/pkg/errors"
 	"github.com/kubecost/cost-model/pkg/log"
 	"github.com/kubecost/cost-model/pkg/prom"
+	"github.com/kubecost/cost-model/pkg/util"
 
 	promclient "github.com/prometheus/client_golang/api"
 	"github.com/prometheus/client_golang/prometheus"
@@ -790,7 +791,11 @@ func (cmme *CostModelMetricsEmitter) Start() bool {
 		var defaultRegion string = ""
 		nodeList := cmme.KubeClusterCache.GetAllNodes()
 		if len(nodeList) > 0 {
-			defaultRegion = nodeList[0].Labels[v1.LabelZoneRegion]
+			var ok bool
+			defaultRegion, ok = util.GetRegion(nodeList[0].Labels)
+			if !ok {
+				log.DedupedWarningf(5, "Failed to locate default region")
+			}
 		}
 
 		for {

pkg/env/costmodelenv.go

@@ -64,7 +64,7 @@ const (
 // GetAWSAccessKeyID returns the environment variable value for AWSAccessKeyIDEnvVar which represents
 // the AWS access key for authentication
 func GetAppVersion() string {
-	return Get(AppVersionEnvVar, "1.71.0")
+	return Get(AppVersionEnvVar, "1.72.0")
 }
 
 // IsEmitNamespaceAnnotationsMetric returns true if cost-model is configured to emit the kube_namespace_annotations metric

pkg/kubecost/asset.go

@@ -114,7 +114,7 @@ type AssetLabels map[string]string
 
 // Clone returns a cloned map of labels
 func (al AssetLabels) Clone() AssetLabels {
-	clone := AssetLabels{}
+	clone := make(AssetLabels, len(al))
 
 	for label, value := range al {
 		clone[label] = value
@@ -2392,17 +2392,15 @@ func (as *AssetSet) Clone() *AssetSet {
 	as.RLock()
 	defer as.RUnlock()
 
-	assets := map[string]Asset{}
+	assets := make(map[string]Asset, len(as.assets))
 	for k, v := range as.assets {
 		assets[k] = v.Clone()
 	}
 
 	var props []AssetProperty
 	if as.props != nil {
-		props = []AssetProperty{}
-		for _, p := range as.props {
-			props = append(props, p)
-		}
+		props = make([]AssetProperty, len(as.props))
+		copy(props, as.props)
 	}
 
 	s := as.Start()

pkg/kubecost/kubecost_codecs.go

@@ -421,8 +421,8 @@ func (target *AllocationSet) UnmarshalBinary(data []byte) (err error) {
 		target.allocations = nil
 	} else {
 		// --- [begin][read][map](map[string]*Allocation) ---
-		a := make(map[string]*Allocation)
 		b := buff.ReadInt() // map len
+		a := make(map[string]*Allocation, b)
 		for i := 0; i < b; i++ {
 			var k string
 			c := buff.ReadString() // read string
@@ -454,8 +454,8 @@ func (target *AllocationSet) UnmarshalBinary(data []byte) (err error) {
 		target.idleKeys = nil
 	} else {
 		// --- [begin][read][map](map[string]bool) ---
-		g := make(map[string]bool)
 		h := buff.ReadInt() // map len
+		g := make(map[string]bool, h)
 		for j := 0; j < h; j++ {
 			var kk string
 			l := buff.ReadString() // read string
@@ -745,8 +745,8 @@ func (target *Any) UnmarshalBinary(data []byte) (err error) {
 		a = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		b := make(map[string]string)
 		c := buff.ReadInt() // map len
+		b := make(map[string]string, c)
 		for i := 0; i < c; i++ {
 			var k string
 			d := buff.ReadString() // read string
@@ -1046,8 +1046,8 @@ func (target *AssetSet) UnmarshalBinary(data []byte) (err error) {
 		target.assets = nil
 	} else {
 		// --- [begin][read][map](map[string]Asset) ---
-		a := make(map[string]Asset)
 		b := buff.ReadInt() // map len
+		a := make(map[string]Asset, b)
 		for i := 0; i < b; i++ {
 			var k string
 			c := buff.ReadString() // read string
@@ -1447,8 +1447,8 @@ func (target *Cloud) UnmarshalBinary(data []byte) (err error) {
 		a = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		b := make(map[string]string)
 		c := buff.ReadInt() // map len
+		b := make(map[string]string, c)
 		for i := 0; i < c; i++ {
 			var k string
 			d := buff.ReadString() // read string
@@ -1622,8 +1622,8 @@ func (target *ClusterManagement) UnmarshalBinary(data []byte) (err error) {
 		a = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		b := make(map[string]string)
 		c := buff.ReadInt() // map len
+		b := make(map[string]string, c)
 		for i := 0; i < c; i++ {
 			var k string
 			d := buff.ReadString() // read string
@@ -1808,8 +1808,8 @@ func (target *Disk) UnmarshalBinary(data []byte) (err error) {
 		a = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		b := make(map[string]string)
 		c := buff.ReadInt() // map len
+		b := make(map[string]string, c)
 		for i := 0; i < c; i++ {
 			var k string
 			d := buff.ReadString() // read string
@@ -2038,8 +2038,8 @@ func (target *LoadBalancer) UnmarshalBinary(data []byte) (err error) {
 		d = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		e := make(map[string]string)
 		f := buff.ReadInt() // map len
+		e := make(map[string]string, f)
 		for i := 0; i < f; i++ {
 			var k string
 			g := buff.ReadString() // read string
@@ -2232,8 +2232,8 @@ func (target *Network) UnmarshalBinary(data []byte) (err error) {
 		d = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		e := make(map[string]string)
 		f := buff.ReadInt() // map len
+		e := make(map[string]string, f)
 		for i := 0; i < f; i++ {
 			var k string
 			g := buff.ReadString() // read string
@@ -2463,8 +2463,8 @@ func (target *Node) UnmarshalBinary(data []byte) (err error) {
 		d = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		e := make(map[string]string)
 		f := buff.ReadInt() // map len
+		e := make(map[string]string, f)
 		for i := 0; i < f; i++ {
 			var k string
 			g := buff.ReadString() // read string
@@ -2689,8 +2689,8 @@ func (target *SharedAsset) UnmarshalBinary(data []byte) (err error) {
 		d = nil
 	} else {
 		// --- [begin][read][map](map[string]string) ---
-		e := make(map[string]string)
 		f := buff.ReadInt() // map len
+		e := make(map[string]string, f)
 		for i := 0; i < f; i++ {
 			var k string
 			g := buff.ReadString() // read string

pkg/kubecost/properties.go

@@ -65,7 +65,7 @@ func (p *Properties) Clone() Properties {
 		return nil
 	}
 
-	clone := Properties{}
+	clone := make(Properties, len(*p))
 	for k, v := range *p {
 		clone[k] = v
 	}
@@ -707,8 +707,8 @@ func (p *Properties) UnmarshalBinary(data []byte) error {
 
 	// LabelProp
 	if buff.ReadUInt8() == 1 { // read nil byte
-		labels := map[string]string{}
 		length := buff.ReadInt() // read map len
+		labels := make(map[string]string, length)
 		for idx := 0; idx < length; idx++ {
 			key := buff.ReadString()
 			val := buff.ReadString()
@@ -719,8 +719,8 @@ func (p *Properties) UnmarshalBinary(data []byte) error {
 
 	// AnnotationProp
 	if buff.ReadUInt8() == 1 { // read nil byte
-		annotations := map[string]string{}
 		length := buff.ReadInt() // read map len
+		annotations := make(map[string]string, length)
 		for idx := 0; idx < length; idx++ {
 			key := buff.ReadString()
 			val := buff.ReadString()
@@ -731,11 +731,11 @@ func (p *Properties) UnmarshalBinary(data []byte) error {
 
 	// ServiceProp
 	if buff.ReadUInt8() == 1 { // read nil byte
-		services := []string{}
 		length := buff.ReadInt() // read map len
+		services := make([]string, length)
 		for idx := 0; idx < length; idx++ {
 			val := buff.ReadString()
-			services = append(services, val)
+			services[idx] = val
 		}
 		p.SetServices(services)
 	}

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:]

pkg/util/time_test.go

@@ -0,0 +1,56 @@
+package util
+
+import (
+	"testing"
+	"time"
+)
+
+func TestDurationOffsetStrings(t *testing.T) {
+	dur, off := "", ""
+
+	dur, off = DurationOffsetStrings(0, 0)
+	if dur != "" || off != "" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "", "", dur, off)
+	}
+
+	dur, off = DurationOffsetStrings(24*time.Hour, 0)
+	if dur != "1d" || off != "" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "1d", "", dur, off)
+	}
+
+	dur, off = DurationOffsetStrings(24*time.Hour+5*time.Minute, 0)
+	if dur != "1445m" || off != "" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "1445m", "", dur, off)
+	}
+
+	dur, off = DurationOffsetStrings(25*time.Hour, 5*time.Minute)
+	if dur != "25h" || off != "5m" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "25h", "5m", dur, off)
+	}
+
+	dur, off = DurationOffsetStrings(25*time.Hour, 60*time.Minute)
+	if dur != "25h" || off != "1h" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "25h", "1h", dur, off)
+	}
+
+	dur, off = DurationOffsetStrings(72*time.Hour, 1440*time.Minute)
+	if dur != "3d" || off != "1d" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "3d", "1d", dur, off)
+	}
+
+	dur, off = DurationOffsetStrings(25*time.Hour, 1*time.Second)
+	if dur != "25h" || off != "1s" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "25h", "1s", dur, off)
+	}
+
+	dur, off = DurationOffsetStrings(24*time.Hour+time.Second, 1*time.Second)
+	if dur != "86401s" || off != "1s" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "86401s", "1s", dur, off)
+	}
+
+	// Expect empty strings if durations are negative
+	dur, off = DurationOffsetStrings(-25*time.Hour, -1*time.Second)
+	if dur != "" || off != "" {
+		t.Fatalf("DurationOffsetStrings: exp (%s %s); act (%s, %s)", "", "", dur, off)
+	}
+}