Implement openflow metrics query interface #1140

weiqiangt · 2020-08-24T08:03:51Z

Add flows to calculate packet and session numbers of each NetworkPolicy
rule.
Provide functions to query metrics of each NetworkPolicy rule.

antrea-bot · 2020-08-24T08:04:04Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

pkg/agent/types/networkpolicy.go

weiqiangt · 2020-08-28T06:44:53Z

/test-all

codecov-commenter · 2020-08-28T07:49:43Z

Codecov Report

Merging #1140 into master will increase coverage by 0.08%.
The diff coverage is 65.62%.

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
+ Coverage   56.20%   56.29%   +0.08%     
==========================================
  Files         105      108       +3     
  Lines       11550    12027     +477     
==========================================
+ Hits         6492     6770     +278     
- Misses       4490     4669     +179     
- Partials      568      588      +20

Flag	Coverage Δ
#integration-tests	`46.12% <31.87%> (-1.22%)`	⬇️
#unit-tests	`42.53% <61.53%> (+0.87%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/openflow/client.go	`35.60% <ø> (ø)`
pkg/agent/types/networkpolicy.go	`16.66% <0.00%> (-8.34%)`	⬇️
pkg/ovs/openflow/ofctrl_action.go	`83.79% <ø> (+1.58%)`	⬆️
pkg/agent/openflow/pipeline.go	`57.11% <54.09%> (+0.31%)`	⬆️
pkg/agent/openflow/network_policy.go	`73.61% <66.17%> (-0.84%)`	⬇️
pkg/ovs/openflow/ofctrl_builder.go	`74.91% <100.00%> (+2.60%)`	⬆️
...g/controller/networkpolicy/store/appliedtogroup.go	`84.84% <0.00%> (-3.91%)`	⬇️
pkg/agent/controller/networkpolicy/reconciler.go	`69.07% <0.00%> (-3.16%)`	⬇️
pkg/agent/controller/networkpolicy/cache.go	`77.31% <0.00%> (-2.26%)`	⬇️
pkg/apiserver/storage/ram/store.go	`80.39% <0.00%> (-1.31%)`	⬇️
... and 9 more

pkg/agent/types/networkpolicy.go

tnqn

Could you update the title of the PR and commit to something more specific? "Implement policy metrics" is too high level and may be confusing since this is only a part of "policy metrics", maybe "Implement openflow metrics query interface"

pkg/agent/openflow/network_policy.go

antoninbas · 2020-09-02T23:14:55Z

@weiqiangt @tnqn Apologies. I didn't realize that this PR was part of #1172. I left some relevant comments in that other PR.

weiqiangt · 2020-09-03T09:17:50Z

@weiqiangt @tnqn Apologies. I didn't realize that this PR was part of #1172. I left some relevant comments in that other PR.

Thanks for reviewing, I have updated the code according to your comment.

srikartati

Thanks for the change. I have a couple of comments.

srikartati · 2020-09-08T17:02:38Z

pkg/agent/openflow/network_policy.go

