Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix extra latency and add tests for that and non-unit width #104345

Merged
merged 1 commit into from
Aug 23, 2021
Merged

Fix extra latency and add tests for that and non-unit width #104345

merged 1 commit into from
Aug 23, 2021

Conversation

MikeSpreitzer
Copy link
Member

@MikeSpreitzer MikeSpreitzer commented Aug 12, 2021 •
edited

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

This PR adds dispatching after delayed release of seats.

This PR updates the logging in all six cases of execution completion and/or seat release.

This PR adds behavioral tests of various combinations of think time and extra latency,
which revealed the fact that the dispatching was missing.

This PR adds two behavioral tests of non-unit "width".

This PR also adds two tests for baseline functionality.

This PR also improves some comments and other logging in queueset.go.

Which issue(s) this PR fixes:

Fixes #

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.:

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 12, 2021
@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 12, 2021
@MikeSpreitzer
Copy link
Member Author

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

@MikeSpreitzer
Copy link
Member Author

This thing we have been calling "virtual time" no longer even has units of time; now it is seat-seconds.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2021
@MikeSpreitzer MikeSpreitzer changed the title Add queueSet tests for width Fix extra latency and add tests for that and non-unit width Aug 13, 2021
@MikeSpreitzer
Copy link
Member Author

The force-push to 572ca92c44d is a rebase plus the bug fix and testing for extra latency.

@MikeSpreitzer
Copy link
Member Author

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 13, 2021
@MikeSpreitzer
Copy link
Member Author

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 13, 2021
@MikeSpreitzer
Copy link
Member Author

The force-push to 7e5c7620b4c adds checking that extra latency does not affect the timing of the return to the client and expands the test cases for extra latency.

@MikeSpreitzer
Fix extra latency and add tests for that and width
d2a27a5 
Added missing dispatching after delayed release of seats.

Updated logging for all six situations of execution completion and
seat release.

Added behavioral tests for non-zero extra latency and non-unit width.

Also added two tests for baseline functionality.

Also improved some comments and other logging in `queueset.go`.
@MikeSpreitzer
Copy link
Member Author

MikeSpreitzer commented Aug 13, 2021 •
edited

The force-push to d2a27a5 locks down every test of extra latency to allow zero relative error.

@fedebongio
Copy link
Contributor

/triage accepted

@k8s-ci-robot 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 Aug 17, 2021
wojtek-t
wojtek-t reviewed Aug 23, 2021
View reviewed changes
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/queueset.go
}
}

defer func() {
if r.workEstimate.AdditionalLatency <= 0 {
// release the seats allocated to this request immediately
releaseSeatsLocked()
if !klog.V(6).Enabled() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a slightly weird pattern to me, but it's harmless, so I'm not going to block on it.

[In general, I don't think logs at 4+ verbosity are useful, because there are so many of them that it's impossible to find anything there anyway. But we can follow up on it separately.]

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/queueset.go
} else {
klog.Infof("QS(%s) at r=%s v=%.9fss: request %#+v %#+v finished lingering on %d seats, qs will have %d requests occupying %d seats", qs.qCfg.Name, now.Format(nsTimeFmt), qs.virtualTime, r.descr1, r.descr2, r.workEstimate.Seats, qs.totRequestsExecuting, qs.totSeatsInUse)
}
qs.dispatchAsMuchAsPossibleLocked()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can thank the behavioral tests.

@wojtek-t
Copy link
Member

/lgtm
/approve

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2021
@k8s-ci-robot
Copy link
Contributor

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

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

@wojtek-t
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit b9565be into kubernetes:master Aug 23, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Aug 23, 2021
@MikeSpreitzer MikeSpreitzer deleted the test-width branch August 23, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtek-t wojtek-t wojtek-t left review comments

@deads2k deads2k Awaiting requested review from deads2k

@lavalamp lavalamp Awaiting requested review from lavalamp

@tkashem tkashem Awaiting requested review from tkashem

Assignees

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MikeSpreitzer @fedebongio @wojtek-t @k8s-ci-robot