Fake clientset not sending events

[JosephSalisbury]

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug

/kind feature

What happened:
executing the following code:

package main

import (
	"log"
	"time"

	"k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/client-go/kubernetes/fake"
)

func main() {
	client := fake.NewSimpleClientset()

	go func() {
		time.Sleep(3 * time.Second)

		client.CoreV1().Services("default").Create(&v1.Service{
			ObjectMeta: metav1.ObjectMeta{
				Namespace: "default",
				Name:      "foo",
			},
		})
		log.Printf("service created!")
	}()

	watch, _ := client.CoreV1().Services("default").Watch(metav1.ListOptions{})

	c := watch.ResultChan()

	for {
		select {
		case <-c:
			log.Println("an event occurred!")
		case <-time.After(10 * time.Second):
			log.Fatalf("timeout!")
		}
	}
}

prints

$ ./fake-watch-test
2017/10/17 16:57:06 service created!
2017/10/17 16:57:13 timeout!

What you expected to happen:
i would expect the "an event occurred" line to be printed, when the watch channel receives the event for the service creation

How to reproduce it (as minimally and precisely as possible):
go build the above, execute it.

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version):
  Latest client-go (076e344c86e52f088b78615f815b245f6d613537)
• Cloud provider or hardware configuration**:
• OS (e.g. from /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools:
• Others:

[JosephSalisbury]

/sig api-machinery

[JosephSalisbury]

@kubernetes/sig-api-machinery-bugs

[k8s-ci-robot]

@JosephSalisbury: Reiterating the mentions to trigger a notification:
@kubernetes/sig-api-machinery-bugs

In response to this:

@kubernetes/sig-api-machinery-bugs

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.

[ncdc]

The fake clientset has a WatchReactor for testing watch events. You typically do something like this:

// setup
client := fake.NewSimpleClientset()
watcher := watch.NewFake()
// note, testcore is "k8s.io/client-go/testing"
client.PrependWatchReactor("pods", testcore.DefaultWatchReactor(watcher, nil))

// simulate add/update/delete watch events
watcher.Add(myPod)

// test something that uses the client to do a watch
someController.doSomething(client)

@caesarxuchao @deads2k @sttts do you think it's worth trying to make the fake clientset send out watch events in response to CRUD operations?

[sttts]

Isn't a fake object always a compromise in terms of completeness? It will never be a full apiserver. Would it make tests much easier if it sent out watch events?

[ncdc]

@sttts I'm happy with the watch reactor semantics we have now. I'd vote that if one wants to test the client CRUD -> watch event flow, it should be an integration test with an apiserver in the middle.

[JosephSalisbury]

For context, I'm trying to test a controller/operator that uses a watch internally. Passing in a fake clientset is straightforward, having to simulate the watch events is a bit trickier.

I would agree that a larger integration test is probably the 'better' way, but I would like to be able to unit-test watches.

Happy to provide some PRs, if there consensus on the value here.

[deads2k]

I think it would be cool, but I suspect the cost will be significant in terms of updating all the usage locations. I wouldn't undertake it myself, but I wouldn't object to seeing it.

@ncdc @sttts do you actually object or just think its not worth the cost?

[ncdc]

I would be fine if someone undertook the effort to make the fake clientset generate watch events, but I think we have other avenues we can use right now that make it possible to test (e.g. making the sync handler a function pointer so it's easier to unit test, using the fake watch + reactor pattern).

@JosephSalisbury maybe you could expand a bit on how your code is structured and we can see if we can help you determine a way to unit test it?

[sttts]

do you actually object or just think its not worth the cost?

I don't object. If added complexity and effort to implement it pays off enough in test coverage & simplicity of tests.

[caesarxuchao]

I would be fine if someone undertook the effort to make the fake clientset generate watch events, but I think we have other avenues we can use right now that make it possible to test (e.g. making the sync handler a function pointer so it's easier to unit test, using the fake watch + reactor pattern).

I couldn't have said it any better.

[deads2k]

Happy to provide some PRs, if there consensus on the value here.

@JosephSalisbury sounds like everyone agrees it would be fine to do, but that it's likely to be a lot of work and we're not prepared to do it ourselves. If you want to undertake the effort, I'd suggest starting with converting just a few exemplar packages and having one (or more) of us four look at your approach before tackling the entire codebase.

[yue9944882]

@deads2k @sttts @ncdc @JosephSalisbury @caesarxuchao
I've implemented a very simple watch method for fake client.#57504
And these implementation will effect nothing seriously currently. WDYT?