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] Support IGMPv3 leave action #3389

Merged
merged 1 commit into from Mar 18, 2022

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Mar 3, 2022

IGMPv3 doesn't have an explicit message to describe a member leaving a
multicast group. Instead, IGMPv3 report message is used, but the group
record has an empty "include" sources. So Antrea Agent checks the group
record type and source number with IGMPv3 report message, and picks out
the groups which the member expects to leave.

Fixes #3388

Signed-off-by: wenyingd wenyingd@vmware.com

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2022

Codecov Report

Merging #3389 (48c9d7c) into main (24f85ce) will decrease coverage by 8.30%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3389      +/-   ##
==========================================
- Coverage   62.11%   53.80%   -8.31%     
==========================================
  Files         269      375     +106     
  Lines       26852    51498   +24646     
==========================================
+ Hits        16680    27711   +11031     
- Misses       8334    21376   +13042     
- Partials     1838     2411     +573     
Flag Coverage Δ
e2e-tests 50.70% <0.00%> (?)
integration-tests 35.78% <ø> (?)
kind-e2e-tests 47.43% <0.00%> (-1.51%) ⬇️
unit-tests 42.64% <100.00%> (+0.29%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/multicast/mcast_discovery.go 52.97% <83.33%> (+27.97%) ⬆️
pkg/agent/cniserver/pod_configuration_linux.go 26.31% <0.00%> (-40.36%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 48.71% <0.00%> (-31.57%) ⬇️
pkg/agent/flowexporter/connections/conntrack.go 45.71% <0.00%> (-30.48%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 61.46% <0.00%> (-29.97%) ⬇️
.../agent/flowexporter/priorityqueue/priorityqueue.go 63.29% <0.00%> (-29.31%) ⬇️
pkg/controller/egress/controller.go 62.19% <0.00%> (-26.26%) ⬇️
.../flowexporter/connections/conntrack_connections.go 58.16% <0.00%> (-25.05%) ⬇️
pkg/controller/ipam/validate.go 57.95% <0.00%> (-24.31%) ⬇️
.../registry/networkpolicy/clustergroupmember/rest.go 64.28% <0.00%> (-23.95%) ⬇️
... and 338 more

klog.InfoS("Received IGMPv3 Report message", "group", mgroup.String(), "interface", iface.PodName)
klog.InfoS("Received IGMPv3 Report message", "group", mgroup.String(), "interface", iface.InterfaceName, "recordType", gr.Type, "sourceCount", gr.NumberOfSources)
evtType := groupJoin
if gr.Type == protocol.IGMPIsIn && gr.NumberOfSources == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be gr.Type == protocol.IGMPToIn

Copy link
Contributor Author

@wenyingd wenyingd Mar 4, 2022

Choose a reason for hiding this comment

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

Do you mean we don't check gr.NumberOfSources? I think it is necessary, from rfc 3376 (IGMPv3), recordType==IsIn and source list is empty are required at the same time. https://datatracker.ietf.org/doc/html/rfc3376

   Previous versions of IGMP did not support source filters and had a
   simpler service interface consisting of Join and Leave operations to
   enable and disable reception of a given multicast address (from *all*
   sources) on a given interface.  The equivalent operations in the new
   service interface follow:

   The Join operation is equivalent to

      IPMulticastListen ( socket, interface, multicast-address,
                          EXCLUDE, {} )

   and the Leave operation is equivalent to:

      IPMulticastListen ( socket, interface, multicast-address,
                          INCLUDE, {} )

   where {} is an empty source list.

Copy link
Contributor

@ceclinux ceclinux Mar 4, 2022

Choose a reason for hiding this comment

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

My bad. I forgot to add gr.NumberOfSources == 0. So the correct logic should be?

if gr.Type == protocol.IGMPToIn && gr.NumberOfSources == 0 {

Copy link
Member

Choose a reason for hiding this comment

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

Should it check if it's MODE_IS_INCLUDE or CHANGE_TO_INCLUDE_MODE? or we handle response only?
https://datatracker.ietf.org/doc/html/rfc3376#section-4.2.12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think your concern is correct, I would add support for CHANGE_TO_INCLUDE_MODE.

pkg/agent/multicast/mcast_discovery.go Outdated Show resolved Hide resolved
klog.InfoS("Received IGMPv3 Report message", "group", mgroup.String(), "interface", iface.PodName)
klog.InfoS("Received IGMPv3 Report message", "group", mgroup.String(), "interface", iface.InterfaceName, "recordType", gr.Type, "sourceCount", gr.NumberOfSources)
evtType := groupJoin
if gr.Type == protocol.IGMPIsIn && gr.NumberOfSources == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Should it check if it's MODE_IS_INCLUDE or CHANGE_TO_INCLUDE_MODE? or we handle response only?
https://datatracker.ietf.org/doc/html/rfc3376#section-4.2.12

pkg/agent/multicast/mcast_controller_test.go Outdated Show resolved Hide resolved
pkg/agent/multicast/mcast_controller_test.go Outdated Show resolved Hide resolved
pkg/agent/multicast/mcast_controller_test.go Outdated Show resolved Hide resolved
@tnqn
Copy link
Member

tnqn commented Mar 8, 2022

In commit message:
s/a explicit/an explicit/
s/describe a member leaves a multicast group/describe a member leaving a multicast group/
s/a empty/en empty/
s/the groups from which the member expects to leave/the groups which the member expects to leave/

IGMPv3 doesn't have an explicit message to describe a member leaving a
multicast group. Instead, IGMPv3 report message is used, but the group
record has an empty "include" sources. So Antrea Agent checks the group
record type and source number with IGMPv3 report message, and picks out
the groups which the member expects to leave.

Signed-off-by: wenyingd <wenyingd@vmware.com>
@wenyingd
Copy link
Contributor Author

wenyingd commented Mar 9, 2022

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

Windows and IPv6 are not relevant, skip those tests.

@tnqn
Copy link
Member

tnqn commented Mar 9, 2022

@wenyingd there is no update pushed

@tnqn tnqn added this to the Antrea v1.6 release milestone Mar 9, 2022
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 9, 2022
@wenyingd
Copy link
Contributor Author

@wenyingd there is no update pushed

My bad, seems network issue, changes are pushed

@wenyingd
Copy link
Contributor Author

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

Windows and IPv6 are not relevant, skip those tests.

@wenyingd
Copy link
Contributor Author

/test-multicast-e2e

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

@tnqn
Copy link
Member

tnqn commented Mar 10, 2022

@liu4480 @ceclinux any more comment?

@tnqn tnqn requested review from ceclinux and liu4480 March 11, 2022 02:47
@wenyingd
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor

LGTM

@ceclinux
Copy link
Contributor

/test-multicast-e2e

2 similar comments
@wenyingd
Copy link
Contributor Author

/test-multicast-e2e

@XinShuYang
Copy link
Contributor

/test-multicast-e2e

@wenyingd
Copy link
Contributor Author

The failed cases in multicast e2e is about TestAntreaPolicy, not the patch relevant. Multicast dedicated cases are passed

@tnqn tnqn merged commit f64356e into antrea-io:main Mar 18, 2022
@wenyingd wenyingd deleted the multicast_igmpv3_leave branch June 13, 2022 08:47
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.

IGMPv3 leave is not supported correctly
7 participants