Fix TerminationGracePeriodSeconds is negative (part 1) #98866

wzshiming · 2021-02-08T08:27:46Z

What type of PR is this?

/kind bug
/kind feature

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?:

ACTION REQUIRED: TerminationGracePeriodSeconds on pod specs and container probes should not be negative.
Negative values of TerminationGracePeriodSeconds will be treated as the value `1s` on the delete path.
Immutable field validation will be relaxed in order to update negative values. 
In a future release, negative values will not be permitted.

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

NONE

wzshiming · 2021-02-08T09:42:12Z

/retest

fejta-bot · 2021-02-08T10:06:24Z

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.

ehashman · 2021-02-08T18:31:44Z

/triage accepted
/priority important-longterm

pkg/kubelet/kuberuntime/kuberuntime_container.go

pkg/kubelet/nodeshutdown/nodeshutdown_manager_linux.go

pkg/apis/core/validation/validation.go

pkg/registry/core/pod/strategy.go

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go

liggitt · 2021-02-09T14:13:10Z

the BeforeDelete implementation currently recalculates and updates DeletionGracePeriodSeconds:

kubernetes/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go

Lines 105 to 123 in 217a1c4

    
           // a resource was previously left in a state that was non-recoverable.  We 
        
           // check if the existing stored resource has a grace period as 0 and if so 
        
           // attempt to delete immediately in order to recover from this scenario. 
        
           if objectMeta.GetDeletionGracePeriodSeconds() == nil || *objectMeta.GetDeletionGracePeriodSeconds() == 0 { 
        
           	return false, false, nil 
        
           } 
        
           // only a shorter grace period may be provided by a user 
        
           if options.GracePeriodSeconds != nil { 
        
           	period := int64(*options.GracePeriodSeconds) 
        
           	if period >= *objectMeta.GetDeletionGracePeriodSeconds() { 
        
           		return false, true, nil 
        
           	} 
        
           	newDeletionTimestamp := metav1.NewTime( 
        
           		objectMeta.GetDeletionTimestamp().Add(-time.Second * time.Duration(*objectMeta.GetDeletionGracePeriodSeconds())). 
        
           			Add(time.Second * time.Duration(*options.GracePeriodSeconds))) 
        
           	objectMeta.SetDeletionTimestamp(&newDeletionTimestamp) 
        
           	objectMeta.SetDeletionGracePeriodSeconds(&period) 
        
           	return true, false, nil 
        
           }

There are two things that need changing here (with unit tests that exercise each change):

if the existing grace period is already less than zero we should delete immediately
if the new period is less than zero we should clip to zero

diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go
index 3e7ca85b761..7f90e5e18be 100644
--- a/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go
+++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go
@@ -103,9 +103,9 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx context.Context, obj runtime.
 		// 2. Delete the object from storage.
 		// If the update succeeds, but the delete fails (network error, internal storage error, etc.),
 		// a resource was previously left in a state that was non-recoverable.  We
-		// check if the existing stored resource has a grace period as 0 and if so
+		// check if the existing stored resource has a grace period <= 0 and if so
 		// attempt to delete immediately in order to recover from this scenario.
-		if objectMeta.GetDeletionGracePeriodSeconds() == nil || *objectMeta.GetDeletionGracePeriodSeconds() == 0 {
+		if objectMeta.GetDeletionGracePeriodSeconds() == nil || *objectMeta.GetDeletionGracePeriodSeconds() <= 0 {
 			return false, false, nil
 		}
 		// only a shorter grace period may be provided by a user
@@ -113,10 +113,13 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx context.Context, obj runtime.
 			period := int64(*options.GracePeriodSeconds)
 			if period >= *objectMeta.GetDeletionGracePeriodSeconds() {
 				return false, true, nil
+			} else if period < 0 {
+				// clip to zero
+				period = 0
 			}
 			newDeletionTimestamp := metav1.NewTime(
 				objectMeta.GetDeletionTimestamp().Add(-time.Second * time.Duration(*objectMeta.GetDeletionGracePeriodSeconds())).
-					Add(time.Second * time.Duration(*options.GracePeriodSeconds)))
+					Add(time.Second * time.Duration(period)))
 			objectMeta.SetDeletionTimestamp(&newDeletionTimestamp)
 			objectMeta.SetDeletionGracePeriodSeconds(&period)
 			return true, false, nil

ehashman · 2021-06-23T20:26:13Z

/priority important-soon
/remove-priority important-longterm

liggitt · 2021-06-26T21:02:32Z

staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go

+
+	// Negative values will be treated as the value `1s` on the delete path.
+	if gracePeriodSeconds := options.GracePeriodSeconds; gracePeriodSeconds != nil && *gracePeriodSeconds < 0 {
+		options.GracePeriodSeconds = utilpointer.Int64(1)


adding another options mutation looks like it will exacerbate the issue being fixed in #100101

@deads2k, is that PR close? is there something this PR should do differently to avoid that issue?

liggitt · 2021-06-26T21:42:16Z

pkg/apis/core/validation/validation.go

@@ -3873,9 +3873,35 @@ func ValidateContainerUpdates(newContainers, oldContainers []core.Container, fld
 			allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("image"), ctr.Image, "must not have leading or trailing whitespace"))
 		}
 	}
