Fix TerminationGracePeriodSeconds is negative (part 1)

[wzshiming]

What type of PR is this?

/kind bug
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #98506
xref #98507
xref #103476

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]

/retest

[fejta-bot]

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]

/triage accepted
/priority important-longterm

[liggitt]

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

[liggitt]

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]

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?

[wzshiming]

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

[liggitt]

this lgtm

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

[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]

/kind feature