From cde14ceda8349b207878a00e029c3e94fcbcbc23 Mon Sep 17 00:00:00 2001 From: William Poussier Date: Tue, 22 Feb 2022 18:42:45 +0100 Subject: [PATCH] fix: correct the computation of the percentage differences and stats Fix the computation of the CPU/Memory difference columns to express the result as the increase/decrease of the request in terms of the recommendation. Print the memory recommendation quantity using the same scale as the request in a human-friendly format. Scale the statistics values to the number of replicas requested in the spec of each controller. --- README.md | 4 ++ cli/command.go | 1 + cli/table.go | 88 ++++++++++++++++++++++++++--------- internal/humanize/bigbytes.go | 52 +++++++++++++++++++++ vpa/resource.go | 45 ++++-------------- vpa/resource_test.go | 64 ++++++++++++------------- vpa/target.go | 26 ++++++++++- 7 files changed, 190 insertions(+), 90 deletions(-) create mode 100644 internal/humanize/bigbytes.go diff --git a/README.md b/README.md index cbe9762..9027378 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,10 @@ It creates a single binary file for the host machine platform/architecture in th ## Usage +### How to interpret the output? + +The columns `% CPU DIFF` and `% MEMORY DIFF` represents the percentage of increase/decrease for the request in terms of the recommendation. For example, if the request is 4 CPU (4000m), and the recommendation is only 1 CPU (1000m), the difference is +300%. As a rule of thumb, you can think of positive values as "overcommitment" and negative values as "under commitment". + ### Demo The following examples were produced from a brand-new Kubernetes cluster created with [`k3d`](https://k3d.io/v5.2.2/). The `VerticalPodAutoscaler` resources were automatically created by the [`goldilocks`](https://github.com/FairwindsOps/goldilocks) operator. diff --git a/cli/command.go b/cli/command.go index cac690e..9788e45 100644 --- a/cli/command.go +++ b/cli/command.go @@ -272,6 +272,7 @@ func newTableRow(v *vpav1.VerticalPodAutoscaler, tc *vpa.TargetController, name Namespace: v.Namespace, GVK: v.GroupVersionKind(), Mode: updateModeFromSpec(v.Spec.UpdatePolicy), + Target: tc, TargetName: tc.Name, TargetGVK: tc.GroupVersionKind, Requests: rqs, diff --git a/cli/table.go b/cli/table.go index 71d7cc5..71e68c5 100644 --- a/cli/table.go +++ b/cli/table.go @@ -8,13 +8,13 @@ import ( "sort" "strings" - "github.com/dustin/go-humanize" "github.com/muesli/termenv" "github.com/olekukonko/tablewriter" "gopkg.in/inf.v0" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/runtime/schema" + "github.com/wI2L/kubectl-vpa-recommendation/internal/humanize" "github.com/wI2L/kubectl-vpa-recommendation/vpa" ) @@ -54,8 +54,10 @@ type tableRow struct { Namespace string GVK schema.GroupVersionKind Mode string + Target *vpa.TargetController TargetName string TargetGVK schema.GroupVersionKind + TargetReplicas int32 Requests vpa.ResourceQuantities Recommendations vpa.ResourceQuantities CPUDifference *float64 @@ -88,18 +90,30 @@ func (tr tableRow) toTableData(flags *Flags, isChild bool) []string { if flags.wide { rowData = append(rowData, - formatQuantity(tr.Requests.CPU), - formatQuantity(tr.Recommendations.CPU), + formatQuantity(tr.Requests.CPU), formatQuantity(tr.Recommendations.CPU), ) } rowData = append(rowData, formatPercentage(tr.CPUDifference, flags.NoColors)) if flags.wide { + var str string + d := inf.Dec{} + d.Round(tr.Recommendations.Memory.AsDec(), 0, inf.RoundUp) + b := d.UnscaledBig() + switch tr.Requests.Memory.Format { + case resource.DecimalSI: + str = humanize.BigBytes(b, 2) + case resource.BinarySI: + str = humanize.BigIBytes(b, 2) + default: + str = tr.Recommendations.Memory.String() + } rowData = append(rowData, formatQuantity(tr.Requests.Memory), - formatQuantity(tr.Recommendations.Memory), + str, ) } rowData = append(rowData, formatPercentage(tr.MemoryDifference, flags.NoColors)) + return rowData } @@ -191,35 +205,48 @@ func (t table) printStats(w io.Writer) error { t.medianQuantities, } rows := []struct { - name string - getter func(i int) *resource.Quantity - asBytes bool + name string + quantityGetter func(i int) *resource.Quantity + asBytes bool }{ - {"CPU Recommendations (# cores)", func(i int) *resource.Quantity { return t[i].Recommendations.CPU }, false}, {"CPU Requests (# cores)", func(i int) *resource.Quantity { return t[i].Requests.CPU }, false}, - {"MEM Recommendations (IEC/SI)", func(i int) *resource.Quantity { return t[i].Recommendations.Memory }, true}, + {"CPU Recommendations (# cores)", func(i int) *resource.Quantity { return t[i].Recommendations.CPU }, false}, {"MEM Requests (IEC/SI)", func(i int) *resource.Quantity { return t[i].Requests.Memory }, true}, + {"MEM Recommendations (IEC/SI)", func(i int) *resource.Quantity { return t[i].Recommendations.Memory }, true}, } for _, row := range rows { values := make([]string, 0, len(statFuncs)) + for _, fn := range statFuncs { - q := fn(row.getter) + scaledQuantity := func(i int) *resource.Quantity { + q := row.quantityGetter(i) + + // Scale the quantity according to the number + // of replicas declared in the controller's spec. + var replicas int64 + n, err := t[i].Target.ReplicasCount() + if err != nil { + replicas = 1 + } else { + replicas = n + } + return multiplyQuantity(q, replicas) + } + q := fn(scaledQuantity) - var str string - if q == nil { - str = tableUnsetCell - } else { + s := tableUnsetCell + if q != nil { if row.asBytes { - tmp := inf.Dec{} - tmp.Round(q.AsDec(), 0, inf.RoundUp) - big := tmp.UnscaledBig() - str = humanize.BigIBytes(big) + "/" + humanize.BigBytes(big) - str = strings.ReplaceAll(str, " ", "") + d := inf.Dec{} + d.Round(q.AsDec(), 0, inf.RoundUp) + b := d.UnscaledBig() + s = humanize.BigIBytes(b, 2) + "/" + humanize.BigBytes(b, 2) + s = strings.ReplaceAll(s, " ", "") } else { - str = q.AsDec().String() + s = q.AsDec().Round(q.AsDec(), 2, inf.RoundCeil).String() } } - values = append(values, str) + values = append(values, s) } tw.Append(append([]string{row.name}, values...)) } @@ -229,6 +256,23 @@ func (t table) printStats(w io.Writer) error { return nil } +func multiplyQuantity(q *resource.Quantity, n int64) *resource.Quantity { + if q == nil || n == 0 { + return nil + } + if n == 1 { + return q + } + // The resource.Quantity type does not define a + // multiplication method, so instead we add the + // same amount n-1 times. + ret := q.DeepCopy() + for i := 0; int64(i) < n-1; i++ { + ret.Add(*q) + } + return &ret +} + func (t table) sumQuantities(column func(i int) *resource.Quantity) *resource.Quantity { var sum resource.Quantity for i := range t { @@ -279,7 +323,7 @@ func (t table) medianQuantities(column func(i int) *resource.Quantity) *resource q := values[l/2-1] q.Add(*(values[l/2])) tmp := inf.Dec{} - tmp.QuoRound(q.AsDec(), inf.NewDec(2, 0), 3, inf.RoundDown) + tmp.QuoRound(q.AsDec(), inf.NewDec(2, 0), 2, inf.RoundUp) return resource.NewDecimalQuantity(tmp, resource.DecimalSI) } diff --git a/internal/humanize/bigbytes.go b/internal/humanize/bigbytes.go new file mode 100644 index 0000000..17be357 --- /dev/null +++ b/internal/humanize/bigbytes.go @@ -0,0 +1,52 @@ +package humanize + +import ( + "fmt" + "math/big" +) + +var ( + bigSIExp = big.NewInt(1000) + bigIECExp = big.NewInt(1024) +) + +// BigBytes produces a human-readable representation of an SI size. +func BigBytes(s *big.Int, precision int) string { + sizes := []string{"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"} + return humanizeBigBytes(s, bigSIExp, sizes, precision) +} + +// BigIBytes produces a human-readable representation of an IEC size. +func BigIBytes(s *big.Int, precision int) string { + sizes := []string{"B", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi", "Yi"} + return humanizeBigBytes(s, bigIECExp, sizes, precision) +} + +var ten = big.NewInt(10) + +func humanizeBigBytes(s, base *big.Int, sizes []string, precision int) string { + if s.Cmp(ten) < 0 { + return fmt.Sprintf("%d B", s) + } + c := (&big.Int{}).Set(s) + val, mag := orderOfMagnitude(c, base, len(sizes)-1) + suffix := sizes[mag] + f := "%.0f %s" + if val < 10 { + f = fmt.Sprintf("%%.%df %%s", precision) + } + return fmt.Sprintf(f, val, suffix) +} + +func orderOfMagnitude(n, b *big.Int, max int) (float64, int) { + mag := 0 + m := &big.Int{} + for n.Cmp(b) >= 0 { + n.DivMod(n, b, m) + mag++ + if mag == max && max >= 0 { + break + } + } + return float64(n.Int64()) + (float64(m.Int64()) / float64(b.Int64())), mag +} diff --git a/vpa/resource.go b/vpa/resource.go index abfe969..2288850 100644 --- a/vpa/resource.go +++ b/vpa/resource.go @@ -2,14 +2,10 @@ package vpa import ( "math" - "strconv" - "gopkg.in/inf.v0" "k8s.io/apimachinery/pkg/api/resource" ) -const precision = 2 - // ResourceQuantities is a pair of resource quantities // that can represent the recommendations of a VerticalPodAutoscaler // of the requests of a pod's container. @@ -18,39 +14,18 @@ type ResourceQuantities struct { Memory *resource.Quantity } -// DiffQuantitiesAsPercent return the difference between -// two quantities expressed as a percentage of each other, -// where x is the request and y the recommendation. -func DiffQuantitiesAsPercent(x, y *resource.Quantity) *float64 { - if x == nil || y == nil || x.IsZero() || y.IsZero() { +// DiffQuantitiesAsPercent return the difference between two +// quantities. The return value is expressed as the increase/decrease +// of the request in terms of the recommendation. +func DiffQuantitiesAsPercent(req, rec *resource.Quantity) *float64 { + if req == nil || rec == nil || req.IsZero() || rec.IsZero() { return nil } - ai, oka := x.AsInt64() - bi, okb := y.AsInt64() - - if oka && okb { - f := (float64(ai) - float64(bi)) / float64(ai) * 100 - exp := powFloat64(10, precision) - // Round down to 'precision' decimal places. - f = math.Floor(f*exp) / exp - return &f - } - ad := x.AsDec() - bd := y.AsDec() - - p := &inf.Dec{} - p.Sub(ad, bd) - p.QuoRound(p, ad, precision+precision, inf.RoundDown) - p.Mul(p, inf.NewDec(100, 0)) - p.Round(p, precision, inf.RoundDown) + xf := req.AsApproximateFloat64() + yf := rec.AsApproximateFloat64() - f, err := strconv.ParseFloat(p.String(), 64) - if err != nil { - return nil - } - return &f -} + p := (xf - yf) / yf * 100.0 + p = math.Round(p*100) / 100 // round nearest -func powFloat64(x, y int) float64 { - return math.Pow(float64(x), float64(y)) + return &p } diff --git a/vpa/resource_test.go b/vpa/resource_test.go index 9106c9e..697ead3 100644 --- a/vpa/resource_test.go +++ b/vpa/resource_test.go @@ -9,60 +9,60 @@ import ( func TestDiffQuantitiesAsPercent(t *testing.T) { testCases := []struct { - X *resource.Quantity - Y *resource.Quantity - P *float64 + Req *resource.Quantity + Rec *resource.Quantity + P *float64 }{ { - X: resource.NewQuantity(0, resource.DecimalSI), - Y: resource.NewQuantity(25, resource.DecimalSI), - P: nil, + Req: resource.NewQuantity(4, resource.DecimalSI), + Rec: resource.NewQuantity(1, resource.DecimalSI), + P: pointer.Float64(300.00), }, { - X: resource.NewQuantity(25, resource.DecimalSI), - Y: resource.NewQuantity(0, resource.DecimalSI), - P: nil, + Req: resource.NewQuantity(100, resource.DecimalSI), + Rec: resource.NewQuantity(25, resource.DecimalSI), + P: pointer.Float64(300.00), }, { - X: resource.NewQuantity(100, resource.DecimalSI), - Y: resource.NewQuantity(25, resource.DecimalSI), - P: pointer.Float64(75.00), + Req: resource.NewQuantity(444, resource.BinarySI), + Rec: resource.NewQuantity(1000, resource.BinarySI), + P: pointer.Float64(-55.60), }, { - X: resource.NewQuantity(444, resource.BinarySI), - Y: resource.NewQuantity(1000, resource.BinarySI), - P: pointer.Float64(-125.23), + Req: resource.NewQuantity(1000, resource.BinarySI), + Rec: resource.NewQuantity(444, resource.BinarySI), + P: pointer.Float64(125.23), }, { - X: resource.NewQuantity(1000, resource.BinarySI), - Y: resource.NewQuantity(444, resource.BinarySI), - P: pointer.Float64(55.60), + Req: resource.NewScaledQuantity(8, resource.Peta), + Rec: resource.NewScaledQuantity(24, resource.Peta), + P: pointer.Float64(-66.67), }, { - X: resource.NewScaledQuantity(8, resource.Peta), - Y: resource.NewScaledQuantity(24, resource.Peta), - P: pointer.Float64(-200.00), + Req: resource.NewScaledQuantity(128, resource.Exa), + Rec: resource.NewScaledQuantity(512, resource.Exa), + P: pointer.Float64(-75.00), }, { - X: resource.NewScaledQuantity(128, resource.Exa), - Y: resource.NewScaledQuantity(512, resource.Exa), - P: pointer.Float64(-300.00), + Req: resource.NewScaledQuantity(666, resource.Giga), + Rec: resource.NewScaledQuantity(32, resource.Exa), + P: pointer.Float64(-100.00), }, { - X: resource.NewScaledQuantity(666, resource.Giga), - Y: resource.NewScaledQuantity(32, resource.Exa), - P: pointer.Float64(-4804804704.80), + Req: resource.NewScaledQuantity(32, resource.Exa), + Rec: resource.NewScaledQuantity(666, resource.Giga), + P: pointer.Float64(4804804704.80), }, { - X: resource.NewScaledQuantity(32, resource.Exa), - Y: resource.NewScaledQuantity(666, resource.Giga), - P: pointer.Float64(99.99), + Req: resource.NewMilliQuantity(100, resource.DecimalSI), + Rec: resource.NewMilliQuantity(25, resource.DecimalSI), + P: pointer.Float64(300.00), }, } for _, tc := range testCases { - t.Logf("X: %s, Y: %s", tc.X, tc.Y) + t.Logf("Req: %s, Rec: %s", tc.Req, tc.Rec) - p := DiffQuantitiesAsPercent(tc.X, tc.Y) + p := DiffQuantitiesAsPercent(tc.Req, tc.Rec) if p == nil { if tc.P != nil { t.Errorf("got nil result, want %.2f", *tc.P) diff --git a/vpa/target.go b/vpa/target.go index a40a21e..7793914 100644 --- a/vpa/target.go +++ b/vpa/target.go @@ -178,6 +178,30 @@ func resolveLabelSelector(obj *unstructuredv1.Unstructured) (*metav1.LabelSelect return selector, nil } +// ReplicasCount returns the number of replicas of the controller. +// It is used to scale the resource/recommendation statistics to get +// real usage values that reflect the number of pods schedules for +// each target controller. +func (tc *TargetController) ReplicasCount() (int64, error) { + fields := []string{ + "spec", + "replicas", + } + var err error + fields, err = genericControllerSpecPath(tc.controllerObj.GetKind(), fields) + if err != nil { + return 0, err + } + replicas, ok, err := unstructuredv1.NestedInt64(tc.controllerObj.Object, fields...) + if err != nil { + return 0, fmt.Errorf("nested field has invalid type: %w", err) + } + if !ok { + return 0, fmt.Errorf("nested field with path %s not found", strings.Join(fields, ".")) + } + return replicas, nil +} + func genericControllerSpecPath(kind string, fields []string) ([]string, error) { switch wellKnownControllerKind(kind) { case ds, deploy, job, rs, rc, sts: @@ -197,7 +221,7 @@ func genericControllerSpecPath(kind string, fields []string) ([]string, error) { func decodeNestedFieldInto(obj *unstructuredv1.Unstructured, fields []string, into interface{}) error { nmap, ok, err := unstructuredv1.NestedMap(obj.Object, fields...) if err != nil { - return fmt.Errorf("nested field has invalid type") + return fmt.Errorf("nested field has invalid type: %w", err) } if !ok { return fmt.Errorf("nested field with path %s not found", strings.Join(fields, "."))