Implement InjectRecorder #23

lichuqiang · 2018-06-12T13:14:30Z

Address: kubernetes-sigs/kubebuilder#255

pwittrock · 2018-06-13T03:04:03Z

pkg/recorder/recorder.go

+	"k8s.io/client-go/tools/record"
+)
+
+type RecorderGetFuncType func(string, *runtime.Scheme) record.EventRecorder


I don't think the interface should take the Scheme. This should be set by the manager.Manager so users don't need to do the wiring themselves.

If this is an interface instead of a function, we can provide the interface. Maybe RecoderProvider?

pwittrock · 2018-06-13T03:06:27Z

pkg/recorder/recorder.go

+
+func GetEventRecorderFor(name string, scheme *runtime.Scheme) record.EventRecorder {
+	if eventBroadcaster == nil {
+		cs := config.GetKubernetesClientSetOrDie()


We should get the clientset from the manager.Manager, or have users set it on a struct. All components should use the same client.

pwittrock · 2018-06-13T14:55:45Z

pkg/manager/manager.go

+
+	cm.recorderProvider = recorder.NewProvider()
+	cm.recorderProvider.SetScheme(cm.scheme)
+	cm.recorderProvider.SetClientSet(clientconfig.GetKubernetesClientSetOrDieWithConfig(config))


We shouldn't be dying. This function returns an error, we should return an error instead of panicing.

pwittrock · 2018-06-13T14:57:50Z

pkg/recorder/recorder.go

+}
+
+// NewProvider create a new Provider instance.
+func NewProvider() Provider {


The New pattern is useful when step requires synchronous steps, or setup can fail. Since we don't have either in this case, I am not sure how much value this provides.

The eventbroadercaster setup could be done here.

We should either take the deps in the New function or inject them with Inject functions. provider doesn't implement either inject.Scheme or inject.Config so it can't automatically get its deps from manager.SetFields(provider). If we are making the implementation internal, it doesn't really matter which we choose since we can change it without breaking consumers. New is probably the simpler approach for now, so I'd go with that.

e.g. either

// setup clientset and stuff here New(config *rest.Config, s scheme *runtime.Scheme) { ... }

or

// Setup clientset and stuff here (p *provider) InjectClient() {...} (p *provider) InjectScheme() {...}

Yah, I think we do inject for things that cross components, and here we just make it once, so a New function should be enough.

pwittrock · 2018-06-13T15:00:16Z

pkg/recorder/recorder.go

+		return nil, fmt.Errorf("must call SetScheme to specify Scheme before getting recorder")
+	}
+
+	if p.eventBroadcaster == nil {


This need to be threadsafe. Either wrap it in a sync.Once.Do call, or put it in "New"

pwittrock · 2018-06-13T15:02:28Z

pkg/manager/manager.go

@@ -131,5 +133,9 @@ func New(config *rest.Config, options Options) (Manager, error) {

 	cm.fieldIndexes = cm.cache
 	cm.client = client.DelegatingClient{ReadInterface: cm.cache, WriteInterface: writeObj}
+
+	cm.recorderProvider = recorder.NewProvider()


Add a function to manager.go for calling the provider - e.g. add this to the Manager interface and implement it on manager.

type Manager interface { ... // GetRecorder returns a new EventRecorder for the provided name GetRecorder(name string) (record.EventRecorder, error) ... }

what the function for? you mean we should not take the provider as a field of manager, and pass it into the recorder injection?

pwittrock · 2018-06-13T15:11:31Z

pkg/recorder/recorder.go

+}
+
+// NewProvider create a new Provider instance.
+func NewProvider() Provider {


Lets move New under pkg/internal until we are sure we know users will need to get informers directly. They should be able to get them through the manager.Manager.

pwittrock · 2018-06-13T15:11:56Z

pkg/runtime/inject/inject.go

+// Recorder is used by the ControllerManager to inject recorder into Sources, EventHandlers, Predicates, and
+// Reconciles
+type Recorder interface {
+	InjectRecorder(provider recorder.Provider) error


Document this.

pwittrock · 2018-06-13T15:13:13Z

pkg/recorder/recorder_test.go

+package recorder_test
+
+import (
+	"fmt"


Also update the pkg/controller/controller_integration_test.go with a case that uses an EventRecorder and creates and event from the Reconcile loop, then the test checks that the event was recorded.

sure thing, just have not got time to make it today

pwittrock · 2018-06-13T15:14:06Z

Headed in the right direction. Left a few comments on how we want to structure things. Please run test.sh and address the lint errors.

lichuqiang · 2018-06-14T09:30:53Z

have not inject recorder into the components, and want to make clear its boundary:

Whether we really need recorder in all of the components: Reconcilers, Sources, EventHandlers, and Predicates...
Whether we need recorder in all the instances of certain component, or simply that of certain kind.
eg: whether we should implement InjectRecorder in the source interface layer, or just implement it for Channel/Kind source.

pwittrock · 2018-06-15T01:23:59Z

SGTM

pwittrock · 2018-06-15T01:26:37Z

pkg/manager/manager.go

 	return cm, nil
 }
+
+// setOptionsDefaults set default values for Options fields


pwittrock · 2018-06-15T01:26:55Z

pkg/manager/manager_test.go

@@ -442,3 +473,10 @@ func (i *injectable) InjectStopChannel(stop <-chan struct{}) error {
 func (i *injectable) Start(<-chan struct{}) error {
 	return nil
 }
+
+func (i *injectable) InjectRecorder(provider recorder.Provider) error {


If we aren't injecting this, we don't need it.

pwittrock · 2018-06-15T01:27:39Z

pkg/internal/recorder/recorder_test.go

+			provider, err := recorder.NewProvider(cfg, scheme.Scheme)
+			Expect(err).NotTo(HaveOccurred())
+
+			recorder := provider.GetEventRecorderFor("test")


Can you also test that the event recorder will publish events?

pwittrock · 2018-06-15T16:58:24Z

pkg/internal/recorder/recorder_integration_test.go

+			evtWatcher, err := clientset.CoreV1().Events("default").Watch(metav1.ListOptions{})
+			Expect(err).NotTo(HaveOccurred())
+
+			resultEvent := <-evtWatcher.ResultChan()


This is an awesome way to both test the watching of these events and make the test run faster without polling!

pwittrock · 2018-06-15T17:00:02Z

pkg/manager/internal.go

@@ -122,6 +128,10 @@ func (cm *controllerManager) GetCache() cache.Cache {
 	return cm.cache
 }

+func (cm *controllerManager) GetRecorder(name string) record.EventRecorder {


Please add a comment for this function

Implement InjectRecorder

Add .gitignore

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 12, 2018

pwittrock reviewed Jun 13, 2018

View reviewed changes

lichuqiang force-pushed the recorder branch from a28e7e1 to 9dff8e7 Compare June 13, 2018 07:21

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 Jun 13, 2018

lichuqiang force-pushed the recorder branch 2 times, most recently from 8bd21b3 to fb97e26 Compare June 13, 2018 08:29

pwittrock reviewed Jun 13, 2018

View reviewed changes

lichuqiang force-pushed the recorder branch from 0bb2c57 to f2272e2 Compare June 14, 2018 08:52

lichuqiang force-pushed the recorder branch 2 times, most recently from 1fcca7d to d748c8b Compare June 14, 2018 12:10

pwittrock reviewed Jun 15, 2018

View reviewed changes

pkg/manager/manager.go

return cm, nil

}

// setOptionsDefaults set default values for Options fields

Copy link

Contributor

pwittrock Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

pwittrock reviewed Jun 15, 2018

View reviewed changes

lichuqiang force-pushed the recorder branch 2 times, most recently from 65fb07a to 2d0510c Compare June 15, 2018 09:46

lichuqiang changed the title ~~WIP: Implement InjectRecorder~~ Implement InjectRecorder Jun 15, 2018

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 Jun 15, 2018

lichuqiang added 2 commits June 15, 2018 19:57

implement InjectRecorder

86150d4

add recorder test

c8a60c1

lichuqiang force-pushed the recorder branch from 2d0510c to c8a60c1 Compare June 15, 2018 11:57

pwittrock reviewed Jun 15, 2018

View reviewed changes

pwittrock added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 15, 2018

k8s-ci-robot merged commit b4748ac into kubernetes-sigs:master Jun 15, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#23 from lichuqiang/recorder

8bdb185

Implement InjectRecorder

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #23 from pwittrock/fixes

bd65451

Add .gitignore

Implement InjectRecorder #23

Implement InjectRecorder #23

Uh oh!

Conversation

lichuqiang commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwittrock Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwittrock Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwittrock commented Jun 13, 2018

Uh oh!

lichuqiang commented Jun 14, 2018

Uh oh!

pwittrock commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lichuqiang commented Jun 12, 2018 •

edited

Loading

pwittrock Jun 13, 2018 •

edited

Loading

pwittrock Jun 13, 2018 •

edited

Loading