Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions pkg/handler/enqueue.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (

var enqueueLog = logf.RuntimeLog.WithName("eventhandler").WithName("EnqueueRequestForObject")

type empty struct{}

var _ EventHandler = &EnqueueRequestForObject{}

// EnqueueRequestForObject enqueues a Request containing the Name and Namespace of the object that is the source of the Event.
Expand All @@ -47,22 +49,18 @@ func (e *EnqueueRequestForObject) Create(evt event.CreateEvent, q workqueue.Rate

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

if evt.ObjectNew != nil {
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
Name: evt.ObjectNew.GetName(),
Namespace: evt.ObjectNew.GetNamespace(),
}})
} else if evt.ObjectOld != nil {
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{
Name: evt.ObjectOld.GetName(),
Namespace: evt.ObjectOld.GetNamespace(),
}})
} else {
enqueueLog.Error(nil, "UpdateEvent received with no new metadata", "event", evt)
enqueueLog.Error(nil, "UpdateEvent received with no metadata", "event", evt)
}
}

Expand Down
7 changes: 6 additions & 1 deletion pkg/handler/enqueue_mapped.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,13 @@ func (e *enqueueRequestsFromMapFunc) Generic(evt event.GenericEvent, q workqueue
}

func (e *enqueueRequestsFromMapFunc) mapAndEnqueue(q workqueue.RateLimitingInterface, object client.Object) {
reqs := map[reconcile.Request]empty{}
Comment thread
coderanger marked this conversation as resolved.
Outdated
for _, req := range e.toRequests(object) {
q.Add(req)
_, ok := reqs[req]
if !ok {
q.Add(req)
reqs[req] = empty{}
}
}
}

Expand Down
32 changes: 17 additions & 15 deletions pkg/handler/enqueue_owner.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,31 +59,37 @@ type EnqueueRequestForOwner struct {

// Create implements EventHandler
func (e *EnqueueRequestForOwner) Create(evt event.CreateEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.Object) {
reqs := map[reconcile.Request]empty{}
e.getOwnerReconcileRequest(evt.Object, reqs)
for req := range reqs {
q.Add(req)
}
}

// Update implements EventHandler
func (e *EnqueueRequestForOwner) Update(evt event.UpdateEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.ObjectOld) {
q.Add(req)
}
for _, req := range e.getOwnerReconcileRequest(evt.ObjectNew) {
reqs := map[reconcile.Request]empty{}
e.getOwnerReconcileRequest(evt.ObjectOld, reqs)
e.getOwnerReconcileRequest(evt.ObjectNew, reqs)
for req := range reqs {
q.Add(req)
}
}

// Delete implements EventHandler
func (e *EnqueueRequestForOwner) Delete(evt event.DeleteEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.Object) {
reqs := map[reconcile.Request]empty{}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed here and in Generic? I would have assumed that kube doesn't allow multiple identical ownerRefs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't actually know what happens if you try to give the same non-controller owner twice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I bet you could have duplicates, +patchMergeKey=uid so it will theoretically dedup on UID but you could have invalid owner refs with the wrong UID and duplicate names? I would also be 100% fine with not handling that cleanly and risking spurious reconciles since that should be very hard to make happen.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I would prefer to omit that here and in Generic

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If not in Delete nor Generic, why have it in Create?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking at the code again, I think we should probably just leave it. It would complicate the flow to only do it sometimes, would have to switch back to returning a slice and then process it again, the extra memory allocations and code complexity seems not worth it.

e.getOwnerReconcileRequest(evt.Object, reqs)
for req := range reqs {
q.Add(req)
}
}

// Generic implements EventHandler
func (e *EnqueueRequestForOwner) Generic(evt event.GenericEvent, q workqueue.RateLimitingInterface) {
for _, req := range e.getOwnerReconcileRequest(evt.Object) {
reqs := map[reconcile.Request]empty{}
e.getOwnerReconcileRequest(evt.Object, reqs)
for req := range reqs {
q.Add(req)
}
}
Expand Down Expand Up @@ -111,17 +117,16 @@ func (e *EnqueueRequestForOwner) parseOwnerTypeGroupKind(scheme *runtime.Scheme)

// getOwnerReconcileRequest looks at object and returns a slice of reconcile.Request to reconcile
// owners of object that match e.OwnerType.
func (e *EnqueueRequestForOwner) getOwnerReconcileRequest(object metav1.Object) []reconcile.Request {
Comment thread
coderanger marked this conversation as resolved.
func (e *EnqueueRequestForOwner) getOwnerReconcileRequest(object metav1.Object, result map[reconcile.Request]empty) {
// Iterate through the OwnerReferences looking for a match on Group and Kind against what was requested
// by the user
var result []reconcile.Request
for _, ref := range e.getOwnersReferences(object) {
// Parse the Group out of the OwnerReference to compare it to what was parsed out of the requested OwnerType
refGV, err := schema.ParseGroupVersion(ref.APIVersion)
if err != nil {
log.Error(err, "Could not parse OwnerReference APIVersion",
"api version", ref.APIVersion)
return nil
return
}

// Compare the OwnerReference Group and Kind against the OwnerType Group and Kind specified by the user.
Expand All @@ -138,18 +143,15 @@ func (e *EnqueueRequestForOwner) getOwnerReconcileRequest(object metav1.Object)
mapping, err := e.mapper.RESTMapping(e.groupKind, refGV.Version)
if err != nil {
log.Error(err, "Could not retrieve rest mapping", "kind", e.groupKind)
return nil
return
}
if mapping.Scope.Name() != meta.RESTScopeNameRoot {
request.Namespace = object.GetNamespace()
}

result = append(result, request)
result[request] = empty{}
}
}

// Return the matches
return result
}

// getOwnersReferences returns the OwnerReferences for an object as specified by the EnqueueRequestForOwner
Expand Down
134 changes: 83 additions & 51 deletions pkg/handler/eventhandler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ var _ = Describe("Eventhandler", func() {
close(done)
})

It("should enqueue a Request with the Name / Namespace of both objects in the UpdateEvent.",
It("should enqueue a Request with the Name / Namespace of one object in the UpdateEvent.",
func(done Done) {
newPod := pod.DeepCopy()
newPod.Name = "baz2"
Expand All @@ -97,18 +97,12 @@ var _ = Describe("Eventhandler", func() {
ObjectNew: newPod,
}
instance.Update(evt, q)
Expect(q.Len()).To(Equal(2))
Expect(q.Len()).To(Equal(1))

i, _ := q.Get()
Expect(i).NotTo(BeNil())
req, ok := i.(reconcile.Request)
Expect(ok).To(BeTrue())
Expect(req.NamespacedName).To(Equal(types.NamespacedName{Namespace: "biz", Name: "baz"}))

i, _ = q.Get()
Expect(i).NotTo(BeNil())
req, ok = i.(reconcile.Request)
Expect(ok).To(BeTrue())
Expect(req.NamespacedName).To(Equal(types.NamespacedName{Namespace: "biz2", Name: "baz2"}))

close(done)
Expand Down Expand Up @@ -212,13 +206,14 @@ var _ = Describe("Eventhandler", func() {
instance.Create(evt, q)
Expect(q.Len()).To(Equal(2))

i, _ := q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}}))

i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz"}}))
i1, _ := q.Get()
i2, _ := q.Get()
Expect([]interface{}{i1, i2}).To(ConsistOf(
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}},
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz"}},
))
})

