Update CEL runtime cost limit #108595

cici37 · 2022-03-08T20:05:15Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR is to update CEL runtime cost limit

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fedebongio · 2022-03-08T21:15:23Z

/cc @jpbetz
/triage accepted

jpbetz · 2022-03-08T21:32:00Z

/lgtm

This is roughly what I was expecting for an initial limit given our current heuristic that 1 cost ~= 50ns. It seems high enough that it should be primarily a backstop for runaway execution.

cc @liggitt @DangerOnTheRanger I expect we will refine this number before the release, but does this seem like a good starting point to you both?

DangerOnTheRanger · 2022-03-09T02:34:17Z

Yeah, I think especially that we should have an ample amount of time to change that limit if need be, it seems like a good place to start to me.

liggitt · 2022-03-10T15:21:52Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go

+	// perCallLimit specify the actual cost limit per CEL validation call
+	// current perCallLimit gives roughly one second for each expression validation call
+	perCallLimit = 20000000
+
+	// RuntimeCELCostBudget is the overall cost budget for runtime CEL validation cost per CustomResource
+	// current RuntimeCELCostBudget gives roughly 10 seconds for CR validation
+	RuntimeCELCostBudget = 200000000


since these represent dedicated CPU time, both of these are about an order of magnitude higher than I expected... are we really ok with 10 seconds of devoted CPU time per custom resource write validation?

This number is fairly high now. I was thinking to start with a higher number and reduce based on performance run. Would you have any suggestions on the CPU time custom resource could consume?

at most, I would expect fractions of a second of dedicated CPU per call and less than a second of dedicated CPU for validating the entire resource. I wouldn't start any higher than that, and would actually like to see that ramp even further down as we prove you can do significant amounts of complex validation with much lower limits. I'd probably drop an order of magnitude from each of these to start.

I have updated to start with 2000000 for PerCallLimit and 20000000 for RuntimeCELCostBudget. cc @jpbetz @DangerOnTheRanger

liggitt · 2022-03-10T15:22:35Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go

@@ -92,34 +93,55 @@ func validator(s *schema.Structural, isResourceRoot bool) *Validator {
 }

 // Validate validates all x-kubernetes-validations rules in Validator against obj and returns any errors.
-func (s *Validator) Validate(fldPath *field.Path, sts *schema.Structural, obj interface{}) field.ErrorList {


I'm ignoring changes to this file and assuming this will be rebased on top of #108482

Yes it is going to be rebased on #108482 . This PR is only for updating the budget. Thanks

cici37 · 2022-03-14T17:40:54Z

/retest

cici37 · 2022-03-14T19:05:02Z

/test pull-kubernetes-e2e-kind-ipv6

k8s-triage-robot · 2022-03-14T19:11:19Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

liggitt · 2022-03-14T19:12:15Z

these numbers look like plausible starting points... is there a tracking issue or spreadsheet item for finalizing these for 1.24?

cici37 · 2022-03-14T22:13:24Z

these numbers look like plausible starting points... is there a tracking issue or spreadsheet item for finalizing these for 1.24?

Here is the umbrella issue for tracking: #107573
Here is the spreadsheet for tracking the progress

liggitt · 2022-03-15T00:23:31Z

/lgtm
/approve
/retest

k8s-ci-robot · 2022-03-15T00:23:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-triage-robot · 2022-03-15T04:51:18Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

yangjunmyfm192085

/retest

k8s-triage-robot · 2022-03-15T10:37:18Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from jpbetz and yue9944882 March 8, 2022 20:05

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Mar 8, 2022

cici37 mentioned this pull request Mar 8, 2022

Add CEL runtime cost into CR validation #108482

Merged

k8s-ci-robot assigned jpbetz Mar 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2022

liggitt reviewed Mar 10, 2022

View reviewed changes

cici37 force-pushed the updateLimit branch from 68fe0b2 to de0d43e Compare March 14, 2022 17:06

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 14, 2022

Update cost budget

3167752

cici37 force-pushed the updateLimit branch from de0d43e to 3167752 Compare March 14, 2022 22:11

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 14, 2022

k8s-ci-robot assigned liggitt Mar 15, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2022

yangjunmyfm192085 reviewed Mar 15, 2022

View reviewed changes

k8s-ci-robot merged commit 4bca479 into kubernetes:master Mar 15, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 15, 2022

This was referenced Jul 26, 2022

Promote feature CustomResourceValidationExpressions to beta #111158

Closed

Promote feature CustomResourceValidationExpressions to beta #111524

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update CEL runtime cost limit #108595

Update CEL runtime cost limit #108595

cici37 commented Mar 8, 2022 •

edited

fedebongio commented Mar 8, 2022

jpbetz commented Mar 8, 2022

DangerOnTheRanger commented Mar 9, 2022 •

edited

liggitt Mar 10, 2022

cici37 Mar 10, 2022

liggitt Mar 11, 2022 •

edited

cici37 Mar 14, 2022

liggitt Mar 10, 2022

cici37 Mar 10, 2022

cici37 commented Mar 14, 2022

cici37 commented Mar 14, 2022

k8s-triage-robot commented Mar 14, 2022

liggitt commented Mar 14, 2022

cici37 commented Mar 14, 2022

liggitt commented Mar 15, 2022

k8s-ci-robot commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

yangjunmyfm192085 left a comment

k8s-triage-robot commented Mar 15, 2022

Update CEL runtime cost limit #108595

Update CEL runtime cost limit #108595

Conversation

cici37 commented Mar 8, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fedebongio commented Mar 8, 2022

jpbetz commented Mar 8, 2022

DangerOnTheRanger commented Mar 9, 2022 • edited

liggitt Mar 10, 2022

Choose a reason for hiding this comment

cici37 Mar 10, 2022

Choose a reason for hiding this comment

liggitt Mar 11, 2022 • edited

Choose a reason for hiding this comment

cici37 Mar 14, 2022

Choose a reason for hiding this comment

liggitt Mar 10, 2022

Choose a reason for hiding this comment

cici37 Mar 10, 2022

Choose a reason for hiding this comment

cici37 commented Mar 14, 2022

cici37 commented Mar 14, 2022

k8s-triage-robot commented Mar 14, 2022

liggitt commented Mar 14, 2022

cici37 commented Mar 14, 2022

liggitt commented Mar 15, 2022

k8s-ci-robot commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

k8s-triage-robot commented Mar 15, 2022

cici37 commented Mar 8, 2022 •

edited

DangerOnTheRanger commented Mar 9, 2022 •

edited

liggitt Mar 11, 2022 •

edited