-
Notifications
You must be signed in to change notification settings - Fork 1.2k
⚠️ Migration to the new events API #3262
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
Open
clebs
wants to merge
10
commits into
kubernetes-sigs:main
Choose a base branch
from
clebs:new-events-api
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+327
−64
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f7f07fc
Base migration to the new events API
clebs 0b9ab48
Add support for both the new and old events APIs simultaneously.
clebs a4158e4
Adapt StartEventWatcher to the new API
clebs 538bfb4
Revert renaming of GetEventRecorderFor
clebs 2fca48d
Remove StartEventWatcher call
clebs a5aa9ef
Fix linting issues
clebs 7e63d7a
Improve message when no action is provided for an event.
clebs 2799070
Support both events APIs
clebs bc36d51
Add unit tests for the new events API
clebs 95e07d9
Fix new API not broadcasting events and add integration test
clebs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,16 +24,19 @@ import ( | |
|
||
"github.com/go-logr/logr" | ||
corev1 "k8s.io/api/core/v1" | ||
eventsv1 "k8s.io/api/events/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
corev1client "k8s.io/client-go/kubernetes/typed/core/v1" | ||
"k8s.io/client-go/rest" | ||
"k8s.io/client-go/tools/events" | ||
"k8s.io/client-go/tools/record" | ||
) | ||
|
||
// EventBroadcasterProducer makes an event broadcaster, returning | ||
// whether or not the broadcaster should be stopped with the Provider, | ||
// or not (e.g. if it's shared, it shouldn't be stopped with the Provider). | ||
type EventBroadcasterProducer func() (caster record.EventBroadcaster, stopWithProvider bool) | ||
// This producer currently produces both an old API and a new API broadcaster. | ||
type EventBroadcasterProducer func() (deprecatedCaster record.EventBroadcaster, caster events.EventBroadcaster, stopWithProvider bool) | ||
|
||
// Provider is a recorder.Provider that records events to the k8s API server | ||
// and to a logr Logger. | ||
|
@@ -49,8 +52,11 @@ type Provider struct { | |
makeBroadcaster EventBroadcasterProducer | ||
|
||
broadcasterOnce sync.Once | ||
broadcaster record.EventBroadcaster | ||
stopBroadcaster bool | ||
broadcaster events.EventBroadcaster | ||
stopCh chan struct{} | ||
// Deprecated: will be removed in a future release. Use the broadcaster above instead. | ||
deprecatedBroadcaster record.EventBroadcaster | ||
stopBroadcaster bool | ||
} | ||
|
||
// NB(directxman12): this manually implements Stop instead of Being a runnable because we need to | ||
|
@@ -71,10 +77,12 @@ func (p *Provider) Stop(shutdownCtx context.Context) { | |
// almost certainly already been started (e.g. by leader election). We | ||
// need to invoke this to ensure that we don't inadvertently race with | ||
// an invocation of getBroadcaster. | ||
broadcaster := p.getBroadcaster() | ||
deprecatedBroadcaster, broadcaster := p.getBroadcaster() | ||
if p.stopBroadcaster { | ||
p.lock.Lock() | ||
broadcaster.Shutdown() | ||
close(p.stopCh) | ||
deprecatedBroadcaster.Shutdown() | ||
p.stopped = true | ||
p.lock.Unlock() | ||
} | ||
|
@@ -89,25 +97,35 @@ func (p *Provider) Stop(shutdownCtx context.Context) { | |
|
||
// getBroadcaster ensures that a broadcaster is started for this | ||
// provider, and returns it. It's threadsafe. | ||
func (p *Provider) getBroadcaster() record.EventBroadcaster { | ||
func (p *Provider) getBroadcaster() (record.EventBroadcaster, events.EventBroadcaster) { | ||
// NB(directxman12): this can technically still leak if something calls | ||
// "getBroadcaster" (i.e. Emits an Event) but never calls Start, but if we | ||
// create the broadcaster in start, we could race with other things that | ||
// are started at the same time & want to emit events. The alternative is | ||
// silently swallowing events and more locking, but that seems suboptimal. | ||
|
||
p.broadcasterOnce.Do(func() { | ||
broadcaster, stop := p.makeBroadcaster() | ||
broadcaster.StartRecordingToSink(&corev1client.EventSinkImpl{Interface: p.evtClient}) | ||
broadcaster.StartEventWatcher( | ||
p.deprecatedBroadcaster, p.broadcaster, p.stopBroadcaster = p.makeBroadcaster() | ||
|
||
// init deprecated broadcaster | ||
p.deprecatedBroadcaster.StartRecordingToSink(&corev1client.EventSinkImpl{Interface: p.evtClient}) | ||
p.deprecatedBroadcaster.StartEventWatcher( | ||
func(e *corev1.Event) { | ||
p.logger.V(1).Info(e.Message, "type", e.Type, "object", e.InvolvedObject, "reason", e.Reason) | ||
}) | ||
p.broadcaster = broadcaster | ||
p.stopBroadcaster = stop | ||
|
||
// init new broadcaster | ||
p.stopCh = make(chan struct{}) | ||
p.broadcaster.StartRecordingToSink(p.stopCh) | ||
_, _ = p.broadcaster.StartEventWatcher(func(event runtime.Object) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this returns an error. I think we should handle it in some way. At the very least let's log the error if it's not possible to propagate it up |
||
e, isEvt := event.(*eventsv1.Event) | ||
if isEvt { | ||
p.logger.V(1).Info(e.Note, "type", e.Type, "object", e.Related, "action", e.Action, "reason", e.Reason) | ||
} | ||
}) | ||
}) | ||
|
||
return p.broadcaster | ||
return p.deprecatedBroadcaster, p.broadcaster | ||
} | ||
|
||
// NewProvider create a new Provider instance. | ||
|
@@ -128,6 +146,15 @@ func NewProvider(config *rest.Config, httpClient *http.Client, scheme *runtime.S | |
// GetEventRecorderFor returns an event recorder that broadcasts to this provider's | ||
// broadcaster. All events will be associated with a component of the given name. | ||
func (p *Provider) GetEventRecorderFor(name string) record.EventRecorder { | ||
return &deprecatedRecorder{ | ||
prov: p, | ||
name: name, | ||
} | ||
} | ||
|
||
// GetEventRecorder returns an event recorder that broadcasts to this provider's | ||
// broadcaster. All events will be associated with a component of the given name. | ||
func (p *Provider) GetEventRecorder(name string) events.EventRecorder { | ||
return &lazyRecorder{ | ||
prov: p, | ||
name: name, | ||
|
@@ -141,18 +168,46 @@ type lazyRecorder struct { | |
name string | ||
|
||
recOnce sync.Once | ||
rec record.EventRecorder | ||
rec events.EventRecorder | ||
} | ||
|
||
// ensureRecording ensures that a concrete recorder is populated for this recorder. | ||
func (l *lazyRecorder) ensureRecording() { | ||
l.recOnce.Do(func() { | ||
broadcaster := l.prov.getBroadcaster() | ||
l.rec = broadcaster.NewRecorder(l.prov.scheme, corev1.EventSource{Component: l.name}) | ||
_, broadcaster := l.prov.getBroadcaster() | ||
l.rec = broadcaster.NewRecorder(l.prov.scheme, l.name) | ||
}) | ||
} | ||
|
||
func (l *lazyRecorder) Event(object runtime.Object, eventtype, reason, message string) { | ||
func (l *lazyRecorder) Eventf(regarding runtime.Object, related runtime.Object, eventtype, reason, action, note string, args ...any) { | ||
l.ensureRecording() | ||
|
||
l.prov.lock.RLock() | ||
if !l.prov.stopped { | ||
l.rec.Eventf(regarding, related, eventtype, reason, action, note, args...) | ||
} | ||
l.prov.lock.RUnlock() | ||
} | ||
|
||
// deprecatedRecorder implements the old events API during the tranisiton and will be removed in a future release. | ||
// Deprecated: will be removed in a future release. | ||
type deprecatedRecorder struct { | ||
prov *Provider | ||
name string | ||
|
||
recOnce sync.Once | ||
rec record.EventRecorder | ||
} | ||
|
||
// ensureRecording ensures that a concrete recorder is populated for this recorder. | ||
func (l *deprecatedRecorder) ensureRecording() { | ||
l.recOnce.Do(func() { | ||
deprecatedBroadcaster, _ := l.prov.getBroadcaster() | ||
l.rec = deprecatedBroadcaster.NewRecorder(l.prov.scheme, corev1.EventSource{Component: l.name}) | ||
}) | ||
} | ||
|
||
func (l *deprecatedRecorder) Event(object runtime.Object, eventtype, reason, message string) { | ||
l.ensureRecording() | ||
|
||
l.prov.lock.RLock() | ||
|
@@ -161,7 +216,8 @@ func (l *lazyRecorder) Event(object runtime.Object, eventtype, reason, message s | |
} | ||
l.prov.lock.RUnlock() | ||
} | ||
func (l *lazyRecorder) Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) { | ||
|
||
func (l *deprecatedRecorder) Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...any) { | ||
l.ensureRecording() | ||
|
||
l.prov.lock.RLock() | ||
|
@@ -170,7 +226,8 @@ func (l *lazyRecorder) Eventf(object runtime.Object, eventtype, reason, messageF | |
} | ||
l.prov.lock.RUnlock() | ||
} | ||
func (l *lazyRecorder) AnnotatedEventf(object runtime.Object, annotations map[string]string, eventtype, reason, messageFmt string, args ...interface{}) { | ||
|
||
func (l *deprecatedRecorder) AnnotatedEventf(object runtime.Object, annotations map[string]string, eventtype, reason, messageFmt string, args ...any) { | ||
l.ensureRecording() | ||
|
||
l.prov.lock.RLock() | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems deprecated. Let's use StartRecordingToSinkWithContext instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they forgot to add an empty line above // Deprecated in client-go and that's why the deprecation is not actually working.
Or at least I think that's how staticcheck / Intellij is detecting deprecations, not sure if this is generally applicable.
I took a look, there are a lot of cases in client-go where they have the empty line, but also a lot where they don't have it.
I've started a thread in sig-apimachinery: https://kubernetes.slack.com/archives/C0EG7JC6T/p1754999884784489
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check all
// Deprecated
comments in this PR that they have the empty line. I'll open a PR for mainThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I used this one since it is more straightforward and seems more fitting in this context.
But if it is indeed going away I will change it.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality-wise it's the same (there have been similar changes across a few other components from client-go that we use)