Track executing requests

[MikeSpreitzer]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR extends API Priority and Fairness to track the individual executing requests and optionally report them through a debug URL.

Which issue(s) this PR fixes:

Fixes #118746

Special notes for your reviewer:

This PR is not done yet, there is an interface design question in #118746 to be resolved first.

This PR builds on #118955 and will be rebased after that one merges.

Does this PR introduce a user-facing change?

The apiserver debug endpoint `/debug/api_priority_and_fairness/dump_requests` has been extended to dump executing requests as well as queued ones.  A column for StartTime has been added to the returned table, with the queued requests having a StartTime of "0001-01-01T00:00:00Z".  The executing requests have a RequestIndexInQueue of -1, and the QueueIndex is also -1 for priority levels without queues.

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

[MikeSpreitzer]

/cc @wojtek-t
@tkashem
/uncc @yue9944882

[MikeSpreitzer]

@natherz97

[MikeSpreitzer]

/retest

[natherz97]

Thank you for taking this up! When possible, can you share example output from the new debug command?

[jiahuif]

/assign @wojtek-t @tkashem
/triage accepted

[MikeSpreitzer]

@natherz97 : I realized that the design is not obvious (to me, anyway) and posted about that in the issue. I am waiting for feedback on #118746 (comment)

[MikeSpreitzer]

@natherz97: See the Release Note in the opening comment here. The added column for StartTime is inserted right after the AdditionalLatency column.

[MikeSpreitzer]

The tracking of executing requests in QueueSet is wrong --- the requests are tracked in each queue, but not every QueueSet has queues.
/hold

[MikeSpreitzer]

Bug fixed.
/unhold

[MikeSpreitzer]

@natherz97 : here is an example.

root@ubu2204a:/var/run/kubernetes# kubectl get --raw /debug/api_priority_and_fairness/dump_requests
PriorityLevelName, FlowSchemaName, QueueIndex, RequestIndexInQueue, FlowDistingsher, ArriveTime,                     InitialSeats, FinalSeats, AdditionalLatency, StartTime
exempt,            exempt,         -1,         -1,                  ,                2023-07-15T04:51:25.596404345Z, 1,            0,          0s,                2023-07-15T04:51:25.596404345Z

[natherz97]

@natherz97 : here is an example.

root@ubu2204a:/var/run/kubernetes# kubectl get --raw /debug/api_priority_and_fairness/dump_requests
PriorityLevelName, FlowSchemaName, QueueIndex, RequestIndexInQueue, FlowDistingsher, ArriveTime,                     InitialSeats, FinalSeats, AdditionalLatency, StartTime
exempt,            exempt,         -1,         -1,                  ,                2023-07-15T04:51:25.596404345Z, 1,            0,          0s,                2023-07-15T04:51:25.596404345Z

@MikeSpreitzer thank you! It also looks like dump_requests supports the includeRequestDetails=1 parameter to provide information like the username, verb, and path?

[MikeSpreitzer]

Yes. I just gave an example of the simpler usage. The columns added by includeRequestDetails=1 are the same as before.

[wojtek-t]

I have two comments, but overall this looks reasonable to me.

[wojtek-t]

IMHO introducing generic functions if they are used only in a single pattern is an overkill and makes it harder to understand the code (btw the name doesn't really help).

I would much rather prefer to just have:

func dumpRequestsSet(s *sets.Set[*request], includeDetails bool) []RequestDump {
  res := make([]RequestDump, 0, s.Len())
  for elem := range s {
    res = append(res, elem.dump(includeDetails)
  }
}

[wojtek-t]

Given it's a function that is used only in the test, can we move it to that test file? I doubt we will ever remember about this function exists here...

[MikeSpreitzer]

This is a form of map-reduce, I am sure I will remember that map-reduce exists and that I am tired of writing it over and over.

[MikeSpreitzer]

Perhaps it should be

func SliceMapReduce[Elt, Result any](mapFn func(Elt) Result, reduceFn func(Result,Result) Result) func([]Elt) Result {...{

[MikeSpreitzer]

The force-push to a8a2fb3 removes the currying function and restates the two map-reduce functions in the general form that hopefully makes the hope for more future use clear.

[wojtek-t]

/lgtm
/approve

Thanks!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 6d471ceb06c3f0cb48ab112ae624d413bda42f86

[k8s-ci-robot]

[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