multicast: make igmp query interval configurable by liu4480 · Pull Request #3819 · antrea-io/antrea

liu4480 · 2022-05-23T07:39:22Z

For multicast, IGMP query is sent by antrea-agent every 125 seconds. This patch makes IGMP query interval configurable.

codecov-commenter · 2022-05-23T08:02:13Z

Codecov Report

Merging #3819 (4f73825) into main (1886ecb) will decrease coverage by 7.98%.
The diff coverage is 91.30%.

@@            Coverage Diff             @@
##             main    #3819      +/-   ##
==========================================
- Coverage   64.38%   56.39%   -7.99%     
==========================================
  Files         284      398     +114     
  Lines       40248    55938   +15690     
==========================================
+ Hits        25912    31547    +5635     
- Misses      12304    21923    +9619     
- Partials     2032     2468     +436

Flag	Coverage Δ
integration-tests	`38.20% <ø> (?)`
kind-e2e-tests	`51.29% <0.00%> (-1.50%)`	⬇️
unit-tests	`43.92% <91.30%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/multicast/mcast_controller.go	`61.63% <89.47%> (+0.25%)`	⬆️
pkg/agent/multicast/mcast_discovery.go	`61.90% <100.00%> (ø)`
pkg/controller/egress/store/egressgroup.go	`1.72% <0.00%> (-54.32%)`	⬇️
.../agent/flowexporter/connections/conntrack_linux.go	`58.06% <0.00%> (-27.42%)`	⬇️
pkg/agent/util/net.go	`29.33% <0.00%> (-22.23%)`	⬇️
pkg/agent/route/route_linux.go	`30.17% <0.00%> (-19.63%)`	⬇️
pkg/agent/util/ipset/ipset.go	`59.45% <0.00%> (-8.11%)`	⬇️
pkg/agent/openflow/service.go	`78.16% <0.00%> (-5.75%)`	⬇️
pkg/util/k8s/client.go	`35.71% <0.00%> (-5.36%)`	⬇️
pkg/controller/networkpolicy/tier.go	`50.00% <0.00%> (-5.00%)`	⬇️
... and 127 more

liu4480 · 2022-05-23T08:43:06Z

verified in testbed

tnqn

It doesn't make sense to add a user-facing configuration parameter just to make test quicker.
But I think igmp query interval itself is good to be configurable since this is configurable in physical routers. I would not say this is for testing only in PR description.
And the title can be simplified: "multicast: make igmp query interval configurable"

tnqn · 2022-05-23T12:02:15Z

 	MulticastInterfaces []string `yaml:"multicastInterfaces,omitempty"`
+	// The interval for antrea-agent to send IGMP query to pods, time uint is second.
+	// Defaults to 125 seconds.
+	IGMPQueryInterval int `yaml:"igmpQueryInterval"`


@wenyingd @liu4480 do we likely have other similar configurations in the future? I wonder if we should group them in one struct, including multicastInterfaces.
cc @antoninbas

I can't say no other configurations are needed in future. Agree to use one struct to group them if possible.

OK, I will add a group for multicast

grouping them sounds good to me.

wenyingd

LGTM

tnqn · 2022-05-24T11:41:13Z

        --help, -h                    Print this message and exit
        --multicast                   Generates a manifest for multicast.
        --multicast-interfaces        Multicast interface names (default is empty)
+        --igmpquery-interval          Interval to send IGMP query, time unit is second (default is 125)


This seems too specific. I think you can use --extra-helm-values-file or add a --extra-helm-values argument.

removed this option, and use --extra-helm-values-file

jianjuns · 2022-05-24T17:25:52Z

 | kubeAPIServerOverride | string | `""` | Address of Kubernetes apiserver, to override any value provided in kubeconfig or InClusterConfig. |
 | logVerbosity | int | `0` |  |
-| multicastInterfaces | list | `[]` | Names of the interfaces on Nodes that are used to forward multicast traffic. |
+| multicast.igmpQueryInterval | int | `125` | The interval for antrea-agent to send IGMP query to pods, time uint is second. Defaults to 125 seconds. |


pods -> Pods

query -> queries

jianjuns · 2022-05-24T17:26:44Z

+  {{- toYaml . | nindent 4 }}
+  {{- end }}
+
+# The interval for antrea-agent to send IGMP query to Pods, time unit is second.


jianjuns · 2022-05-24T17:27:30Z

+	// The names of the interfaces on Nodes that are used to forward multicast traffic.
+	// Defaults to transport interface if not set.
+	MulticastInterfaces []string `yaml:"multicastInterfaces,omitempty"`
+	// The interval for antrea-agent to send IGMP query to pods, time uint is second.


queries

Pods

antoninbas · 2022-05-24T17:34:44Z

+multicast:
+  # -- Names of the interfaces on Nodes that are used to forward multicast traffic.
+  multicastInterfaces: []
+  # -- The interval for antrea-agent to send IGMP query to pods, time uint is second.


Suggested change

# -- The interval for antrea-agent to send IGMP query to pods, time uint is second.

# -- The interval in seconds at which the antrea-agent send IGMP queries to Pods.

antoninbas · 2022-05-24T17:35:23Z

+  # -- Names of the interfaces on Nodes that are used to forward multicast traffic.
+  multicastInterfaces: []
+  # -- The interval for antrea-agent to send IGMP query to pods, time uint is second.
+  # Defaults to 125 seconds.


you can remove this IMO, the default is already used to configure the value below

antoninbas · 2022-05-24T17:36:47Z

+  multicastInterfaces: []
+  # -- The interval for antrea-agent to send IGMP query to pods, time uint is second.
+  # Defaults to 125 seconds.
+  igmpQueryInterval: 125


@tnqn do you think that for time durations we should consistently use go duration strings (e.g. "125s", "2m", "1m30s", ...)?
I think that K8s components do that and we also do that for other Antrea configuration parameters (for the Flow Aggregator IIRC).

tnqn · 2022-05-25T13:30:53Z

+func (o *Options) validateMulticastConfig() error {
+	if features.DefaultFeatureGate.Enabled(features.Multicast) {
+		var err error
+		o.igmpQueryInterval, err = time.ParseDuration(o.config.Multicast.IGMPQueryInterval)


I think it should only parse it when it's not empty? otherwise empty value would crash the process?

thanks, done

antoninbas

small comment, otherwise LGTM

antoninbas · 2022-05-25T17:54:38Z

+  {{- toYaml . | nindent 4 }}
+  {{- end }}
+
+# The interval at which the antrea-agent to send IGMP queries to Pods.


s/to send/sends

in all the other places as well

antoninbas · 2022-05-25T17:56:00Z

 	staleConnectionTimeout time.Duration
-	nplStartPort           int
-	nplEndPort             int
+	// IGMP query interval


not a useful comment IMO, it just repeats the variable name; you could remove that line altogether

Signed-off-by: Bin Liu <biliu@vmware.com>

jianjuns · 2022-05-25T23:07:19Z

/test-all

tnqn

LGTM

tnqn · 2022-05-26T03:51:15Z

/skip-conformance no change related to conformance test

Signed-off-by: Bin Liu <biliu@vmware.com> Signed-off-by: Scott Rosenberg <scott@terasky.com>

)" This reverts commit 6d3036b. Signed-off-by: Shuyang Xin <gavinx@vmware.com>

liu4480 requested review from ceclinux, tnqn and wenyingd May 23, 2022 07:39

wenyingd reviewed May 23, 2022

View reviewed changes

Comment thread pkg/agent/multicast/mcast_discovery.go Outdated

liu4480 force-pushed the configure-IGMP-query-interval branch from fe885c8 to 489cae5 Compare May 23, 2022 08:24

liu4480 requested a review from wenyingd May 23, 2022 08:29

liu4480 force-pushed the configure-IGMP-query-interval branch from 489cae5 to 7884919 Compare May 23, 2022 09:18

tnqn reviewed May 23, 2022

View reviewed changes

Comment thread build/charts/antrea/README.md Outdated

Comment thread pkg/agent/multicast/mcast_discovery.go

Comment thread pkg/agent/multicast/mcast_discovery.go Outdated

liu4480 changed the title ~~multicast: set igmp query interval by adding igmpQueryInterval in agent.conf~~ multicast: make igmp query interval configurable May 23, 2022

liu4480 force-pushed the configure-IGMP-query-interval branch from 7884919 to ba33470 Compare May 23, 2022 10:41

wenyingd reviewed May 23, 2022

View reviewed changes

Comment thread pkg/agent/multicast/mcast_controller_test.go Outdated

liu4480 force-pushed the configure-IGMP-query-interval branch from ba33470 to 0fc2e49 Compare May 23, 2022 11:52

tnqn reviewed May 23, 2022

View reviewed changes

liu4480 force-pushed the configure-IGMP-query-interval branch from 0fc2e49 to 96d569c Compare May 24, 2022 09:32

liu4480 requested review from antoninbas, tnqn and wenyingd May 24, 2022 09:33

liu4480 added this to the Antrea v1.7 release milestone May 24, 2022

liu4480 force-pushed the configure-IGMP-query-interval branch 2 times, most recently from 99514c2 to ec768e5 Compare May 24, 2022 10:44

wenyingd previously approved these changes May 24, 2022

View reviewed changes

tnqn reviewed May 24, 2022

View reviewed changes

liu4480 dismissed wenyingd’s stale review via 1bff55b May 24, 2022 11:54

liu4480 force-pushed the configure-IGMP-query-interval branch from ec768e5 to 1bff55b Compare May 24, 2022 11:54

jianjuns reviewed May 24, 2022

View reviewed changes

antoninbas reviewed May 24, 2022

View reviewed changes

liu4480 force-pushed the configure-IGMP-query-interval branch from 1bff55b to 1ad8513 Compare May 25, 2022 01:00

liu4480 requested a review from antoninbas May 25, 2022 01:00

liu4480 requested review from jianjuns, tnqn and wenyingd May 25, 2022 01:00

jianjuns previously approved these changes May 25, 2022

View reviewed changes

liu4480 dismissed jianjuns’s stale review via 949a135 May 25, 2022 02:39

liu4480 force-pushed the configure-IGMP-query-interval branch from 1ad8513 to 949a135 Compare May 25, 2022 02:39

tnqn reviewed May 25, 2022

View reviewed changes

liu4480 force-pushed the configure-IGMP-query-interval branch from 949a135 to b58f396 Compare May 25, 2022 14:14

antoninbas reviewed May 25, 2022

View reviewed changes

multicast: make igmp query interval configurable

4f73825

Signed-off-by: Bin Liu <biliu@vmware.com>

liu4480 force-pushed the configure-IGMP-query-interval branch from b58f396 to 4f73825 Compare May 25, 2022 23:02

liu4480 requested review from antoninbas, jianjuns and tnqn May 26, 2022 00:58

jianjuns approved these changes May 26, 2022

View reviewed changes

tnqn approved these changes May 26, 2022

View reviewed changes

tnqn merged commit 6d3036b into antrea-io:main May 26, 2022

liu4480 deleted the configure-IGMP-query-interval branch May 26, 2022 11:19

vrabbi pushed a commit to vrabbi/antrea that referenced this pull request May 26, 2022

multicast: make igmp query interval configurable (antrea-io#3819)

5bc625b

Signed-off-by: Bin Liu <biliu@vmware.com> Signed-off-by: Scott Rosenberg <scott@terasky.com>

XinShuYang added a commit to XinShuYang/antrea that referenced this pull request Jun 9, 2022

Revert "multicast: make igmp query interval configurable (antrea-io#3819

b89f13c

)" This reverts commit 6d3036b. Signed-off-by: Shuyang Xin <gavinx@vmware.com>

	# -- The interval for antrea-agent to send IGMP query to pods, time uint is second.
	# -- The interval in seconds at which the antrea-agent send IGMP queries to Pods.

Conversation

liu4480 commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liu4480 commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnqn May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoninbas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianjuns commented May 25, 2022

Uh oh!

tnqn left a comment

Choose a reason for hiding this comment

Uh oh!

liu4480 commented May 23, 2022 •

edited

Loading

codecov-commenter commented May 23, 2022 •

edited

Loading

liu4480 commented May 23, 2022 •

edited

Loading

tnqn May 25, 2022 •

edited

Loading