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.

Sign up for GitHub

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

Jump to bottom

⚠️ Use controllerutil.Object for event handling #1118

Merged
merged 1 commit into from
Aug 6, 2020
Merged

⚠️ Use controllerutil.Object for event handling #1118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/builder/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ var _ = Describe("application", func() {
CreateFunc: func(e event.CreateEvent) bool {
defer GinkgoRecover()
// check that it was called only for deployment
Expect(e.Meta).To(BeAssignableToTypeOf(&appsv1.Deployment{}))
Expect(e.Object).To(BeAssignableToTypeOf(&appsv1.Deployment{}))
deployPrctExecuted = true
return true
},
Expand All @@ -261,7 +261,7 @@ var _ = Describe("application", func() {
CreateFunc: func(e event.CreateEvent) bool {
defer GinkgoRecover()
// check that it was called only for replicaset
Expect(e.Meta).To(BeAssignableToTypeOf(&appsv1.ReplicaSet{}))
Expect(e.Object).To(BeAssignableToTypeOf(&appsv1.ReplicaSet{}))
replicaSetPrctExecuted = true
return true
},
Expand All @@ -271,7 +271,7 @@ var _ = Describe("application", func() {
CreateFunc: func(e event.CreateEvent) bool {
defer GinkgoRecover()
//check that it was called for all registered kinds
Expect(e.Meta).Should(Or(
Expect(e.Object).Should(Or(
BeAssignableToTypeOf(&appsv1.Deployment{}),
BeAssignableToTypeOf(&appsv1.ReplicaSet{}),
))
Expand Down
28 changes: 6 additions & 22 deletions pkg/event/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,44 +17,31 @@ limitations under the License.
package event

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
)

// CreateEvent is an event where a Kubernetes object was created. CreateEvent should be generated
// by a source.Source and transformed into a reconcile.Request by an handler.EventHandler.
type CreateEvent struct {
// Meta is the ObjectMeta of the Kubernetes Type that was created
Meta metav1.Object

// Object is the object from the event
Object runtime.Object
Object controllerutil.Object
Copy link
Member

Choose a reason for hiding this comment

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

Sidenote: Should we move this Object interface somewhere else?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think having it under reconcile.Object?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think pretty much all places we have are subpar and this should be in upstream somewhere. We can still move it around freely later and then alias it.

If possible I would like to keep that discussion out of this PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Well, while I agree that there needs to be a better place to have this interface, it feels weird that we're now using an util package throughout our codebase. We might also get into circular dependencies if at any point controllerutil needs to use a symbol from one of the packages that now uses this interface

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but solving this now is premature optimization. If we actually get that problem, we can still move the definition elsewhere and make controllerutil.Object an alias.

I guess my point here is mostly that I think that discussion is outside of the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I guess my point here is mostly that I think that discussion is outside of the scope of this PR.

+1, was brainstorming to follow-up with another PR later, mostly wanted to agree on something and change it before we cut v0.7.0

}

// UpdateEvent is an event where a Kubernetes object was updated. UpdateEvent should be generated
// by a source.Source and transformed into a reconcile.Request by an handler.EventHandler.
type UpdateEvent struct {
// MetaOld is the ObjectMeta of the Kubernetes Type that was updated (before the update)
MetaOld metav1.Object

// ObjectOld is the object from the event
ObjectOld runtime.Object

// MetaNew is the ObjectMeta of the Kubernetes Type that was updated (after the update)
MetaNew metav1.Object
ObjectOld controllerutil.Object

// ObjectNew is the object from the event
ObjectNew runtime.Object
ObjectNew controllerutil.Object
}

// DeleteEvent is an event where a Kubernetes object was deleted. DeleteEvent should be generated
// by a source.Source and transformed into a reconcile.Request by an handler.EventHandler.
type DeleteEvent struct {
// Meta is the ObjectMeta of the Kubernetes Type that was deleted
Meta metav1.Object

// Object is the object from the event
Object runtime.Object
Object controllerutil.Object

// DeleteStateUnknown is true if the Delete event was missed but we identified the object
// as having been deleted.
Expand All @@ -65,9 +52,6 @@ type DeleteEvent struct {
// GenericEvent should be generated by a source.Source and transformed into a reconcile.Request by an
// handler.EventHandler.
type GenericEvent struct {
// Meta is the ObjectMeta of a Kubernetes Type this event is for
Meta metav1.Object

// Object is the object from the event
Object runtime.Object
Object controllerutil.Object
}
30 changes: 15 additions & 15 deletions pkg/handler/enqueue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,31 +35,31 @@ type EnqueueRequestForObject struct{}

// Create implements EventHandler
func (e *EnqueueRequestForObject) Create(evt event.CreateEvent, q workqueue.RateLimitingInterface) {
if evt.Meta == nil {
if evt.Object == nil {
enqueueLog.Error(nil, "CreateEvent received with no metadata", "event", evt)
return
}
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
Name: evt.Meta.GetName(),
Namespace: evt.Meta.GetNamespace(),
Name: evt.Object.GetName(),
Namespace: evt.Object.GetNamespace(),
}})
}

// Update implements EventHandler
func (e *EnqueueRequestForObject) Update(evt event.UpdateEvent, q workqueue.RateLimitingInterface) {
if evt.MetaOld != nil {
if evt.ObjectOld != nil {
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
Name: evt.MetaOld.GetName(),
Namespace: evt.MetaOld.GetNamespace(),
Name: evt.ObjectOld.GetName(),
Namespace: evt.ObjectOld.GetNamespace(),
}})
} else {
enqueueLog.Error(nil, "UpdateEvent received with no old metadata", "event", evt)
}

if evt.MetaNew != nil {
if evt.ObjectNew != nil {
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
Name: evt.MetaNew.GetName(),
Namespace: evt.MetaNew.GetNamespace(),
Name: evt.ObjectNew.GetName(),
Namespace: evt.ObjectNew.GetNamespace(),
}})
} else {
enqueueLog.Error(nil, "UpdateEvent received with no new metadata", "event", evt)
Expand All @@ -68,24 +68,24 @@ func (e *EnqueueRequestForObject) Update(evt event.UpdateEvent, q workqueue.Rate

// Delete implements EventHandler
func (e *EnqueueRequestForObject) Delete(evt event.DeleteEvent, q workqueue.RateLimitingInterface) {
if evt.Meta == nil {
if evt.Object == nil {
enqueueLog.Error(nil, "DeleteEvent received with no metadata", "event", evt)
return
}
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
Name: evt.Meta.GetName(),
Namespace: evt.Meta.GetNamespace(),
Name: evt.Object.GetName(),
Namespace: evt.Object.GetNamespace(),
}})
}

// Generic implements EventHandler
func (e *EnqueueRequestForObject) Generic(evt event.GenericEvent, q workqueue.RateLimitingInterface) {
if evt.Meta == nil {
if evt.Object == nil {
enqueueLog.Error(nil, "GenericEvent received with no metadata", "event", evt)
return
}
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
Name: evt.Meta.GetName(),
Namespace: evt.Meta.GetNamespace(),
Name: evt.Object.GetName(),
Namespace: evt.Object.GetNamespace(),
}})
}
19 changes: 7 additions & 12 deletions pkg/handler/enqueue_mapped.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ limitations under the License.
package handler

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/util/workqueue"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
Expand All @@ -44,23 +43,23 @@ type EnqueueRequestsFromMapFunc struct {

// Create implements EventHandler
func (e *EnqueueRequestsFromMapFunc) Create(evt event.CreateEvent, q workqueue.RateLimitingInterface) {
e.mapAndEnqueue(q, MapObject{Meta: evt.Meta, Object: evt.Object})
e.mapAndEnqueue(q, MapObject{Object: evt.Object})
}

// Update implements EventHandler
func (e *EnqueueRequestsFromMapFunc) Update(evt event.UpdateEvent, q workqueue.RateLimitingInterface) {
e.mapAndEnqueue(q, MapObject{Meta: evt.MetaOld, Object: evt.ObjectOld})
e.mapAndEnqueue(q, MapObject{Meta: evt.MetaNew, Object: evt.ObjectNew})
e.mapAndEnqueue(q, MapObject{Object: evt.ObjectOld})
e.mapAndEnqueue(q, MapObject{Object: evt.ObjectNew})
}

// Delete implements EventHandler
func (e *EnqueueRequestsFromMapFunc) Delete(evt event.DeleteEvent, q workqueue.RateLimitingInterface) {
e.mapAndEnqueue(q, MapObject{Meta: evt.Meta, Object: evt.Object})
e.mapAndEnqueue(q, MapObject{Object: evt.Object})
}

// Generic implements EventHandler
func (e *EnqueueRequestsFromMapFunc) Generic(evt event.GenericEvent, q workqueue.RateLimitingInterface) {
e.mapAndEnqueue(q, MapObject{Meta: evt.Meta, Object: evt.Object})
e.mapAndEnqueue(q, MapObject{Object: evt.Object})
}

func (e *EnqueueRequestsFromMapFunc) mapAndEnqueue(q workqueue.RateLimitingInterface, object MapObject) {
Expand All @@ -87,11 +86,7 @@ type Mapper interface {

// MapObject contains information from an event to be transformed into a Request.
type MapObject struct {
// Meta is the meta data for an object from an event.
Meta metav1.Object

// Object is the object from an event.
Object runtime.Object
Object controllerutil.Object
}

var _ Mapper = ToRequestsFunc(nil)
Expand Down
10 changes: 5 additions & 5 deletions pkg/handler/enqueue_owner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,31 +59,31 @@ type EnqueueRequestForOwner struct {

// Create implements EventHandler
func (e *EnqueueRequestForOwner) Create(evt event.CreateEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.Meta) {
for _, req := range e.getOwnerReconcileRequest(evt.Object) {
q.Add(req)
}
}

// Update implements EventHandler
func (e *EnqueueRequestForOwner) Update(evt event.UpdateEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.MetaOld) {
for _, req := range e.getOwnerReconcileRequest(evt.ObjectOld) {
q.Add(req)
}
for _, req := range e.getOwnerReconcileRequest(evt.MetaNew) {
for _, req := range e.getOwnerReconcileRequest(evt.ObjectNew) {
q.Add(req)
}
}

// Delete implements EventHandler
func (e *EnqueueRequestForOwner) Delete(evt event.DeleteEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.Meta) {
for _, req := range e.getOwnerReconcileRequest(evt.Object) {
q.Add(req)
}
}

// Generic implements EventHandler
func (e *EnqueueRequestForOwner) Generic(evt event.GenericEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.Meta) {
for _, req := range e.getOwnerReconcileRequest(evt.Object) {
q.Add(req)
}
}
Expand Down