Multicast route configuration and support for join/leave transport node #2835

ceclinux · 2021-09-24T02:48:09Z

This pr creates multicast route client with multicast socket, which handles IGMPMSG_NOCACHE messages:

If the message has VIF equals to gateway interface's VIF, which means it is outbound multicast traffic from pods to external interfaces. The client configures the multicast route from Antrea gateway to external interfaces.
If the message has VIF different from gateway interface's VIF, which means it is inbound multicast traffic. The client configures the multicast route from the external interface to the Antrea gateway.

Also the multicast client delete the multicast routes and configure the interfaces to join/leave multicast groups for the following circumstances:

If all the multicast receivers inside the pods leaves the multicast group A, it deletes inbound multicast route from external interfaces to gateway for the group A and ensures all the external interfaces leave the multicast group A.
If at least one receiver inside the pod joins the multicast group A, it configures all the external interfaces join the multicast group A.

codecov-commenter · 2021-10-13T06:40:51Z

Codecov Report

Merging #2835 (32d3bef) into main (d9fb47f) will increase coverage by 0.93%.
The diff coverage is 26.86%.

@@            Coverage Diff             @@
##             main    #2835      +/-   ##
==========================================
+ Coverage   59.45%   60.39%   +0.93%     
==========================================
  Files         298      302       +4     
  Lines       25589    36115   +10526     
==========================================
+ Hits        15215    21812    +6597     
- Misses       8738    12482    +3744     
- Partials     1636     1821     +185

