Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Multicast] removal of stale multicast routes #3242

Merged
merged 1 commit into from
May 10, 2024

Conversation

ceclinux
Copy link
Contributor

@ceclinux ceclinux commented Jan 25, 2022

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).

Xnip2024-03-22_19-01-16

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

ceclinux commented Feb 9, 2022

/test-multicast-e2e

2 similar comments
@ceclinux
Copy link
Contributor Author

ceclinux commented Feb 9, 2022

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux ceclinux force-pushed the test_get_oubound_mroute_stats branch from dc5d9aa to f5d8334 Compare February 11, 2022 01:51
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

1 similar comment
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux ceclinux force-pushed the test_get_oubound_mroute_stats branch from f5d8334 to 59cc317 Compare February 21, 2022 17:12
@ceclinux 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 ceclinux changed the title [WIP Multicast] removal of staled outbound multicast routes [Multicast] removal of staled outbound multicast routes Mar 4, 2022
@ceclinux ceclinux changed the title [Multicast] removal of staled outbound multicast routes [WIP Multicast] removal of staled outbound multicast routes Mar 4, 2022
@ceclinux 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 ceclinux changed the title [WIP Multicast] removal of staled outbound multicast routes [Multicast] removal of staled outbound multicast routes Mar 4, 2022
@ceclinux ceclinux requested review from tnqn and wenyingd March 4, 2022 09:13
@ceclinux 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 ceclinux changed the title [Multicast] removal of staled outbound multicast routes [Multicast] removal of staled multicast routes Mar 16, 2022
@ceclinux 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 ceclinux requested a review from liu4480 March 17, 2022 02:35
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux 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 ceclinux force-pushed the test_get_oubound_mroute_stats branch from 30b1c69 to 3620719 Compare March 28, 2024 09:43
func (c *MRouteClient) updateInboundMrouteStats() {
for _, obj := range c.inboundRouteCache.List() {
entry := obj.(*inboundMulticastRouteEntry)
isDeleted, newEntry := c.updateMulticastRouteStatsEntry(&entry.multicastRouteEntry)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Grp [4]byte /* in_addr */
Pktcnt uint32
Bytecnt uint32
If uint32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// type inboundMulticastRouteEntry struct {
// type outboundMulticastRouteEntry struct {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@ceclinux 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 ceclinux requested a review from wenyingd April 2, 2024 00:12
@ceclinux
Copy link
Contributor Author

ceclinux commented Apr 2, 2024

/test-multicast-e2e

2 similar comments
@ceclinux
Copy link
Contributor Author

ceclinux commented Apr 2, 2024

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

ceclinux commented Apr 2, 2024

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

ceclinux commented Apr 2, 2024

/test-flexible-ipam-e2e

2 similar comments
@ceclinux
Copy link
Contributor Author

ceclinux commented Apr 2, 2024

/test-flexible-ipam-e2e

@ceclinux
Copy link
Contributor Author

ceclinux commented Apr 2, 2024

/test-flexible-ipam-e2e

Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, others LGTM.

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@ceclinux ceclinux force-pushed the test_get_oubound_mroute_stats branch from 0be1bdd to 872c8ab Compare April 10, 2024 03:39
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 ceclinux force-pushed the test_get_oubound_mroute_stats branch from 872c8ab to 1b29753 Compare April 10, 2024 03:51
@ceclinux
Copy link
Contributor Author

One comment, others LGTM.

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

rebased

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

2 similar comments
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-flexible-ipam-e2e

2 similar comments
@ceclinux
Copy link
Contributor Author

/test-flexible-ipam-e2e

@ceclinux
Copy link
Contributor Author

/test-flexible-ipam-e2e

@ceclinux ceclinux requested a review from wenyingd April 16, 2024 08:29
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@tnqn
Copy link
Member

tnqn commented May 10, 2024

/test-all

@tnqn
Copy link
Member

tnqn commented May 10, 2024

/skip-conformance the test succeeded

@tnqn tnqn merged commit 4646348 into antrea-io:main May 10, 2024
52 of 55 checks passed
@ceclinux ceclinux deleted the test_get_oubound_mroute_stats branch May 14, 2024 06:06
@ceclinux ceclinux restored the test_get_oubound_mroute_stats branch May 14, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants