Fix event recorder scheme #1981
Conversation
k8s-ci-robot
added
size/S
| defer loggingWatch.Stop() | ||
| recordingWatch := eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: k8sKubeClient.CoreV1().Events("")}) | ||
| defer recordingWatch.Stop() | ||
| recorder := eventBroadcaster.NewRecorder(eventsScheme, v1.EventSource{Component: controllerManagerAgentName}) |
There was a problem hiding this comment.
is there any "wait to be ready" type function that needs to be called? I'm thinking of how informers work. I think because this is a push, where that is a watch, we're fine without any such call being necessary.
There was a problem hiding this comment.
No, afaik. Events work for instances and bindings, for example.
There was a problem hiding this comment.
That's my understanding, yes.
| @@ -187,10 +190,25 @@ func Run(controllerManagerOptions *options.ControllerManagerServer) error { | |||
|
|
|||
| // Create event broadcaster | |||
| glog.V(4).Info("Creating event broadcaster") | |||
| eventsScheme := runtime.NewScheme() | |||
| // We use ConfigMapLock/EndpointsLock which emit events for ConfigMap/Endpoints and hence we need core/v1 types for it | |||
|
I had spent some time investigating this issue yesterday and found the Scheme we were using was "bad" - I found that using Scheme from k8s.io/client-go/kubernetes/scheme also addressed the eventing issue, though I'm not certain it had Catalog's types registered. Your fix LGTM. |
|
I was wondering about a test myself but wasn't sure how to even go about testing it? I'll hold off on merging for a sec just in case. |
|
When I was working on a fix separately yesterday, I changed https://github.com/kubernetes-incubator/service-catalog/blob/master/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go#L199 to add Probably achievable with an integration test, but I'm not certain its worth the effort. |
|
Executive Decision: |
Fixes #1628.