Flag	Coverage Δ
e2e-tests	`49.43% <1.86%> (?)`
kind-e2e-tests	`46.29% <1.44%> (+1.36%)`	⬆️
unit-tests	`40.59% <37.16%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/multicast/mcast_socket_linux.go	`0.00% <0.00%> (ø)`
pkg/agent/util/iptables/iptables.go	`42.48% <ø> (+10.57%)`	⬆️
pkg/agent/util/syscall/syscall_unix.go	`0.00% <0.00%> (ø)`
pkg/ovs/ovsconfig/ovs_client.go	`49.92% <0.00%> (+3.78%)`	⬆️
pkg/agent/multicast/mcast_controller.go	`47.05% <29.26%> (-24.19%)`	⬇️
pkg/agent/route/route_linux.go	`43.40% <30.76%> (-2.99%)`	⬇️
pkg/agent/multicast/mcast_route_linux.go	`37.50% <37.50%> (ø)`
pkg/agent/multicast/mcast_route.go	`45.77% <45.77%> (ø)`
pkg/agent/agent.go	`56.61% <100.00%> (+5.71%)`	⬆️
pkg/agent/cniserver/pod_configuration_linux.go	`26.31% <0.00%> (-40.36%)`	⬇️
... and 291 more

cmd/antrea-agent/agent.go

pkg/agent/controller/multicast/kernel_source.go

pkg/agent/controller/multicast/mcast_controller.go

pkg/agent/controller/multicast/multicast_route.go

pkg/agent/controller/multicast/vif_allocator.go

test/e2e/multicast_test.go

lgtm-com · 2021-11-17T04:03:27Z

This pull request introduces 2 alerts when merging eb792e4 into db0c92d - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

pkg/agent/agent.go

pkg/agent/multicast/multicast_route.go

pkg/agent/multicast/source_utils.go

jianjuns · 2021-11-19T00:21:52Z

pkg/agent/multicast/vif_allocator.go

+	"k8s.io/klog/v2"
+)
+
+type vifAllocator struct {


Add comments.

Is VIF a term in the Linux IGMP or mroute implementation? Probably you should add comments to explain what is a VIF too.

And probably you should use VIF rather than Vif in names.

comment added.

VIF stands for virtual interface in the context of multicast https://github.com/torvalds/linux/blob/d58071a8a76d779eedab38033ae4c821c30295a5/include/uapi/linux/mroute.h#L21 . The interface should be added as VIF first before configuring multicast routing related to this interface

pkg/agent/multicast/vif_allocator.go

lgtm-com · 2021-11-30T02:26:53Z

This pull request introduces 2 alerts when merging 6987f32 into c135609 - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

lgtm-com · 2021-11-30T09:54:55Z

This pull request introduces 2 alerts when merging 827df45 into c135609 - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

jianjuns · 2021-11-30T19:54:07Z

pkg/agent/multicast/multicast_route.go

+		_, ok := interfacesMap[iface.Name]
+		// the interface should be multicast enabled
+		// and not in the ifaceStore
+		if ok || !strings.Contains(iface.Flags.String(), "multicast") {


Do we have a more efficient way to check this? At least define a constant for "multicast".

After offline discussion with @wenyingd , we agreed allowing users manually set external multicast interfaces in the antrea config file would be a better and simpler solution. If set in the antrea config file, this method could be removed and no need to detect and handle the multicast interfaces change. what's your opinion? Thanks

That sounds good to me. But considering in many cases the Node just has one interface, do we have a way to look up and use that interface by default?

I think you the one interface you mentioned is transportInterface. I have added externalMulticastInterface in the antrea agent config file. Basically the result of externalMulticastInterface union TransportInterface will be the multicast interfaces we will use to route multicast traffic.

No, TransportInterface is also optional. When the Node has only one interface, TransportInterface parameter is useless, and so will not be set either.

BTW, when externalMulticastInterface is set, we should just use it; only when it is not set we consider TransportInterface next.

Updated.

Basically, we need one interface to route multicast traffic to remote Nodes base on the order of transportInterface, transportInterfaceCIDRs and The Node IP, which is NodeTransportName calculated in NodeConfig. Also user can specify externalMulticastInterfaces to route multicast traffic to the outside world. the result of externalMulticastInterface union [NodeTransportName] will be all the multicast interfaces to route multicast traffic.

Please correct me If I understand it incorrectly, thanks.

pkg/agent/multicast/multicast_route.go

lgtm-com · 2021-12-02T02:16:51Z

This pull request introduces 2 alerts when merging b88d040 into 9a52de9 - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

tnqn · 2022-01-12T03:39:42Z

third_party/unix/ztypes_linux.go

+	Wrong_if uint32
+	Expire   int32
+}
+type Vifctl struct {


Insert a line in front of it

tnqn · 2022-01-12T03:40:51Z

third_party/unix/ztypes_linux.go

+// go run mksyscall.go -l32 -arm -tags linux,arm syscall_linux.go syscall_linux_arm.go
+// Code generated by the command above; see README.md. DO NOT EDIT.
+Modifies:
+- All the consts and types defined below are addition to the original code.


Use empty lines to separate the command and modfifies, otherwise it's hard to read.

tnqn · 2022-01-12T03:41:12Z

third_party/unix/syscall_unix.go

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+Modifies:


tnqn · 2022-01-12T03:45:57Z

third_party/unix/zerrors_linux.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+Modifies:
+- All the consts defined below are copied from the original code.


The geneted files has both "DO NOT EDIT" and "Modifies", have they been updated manually?
Asking for the maintain of such code, could you make README more complete to list what files are written by us and what files are gererated, and the whole steps to generate the code. I think Jianjun has this concern too: #2835 (comment)

README updated. Modifies are added by myself. compile command and DO NOT EDIT are copied from the original file, for instance, https://github.com/golang/sys/blob/5a964db013201115fcba5c3d31ade965d0969335/unix/ztypes_linux_amd64.go#L2

So we do update them? The "DO NOT EDIT" warning will confuse people. Since you already remove many code, why don't remove the confusing comment which is false here.
Better to have a link of the original code.
You didn't link mention your update in this file in README, could you add it and anything missing if there is?

zerrors_linux.go has already removed

ceclinux · 2022-01-12T15:59:55Z

/test-all
/skip-windows-all
/skip-ipv6-all
/skip-ipv6-only-all

jianjuns · 2022-01-12T22:50:10Z

third_party/unix/syscall_unix.go

+	return e
+}
+
+func SetsockoptViMfcctl(fd, level, opt int, mfcctl *Mfcctl) error {


What does "Vi" stand for? Should we just name the func SetsockoptMfcctl()?

ceclinux · 2022-01-13T09:33:31Z

/test-all
/skip-windows-all
/skip-ipv6-all
/skip-ipv6-only-all
/test-e2e
/test-conformance

tnqn · 2022-01-13T12:30:21Z

third_party/unix/README

@@ -0,0 +1,11 @@
+The code from Package unix is generated by running GOOS=linux GOARCH=amd64 ./mkall.sh from [golang/sys/internal-branch.go1.17-vendor](https://github.com/golang/sys/tree/internal-branch.go1.17-vendor)


Please name the file README.md so that it will be rendered as markdown by github.

Please wrap the lines like other documents.

README.md updated.

tnqn · 2022-01-13T12:33:07Z

third_party/unix/linux/types.go

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+Modifies:


Moved third_party/unix to pkg/util/syscall. Modifies: sections are removed.

tnqn · 2022-01-13T12:42:56Z

third_party/unix/zerrors_linux.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+Modifies:
+- All the consts defined below are copied from the original code.


So we do update them? The "DO NOT EDIT" warning will confuse people. Since you already remove many code, why don't remove the confusing comment which is false here.
Better to have a link of the original code.
You didn't link mention your update in this file in README, could you add it and anything missing if there is?

ceclinux · 2022-01-14T04:12:18Z

Moved third_party/unix to pkg/util/syscall as discussed for the following reasons:

Most code are actually hand-written.
Simpler
better Maintainability and easier for readers to comprehend
@tnqn

tnqn

LGTM except the place of syscall package

tnqn · 2022-01-14T17:40:05Z

pkg/util/syscall/linux/types.go

@@ -0,0 +1,53 @@
+//go:build ignore


Currently pkg/util contains utils that may be used for both controller and agent while pkg/agent/util contains utils specific to agent like iptables, route, arp tools. As this is about syscall, which it's unlikely to be required by controller, I would suggest to move it to the latter.

1. Add and delete static multicast route entries for inbound and outbound multicast traffic. 2. Configure OVS bridge to support multicast snooping and disable flooding of unregistered multicast packets to all ports. 3. Add an iptables rule to prevent multicast traffic masquerade. Signed-off-by: Ruochen Shen <[email protected]>

ceclinux · 2022-01-16T11:04:03Z

/test-e2e
/test-conformance
/test-networkpolicy

ceclinux · 2022-01-16T18:22:17Z

/test-e2e
/test-conformance
/test-networkpolicy

ceclinux · 2022-01-17T01:59:25Z

/test-networkpolicy

tnqn

LGTM

tnqn · 2022-01-17T04:13:22Z

/test-windows-e2e
/test-windows-conformance
/test-windows-networkpolicy

ceclinux · 2022-01-17T10:28:32Z

/test-windows-e2e
/test-windows-conformance
/test-windows-networkpolicy

ceclinux · 2022-01-18T01:03:06Z

Appreciate for detailed review @tnqn @wenyingd @jianjuns 👍

1. Add and delete static multicast route entries for inbound and outbound multicast traffic. 2. Configure OVS bridge to support multicast snooping and disable flooding of unregistered multicast packets to all ports. 3. Add an iptables rule to prevent multicast traffic masquerade. Signed-off-by: Ruochen Shen <[email protected]>

ceclinux force-pushed the mulitcast_route branch 3 times, most recently from c0167c6 to 6fc942f Compare October 24, 2021 09:34

wenyingd reviewed Oct 25, 2021

View reviewed changes

ceclinux force-pushed the mulitcast_route branch 3 times, most recently from 068e23e to b78862c Compare October 27, 2021 13:13

ceclinux force-pushed the mulitcast_route branch 2 times, most recently from 23ac04f to c808f85 Compare November 3, 2021 08:00

wenyingd reviewed Nov 3, 2021

View reviewed changes

test/e2e/multicast_test.go Outdated Show resolved Hide resolved

test/e2e/multicast_test.go Outdated Show resolved Hide resolved

test/e2e/multicast_test.go Outdated Show resolved Hide resolved

ceclinux force-pushed the mulitcast_route branch 4 times, most recently from 43ce322 to 9b207e8 Compare November 8, 2021 01:04

ceclinux force-pushed the mulitcast_route branch from 9b207e8 to eb792e4 Compare November 17, 2021 03:31

ceclinux changed the title ~~[WIP] Mulitcast route configuration and support for join/leave transport node~~ Mulitcast route configuration and support for join/leave transport node Nov 17, 2021

ceclinux requested a review from jianjuns November 17, 2021 03:36

jianjuns reviewed Nov 19, 2021

View reviewed changes

ceclinux force-pushed the mulitcast_route branch from eb792e4 to 6987f32 Compare November 30, 2021 01:29

jianjuns reviewed Nov 30, 2021

View reviewed changes

pkg/agent/multicast/vif_allocator.go Outdated Show resolved Hide resolved

pkg/agent/multicast/vif_allocator.go Outdated Show resolved Hide resolved

pkg/agent/multicast/vif_allocator.go Outdated Show resolved Hide resolved

ceclinux force-pushed the mulitcast_route branch 2 times, most recently from 9b52d8c to 827df45 Compare November 30, 2021 09:19

jianjuns reviewed Nov 30, 2021

View reviewed changes

ceclinux force-pushed the mulitcast_route branch from 827df45 to b88d040 Compare December 2, 2021 01:50

ceclinux force-pushed the mulitcast_route branch from b88d040 to f3544fb Compare December 3, 2021 01:33

tnqn reviewed Jan 12, 2022

View reviewed changes

ceclinux force-pushed the mulitcast_route branch 2 times, most recently from 85f0737 to 83ebb08 Compare January 12, 2022 13:46

jianjuns reviewed Jan 12, 2022

View reviewed changes

ceclinux force-pushed the mulitcast_route branch from 83ebb08 to b298c0d Compare January 12, 2022 23:53

tnqn reviewed Jan 13, 2022

View reviewed changes

ceclinux force-pushed the mulitcast_route branch from b298c0d to 988f161 Compare January 14, 2022 04:04

ceclinux force-pushed the mulitcast_route branch 4 times, most recently from ac9081e to 9b942b8 Compare January 14, 2022 10:08

tnqn reviewed Jan 14, 2022

View reviewed changes

ceclinux force-pushed the mulitcast_route branch 2 times, most recently from df265a6 to 486f858 Compare January 16, 2022 10:08

ceclinux force-pushed the mulitcast_route branch from 486f858 to 32d3bef Compare January 16, 2022 10:12

tnqn approved these changes Jan 17, 2022

View reviewed changes

tnqn merged commit 4a9eee7 into antrea-io:main Jan 17, 2022

ceclinux mentioned this pull request Jan 18, 2022

[Multicast] Add multicast e2e tests #2986

Merged

wenyingd mentioned this pull request Jan 21, 2022

Multicast Support in Antrea #2251

Closed

12 tasks

		@@ -0,0 +1,11 @@
		The code from Package unix is generated by running GOOS=linux GOARCH=amd64 ./mkall.sh from [golang/sys/internal-branch.go1.17-vendor](https://github.com/golang/sys/tree/internal-branch.go1.17-vendor)

Multicast route configuration and support for join/leave transport node #2835

Multicast route configuration and support for join/leave transport node #2835

Conversation

ceclinux commented Sep 24, 2021 • edited Loading

codecov-commenter commented Oct 13, 2021 • edited Loading

Codecov Report

lgtm-com bot commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 30, 2021

lgtm-com bot commented Nov 30, 2021

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

lgtm-com bot commented Dec 2, 2021

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 Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceclinux commented Jan 13, 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

ceclinux commented Jan 14, 2022

tnqn 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 Jan 16, 2022

ceclinux commented Jan 16, 2022

ceclinux commented Jan 17, 2022

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jan 17, 2022

ceclinux commented Jan 17, 2022

ceclinux commented Jan 18, 2022

ceclinux commented Sep 24, 2021 •

edited

Loading

codecov-commenter commented Oct 13, 2021 •

edited

Loading