[helm] Usage of function mimir.siToBytes seems inconsistent

[edwintye]

This applies to helm chart mimir-distributor version 5.5.1 and from what I can tell possible all versions where the function mimir.siToBytes existed.

Describe the bug

The function mimir.siToBytes uses the mul function which is designed for integer arithmetics, but this is not guaranteed in the input. For example, it is common (at least not uncommon) to see something like 2.5Gi being used as a memory request, which is a float. The impact is that the resulting bytes will always be rounded down.

To Reproduce

This can be reproduced by:

1. Change the value of small.yaml in the helm chart from store_gateway.resources.requests.memory: 1.5Gi to store_gateway.resources.requests.memory: 1Gi
2. Build the tests/just helm template it out
3. Compare GOMEMLIMIT value between the original and the new.

Expected behavior

I would expect that the value of GOMEMLIMIT to be different for different values of memory request. Specifically, the calculated value of a whole number, e.g. 1Gi, is at exactly 1Gi which does not follow the general wisdom of "setting GOMEMLIMIT at 80% of memory requested".

Furthermore, when I look at the other situations where the function mimir.siToBytes, e.g. distributor ScaledObject and ruler ScaledObject, the logic of multiplying against a target utilization exists. My gut feel is that the GOMEMLIMIT calculation should be changed here to be more in line with the rest of the logic.

Additional Context

I am willing to contribute if I know which direction to take. The main concern for me is that I am unsure about the blast radius since it will also impact the autoscaling values. We don't use the keda rules provided by the chart and have cooked something else up, and isn't really in a position to test how the autoscaling rule would work post change.

I am also attaching a small go program here to demonstrate my personal preference on which direction to take; I feel the change should be from mul to mulf and add in a multiplication factor when calculating GOMEMLIMIT.

package main

import (
	"bytes"
	"fmt"
	"log"
	"text/template"

	sprig "github.com/Masterminds/sprig/v3"
)

func main() {
	// original mul which floors the computation
	fmt.Println(`Using the original computation with function "mul""`)
	tpl := `{{- trimSuffix "Gi" .Memory | float64 | mul 1073741824 | ceil | int64 -}}`

	mems := []struct{ Memory string }{
		{"2Gi"},
		{"2.1Gi"},
		{"2.5Gi"},
		{"2.99Gi"},
	}

	fmap := sprig.TxtFuncMap()
	for _, mem := range mems {
		b := new(bytes.Buffer)
		if err := template.Must(template.New("tmpl").Funcs(fmap).Parse(tpl)).Execute(b, mem); err != nil {
			log.Fatal(err)
		}
		fmt.Println(b.String())
	}
	// Doing the same again but with mulf, this is the change in the `mimir.siToBytes` function
	fmt.Println(`Changing the computation to using "mulf"`)
	tpl = `{{- trimSuffix "Gi" .Memory | float64 | mulf 1073741824 | ceil | int64 -}}`

	mems = []struct{ Memory string }{
		{"2Gi"},
		{"2.1Gi"},
		{"2.5Gi"},
		{"2.99Gi"},
	}

	for _, mem := range mems {
		b := new(bytes.Buffer)
		if err := template.Must(template.New("tmpl").Funcs(fmap).Parse(tpl)).Execute(b, mem); err != nil {
			log.Fatal(err)
		}
		fmt.Println(b.String())
	}

	// The proposed change to setting GOMEMLIMIT in store gateway, where the bracket contains the function
        // and the following calculation would be in `store-gateway-statefulset.yaml`
	fmt.Println(`GOMEMLIMIT setting using "mulf" and at 80%`)
	tpl = `{{- (trimSuffix "Gi" .Memory | float64 | mulf 1073741824 | ceil | int64) | mulf 0.8 | floor | int64 -}}`

	mems = []struct{ Memory string }{
		{"2Gi"},
		{"2.1Gi"},
		{"2.5Gi"},
		{"2.99Gi"},
	}

	for _, mem := range mems {
		b := new(bytes.Buffer)
		if err := template.Must(template.New("tmpl").Funcs(fmap).Parse(tpl)).Execute(b, mem); err != nil {
			log.Fatal(err)
		}
		fmt.Println(b.String())
	}
}