Fix metrics AlreadyRegisteredError on TestSampler unit test #105886

CatherineF-dev · 2021-10-25T16:55:38Z

Test:

make test KUBE_RACE=-race KUBE_TIMEOUT=--timeout=600s GOFLAGS=-count=10 WHAT=./staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/

Add to #105809 ```` make test KUBE_RACE=-race KUBE_TIMEOUT=--timeout=600s GOFLAGS=-count=1 WHAT=./staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/ ```

k8s-ci-robot · 2021-10-25T16:55:40Z

@CatherineF-dev: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-10-25T16:55:45Z

Welcome @CatherineF-dev!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-10-25T16:55:46Z

Hi @CatherineF-dev. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Change variable name once -> registerMetrics

CatherineF-dev · 2021-10-25T17:05:34Z

/assign @logicalhan

MikeSpreitzer · 2021-10-25T17:24:13Z

@MikeSpreitzer

MikeSpreitzer · 2021-10-25T18:10:50Z

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

@@ -61,7 +64,9 @@ func TestSampler(t *testing.T) {
 	saw := gen.Generate(0, 1, []string{})
 	regs := gen.metrics()
 	for _, reg := range regs {
-		legacyregistry.MustRegister(reg)
+		registerMetrics.Do(func() {


Surely the Do should be outside the loop, rather than inside.
And don't forget the Reset. If I understand the problem, it is repeated runs of the same test.
I suggest putting the fake clock and SampleAndWatermarkObserverGenerator in file-level vars, and make the test func Reset the metrics and set the fake clock. The file-level vars could be initialized with init() (erm, maybe there's a reason not to use this for tests, but I forget) or test-suite initialization.

If I understand correctly, this PR is about making this code safe for the use case of running the same test func several times in series in the same process. That would be a surprise to the current version of the func, because it assumes that the values read from the metrics --- which are cumulative --- reflect only the activity of the current invocation of the func. So we need not only to register the metrics only once, we also need to reset their accumulators to zero at the start of the test func. If you look at the Prometheus client library, you will see that these metrics objects have a Reset() method that does that.

MikeSpreitzer · 2021-10-25T18:13:14Z

staging/src/k8s.io/component-base/metrics/testutil/metrics_test.go

@@ -30,6 +30,8 @@ import (
 	"k8s.io/utils/pointer"
 )

+var registerMetrics sync.Once


Again, you don't want the Do inside the loop.
You want Reset at the start of the test func.

MikeSpreitzer · 2021-10-29T20:05:31Z

The latest revision retreats to just ignoring the errors rather than fixing the problem.

MikeSpreitzer · 2021-10-29T20:12:29Z

The latest revision does not fix the problem. See the evidence in https://gist.github.com/MikeSpreitzer/0549a42e51c4bcd83587a7dcf4d50d5a

CatherineF-dev · 2021-10-29T20:21:12Z

The latest revision does not fix the problem. See the evidence in https://gist.github.com/MikeSpreitzer/0549a42e51c4bcd83587a7dcf4d50d5a

Yes. This testing has new errors after duplicate metrics collector registration attempted was solved.

Will understand test logics and solve it also.

MikeSpreitzer · 2021-10-29T20:28:33Z

The problem is that the accumulators are not reset to zero between tests.

CatherineF-dev · 2021-11-01T03:59:17Z

/retest

CatherineF-dev · 2021-11-01T13:35:57Z

The latest revision does not fix the problem. See the evidence in https://gist.github.com/MikeSpreitzer/0549a42e51c4bcd83587a7dcf4d50d5a

Hi Mike, could you help review again? Thanks!
@MikeSpreitzer

MikeSpreitzer

The change to sample_and_watermark_test.go works when I test serial repetitions in the same process. The revised test func will not work if there are concurrent runs of the same test in the same process.

As noted in #105809 (comment), if this test func created and used its own private registry then concurrent invocations would also work.

MikeSpreitzer · 2021-11-01T19:26:08Z

The revised staging/src/k8s.io/component-base/metrics/testutil/metrics_test.go also works with repeated invocations in the same process --- but not for good reasons. Note the call to legacyregistry.Register with the freshly constructed HistogramVec. In the first repetition, this succeeds. In later repetitions, I figure this returns an error. The calls to Observe update the current HistogramVec, but the call to GetHistogramVecFromGatherer always examines the HistogramVec from the first repetition.

MikeSpreitzer

This looks correct. One small thing noted inline.

MikeSpreitzer · 2021-11-04T15:28:18Z

staging/src/k8s.io/component-base/metrics/testutil/metrics_test.go

 			if diff := cmp.Diff(tt.wantVec, histogramVec); diff != "" {
 				t.Errorf("Got unexpected HistogramVec (-want +got):\n%s", diff)
 			}
+			registry.Reset()


I think this Reset() is not needed, because the registry involved is not used for anything after this statement.

MikeSpreitzer · 2021-11-04T16:43:45Z

I should also note that staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/sample_and_watermark_test.go could be changed to use a new private registry in each invocation of TestSampler, and that would make it work in the presence of concurrent invocations.

MikeSpreitzer · 2021-11-05T04:13:56Z

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

+var (
+	t0      = time.Now()
+	clk     = testclock.NewFakePassiveClock(t0)
+	buckets = []float64{0, 1}
+	gen     = NewSampleAndWaterMarkHistogramsGenerator(clk, samplingPeriod,
+		&compbasemetrics.HistogramOpts{Name: samplesHistName, Buckets: buckets},
+		&compbasemetrics.HistogramOpts{Name: "marks", Buckets: buckets},
+		[]string{})
+	dt time.Duration
+)


with a frame-local registry, these can remain frame-local too.

In fact, it would be much tidier to keep these local to the test func that uses them.

I remember I tried before. It didn't work.

https://gist.github.com/CatherineF-dev/ea06940aa9101f428685560f83c67cc2

That gist pointer only shows me some code, not testing results.
I tried testing a simple fix myself, and that revealed a bug in the test.
See #105886 for both a bug fix to the test and support for repeating the test.

k8s-ci-robot · 2021-11-10T03:35:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: CatherineF-dev
To complete the pull request process, please ask for approval from mikespreitzer after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/OWNERS

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 · 2021-11-16T22:41:56Z

@CatherineF-dev: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

CatherineF-dev · 2021-11-17T00:38:02Z

It's solved in #103526

Fix other metrics AlreadyRegisteredErrors

bde7e54

Add to #105809 ```` make test KUBE_RACE=-race KUBE_TIMEOUT=--timeout=600s GOFLAGS=-count=1 WHAT=./staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/ ```

k8s-ci-robot added 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. labels Oct 25, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 25, 2021

k8s-ci-robot requested review from deads2k and MikeSpreitzer October 25, 2021 16:56

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 Oct 25, 2021

CatherineF-dev added 2 commits October 25, 2021 13:00

Update sample_and_watermark_test.go

c576748

Change variable name once -> registerMetrics

Update metrics_test.go

10ae913

CatherineF-dev mentioned this pull request Oct 25, 2021

UT failure: panic: duplicate metrics collector registration attempted #104940

Closed

k8s-ci-robot assigned logicalhan Oct 25, 2021

MikeSpreitzer reviewed Oct 25, 2021

View reviewed changes

CatherineF-dev added 2 commits October 28, 2021 22:32

Use registry.Register

5b37ec9

go fmt

2eb3eb7

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 29, 2021

Move vars to global vars

a97a640

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 1, 2021

MikeSpreitzer reviewed Nov 1, 2021

View reviewed changes

MikeSpreitzer mentioned this pull request Nov 1, 2021

fix flake on TestRecordOperation unit test #105809

Closed

Use local registry

20054b0

MikeSpreitzer reviewed Nov 4, 2021

View reviewed changes

remove Reset()

e5110c5

Use local registry

cc3f91c

MikeSpreitzer reviewed Nov 5, 2021

View reviewed changes

CatherineF-dev changed the title ~~Fix metrics AlreadyRegisteredErrors in tests~~ Fix metrics AlreadyRegisteredError on TestSampler unit test Nov 10, 2021

Move changes in metrics_test.go to issue #106289

2fc1157

MikeSpreitzer mentioned this pull request Nov 10, 2021

Fix sample_and_watermark_test.go for bad luck, repeated test #106325

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2021

CatherineF-dev closed this Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix metrics AlreadyRegisteredError on TestSampler unit test #105886

Fix metrics AlreadyRegisteredError on TestSampler unit test #105886

CatherineF-dev commented Oct 25, 2021 •

edited

k8s-ci-robot commented Oct 25, 2021

k8s-ci-robot commented Oct 25, 2021

k8s-ci-robot commented Oct 25, 2021

CatherineF-dev commented Oct 25, 2021

MikeSpreitzer commented Oct 25, 2021

MikeSpreitzer Oct 25, 2021 •

edited

MikeSpreitzer Oct 26, 2021

MikeSpreitzer Oct 25, 2021

MikeSpreitzer commented Oct 29, 2021

MikeSpreitzer commented Oct 29, 2021

CatherineF-dev commented Oct 29, 2021 •

edited

MikeSpreitzer commented Oct 29, 2021

CatherineF-dev commented Nov 1, 2021

CatherineF-dev commented Nov 1, 2021 •

edited

MikeSpreitzer left a comment

MikeSpreitzer commented Nov 1, 2021

MikeSpreitzer left a comment

MikeSpreitzer Nov 4, 2021

MikeSpreitzer commented Nov 4, 2021

MikeSpreitzer Nov 5, 2021

MikeSpreitzer Nov 5, 2021

CatherineF-dev Nov 9, 2021

MikeSpreitzer Nov 10, 2021

k8s-ci-robot commented Nov 10, 2021

k8s-ci-robot commented Nov 16, 2021

CatherineF-dev commented Nov 17, 2021

Fix metrics AlreadyRegisteredError on TestSampler unit test #105886

Fix metrics AlreadyRegisteredError on TestSampler unit test #105886

Conversation

CatherineF-dev commented Oct 25, 2021 • edited

k8s-ci-robot commented Oct 25, 2021

k8s-ci-robot commented Oct 25, 2021

k8s-ci-robot commented Oct 25, 2021

CatherineF-dev commented Oct 25, 2021

MikeSpreitzer commented Oct 25, 2021

MikeSpreitzer Oct 25, 2021 • edited

Choose a reason for hiding this comment

MikeSpreitzer Oct 26, 2021

Choose a reason for hiding this comment

MikeSpreitzer Oct 25, 2021

Choose a reason for hiding this comment

MikeSpreitzer commented Oct 29, 2021

MikeSpreitzer commented Oct 29, 2021

CatherineF-dev commented Oct 29, 2021 • edited

MikeSpreitzer commented Oct 29, 2021

CatherineF-dev commented Nov 1, 2021

CatherineF-dev commented Nov 1, 2021 • edited

MikeSpreitzer left a comment

Choose a reason for hiding this comment

MikeSpreitzer commented Nov 1, 2021

MikeSpreitzer left a comment

Choose a reason for hiding this comment

MikeSpreitzer Nov 4, 2021

Choose a reason for hiding this comment

MikeSpreitzer commented Nov 4, 2021

MikeSpreitzer Nov 5, 2021

Choose a reason for hiding this comment

MikeSpreitzer Nov 5, 2021

Choose a reason for hiding this comment

CatherineF-dev Nov 9, 2021

Choose a reason for hiding this comment

MikeSpreitzer Nov 10, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 10, 2021

k8s-ci-robot commented Nov 16, 2021

CatherineF-dev commented Nov 17, 2021

CatherineF-dev commented Oct 25, 2021 •

edited

MikeSpreitzer Oct 25, 2021 •

edited

CatherineF-dev commented Oct 29, 2021 •

edited

CatherineF-dev commented Nov 1, 2021 •

edited