feat(fakeclient): push event on watched channel on add/update/delete #57504

yue9944882 · 2017-12-21T08:00:34Z

What this PR does / why we need it:

This PR enables watch function for kubernetes fakeclient.

This fake client add watchReactorFunction by wrapping watch.NewFake which is a chan Event but actually nothing pushes objects into this channel. So all watch function called by fake client will never return or never receive any object.

This PR intercepts ReactionFunc of Create / Update / DeleteActionImpl and will push the requested object to channel.

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #54075

Special notes for your reviewer:

Release note:

enable watch function for fake client

yue9944882 · 2017-12-21T09:14:33Z

/assign @sttts
/area client-libraries
/kind bug

@justinsb @ncdc PTAL
Any comment or review will be appreciated.

yue9944882 · 2017-12-21T09:16:41Z

BTW I previously submitted a PR and it's merged. Why didn't I have 'Contributor' tag on my dialog?

dims · 2017-12-24T16:30:00Z

/ok-to-test

yue9944882 · 2017-12-25T04:46:18Z

/assign @lavalamp

yue9944882 · 2018-01-16T13:12:38Z

@deads2k Hi~ PTAL, thx.

deads2k · 2018-01-16T13:22:04Z

staging/src/k8s.io/client-go/testing/fixture.go

+	decoder  runtime.Decoder
+	lock     sync.RWMutex
+	objects  map[schema.GroupVersionResource][]runtime.Object
+	watchers map[schema.GroupVersionResource]map[string][]*watch.FakeWatcher


godoc explaining the map keys and values.

deads2k · 2018-01-16T13:22:33Z

Minor comment. Squash into "real" and "generated" commits, then lgtm

/approve

yue9944882 · 2018-01-16T14:06:40Z

@stts @lavalamp @piosz @DirectXMan12
Hi, do you think about approving this PR plz?
Thx a lot.
cc @deads2k

deads2k · 2018-01-16T14:07:44Z

@stts @lavalamp @piosz @DirectXMan12
Hi, do you think about approving this PR plz?
Thx a lot.
cc @deads2k

Don't forget about the squash to two commits

fix races with watch call add test for non-namespace resource watch add matching for all-namespace-watch fix delete namespace watch & restrict test fix multiple invocation on same resource & namespace add descriptive doc for tracker.watchers

yue9944882 · 2018-01-16T14:13:12Z

@deads2k
Squashed. Thx for reminding.

deads2k · 2018-01-16T14:25:53Z

pkg/client/clientset_generated/internalclientset/fake/clientset_generated.go

