Add ovs meter metric #5165

mengdie-song · 2023-06-21T10:08:28Z

We have implemented rate-limiting for packet-in messages on NetworkPolicy audit logging and Traceflow.
This change adds a metric to show the packet count which is got from meter statistics. A separate goroutine is used here to get the statistics every 30 seconds and collect the metric. The value more than 0 indicates that current rate exceeds predefined limit(100 per second).

Fixes: #5037
Signed-off-by: Mengdie Song [email protected]

pkg/agent/metrics/collector.go

pkg/agent/metrics/prometheus.go

pkg/ovs/ovsctl/ovsctl.go

luolanzone · 2023-06-26T07:20:13Z

@mengdie-song please add unit test for this PR, the patch unit test coverage is quite low.
Btw, the DCO check is failed, you need to provide sign-off info.

pkg/agent/metrics/collector.go

pkg/agent/openflow/meters_others.go

pkg/ovs/ovsctl/ovsctl.go

pkg/agent/metrics/collector.go

pkg/ovs/ovsctl/ovsctl.go

pkg/agent/metrics/collector.go

pkg/agent/metrics/prometheus.go

pkg/ovs/openflow/ofctrl_bridge.go

cmd/antrea-agent/agent.go

pkg/agent/metrics/prometheus.go

pkg/ovs/openflow/ofctrl_bridge.go

pkg/ovs/openflow/ofctrl_bridge_test.go

pkg/agent/openflow/client.go

pkg/ovs/openflow/ofctrl_bridge.go

cmd/antrea-agent/agent.go

tnqn · 2023-07-12T05:01:41Z

test/e2e/prometheus_test.go

