Couple code cleanups for APF code #103820

wojtek-t · 2021-07-21T10:22:37Z

NONE

/kind cleanup
/priority important-longterm
/sig api-machinery

/assign @MikeSpreitzer @tkashem

wojtek-t · 2021-07-21T10:24:42Z

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

 		go func() {
 			defer runtime.HandleCrash()
-			qs.goroutineDoneOrBlocked()


One more thing I want to do in subsequent PR is to ensure that production code isn't really tracking goroutines at all (it doesn't use this information) by using non-tracking promise in prod and tracking one (build on top of non-tracking one) in tests.

The production code already is not really tracking goroutines --- see https://github.com/kubernetes/apiserver/blob/b94b411ffc0d129451a850a78bd39b9457a15518/pkg/util/flowcontrol/apf_filter.go#L85

By removing the updates through the counter interface, this makes it impossible to test queueset with a fake clock. It is important to be able to test with a fake clock:

so that we can control timing precisely, and

so that we can run tests faster.

Sorry - I wasn't clear exactly.
I'm aware we're using no-op tracker in production code.

But I'm really not a fan of having test-only related code in production code. This complicates production code.
All the test-related code should really be separate out from the code that is running in production.

By removing the updates through the counter interface, this makes it impossible to test queueset with a fake clock. It is important to be able to test with a fake clock:

I disagree with this one. Currently none of the tests is exercising this. But even if at some point we extend our testing to take advantage of it, what we should do is:

make a class function from this goroutine [and leave it without counter]

in tests, define our own TestingQueuset (or whatever name we choose) that will be basically inheriting everything from the QueueSet, we will just redefing this single function

