[Multicast] removal of stale multicast routes #3242

ceclinux · 2022-01-25T08:23:59Z

This commit removes the relevant stale multicast routes. When multicast sender Pods are deleted or stop sending multicast traffic in 10 minutes, the related multicast routes should be considered as stale and are removed with this commit. MRouted implements a similar stale routes cleanup mechanism in OpenBSD and NetBSD.

Periodically cleaning stale multicast routes would improve the readability of multicast routes, such as the readability of "ip mroute" output. Also, the memory consumption of Nodes could be slightly reduced(
running 28 multicast senders, each sending multicast traffic to 2000 multicast groups, around 16M Slab Unclaimable memory was consumed by 56000 multicast route entries, observed from Netdata).

codecov-commenter · 2022-01-25T09:54:47Z

Codecov Report

Attention: Patch coverage is 0.87719% with 113 lines in your changes are missing coverage. Please review.

Project coverage is 65.58%. Comparing base (5843387) to head (555050f).
Report is 3 commits behind head on main.

❗ Current head 555050f differs from pull request most recent head 3310ad2. Consider uploading reports for the commit 3310ad2 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3242      +/-   ##
==========================================
- Coverage   72.72%   65.58%   -7.14%     
==========================================
  Files         427      390      -37     
  Lines       66544    59043    -7501     
