✨ Added the LabelSelectorPredicate function for filtering events #1121

VenkatRamaraju · 2020-08-07T21:12:41Z

The LabelSelectorPredicate function implements predicate functions to only reconcile events that contain certain labels.

This allows a user to pre-determine which events will undergo reconciliation via a Selector object that will compare against an event's labels, thereby "filtering" events.

k8s-ci-robot · 2020-08-07T21:12:44Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2020-08-07T21:12:49Z

Welcome @VenkatRamaraju!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-08-07T21:12:49Z

Hi @VenkatRamaraju. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alvaroaleman · 2020-08-07T22:58:49Z

pkg/predicate/predicate.go

+}
+
+// NewLabelSelectorPredicate instantiates a new LabelSelectorPredicate with the LabelSelector specified through parameters.
+func NewLabelSelectorPredicate(s metav1.LabelSelector) (Predicate, error) {


I think the implementation of this can be drastically simplified by making it use NewPredicateFuncs:

return NewPredicateFuncs(func (o controllerutil.Object) bool { selector.Matches(labels.Set(o.GetLabels()) }

NewLabelSelectorPredicate is used to initialize the struct. If there is a way to have all types of predicate functions (Create, Delete, Update, Generic) pass through a "general" function with a "general object" as its parameter, we can simplify the implementation by doing the Matches within that.

I'm a little confused about the implementation you suggested, could you explain a little more how that would work?

func NewLabelSelectorPredicate(s metav1.LabelSelector)(Predicate, error){ selector, err := metav1.LabelSelectorAsSelector(&s) if err != nil {return nil, err} return NewPredicateFuncs(func (o controllerutil.Object) bool { selector.Matches(labels.Set(o.GetLabels()) } }

alvaroaleman · 2020-08-07T22:59:52Z

pkg/predicate/predicate.go

+// If the labels match, the event will undergo reconciliation. If the labels do not match, it will skip reconciliation for that particular event.
+
+type LabelSelectorPredicate struct {
+	Funcs


Why the embedded Funcs? Its unused or not? And why is the type exported?

I realized that the Funcs are available by default. I will remove them from the struct and un-export the struct too.

alvaroaleman · 2020-08-11T20:50:56Z

pkg/predicate/predicate.go

+// EventReconcileFilter takes in a LabelSelector as a parameter and returns predicate functions. The labels of
+// events that are contained as parameters in these predicate functions are compared against the instantiated
+// Selector object and reconciliation of the event will only take place if the labels match.
+func EventReconcileFilter(s metav1.LabelSelector) (Funcs, error) {


Please rename to LabelSelectorPredicate, no one will find this by its current name. Same goes for the PR title.
Also, the PR has merge conflicts and needs to be squashed so there is only one commit.

Will do.

Quick question:
The conflict was caused because a lot of the event struct's fields have changed (since I forked this repo), now taking in a controllerutil object. The events require a controllerutil.Object type (interface), which I believe can be implemented as metav1.Object. However, the type that supports labels as metadata is the metav1.ObjectMeta. How would I initialize a controllerutil.Object -typed object that contains labels in it? That would help me write some tests.

Thanks!

All kube objects that are not lists fullfil the controllerutil.Object interface, for example *corev1.Pod

alvaroaleman · 2020-08-11T21:55:44Z

pkg/predicate/predicate.go

@@ -268,3 +270,16 @@ func (o or) Generic(e event.GenericEvent) bool {
 	}
 	return false
 }
+
+// LabelSelectorPredicate takes in a LabelSelector as a parameter and returns predicate functions. The labels of


Suggested change

// LabelSelectorPredicate takes in a LabelSelector as a parameter and returns predicate functions. The labels of

// LabelSelectorPredicate constructs a Predicate from a LabelSelector. Only objects matching the LabelSelector will be admitted.

alvaroaleman · 2020-08-11T21:56:21Z

pkg/predicate/predicate.go

+// LabelSelectorPredicate takes in a LabelSelector as a parameter and returns predicate functions. The labels of
+// events that are contained as parameters in these predicate functions are compared against the instantiated
+// Selector object and reconciliation of the event will only take place if the labels match.
+func LabelSelectorPredicate(s metav1.LabelSelector) (Funcs, error) {


Please change this to return (Predicate, error). The Funcs are an implementation detail ppl should not rely on.

alvaroaleman · 2020-08-11T21:58:36Z

pkg/predicate/predicate_test.go

@@ -516,4 +516,39 @@ var _ = Describe("Predicate", func() {
 			})
 		})
 	})
+
+	Describe("When checking a LabelSelectorPredicate", func() {
+		instance, err := predicate.LabelSelectorPredicate(*(&metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}))


Why do you first construct a pointer just to instantly dereference it?

alvaroaleman · 2020-08-11T21:59:16Z

pkg/predicate/predicate_test.go

+			It("should return false", func() {
+				failMatch := &corev1.Pod{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels: map[string]string{"bar": "foo"},


Nit: Just using &corev1.Pod{} would be enough, no need to give ti a different set of labels

alvaroaleman

/lgtm
/approve

k8s-ci-robot · 2020-08-11T22:50:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, VenkatRamaraju

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alvaroaleman · 2020-08-11T22:50:38Z

Thanks @VenkatRamaraju !

alvaroaleman · 2020-08-11T23:33:24Z

/retest

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 7, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 7, 2020

k8s-ci-robot requested review from mengqiy and vincepri August 7, 2020 21:12

alvaroaleman reviewed Aug 7, 2020

View reviewed changes

VenkatRamaraju force-pushed the filter-predicate branch from 4cfbfff to 45354e3 Compare August 11, 2020 17:15

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2020

VenkatRamaraju changed the title ~~✨ Added the Label Selector predicate for filtering events~~ ✨ Added the EventReconcileFilter function for filtering events Aug 11, 2020

VenkatRamaraju requested a review from alvaroaleman August 11, 2020 17:17

alvaroaleman reviewed Aug 11, 2020

View reviewed changes

VenkatRamaraju force-pushed the filter-predicate branch from 45354e3 to 3f0bf57 Compare August 11, 2020 21:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2020

VenkatRamaraju changed the title ~~✨ Added the EventReconcileFilter function for filtering events~~ ✨ Added the LabelSelectorPredicate function for filtering events Aug 11, 2020

alvaroaleman reviewed Aug 11, 2020

View reviewed changes

Implementation + Tests

1ee9470

VenkatRamaraju force-pushed the filter-predicate branch from 3f0bf57 to 1ee9470 Compare August 11, 2020 22:18

VenkatRamaraju requested a review from alvaroaleman August 11, 2020 22:32

alvaroaleman approved these changes Aug 11, 2020

View reviewed changes

k8s-ci-robot assigned alvaroaleman Aug 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2020

k8s-ci-robot merged commit 5ef5ed9 into kubernetes-sigs:master Aug 11, 2020

VenkatRamaraju mentioned this pull request Jul 23, 2021

Refactoring code for label-based filtering for Ansible Operators operator-framework/operator-sdk#5086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Added the LabelSelectorPredicate function for filtering events #1121

✨ Added the LabelSelectorPredicate function for filtering events #1121

VenkatRamaraju commented Aug 7, 2020 •

edited

k8s-ci-robot commented Aug 7, 2020

k8s-ci-robot commented Aug 7, 2020

k8s-ci-robot commented Aug 7, 2020

alvaroaleman Aug 7, 2020 •

edited

VenkatRamaraju Aug 10, 2020

alvaroaleman Aug 10, 2020

alvaroaleman Aug 7, 2020

VenkatRamaraju Aug 10, 2020

alvaroaleman Aug 11, 2020

VenkatRamaraju Aug 11, 2020

alvaroaleman Aug 11, 2020

alvaroaleman Aug 11, 2020

alvaroaleman Aug 11, 2020

alvaroaleman Aug 11, 2020

alvaroaleman Aug 11, 2020

alvaroaleman left a comment

k8s-ci-robot commented Aug 11, 2020

alvaroaleman commented Aug 11, 2020

alvaroaleman commented Aug 11, 2020

	// LabelSelectorPredicate takes in a LabelSelector as a parameter and returns predicate functions. The labels of
	// LabelSelectorPredicate constructs a Predicate from a LabelSelector. Only objects matching the LabelSelector will be admitted.

✨ Added the LabelSelectorPredicate function for filtering events #1121

✨ Added the LabelSelectorPredicate function for filtering events #1121

Conversation

VenkatRamaraju commented Aug 7, 2020 • edited

k8s-ci-robot commented Aug 7, 2020

k8s-ci-robot commented Aug 7, 2020

k8s-ci-robot commented Aug 7, 2020

alvaroaleman Aug 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 11, 2020

alvaroaleman commented Aug 11, 2020

alvaroaleman commented Aug 11, 2020

VenkatRamaraju commented Aug 7, 2020 •

edited

alvaroaleman Aug 7, 2020 •

edited