I can live with complicate tests, but I really want to have production code as simple as possible [it's not just about P&F of course].

https://github.com/kubernetes/apiserver/blob/b94b411ffc0d129451a850a78bd39b9457a15518/pkg/util/flowcontrol/fairqueuing/queueset/queueset_test.go#L653 does exercise this.

If we factor out this goroutine plus old line 298, the resulting surface seems odder (less simple) to me than the one we have now. What is the meaning of this goroutine plus old line 298? It's just a random bit of implementation. Counting active goroutines, on the other hand, is a simple self-contained semantic.

https://github.com/kubernetes/apiserver/blob/b94b411ffc0d129451a850a78bd39b9457a15518/pkg/util/flowcontrol/fairqueuing/queueset/queueset_test.go#L653 does exercise this.

It exercises that code path, but it doesn't exercise the counter:

clk is not used anywhere after initialization

counter is only updated through the text but never check
[as the test shows - it passed consistently after removing it]

If we factor out this goroutine plus old line 298, the resulting surface seems odder (less simple) to me than the one we have now. What is the meaning of this goroutine plus old line 298? It's just a random bit of implementation. Counting active goroutines, on the other hand, is a simple self-contained semantic.

I disagree with this. When I'm reading the code I would like to be able to quickly understand what exactly is happening in production. And mixing test code with production code and just initializing it differently is not the best practice.

MikeSpreitzer · 2021-07-21T19:31:20Z

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

-
-// WriteOnceOnly represents a variable that is initially not set and
-// can be set once.
-type WriteOnceOnly interface {


In case anyone is not aware, the Go language has improved in a relevant way here since these interfaces were defined. In the bad old days, Go did not allow a diamond-shaped inheritance graph among interfaces. That is why this is written in mixin-style. Now, Go supports diamon-shaped inheritance. It would be possible to maintain the producer/consumer distinction here with fewer interface definitions.

I think we shouldn't be introducing interfaces for the sake of introducing them. Let's not overcomplicate stuff and not introduce things that we don't even use/need anywhere.

MikeSpreitzer · 2021-07-21T19:44:29Z

....io/apiserver/pkg/util/flowcontrol/fairqueuing/promise/lockingpromise/lockingpromise_test.go

-	}
-}
-
-func TestLockingWriteOnce(t *testing.T) {


Why delete this test code?

Of couse it shouldn't be deleted - reverted. Thanks for catching.

MikeSpreitzer · 2021-07-21T19:46:17Z

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

 		go func() {
 			defer runtime.HandleCrash()
-			qs.goroutineDoneOrBlocked()


The production code already is not really tracking goroutines --- see https://github.com/kubernetes/apiserver/blob/b94b411ffc0d129451a850a78bd39b9457a15518/pkg/util/flowcontrol/apf_filter.go#L85

By removing the updates through the counter interface, this makes it impossible to test queueset with a fake clock. It is important to be able to test with a fake clock:

so that we can control timing precisely, and

so that we can run tests faster.

fedebongio · 2021-07-22T20:07:37Z

/triage accepted

MikeSpreitzer · 2021-07-23T00:19:03Z

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

@@ -125,8 +125,7 @@ type configController struct {
 	requestWaitLimit time.Duration

 	// This must be locked while accessing flowSchemas or
-	// priorityLevelStates.  It is the lock involved in
-	// LockingWriteMultiple.


The remark

It is the lock involved in LockingWriteMultiple

got misplaced in past refactoring. It belongs on the lock in queueSet and refers to the promise in every request in the queueSet. It wouldn't hurt to reiterate the point in the comment on the decision field of request.

I went step further and simplified it further (we don't really take advantage of "locking" part of the interface anywhere).

So to make it clear for everyone - I updated the interface itself to not be thread-safe [which is how it is used - we care about protecting it ourselves in queueset]

wojtek-t · 2021-07-28T09:35:20Z

@MikeSpreitzer - PTAL

MikeSpreitzer · 2021-07-28T22:26:19Z

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

+		lock.Lock()
+		defer lock.Unlock()
+		if !wr.IsSet() {
+			t.Error("IsSet()==false after second Set")


s/second Set/second Get/

MikeSpreitzer · 2021-07-28T22:28:29Z

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

-	decision promise.LockingWriteOnce
+	//
+	// The field is NOT thread-safe and should be protected by the
+	// external lock.


s/external/queueset's/
(this type is not designed or available for use outside this package)

MikeSpreitzer

Looks OK except for a couple of minor comments.

wojtek-t

Thanks @MikeSpreitzer - PTAL

wojtek-t · 2021-07-29T09:04:33Z

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

+		lock.Lock()
+		defer lock.Unlock()
+		if !wr.IsSet() {
+			t.Error("IsSet()==false after second Set")


wojtek-t · 2021-07-29T09:05:48Z

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

-	decision promise.LockingWriteOnce
+	//
+	// The field is NOT thread-safe and should be protected by the
+	// external lock.


wojtek-t · 2021-07-29T10:07:22Z

/retest

MikeSpreitzer

/LGTM

k8s-ci-robot · 2021-07-30T20:45:52Z

[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]
~~vendor/OWNERS~~ [wojtek-t]

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

MikeSpreitzer · 2021-07-30T20:46:37Z

/retest

k8s-ci-robot assigned MikeSpreitzer and tkashem Jul 21, 2021

k8s-ci-robot requested review from lavalamp and yue9944882 July 21, 2021 10:23

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver labels Jul 21, 2021

wojtek-t commented Jul 21, 2021

View reviewed changes

MikeSpreitzer suggested changes Jul 21, 2021

View reviewed changes

wojtek-t force-pushed the pf_remove_counter branch from 86653e8 to 19cb29b Compare July 22, 2021 06:18

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 Jul 22, 2021

MikeSpreitzer reviewed Jul 23, 2021

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 23, 2021

wojtek-t force-pushed the pf_remove_counter branch from c63d9e9 to 44e5d94 Compare July 23, 2021 13:18

Remove unused promise code from APF

a1cf44e

wojtek-t force-pushed the pf_remove_counter branch from 44e5d94 to b97b42f Compare July 28, 2021 09:34

MikeSpreitzer reviewed Jul 28, 2021

View reviewed changes

MikeSpreitzer mentioned this pull request Jul 29, 2021

Refactor goroutine counting #104002

Merged

Simplify APF promise to what is really used in the code

9f735e7

wojtek-t force-pushed the pf_remove_counter branch from b97b42f to 9f735e7 Compare July 29, 2021 09:05

wojtek-t commented Jul 29, 2021

View reviewed changes

MikeSpreitzer approved these changes Jul 30, 2021

View reviewed changes

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

k8s-ci-robot merged commit e3b01a6 into kubernetes:master Aug 5, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Couple code cleanups for APF code #103820

Couple code cleanups for APF code #103820

wojtek-t commented Jul 21, 2021

wojtek-t Jul 21, 2021

MikeSpreitzer Jul 21, 2021

wojtek-t Jul 22, 2021

MikeSpreitzer Jul 22, 2021

MikeSpreitzer Jul 22, 2021 •

edited

wojtek-t Jul 23, 2021

MikeSpreitzer Jul 21, 2021

wojtek-t Jul 22, 2021

MikeSpreitzer Jul 21, 2021

wojtek-t Jul 22, 2021

MikeSpreitzer Jul 21, 2021

fedebongio commented Jul 22, 2021

MikeSpreitzer Jul 23, 2021 •

edited

wojtek-t Jul 23, 2021

wojtek-t commented Jul 28, 2021

MikeSpreitzer Jul 28, 2021

wojtek-t Jul 29, 2021

MikeSpreitzer Jul 28, 2021

wojtek-t Jul 29, 2021

MikeSpreitzer left a comment

wojtek-t left a comment

wojtek-t Jul 29, 2021

wojtek-t Jul 29, 2021

wojtek-t commented Jul 29, 2021

MikeSpreitzer left a comment

k8s-ci-robot commented Jul 30, 2021

MikeSpreitzer commented Jul 30, 2021

Couple code cleanups for APF code #103820

Couple code cleanups for APF code #103820

Conversation

wojtek-t commented Jul 21, 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

MikeSpreitzer Jul 22, 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

fedebongio commented Jul 22, 2021

MikeSpreitzer Jul 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Jul 28, 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

MikeSpreitzer left a comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Jul 29, 2021

MikeSpreitzer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 30, 2021

MikeSpreitzer commented Jul 30, 2021

MikeSpreitzer Jul 22, 2021 •

edited

MikeSpreitzer Jul 23, 2021 •

edited