Skip to content

KEP-4818: Relaxed validation for allowing zero in PreStop hook sleep action#127094

Merged
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
sreeram-venkitesh:4818-allow-zero-for-prestop-hook
Oct 31, 2024
Merged

KEP-4818: Relaxed validation for allowing zero in PreStop hook sleep action#127094
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
sreeram-venkitesh:4818-allow-zero-for-prestop-hook

Conversation

@sreeram-venkitesh

@sreeram-venkitesh sreeram-venkitesh commented Sep 3, 2024

Copy link
Copy Markdown
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds a feature gate, PodLifecycleSleepActionAllowZero, to allow zero value for the sleep action in the PreStop container lifecycle hook.

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#4818

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Allows PreStop lifecycle handler's sleep action to have a zero value

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/4818

@k8s-ci-robot

k8s-ci-robot commented Sep 3, 2024

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 3, 2024
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 3, 2024
kannon92
kannon92 reviewed Sep 3, 2024
View reviewed changes
Comment thread pkg/apis/core/validation/validation.go Outdated
@sreeram-venkitesh

sreeram-venkitesh commented Sep 3, 2024

Copy link
Copy Markdown
Member Author

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 3, 2024
@sreeram-venkitesh

sreeram-venkitesh commented Sep 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@kannon92 Addressing your comment, I've updated the logic to use the feature gate set in PodValidationOptions. I've tried to describe the change in my second commit with the flow given below. The validation takes place in validateSleepAction, for which I've passed the PodValidationOptions down from validatePodSpec, through the different functions which ultimately end up calling validateHandler and thus validateSleepAction.

validatePodSpec
|__validateContainers
|__validateInitContainers
   |__validateLifecycle
   |    |__validateHandler
   |      |__validateSleepAction
   |__validateLivenessProbe
   |	  |__validateProbe
   |	     |__validateHandler
   |	        |__validateSleepAction
   |__validateReadinessProbe
   |	  |__validateProbe
   |	     |__validateHandler
   |	        |__validateSleepAction
   |__validateStartupProbe
   	  |__validateProbe
   	     |__validateHandler
   	        |__validateSleepAction

@sreeram-venkitesh sreeram-venkitesh force-pushed the 4818-allow-zero-for-prestop-hook branch from f633138 to 4508a72 Compare September 8, 2024 18:18
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2024
@sreeram-venkitesh sreeram-venkitesh marked this pull request as ready for review September 8, 2024 18:20
@sreeram-venkitesh

sreeram-venkitesh commented Sep 9, 2024

Copy link
Copy Markdown
Member Author

I'm working on fixing the tests.

@kannon92

kannon92 commented Sep 9, 2024

Copy link
Copy Markdown
Contributor

/hold

Please open a KEP first. You can leave this up but don't expect much reviews until the KEP is approved.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 9, 2024
@sreeram-venkitesh sreeram-venkitesh mentioned this pull request Sep 14, 2024
Closed
14 tasks
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/kubelet labels Oct 17, 2024
@sreeram-venkitesh sreeram-venkitesh force-pushed the 4818-allow-zero-for-prestop-hook branch from 8ccfa46 to f568239 Compare October 17, 2024 08:46
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2024
SergeyKanzhelev
SergeyKanzhelev approved these changes Oct 17, 2024
View reviewed changes

@SergeyKanzhelev SergeyKanzhelev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2024
@k8s-ci-robot

k8s-ci-robot commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: f96f63dd1bbe1afc491a7c636f4c4a5bd3f01037

@SergeyKanzhelev

SergeyKanzhelev commented Oct 17, 2024

Copy link
Copy Markdown
Member

/assign @thockin

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024
@sreeram-venkitesh
Clean kube_features.go
f1f9e7b 
Added tests, info about new feature gate in error message, fixes from review

Added basic e2e test

Added unit tests

Ran hack/update-featuregates.sh

Tolerate updates to existing resources after disabling feature gate

Added feature gate to versioned_kube_features.go

Fixed existing tests

Use PodValidationOptions for validation instead of using feature gate directly

Relaxed validation for allowing zero in prestop hook sleep action
@sreeram-venkitesh sreeram-venkitesh force-pushed the 4818-allow-zero-for-prestop-hook branch from f568239 to f1f9e7b Compare October 18, 2024 16:35
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 18, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024
@sreeram-venkitesh sreeram-venkitesh mentioned this pull request Oct 19, 2024
Merged
@sreeram-venkitesh

sreeram-venkitesh commented Oct 30, 2024

Copy link
Copy Markdown
Member Author

/retest

@thockin

thockin commented Oct 30, 2024

Copy link
Copy Markdown
Member

/approve

@k8s-ci-robot

k8s-ci-robot commented Oct 30, 2024

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SergeyKanzhelev, sreeram-venkitesh, thockin

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

The pull request process is described here

Details 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 Oct 30, 2024
@sreeram-venkitesh

sreeram-venkitesh commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

Thanks Tim!

@SergeyKanzhelev lgtm was removed after rebase

@thockin

thockin commented Oct 31, 2024

Copy link
Copy Markdown
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2024
@k8s-ci-robot

k8s-ci-robot commented Oct 31, 2024

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 2aa2a1233e48f77f728146fa4ee4005f0f806d9d

@disc disc mentioned this pull request Nov 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@dims dims Awaiting requested review from dims

@pwittrock pwittrock Awaiting requested review from pwittrock

@thockin thockin Awaiting requested review from thockin

@pacoxu pacoxu Awaiting requested review from pacoxu

@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

3 more reviewers

@kannon92 kannon92 kannon92 left review comments

@AxeZhan AxeZhan AxeZhan left review comments

@hashim21223445 hashim21223445 hashim21223445 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pacoxu pacoxu

@thockin thockin

@SergeyKanzhelev SergeyKanzhelev

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

SIG Apps
Archived in project
SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Archived in project

Milestone

v1.32

Development

Successfully merging this pull request may close these issues.

8 participants

@sreeram-venkitesh @k8s-ci-robot @kannon92 @pacoxu @SergeyKanzhelev @thockin @AxeZhan @hashim21223445