Policy Cache & Policy Processor fixes for #566 by brecode · Pull Request #570 · contiv/vpp

brecode · 2018-02-03T03:52:02Z

This PR addresses comment suggestions of #566.

Refactor code for match_expression and match_label go files to return only []string
instead of (bool, []string)
Simplified getPodsByNSLabelSelector & getPodsByLabelSelector in match_label.go to be
easily readable
Added missing comments for function description and more robust comments in code
Fixed two bugs in processor.go (missing "!" and worng function call)

This PR addresses comment suggestions of contiv#566. - Refactor code for match expression and match label files to return only []string - Simplified getPodsByNSLabelSelector & getPodsByLabelSelector in match_label.go to be easily readable - Added missing comments for function description and more robust comments in code - Fixed two bugs in processor.go (missing "!" and worng function call)

coveralls · 2018-02-03T03:59:29Z

Coverage remained the same at 76.822% when pulling e942add on brecode:master into 0ae2899 on contiv:master.

tiewei

Good to see improvements here, would love to see the changes for the Intersect related improvements and, i think currently getPodsByLabelSelector and getPodsByNSLabelSelector are potentially broken due to empty labels passed

tiewei · 2018-02-03T05:37:49Z

plugins/policy/cache/match_label.go

+		newPodSet := pc.configuredPods.LookupPodsByNSLabelSelector(newNSLabelSelector)
+		current := utils.Intersect(prevPodSet, newPodSet)
+		if len(current) == 0 {
+			return []string{}


you can just break here, but it's not big deal

tiewei · 2018-02-03T05:54:06Z

plugins/policy/cache/cache_impl.go

 	}

-	return nil
+	return []podmodel.ID{}


L107 - L140 can be as simple as

mlPods := pc.getPodsByNSLabelSelector(policyNamespace, matchLabels) mePods := pc.getMatchExpressionPods(policyNamespace, matchExpressions) return utils.UnstringPodID(utils.Intersect(mlPods, mePods))

The empty case should be handled on the methods getPodsByNSLabelSelector and getMatchExpressionPods, they should return empty slice when no labels provided,
Intersectand UnstringPodID are smart enough to be if any of slice is empty, return empty

tiewei · 2018-02-03T05:55:12Z

plugins/policy/cache/match_label.go

-	newPodSet := []string{}
-
+// getPodsByNSLabelSelector returns the pods that match a collection of Label Selectors in the same namespace
+func (pc *PolicyCache) getPodsByNSLabelSelector(namespace string, labels []*policymodel.Policy_Label) []string {


you should check if labels is empty here, otherwise you may will get error at labes[0]

tiewei · 2018-02-03T05:55:50Z

plugins/policy/cache/match_label.go

-// GetPodsByNSLabelSelector returns the pods that match a collection of Label Selectors
-func (pc *PolicyCache) getPodsByLabelSelector(labels []*policymodel.Policy_Label) (bool, []string) {
+// getPodsByLabelSelector returns the pods that match a collection of Label Selectors
+func (pc *PolicyCache) getPodsByLabelSelector(labels []*policymodel.Policy_Label) []string {


you should check if labels is empty, or there will be error

tiewei · 2018-02-03T05:56:34Z

plugins/policy/cache/match_label.go

+		newNamespaceSet := pc.configuredNamespaces.LookupNamespacesByLabelSelector(newNSLabelSelector)
+		current = utils.Intersect(prevNamespaceSet, newNamespaceSet)
+		if len(current) == 0 {
+			return []string{}


i would break here,

tiewei · 2018-02-03T06:36:06Z

plugins/policy/cache/match_expression.go

-		return false, nil
-	}
-	return true, pods
+	return pods


same here, I would say update utils.Intersect to make it smarter on either of them is empty case, like

func Intersect(a []string, b []string) []string { if len(a) == 0 || len(b) == 0 { return []string{} } ... }

or further improve it to take a slice variables

// Intersect at least need two lists func Intersect(a []string, b []string, s ...[]string) []string { if len(a) == 0 || len(b) == 0 { return []string{} } set := make([]string, 0) hash := make(map[string]bool) for _, el := range a { hash[el] = true } for _, el := range b { if _, found := hash[el]; found { set = append(set, el) } } if len(s) == 0 { return set } return Intersect(set, s[0], s[1:]...) }

In this way, a lot places can be just

return utils.Intersect(inPodSet, notInPodSet, existsPodSet, notExistPodSet)

tiewei · 2018-02-03T06:48:00Z

plugins/policy/cache/match_expression.go

@@ -97,17 +97,14 @@ func (pc *PolicyCache) getMatchExpressionPods(namespace string, expressions []*p
 	notExistPodSet = utils.RemoveDuplicates(inPodSet)

 	inMatcher := utils.Intersect(inPodSet, notInPodSet)


aren't L95-L97 bugs ?

notInPodSet = utils.RemoveDuplicates(notInPodSet) existsPodSet = utils.RemoveDuplicates(existsPodSet) notExistPodSet = utils.RemoveDuplicates(notExistPodSet)

- Changed Interesct function to calculate more than two slices. Added an additional check for empty slices. - Added an additional check for empty labels in getPodsByNSLabelSelector and getMatchExpressionPods on file match_label.on. If empty labels empty slice is returned - Simplified cache_implementation code by removing if cases when no labels provided. The if cases are now handled in getPodsByNSLabelSelector and getMatchExpressionPods.

- Changed Interesct function to calculate more than two slices. Added an additional check for empty slices. - Added an additional check for empty labels in getPodsByNSLabelSelector and getMatchExpressionPods on file match_label.on. If empty labels empty slice is returned - Simplified cache_implementation code by removing if cases when no labels provided. The if cases are now handled in getPodsByNSLabelSelector and getMatchExpressionPods. - Removed unecessary function call on duplicates to slices on match_expression.go The duplicate removal is handled by Intersect function

tiewei · 2018-02-04T20:01:07Z

plugins/policy/utils/utils.go


-// Intersect returns the common elements of two slices
-func Intersect(a []string, b []string) []string {
+// Intersect returns the common elements of or more slices


a typo: two or more slices

tiewei · 2018-02-04T20:01:24Z

plugins/policy/utils/utils.go

-// Intersect returns the common elements of two slices
-func Intersect(a []string, b []string) []string {
+// Intersect returns the common elements of or more slices
+// Intersect needs at least two lists


if corrected above line, this line is not needed

tiewei

Just need a fix on godoc of Intersect, rest are good

brecode requested review from jmedved, milanlenco and tiewei February 3, 2018 03:52

tiewei suggested changes Feb 3, 2018

View reviewed changes

tiewei reviewed Feb 3, 2018

View reviewed changes

brecode force-pushed the master branch from 22f14de to f944273 Compare February 3, 2018 22:04

tiewei reviewed Feb 4, 2018

View reviewed changes

tiewei approved these changes Feb 4, 2018

View reviewed changes

Fixed typo on utils.go godoc

e942add

milanlenco merged commit f104b8b into contiv:master Feb 5, 2018

		@@ -97,17 +97,14 @@ func (pc PolicyCache) getMatchExpressionPods(namespace string, expressions []p
		notExistPodSet = utils.RemoveDuplicates(inPodSet)

		inMatcher := utils.Intersect(inPodSet, notInPodSet)

Conversation

brecode commented Feb 3, 2018

Uh oh!

coveralls commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiewei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiewei left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 3, 2018 •

edited

Loading