Closes #2378: Filtering of watched resources #3275

VenkatRamaraju · 2020-06-22T22:50:23Z

Description of the change:
Added a predicate that will filter resources by labels based on the selectors that are specified in watches.yaml

Motivation for the #change:
Avoid having to do an Ansible initialization before skipping an event.
Fixes the issue: #2378

tengqm · 2020-06-23T05:20:42Z

changelog/fragments/predicateChanges.yaml

+entries:
+  - description: >
+      Added predicate filtering function for labels based on selectors specified in watches.yaml. 
+      Added temporary structs to watches.go for yaml parsing in UnmarshalYAML


line 4 is kinda implementation detail not meant to surface as user facing change logs?

Yeah, what you should put here is a brief description of the feature from the user's perspective, no need to talk about the code base directly

tengqm · 2020-06-23T05:22:01Z

changelog/fragments/predicateChanges.yaml

+
+    migration:
+      header: 
+      body: >


The .DS_Store file below was added by mistake.

tengqm · 2020-06-23T05:26:07Z

pkg/ansible/controller/controller.go

 	if err := c.Watch(&source.Kind{Type: u}, &crthandler.EnqueueRequestForObject{},
-		predicate.GenerationChangedPredicate{}); err != nil {
+		predicate.GenerationChangedPredicate{},
+		predicate.ResourceFilterPredicate{Selector: options.Selector}); err != nil {


I'd suggest we add some vars as arguments to c.Watch() because readability of this whole line is important as well.

tengqm · 2020-06-23T05:27:02Z

pkg/ansible/run.go

@@ -141,6 +141,7 @@ func Run(flags *aoflags.AnsibleOperatorFlags) error {
 			AnsibleDebugLogs: getAnsibleDebugLog(),
 			MaxWorkers:       w.MaxWorkers,
 			ReconcilePeriod:  w.ReconcilePeriod,
+			Selector: 		  w.Selector,


this indentation is weird ... forgot to go fmt your code?

tengqm · 2020-06-23T05:30:03Z

pkg/ansible/watches/watches.go


 	// these are overridden by cmdline flags
 	maxWorkersDefault       = 1
 	ansibleVerbosityDefault = 2
 )

+// Creates, populates and returns Label Selector object during UnmarshalYAML
+func parseTemp(dls tempLabelSelector) metav1.LabelSelector {


any alternative function names in mind?

Moving forward ... this label filtering can be used for other types of operator as well?
Pls ignore this question if we already have this for helm or go.

Yeah, would be good to make it clearer that this is for parsing the label selectors.

wrt the second question, currently this will not be exposed to other operator types, but it could be exposed eventually

tengqm · 2020-06-23T05:37:11Z

pkg/ansible/watches/watches.go

@@ -304,6 +342,8 @@ func Load(path string, maxWorkers, ansibleVerbosity int) ([]Watch, error) {
 			log.Error(err, fmt.Sprintf("Watch with GVK %v failed validation", watch.GroupVersionKind.String()))
 			return nil, err
 		}
+
+		fmt.Printf("%+v", watch)


revise this?

Yeah looks like a leftover debug print, should be removed

fabianvf

Nice first PR! Few changes that should be dropped from the PR, stylistic nits and some best practices, plus tests and docs, but implementation looks sound to me

fabianvf · 2020-06-23T14:09:11Z

changelog/fragments/predicateChanges.yaml

+entries:
+  - description: >
+      Added predicate filtering function for labels based on selectors specified in watches.yaml. 
+      Added temporary structs to watches.go for yaml parsing in UnmarshalYAML


Yeah, what you should put here is a brief description of the feature from the user's perspective, no need to talk about the code base directly

fabianvf · 2020-06-23T14:10:00Z

go.mod

@@ -30,8 +30,9 @@ require (
 	github.com/spf13/viper v1.4.0
 	github.com/stretchr/testify v1.5.1
 	go.uber.org/zap v1.14.1
-	golang.org/x/net v0.0.0-20200301022130-244492dfa37a


This file shouldn't be changed in this PR

revert this change to go.mod

fabianvf · 2020-06-23T14:10:13Z

go.sum

@@ -91,6 +91,7 @@ github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuy
 github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
 github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
+github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d h1:UQZhZ2O0vMHr2cI+DC1Mbh0TJxzA3RcLoMsFw+aXw7E=


Same with this file

fabianvf · 2020-06-23T14:14:06Z

pkg/ansible/watches/watches.go


 	// these are overridden by cmdline flags
 	maxWorkersDefault       = 1
 	ansibleVerbosityDefault = 2
 )

+// Creates, populates and returns Label Selector object during UnmarshalYAML
+func parseTemp(dls tempLabelSelector) metav1.LabelSelector {


Yeah, would be good to make it clearer that this is for parsing the label selectors.

wrt the second question, currently this will not be exposed to other operator types, but it could be exposed eventually

fabianvf · 2020-06-23T14:14:43Z

pkg/ansible/watches/watches.go


 	return nil
 }

-// addRolePlaybookPaths will add the full path based on the current dir


Don't think you meant to delete this word

fabianvf · 2020-06-23T14:15:13Z

pkg/ansible/watches/watches.go

@@ -304,6 +342,8 @@ func Load(path string, maxWorkers, ansibleVerbosity int) ([]Watch, error) {
 			log.Error(err, fmt.Sprintf("Watch with GVK %v failed validation", watch.GroupVersionKind.String()))
 			return nil, err
 		}
+
+		fmt.Printf("%+v", watch)


Yeah looks like a leftover debug print, should be removed

fabianvf · 2020-06-23T14:16:50Z

pkg/predicate/predicate.go

@@ -51,3 +58,37 @@ func (GenerationChangedPredicate) Update(e event.UpdateEvent) bool {
 	}
 	return true
 }
+
+// Skips events that have labels matching a selector defined in watches.yaml
+func EventFilter(eventLabels map[string]string, r ResourceFilterPredicate) bool {


This should be func (r ResourceFilterPredicate) Event filter(eventLabels map[string]string) bool

Also should probably be lowercase eventFilter, this is an implementation detail and not something we should expose outside of this package

fabianvf · 2020-06-23T14:17:46Z

pkg/predicate/predicate.go

+
+	if err != nil {
+		log.Error(nil, "Unable to retrieve requirement", err)
+		return false


Hmm, if we're unable to parse the requirement should our behavior be to reconcile or not? My inclination is to reconcile in this case but curious what others think

fabianvf · 2020-06-23T14:19:28Z

pkg/predicate/predicate.go

+
+// Predicate functions that call the EventFilter Function
+func (r ResourceFilterPredicate) Update(e event.UpdateEvent) bool {
+	return EventFilter(e.MetaNew.GetLabels(), r)


With the signature above, these would turn into r.EventFilter(e.Meta.GetLabels())

fabianvf

The .DS_STORE and cmd/.DS_STORE files should not be committed. Few other comments, but getting close

fabianvf · 2020-06-30T14:03:03Z

test/ansible/molecule/cluster/tasks/selector_test.yml

+    that: cm.data.hello == 'world'
+  vars:
+    cm: "{{ q('k8s', api_version='v1', kind='ConfigMap', namespace=namespace,
+    resource_name='selector-test').0 }}"


We should probably have a second test case where we create a SelectorTest sans label and verify it doesn't reconcile

fabianvf · 2020-06-30T14:06:20Z

pkg/predicate/predicate.go

+}
+
+func NewResourceFilterPredicate(s metav1.LabelSelector) (ResourceFilterPredicate, error) {
+	// fmt.Println(s)


These commented lines should be removed, and there probably doesn't need to be so many newlines (wouldn't normally even comment on that but since you'll be changing the file anyway figured I'd mention it)

