Skip to content

Commit ac3666e

Browse files
oilbeaterclaude
oilbeater
and
claude
authored
fix(controller): handle missing logical switch in handleMcastQuerierChange (#6542)
* fix(controller): return error when logical switch not found in handleMcastQuerierChange When disabling multicast snoop, handleMcastQuerierChange silently returned nil if the logical switch did not exist (err==nil && len==0), causing the caller to skip multicast cleanup. Split the combined condition into two separate checks so a missing logical switch now returns an explicit error. Added unit tests covering all success and failure paths for both enable and disable branches. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> * fix(controller): skip cleanup gracefully when logical switch not found When the logical switch is already gone, multicast cleanup is unnecessary. Return nil with a warning instead of an error to avoid infinite requeue by the work queue. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> --------- Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 4146075 commit ac3666e

File tree

2 files changed

+180
-1
lines changed

2 files changed

+180
-1
lines changed

‎pkg/controller/subnet.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2912,10 +2912,14 @@ func (c *Controller) handleMcastQuerierChange(subnet *kubeovnv1.Subnet) error {
29122912
lss, err := c.OVNNbClient.ListLogicalSwitch(false, func(ls *ovnnb.LogicalSwitch) bool {
29132913
return ls.Name == subnet.Name
29142914
})
2915-
if err != nil || len(lss) == 0 {
2915+
if err != nil {
29162916
klog.Errorf("failed to list logical switch %s: %v", subnet.Name, err)
29172917
return err
29182918
}
2919+
if len(lss) == 0 {
2920+
klog.Warningf("logical switch %s not found, skipping multicast snoop cleanup", subnet.Name)
2921+
return nil
2922+
}
29192923

29202924
multicastSnoopFlag := map[string]string{
29212925
"mcast_snoop": lss[0].OtherConfig["mcast_snoop"],

‎pkg/controller/subnet_test.go‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,15 @@ package controller
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
67
"testing"
78
"time"
89

910
"github.com/stretchr/testify/require"
1011
"go.uber.org/mock/gomock"
1112

13+
"github.com/ovn-kubernetes/libovsdb/ovsdb"
1214
corev1 "k8s.io/api/core/v1"
1315
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1416

@@ -698,3 +700,176 @@ func Test_validateSubnetVlan(t *testing.T) {
698700
require.NoError(t, err)
699701
})
700702
}
703+
704+
func Test_handleMcastQuerierChange(t *testing.T) {
705+
t.Parallel()
706+
707+
subnetName := "test-mcast-subnet"
708+
lspName := fmt.Sprintf(util.McastQuerierName, subnetName)
709+
querierIP := "10.16.0.100"
710+
querierMAC := "00:00:00:ab:cd:ef"
711+
712+
t.Run("enable multicast snoop successfully", func(t *testing.T) {
713+
fakeController := newFakeController(t)
714+
ctrl := fakeController.fakeController
715+
mockOvnClient := fakeController.mockOvnClient
716+
717+
subnet := &kubeovnv1.Subnet{
718+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
719+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: true},
720+
Status: kubeovnv1.SubnetStatus{McastQuerierIP: querierIP, McastQuerierMAC: querierMAC},
721+
}
722+
723+
mockOvnClient.EXPECT().CreateLogicalSwitchPort(subnetName, lspName, querierIP, querierMAC, lspName, metav1.NamespaceDefault, false, "", "", false, nil, "").Return(nil)
724+
mockOvnClient.EXPECT().LogicalSwitchUpdateOtherConfig(subnetName, ovsdb.MutateOperationInsert, gomock.Any()).Return(nil)
725+
726+
err := ctrl.handleMcastQuerierChange(subnet)
727+
require.NoError(t, err)
728+
})
729+
730+
t.Run("enable multicast snoop create lsp fails", func(t *testing.T) {
731+
fakeController := newFakeController(t)
732+
ctrl := fakeController.fakeController
733+
mockOvnClient := fakeController.mockOvnClient
734+
735+
subnet := &kubeovnv1.Subnet{
736+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
737+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: true},
738+
Status: kubeovnv1.SubnetStatus{McastQuerierIP: querierIP, McastQuerierMAC: querierMAC},
739+
}
740+
741+
mockOvnClient.EXPECT().CreateLogicalSwitchPort(subnetName, lspName, querierIP, querierMAC, lspName, metav1.NamespaceDefault, false, "", "", false, nil, "").Return(errors.New("create lsp failed"))
742+
743+
err := ctrl.handleMcastQuerierChange(subnet)
744+
require.Error(t, err)
745+
require.Contains(t, err.Error(), "create lsp failed")
746+
})
747+
748+
t.Run("enable multicast snoop update other config fails", func(t *testing.T) {
749+
fakeController := newFakeController(t)
750+
ctrl := fakeController.fakeController
751+
mockOvnClient := fakeController.mockOvnClient
752+
753+
subnet := &kubeovnv1.Subnet{
754+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
755+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: true},
756+
Status: kubeovnv1.SubnetStatus{McastQuerierIP: querierIP, McastQuerierMAC: querierMAC},
757+
}
758+
759+
mockOvnClient.EXPECT().CreateLogicalSwitchPort(subnetName, lspName, querierIP, querierMAC, lspName, metav1.NamespaceDefault, false, "", "", false, nil, "").Return(nil)
760+
mockOvnClient.EXPECT().LogicalSwitchUpdateOtherConfig(subnetName, ovsdb.MutateOperationInsert, gomock.Any()).Return(errors.New("update config failed"))
761+
762+
err := ctrl.handleMcastQuerierChange(subnet)
763+
require.Error(t, err)
764+
require.Contains(t, err.Error(), "update config failed")
765+
})
766+
767+
t.Run("disable multicast snoop successfully", func(t *testing.T) {
768+
fakeController := newFakeController(t)
769+
ctrl := fakeController.fakeController
770+
mockOvnClient := fakeController.mockOvnClient
771+
772+
subnet := &kubeovnv1.Subnet{
773+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
774+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: false},
775+
}
776+
777+
lss := []ovnnb.LogicalSwitch{{
778+
Name: subnetName,
779+
OtherConfig: map[string]string{
780+
"mcast_snoop": "true",
781+
"mcast_querier": "true",
782+
"mcast_ip4_src": querierIP,
783+
"mcast_eth_src": querierMAC,
784+
},
785+
}}
786+
787+
mockOvnClient.EXPECT().ListLogicalSwitch(false, gomock.Any()).Return(lss, nil)
788+
mockOvnClient.EXPECT().LogicalSwitchUpdateOtherConfig(subnetName, ovsdb.MutateOperationDelete, gomock.Any()).Return(nil)
789+
mockOvnClient.EXPECT().DeleteLogicalSwitchPort(lspName).Return(nil)
790+
791+
err := ctrl.handleMcastQuerierChange(subnet)
792+
require.NoError(t, err)
793+
})
794+
795+
t.Run("disable multicast snoop list logical switch fails", func(t *testing.T) {
796+
fakeController := newFakeController(t)
797+
ctrl := fakeController.fakeController
798+
mockOvnClient := fakeController.mockOvnClient
799+
800+
subnet := &kubeovnv1.Subnet{
801+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
802+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: false},
803+
}
804+
805+
mockOvnClient.EXPECT().ListLogicalSwitch(false, gomock.Any()).Return(nil, errors.New("list failed"))
806+
807+
err := ctrl.handleMcastQuerierChange(subnet)
808+
require.Error(t, err)
809+
require.Contains(t, err.Error(), "list failed")
810+
})
811+
812+
// logical switch not found: cleanup is unnecessary, should return nil to avoid infinite requeue
813+
t.Run("disable multicast snoop logical switch not found skips cleanup", func(t *testing.T) {
814+
fakeController := newFakeController(t)
815+
ctrl := fakeController.fakeController
816+
mockOvnClient := fakeController.mockOvnClient
817+
818+
subnet := &kubeovnv1.Subnet{
819+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
820+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: false},
821+
}
822+
823+
mockOvnClient.EXPECT().ListLogicalSwitch(false, gomock.Any()).Return([]ovnnb.LogicalSwitch{}, nil)
824+
825+
err := ctrl.handleMcastQuerierChange(subnet)
826+
require.NoError(t, err)
827+
})
828+
829+
t.Run("disable multicast snoop delete other config fails", func(t *testing.T) {
830+
fakeController := newFakeController(t)
831+
ctrl := fakeController.fakeController
832+
mockOvnClient := fakeController.mockOvnClient
833+
834+
subnet := &kubeovnv1.Subnet{
835+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
836+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: false},
837+
}
838+
839+
lss := []ovnnb.LogicalSwitch{{
840+
Name: subnetName,
841+
OtherConfig: map[string]string{},
842+
}}
843+
844+
mockOvnClient.EXPECT().ListLogicalSwitch(false, gomock.Any()).Return(lss, nil)
845+
mockOvnClient.EXPECT().LogicalSwitchUpdateOtherConfig(subnetName, ovsdb.MutateOperationDelete, gomock.Any()).Return(errors.New("delete config failed"))
846+
847+
err := ctrl.handleMcastQuerierChange(subnet)
848+
require.Error(t, err)
849+
require.Contains(t, err.Error(), "delete config failed")
850+
})
851+
852+
t.Run("disable multicast snoop delete lsp fails", func(t *testing.T) {
853+
fakeController := newFakeController(t)
854+
ctrl := fakeController.fakeController
855+
mockOvnClient := fakeController.mockOvnClient
856+
857+
subnet := &kubeovnv1.Subnet{
858+
ObjectMeta: metav1.ObjectMeta{Name: subnetName},
859+
Spec: kubeovnv1.SubnetSpec{EnableMulticastSnoop: false},
860+
}
861+
862+
lss := []ovnnb.LogicalSwitch{{
863+
Name: subnetName,
864+
OtherConfig: map[string]string{},
865+
}}
866+
867+
mockOvnClient.EXPECT().ListLogicalSwitch(false, gomock.Any()).Return(lss, nil)
868+
mockOvnClient.EXPECT().LogicalSwitchUpdateOtherConfig(subnetName, ovsdb.MutateOperationDelete, gomock.Any()).Return(nil)
869+
mockOvnClient.EXPECT().DeleteLogicalSwitchPort(lspName).Return(errors.New("delete lsp failed"))
870+
871+
err := ctrl.handleMcastQuerierChange(subnet)
872+
require.Error(t, err)
873+
require.Contains(t, err.Error(), "delete lsp failed")
874+
})
875+
}

0 commit comments

Comments
 (0)