Feature/validating admission policy/metrics improvement #126124
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
approved
|
/retest |
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
/assign @jpbetz Thanks :) |
|
@logicalhan Could we get a SIG insturmentation review for this one? |
|
cc @richabanker |
|
For API Machinery aspects: This looks like a useful improvement. For SIG Instrumentation: Is this change acceptable for the stability level of this metric? |
|
Hi @richabanker , thanks for the comment. I do want to promote the stability level of the existing metrics from alpha to beta in a followup pr. Wondering if there is any requirement regarding with the metrics promotion? Any required soak period or related feature status? Thank you! |
|
I am not aware of any documented set of requirements that need to be followed in order to graduate a metric from alpha -> beta -> stable, but maybe @logicalhan has more information on this. I could only find this section on how to go about proposing updating the stability level of a metric. |
|
/sig instrumentation |
k8s-ci-robot
added
the
sig/instrumentation
|
Hi @richabanker, would you have time to review this pr? Thanks |
| var ErrCompilation = fmt.Errorf("%w: compilation error", ErrInvalid) | ||
|
|
||
| // ErrOutOfBudget is the basic error that occurs when the expression fails due to | ||
| // exceeding budget. |
There was a problem hiding this comment.
Is it worth expanding on what "budget" means here? Apologies if this is something tied to CEL that I don't know.
There was a problem hiding this comment.
Thank you so much for the review!
Yes it is a cel thing and we use it in returned err message as well :)
|
|
||
| const ( | ||
| // ValidationCompileError indicates that the expression fails to compile. | ||
| ValidationCompileError ValidationErrorType = "compile_error" |
There was a problem hiding this comment.
Does it make sense to move these also to staging/src/k8s.io/apiserver/pkg/cel/errors.go ?
There was a problem hiding this comment.
I added into here since it is only used by metrics. I could add it into metrics/errors.go in my next metric pr is that makes sense :)
| } | ||
|
|
||
| _, err = cel.AstToCheckedExpr(ast) | ||
| if err != nil { | ||
| // should be impossible since env.Compile returned no issues | ||
| return resultError("unexpected compilation error: "+err.Error(), apiservercel.ErrorTypeInternal) | ||
| return resultError("unexpected compilation error: "+err.Error(), apiservercel.ErrorTypeInternal, nil) |
There was a problem hiding this comment.
Since we are passing nil here, we should ensure that we do nil checks for its usages, I couldn't find any instance in this PR but was just something that stood out to me. Maybe replacing nil here with a "apiservercel.UnknownError" or something would be better? Leave it to you.
There was a problem hiding this comment.
This is the code we would never expect to hit :) So we don't have a specific err type for this :)
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: b6106815d334a6f96ea2c753521cf7f4a4bc039d
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cici37, richabanker 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:
Approvers can indicate their approval by writing |
cici37
deleted the
feature/validating-admission-policy/metrics-improvement
branch
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Improve the VAP metrics by wrapping the error clearly.
This PR is to merge changes from #124330
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Current PR includes commits from #126123. Could be reviewed after #126123 is merged. Thanks.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: