Fix missing call of removing groupDb cache when deleting OVS group

The old group will be reused unexpectedly if groupDb cache managed
by OFBridge is not cleared for that group, which causes a new group
claims to have a different group type acquiring the old group type.

Fixes #4575

Signed-off-by: ceclinux <src655@gmail.com>

pkg/agent/openflow/client.go

@@ -644,7 +644,10 @@ func (c *client) UninstallServiceGroup(groupID binding.GroupIDType) error {
 	gCache, ok := c.featureService.groupCache.Load(groupID)
 	if ok {
 		if err := c.ofEntryOperations.DeleteOFEntries([]binding.OFEntry{gCache.(binding.Group)}); err != nil {
-			return fmt.Errorf("error when deleting Service Endpoints Group %d: %w", groupID, err)
+			return fmt.Errorf("error when deleting Openflow entries for Service Endpoints Group %d: %w", groupID, err)
+		}
+		if err := c.bridge.DeleteGroup(groupID); err != nil {
+			return fmt.Errorf("error when deleting OFSwitch groupDb Cache for Service Endpoints Group %d: %w", groupID, err)
 		}
 		c.featureService.groupCache.Delete(groupID)
 	}
@@ -1346,7 +1349,10 @@ func (c *client) UninstallMulticastGroup(groupID binding.GroupIDType) error {
 	gCache, ok := c.featureMulticast.groupCache.Load(groupID)
 	if ok {
 		if err := c.ofEntryOperations.DeleteOFEntries([]binding.OFEntry{gCache.(binding.Group)}); err != nil {
-			return fmt.Errorf("error when deleting Multicast receiver Group %d: %w", groupID, err)
+			return fmt.Errorf("error when deleting Openflow entries for Multicast receiver Group %d: %w", groupID, err)
+		}
+		if err := c.bridge.DeleteGroup(groupID); err != nil {
+			return fmt.Errorf("error when deleting OFSwitch groupDb Cache for Multicast receiver Group %d: %w", groupID, err)
 		}
 		c.featureMulticast.groupCache.Delete(groupID)
 	}

pkg/agent/openflow/client_test.go

@@ -909,6 +909,17 @@ func Test_client_InstallServiceGroup(t *testing.T) {
 				"bucket=bucket_id:0,weight:100,actions=set_field:0xfec00010001000000000000000000100->xxreg3,set_field:0x50/0xffff->reg4,resubmit:ServiceLB," +
 				"bucket=bucket_id:1,weight:100,actions=set_field:0xfec00010001000000000000000000101->xxreg3,set_field:0x50/0xffff->reg4,resubmit:ServiceLB",
 		},
+		{
+			name: "delete group failed for IPv4 Endpoints",
+			endpoints: []proxy.Endpoint{
+				proxy.NewBaseEndpointInfo("10.10.0.100", "", "", 80, false, true, false, false, nil),
+				proxy.NewBaseEndpointInfo("10.10.0.101", "", "", 80, false, true, false, false, nil),
+			},
+			expectedGroup: "group_id=100,type=select," +
+				"bucket=bucket_id:0,weight:100,actions=set_field:0xa0a0064->reg3,set_field:0x50/0xffff->reg4,resubmit:EndpointDNAT," +
+				"bucket=bucket_id:1,weight:100,actions=set_field:0xa0a0065->reg3,set_field:0x50/0xffff->reg4,resubmit:EndpointDNAT",
+			deleteOFEntriesError: fmt.Errorf("error when deleting Openflow entries for Service Endpoints Group 100"),
+		},
 	}
 
 	for _, tc := range testCases {
@@ -928,9 +939,15 @@ func Test_client_InstallServiceGroup(t *testing.T) {
 			group := getGroupFromCache(gCacheI.(binding.Group))
 			assert.Equal(t, tc.expectedGroup, group)
 
-			assert.NoError(t, fc.UninstallServiceGroup(groupID))
-			_, ok = fc.featureService.groupCache.Load(groupID)
-			require.False(t, ok)
+			if tc.deleteOFEntriesError == nil {
+				assert.NoError(t, fc.UninstallServiceGroup(groupID))
+				_, ok = fc.featureService.groupCache.Load(groupID)
+				require.False(t, ok)
+			} else {
+				assert.Error(t, fc.UninstallServiceGroup(groupID))
+				_, ok = fc.featureService.groupCache.Load(groupID)
+				require.True(t, ok)
+			}
 		})
 	}
 }
