pod quota evaluator adds termination grace period twice

[bazzargh]

What happened:
Dev asked me to explain why deletionTimestamps for pods were in the future, went to find the line that added the grace period before setting the timestamp and found this:

kubernetes/pkg/quota/v1/evaluator/core/pods.go

Line 398 in 5856397

if now.After(deletionTime.Add(gracePeriod)) {

if now.After(deletionTime.Add(gracePeriod)) {

What you expected to happen:
this code seems wrong in a couple of ways. deletionTimestamp already has the grace period added here (the line of code I was looking for...):
https://github.com/kubernetes/apiserver/blob/d1b1b64dd9245cc8aa4391714eaad8eef96fe5a9/pkg/registry/rest/delete.go#L132-L133

staging link for embed:

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

Lines 132 to 133 in 5856397

	now := metav1.NewTime(metav1.Now().Add(time.Second * time.Duration(*options.GracePeriodSeconds)))
	objectMeta.SetDeletionTimestamp(&now)

...what is more, the grace period applied for a deletion may not be the grace period that applied to any given deletion. By just comparing to the deletionTimestamp, it would make the correct comparison. Furthermore, since deletion timestamps can be in the future, the quota evaluator should be checking deletionTimestamps, whether or not the pod had a grace period set.

Suggested fix to the quota evaluator would be:

	if pod.DeletionTimestamp != nil {
		now := clock.Now()
		deletionTime := pod.DeletionTimestamp.Time
		if now.After(deletionTime) {
			return false
		}
	}

How to reproduce it (as minimally and precisely as possible):

It's a code review issue, you just need to read the code at the two links above.

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version): master (in the links above I selected master branch then pressed 'y' in github to get latest commit)
• Cloud provider or hardware configuration: N/A
• OS (e.g: cat /etc/os-release): N/A
• Kernel (e.g. uname -a): N/A
• Install tools: N/A
• Network plugin and version (if this is a network-related bug): N/A
• Others: N/A

[bazzargh]

/sig scheduling
(I think?)

[tedyu]

@bazzargh
Do you want to submit a PR ?

[bazzargh]

@tedyu no, for 2 reasons - I'm not familiar enough with that quota code to write an appropriate test, and legal at $job would take an age to approve me signing the CLA (they would eventually, it's just slow - and I should probably get have got those wheels in motion anyway, I'm sure we'll find more stuff)

[tedyu]

The following code from staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go around line 117 verifies your assumption:

                        newDeletionTimestamp := metav1.NewTime(
                                objectMeta.GetDeletionTimestamp().Add(-time.Second * time.Duration(*objectMeta.GetDeletionGracePeriodSeconds())).
                                        Add(time.Second * time.Duration(*options.GracePeriodSeconds)))

[bazzargh]

It seems we have signed the CLA as an org already, getting my name added to our authorised users list so I can send a patch

[xiaoxubeii]

You can submit PRs directly.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.