==========================================
- Hits        48393    38725    -9668     
- Misses      14981    17600    +2619     
+ Partials     3170     2718     -452

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.34% <ø> (+7.21%)`	⬆️	Carriedforward from 9df5a81
integration-tests	`34.48% <0.00%> (+2.16%)`	⬆️
kind-e2e-tests	`46.95% <0.00%> (+13.27%)`	⬆️
unit-tests	`57.77% <2.56%> (-9.40%)`	⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
pkg/agent/multicast/mcast_socket_others.go	`0.00% <0.00%> (ø)`
pkg/agent/util/syscall/syscall_unix.go	`0.00% <0.00%> (ø)`
pkg/agent/multicast/mcast_socket_linux.go	`0.00% <0.00%> (ø)`
pkg/agent/multicast/mcast_route.go	`23.52% <2.85%> (-42.21%)`	⬇️
pkg/agent/multicast/mcast_route_linux.go	`0.00% <0.00%> (-41.31%)`	⬇️

... and 339 files with indirect coverage changes

ceclinux · 2022-02-09T09:01:31Z

/test-multicast-e2e

ceclinux · 2022-02-09T09:55:09Z

/test-multicast-e2e

ceclinux · 2022-02-10T08:45:42Z

/test-multicast-e2e

ceclinux · 2022-02-11T01:51:47Z

/test-multicast-e2e

ceclinux · 2022-02-11T02:17:04Z

/test-multicast-e2e

ceclinux · 2022-03-17T02:57:00Z

/test-multicast-e2e

wenyingd · 2024-04-01T06:23:28Z

pkg/agent/multicast/mcast_route_linux.go

+func (c *MRouteClient) updateInboundMrouteStats() {
+	for _, obj := range c.inboundRouteCache.List() {
+		entry := obj.(*inboundMulticastRouteEntry)
+		isDeleted, newEntry := c.updateMulticastRouteStatsEntry(&entry.multicastRouteEntry)


Suggested change

isDeleted, newEntry := c.updateMulticastRouteStatsEntry(&entry.multicastRouteEntry)

isStale, newEntry := c.updateMulticastRouteStatsEntry(&entry.multicastRouteEntry)

wenyingd · 2024-04-01T06:30:59Z

pkg/agent/multicast/mcast_route_linux.go

+func (c *MRouteClient) updateOutboundMrouteStats() {
+	for _, obj := range c.outboundRouteCache.List() {
+		entry := obj.(*outboundMulticastRouteEntry)
+		isDeleted, newEntry := c.updateMulticastRouteStatsEntry(&entry.multicastRouteEntry)


wenyingd · 2024-04-01T06:40:02Z

pkg/agent/util/syscall/ztypes_linux.go

+	Grp     [4]byte /* in_addr */
+	Pktcnt  uint32
+	Bytecnt uint32
+	If      uint32


What is the field "If" meaning, iif or oif ?

IIF. This generated field is not used in our code. According to the Linux source code, the name should be Wrong_if, which is assigned by the wrong_if field in mr_mfc https://github.com/torvalds/linux/blob/master/include/linux/mroute_base.h#L115-L160

wenyingd · 2024-04-01T06:46:05Z

pkg/agent/multicast/mcast_route.go

 // inboundMulticastRouteEntry encodes the inbound multicast routing entry.
+// It has extra field Iif to represent inbound interface VIF.


Suggested change

// It has extra field Iif to represent inbound interface VIF.

// It has extra field vif to represent inbound interface VIF.

wenyingd · 2024-04-01T06:46:29Z

pkg/agent/multicast/mcast_route.go

+// outboundMulticastRouteEntry encodes the outbound multicast routing entry.
+// For example,
+//
+//	type inboundMulticastRouteEntry struct {


Suggested change

// type inboundMulticastRouteEntry struct {

// type outboundMulticastRouteEntry struct {

wenyingd · 2024-04-01T06:58:12Z

pkg/agent/multicast/mcast_route_test.go

+func TestUpdateOutboundMrouteStats(t *testing.T) {
+	mRoute := newMockMulticastRouteClient(t)
+	err := mRoute.initialize(t)
+	assert.Nil(t, err)


use "require.NoError(t, err)" to ensure the succeeding logic can run as expected.

wenyingd · 2024-04-01T07:03:39Z

pkg/agent/multicast/mcast_route_test.go

+func TestUpdateInboundMrouteStats(t *testing.T) {
+	mRoute := newMockMulticastRouteClient(t)
+	err := mRoute.initialize(t)
+	assert.Nil(t, err)


wenyingd · 2024-04-01T07:04:31Z

pkg/agent/multicast/mcast_route_test.go

+			mockMulticastSocket.EXPECT().DelMrouteEntry(net.ParseIP(m.source), net.ParseIP(m.group), uint16(0)).Times(1)
+		}
+	}
+	mRoute.updateMrouteStats()


Would you also verify if the stale entry is finally deleted from the cache? Also add this verification in TestUpdateInboundMrouteStats?

ceclinux · 2024-04-02T00:13:02Z

/test-multicast-e2e

ceclinux · 2024-04-02T00:13:05Z

/test-multicast-e2e

ceclinux · 2024-04-02T00:13:08Z

/test-multicast-e2e

ceclinux · 2024-04-02T00:13:37Z

/test-flexible-ipam-e2e

ceclinux · 2024-04-02T00:13:40Z

/test-flexible-ipam-e2e

ceclinux · 2024-04-02T00:13:44Z

/test-flexible-ipam-e2e

wenyingd

One comment, others LGTM.

BTW, don't forget to rebase your patch on main branch, and resolve candidate conflicts.

wenyingd · 2024-04-08T02:57:29Z

pkg/agent/multicast/mcast_route_linux.go

 	for i := 0; i < int(workerCount); i++ {
 		go c.worker(stopCh)
 	}
 	<-stopCh
 	c.socket.FlushMRoute()
 	syscall.Close(c.socket.GetFD())
 }
+
+func (c *MRouteClient) updateMulticastRouteStatsEntry(entry *multicastRouteEntry) (isStale bool, newEntry *multicastRouteEntry) {


I feel we can modify the parameter entry to use the value instead of a pointer, since the struct is small and the function logic only reads the values but not modifies it. Then you don't need to get the pointer inside a for loop in the caller?

Check the packet count difference every minute for each multicast route and remove the ones that have identical packet count in the past 10 minutes. Signed-off-by: ceclinux <[email protected]>

ceclinux · 2024-04-10T05:09:39Z

One comment, others LGTM.

BTW, don't forget to rebase your patch on main branch, and resolve candidate conflicts.

rebased

ceclinux · 2024-04-10T17:31:34Z

/test-multicast-e2e

ceclinux · 2024-04-10T17:31:37Z

/test-multicast-e2e

ceclinux · 2024-04-10T17:31:40Z

/test-multicast-e2e

ceclinux · 2024-04-10T17:31:55Z

/test-flexible-ipam-e2e

ceclinux · 2024-04-10T17:31:57Z

/test-flexible-ipam-e2e

ceclinux · 2024-04-10T17:32:00Z

/test-flexible-ipam-e2e

wenyingd

LGTM

tnqn

LGTM, thanks

tnqn · 2024-05-10T07:08:46Z

/test-all

tnqn · 2024-05-10T11:03:15Z

/skip-conformance the test succeeded

ceclinux force-pushed the test_get_oubound_mroute_stats branch from dc5d9aa to f5d8334 Compare February 11, 2022 01:51

ceclinux force-pushed the test_get_oubound_mroute_stats branch from f5d8334 to 59cc317 Compare February 21, 2022 17:12

ceclinux force-pushed the test_get_oubound_mroute_stats branch 2 times, most recently from 7fc2619 to 5003af1 Compare March 4, 2022 01:46

ceclinux changed the title ~~[WIP Multicast] removal of staled outbound multicast routes~~ [Multicast] removal of staled outbound multicast routes Mar 4, 2022

ceclinux changed the title ~~[Multicast] removal of staled outbound multicast routes~~ [WIP Multicast] removal of staled outbound multicast routes Mar 4, 2022

ceclinux force-pushed the test_get_oubound_mroute_stats branch 4 times, most recently from 8518edd to 2d73d5d Compare March 4, 2022 09:11

ceclinux changed the title ~~[WIP Multicast] removal of staled outbound multicast routes~~ [Multicast] removal of staled outbound multicast routes Mar 4, 2022

ceclinux requested review from tnqn and wenyingd March 4, 2022 09:13

ceclinux force-pushed the test_get_oubound_mroute_stats branch 3 times, most recently from e249a80 to 1dbefe2 Compare March 16, 2022 14:44

ceclinux changed the title ~~[Multicast] removal of staled outbound multicast routes~~ [Multicast] removal of staled multicast routes Mar 16, 2022

ceclinux force-pushed the test_get_oubound_mroute_stats branch 2 times, most recently from 2626238 to b766d00 Compare March 16, 2022 14:59

ceclinux requested a review from liu4480 March 17, 2022 02:35

ceclinux force-pushed the test_get_oubound_mroute_stats branch 3 times, most recently from c3785c7 to 65ce623 Compare April 25, 2022 17:19

ceclinux force-pushed the test_get_oubound_mroute_stats branch from 30b1c69 to 3620719 Compare March 28, 2024 09:43

wenyingd reviewed Apr 1, 2024

View reviewed changes

ceclinux force-pushed the test_get_oubound_mroute_stats branch 2 times, most recently from e41efa8 to 0be1bdd Compare April 2, 2024 00:04

ceclinux requested a review from wenyingd April 2, 2024 00:12

wenyingd reviewed Apr 8, 2024

View reviewed changes

ceclinux force-pushed the test_get_oubound_mroute_stats branch from 0be1bdd to 872c8ab Compare April 10, 2024 03:39

[Multicast] Support removal of stale multicast routes

1b29753

Check the packet count difference every minute for each multicast route and remove the ones that have identical packet count in the past 10 minutes. Signed-off-by: ceclinux <[email protected]>

ceclinux force-pushed the test_get_oubound_mroute_stats branch from 872c8ab to 1b29753 Compare April 10, 2024 03:51

ceclinux requested a review from wenyingd April 16, 2024 08:29

wenyingd approved these changes Apr 16, 2024

View reviewed changes

tnqn approved these changes May 10, 2024

View reviewed changes

tnqn merged commit 4646348 into antrea-io:main May 10, 2024
52 of 55 checks passed

ceclinux deleted the test_get_oubound_mroute_stats branch May 14, 2024 06:06

ceclinux restored the test_get_oubound_mroute_stats branch May 14, 2024 06:06

	isDeleted, newEntry := c.updateMulticastRouteStatsEntry(&entry.multicastRouteEntry)
	isStale, newEntry := c.updateMulticastRouteStatsEntry(&entry.multicastRouteEntry)

		// inboundMulticastRouteEntry encodes the inbound multicast routing entry.
		// It has extra field Iif to represent inbound interface VIF.

	// It has extra field Iif to represent inbound interface VIF.
	// It has extra field vif to represent inbound interface VIF.

	// type inboundMulticastRouteEntry struct {
	// type outboundMulticastRouteEntry struct {

[Multicast] removal of stale multicast routes #3242

[Multicast] removal of stale multicast routes #3242

Conversation

ceclinux commented Jan 25, 2022 • edited Loading

codecov-commenter commented Jan 25, 2022 • edited by codecov bot Loading

Codecov Report

ceclinux commented Feb 9, 2022

ceclinux commented Feb 9, 2022

ceclinux commented Feb 10, 2022

ceclinux commented Feb 11, 2022

ceclinux commented Feb 11, 2022

ceclinux commented Mar 17, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceclinux commented Apr 2, 2024

ceclinux commented Apr 2, 2024

ceclinux commented Apr 2, 2024

ceclinux commented Apr 2, 2024

ceclinux commented Apr 2, 2024

ceclinux commented Apr 2, 2024

wenyingd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceclinux commented Apr 10, 2024

ceclinux commented Apr 10, 2024

ceclinux commented Apr 10, 2024

ceclinux commented Apr 10, 2024

ceclinux commented Apr 10, 2024

ceclinux commented Apr 10, 2024

ceclinux commented Apr 10, 2024

wenyingd left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented May 10, 2024

tnqn commented May 10, 2024

ceclinux commented Jan 25, 2022 •

edited

Loading

codecov-commenter commented Jan 25, 2022 •

edited by codecov bot

Loading