Remove presumptions about what decision has been made #105729

MikeSpreitzer · 2021-10-18T08:17:59Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR updates queueSet to recognize that a request's decision might have already been made asynchronously due to context cancellation/timeout.

Which issue(s) this PR fixes:

Fixes #105338

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

/sig api-machinery
/cc @tkashem
/cc @wojtek-t
@deads2k
@lavalamp

wojtek-t · 2021-10-18T08:58:18Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/fifo_list.go

-// It returns the request removed from the list.
-type removeFromFIFOFunc func() *request
+// The returned bool indicates whether a change was made.
+type removeFromFIFOFunc func() bool


I wouldn't personally change this - the fact that it returns a request might be quite useful.

And you don't need it to do the changes you want to do - instead of checking if it is true, you just need to check if it is !=nil.

Before this PR, a removeFromFIFOFunc returns the *request in question regardless of whether it was in the fifo.

hmm, i overlooked it - i opened a PR keeping the same signature type removeFromFIFOFunc func() *request - #105738. let me know if you want it, otherwise i will close it.

I am folding it into this PR, thanks.

wojtek-t · 2021-10-18T08:59:13Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/queueset.go

@@ -592,14 +593,16 @@ func (qs *queueSet) removeTimedOutRequestsFromQueueLocked(queue *queue, fsName s
 		return false
 	})

+	for _, req := range rejects {


What's the point of moving it here and not handling it above on per-request basis?

"walkFunc must not remove the given request from the fifo"

We could have a more efficient code base if we change fifo::Walk to allow removal of the current *request and no other fifo mutations; this would exchange the slice that escapes with a stack-allocated variable holding the next *request to enumerate.

The force-push to 62eba5f54bc makes this change, and removes the redundant func enumerate_fifo.

MikeSpreitzer · 2021-10-18T15:18:23Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/fifo_list.go


 // walkFunc is called for each request in the list in the
 // oldest -> newest order.
 // ok: if walkFunc returns false then the iteration stops immediately.
+// walkFunc must not remove the given request from the fifo.


BTW, since there are no comments about other fifo mutations, the careful programmer will be unsure whether any are allowed.

MikeSpreitzer · 2021-10-19T03:49:03Z

The force-push to e290a6ababf makes a removeFromFIFOFunc return the *request if it was removed and nil otherwise.

MikeSpreitzer · 2021-10-19T04:57:43Z

/retest

fedebongio · 2021-10-19T20:11:03Z

/triage accepted

MikeSpreitzer · 2021-10-20T05:10:17Z

The force-push to 0bc77f6b709 is a rebase onto master.

MikeSpreitzer · 2021-10-20T05:16:09Z

The force-push to 06b1658a72d adds a missing period in the comment on removeFromFIFOFunc.

shivanshuraj1333 · 2021-10-20T09:19:09Z

/retest

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/fifo_list.go

wojtek-t · 2021-10-20T14:19:18Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/queueset.go

-			metrics.AddRequestsInQueues(req.ctx, qs.qCfg.Name, req.fsName, -1)
-			req.NoteQueued(false)
+		if arrivalLimit.After(req.arrivalTime) {
+			if req.decision.Set(decisionReject) {


nit:

if req.decision.Set(decisionReject) && req.removeFromQueueLocked() != nil {

wojtek-t · 2021-10-20T14:23:00Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/queueset.go

-	if request == nil { // This should never happen.  But if it does...
-		return false
+	dequeued := false
+	for qs.totRequestsWaiting != 0 && qs.totSeatsInUse < qs.dCfg.ConcurrencyLimit {


TBH - I much more preferred the previous pattern - i.e.

this function actually dispatching at most one item

having additional function doing a for-loop

Alternatively - can just switch this to a single if:

if qs.totRequestWaiting == 0 || qs.totSeatsInUse >= qs.dCfg.ConcurrencyLimit { return false } // The rest of the code comes here and is also not indented.

MikeSpreitzer · 2021-10-20T16:06:26Z

The force-push to e70999b addresses the latest review.

wojtek-t · 2021-10-20T19:05:40Z

/lgtm
/approve

/retest

k8s-ci-robot · 2021-10-20T19:05:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, wojtek-t

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:

~~staging/src/k8s.io/apiserver/pkg/util/flowcontrol/OWNERS~~ [MikeSpreitzer,wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from tkashem and wojtek-t October 18, 2021 08:18

wojtek-t reviewed Oct 18, 2021

View reviewed changes

wojtek-t self-assigned this Oct 18, 2021

MikeSpreitzer commented Oct 18, 2021

View reviewed changes

MikeSpreitzer force-pushed the do-not-assume-decision branch from 38f6f35 to 62eba5f Compare October 18, 2021 16:10

MikeSpreitzer mentioned this pull request Oct 19, 2021

apf: return nil for a request that has been removed from queue #105738

Merged

MikeSpreitzer force-pushed the do-not-assume-decision branch from 62eba5f to e290a6a Compare October 19, 2021 03:47

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 19, 2021

MikeSpreitzer force-pushed the do-not-assume-decision branch from e290a6a to 0bc77f6 Compare October 20, 2021 05:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2021

MikeSpreitzer force-pushed the do-not-assume-decision branch from 0bc77f6 to 06b1658 Compare October 20, 2021 05:15

wojtek-t reviewed Oct 20, 2021

View reviewed changes

Remove presumptions about what decision has been made

e70999b

MikeSpreitzer force-pushed the do-not-assume-decision branch from 06b1658 to e70999b Compare October 20, 2021 16:06

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

k8s-ci-robot merged commit c269494 into kubernetes:master Oct 20, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 20, 2021

MikeSpreitzer deleted the do-not-assume-decision branch October 21, 2021 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove presumptions about what decision has been made #105729

Remove presumptions about what decision has been made #105729

MikeSpreitzer commented Oct 18, 2021

wojtek-t Oct 18, 2021

MikeSpreitzer Oct 18, 2021

tkashem Oct 18, 2021 •

edited

MikeSpreitzer Oct 19, 2021

wojtek-t Oct 18, 2021

MikeSpreitzer Oct 18, 2021

MikeSpreitzer Oct 18, 2021

MikeSpreitzer Oct 18, 2021

MikeSpreitzer Oct 18, 2021

MikeSpreitzer commented Oct 19, 2021

MikeSpreitzer commented Oct 19, 2021

fedebongio commented Oct 19, 2021

MikeSpreitzer commented Oct 20, 2021

MikeSpreitzer commented Oct 20, 2021

shivanshuraj1333 commented Oct 20, 2021

wojtek-t Oct 20, 2021

wojtek-t Oct 20, 2021

wojtek-t Oct 20, 2021

MikeSpreitzer commented Oct 20, 2021

wojtek-t commented Oct 20, 2021

k8s-ci-robot commented Oct 20, 2021

Remove presumptions about what decision has been made #105729

Remove presumptions about what decision has been made #105729

Conversation

MikeSpreitzer commented Oct 18, 2021

What type of PR is this?

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?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer commented Oct 19, 2021

MikeSpreitzer commented Oct 19, 2021

fedebongio commented Oct 19, 2021

MikeSpreitzer commented Oct 20, 2021

MikeSpreitzer commented Oct 20, 2021

shivanshuraj1333 commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer commented Oct 20, 2021

wojtek-t commented Oct 20, 2021

k8s-ci-robot commented Oct 20, 2021

tkashem Oct 18, 2021 •

edited