Add api-machinery 'watch-consistency' e2e test #69829

jpbetz · 2018-10-15T20:28:34Z

Added a watch consistency e2e test as originally proposed in #67717.

Plan is to make this a conformance test after it's had sufficient bake time.

This test continues to use ConfigMap for watch tests. We will expand the watch tests to also test others resource separately as part of #67718.

to ensure concurrent writes throughout the test, event production runs in the background throughout the test. It's rated limited to no more than 200 events/sec, but runs indefinitely until the watchers complete their testing.

100 iterations was selected since that runs in ~15 seconds and the recommended limit for e2e tests is 20 seconds.

NONE

/kind cleanup
/sig api-machinery
/cc @lavalamp

lavalamp · 2018-10-15T21:38:05Z

test/e2e/apimachinery/watch.go

@@ -314,6 +316,49 @@ var _ = SIGDescribe("Watchers", func() {
 		expectEvent(testWatch, watch.Modified, testConfigMapThirdUpdate)
 		expectEvent(testWatch, watch.Deleted, nil)
 	})
+
+	/*
+						    Testname: watch-consistency


Something weird happened to this comment

lavalamp

Nits. Do we want to resume a watch if it fails for some reason, or should that fail the test? I'm unsure. Watches shouldn't really randomly fail.

lavalamp · 2018-10-15T21:49:38Z

test/e2e/apimachinery/watch.go

+			Expect(err).NotTo(HaveOccurred())
+			wcs = append(wcs, wc)
+			resourceVersion = waitForNextConfigMapEvent(wcs[0]).ResourceVersion
+			for _, wc := range wcs[1:] {


I bet doing these all in parallel would significantly reduce the wall time.

A bit to my surprise, this didn't speed things up. I'm guessing that the buffered channels <-watch.ResultChan() in waitForNextConfigMapEvent() are sufficiently concurrent already, since the watches are initiated in the outer for loop.

Interesting.

lavalamp · 2018-10-15T21:54:15Z

test/e2e/apimachinery/watch.go

+
+	existing := []int{}
+	for i := 0; ; i++ {
+		waitc := time.After(minWaitBetweenEvents) // rate limit


tc := time.NewTicker(minWaitBetweenEvents) defer tc.Stop() for range tc.C

Ah I see you have a multi channel select statement at the bottom, so the for statement I suggested won't work. I think it still makes sense to use the ticker?

Code using the ticker reads better. Thanks!

lavalamp · 2018-10-15T21:57:41Z

test/e2e/apimachinery/watch.go

+			Expect(err).NotTo(HaveOccurred())
+			existing = append(existing, i)
+		case updateEvent:
+			idx := rand.Int() % len(existing)


Intn(existing)

Mod leaves a biased result if existing isn't a divisor of 2^32. This 100% doesn't matter for this use but no need to leave the example here to confuse people :)

yikes, yes, happy to leave a good example here.

lavalamp · 2018-10-15T22:00:45Z

test/e2e/apimachinery/watch.go

+const (
+	createEvent = 0
+	updateEvent = iota
+	deleteEvent = iota


I think you can just:

createEvent = iota updateEvent deleteEvent

Much better, fixed.

lavalamp · 2018-10-15T22:02:37Z

test/e2e/apimachinery/watch.go

+			Expect(err).NotTo(HaveOccurred())
+		case deleteEvent:
+			idx := rand.Int() % len(existing)
+			name := fmt.Sprintf("cm-%d", existing[idx])


nit: might be worth it to declare name := func(n int) string { return fmt.Sprintf("cm-%d", existing[n]) } somewhere?

lavalamp · 2018-10-15T22:05:27Z

I think we need to get the test in and demonstrate that it works before adding it to conformance.

jpbetz · 2018-10-16T19:42:25Z

Thanks @lavalamp. Feedback applied. I've removed this from conformance (in code and in PR title/desc/labels). Once its had some bake time I'll propose it for conformance via a separate PR.

jpbetz · 2018-10-16T21:46:03Z

/test pull-kubernetes-e2e-kops-aws

jpbetz · 2018-10-16T21:59:38Z

/retest

jpbetz · 2018-10-16T23:14:04Z

/retest

jpbetz · 2018-10-17T17:39:22Z

This is ready for review. Tests are all passing.

lavalamp · 2018-10-19T20:58:10Z

test/e2e/apimachinery/watch.go

+		go func() {
+			defer GinkgoRecover()
+			produceConfigMapEvents(f, stopc, 5*time.Millisecond)
+			close(donec)


Nit: probably best to defer this.

Fixed. Thanks!

lavalamp · 2018-10-19T20:58:47Z

/approve

k8s-ci-robot · 2018-10-19T21:00:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, lavalamp

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:

~~test/OWNERS~~ [lavalamp]

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

jpbetz · 2018-10-19T21:50:38Z

/retest

jpbetz · 2018-10-19T22:36:00Z

/retest

lavalamp · 2018-10-19T22:54:18Z

/lgtm

jpbetz · 2018-10-19T23:24:26Z

/retest

fejta-bot · 2018-10-20T03:22:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

jpbetz self-assigned this Oct 15, 2018

k8s-ci-robot requested a review from lavalamp October 15, 2018 20:28

jpbetz force-pushed the watch-e2e-test1 branch from 6a31f91 to cc826d5 Compare October 15, 2018 20:59

jpbetz changed the title ~~Add api-machinery 'watch-consistency' e2e test~~ Add api-machinery 'watch-consistency' e2e conformance test Oct 15, 2018

jpbetz force-pushed the watch-e2e-test1 branch from cc826d5 to 44995af Compare October 15, 2018 21:04

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 15, 2018

lavalamp reviewed Oct 15, 2018

View reviewed changes

jpbetz force-pushed the watch-e2e-test1 branch from 44995af to 5ee5ff1 Compare October 15, 2018 21:42

jpbetz added area/conformance Issues or PRs related to kubernetes conformance tests area/etcd labels Oct 15, 2018

jpbetz force-pushed the watch-e2e-test1 branch from 5ee5ff1 to ad16c94 Compare October 15, 2018 21:45

k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Oct 15, 2018

lavalamp reviewed Oct 15, 2018

View reviewed changes

jpbetz force-pushed the watch-e2e-test1 branch from ad16c94 to 88ac549 Compare October 16, 2018 19:35

jpbetz changed the title ~~Add api-machinery 'watch-consistency' e2e conformance test~~ Add api-machinery 'watch-consistency' e2e test Oct 16, 2018

jpbetz removed the area/conformance Issues or PRs related to kubernetes conformance tests label Oct 16, 2018

jpbetz force-pushed the watch-e2e-test1 branch 2 times, most recently from d39bd21 to 8616fbb Compare October 16, 2018 20:57

lavalamp reviewed Oct 19, 2018

View reviewed changes

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

Add api-machinery 'watch-consistency' e2e test

f87d2c6

jpbetz force-pushed the watch-e2e-test1 branch from 8616fbb to f87d2c6 Compare October 19, 2018 21:05

k8s-ci-robot assigned lavalamp Oct 19, 2018

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

k8s-ci-robot merged commit 570f919 into kubernetes:master Oct 20, 2018

jpbetz mentioned this pull request Apr 9, 2019

watch conformance test: Rigorously test watch consistency #67717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add api-machinery 'watch-consistency' e2e test #69829

Add api-machinery 'watch-consistency' e2e test #69829

jpbetz commented Oct 15, 2018 •

edited

lavalamp Oct 15, 2018 •

edited

jpbetz Oct 15, 2018

lavalamp left a comment

lavalamp Oct 15, 2018

jpbetz Oct 16, 2018 •

edited

lavalamp Oct 19, 2018

lavalamp Oct 15, 2018

lavalamp Oct 15, 2018

jpbetz Oct 16, 2018

lavalamp Oct 15, 2018

jpbetz Oct 16, 2018

lavalamp Oct 15, 2018

jpbetz Oct 16, 2018

lavalamp Oct 15, 2018

jpbetz Oct 16, 2018

lavalamp commented Oct 15, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 17, 2018

lavalamp Oct 19, 2018

jpbetz Oct 19, 2018

lavalamp commented Oct 19, 2018

k8s-ci-robot commented Oct 19, 2018

jpbetz commented Oct 19, 2018

jpbetz commented Oct 19, 2018

lavalamp commented Oct 19, 2018

jpbetz commented Oct 19, 2018

fejta-bot commented Oct 20, 2018

Add api-machinery 'watch-consistency' e2e test #69829

Add api-machinery 'watch-consistency' e2e test #69829

Conversation

jpbetz commented Oct 15, 2018 • edited

lavalamp Oct 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbetz Oct 16, 2018 • 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

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

lavalamp commented Oct 15, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 16, 2018

jpbetz commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Oct 19, 2018

k8s-ci-robot commented Oct 19, 2018

jpbetz commented Oct 19, 2018

jpbetz commented Oct 19, 2018

lavalamp commented Oct 19, 2018

jpbetz commented Oct 19, 2018

fejta-bot commented Oct 20, 2018

jpbetz commented Oct 15, 2018 •

edited

lavalamp Oct 15, 2018 •

edited

jpbetz Oct 16, 2018 •

edited