-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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.
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
Code changes for Probe-level Termination Grace Period Beta #103168
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @raisaat! |
Hi @raisaat. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Please make sure you sign the CLA: #103168 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/sig node
/priority important-soon
/triage accepted
Per https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2238-liveness-probe-grace-period#beta-122 we still need to do the following:
- Move feature flag to beta, leave as false
- Remove feature gate from kubelet
- Ensure that when feature gate is off in API server, probe-level TerminationGracePeriodSeconds is blanked out.
- Add validation to ensure terminationGracePeriodSeconds is non-negative. -> see Fix TerminationGracePeriodSeconds is negative (part 1) #98866
pkg/api/pod/util_test.go
Outdated
// new pod should not have grace period if the feature gate is disabled | ||
t.Errorf("new pod should not have grace period if the feature is disabled") | ||
|
||
case reflect.DeepEqual(newPod, newPodInfo.pod()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me as a case. We're unconditionally evaluating this if the first two cases don't happen, and if it's true, throwing an error. Whether they're equal is a test outcome, not a dev expectation. But, this is also opposite to the default case below, so I think this was just an oversight.
We need a case for oldPodHasGracePeriod
to assert that the field was dropped. I'd suggest we repurpose this case for that.
Test matrix is
- Feature flag is on:
- Pod has grace period: unchanged
- Pod does not have grace period: unchanged
- Feature flag is off:
- Pod has grace period: should be dropped
- Pod does not have grace period: unchanged
Thus, I'd expect our cases to be:
-
if enabled || !oldPodHasGracePeriod
: should be unchanged - Now we know it's disabled. So, we should check next,
if !enabled && newPodHasGracePeriod
and ensure the field is blanked out (note: it is probably clearer to write it the way I've written in this comment rather than trying to be clever with boolean logic) -
if !enabled && oldHasGracePeriod
: should be dropped -- need a case for this still
I think we can keep the default
case below as written unchanged.
/test pull-kubernetes-verify |
I signed the CLA |
/check-cla |
pkg/api/pod/util_test.go
Outdated
// new pod should not have subpaths | ||
if !reflect.DeepEqual(newPod, podWithoutProbeGracePeriod()) { | ||
t.Errorf("new pod had probe-level terminationGracePeriod: %v", cmp.Diff(newPod, podWithoutProbeGracePeriod())) | ||
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newPod
and newPodInfo.pod()
should be different after the feature is disabled and the feature is dropped via the dropDisabledFields
function with the dropDisabledFields
function written as is right now, right? The reason is that the dropDisabledFields
function blanks out the LivenessProbe.TerminationGracePeriodSeconds
and StartupProbe.TerminationGracePeriodSeconds
fields of the containers in newPod
while newPodInfo.pod()
remains unchanged. So reflect.DeepEqual(newPod, newPodInfo.pod())
should return a false
for the condition if !enabled && newPodHasGracePeriod
, right? Trying to figure out why it returns true
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you're discussing here should be covered by the case newPodHasGracePeriod:
below, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I throw an error directly for case newPodHasGracePeriod
, 5 test cases fail. So I tried adding if reflect.DeepEqual(newPod, newPodInfo.pod())
under case newPodHasGracePeriod
and a t.Errorf
under that if
condition. The 5 test cases fail for the latter as well.
pkg/api/pod/util_test.go
Outdated
if newPod == nil { | ||
continue | ||
} | ||
|
||
t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) { | ||
|
||
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ProbeTerminationGracePeriod, enabled)() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left this line and the for
loop for enabling/disabling the feature unchanged (discarded all the other changes I had made in the previous commits). The test cases where the feature flag is disabled and the newPod
has a probe-level termination grace period are still failing.
/test pull-kubernetes-unit |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
API bits lgtm /approve |
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ehashman, liggitt, mrunalp, raisaat 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 |
/milestone v1.22 |
Looks like this needs a rebase... |
3251fce
to
1d786a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
assuming CI passes
/hold cancel
What type of PR is this?
/kind feature
/sig node
/priority important-soon
What this PR does / why we need it:
Code changes to graduate this KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2238-liveness-probe-grace-period to beta
Which issue(s) this PR fixes:
KEP issue: kubernetes/enhancements#2238
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: