From 425f694fe6c5a64f095bf4f10bbc2f39311bb66f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 18 Oct 2024 10:11:34 +0200 Subject: [PATCH] DRA CEL: log actual cost This may be useful for validating the cost estimate. --- .../dynamic-resource-allocation/cel/compile.go | 14 +++++++------- .../cel/compile_test.go | 13 ++++++++++++- .../structured/allocator.go | 7 ++++--- 3 files changed, 23 insertions(+), 11 deletions(-) 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 663e550be92..c8a33e5487a 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go @@ -198,14 +198,14 @@ func getAttributeValue(attr resourceapi.DeviceAttribute) (any, error) { var boolType = reflect.TypeOf(true) -func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (bool, error) { +func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (bool, *cel.EvalDetails, error) { // TODO (future): avoid building these maps and instead use a proxy // which wraps the underlying maps and directly looks up values. attributes := make(map[string]any) for name, attr := range input.Attributes { value, err := getAttributeValue(attr) if err != nil { - return false, fmt.Errorf("attribute %s: %w", name, err) + return false, nil, fmt.Errorf("attribute %s: %w", name, err) } domain, id := parseQualifiedName(name, input.Driver) if attributes[domain] == nil { @@ -231,19 +231,19 @@ func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (boo }, } - result, _, err := c.Program.ContextEval(ctx, variables) + result, details, err := c.Program.ContextEval(ctx, variables) if err != nil { - return false, err + return false, details, err } resultAny, err := result.ConvertToNative(boolType) if err != nil { - return false, fmt.Errorf("CEL result of type %s could not be converted to bool: %w", result.Type().TypeName(), err) + return false, details, fmt.Errorf("CEL result of type %s could not be converted to bool: %w", result.Type().TypeName(), err) } resultBool, ok := resultAny.(bool) if !ok { - return false, fmt.Errorf("CEL native result value should have been a bool, got instead: %T", resultAny) + return false, details, fmt.Errorf("CEL native result value should have been a bool, got instead: %T", resultAny) } - return resultBool, nil + return resultBool, details, nil } func mustBuildEnv() *environment.EnvSet { diff --git a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go index b0fa08f30f9..519477a84ce 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go @@ -254,7 +254,18 @@ device.attributes["dra.example.com"]["version"].isGreaterThan(semver("0.0.1")) t.Errorf("expected CEL cost %d, got %d instead", expect, actual) } - match, err := result.DeviceMatches(ctx, Device{Attributes: scenario.attributes, Capacity: scenario.capacity, Driver: scenario.driver}) + match, details, err := result.DeviceMatches(ctx, Device{Attributes: scenario.attributes, Capacity: scenario.capacity, Driver: scenario.driver}) + // details.ActualCost can be called for nil details, no need to check. + actualCost := ptr.Deref(details.ActualCost(), 0) + if scenario.expectCost > 0 { + t.Logf("actual cost %d, %d%% of worst-case estimate", actualCost, actualCost*100/scenario.expectCost) + } else { + t.Logf("actual cost %d, expected zero costs", actualCost) + if actualCost > 0 { + t.Errorf("expected zero costs for (presumably) constant expression %q, got instead %d", scenario.expression, actualCost) + } + } + if err != nil { if scenario.expectMatchError == "" { t.Fatalf("unexpected evaluation error: %v", err) diff --git a/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go b/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go index c7bdd1f4247..7ec9ca61e21 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go @@ -30,6 +30,7 @@ import ( resourcelisters "k8s.io/client-go/listers/resource/v1alpha3" "k8s.io/dynamic-resource-allocation/cel" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ) // ClaimLister returns a subset of the claims that a @@ -665,11 +666,11 @@ func (alloc *allocator) selectorsMatch(r requestIndices, device *resourceapi.Bas return false, fmt.Errorf("claim %s: selector #%d: CEL compile error: %w", klog.KObj(alloc.claimsToAllocate[r.claimIndex]), i, expr.Error) } - matches, err := expr.DeviceMatches(alloc.ctx, cel.Device{Driver: deviceID.Driver, Attributes: device.Attributes, Capacity: device.Capacity}) + matches, details, err := expr.DeviceMatches(alloc.ctx, cel.Device{Driver: deviceID.Driver, Attributes: device.Attributes, Capacity: device.Capacity}) if class != nil { - alloc.logger.V(7).Info("CEL result", "device", deviceID, "class", klog.KObj(class), "selector", i, "expression", selector.CEL.Expression, "matches", matches, "err", err) + alloc.logger.V(7).Info("CEL result", "device", deviceID, "class", klog.KObj(class), "selector", i, "expression", selector.CEL.Expression, "matches", matches, "actualCost", ptr.Deref(details.ActualCost(), 0), "err", err) } else { - alloc.logger.V(7).Info("CEL result", "device", deviceID, "claim", klog.KObj(alloc.claimsToAllocate[r.claimIndex]), "selector", i, "expression", selector.CEL.Expression, "matches", matches, "err", err) + alloc.logger.V(7).Info("CEL result", "device", deviceID, "claim", klog.KObj(alloc.claimsToAllocate[r.claimIndex]), "selector", i, "expression", selector.CEL.Expression, "actualCost", ptr.Deref(details.ActualCost(), 0), "matches", matches, "err", err) } if err != nil {