@@ -1992,10 +2009,11 @@ func Test_client_InstallMulticastGroup(t *testing.T) {
 	localReceivers := []uint32{50, 100}
 	remoteNodeReceivers := []net.IP{net.ParseIP("192.168.77.101"), net.ParseIP("192.168.77.102")}
 	testCases := []struct {
-		name                string
-		localReceivers      []uint32
-		remoteNodeReceivers []net.IP
-		expectedGroup       string
+		name                 string
+		localReceivers       []uint32
+		remoteNodeReceivers  []net.IP
+		expectedGroup        string
+		deleteOFEntriesError error
 	}{
 		{
 			name:           "Local Receivers",
@@ -2021,6 +2039,14 @@ func Test_client_InstallMulticastGroup(t *testing.T) {
 				"bucket=bucket_id:2,actions=set_field:0x100/0x100->reg0,set_field:0x1->reg1,set_field:192.168.77.101->tun_dst,resubmit:MulticastOutput," +
 				"bucket=bucket_id:3,actions=set_field:0x100/0x100->reg0,set_field:0x1->reg1,set_field:192.168.77.102->tun_dst,resubmit:MulticastOutput",
 		},
+		{
+			name:           "DeleteOFEntries Failed",
+			localReceivers: localReceivers,
+			expectedGroup: "group_id=101,type=all," +
+				"bucket=bucket_id:0,actions=set_field:0x100/0x100->reg0,set_field:0x32->reg1,resubmit:MulticastIngressRule," +
+				"bucket=bucket_id:1,actions=set_field:0x100/0x100->reg0,set_field:0x64->reg1,resubmit:MulticastIngressRule",
+			deleteOFEntriesError: fmt.Errorf("error when deleting Openflow entries for Multicast receiver Group 101"),
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -2032,17 +2058,23 @@ func Test_client_InstallMulticastGroup(t *testing.T) {
 			defer resetPipelines()
 
 			m.EXPECT().AddOFEntries(gomock.Any()).Return(nil).Times(1)
-			m.EXPECT().DeleteOFEntries(gomock.Any()).Return(nil).Times(1)
+			m.EXPECT().DeleteOFEntries(gomock.Any()).Return(tc.deleteOFEntriesError).Times(1)
 
 			assert.NoError(t, fc.InstallMulticastGroup(groupID, tc.localReceivers, tc.remoteNodeReceivers))
 			gCacheI, ok := fc.featureMulticast.groupCache.Load(groupID)
 			require.True(t, ok)
 			group := getGroupFromCache(gCacheI.(binding.Group))
 			assert.Equal(t, tc.expectedGroup, group)
 
-			assert.NoError(t, fc.UninstallMulticastGroup(groupID))
-			_, ok = fc.featureMulticast.groupCache.Load(groupID)
-			require.False(t, ok)
+			if tc.deleteOFEntriesError == nil {
+				assert.NoError(t, fc.UninstallMulticastGroup(groupID))
+				_, ok = fc.featureMulticast.groupCache.Load(groupID)
+				require.False(t, ok)
+			} else {
+				assert.Error(t, fc.UninstallMulticastGroup(groupID))
+				_, ok = fc.featureMulticast.groupCache.Load(groupID)
+				require.True(t, ok)
+			}
 		})
 	}
 }

pkg/ovs/openflow/ofctrl_bridge.go

@@ -224,15 +224,14 @@ func (b *OFBridge) createGroupWithType(id GroupIDType, groupType ofctrl.GroupTyp
 	return g
 }
 
+// DeleteGroup deletes a specified group in groupDb. Note that to record OVS operations in Prometheus,
+// openFlowClient.DeleteOFEntries([]binding.OFEntry{groupCache})should be called in advance,
+// and deleteGroupOnOVS is unnecessary.
 func (b *OFBridge) DeleteGroup(id GroupIDType) error {
 	ofctrlGroup := b.ofSwitch.GetGroup(uint32(id))
 	if ofctrlGroup == nil {
 		return nil
 	}
-	g := &ofGroup{bridge: b, ofctrl: ofctrlGroup}
-	if err := g.Delete(); err != nil {
-		return fmt.Errorf("failed to delete the group: %w", err)
-	}
 	return b.ofSwitch.DeleteGroup(uint32(id))
 }
 

pkg/ovs/openflow/ofctrl_bridge_test.go

@@ -22,6 +22,7 @@ import (
 
 	"antrea.io/libOpenflow/util"
 	"antrea.io/ofnet/ofctrl"
+	"github.com/stretchr/testify/assert"
 
 	"antrea.io/antrea/pkg/ovs/ovsconfig"
 )
@@ -85,3 +86,34 @@ func TestOFBridgeIsConnected(t *testing.T) {
 	}()
 	wg.Wait()
 }
+
+func TestDeleteGroup(t *testing.T) {
+	b := NewOFBridge("test-br", GetMgmtAddress(ovsconfig.DefaultOVSRunDir, "test-br"))
+
+	for _, m := range []struct {
+		name            string
+		existingGroupID GroupIDType
+		deleteGroupID   GroupIDType
+		err             error
+	}{
+		{
+			name:            "delete existing group without flow",
+			existingGroupID: 20,
+			deleteGroupID:   20,
+			err:             nil,
+		},
+		{
+			name:            "delete non-existing group",
+			existingGroupID: 20,
+			deleteGroupID:   30,
+			err:             nil,
+		},
+	} {
+		t.Run(m.name, func(t *testing.T) {
+			b.ofSwitch = newFakeOFSwitch(b)
+			b.CreateGroup(m.existingGroupID)
+			err := b.DeleteGroup(m.deleteGroupID)
+			assert.Equal(t, m.err, err)
+		})
+	}
+}