[Multicast] Clone remoteMembers when creating an updated GroupMemberStatus #4903

ceclinux · 2023-04-26T04:29:16Z

This patch is to resolve a concurrency issue in Multicast feature with encap mode.

The root cause is the existing code doesn't create a new object for field "remoteMembers" before updating it into cache.

Fixed: #4904

codecov · 2023-04-26T04:50:18Z

Codecov Report

Merging #4903 (5354f38) into main (97b8e4a) will decrease coverage by 0.78%.
The diff coverage is 66.66%.

❗ Current head 5354f38 differs from pull request most recent head 044084a. Consider uploading reports for the commit 044084a to get more accurate results

@@            Coverage Diff             @@
##             main    #4903      +/-   ##
==========================================
- Coverage   72.13%   71.36%   -0.78%     
==========================================
  Files         409      409              
  Lines       61161    59956    -1205     
==========================================
- Hits        44121    42788    -1333     
- Misses      14127    14286     +159     
+ Partials     2913     2882      -31

Flag	Coverage Δ
e2e-tests	`?`
integration-tests	`33.79% <0.00%> (-0.08%)`	⬇️
kind-e2e-tests	`46.83% <0.00%> (-0.48%)`	⬇️
unit-tests	`62.75% <66.66%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pkg/agent/multicast/mcast_controller.go	`69.12% <66.66%> (-0.16%)`	⬇️

... and 56 files with indirect coverage changes

wenyingd · 2023-04-26T06:25:43Z

pkg/agent/multicast/mcast_controller.go

@@ -82,12 +82,14 @@ type GroupMemberStatus struct {
 	group net.IP
 	// localMembers is a map for the local Pod member and its last update time, key is the Pod's interface name,
 	// and value is its last update time.
-	localMembers map[string]time.Time
+	localMembers      map[string]time.Time


I don't think it is necessary to introduce Mutexes for localMembers and remoteMembers. The root cause of the issue is we didn't create a new object for remoteMembers before updating the cache. The change in line 137 is enough.

luolanzone · 2023-04-26T06:51:17Z

pkg/agent/multicast/mcast_controller.go

@@ -131,7 +131,7 @@ func (c *Controller) updateGroupMemberStatus(obj interface{}, e *mcastGroupEvent
 	newStatus := &GroupMemberStatus{
 		group:          status.group,
 		localMembers:   make(map[string]time.Time),
-		remoteMembers:  status.remoteMembers,
+		remoteMembers:  sets.NewString(status.remoteMembers.List()...),


@wenyingd could you provide a few background for this issue in the PR summary?

I would create an issue for it.

luolanzone · 2023-04-26T06:52:14Z

/test-multicast-e2e

luolanzone

LGTM

tnqn · 2023-04-26T09:21:18Z

pkg/agent/multicast/mcast_controller.go

@@ -131,7 +131,7 @@ func (c *Controller) updateGroupMemberStatus(obj interface{}, e *mcastGroupEvent
 	newStatus := &GroupMemberStatus{
 		group:          status.group,
 		localMembers:   make(map[string]time.Time),
-		remoteMembers:  status.remoteMembers,
+		remoteMembers:  sets.NewString(status.remoteMembers.List()...),


Suggested change

remoteMembers: sets.NewString(status.remoteMembers.List()...),

remoteMembers: status.remoteMembers.Union(),

List performs unnecessary sorting.

pkg/agent/multicast/mcast_controller.go

tnqn · 2023-04-26T09:33:47Z

pkg/agent/multicast/mcast_controller_test.go

+	wg.Add(4)
+	// Below func adds local group join events.
+	go func() {


Below code is too redundant, can we reduce redundancy?

And there seems no point to generate the events concurrently as they are eventually consumed by a single event handler. The race is not between the event generator and others, but the eventHandler and worker

tnqn · 2023-04-26T09:35:39Z

pkg/agent/multicast/mcast_controller_test.go

+	for i := 0; i < numEvents; i++ {
+		mockIfaceStore.EXPECT().GetInterfaceByName(fmt.Sprintf("local-interfaceName-%d", i)).Return(&interfacestore.InterfaceConfig{
+			Type:          interfacestore.ContainerInterface,
+			InterfaceName: fmt.Sprintf("local-interfaceName-%d", i),
+			OVSPortConfig: &interfacestore.OVSPortConfig{
+				OFPort: int32(i),
+			},
+		}, true).AnyTimes()
+	}


It's contradictory to assert a function is called "anytimes" many times.
If you want accurate assertion, it shouldn't use Anytimes.
If you want it to be called anytimes, no need to have a for loop here.

Use a valid interfaceStore to avoid such mock.

tnqn · 2023-04-26T09:42:11Z

pkg/agent/multicast/mcast_controller_test.go

+	for i := 0; i < 2; i++ {
+		go wait.Until(c.worker, time.Second, stopCh)
+	}
+	wg.Wait()


there is no guaratee that when this call finishes, eventHander and worker have processed the events. Could we use some determinstic input and decide when the goroutines finish their job by polling the result, which could be more determinstic?

Added a check on the size of eventCh and queue after all events have been sent.

tnqn · 2023-04-26T15:55:35Z

pkg/agent/multicast/mcast_controller.go

@@ -131,7 +131,7 @@ func (c *Controller) updateGroupMemberStatus(obj interface{}, e *mcastGroupEvent
 	newStatus := &GroupMemberStatus{
 		group:          status.group,
 		localMembers:   make(map[string]time.Time),
-		remoteMembers:  status.remoteMembers,
+		remoteMembers:  sets.NewString().Union(status.remoteMembers),


This writing initializes an extra set. Why don't just use status.remoteMembers.Union(nil) which is quite common in our code base?

tnqn · 2023-04-26T15:56:45Z

pkg/agent/multicast/mcast_controller_test.go

+	wg.Add(4)
+
+	eventFunc := func(eType eventType, isLocal bool) {
+		LeastSignificantByteArr := rand.Perm(numEvents)


Suggested change

LeastSignificantByteArr := rand.Perm(numEvents)

leastSignificantByteArr := rand.Perm(numEvents)

tnqn · 2023-04-26T16:14:01Z

pkg/agent/multicast/mcast_controller_test.go

+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	groupIP := net.ParseIP("224.3.4.5")
+	numEvents := 250


With this number, the test's logging could flood the output and even make the race stack being cut, making it hard to figure out what causes the failure. You can try removing the fix and runing the test alone. I tested a few times setting it to 10 could always trigger the race and only produced reasonable logs.

tnqn

LGTM

tnqn · 2023-04-27T03:10:09Z

/test-all

tnqn · 2023-04-27T03:10:37Z

/test-multicast-e2e

luolanzone · 2023-04-27T06:11:56Z

@tnqn can you change the commit title when you merge it? the commit title doesn't match the change. Thanks.

When updating GroupMemberStatus, a clone of remoteMembers should assign to the updated GroupMemberStatus. Simply setting it with original remoteMembers will cause race conditions. Fixes antrea-io#4904 Signed-off-by: ceclinux <src655@gmail.com>

ceclinux · 2023-04-27T08:16:32Z

@tnqn can you change the commit title when you merge it? the commit title doesn't match the change. Thanks.

I have updated the title and commit message.

luolanzone · 2023-04-27T08:29:47Z

@ceclinux I recalled that you said this issue is also found in v1.9? I suppose we should backport this to v1.9 and v1.10? @tnqn

tnqn · 2023-04-27T10:19:15Z

/skip-all

tnqn · 2023-04-27T10:21:01Z

@ceclinux I recalled that you said this issue is also found in v1.9? I suppose we should backport this to v1.9 and v1.10? @tnqn

The releases in maintenance are 1.10 and v1.11, so let's backport to these two releases.

…trea-io#4903) When updating GroupMemberStatus, a clone of remoteMembers should assign to the updated GroupMemberStatus. Simply setting it with original remoteMembers will cause race conditions. Fixes antrea-io#4904 Signed-off-by: ceclinux <src655@gmail.com>

luolanzone added the action/backport Indicates a PR that requires backports. label Apr 26, 2023

wenyingd reviewed Apr 26, 2023

View reviewed changes

wenyingd force-pushed the concurrency-control-for-members-in-GroupMemberStatus branch from 63d63d1 to 5354f38 Compare April 26, 2023 06:48

luolanzone reviewed Apr 26, 2023

View reviewed changes

luolanzone previously approved these changes Apr 26, 2023

View reviewed changes

tnqn reviewed Apr 26, 2023

View reviewed changes

wenyingd dismissed luolanzone’s stale review via 044084a April 26, 2023 11:36

wenyingd force-pushed the concurrency-control-for-members-in-GroupMemberStatus branch from 5354f38 to 044084a Compare April 26, 2023 11:36

tnqn reviewed Apr 26, 2023

View reviewed changes

ceclinux force-pushed the concurrency-control-for-members-in-GroupMemberStatus branch from 044084a to 60559d0 Compare April 27, 2023 02:10

tnqn previously approved these changes Apr 27, 2023

View reviewed changes

ceclinux changed the title ~~[Multicast] Add concurrency control in GroupMemberStatus~~ [Multicast] Clone new remoteMembers when creating GroupMemberStatus Apr 27, 2023

ceclinux changed the title ~~[Multicast] Clone new remoteMembers when creating GroupMemberStatus~~ [Multicast] Clone remoteMembers when creating GroupMemberStatus Apr 27, 2023

ceclinux changed the title ~~[Multicast] Clone remoteMembers when creating GroupMemberStatus~~ [Multicast] Clone remoteMembers when creating an updated GroupMemberStatus Apr 27, 2023

ceclinux dismissed tnqn’s stale review via 2ddfcb8 April 27, 2023 07:55

ceclinux force-pushed the concurrency-control-for-members-in-GroupMemberStatus branch from 60559d0 to 2ddfcb8 Compare April 27, 2023 07:55

ceclinux force-pushed the concurrency-control-for-members-in-GroupMemberStatus branch from 2ddfcb8 to 039f0c8 Compare April 27, 2023 08:15

tnqn approved these changes Apr 27, 2023

View reviewed changes

tnqn merged commit a1e96e4 into antrea-io:main Apr 27, 2023

luolanzone mentioned this pull request Apr 27, 2023

Automated cherry pick of #4903: Clone remoteMembers when updating a #4908

Merged

luolanzone mentioned this pull request Apr 27, 2023

Automated cherry pick of #4903: Clone remoteMembers when updating a #4909

Merged

wenyingd mentioned this pull request May 24, 2023

Release 1.12.0 #5014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Multicast] Clone remoteMembers when creating an updated GroupMemberStatus #4903

[Multicast] Clone remoteMembers when creating an updated GroupMemberStatus #4903

ceclinux commented Apr 26, 2023 •

edited by wenyingd

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

wenyingd Apr 26, 2023

luolanzone Apr 26, 2023

wenyingd Apr 26, 2023

luolanzone commented Apr 26, 2023

luolanzone left a comment

tnqn Apr 26, 2023

wenyingd Apr 26, 2023

tnqn Apr 26, 2023

wenyingd Apr 26, 2023

tnqn Apr 26, 2023

wenyingd Apr 26, 2023

tnqn Apr 26, 2023

wenyingd Apr 26, 2023

tnqn Apr 26, 2023

ceclinux Apr 27, 2023

tnqn Apr 26, 2023

ceclinux Apr 27, 2023

tnqn Apr 26, 2023

ceclinux Apr 27, 2023

tnqn left a comment

tnqn commented Apr 27, 2023

tnqn commented Apr 27, 2023

luolanzone commented Apr 27, 2023

ceclinux commented Apr 27, 2023

luolanzone commented Apr 27, 2023

tnqn commented Apr 27, 2023

tnqn commented Apr 27, 2023

	remoteMembers: sets.NewString(status.remoteMembers.List()...),
	remoteMembers: status.remoteMembers.Union(),

	LeastSignificantByteArr := rand.Perm(numEvents)
	leastSignificantByteArr := rand.Perm(numEvents)

[Multicast] Clone remoteMembers when creating an updated GroupMemberStatus #4903

[Multicast] Clone remoteMembers when creating an updated GroupMemberStatus #4903

Conversation

ceclinux commented Apr 26, 2023 • edited by wenyingd Loading

codecov bot commented Apr 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Apr 26, 2023

luolanzone left a 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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 27, 2023

tnqn commented Apr 27, 2023

luolanzone commented Apr 27, 2023

ceclinux commented Apr 27, 2023

luolanzone commented Apr 27, 2023

tnqn commented Apr 27, 2023

tnqn commented Apr 27, 2023

ceclinux commented Apr 26, 2023 •

edited by wenyingd

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading