Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External event recorder adhere to EventRecorder #161

Merged
merged 3 commits into from
Oct 19, 2021

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Oct 10, 2021

⚠️ Removes controller.Events.

This change updates the external event recorder to be compatible with the k8s EventRecorder interface and embed the k8s event recorder. Since the events.Recorder implements the k8s EventRecorder, it can be directly used in reconcilers that embed the k8s EventRecorder interface. This makes the controller.Events redundant. The k8s event recorder is embedded in the events.Recorder to be able to post events to k8s and any external recorders.

This also introduces a new event type EventTypeTrace to map with the severity type trace.
Since the kubernetes API only accepts Normal and Warning event types. Sending a Trace event results in:

E1018 10:37:56.133861 1768805 event.go:334] Unsupported event type: 'Trace'

In order to keep the trace events only in k8s, the trace events are passed to k8s as Normal events. Trace events are not passed to the external event recorders, refer #139.

Since the EventRecorder interface methods don't return any error, the external event recorder has a logger to log any event recording related errors.

Replacing corev1.ObjectReference with runtime.Object due to the function signature change required updating the existing tests objects to be runtime.Object compatible.

Closes #134

@darkowlzz darkowlzz changed the title External event recorder adhere to EventRecorder and delegating event recorder External event recorder adhere to EventRecorder Oct 18, 2021
@darkowlzz darkowlzz force-pushed the event-rec-to-k8s-upstream branch 2 times, most recently from a895fbe to 1510ec0 Compare October 18, 2021 06:10
runtime/events/recorder.go Outdated Show resolved Hide resolved
@darkowlzz darkowlzz force-pushed the event-rec-to-k8s-upstream branch 2 times, most recently from d778a96 to 405f475 Compare October 18, 2021 11:19
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Happy with this @darkowlzz, LGTM 🥇

Update external event recorder to be compatible with the upstream k8s
EventRecorder interface.
Since the EventRecorder interface methods don't return any error, add a
logger to the external event recorder to log any errors.

Add an event type constant for trace events.

Update `controller.Events.EventWithMetaf()` to send the available
metadata information to the kubernetes event recorder with
`AnnotatedEventf()`.

Update the tests to work with the function signature changes.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
Since events.Recorder implements the k8s EventRecorder interface, there
is no need of the controller.Events any more. The reconcilers can embed
the k8s EventRecorder and use a events.Recorder recorder.

Update the events.Recorder to embed a k8s event recorder to pass events
to k8s along with an external recorder.

The trace events are passed to k8s recorder as a normal event since it
only accepts normal and warning event types.

Update tests to use testenv with suite_test.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz
Copy link
Contributor Author

Rebased and added object information in the recorder logger.

The error log from the retry test contains the object info, name/namespace:

E1019 00:36:16.462960 2396418 recorder.go:195]  "msg"="unable to record event" "error"="POST http://127.0.0.1:33683 giving up after 3 attempt(s): unexpected HTTP status 500 Internal Server Error" "name"="webapp" "namespace"="gitops-system" 

@stefanprodan
Copy link
Member

stefanprodan commented Oct 19, 2021

For the logs to be compatible with flux logs we need the following fields: reconciler kind, name, namespace. The reconciler kind is the involved object kind.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz
Copy link
Contributor Author

darkowlzz commented Oct 19, 2021

Added reconciler kind. Based on the test logs:

"name"="webapp" "namespace"="gitops-system" "reconciler kind"="ConfigMap"

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @darkowlzz

@hiddeco hiddeco merged commit 5e99e19 into fluxcd:main Oct 19, 2021
@hiddeco hiddeco added area/runtime Controller runtime related issues and pull requests enhancement New feature or request labels Oct 19, 2021
@darkowlzz darkowlzz deleted the event-rec-to-k8s-upstream branch May 28, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/runtime Controller runtime related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

External event recorder should adhere to EventRecorder interface
3 participants