move all variables in sampleAndWaterMarkHistograms::innerSet #97860

MikeSpreitzer · 2021-01-08T18:36:41Z

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:
This is a refactoring to tiptoe around golang/go#43570 for #97685

Which issue(s) this PR fixes:

Special notes for your reviewer:
This is an alternative to #97791 and #97854

Does this PR introduce a user-facing change?:

Fixes a performance regression in 1.19 on ARM architectures computing priority and fairness metrics

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

to tiptoe around golang/go#43570 for kubernetes#97685

MikeSpreitzer · 2021-01-08T18:37:38Z

@kubernetes/sig-api-machinery-bugs

MikeSpreitzer · 2021-01-08T18:38:06Z

@liggitt
@erikwilson
@brandond
@tkashem
@adtac

liggitt · 2021-01-08T18:44:20Z

/lgtm
/approve

liggitt · 2021-01-08T18:45:39Z

/hold for verification on ARM

brandond · 2021-01-08T19:14:58Z

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

-	var acc sampleAndWaterMarkAccumulator
-	var wellOrdered bool
-	func() {
+	when, whenInt, acc, wellOrdered := func() (time.Time, int64, sampleAndWaterMarkAccumulator, bool) {


At this point it seems like there's not much point in keeping it as an anonymous function, might as well just move that whole block out to func (saw *sampleAndWaterMarkHistograms) validateQuantization() (time.Time, int64, sampleAndWaterMarkAccumulator, bool) (name it something better) unless the intent is to revert all of this once the golang bug is fixed.

the point of the inner func is to unlock via a defer as soon as our calculations are done before we log or call out to record metrics. Since the function has side effects, I'd prefer not to distribute it among several split up functions that are only expected to be called in a specific order

The side effects argument makes sense I suppose. I get that it just exists to handle the unlock cleanly, but to me the code picks up a bit of an odor when anonymous functions grow return values and such. I'm just here as peanut gallery though, feel free to ignore my feedback.

I agree with @liggitt , the inner function should never be invoked in any other context and so I would prefer to not expose it.

MikeSpreitzer · 2021-01-08T19:15:57Z

This fixes the problem for 386 on linux.

mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ git checkout test-saw
Switched to branch 'test-saw'
Your branch is up to date with 'origin/test-saw'.
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ git branch -D test-and-fix
Deleted branch test-and-fix (was 91ef7229c36).
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ git pull
remote: Enumerating objects: 33, done.
remote: Counting objects: 100% (33/33), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 33 (delta 27), reused 33 (delta 27), pack-reused 0
Unpacking objects: 100% (33/33), 3.00 KiB | 439.00 KiB/s, done.
From https://github.com/MikeSpreitzer/kubernetes
 + 714f40f3be9...14f94b6c5e5 rejigger-quantize-3  -> origin/rejigger-quantize-3  (forced update)
 * [new branch]              rejigger-quantize-3a -> origin/rejigger-quantize-3a
 * [new branch]              rejigger-quantize-4  -> origin/rejigger-quantize-4
Already up to date.
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ git checkout -b test-and-fix
Switched to a new branch 'test-and-fix'
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ git cherry-pick origin/rejigger-quantize-4
[test-and-fix d2613d7a69f] move all variables in sampleAndWaterMarkHistograms::innerSet
 Date: Fri Jan 8 13:32:38 2021 -0500
 1 file changed, 7 insertions(+), 9 deletions(-)
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ 
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ 
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ GOARCH=386 go test -c .
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ for i in {1..100}; do if GOGC=1 ./metrics.test >/dev/null 2>&1  ; then echo -n . ; else echo -n "F" ; fi; done; echo ""
....................................................................................................
mspreitz@ubu20:~/go/src/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics$ for i in {1..100}; do if ./metrics.test >/dev/null 2>&1  ; then echo -n . ; else echo -n "F" ; fi; done; echo ""
....................................................................................................

liggitt · 2021-01-08T19:43:51Z

can /hold cancel once this is verified on ARM

brandond · 2021-01-08T20:08:29Z

I can verify on arm this afternoon, if nobody else beats me to it.

liggitt · 2021-01-08T20:09:21Z

@MikeSpreitzer if you want to go ahead and open the cherry-pick to release-1.20 so CI is happy, that would help meet the 1.20.2 deadline of EOD

hasheddan

/lgtm

@MikeSpreitzer is this ready to go?

k8s-ci-robot · 2021-01-08T21:34:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hasheddan, liggitt, MikeSpreitzer

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,liggitt]

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

brandond · 2021-01-08T22:42:06Z

Working for me on arm:

sysadm@pi02:kubernetes$ uname -a
Linux pi02.lan.khaus 5.8.0-1008-raspi #11-Ubuntu SMP PREEMPT Thu Nov 12 15:56:06 UTC 2020 armv7l armv7l armv7l GNU/Linux
sysadm@pi02:kubernetes$ git log --pretty=oneline | head -n 3
ba7a0759472012798fc3862bcf1b63558f6df2c0 Explain warnings in new test
737d4e2cef29d1f2089452be36bf9d46423ef553 Add unit test for sample-and-watermark histograms
611184aa59d0cd40466bc3bc4b40a3712a038171 move all variables in sampleAndWaterMarkHistograms::innerSet
sysadm@pi02:kubernetes$ cd staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics
sysadm@pi02:metrics$ GOARCH=arm GOARM=7 go test -c .
sysadm@pi02:metrics$ for i in {1..100}; do if GOGC=1 ./metrics.test >/dev/null 2>&1  ; then echo -n . ; else echo -n "F" ; fi; done; echo ""
....................................................................................................
sysadm@pi02:metrics$ for i in {1..100}; do if ./metrics.test >/dev/null 2>&1  ; then echo -n . ; else echo -n "F" ; fi; done; echo ""
....................................................................................................

liggitt · 2021-01-08T22:42:50Z

thanks

/hold cancel

…-#97860-upstream-release-1.20 Automated cherry pick of #97860: move all variables in sampleAndWaterMarkHistograms::innerSet

caesarxuchao · 2021-01-12T21:13:27Z

/triage accepted

…0-upstream-release-1.19 Automated cherry pick of #97860: move all variables in sampleAndWaterMarkHistograms::innerSet

move all variables in sampleAndWaterMarkHistograms::innerSet

611184a

to tiptoe around golang/go#43570 for kubernetes#97685

MikeSpreitzer mentioned this pull request Jan 8, 2021

pass-by-reference for sampleAndWaterMarkObserverGenerator::quantize #97854

Closed

k8s-ci-robot requested review from lavalamp and yue9944882 January 8, 2021 18:37

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

k8s-ci-robot assigned liggitt Jan 8, 2021

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2021

brandond suggested changes Jan 8, 2021

View reviewed changes

MikeSpreitzer mentioned this pull request Jan 8, 2021

Automated cherry pick of #97860: move all variables in sampleAndWaterMarkHistograms::innerSet #97866

Merged

hasheddan approved these changes Jan 8, 2021

View reviewed changes

k8s-ci-robot assigned hasheddan Jan 8, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2021

k8s-ci-robot merged commit d9a26fb into kubernetes:master Jan 8, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 8, 2021

k8s-ci-robot added a commit that referenced this pull request Jan 8, 2021

Merge pull request #97866 from MikeSpreitzer/automated-cherry-pick-of…

a5351e9

…-#97860-upstream-release-1.20 Automated cherry pick of #97860: move all variables in sampleAndWaterMarkHistograms::innerSet

MikeSpreitzer mentioned this pull request Jan 9, 2021

Pass by reference to sampleAndWaterMarkObserverGenerator::quantize #97791

Closed

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 Jan 12, 2021

github-actions bot mentioned this pull request Jan 12, 2021

Week Ending January 10, 2021 dev-obs/actus#319

Open

This was referenced Jan 18, 2021

k3s server stops working after a few minutes k3s-io/k3s#2823

Closed

K3s v1.20.0+k3s2: Client.Timeout exceeded while awaiting headers k3s-io/k3s#2829

Closed

tkashem mentioned this pull request Jan 26, 2021

Automated cherry pick of #97860: move all variables in sampleAndWaterMarkHistograms::innerSet #98432

Merged

k8s-ci-robot added a commit that referenced this pull request Jan 26, 2021

Merge pull request #98432 from tkashem/automated-cherry-pick-of-#9786…

3545719

…0-upstream-release-1.19 Automated cherry pick of #97860: move all variables in sampleAndWaterMarkHistograms::innerSet

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move all variables in sampleAndWaterMarkHistograms::innerSet #97860

move all variables in sampleAndWaterMarkHistograms::innerSet #97860

MikeSpreitzer commented Jan 8, 2021 •

edited by liggitt

MikeSpreitzer commented Jan 8, 2021

MikeSpreitzer commented Jan 8, 2021

liggitt commented Jan 8, 2021

liggitt commented Jan 8, 2021

brandond Jan 8, 2021 •

edited

liggitt Jan 8, 2021 •

edited

brandond Jan 8, 2021

MikeSpreitzer Jan 8, 2021

MikeSpreitzer commented Jan 8, 2021

liggitt commented Jan 8, 2021

brandond commented Jan 8, 2021

liggitt commented Jan 8, 2021

hasheddan left a comment

k8s-ci-robot commented Jan 8, 2021

brandond commented Jan 8, 2021 •

edited

liggitt commented Jan 8, 2021

caesarxuchao commented Jan 12, 2021

move all variables in sampleAndWaterMarkHistograms::innerSet #97860

move all variables in sampleAndWaterMarkHistograms::innerSet #97860

Conversation

MikeSpreitzer commented Jan 8, 2021 • edited by liggitt

MikeSpreitzer commented Jan 8, 2021

MikeSpreitzer commented Jan 8, 2021

liggitt commented Jan 8, 2021

liggitt commented Jan 8, 2021

brandond Jan 8, 2021 • edited

Choose a reason for hiding this comment

liggitt Jan 8, 2021 • edited

Choose a reason for hiding this comment

brandond Jan 8, 2021

Choose a reason for hiding this comment

MikeSpreitzer Jan 8, 2021

Choose a reason for hiding this comment

MikeSpreitzer commented Jan 8, 2021

liggitt commented Jan 8, 2021

brandond commented Jan 8, 2021

liggitt commented Jan 8, 2021

hasheddan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 8, 2021

brandond commented Jan 8, 2021 • edited

liggitt commented Jan 8, 2021

caesarxuchao commented Jan 12, 2021

MikeSpreitzer commented Jan 8, 2021 •

edited by liggitt

brandond Jan 8, 2021 •

edited

liggitt Jan 8, 2021 •

edited

brandond commented Jan 8, 2021 •

edited