Skip to content

✨Add predicates as variadic args for Owns, For, and Watches func #602

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
66 changes: 43 additions & 23 deletions pkg/builder/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,15 @@ var getGvk = apiutil.GVKForObject

// Builder builds a Controller.
type Builder struct {
apiType runtime.Object
mgr manager.Manager
predicates []predicate.Predicate
managedObjects []runtime.Object
watchRequest []watchRequest
config *rest.Config
ctrl controller.Controller
ctrlOptions controller.Options
name string
apiType simpleWatch
mgr manager.Manager
globalPredicates []predicate.Predicate
managedObjects []simpleWatch
watchRequest []watchRequest
config *rest.Config
ctrl controller.Controller
ctrlOptions controller.Options
name string
}

// ControllerManagedBy returns a new controller builder that will be started by the provided Manager
Expand All @@ -63,32 +63,46 @@ func (blder *Builder) ForType(apiType runtime.Object) *Builder {
return blder.For(apiType)
}

// simpleWatch represents the information required to construct a "simple" For
// or Owns watch.
type simpleWatch struct {
item runtime.Object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
item runtime.Object
object runtime.Object

predicates []predicate.Predicate
}

// For defines the type of Object being *reconciled*, and configures the ControllerManagedBy to respond to create / delete /
// update events by *reconciling the object*.
// This is the equivalent of calling
// Watches(&source.Kind{Type: apiType}, &handler.EnqueueRequestForObject{})
func (blder *Builder) For(apiType runtime.Object) *Builder {
blder.apiType = apiType
func (blder *Builder) For(reconciledAPIType runtime.Object, prct ...predicate.Predicate) *Builder {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (blder *Builder) For(reconciledAPIType runtime.Object, prct ...predicate.Predicate) *Builder {
func (blder *Builder) For(object runtime.Object, predicates ...predicate.Predicate) *Builder {

No need for shorter names

blder.apiType = simpleWatch{
item: reconciledAPIType,
predicates: prct,
}
return blder
}

// Owns defines types of Objects being *generated* by the ControllerManagedBy, and configures the ControllerManagedBy to respond to
// create / delete / update events by *reconciling the owner object*. This is the equivalent of calling
// Watches(&source.Kind{Type: <ForType-apiType>}, &handler.EnqueueRequestForOwner{OwnerType: apiType, IsController: true})
func (blder *Builder) Owns(apiType runtime.Object) *Builder {
blder.managedObjects = append(blder.managedObjects, apiType)
func (blder *Builder) Owns(apiType runtime.Object, prct ...predicate.Predicate) *Builder {
Copy link
Member

Choose a reason for hiding this comment

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

@DirectXMan12 Is this the best way to add this functionality? Having varargs here prevents us to add more functionality in the future if we wanted to expose more configurable options.

We could opt instead for a generic interface, similar for what we do for client.List for example.

Copy link
Contributor Author

@mszostok mszostok Feb 13, 2020

Choose a reason for hiding this comment

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

yep, it is a functional options pattern and it could be better in that case at the beginning I just do not want to make it to complicated because I thought I will not break a contract

blder.managedObjects = append(blder.managedObjects, simpleWatch{item: apiType, predicates: prct})
return blder
}

// watchRequest represents the information needed to construct a
// manually-defined (non-"simple") watch.
type watchRequest struct {
src source.Source
eventhandler handler.EventHandler
predicates []predicate.Predicate
}

// Watches exposes the lower-level ControllerManagedBy Watches functions through the builder. Consider using
// Owns or For instead of Watches directly.
func (blder *Builder) Watches(src source.Source, eventhandler handler.EventHandler) *Builder {
blder.watchRequest = append(blder.watchRequest, watchRequest{src: src, eventhandler: eventhandler})
// Specified predicates are registered only for given source.
func (blder *Builder) Watches(src source.Source, eventhandler handler.EventHandler, prct ...predicate.Predicate) *Builder {
blder.watchRequest = append(blder.watchRequest, watchRequest{src: src, eventhandler: eventhandler, predicates: prct})
return blder
}

Expand All @@ -102,9 +116,10 @@ func (blder *Builder) WithConfig(config *rest.Config) *Builder {

// WithEventFilter sets the event filters, to filter which create/update/delete/generic events eventually
// trigger reconciliations. For example, filtering on whether the resource version has changed.
// Given predicate is added for all watched objects.
// Defaults to the empty list.
func (blder *Builder) WithEventFilter(p predicate.Predicate) *Builder {
blder.predicates = append(blder.predicates, p)
blder.globalPredicates = append(blder.globalPredicates, p)
return blder
}

Expand Down Expand Up @@ -157,28 +172,33 @@ func (blder *Builder) Build(r reconcile.Reconciler) (controller.Controller, erro

func (blder *Builder) doWatch() error {
// Reconcile type
src := &source.Kind{Type: blder.apiType}
src := &source.Kind{Type: blder.apiType.item}
hdler := &handler.EnqueueRequestForObject{}
err := blder.ctrl.Watch(src, hdler, blder.predicates...)
allPredicates := append(blder.globalPredicates, blder.apiType.predicates...)
err := blder.ctrl.Watch(src, hdler, allPredicates...)
if err != nil {
return err
}

// Watches the managed types
for _, obj := range blder.managedObjects {
src := &source.Kind{Type: obj}
src := &source.Kind{Type: obj.item}
hdler := &handler.EnqueueRequestForOwner{
OwnerType: blder.apiType,
OwnerType: blder.apiType.item,
IsController: true,
}
if err := blder.ctrl.Watch(src, hdler, blder.predicates...); err != nil {
allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
allPredicates = append(allPredicates, obj.predicates...)
Comment on lines +190 to +191
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified with

allPredicates := append(blder.globalPredicates, w.predicates...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as here: #602 (comment)

if err := blder.ctrl.Watch(src, hdler, allPredicates...); err != nil {
return err
}
}

// Do the watch requests
for _, w := range blder.watchRequest {
if err := blder.ctrl.Watch(w.src, w.eventhandler, blder.predicates...); err != nil {
allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
allPredicates = append(allPredicates, w.predicates...)
Comment on lines +199 to +200
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified with

allPredicates := append(blder.globalPredicates, w.predicates...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was like that at the beginning but @DirectXMan12 is right, it should be copied: #602 (comment)

if err := blder.ctrl.Watch(w.src, w.eventhandler, allPredicates...); err != nil {
return err
}

Expand All @@ -196,7 +216,7 @@ func (blder *Builder) getControllerName() (string, error) {
if blder.name != "" {
return blder.name, nil
}
gvk, err := getGvk(blder.apiType, blder.mgr.GetScheme())
gvk, err := getGvk(blder.apiType.item, blder.mgr.GetScheme())
if err != nil {
return "", err
}
Expand Down
63 changes: 63 additions & 0 deletions pkg/builder/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/scheme"
"sigs.k8s.io/controller-runtime/pkg/source"
Expand Down Expand Up @@ -180,6 +182,67 @@ var _ = Describe("application", func() {
close(done)
}, 10)
})

Describe("Set custom predicates", func() {
It("should execute registered predicates only for assigned kind", func(done Done) {
m, err := manager.New(cfg, manager.Options{})
Expect(err).NotTo(HaveOccurred())

var (
deployPrctExecuted = false
replicaSetPrctExecuted = false
allPrctExecuted = 0
)

deployPrct := predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
defer GinkgoRecover()
// check that it was called only for deployment
Expect(e.Meta).To(BeAssignableToTypeOf(&appsv1.Deployment{}))
deployPrctExecuted = true
return true
},
}

replicaSetPrct := predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
defer GinkgoRecover()
// check that it was called only for replicaset
Expect(e.Meta).To(BeAssignableToTypeOf(&appsv1.ReplicaSet{}))
replicaSetPrctExecuted = true
return true
},
}

allPrct := predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
defer GinkgoRecover()
//check that it was called for all registered kinds
Expect(e.Meta).Should(Or(
BeAssignableToTypeOf(&appsv1.Deployment{}),
BeAssignableToTypeOf(&appsv1.ReplicaSet{}),
))

allPrctExecuted++
return true
},
}

bldr := ControllerManagedBy(m).
For(&appsv1.Deployment{}, deployPrct).
Owns(&appsv1.ReplicaSet{}, replicaSetPrct).
WithEventFilter(allPrct)

doReconcileTest("5", stop, bldr, m, true)

Expect(deployPrctExecuted).To(BeTrue(), "Deploy predicated should be called at least once")
Expect(replicaSetPrctExecuted).To(BeTrue(), "ReplicaSet predicated should be called at least once")
Expect(allPrctExecuted).To(BeNumerically(">=", 2), "Global Predicated should be called at least twice")

close(done)
})
})

})

func doReconcileTest(nameSuffix string, stop chan struct{}, blder *Builder, mgr manager.Manager, complete bool) {
Expand Down