agreed on both counts

fabianvf · 2020-06-30T14:09:09Z

pkg/ansible/controller/controller.go

+	filterPredicate, err := predicate.NewResourceFilterPredicate(options.Selector)
+
+	if err != nil {
+		log.Error(nil, "Unable to retreieve requirement", err)


The first argument here should be the error, and the message should make it a little more clear why it exited. The user won't know what you mean by requirement, you need to make it clear that it's an issue with the selector they defined

fabianvf · 2020-06-30T14:09:49Z

go.sum

@@ -91,6 +91,7 @@ github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuy
 github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=


This file should be left unchanged

fabianvf · 2020-06-30T14:10:07Z

go.mod

@@ -30,8 +30,9 @@ require (
 	github.com/spf13/viper v1.4.0
 	github.com/stretchr/testify v1.5.1


This file should be left unchanged

jmrodri

Some minor changes. The biggest one is remove the .DS_Store file from this PR. We do not want that in the repo.

jmrodri · 2020-06-30T14:32:45Z

go.mod

@@ -30,8 +30,9 @@ require (
 	github.com/spf13/viper v1.4.0
 	github.com/stretchr/testify v1.5.1
 	go.uber.org/zap v1.14.1
-	golang.org/x/net v0.0.0-20200301022130-244492dfa37a


revert this change to go.mod

jmrodri · 2020-06-30T14:57:30Z

pkg/ansible/controller/controller.go

 	u := &unstructured.Unstructured{}
 	u.SetGroupVersionKind(options.GVK)
+
+	filterPredicate, err := predicate.NewResourceFilterPredicate(options.Selector)
+


Remove the blank line, this makes it more clear that the err check is for the predicate error.

jmrodri · 2020-06-30T15:00:22Z

pkg/ansible/watches/watches.go

+		requirement := metav1.LabelSelectorRequirement{}
+		requirement.Key = v.Key
+		requirement.Operator = v.Operator
+		requirement.Values = v.Values


Any specific reason you chose to initialize the variable like this? Normally I do it as one statement:

requirement := metav1.LabelSelectorRequirement{ Key: v.Key, Operator: v.Operator, Values: v.Values, }

jmrodri · 2020-06-30T15:01:44Z

pkg/ansible/watches/watches.go

@@ -278,6 +315,7 @@ func Load(path string, maxWorkers, ansibleVerbosity int) ([]Watch, error) {
 	maxWorkersDefault = maxWorkers
 	ansibleVerbosityDefault = ansibleVerbosity
 	b, err := ioutil.ReadFile(path)
+


remove this blank line that was added

jmrodri · 2020-06-30T15:02:36Z

pkg/predicate/predicate.go

+}
+
+func NewResourceFilterPredicate(s metav1.LabelSelector) (ResourceFilterPredicate, error) {
+	// fmt.Println(s)


agreed on both counts

VenkatRamaraju · 2020-07-01T16:25:30Z

Made all the changes.

jmrodri

/lgtm

fabianvf

/lgtm

drewwells · 2020-07-13T14:10:00Z

This would be useful for the go operator too. Adding labels from cli args is tricky b/c the manager.go/manager.Options code is vendored

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2020

openshift-ci-robot requested review from hasbro17 and jmrodri June 22, 2020 22:50

tengqm reviewed Jun 23, 2020

View reviewed changes

fabianvf suggested changes Jun 23, 2020

View reviewed changes

VenkatRamaraju requested a review from fabianvf June 25, 2020 18:35

tengqm mentioned this pull request Jun 28, 2020

docs: Added the selector feature to docs #3302

Merged

fabianvf changed the title ~~[WIP] Closes #2378: Filtering of watched resources~~ Closes #2378: Filtering of watched resources Jun 30, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2020

fabianvf suggested changes Jun 30, 2020

View reviewed changes

jmrodri requested changes Jun 30, 2020

View reviewed changes

Added predicate to filter by labels with a selector

c57b573

VenkatRamaraju requested review from fabianvf and jmrodri July 1, 2020 16:24

jmrodri approved these changes Jul 1, 2020

View reviewed changes

openshift-ci-robot assigned jmrodri Jul 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2020

fabianvf approved these changes Jul 1, 2020

View reviewed changes

openshift-ci-robot assigned fabianvf Jul 1, 2020

fabianvf merged commit d8b7a33 into operator-framework:master Jul 1, 2020

joelanford added the language/ansible Issue is related to an Ansible operator project label Aug 28, 2020

VenkatRamaraju mentioned this pull request Jul 23, 2021

Refactoring code for label-based filtering for Ansible Operators #5086

Merged

		@@ -91,6 +91,7 @@ github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuy
		github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=

		@@ -30,8 +30,9 @@ require (
		github.com/spf13/viper v1.4.0
		github.com/stretchr/testify v1.5.1

Closes #2378: Filtering of watched resources #3275

Closes #2378: Filtering of watched resources #3275

Conversation

VenkatRamaraju commented Jun 22, 2020

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

fabianvf left a 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

Choose a reason for hiding this comment

fabianvf left a 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

jmrodri left a 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

VenkatRamaraju commented Jul 1, 2020

jmrodri left a comment

Choose a reason for hiding this comment

fabianvf left a comment

Choose a reason for hiding this comment

drewwells commented Jul 13, 2020