Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KEP-4008: CRD Validation Ratcheting #4013

Merged
merged 1 commit into from Jun 15, 2023
Merged

KEP-4008: CRD Validation Ratcheting #4013

merged 1 commit into from Jun 15, 2023

Conversation

alexzielenski
Copy link
Contributor

@alexzielenski alexzielenski commented May 16, 2023 •
edited

  • One-line PR description: Adds CRD Validation Ratcheting KEP

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 16, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2023
@alexzielenski
Copy link
Contributor Author

KEP updated with all comments addressed. Please take another look when you have time

@jpbetz
@deads2k
@sttts

@alexzielenski alexzielenski requested a review from jpbetz June 5, 2023 18:52
@alexzielenski alexzielenski mentioned this pull request Jun 5, 2023
Open
12 tasks
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2023
@jpbetz
Copy link
Contributor

jpbetz commented Jun 14, 2023

/assign @deads2k

PRR LGTM as shadow reviewer. Would you have a look?

@jpbetz
Copy link
Contributor

jpbetz commented Jun 14, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2023
deads2k
deads2k reviewed Jun 14, 2023
View reviewed changes
keps/sig-api-machinery/4008-crd-ratcheting/README.md
Pick one more of these and delete the rest.
-->

- [ ] Metrics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this enhancement called out a performance cost allowable for this feature. what metric is actually being checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template for this section says it is only required when targeting beta to a release. I would expect for beta we would add a metric to capture the total time used for ratcheting, but did not think it needed to be explicitly part of the KEP yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically the 5% cost metric would be against the total time for a request during a write, so apiserver_request_duration_seconds

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2023
@alexzielenski alexzielenski force-pushed the 4008-crd-validation branch 2 times, most recently from ec9cb7a to 9a7663e Compare June 14, 2023 22:36
johnbelamaric
johnbelamaric reviewed Jun 15, 2023
View reviewed changes
Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRR is OK for alpha but please change approver to me

keps/prod-readiness/sig-api-machinery/4008.yaml Outdated Show resolved Hide resolved
keps/sig-api-machinery/4008-crd-ratcheting/README.md

We may consider adding a metric in a future release.

In the absence of a metric, an operator may test if the feature is present by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking

The idea here is that operators want to see if their users are making using of the feature. It helps them know two things: 1) is anyone using the feature, or is it safe to turn it off; 2) if no one is using it, then it can't be (or is unlikely to be) the source of some production issue (e.g., during troubleshooting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification. We likely will add a metric to measure the total time taken for the comparison in a later release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A metric that shows when ratcheting takes place in writes would be helpful here.

@johnbelamaric
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, johnbelamaric, jpbetz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2023
@sttts
Copy link
Contributor

sttts commented Jun 15, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2023
@k8s-ci-robot k8s-ci-robot merged commit 99e6774 into kubernetes:master Jun 15, 2023
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jun 15, 2023
@sttts sttts mentioned this pull request Jun 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpbetz jpbetz jpbetz left review comments

@sttts sttts sttts left review comments

@liggitt liggitt liggitt left review comments

@benluddy benluddy benluddy left review comments

@apelisse apelisse apelisse left review comments

@deads2k deads2k deads2k left review comments

@johnbelamaric johnbelamaric johnbelamaric left review comments

@cici37 cici37 Awaiting requested review from cici37

Assignees

@jpbetz jpbetz

@sttts sttts

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@alexzielenski @sttts @jpbetz @johnbelamaric @k8s-ci-robot @liggitt @benluddy @apelisse @deads2k