It("should enqueue a Request with the function applied to the DeleteEvent.", func() {
Expand All @@ -243,13 +238,14 @@ var _ = Describe("Eventhandler", func() {
instance.Delete(evt, q)
Expect(q.Len()).To(Equal(2))

i, _ := q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}}))

i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz"}}))
i1, _ := q.Get()
i2, _ := q.Get()
Expect([]interface{}{i1, i2}).To(ConsistOf(
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}},
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz"}},
))
})

It("should enqueue a Request with the function applied to both objects in the UpdateEvent.",
Expand Down Expand Up @@ -281,20 +277,16 @@ var _ = Describe("Eventhandler", func() {
Expect(q.Len()).To(Equal(4))

i, _ := q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "baz-bar"}}))
Expect(i).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "foo", Name: "baz-bar"}}))
Comment thread
coderanger marked this conversation as resolved.

i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz-baz"}}))
Expect(i).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz-baz"}}))

i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "baz2-bar"}}))
Expect(i).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "foo", Name: "baz2-bar"}}))

i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz2-baz"}}))
Expect(i).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz2-baz"}}))
})

It("should enqueue a Request with the function applied to the GenericEvent.", func() {
Expand All @@ -319,13 +311,14 @@ var _ = Describe("Eventhandler", func() {
instance.Generic(evt, q)
Expect(q.Len()).To(Equal(2))

i, _ := q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}}))

i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz"}}))
i1, _ := q.Get()
i2, _ := q.Get()
Expect([]interface{}{i1, i2}).To(ConsistOf(
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}},
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: "biz", Name: "baz"}},
))
})
})

Expand Down Expand Up @@ -412,13 +405,50 @@ var _ = Describe("Eventhandler", func() {
instance.Update(evt, q)
Expect(q.Len()).To(Equal(2))

i, _ := q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo1-parent"}}))
i1, _ := q.Get()
i2, _ := q.Get()
Expect([]interface{}{i1, i2}).To(ConsistOf(
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo1-parent"}},
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: newPod.GetNamespace(), Name: "foo2-parent"}},
))
})

It("should enqueue a Request with the one duplicate Owner of both objects in the UpdateEvent.", func() {
newPod := pod.DeepCopy()
newPod.Name = pod.Name + "2"

i, _ = q.Get()
instance := handler.EnqueueRequestForOwner{
OwnerType: &appsv1.ReplicaSet{},
}
Expect(instance.InjectScheme(scheme.Scheme)).To(Succeed())
Expect(instance.InjectMapper(mapper)).To(Succeed())

pod.OwnerReferences = []metav1.OwnerReference{
{
Name: "foo-parent",
Kind: "ReplicaSet",
APIVersion: "apps/v1",
},
}
newPod.OwnerReferences = []metav1.OwnerReference{
{
Name: "foo-parent",
Kind: "ReplicaSet",
APIVersion: "apps/v1",
},
}
evt := event.UpdateEvent{
ObjectOld: pod,
ObjectNew: newPod,
}
instance.Update(evt, q)
Expect(q.Len()).To(Equal(1))

i, _ := q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: newPod.GetNamespace(), Name: "foo2-parent"}}))
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo-parent"}}))
})

It("should enqueue a Request with the Owner of the object in the GenericEvent.", func() {
Expand Down Expand Up @@ -659,15 +689,17 @@ var _ = Describe("Eventhandler", func() {
instance.Create(evt, q)
Expect(q.Len()).To(Equal(3))

i, _ := q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo1-parent"}}))
i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo2-parent"}}))
i, _ = q.Get()
Expect(i).To(Equal(reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo3-parent"}}))
i1, _ := q.Get()
i2, _ := q.Get()
i3, _ := q.Get()
Expect([]interface{}{i1, i2, i3}).To(ConsistOf(
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo1-parent"}},
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo2-parent"}},
reconcile.Request{
NamespacedName: types.NamespacedName{Namespace: pod.GetNamespace(), Name: "foo3-parent"}},
))
})
})

Expand Down