@@ -44,6 +44,7 @@ var antreaAgentMetrics = []string{
 	"antrea_agent_ovs_flow_ops_error_count",
 	"antrea_agent_ovs_flow_ops_latency_milliseconds",
 	"antrea_agent_ovs_total_flow_count",
+	"antrea_agent_ovs_ovs_meter_packet_count",


it this correct? two "ovs"?

Nice catch, thanks. It seems that prometheus e2e test is disabled by default on CI. I will double check this one and update.

pkg/agent/metrics/collector.go

pkg/agent/metrics/prometheus.go

pkg/ovs/openflow/ofctrl_bridge.go

antoninbas

Go CI jobs are failing because of build errors

pkg/agent/openflow/client.go

pkg/ovs/openflow/ofctrl_bridge_test.go

pkg/agent/metrics/prometheus.go

docs/prometheus-integration.md

pkg/ovs/openflow/ofctrl_bridge_test.go

antoninbas · 2023-07-17T19:43:24Z

pkg/ovs/openflow/ofctrl_bridge_test.go

+	handleMeterStatsReply := func(packetCounts map[int]int64) {
+		for k, v := range packetCounts {
+			switch k {
+			case 1:
+				metrics.OVSMeterPacketDroppedCount.WithLabelValues("PacketInMeterNetworkPolicy").Set(float64(v))
+			case 2:
+				metrics.OVSMeterPacketDroppedCount.WithLabelValues("PacketInMeterTraceflow").Set(float64(v))


I don't understand why we are using Prometheus metrics at all in this unit test. This handleMeterStatsReply callback function does not need to use metrics. This file is testing the pkg/ovs/openflow package, which has nothing to do with metrics. Instead you could use a map protected by a mutex, or a channel.

I removed the part of checking Prometheus metrics and only checked the packet count in the latest change. Could you help take a look at the latest version?

pkg/agent/openflow/client.go

pkg/ovs/openflow/ofctrl_bridge.go

pkg/agent/openflow/client.go

tnqn · 2023-07-20T10:23:18Z

pkg/ovs/openflow/ofctrl_bridge_test.go

+	}
+	b.mpReplyChsMutex.RUnlock()
+	b.MultipartReply(sw, mpMeterStatsReply)
+	time.Sleep(time.Second)


This is nondeterministic and may be flaky. And having the assertion in the callback can't guarantee it will be called. Even if the tested function does nothing, the test would't fail.

Perhaps you can use a map to collect the received stats and use assert.Eventually to check whether expected stats are received in a reasonable time.

tnqn · 2023-07-21T05:01:42Z

pkg/ovs/openflow/ofctrl_bridge_test.go

+	packetCounts := make(map[int]int64)
+	checkMeterStatsReply := func(meterId int, packetCount int64) {
+		packetCounts[meterId] = packetCount
+	}


This needs mutex or you can use:

var receivedCount1 int64 checkMeterStatsReply := func(meterID int, packetCount int64) { assert.Equal(t, 1, meterID) atomic.StoreInt64(&receivedCount1, packetCount) } ... assert.Eventually(t, func() bool { return int64(100) == atomic.LoadInt64(&receivedCount1) }, ...

Note it's not expected to receive other meterID 2 given your code.

Thanks! I would like to check both meters and have added the mutex.

pkg/ovs/openflow/ofctrl_bridge.go

pkg/ovs/openflow/ofctrl_bridge_test.go

tnqn

LGTM

tnqn · 2023-07-21T12:49:43Z

/test-all
@antoninbas @luolanzone do you have other comments?

luolanzone · 2023-07-21T12:59:41Z

No comment from my side, thanks.

luolanzone

LGTM

pkg/ovs/openflow/ofctrl_bridge_test.go

We have implemented rate-limiting for packet-in messages on NetworkPolicy audit logging and Traceflow. This change adds a metric to show the packet count which is got from meter statistics. A separate goroutine is used here to get the statistics every 30 seconds and collect the metric. The value more than 0 indicates that current rate exceeds predefined limit(100 per second). Fixes: antrea-io#5037 Signed-off-by: Mengdie Song <[email protected]>

tnqn

LGTM

tnqn · 2023-07-25T04:06:50Z

/test-all

tnqn · 2023-07-25T04:10:01Z

As @antoninbas's last comment has been addressed, I'm going to merge the PR given the tight schedule. If there is any new comment after it's merged, we can follow up with new PR.

mengdie-song changed the title ~~Add ovs meter metrics~~ Add ovs meter metric Jun 21, 2023

antoninbas reviewed Jun 22, 2023

View reviewed changes

luolanzone reviewed Jun 27, 2023

View reviewed changes

tnqn reviewed Jun 28, 2023

View reviewed changes

pkg/agent/metrics/collector.go Outdated Show resolved Hide resolved

pkg/ovs/ovsctl/ovsctl.go Outdated Show resolved Hide resolved

pkg/agent/metrics/collector.go Outdated Show resolved Hide resolved

mengdie-song force-pushed the prometheus-ovsmeter branch 5 times, most recently from bf22d1e to 8ee19cb Compare July 4, 2023 08:34

antoninbas reviewed Jul 6, 2023

View reviewed changes

pkg/agent/metrics/prometheus.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

XinShuYang force-pushed the prometheus-ovsmeter branch from 8ee19cb to 257cab9 Compare July 7, 2023 06:06

mengdie-song force-pushed the prometheus-ovsmeter branch 2 times, most recently from 6690e28 to 63bdabb Compare July 11, 2023 09:11

luolanzone reviewed Jul 11, 2023

View reviewed changes

mengdie-song force-pushed the prometheus-ovsmeter branch from 63bdabb to f7ee739 Compare July 11, 2023 10:25

antoninbas reviewed Jul 11, 2023

View reviewed changes

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

tnqn reviewed Jul 12, 2023

View reviewed changes

pkg/agent/metrics/prometheus.go Outdated Show resolved Hide resolved

XinShuYang force-pushed the prometheus-ovsmeter branch from f7ee739 to 79b6a38 Compare July 12, 2023 06:35

antoninbas reviewed Jul 13, 2023

View reviewed changes

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

XinShuYang force-pushed the prometheus-ovsmeter branch from 79b6a38 to a05c1fc Compare July 14, 2023 11:22

antoninbas reviewed Jul 14, 2023

View reviewed changes

jianjuns reviewed Jul 14, 2023

View reviewed changes

pkg/agent/metrics/prometheus.go Outdated Show resolved Hide resolved

pkg/agent/metrics/prometheus.go Outdated Show resolved Hide resolved

docs/prometheus-integration.md Outdated Show resolved Hide resolved

XinShuYang force-pushed the prometheus-ovsmeter branch from a05c1fc to 598c43b Compare July 17, 2023 08:53

antoninbas reviewed Jul 17, 2023

View reviewed changes

XinShuYang force-pushed the prometheus-ovsmeter branch from 598c43b to 7d5b606 Compare July 19, 2023 07:17

luolanzone added this to the Antrea v1.13 release milestone Jul 19, 2023

tnqn reviewed Jul 19, 2023

View reviewed changes

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/ofctrl_bridge.go Outdated Show resolved Hide resolved

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved

XinShuYang force-pushed the prometheus-ovsmeter branch from 7d5b606 to 8f43cda Compare July 20, 2023 07:48

XinShuYang force-pushed the prometheus-ovsmeter branch from 8f43cda to f15d5f7 Compare July 20, 2023 09:53

tnqn reviewed Jul 20, 2023

View reviewed changes

XinShuYang force-pushed the prometheus-ovsmeter branch from f15d5f7 to 259977e Compare July 21, 2023 02:59

tnqn reviewed Jul 21, 2023

View reviewed changes

XinShuYang force-pushed the prometheus-ovsmeter branch from 259977e to c481770 Compare July 21, 2023 08:02

tnqn previously approved these changes Jul 21, 2023

View reviewed changes

luolanzone previously approved these changes Jul 21, 2023

View reviewed changes

antoninbas reviewed Jul 21, 2023

View reviewed changes

pkg/ovs/openflow/ofctrl_bridge_test.go Outdated Show resolved Hide resolved

XinShuYang dismissed stale reviews from luolanzone and tnqn via 417dd54 July 24, 2023 01:25

XinShuYang force-pushed the prometheus-ovsmeter branch from c481770 to 417dd54 Compare July 24, 2023 01:25

tnqn approved these changes Jul 24, 2023

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Jul 25, 2023

tnqn merged commit 2304804 into antrea-io:main Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ovs meter metric #5165

Add ovs meter metric #5165

mengdie-song commented Jun 21, 2023 •

edited

Loading

luolanzone commented Jun 26, 2023 •

edited

Loading

tnqn Jul 12, 2023

mengdie-song Jul 13, 2023

antoninbas left a comment

antoninbas Jul 17, 2023

mengdie-song Jul 19, 2023

tnqn Jul 20, 2023

tnqn Jul 21, 2023

mengdie-song Jul 21, 2023

tnqn left a comment

tnqn commented Jul 21, 2023

luolanzone commented Jul 21, 2023

luolanzone left a comment

tnqn left a comment

tnqn commented Jul 25, 2023

tnqn commented Jul 25, 2023

Add ovs meter metric #5165

Add ovs meter metric #5165

Conversation

mengdie-song commented Jun 21, 2023 • edited Loading

luolanzone commented Jun 26, 2023 • edited Loading

tnqn Jul 12, 2023

Choose a reason for hiding this comment

mengdie-song Jul 13, 2023

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas Jul 17, 2023

Choose a reason for hiding this comment

mengdie-song Jul 19, 2023

Choose a reason for hiding this comment

tnqn Jul 20, 2023

Choose a reason for hiding this comment

tnqn Jul 21, 2023

Choose a reason for hiding this comment

mengdie-song Jul 21, 2023

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jul 21, 2023

luolanzone commented Jul 21, 2023

luolanzone left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jul 25, 2023

tnqn commented Jul 25, 2023

mengdie-song commented Jun 21, 2023 •

edited

Loading

luolanzone commented Jun 26, 2023 •

edited

Loading