Fix TerminationGracePeriodSeconds is negative (part 1)#98866
Conversation
|
/retest |
|
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. |
|
/triage accepted |
|
the kubernetes/staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go Lines 105 to 123 in 217a1c4 There are two things that need changing here (with unit tests that exercise each change):
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 |
c07858f to
c6d3696
Compare
|
/priority important-soon |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure. Update and I sent a new patch to follow this #103245
|
/lgtm |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/kind feature |
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?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: