From f9534391b2e73e04c2e994930423e9b1193ca1c7 Mon Sep 17 00:00:00 2001 From: Sebastian Widmer Date: Mon, 29 Jul 2024 11:17:08 +0200 Subject: [PATCH] Add a threshold for the ratio warning to not notify on small violations (#109) --- config.go | 7 ++++++ config_test.go | 31 +++++++++++++++++++++++ controllers/ratio_controller.go | 8 +++--- controllers/ratio_controller_test.go | 37 +++++++++++++++++++++++++--- main.go | 2 ++ ratio/ratio.go | 11 +++++++-- ratio/ratio_test.go | 18 +++++++++++--- webhooks/ratio_validator.go | 8 +++--- webhooks/ratio_validator_test.go | 21 ++++++++++++++++ 9 files changed, 129 insertions(+), 14 deletions(-) diff --git a/config.go b/config.go index 76728c7..320e02f 100644 --- a/config.go +++ b/config.go @@ -6,6 +6,7 @@ import ( "github.com/appuio/appuio-cloud-agent/limits" "go.uber.org/multierr" + "gopkg.in/inf.v0" "k8s.io/apimachinery/pkg/api/resource" "sigs.k8s.io/yaml" ) @@ -27,6 +28,12 @@ type Config struct { // MemoryPerCoreLimits is the fair use limit of memory usage per CPU core // It is possible to select limits by node selector labels MemoryPerCoreLimits limits.Limits + // MemoryPerCoreWarnThreshold is the threshold at which a warning is emitted if the memory per core limit is exceeded. + // Should be a decimal number resembling a percentage (e.g. "0.8" for 80%), represented as a string. + // The limit is multiplied by the optional threshold before comparison. + // A threshold of 0.95 would mean that the warnings are emitted if the ratio is below 95% of the limit. + // Thus adding a leniency of 5% to the limit. + MemoryPerCoreWarnThreshold *inf.Dec // Privileged* is a list of the given type allowed to bypass restrictions. // Wildcards are supported (e.g. "system:serviceaccount:default:*" or "cluster-*-operator"). diff --git a/config_test.go b/config_test.go index e96fad6..d54f9e7 100644 --- a/config_test.go +++ b/config_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/inf.v0" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -82,3 +83,33 @@ func Test_Config_MemoryPerCoreLimits(t *testing.T) { }) } } + +func Test_Config_MemoryPerCoreWarnThreshold(t *testing.T) { + tc := []struct { + yaml string + expected *inf.Dec + }{ + { + yaml: `MemoryPerCoreWarnThreshold: "0.95"`, + expected: inf.NewDec(95, 2), + }, { + expected: nil, + }, + } + + for _, tC := range tc { + t.Run(tC.yaml, func(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + configPath := filepath.Join(tmp, "config.yaml") + require.NoError(t, os.WriteFile(configPath, []byte(tC.yaml), 0o644)) + + c, warnings, err := ConfigFromFile(configPath) + assert.Len(t, warnings, 0) + require.NoError(t, err) + + assert.Equal(t, tC.expected, c.MemoryPerCoreWarnThreshold) + }) + } +} diff --git a/controllers/ratio_controller.go b/controllers/ratio_controller.go index d28576f..6f09d79 100644 --- a/controllers/ratio_controller.go +++ b/controllers/ratio_controller.go @@ -7,6 +7,7 @@ import ( "github.com/appuio/appuio-cloud-agent/limits" "github.com/appuio/appuio-cloud-agent/ratio" + "gopkg.in/inf.v0" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" @@ -24,8 +25,9 @@ type RatioReconciler struct { Recorder record.EventRecorder Scheme *runtime.Scheme - Ratio ratioFetcher - RatioLimits limits.Limits + Ratio ratioFetcher + RatioLimits limits.Limits + RatioWarnThreshold *inf.Dec } type ratioFetcher interface { @@ -62,7 +64,7 @@ func (r *RatioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl continue } - if ratio.Below(*limit) { + if ratio.Below(*limit, r.RatioWarnThreshold) { l.Info("recording warn event: ratio too low") if err := r.warnPod(ctx, req.Name, req.Namespace, ratio, nodeSel, limit); err != nil { diff --git a/controllers/ratio_controller_test.go b/controllers/ratio_controller_test.go index 470b72d..42d2651 100644 --- a/controllers/ratio_controller_test.go +++ b/controllers/ratio_controller_test.go @@ -8,6 +8,7 @@ import ( "github.com/appuio/appuio-cloud-agent/ratio" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/inf.v0" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -45,6 +46,28 @@ func TestRatioReconciler_Warn(t *testing.T) { require.Len(t, recorder.Events, 2) } +func TestRatioReconciler_Threshold_Ok(t *testing.T) { + recorder := record.NewFakeRecorder(4) + _, err := prepareRatioTest(t, testRatioCfg{ + limit: resource.MustParse("4G"), + fetchMemory: resource.MustParse("4G"), + fetchCPU: resource.MustParse("1100m"), + recorder: recorder, + threshold: inf.NewDec(90, 2), + obj: []client.Object{ + testNs, + testPod, + }, + }).Reconcile(context.TODO(), ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: testNs.Name, + Name: testPod.Name, + }, + }) + assert.NoError(t, err) + requireNEvents(t, recorder, 0) +} + func TestRatioReconciler_Ok(t *testing.T) { recorder := record.NewFakeRecorder(4) _, err := prepareRatioTest(t, testRatioCfg{ @@ -129,11 +152,17 @@ func TestRatioReconciler_RecordFailed(t *testing.T) { }, }) assert.NoError(t, err) - if !assert.Len(t, recorder.Events, 0) { + requireNEvents(t, recorder, 0) +} + +func requireNEvents(t *testing.T, recorder *record.FakeRecorder, n int) { + t.Helper() + + if !assert.Len(t, recorder.Events, n) { for i := 0; i < len(recorder.Events); i++ { - e := <-recorder.Events - t.Log(e) + t.Log("Recorded event: ", <-recorder.Events) } + t.FailNow() } } @@ -142,6 +171,7 @@ type testRatioCfg struct { fetchErr error fetchCPU resource.Quantity fetchMemory resource.Quantity + threshold *inf.Dec obj []client.Object recorder record.EventRecorder } @@ -175,6 +205,7 @@ func prepareRatioTest(t *testing.T, cfg testRatioCfg) *RatioReconciler { Limit: &cfg.limit, }, }, + RatioWarnThreshold: cfg.threshold, } } diff --git a/main.go b/main.go index cec0a47..c48a4f8 100644 --- a/main.go +++ b/main.go @@ -323,6 +323,7 @@ func registerRatioController(mgr ctrl.Manager, conf Config, orgLabel string) { Ratio: &ratio.Fetcher{ Client: mgr.GetClient(), }, + RatioWarnThreshold: conf.MemoryPerCoreWarnThreshold, }, }) @@ -335,6 +336,7 @@ func registerRatioController(mgr ctrl.Manager, conf Config, orgLabel string) { Client: mgr.GetClient(), OrganizationLabel: orgLabel, }, + RatioWarnThreshold: conf.MemoryPerCoreWarnThreshold, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ratio") os.Exit(1) diff --git a/ratio/ratio.go b/ratio/ratio.go index 49bde57..aeef92b 100644 --- a/ratio/ratio.go +++ b/ratio/ratio.go @@ -92,8 +92,15 @@ func (r Ratio) Ratio() *resource.Quantity { // Below returns if the memory to CPU ratio of the recorded objects is below the given limit. // Always returns false if no CPU is requested. -func (r Ratio) Below(limit resource.Quantity) bool { - return r.Ratio() != nil && r.Ratio().Cmp(limit) < 0 +// The limit is multiplied by the optional threshold before comparison. +// If threshold is nil, it defaults to 1. +// A threshold of 0.95 would mean that the function returns true if the ratio is below 95% of the limit. +func (r Ratio) Below(limit resource.Quantity, threshold *inf.Dec) bool { + if threshold == nil { + threshold = inf.NewDec(1, 0) + } + + return r.Ratio() != nil && r.Ratio().AsDec().Cmp(inf.NewDec(0, 0).Mul(limit.AsDec(), threshold)) < 0 } // String implements Stringer to print ratio diff --git a/ratio/ratio_test.go b/ratio/ratio_test.go index 0518355..feb2666 100644 --- a/ratio/ratio_test.go +++ b/ratio/ratio_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/inf.v0" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -211,6 +212,8 @@ func TestRatio_ratio(t *testing.T) { memory string ratio string + threshold *inf.Dec + smallerThan string largerOrEqualTo string }{ @@ -252,6 +255,15 @@ func TestRatio_ratio(t *testing.T) { memory: "801Mi", ratio: "401Mi", }, + { + cpu: "1", + memory: "4Gi", + ratio: "4Gi", + + threshold: inf.NewDec(95, 2), + largerOrEqualTo: "3.9Gi", + smallerThan: "4.3Gi", + }, } for _, tc := range tcs { t.Run(fmt.Sprintf("[%s/%s=%s]", tc.memory, tc.cpu, tc.ratio), func(t *testing.T) { @@ -272,10 +284,10 @@ func TestRatio_ratio(t *testing.T) { assert.Equalf(t, tc.ratio, r.String(), "should pretty print") } if tc.smallerThan != "" { - assert.Truef(t, r.Below(resource.MustParse(tc.smallerThan)), "should be smaller than %s", tc.smallerThan) + assert.Truef(t, r.Below(resource.MustParse(tc.smallerThan), tc.threshold), "should be smaller than %s", tc.smallerThan) } if tc.largerOrEqualTo != "" { - assert.Falsef(t, r.Below(resource.MustParse(tc.largerOrEqualTo)), "should not be smaller than %s", tc.largerOrEqualTo) + assert.Falsef(t, r.Below(resource.MustParse(tc.largerOrEqualTo), tc.threshold), "should not be smaller than %s", tc.largerOrEqualTo) } }) } @@ -310,7 +322,7 @@ func FuzzRatio(f *testing.F) { out := r.Warn(&lim, "") assert.NotEmpty(t, out) - r.Below(lim) + r.Below(lim, nil) }) }) } diff --git a/webhooks/ratio_validator.go b/webhooks/ratio_validator.go index 51d16e9..55361ba 100644 --- a/webhooks/ratio_validator.go +++ b/webhooks/ratio_validator.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" + "gopkg.in/inf.v0" admissionv1 "k8s.io/api/admission/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -30,8 +31,9 @@ type RatioValidator struct { Decoder admission.Decoder Client client.Client - Ratio ratioFetcher - RatioLimits limits.Limits + Ratio ratioFetcher + RatioLimits limits.Limits + RatioWarnThreshold *inf.Dec // DefaultNodeSelector is the default node selector to apply to pods DefaultNodeSelector map[string]string @@ -117,7 +119,7 @@ func (v *RatioValidator) Handle(ctx context.Context, req admission.Request) admi continue } - if r.Below(*limit) { + if r.Below(*limit, v.RatioWarnThreshold) { l.Info("ratio too low", "node_selector", nodeSel, "ratio", r) warnings = append(warnings, r.Warn(limit, nodeSel)) } diff --git a/webhooks/ratio_validator_test.go b/webhooks/ratio_validator_test.go index 723f1f1..1bed829 100644 --- a/webhooks/ratio_validator_test.go +++ b/webhooks/ratio_validator_test.go @@ -8,6 +8,7 @@ import ( "net/http" "strings" + "gopkg.in/inf.v0" admissionv1 "k8s.io/api/admission/v1" appsv1 "k8s.io/api/apps/v1" authenticationv1 "k8s.io/api/authentication/v1" @@ -41,6 +42,7 @@ func TestRatioValidator_Handle(t *testing.T) { mangleObject bool create bool limits limits.Limits + threshold *inf.Dec warn bool fail bool statusCode int32 @@ -303,6 +305,24 @@ func TestRatioValidator_Handle(t *testing.T) { fail: true, statusCode: http.StatusBadRequest, }, + "Allow_UnfairNamespace_WithThreshold": { + user: "appuio#foo", + namespace: "bar", + resources: []client.Object{ + podFromResources("pod1", "foo", podResource{ + {cpu: "1", memory: "4Gi"}, + }), + podFromResources("pod2", "foo", podResource{ + {cpu: "1", memory: "4Gi"}, + }), + podFromResources("slightlyunfair", "bar", podResource{ + {cpu: "1050m", memory: "4Gi"}, + }), + }, + limits: limits.Limits{{Limit: requireParseQuantity(t, "4Gi")}}, + warn: false, + threshold: inf.NewDec(95, 2), + }, } for name, tc := range tests { @@ -312,6 +332,7 @@ func TestRatioValidator_Handle(t *testing.T) { v := prepareTest(t, tc.resources...) v.RatioLimits = tc.limits + v.RatioWarnThreshold = tc.threshold ar := admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{