diff --git a/pkg/agent/openflow/client.go b/pkg/agent/openflow/client.go index 3f6f8178be1..c632a242acf 100644 --- a/pkg/agent/openflow/client.go +++ b/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) } diff --git a/pkg/agent/openflow/client_test.go b/pkg/agent/openflow/client_test.go index d0ca9c89199..cbf39dd6d64 100644 --- a/pkg/agent/openflow/client_test.go +++ b/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,7 +2058,7 @@ 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) @@ -2040,9 +2066,15 @@ func Test_client_InstallMulticastGroup(t *testing.T) { 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) + } }) } } diff --git a/pkg/ovs/openflow/ofctrl_bridge.go b/pkg/ovs/openflow/ofctrl_bridge.go index a8af88edb89..25f80018aff 100644 --- a/pkg/ovs/openflow/ofctrl_bridge.go +++ b/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)) } diff --git a/pkg/ovs/openflow/ofctrl_bridge_test.go b/pkg/ovs/openflow/ofctrl_bridge_test.go index 43450f9893b..08b3aa1fe55 100644 --- a/pkg/ovs/openflow/ofctrl_bridge_test.go +++ b/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) + }) + } +}