From 00faa5e7ae978d4e75e80649cdb2cc46f98ba152 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 22 Jan 2025 08:45:32 +0100 Subject: [PATCH] DRA CEL: skip estimating the cost in the scheduler Compiling a CEL expression used to do the cost estimation, whether the caller needed the result or not. Now callers can skip it. The scheduler does that, through the CEL cache. The main advantage is that failures in the estimator (like panics) are limited to the apiserver. Performance in the scheduler is not expected to benefit much because compilation results are cached. --- .../dynamic-resource-allocation/cel/cache.go | 4 ++- .../cel/cache_test.go | 6 +++++ .../cel/compile.go | 26 ++++++++++++------- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/cache.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/cache.go index 2868886c5bb..e807ba2b9ec 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/cache.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/cache.go @@ -43,6 +43,8 @@ func NewCache(maxCacheEntries int) *Cache { // GetOrCompile checks whether the cache already has a compilation result // and returns that if available. Otherwise it compiles, stores successful // results and returns the new result. +// +// Cost estimation is disabled. func (c *Cache) GetOrCompile(expression string) CompilationResult { // Compiling a CEL expression is expensive enough that it is cheaper // to lock a mutex than doing it several times in parallel. @@ -55,7 +57,7 @@ func (c *Cache) GetOrCompile(expression string) CompilationResult { return *cached } - expr := GetCompiler().CompileCELExpression(expression, Options{}) + expr := GetCompiler().CompileCELExpression(expression, Options{DisableCostEstimation: true}) if expr.Error == nil { c.add(expression, &expr) } diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/cache_test.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/cache_test.go index 3c68a94db82..5ef8bbe2d62 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/cache_test.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/cache_test.go @@ -18,6 +18,7 @@ package cel import ( "fmt" + "math" "sync" "testing" @@ -73,6 +74,11 @@ func TestCacheSemantic(t *testing.T) { if resultFalse == resultFalseAgain { t.Fatal("result of compiling `false` should have been evicted from the cache") } + + // Cost estimation must be off (not needed by scheduler). + if resultFalseAgain.MaxCost != math.MaxUint64 { + t.Error("cost estimation should have been disabled, was enabled") + } } func TestCacheConcurrency(t *testing.T) { diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go index dea295e9c17..d59f7d7d4bb 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "math" "reflect" "strings" "sync" @@ -120,6 +121,10 @@ type Options struct { // CostLimit allows overriding the default runtime cost limit [resourceapi.CELSelectorExpressionMaxCost]. CostLimit *uint64 + + // DisableCostEstimation can be set to skip estimating the worst-case CEL cost. + // If disabled or after an error, [CompilationResult.MaxCost] will be set to [math.Uint64]. + DisableCostEstimation bool } // CompileCELExpression returns a compiled CEL expression. It evaluates to bool. @@ -133,6 +138,7 @@ func (c compiler) CompileCELExpression(expression string, options Options) Compi Detail: errorString, }, Expression: expression, + MaxCost: math.MaxUint64, } } @@ -141,10 +147,6 @@ func (c compiler) CompileCELExpression(expression string, options Options) Compi return resultError(fmt.Sprintf("unexpected error loading CEL environment: %v", err), apiservercel.ErrorTypeInternal) } - // We don't have a SizeEstimator. The potential size of the input (= a - // device) is already declared in the definition of the environment. - estimator := c.newCostEstimator() - ast, issues := env.Compile(expression) if issues != nil { return resultError("compilation failed: "+issues.String(), apiservercel.ErrorTypeInvalid) @@ -176,15 +178,21 @@ func (c compiler) CompileCELExpression(expression string, options Options) Compi OutputType: ast.OutputType(), Environment: env, emptyMapVal: env.CELTypeAdapter().NativeToValue(map[string]any{}), + MaxCost: math.MaxUint64, } - costEst, err := env.EstimateCost(ast, estimator) - if err != nil { - compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInternal, Detail: "cost estimation failed: " + err.Error()} - return compilationResult + if !options.DisableCostEstimation { + // We don't have a SizeEstimator. The potential size of the input (= a + // device) is already declared in the definition of the environment. + estimator := c.newCostEstimator() + costEst, err := env.EstimateCost(ast, estimator) + if err != nil { + compilationResult.Error = &apiservercel.Error{Type: apiservercel.ErrorTypeInternal, Detail: "cost estimation failed: " + err.Error()} + return compilationResult + } + compilationResult.MaxCost = costEst.Max } - compilationResult.MaxCost = costEst.Max return compilationResult }