Advance epoch #105469

MikeSpreitzer · 2021-10-04T20:56:59Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR enhances the queueset implementation in API Priority and Fairness to cope with the possibility of overflow of work accumulators.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is built on #105412 and will be rebased once that PR merges.

Does this PR introduce a user-facing change?

NONE

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

MikeSpreitzer · 2021-10-04T20:57:29Z

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

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

fedebongio · 2021-10-05T20:16:57Z

/triage accepted

wojtek-t · 2021-10-06T08:26:48Z

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

-		klog.ErrorS(errors.New("progress meter wrapped around"), "Wrap", "QS", qs.qCfg.Name, "prevR", prevR, "currentR", qs.currentR)
+	incrR := SeatsTimesDuration(qs.getVirtualTimeRatioLocked(), timeSinceLast)
+	if incrR > RDecrement/2 {
+		klog.ErrorS(errors.New("unusably large R increment"), "Can not do it", "QS", qs.qCfg.Name, "when", realNow.Format(nsTimeFmt), "incrR", incrR)


I would remove this - it's not an actionable error really..

actionable by whom? If it happens, I would like to know because that means we have more work to do.

wojtek-t · 2021-10-06T08:33:40Z

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

 	}
 	metrics.SetCurrentR(qs.qCfg.Name, qs.currentR.ToFloat())
 }

+// advanceEpoch subtracts RDecrement from the global progress meter R
+// and all the readings that have been taked from that meter.
+func (qs *queueSet) advanceEpoch() {


As an admin - I would really like to be able to monitor this.

I think we need a metric that shows
a) when advancing epoch happened
b) any errors asociated with that (in particular I'm seeing the underflow error below).

I am most concerned by the fact that you are reporting seeing an underflow. That is far beyond what I expect. Can you give more details and/or describe how to reproduce?

It seems I wasn't clear - I didn't see underflow myself. I see that as error below.
But I would like to be able to monitor for that (i.e. have metrics - not just logs).

Added the metrics.

wojtek-t · 2021-10-06T08:37:24Z

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

-			t.Errorf("SeatSeonds(%d) formatted as %q rather than expected %q", uint64(testCase.ss), actualStr, testCase.expect)
+		actualStrLen := len(actualStr)
+		actualSuffix := actualStr[actualStrLen-2:]
+		if actualSuffix != "ss" {


I'm against these types of types.
The tests are supposed to be as simple as possible to avoid bugs in tests themselfes.

Given that this in particular is testing if the returned string looks as it should, what we should do is:

have an expectString field in the testCase

simply compare that with with what ss.String() returns

I wanted to have a test that does not require editing when ssScale changes. It was helpful when I experimented with different settings for that. But since you insist, I have sorrowfully made the test use literal strings.

wojtek-t · 2021-10-06T08:38:19Z

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

@@ -149,6 +150,14 @@ const MaxSeatSeconds = SeatSeconds(math.MaxUint64)
 // MinSeatSeconds is the lowest representable value of SeatSeconds
 const MinSeatSeconds = SeatSeconds(0)

+// RDecrement is the amount by which the progress meter R is wound backwards
+// when needed to avoid overflow.
+const RDecrement = MaxSeatSeconds / 2


This is an implementation detail - let's move these constants to queueset.go and make them private.

yes, good idea.

MikeSpreitzer · 2021-10-06T22:27:26Z

The force-push to c4c3d89f0f8 makes the changes suggested in review above.

MikeSpreitzer · 2021-10-06T22:34:16Z

The force-push to 5b3e05d07c4 adds a bit of info to the info log message in queueset::advanceEpoch.

wojtek-t · 2021-10-08T06:19:35Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go

@@ -207,8 +208,8 @@ var (
 			Help:           "Number of requests currently pending in queues of the API Priority and Fairness system",
 			StabilityLevel: compbasemetrics.ALPHA,
 		},
-		[]string{priorityLevel, flowSchema},
-	)
+		[]string{priorityLevel, flowSchema})


Why are you changing those?

[I personally prefer what we had previously]

They weren't all using the same style. I picked a style that some used and involved blank lines, which I think is a good separator when the things being separated are big.

Changed them all to separate with close paren.

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go

wojtek-t · 2021-10-08T06:23:39Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go

+			Namespace:      namespace,
+			Subsystem:      subsystem,
+			Name:           "epoch_advance_total",
+			Help:           "Number of times the queueset's progress meter zero-point was advanced",


I would remove "zero-point" from the description.

That would make it something like "Number of times the queueset's progress meter was jumped bakwards".

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go

