Add agent networkpolicy metrics #770

yktsubo · 2020-05-30T16:54:13Z

Add agent metrics of total number of ingress/egress network policy rules installed on the node.

antrea_agent_ingress_networkpolicy_rule
The number of ingress networkpolicy rules installed on the node
antrea_agent_egress_networkpolicy_rule
The number of egress networkpolicy rules installed on the node

This PR is a part of #713.

antrea-bot · 2020-05-30T16:54:25Z

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-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-all: to trigger all tests.
/skip-all: to skip all tests.

These commands can only be run by members of the vmware-tanzu organization.

ksamoray · 2020-06-01T19:40:24Z

pkg/agent/metricsstore/metric_cache.go

+)
+
+var (
+	IngressNetworkPolicyCount = metrics.NewGauge(&metrics.GaugeOpts{


Any reason to exclude these metrics from pkg/agent/metrics/prometheus.go? Or at least from pkg/agent/metrics?

Agree with Kobi. These should be part of pkg/agent/metrics IMO.
May be a separate directory like pkg/agent/metrics/networkpolicy will be good.
In future, we can possibly add a way to enable/disable specific metrics in antrea agent. What do you think @ksamoray ?

The reason is to avoid import cycle.
pkg/agent/metrics/prometheus.go use pkg/agent/openflow package
I tried that pkg/agent//openflow/network_policy.go use metrics/prometheus.go, which caused the loop.
What was on my mind was

To implement antrea_agent_ovs_flow_table wihtout using openflow package in prometheus.go

Use another package not to have import cycle.

I think 1st one is better considering the future use?

Can we do the following to resolve the dependency? Have separate directories for definition and callback (if needed) for metrics, and just do initialization in prometheus.go
pkg/agent/metrics/ovs --for pod count (ovs container interface count) and ovs flow table count
pkg/agent/metrics/networkpolicy---for network policy related metrics
pkg/agent/metrics/prometheus.go---for initializing/registration of the metrics. In future, we can selectively register the metrics.

Better to get opinion of @ksamoray too.

Thank you for your comment. Let me check.

I'm guessing that importing pkg/agent/openflow wasn't a good practice on my side.
If possible, I think that it's better to avoid a case where some of the metric implementations import pkg/agent/metrics while others are imported by.
Having said that, it could be painful to reimplement antrea_agent_ovs_flow_table while walking around openflow package. Therefore I think that for now you're solution is valid and so is the proposal by @srikartati
Unless someone has a different proposal - e.g maintaining flow counts as counter metrics instead of using a callback. My openflow package it too limited to conclude this.

As for registering metrics selectively, that would require a whole lot of configuration flags. Maybe a few individual metrics which their collection is costly?

@ksamoray I guess it depends on how we group/classify various metrics and interface we provide to enable those groups. Moreover, muxing of metrics is to provide flexibility to the user who is interested in subset of metrics rather than complexity in tracking metrics.

@yktsubo You probably already saw PR#781. I tried to remove the dependency with other packages. You can now define the new metrics in metrics package. Also, ok with not having separate package for network policy metrics in the current patch. Grouping/classification may probably need more discussion and work.

ksamoray · 2020-06-01T19:58:52Z

test/e2e/prometheus_test.go

@@ -36,6 +36,8 @@ const monitoringNamespace string = "monitoring"
 var antreaAgentMetrics = []string{
 	"antrea_agent_local_pod_count",
 	"antrea_agent_ovs_flow_table",
+	"antrea_agent_ingress_networkpolicy_rule",


I had that list sorted, IDK if that has any importance though :)

Thank you for the comment, I'll change based on your suggestion.

srikartati

Thanks for working on this.
Wondering how you were able to verify these metrics through e2e test? As in, were mock network policies and rules are added and verified?

srikartati · 2020-06-01T22:18:08Z

pkg/agent/metricsstore/metric_cache.go

+)
+
+var (
+	IngressNetworkPolicyCount = metrics.NewGauge(&metrics.GaugeOpts{


Agree with Kobi. These should be part of pkg/agent/metrics IMO.
May be a separate directory like pkg/agent/metrics/networkpolicy will be good.
In future, we can possibly add a way to enable/disable specific metrics in antrea agent. What do you think @ksamoray ?

srikartati · 2020-06-01T22:33:37Z

pkg/agent/openflow/network_policy.go

@@ -738,6 +739,14 @@ func (c *client) InstallPolicyRuleFlows(ruleID uint32, rule *types.PolicyRule, n
 	if err := c.applyConjunctiveMatchFlows(ctxChanges); err != nil {
 		return err
 	}
+
+	// Count up antrea_agent_ingress_networkpolicy_rule or antrea_agent_egress_networkpolicy_rule


Why not installOFRule function in pkg/agent/controller/networkpolicy/reconciler.go?

This function could be used to track number of network policies in addition to the current metric, number of network policy rules.

Thank you for the suggestion, it sounds great. I'll take a look into it.

pkg/agent/metricsstore/metric_cache.go

srikartati · 2020-06-01T22:56:34Z

pkg/agent/openflow/network_policy.go

@@ -944,6 +953,21 @@ func (c *client) UninstallPolicyRuleFlows(ruleID uint32) error {
 		return err
 	}

+	isFound := false
+	for _, ctxChange := range ctxChanges {


ctxChanges seem to be some what complicated.

Is it possible to change following function in reconciler.go to accept Direction or pointer to rule as installOFRule?
func (r *reconciler) uninstallOFRule(ofID uint32)

Thank you for your suggestion, let me take a look into it.

srikartati

Any ideas on testing these metrics if they are tracked correctly?
I see e2e test do not have a way for accuracy of metrics. Looks like it is just tracking the list of metrics.
May be we could create some pods and then use createNetworkPolicy/deleteNetworkpolicy and check for the value of metrics in prometheus e2e test. What do you think?

srikartati · 2020-06-04T18:11:16Z

pkg/agent/controller/networkpolicy/reconciler.go

@@ -368,6 +369,14 @@ func (r *reconciler) installOFRule(ofRule *types.PolicyRule, npName, npNamespace
 		r.idAllocator.release(ofID)
 		return 0, fmt.Errorf("error installing ofRule %v: %v", ofID, err)
 	}
+
+	// Count up antrea_agent_ingress_networkpolicy_rule_count or antrea_agent_egress_networkpolicy_rule_count


Looking at it more closely, installOFRule is also invoked for update of network policy rules (through "update" function). We may double count the rules. Please look at "add" function to get the correct count.

Thank you, Actually I wanted to confirm the point whether we have a case to increment or decrement counters in update.

srikartati · 2020-06-04T18:15:43Z

pkg/agent/controller/networkpolicy/reconciler.go

@@ -398,11 +407,19 @@ func (r *reconciler) updateOFRule(ofID uint32, addedFrom []types.Address, addedT
 	return nil
 }

-func (r *reconciler) uninstallOFRule(ofID uint32) error {
+func (r *reconciler) uninstallOFRule(ofID uint32, flowDirection v1beta1.Direction) error {


Here as well, please look at "Forget" function that calls uninstallOFRule.

Thanks, I'll work on it.

ksamoray · 2020-06-04T19:05:42Z

@srikartati for some of the metrics we can validate with the numbers in the controller, agent CRDs.
For others, not sure really what we can compare with.
Either way, this is not within the scope of this change, and could be handled as a new issue.

srikartati · 2020-06-04T21:38:30Z

@srikartati for some of the metrics we can validate with the numbers in the controller, agent CRDs.
For others, not sure really what we can compare with.
Either way, this is not within the scope of this change, and could be handled as a new issue.

We are adding these metrics by hooking the tracking code in the existing code. IMO testing is necessary to get the code right and will be easily maintainable if there any future changes in existing code. Therefore, I do not completely agree that this is out of scope in this PR.
However, I see your point as tests are not there for other metrics, we could consider this as a separate issue. We can check in the code now and rely on manual testing.

As pointed out in my previous comment, testSetup in Prometheus e2e test can be extended to deploy/delete pods, create/delete network policies etc. At least metrics such as network policy, pod count can be covered. In addition, we may also antctl (tests/e2e/antctl_test) to get the expected values from agent/controller to compare with metrics gathered by API.

srikartati · 2020-06-04T22:12:43Z

However, I see your point as tests are not there for other metrics, we could consider this as a separate issue. We can check in the code now and rely on manual testing.

Added a new issue for improving testing: #799
Thanks.

yktsubo · 2020-06-04T22:31:32Z

@ksamoray @srikartati
Thank you for the comment about testing.
Since we have a new issue, I'll let it go at this.

Add ingress and egress networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Add networkpolicy metrics in agent Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Update register function based on kubernetes library upgrade Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Update metric name Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Use reconcier to implement networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Change based on rebase Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Delete unncessary lines Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Apply gofmt to pkg/agent/metrics/prometheus.go Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Use add and forget to update agent networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Delete unnecessary blanks Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Fix metrics name Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Add direction in unit test Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

srikartati

Thanks for fixing the comments. Were you able to do any manual testing?

yktsubo · 2020-06-08T13:11:12Z

Hi @srikartati
Yes, I tested create new netpol(ingress/egress) and delete them in my lab.
I confirmed that metrics increased and decreased as we expect.

If we you need me to put some testing code in networkpolicy unit test, I can think about it.

srikartati · 2020-06-08T16:36:21Z

Hi @srikartati
Yes, I tested create new netpol(ingress/egress) and delete them in my lab.
I confirmed that metrics increased and decreased as we expect.

If we you need me to put some testing code in networkpolicy unit test, I can think about it.

As discussed before, we can take this up in follow up patch standalone or other metrics: #799

LGTM. It'd be better to get approval from @ksamoray

ksamoray

LGTM

ksamoray · 2020-06-08T18:20:18Z

/test-all

* Add ingress and egress networkpolicy metrics Add ingress and egress networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Add networkpolicy metrics in agent Add networkpolicy metrics in agent Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Update register function based on kubernetes library upgrade Update register function based on kubernetes library upgrade Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Update metric name Update metric name Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Delete unnecessary blanks Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Add metrics to testcase Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Gofmt for agent prometheus.go and metric_cache.go Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Use reconcier to implement networkpolicy metrics Use reconcier to implement networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Change based on rebase Change based on rebase Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Delete unncessary package from go.sum Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Fix mistake by rebase Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Delete unncessary lines Delete unncessary lines Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Apply gofmt to pkg/agent/metrics/prometheus.go Apply gofmt to pkg/agent/metrics/prometheus.go Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Use add and forget to update agent networkpolicy metrics Use add and forget to update agent networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Delete unnecessary blanks Delete unnecessary blanks Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Fix metrics name Fix metrics name Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com> * Add direction in unit test Add direction in unit test Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

vmwclabot added the cla-not-required label May 30, 2020

ksamoray reviewed Jun 1, 2020

View reviewed changes

srikartati reviewed Jun 1, 2020

View reviewed changes

yktsubo force-pushed the add_agent_networkpolicy_metrics branch from bd4b623 to 35651c8 Compare June 4, 2020 15:53

srikartati reviewed Jun 4, 2020

View reviewed changes

Yuki Tsuboi added 17 commits June 7, 2020 22:59

Add ingress and egress networkpolicy metrics

07c49d8

Add ingress and egress networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Add networkpolicy metrics in agent

e89acfb

Add networkpolicy metrics in agent Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Update register function based on kubernetes library upgrade

16754fe

Update register function based on kubernetes library upgrade Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Update metric name

7164e0c

Update metric name Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Delete unnecessary blanks

4fc3bf7

Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Add metrics to testcase

74ddfa4

Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Gofmt for agent prometheus.go and metric_cache.go

601c2d5

Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Use reconcier to implement networkpolicy metrics

55f716c

Use reconcier to implement networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Change based on rebase

673121e

Change based on rebase Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Delete unncessary package from go.sum

1bd55a1

Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Fix mistake by rebase

c78bdb6

Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Delete unncessary lines

6d4feab

Delete unncessary lines Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Apply gofmt to pkg/agent/metrics/prometheus.go

ae1daaf

Apply gofmt to pkg/agent/metrics/prometheus.go Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Use add and forget to update agent networkpolicy metrics

071effd

Use add and forget to update agent networkpolicy metrics Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Delete unnecessary blanks

d02ac4d

Delete unnecessary blanks Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Fix metrics name

88eaed6

Fix metrics name Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

Add direction in unit test

80b3c83

Add direction in unit test Signed-off-by: Yuki Tsuboi <ytsuboi@vmware.com>

yktsubo force-pushed the add_agent_networkpolicy_metrics branch from 5ed689e to 80b3c83 Compare June 7, 2020 14:52

srikartati reviewed Jun 8, 2020

View reviewed changes

srikartati approved these changes Jun 8, 2020

View reviewed changes

ksamoray approved these changes Jun 8, 2020

View reviewed changes

srikartati merged commit 037dfd9 into antrea-io:master Jun 8, 2020

yktsubo deleted the add_agent_networkpolicy_metrics branch June 9, 2020 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add agent networkpolicy metrics #770

Add agent networkpolicy metrics #770

yktsubo commented May 30, 2020

antrea-bot commented May 30, 2020

ksamoray Jun 1, 2020

srikartati Jun 1, 2020

yktsubo Jun 2, 2020 •

edited

Loading

srikartati Jun 2, 2020

yktsubo Jun 2, 2020

ksamoray Jun 2, 2020

ksamoray Jun 2, 2020

srikartati Jun 3, 2020

ksamoray Jun 1, 2020

yktsubo Jun 2, 2020

srikartati left a comment •

edited

Loading

srikartati Jun 1, 2020

srikartati Jun 1, 2020

yktsubo Jun 2, 2020

srikartati Jun 1, 2020

yktsubo Jun 2, 2020

srikartati left a comment

srikartati Jun 4, 2020

yktsubo Jun 4, 2020

srikartati Jun 4, 2020

yktsubo Jun 4, 2020

ksamoray commented Jun 4, 2020

srikartati commented Jun 4, 2020

srikartati commented Jun 4, 2020

yktsubo commented Jun 4, 2020

srikartati left a comment

yktsubo commented Jun 8, 2020

srikartati commented Jun 8, 2020

ksamoray left a comment

ksamoray commented Jun 8, 2020

Add agent networkpolicy metrics #770

Add agent networkpolicy metrics #770

Conversation

yktsubo commented May 30, 2020

antrea-bot commented May 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yktsubo Jun 2, 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

srikartati left a comment • 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

srikartati 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

ksamoray commented Jun 4, 2020

srikartati commented Jun 4, 2020

srikartati commented Jun 4, 2020

yktsubo commented Jun 4, 2020

srikartati left a comment

Choose a reason for hiding this comment

yktsubo commented Jun 8, 2020

srikartati commented Jun 8, 2020

ksamoray left a comment

Choose a reason for hiding this comment

ksamoray commented Jun 8, 2020

yktsubo Jun 2, 2020 •

edited

Loading

srikartati left a comment •

edited

Loading