Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Mi units #667

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions cmd/dashboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ import (
)

var (
serverPort int
showAllVPAs bool
basePath string
insightsHost string
enableCost bool
serverPort int
showAllVPAs bool
basePath string
insightsHost string
enableCost bool
useMemoryBinarySI bool
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about naming here 😅

)

func init() {
Expand All @@ -43,6 +44,7 @@ func init() {
dashboardCmd.PersistentFlags().StringVar(&basePath, "base-path", "/", "Path on which the dashboard is served.")
dashboardCmd.PersistentFlags().BoolVar(&enableCost, "enable-cost", true, "If set to false, the cost integration will be disabled on the dashboard.")
dashboardCmd.PersistentFlags().StringVar(&insightsHost, "insights-host", "https://insights.fairwinds.com", "Insights host for retrieving optional cost data.")
dashboardCmd.PersistentFlags().BoolVar(&useMemoryBinarySI, "binary-si", false, "Use binary SI units for memory (base 2) instead of decimal SI units (base 10).")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion please, naming is hard 😓

}

var dashboardCmd = &cobra.Command{
Expand All @@ -59,6 +61,7 @@ var dashboardCmd = &cobra.Command{
dashboard.ShowAllVPAs(showAllVPAs),
dashboard.InsightsHost(insightsHost),
dashboard.EnableCost(enableCost),
dashboard.UseMemoryBinarySI(useMemoryBinarySI),
)
http.Handle("/", router)
klog.Infof("Starting goldilocks dashboard server on port %d and basePath %v", serverPort, validBasePath)
Expand Down
5 changes: 5 additions & 0 deletions cmd/summary.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ import (
var excludeContainers string
var outputFile string
var namespace string
var summaryUseMemoryBinarySI bool

func init() {
rootCmd.AddCommand(summaryCmd)
summaryCmd.PersistentFlags().StringVarP(&excludeContainers, "exclude-containers", "e", "", "Comma delimited list of containers to exclude from recommendations.")
summaryCmd.PersistentFlags().StringVarP(&outputFile, "output-file", "f", "", "File to write output from audit.")
summaryCmd.PersistentFlags().StringVarP(&namespace, "namespace", "n", "", "Limit the summary to only a single Namespace.")
summaryCmd.PersistentFlags().BoolVarP(&summaryUseMemoryBinarySI, "binary-si", "b", false, "Use binary SI units for memory (base 2) instead of decimal SI units (base 10).")
}

var summaryCmd = &cobra.Command{
Expand All @@ -57,6 +59,9 @@ By default the summary will be about all VPAs in all namespaces.`,
opts = append(opts, summary.ExcludeContainers(sets.New[string](strings.Split(excludeContainers, ",")...)))
}

// use binary SI units for memory
summary.UseMemoryBinarySI(summaryUseMemoryBinarySI)

summarizer := summary.NewSummarizer(opts...)
data, err := summarizer.GetSummary()
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions pkg/dashboard/dashboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ func getVPAData(opts Options, namespace, costPerCPU, costPerGB string) (summary.
summary.ForNamespace(namespace),
summary.ForVPAsWithLabels(filterLabels),
summary.ExcludeContainers(opts.ExcludedContainers),
summary.UseMemoryBinarySI(opts.UseMemoryBinarySI),
)

vpaData, err := summarizer.GetSummary()
Expand Down Expand Up @@ -149,6 +150,7 @@ func getVPAData(opts Options, namespace, costPerCPU, costPerGB string) (summary.
}
}
}

return vpaData, nil
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/dashboard/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type Options struct {
ShowAllVPAs bool
InsightsHost string
EnableCost bool
UseMemoryBinarySI bool
}

// default options for the dashboard
Expand All @@ -30,6 +31,7 @@ func defaultOptions() *Options {
OnByDefault: false,
ShowAllVPAs: false,
EnableCost: true,
UseMemoryBinarySI: false,
}
}

Expand Down Expand Up @@ -84,3 +86,9 @@ func EnableCost(enableCost bool) Option {
opts.EnableCost = enableCost
}
}

func UseMemoryBinarySI(useMemoryBinarySI bool) Option {
return func(opts *Options) {
opts.UseMemoryBinarySI = useMemoryBinarySI
}
}
8 changes: 8 additions & 0 deletions pkg/summary/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type options struct {
namespace string
vpaLabels map[string]string
excludedContainers sets.Set[string]
useMemoryBinarySI bool
}

// defaultOptions for a Summarizer
Expand Down Expand Up @@ -52,3 +53,10 @@ func ForVPAsWithLabels(vpaLabels map[string]string) Option {
opts.vpaLabels = vpaLabels
}
}

// UseMemoryBinarySI is an Option for using binary SI units for memory (base 2) instead of decimal SI units (base 10)
func UseMemoryBinarySI(useMemoryBinarySI bool) Option {
return func(opts *options) {
opts.useMemoryBinarySI = useMemoryBinarySI
}
}
12 changes: 6 additions & 6 deletions pkg/summary/summary.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,12 +212,12 @@ func (s Summarizer) GetSummary() (Summary, error) {
if c.Name == containerRecommendation.ContainerName {
cSummary = ContainerSummary{
ContainerName: containerRecommendation.ContainerName,
UpperBound: utils.FormatResourceList(containerRecommendation.UpperBound),
LowerBound: utils.FormatResourceList(containerRecommendation.LowerBound),
Target: utils.FormatResourceList(containerRecommendation.Target),
UncappedTarget: utils.FormatResourceList(containerRecommendation.UncappedTarget),
Limits: utils.FormatResourceList(c.Resources.Limits),
Requests: utils.FormatResourceList(c.Resources.Requests),
UpperBound: utils.FormatResourceList(containerRecommendation.UpperBound, s.useMemoryBinarySI),
LowerBound: utils.FormatResourceList(containerRecommendation.LowerBound, s.useMemoryBinarySI),
Target: utils.FormatResourceList(containerRecommendation.Target, s.useMemoryBinarySI),
UncappedTarget: utils.FormatResourceList(containerRecommendation.UncappedTarget, s.useMemoryBinarySI),
Limits: utils.FormatResourceList(c.Resources.Limits, s.useMemoryBinarySI),
Requests: utils.FormatResourceList(c.Resources.Requests, s.useMemoryBinarySI),
}
klog.V(6).Infof("Resources for %s/%s/%s: Requests: %v Limits: %v", wSummary.ControllerType, wSummary.ControllerName, c.Name, cSummary.Requests, cSummary.Limits)
wSummary.Containers[cSummary.ContainerName] = cSummary
Expand Down
45 changes: 40 additions & 5 deletions pkg/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,21 +77,56 @@ func Difference(a, b []string) (diff []string) {
return
}

const (
Kibibyte = 1024
Mebibyte = Kibibyte * 1024
Gibibyte = Mebibyte * 1024
Tebibyte = Gibibyte * 1024
)

func FormatToBinarySI(actual resource.Quantity, scale resource.Scale) resource.Quantity {
var scaleValue int64
switch scale {
case resource.Kilo:
scaleValue = Kibibyte
case resource.Mega:
scaleValue = Mebibyte
case resource.Giga:
scaleValue = Gibibyte
case resource.Tera:
scaleValue = Tebibyte
Comment on lines +90 to +97
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed the scaled used below

resource.Kilo,
resource.Mega,
resource.Giga,
resource.Tera,

default:
scaleValue = Mebibyte
}
value := actual.Value() / scaleValue
if actual.Value()%scaleValue != 0 {
value++
}
return *resource.NewQuantity(value*scaleValue, resource.BinarySI)
}

// FormatResourceList scales the units of a ResourceList so that they are
// human readable
func FormatResourceList(rl v1.ResourceList) v1.ResourceList {
func FormatResourceList(rl v1.ResourceList, useBinarySI bool) v1.ResourceList {
memoryScales := []resource.Scale{
resource.Kilo,
resource.Mega,
resource.Giga,
resource.Tera,
}
if mem, exists := rl[v1.ResourceMemory]; exists {
i := 0
maxAllowableStringLen := 5
for len(mem.String()) > maxAllowableStringLen && i < len(memoryScales)-1 {
maxLength := 5
if len(mem.String()) <= maxLength {
return rl
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since RoundUp is the issue in this case I decided to simply return early here 🤔

}
for i := 0; i < len(memoryScales); i++ {
mem.RoundUp(memoryScales[i])
i++
if len(mem.String()) <= maxLength {
if useBinarySI {
mem = FormatToBinarySI(mem, memoryScales[i])
}
break
}
}
rl[v1.ResourceMemory] = mem
}
Expand Down
27 changes: 19 additions & 8 deletions pkg/utils/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
)

Expand Down Expand Up @@ -83,18 +83,19 @@ var testDifferenceCases = []struct {

func TestFormatResourceList(t *testing.T) {
for _, tc := range testFormatResourceCases {
res := FormatResourceList(tc.testData)
res := FormatResourceList(tc.testData, tc.useMemoryBinarySI)
resource := res[tc.resourceType]
got := resource.String()
assert.Equal(t, tc.expected, got)
}
}

var testFormatResourceCases = []struct {
description string
testData v1.ResourceList
resourceType v1.ResourceName
expected string
description string
testData v1.ResourceList
resourceType v1.ResourceName
useMemoryBinarySI bool
expected string
}{
{
description: "Unmodified cpu",
Expand All @@ -117,7 +118,17 @@ var testFormatResourceCases = []struct {
testData: v1.ResourceList{
"memory": resource.MustParse("123456k"),
},
resourceType: "memory",
expected: "124M",
resourceType: "memory",
useMemoryBinarySI: false,
expected: "124M",
},
{
description: "Memory in too large of units",
testData: v1.ResourceList{
"memory": resource.MustParse("123456k"),
},
resourceType: "memory",
useMemoryBinarySI: true,
expected: "119Mi",
},
}