MikeSpreitzer · 2021-10-08T19:09:14Z

The force-push to 7abd330961e makes the changes discussed in the latest review.

MikeSpreitzer · 2021-10-08T19:10:48Z

The force-push to 49cecbe715f makes one overlooked change.

Also add test for that situation.

MikeSpreitzer · 2021-10-09T00:12:20Z

The force-push to a797fbd corrected the corresponding test.

wojtek-t · 2021-10-11T10:45:53Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2021-10-11T10:46:31Z

[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 deads2k, lavalamp, tkashem and wojtek-t October 4, 2021 20:57

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. area/apiserver and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 4, 2021

k8s-ci-robot requested a review from yue9944882 October 4, 2021 20:57

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2021

MikeSpreitzer force-pushed the advance-epoch branch 2 times, most recently from 4f1bb71 to 6839bc3 Compare October 4, 2021 21:04

MikeSpreitzer mentioned this pull request Oct 4, 2021

Define and usse datatype for seat-seconds #105412

Merged

wojtek-t reviewed Oct 5, 2021

View reviewed changes

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/fairqueuing/queueset/queueset.go Outdated Show resolved Hide resolved

MikeSpreitzer force-pushed the advance-epoch branch 2 times, most recently from cb40446 to 3708369 Compare October 5, 2021 17:17

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 5, 2021

wojtek-t reviewed Oct 6, 2021

View reviewed changes

MikeSpreitzer force-pushed the advance-epoch branch from 3708369 to c4c3d89 Compare October 6, 2021 22:23

k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Oct 6, 2021

MikeSpreitzer force-pushed the advance-epoch branch from c4c3d89 to 5b3e05d Compare October 6, 2021 22:33

MikeSpreitzer mentioned this pull request Oct 7, 2021

apf: introduce final seats #105243

Merged

wojtek-t reviewed Oct 8, 2021

View reviewed changes

MikeSpreitzer force-pushed the advance-epoch branch from 5b3e05d to 7abd330 Compare October 8, 2021 19:09

MikeSpreitzer force-pushed the advance-epoch branch from 7abd330 to 49cecbe Compare October 8, 2021 19:10

Keep the progress meter R from overflowing

a797fbd

Also add test for that situation.

MikeSpreitzer force-pushed the advance-epoch branch from 49cecbe to a797fbd Compare October 9, 2021 00:12

k8s-ci-robot assigned wojtek-t Oct 11, 2021

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

k8s-ci-robot merged commit 9253fef into kubernetes:master Oct 11, 2021

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

MikeSpreitzer deleted the advance-epoch branch October 11, 2021 16:22

wojtek-t mentioned this pull request Oct 12, 2021

Potential loss of precision in progress meter in queueSet #104831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advance epoch #105469

Advance epoch #105469

MikeSpreitzer commented Oct 4, 2021

MikeSpreitzer commented Oct 4, 2021

fedebongio commented Oct 5, 2021

wojtek-t Oct 6, 2021

MikeSpreitzer Oct 6, 2021

wojtek-t Oct 6, 2021

MikeSpreitzer Oct 6, 2021

wojtek-t Oct 6, 2021

MikeSpreitzer Oct 6, 2021

wojtek-t Oct 6, 2021

MikeSpreitzer Oct 6, 2021

wojtek-t Oct 6, 2021

MikeSpreitzer Oct 6, 2021

MikeSpreitzer commented Oct 6, 2021

MikeSpreitzer commented Oct 6, 2021

wojtek-t Oct 8, 2021

MikeSpreitzer Oct 8, 2021

MikeSpreitzer Oct 8, 2021

wojtek-t Oct 8, 2021

MikeSpreitzer Oct 8, 2021

MikeSpreitzer Oct 8, 2021

MikeSpreitzer commented Oct 8, 2021

MikeSpreitzer commented Oct 8, 2021

MikeSpreitzer commented Oct 9, 2021

wojtek-t commented Oct 11, 2021

k8s-ci-robot commented Oct 11, 2021

Advance epoch #105469

Advance epoch #105469

Conversation

MikeSpreitzer commented Oct 4, 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.:

MikeSpreitzer commented Oct 4, 2021

fedebongio commented Oct 5, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer commented Oct 6, 2021

MikeSpreitzer commented Oct 6, 2021

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 8, 2021

MikeSpreitzer commented Oct 8, 2021

MikeSpreitzer commented Oct 9, 2021

wojtek-t commented Oct 11, 2021

k8s-ci-robot commented Oct 11, 2021