+	m := types.RuleMetric{}
+	pkts, _ := strconv.ParseUint(segs[1][strings.Index(segs[1], "=")+1:], 10, 32)
+	m.Packets = pkts
+	if strings.Contains(segs[4], "+") {


Example format to cover this scenario would be good I guess.

Thanks, added the example format.

srikartati · 2020-09-08T17:04:00Z

pkg/agent/openflow/network_policy.go

+}
+
+func parseMetricFlow(flow string) (uint32, types.RuleMetric) {
+	if strings.Contains(flow, "reg0") {


nit: drop identifier would make this easy to read?

Thanks, added.

srikartati · 2020-09-08T21:20:01Z

pkg/ovs/openflow/ofctrl_builder.go

@@ -184,6 +184,33 @@ func (b *ofFlowBuilder) MatchCTMarkMask(mask uint32) FlowBuilder {
 	return b
 }

+func (b *ofFlowBuilder) MatchCTLabel(high, low uint64) FlowBuilder {


Is this used somewhere?

Nope, removed.

srikartati · 2020-09-08T21:40:47Z

pkg/ovs/openflow/ofctrl_builder.go

+		b.ofFlow.Match.CtLabelHiMask <<= rng[0] - 64
+		b.ofFlow.Match.CtLabelHiMask <<= 127 - rng[1]
+		b.ofFlow.Match.CtLabelHiMask <<= 127 - rng[1]
+		b.ofFlow.Match.CtLabelHiMask = 0


Is this supposed to be CtLabelHiMask as the range doesn't involve LabelLo value?

Thanks for pointing it out. I have updated this part and extract the bit operations to a separate function with a unit test covers.

antoninbas

some minor follow-up comments

antoninbas · 2020-09-10T01:02:11Z

pkg/agent/openflow/network_policy.go

+
+func (c *client) NetworkPolicyMetrics() map[uint32]*types.RuleMetric {
+	result := map[uint32]*types.RuleMetric{}
+	egressFlows, _ := c.ovsctlClient.DumpTableFlows(uint8(EgressMetricTable))


I commented in #1172 (comment). I wasn't suggesting making it a field in the client, just having a local variable for it.

antoninbas · 2020-09-10T01:05:26Z

pkg/agent/openflow/network_policy.go

+	for _, flows := range [][]string{egressFlows, ingressFlows} {
+		for _, flow := range flows {
+			if strings.Contains(flow, "priority=0") {
+				continue
+			}
+			ruleID, metric := parseMetricFlow(flow)
+			// We have two flows for each allow rule. One with ct_state=+new matching calculates session numbers,
+			// and first packets, another ct_state=-new flow is used to calculate following packets. We need to merge
+			// metrics of these two flows to get the right number.
+			if accMetric, ok := result[ruleID]; ok {
+				accMetric.Merge(metric)
+			} else {
+				result[ruleID] = &metric
+			}
+		}
+	}


I actually thought you were going to have something like this:

collectMetricsFromFlows := func(flows []string) { for _, flow := range flows { // ... } } collectMetricsFromFlows(egressFlows) collectMetricsFromFlows(ingressFlows)

(not a big fan of the [][]string and double for loop)

Sure, updated.

pkg/agent/openflow/network_policy.go

pkg/agent/openflow/network_policy_test.go

pkg/agent/openflow/pipeline.go

pkg/ovs/openflow/interfaces.go

weiqiangt · 2020-09-10T08:59:24Z

/test-all

tnqn · 2020-09-10T11:00:25Z

pkg/agent/openflow/network_policy.go

+	pkts, _ := strconv.ParseUint(segs[1][strings.Index(segs[1], "=")+1:], 10, 32)
+	m.Packets = pkts
+	m.Sessions = pkts
+	bytes, _ := strconv.ParseUint(segs[2][strings.Index(segs[2], "=")+1:], 10, 32)


It will overflow once it exceeds 4G bytes? I think all the 3 metrics should be 64 bit size.

Thanks for pointing it out, updated.

pkg/agent/openflow/pipeline.go

tnqn · 2020-09-10T11:52:20Z

pkg/agent/openflow/pipeline.go

+	EgressReg               regType = 5
+	IngressReg              regType = 6
+	TraceflowReg            regType = 9             // Use reg9[28..31] to store traceflow dataplaneTag.
+	cnpDropConjunctionIDReg         = endpointIPReg // marksRegServiceNeedLB indicates a packet need to do service selection.


comment doesn't match, and I think using number directly is more straightforward, people may be confused by endpointIPReg, I thought they have some association before, but they just share the reg, right?

Sure, updated.

tnqn · 2020-09-10T12:00:23Z

pkg/agent/openflow/pipeline.go

+	if !ingress {
+		metricTableID = EgressMetricTable
+		offset = 32
+		labelRange = binding.Range{32, 63}


declaring labelRange as two global variables to avoid repeated allocation, for example low32Range, high32Range?

tnqn · 2020-09-10T12:06:08Z

pkg/agent/openflow/pipeline.go

 		conjReg = EgressReg
+		labelRange = binding.Range{32, 63}


pkg/agent/openflow/pipeline.go

tnqn · 2020-09-10T12:11:18Z

pkg/agent/types/networkpolicy.go

+	Bytes, Packets, Sessions uint64
+}
+
+func (m *RuleMetric) Merge(m1 RuleMetric) {


Use pointer to avoid struct copy?

Sure, updated.

tnqn · 2020-09-10T14:29:08Z

pkg/agent/openflow/pipeline.go

+	// cnpDropConjunctionIDReg reuses reg3 which will also be used for storing endpoint IP to store the rule ID. Since
+	// the service selection will finish when a packet hitting NetworkPolicy related rules, there is no conflict.
+	cnpDropConjunctionIDReg regType = 3
+	marksRegServiceNeedLB   uint32  = 0b001 // marksRegServiceNeedLB indicates a packet need to do service selection.


nit: why changing the comment position? I think the previous one is more common and neat

pkg/agent/openflow/pipeline.go

- Add flows to calculate packet and session numbers of each NetworkPolicy rule. - Provide functions to query metrics of each NetworkPolicy rule. Signed-off-by: Weiqiang Tang <weiqiangt@vmware.com>

tnqn

thanks @weiqiangt, LGTM

weiqiangt · 2020-09-11T03:06:30Z

/test-all

srikartati

I understand that this is merged. Trying to use this patch for adding netpol info in flow records. Have a couple of questions.

What happens to ct_label if both cluster network policy rule and k8s rule are applied on to one flow? Do we support this scenario?

srikartati · 2020-09-16T00:13:23Z

pkg/agent/openflow/network_policy.go

+	bytes, _ := strconv.ParseUint(segs[2][strings.Index(segs[2], "=")+1:], 10, 64)
+	m.Bytes = bytes
+	idRaw := segs[5][strings.Index(segs[5], "0x")+2 : strings.Index(segs[5], "/")]
+	if len(idRaw) > 8 { // only 32 bits are valid.


What happens to egress rule if only 32 bits are valid? I see that in ct_label we use 64 bits like below:

// We use the 0..31 bits of the ct_label to store the ingress rule ID and use the 32..63 bits to store the
// egress rule ID.

~~Thanks for pointing it out, this should be a bug. I will create a PR to fix it.~~

When matching egress rule, there should always be 8 trailing zero while the length of the matching pattern of ingress rule will never exceed 8.
In other words, if the length of idRaw exceeds 8, it must for egress, otherwise it's for ingress.

I see.. makes sense. The comment was a bit confusing and misleading.

weiqiangt · 2020-09-16T07:25:58Z

I understand that this is merged. Trying to use this patch for adding netpol info in flow records. Have a couple of questions.

What happens to ct_label if both cluster network policy rule and k8s rule are applied on to one flow? Do we support this scenario?

For now, only the ID of the last rule which takes effect will be calculated.

srikartati · 2020-09-16T08:27:12Z

I understand that this is merged. Trying to use this patch for adding netpol info in flow records. Have a couple of questions.
What happens to ct_label if both cluster network policy rule and k8s rule are applied on to one flow? Do we support this scenario?

For now, only the ID of the last rule which takes effect will be calculated.

Okay. Maybe using the rest of 64bits is probably the right way I guess, right?

srikartati · 2020-09-16T23:37:23Z

I understand that this is merged. Trying to use this patch for adding netpol info in flow records. Have a couple of questions.
What happens to ct_label if both cluster network policy rule and k8s rule are applied on to one flow? Do we support this scenario?

For now, only the ID of the last rule which takes effect will be calculated.

Okay. Maybe using the rest of 64bits is probably the right way I guess, right?

Please ignore the above comment. I confirmed with @abhiraut that only one rule either from cluster network policy or K8s network policy is being applied.

- Add flows to calculate packet and session numbers of each NetworkPolicy rule. - Provide functions to query metrics of each NetworkPolicy rule. Signed-off-by: Weiqiang Tang <weiqiangt@vmware.com>

vmwclabot added the cla-not-required label Aug 24, 2020

weiqiangt force-pushed the policy-metrics branch 3 times, most recently from 28f25c2 to 2ae1b29 Compare August 25, 2020 04:40

tnqn reviewed Aug 27, 2020

View reviewed changes

pkg/agent/types/networkpolicy.go Outdated Show resolved Hide resolved

weiqiangt force-pushed the policy-metrics branch from 2ae1b29 to 6e82cf0 Compare August 27, 2020 13:12

weiqiangt changed the title ~~[WIP]Implement policy metrics~~ Implement policy metrics Aug 27, 2020

weiqiangt force-pushed the policy-metrics branch from 6e82cf0 to cb0be5d Compare August 28, 2020 03:29

weiqiangt force-pushed the policy-metrics branch from cb0be5d to 84f1af1 Compare August 28, 2020 07:45

tnqn reviewed Aug 31, 2020

View reviewed changes

pkg/agent/types/networkpolicy.go Outdated Show resolved Hide resolved

weiqiangt force-pushed the policy-metrics branch from 84f1af1 to 7432a45 Compare September 2, 2020 01:30

tnqn mentioned this pull request Sep 2, 2020

Support NetworkPolicy statistics #1172

Merged

tnqn reviewed Sep 2, 2020

View reviewed changes

pkg/agent/openflow/network_policy.go Show resolved Hide resolved

tnqn requested a review from wenyingd September 2, 2020 14:57

tnqn mentioned this pull request Sep 2, 2020

Support NetworkPolicy statistics #985

Closed

3 tasks

tnqn added this to the Antrea v0.10.0 release milestone Sep 2, 2020

weiqiangt changed the title ~~Implement policy metrics~~ Implement openflow metrics query interface Sep 3, 2020

weiqiangt force-pushed the policy-metrics branch from 7432a45 to 415714c Compare September 3, 2020 07:38

weiqiangt force-pushed the policy-metrics branch from 415714c to 29371e8 Compare September 3, 2020 09:18

srikartati reviewed Sep 8, 2020

View reviewed changes

weiqiangt force-pushed the policy-metrics branch from 29371e8 to f0f3844 Compare September 9, 2020 05:18

antoninbas reviewed Sep 10, 2020

View reviewed changes

weiqiangt force-pushed the policy-metrics branch from f0f3844 to a75185d Compare September 10, 2020 02:19

weiqiangt mentioned this pull request Sep 10, 2020

Update pipeline.md for policy rule metric flows #1224

Closed

tnqn reviewed Sep 10, 2020

View reviewed changes

weiqiangt force-pushed the policy-metrics branch 2 times, most recently from 9211e3c to 7f55ced Compare September 10, 2020 13:53

tnqn reviewed Sep 10, 2020

View reviewed changes

Implement openflow metrics query interface

3ea1d22

- Add flows to calculate packet and session numbers of each NetworkPolicy rule. - Provide functions to query metrics of each NetworkPolicy rule. Signed-off-by: Weiqiang Tang <weiqiangt@vmware.com>

weiqiangt force-pushed the policy-metrics branch from 7f55ced to 3ea1d22 Compare September 11, 2020 02:14

tnqn approved these changes Sep 11, 2020

View reviewed changes

weiqiangt merged commit be79453 into antrea-io:master Sep 11, 2020

srikartati reviewed Sep 16, 2020

View reviewed changes

weiqiangt deleted the policy-metrics branch September 16, 2020 08:13

Implement openflow metrics query interface #1140

Implement openflow metrics query interface #1140

Conversation

weiqiangt commented Aug 24, 2020 • edited Loading

antrea-bot commented Aug 24, 2020

weiqiangt commented Aug 28, 2020

codecov-commenter commented Aug 28, 2020 • edited Loading

Codecov Report

tnqn left a comment

Choose a reason for hiding this comment

antoninbas commented Sep 2, 2020

weiqiangt commented Sep 3, 2020

srikartati left a comment

Choose a reason for hiding this comment

srikartati Sep 8, 2020 • edited Loading

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

antoninbas 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

weiqiangt commented Sep 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

weiqiangt commented Sep 11, 2020

srikartati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiqiangt Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiqiangt commented Sep 16, 2020

srikartati commented Sep 16, 2020

srikartati commented Sep 16, 2020

weiqiangt commented Aug 24, 2020 •

edited

Loading

codecov-commenter commented Aug 28, 2020 •

edited

Loading

srikartati Sep 8, 2020 •

edited

Loading

weiqiangt Sep 16, 2020 •

edited

Loading