@@ -71,7 +71,15 @@ func NewSimpleClientset(objects ...runtime.Object) *Clientset {

 	fakePtr := testing.Fake{}
 	fakePtr.AddReactor("*", "*", testing.ObjectReaction(o))
-	fakePtr.AddWatchReactor("*", testing.DefaultWatchReactor(watch.NewFake(), nil))


@sttts where does this get generated from?

yue9944882 · 2018-01-16T14:35:22Z

...ing/src/k8s.io/code-generator/cmd/client-gen/generators/fake/generator_fake_for_clientset.go

@@ -127,7 +127,15 @@ func NewSimpleClientset(objects ...runtime.Object) *Clientset {

 	fakePtr := testing.Fake{}
 	fakePtr.AddReactor("*", "*", testing.ObjectReaction(o))
-	fakePtr.AddWatchReactor("*", testing.DefaultWatchReactor(watch.NewFake(), nil))
+	fakePtr.AddWatchReactor("*", func(action testing.Action) (handled bool, ret watch.Interface, err error) {


@deads2k @sttts I believe it's generated from here.

@deads2k @sttts I believe it's generated from here.

lgtm

deads2k · 2018-01-16T14:38:43Z

@sttts if its good for you, we can apply the label. The other changes are just for the generated code.

deads2k · 2018-01-16T15:53:00Z

/lgtm

k8s-ci-robot · 2018-01-16T15:53:15Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: deads2k, yue9944882

Associated issue: #54075

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/client/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]
staging/src/k8s.io/code-generator/OWNERS
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]
staging/src/k8s.io/metrics/OWNERS
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [deads2k]
staging/src/k8s.io/sample-controller/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-01-16T15:54:51Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-01-16T16:39:03Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue (batch tested with PRs 61195, 61479). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use RaceFreeFakeWatcher in ObjectTracker to fix racy watch panics **What this PR does / why we need it**: The `FakeWatcher` added to `ObjectTracker` in #57504 allows sends on the result channel after it's closed; for example calling `Stop()` then `Add(obj)` will cause a panic. In my experience this has led to flaky tests when informers and controllers are running. Replacing `FakeWatcher` with `RaceFreeFakeWatcher` fixes the problem, since `RaceFreeFakeWatcher` ignores additional events that occur after the watcher is stopped. It also panics instead of blocking when the result channel is full, which seems like a more useful behavior in tests than blocking. I removed the `FakeWatchBufferSize` constant since `RaceFreeFakeWatcher` doesn't take a buffer size argument anymore. This seems fine since the `DefaultChanSize` constant is close to the `FakeWatchBufferSize` value (100 vs 128). **Special notes for your reviewer**: I can provide a minimal repro of a flaky test caused by the earlier behavior if necessary. **Release note**: ```release-note Fix racy panics when using fake watches with ObjectTracker ```

yue9944882 changed the title ~~fix(fakeclient): write event to watch channel on add/update/delete~~ [WIP] fix(fakeclient): write event to watch channel on add/update/delete Dec 21, 2017

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2017

k8s-github-robot assigned justinsb and ncdc Dec 21, 2017

yue9944882 force-pushed the fix-fake-client-dummy-watch branch from 4045923 to de205ec Compare December 21, 2017 08:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 21, 2017

yue9944882 force-pushed the fix-fake-client-dummy-watch branch from de205ec to 9016565 Compare December 21, 2017 09:13

yue9944882 changed the title ~~[WIP] fix(fakeclient): write event to watch channel on add/update/delete~~ fix(fakeclient): write event to watch channel on add/update/delete Dec 21, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2017

k8s-ci-robot assigned sttts Dec 21, 2017

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. area/client-libraries labels Dec 21, 2017

yue9944882 changed the title ~~fix(fakeclient): write event to watch channel on add/update/delete~~ fix(fakeclient): push event on watched channel on add/update/delete Dec 24, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 24, 2017

yue9944882 force-pushed the fix-fake-client-dummy-watch branch from 9016565 to 125a30d Compare December 25, 2017 03:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 25, 2017

k8s-ci-robot assigned lavalamp Dec 25, 2017

yue9944882 mentioned this pull request Dec 26, 2017

Fake clientset not sending events #54075

Closed

yue9944882 force-pushed the fix-fake-client-dummy-watch branch 2 times, most recently from b4c2fb2 to 3efe6a2 Compare December 31, 2017 16:19

deads2k reviewed Jan 16, 2018

View reviewed changes

yue9944882 added 2 commits January 16, 2018 22:11

Regenerating code of fake clientset

6f381ab

yue9944882 force-pushed the fix-fake-client-dummy-watch branch from 08fdfc9 to 6f381ab Compare January 16, 2018 14:12

deads2k reviewed Jan 16, 2018

View reviewed changes

yue9944882 commented Jan 16, 2018

View reviewed changes

k8s-ci-robot assigned deads2k Jan 16, 2018

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

deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2018

k8s-github-robot merged commit 268555a into kubernetes:master Jan 16, 2018

yue9944882 mentioned this pull request Jan 18, 2018

remove unstable resource support & use offical fake watch yue9944882/kubernetes-client-lambda#5

Merged

liggitt mentioned this pull request Feb 8, 2018

podtolerationrestriction: fix informer race in test #59367

Merged

grantr mentioned this pull request Feb 24, 2018

Informers in unit tests do not handle events after they've started knative/serving#214

Closed

yue9944882 mentioned this pull request Feb 25, 2018

Needs implementations for fake client pool constructor #60390

Closed

grantr mentioned this pull request Mar 14, 2018

Use RaceFreeFakeWatcher in ObjectTracker to fix racy watch panics #61195

Merged

ingvagabund mentioned this pull request Jun 20, 2018

Backport pr57504 push event on watch to 1.9 #65269

Closed

misterikkit mentioned this pull request Jun 21, 2018

client-go: using a fake client is difficult and undocumented #65333

Closed

cofyc mentioned this pull request Dec 26, 2018

Use fake client real watch mechanism in PV controller tests #72327

Closed

howardjohn mentioned this pull request Feb 3, 2024

Fake client does not populate watch with initial state #123109

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fakeclient): push event on watched channel on add/update/delete #57504

feat(fakeclient): push event on watched channel on add/update/delete #57504

yue9944882 commented Dec 21, 2017 •

edited

yue9944882 commented Dec 21, 2017 •

edited

yue9944882 commented Dec 21, 2017

dims commented Dec 24, 2017

yue9944882 commented Dec 25, 2017

yue9944882 commented Jan 16, 2018

deads2k Jan 16, 2018

deads2k commented Jan 16, 2018

yue9944882 commented Jan 16, 2018

deads2k commented Jan 16, 2018

yue9944882 commented Jan 16, 2018

deads2k Jan 16, 2018

yue9944882 Jan 16, 2018

deads2k Jan 16, 2018

deads2k commented Jan 16, 2018

deads2k commented Jan 16, 2018

k8s-ci-robot commented Jan 16, 2018

k8s-github-robot commented Jan 16, 2018

k8s-github-robot commented Jan 16, 2018

feat(fakeclient): push event on watched channel on add/update/delete #57504

feat(fakeclient): push event on watched channel on add/update/delete #57504

Conversation

yue9944882 commented Dec 21, 2017 • edited

yue9944882 commented Dec 21, 2017 • edited

yue9944882 commented Dec 21, 2017

dims commented Dec 24, 2017

yue9944882 commented Dec 25, 2017

yue9944882 commented Jan 16, 2018

deads2k Jan 16, 2018

Choose a reason for hiding this comment

deads2k commented Jan 16, 2018

yue9944882 commented Jan 16, 2018

deads2k commented Jan 16, 2018

yue9944882 commented Jan 16, 2018

deads2k Jan 16, 2018

Choose a reason for hiding this comment

yue9944882 Jan 16, 2018

Choose a reason for hiding this comment

deads2k Jan 16, 2018

Choose a reason for hiding this comment

deads2k commented Jan 16, 2018

deads2k commented Jan 16, 2018

k8s-ci-robot commented Jan 16, 2018

k8s-github-robot commented Jan 16, 2018

k8s-github-robot commented Jan 16, 2018

yue9944882 commented Dec 21, 2017 •

edited

yue9944882 commented Dec 21, 2017 •

edited