+
+	// validate updated container probe
+	for i, ctr := range newContainers {


if probe.TerminationGracePeriodSeconds is still alpha, can we add the validation to require non-negative values before promotion to beta, rather than making the update validation more complicated/expensive?

Sure. Update and I sent a new patch to follow this #103245

liggitt · 2021-06-26T21:43:23Z

pkg/apis/core/validation/validation.go

@@ -4039,13 +4070,26 @@ func ValidatePodUpdate(newPod, oldPod *core.Pod, opts PodValidationOptions) fiel
 	// tolerations are checked before the deep copy, so munge those too
 	mungedPodSpec.Tolerations = oldPod.Spec.Tolerations // +k8s:verify-mutation:reason=clone

+	// Relax validation of immutable fields to allow it to be set to 1 if it was previously negative.


… previously negative

liggitt · 2021-06-28T13:31:42Z

/lgtm
/approve

k8s-ci-robot · 2021-06-28T13:31:53Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/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

wzshiming · 2021-07-09T02:41:21Z

/kind feature

k8s-ci-robot requested review from juanvallejo and quinton-hoole February 8, 2021 08:28

rphillips suggested changes Feb 8, 2021

View reviewed changes

pkg/kubelet/kuberuntime/kuberuntime_container.go Outdated Show resolved Hide resolved

pkg/kubelet/nodeshutdown/nodeshutdown_manager_linux.go Outdated Show resolved Hide resolved

liggitt reviewed Feb 9, 2021

View reviewed changes

pkg/apis/core/validation/validation.go Outdated Show resolved Hide resolved

liggitt reviewed Feb 9, 2021

View reviewed changes

pkg/registry/core/pod/strategy.go Outdated Show resolved Hide resolved

liggitt reviewed Feb 9, 2021

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go Outdated Show resolved Hide resolved

liggitt self-assigned this Feb 9, 2021

liggitt removed request for juanvallejo and quinton-hoole February 9, 2021 14:13

wzshiming force-pushed the fix/termination_grace_period_seconds_is_negative branch from c07858f to c6d3696 Compare February 9, 2021 14:24

wzshiming requested a review from liggitt June 21, 2021 09:46

liggitt reviewed Jun 26, 2021

View reviewed changes

wzshiming mentioned this pull request Jun 28, 2021

Add validation for Prober TerminationGracePeriodSeconds #103245

Merged

wzshiming force-pushed the fix/termination_grace_period_seconds_is_negative branch from 4a1708d to 963ae22 Compare June 28, 2021 03:45

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. labels Jun 28, 2021

wzshiming requested a review from liggitt June 28, 2021 03:47

wzshiming added 3 commits June 28, 2021 11:49

spec.terminationGracePeriodSeconds allow it to be set to 1s if it was…

40593fa

… previously negative

Treat negative as 1s in delete path

45ce2df

Allow delete with grace period 1s even if metadata period is negative

a8d4cfa

wzshiming force-pushed the fix/termination_grace_period_seconds_is_negative branch from 963ae22 to a8d4cfa Compare June 28, 2021 03:50

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2021

k8s-ci-robot merged commit 5e06f17 into kubernetes:master Jun 28, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 28, 2021

wzshiming deleted the fix/termination_grace_period_seconds_is_negative branch June 28, 2021 15:13

wzshiming mentioned this pull request Jul 5, 2021

Fix TerminationGracePeriodSeconds is negative #103476

Closed

3 tasks

ehashman mentioned this pull request Jul 6, 2021

Add configurable grace period to probes kubernetes/enhancements#2238

Closed

12 tasks

raisaat mentioned this pull request Jul 6, 2021

Code changes for Probe-level Termination Grace Period Beta #103168

Merged

3 tasks

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 9, 2021

wzshiming mentioned this pull request Feb 8, 2023

pod.spec.terminationGracePeriodSeconds is a negative then convert to 1 #115606

Merged

liggitt mentioned this pull request Jun 21, 2024

Add validation to avoid negative TerminationGracePeriodSeconds in pod.Spec #113469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix TerminationGracePeriodSeconds is negative (part 1) #98866

Fix TerminationGracePeriodSeconds is negative (part 1) #98866

Uh oh!

wzshiming commented Feb 8, 2021 •

edited

Loading

Uh oh!

wzshiming commented Feb 8, 2021

Uh oh!

fejta-bot commented Feb 8, 2021

Uh oh!

ehashman commented Feb 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liggitt commented Feb 9, 2021

Uh oh!

ehashman commented Jun 23, 2021

Uh oh!

liggitt Jun 26, 2021

Uh oh!

liggitt Jun 26, 2021

Uh oh!

wzshiming Jun 28, 2021

Uh oh!

liggitt Jun 26, 2021

Uh oh!

liggitt commented Jun 28, 2021

Uh oh!

k8s-ci-robot commented Jun 28, 2021

Uh oh!

wzshiming commented Jul 9, 2021

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

Fix TerminationGracePeriodSeconds is negative (part 1) #98866

Fix TerminationGracePeriodSeconds is negative (part 1) #98866

Uh oh!

Conversation

wzshiming commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wzshiming commented Feb 8, 2021

Uh oh!

fejta-bot commented Feb 8, 2021

Uh oh!

ehashman commented Feb 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liggitt commented Feb 9, 2021

Uh oh!

ehashman commented Jun 23, 2021

Uh oh!

liggitt Jun 26, 2021

Choose a reason for hiding this comment

Uh oh!

liggitt Jun 26, 2021

Choose a reason for hiding this comment

Uh oh!

wzshiming Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

liggitt Jun 26, 2021

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jun 28, 2021

Uh oh!

k8s-ci-robot commented Jun 28, 2021

Uh oh!

wzshiming commented Jul 9, 2021

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

wzshiming commented Feb 8, 2021 •

edited

Loading