fix logging in ovn lr/ls gc (#5014)

we should print lr/ls names instead of ovn nb objects.

Signed-off-by: zhangzujian <zhangzujian.7@gmail.com>

Author: zhangzujian

mocks/pkg/ovs/interface.go

@@ -17,6 +17,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2"
+	"k8s.io/utils/set"
 
 	kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1"
 	"github.com/kubeovn/kube-ovn/pkg/ovs"
@@ -127,34 +128,35 @@ func (c *Controller) gcLogicalSwitch() error {
 		klog.Errorf("failed to list subnet, %v", err)
 		return err
 	}
-	subnetNames := strset.NewWithSize(len(subnets))
-	subnetMap := make(map[string]*kubeovnv1.Subnet, len(subnets))
-	for _, s := range subnets {
-		subnetMap[s.Name] = s
-		subnetNames.Add(s.Name)
-	}
 
-	lss, err := c.OVNNbClient.ListLogicalSwitch(c.config.EnableExternalVpc, nil)
+	lss, err := c.OVNNbClient.ListLogicalSwitchNames(c.config.EnableExternalVpc, nil)
 	if err != nil {
-		klog.Errorf("list logical switch: %v", err)
+		klog.Errorf("failed to list logical switch: %v", err)
 		return err
 	}
 
-	klog.Infof("ls in ovn %v", lss)
-	klog.Infof("subnet in kubernetes %v", subnetNames)
+	subnetNames := set.New[string]()
+	subnetMap := make(map[string]*kubeovnv1.Subnet, len(subnets))
+	for _, s := range subnets {
+		subnetMap[s.Name] = s
+		subnetNames.Insert(s.Name)
+	}
+
+	klog.Infof("logical switch in ovn: %v", lss)
+	klog.Infof("subnet in kubernetes: %v", subnetNames)
 	for _, ls := range lss {
-		if ls.Name == util.InterconnectionSwitch ||
-			ls.Name == util.ExternalGatewaySwitch ||
-			ls.Name == c.config.ExternalGatewaySwitch {
+		if ls == util.InterconnectionSwitch ||
+			ls == util.ExternalGatewaySwitch ||
+			ls == c.config.ExternalGatewaySwitch {
 			continue
 		}
-		if s := subnetMap[ls.Name]; s != nil && isOvnSubnet(s) {
+		if s := subnetMap[ls]; s != nil && isOvnSubnet(s) {
 			continue
 		}
 
-		klog.Infof("gc subnet %s", ls.Name)
-		if err := c.handleDeleteLogicalSwitch(ls.Name); err != nil {
-			klog.Errorf("failed to gc subnet %s, %v", ls.Name, err)
+		klog.Infof("gc logical switch %s", ls)
+		if err = c.handleDeleteLogicalSwitch(ls); err != nil {
+			klog.Errorf("failed to gc logical switch %q: %v", ls, err)
 			return err
 		}
 	}
@@ -188,30 +190,30 @@ func (c *Controller) gcCustomLogicalRouter() error {
 		klog.Errorf("failed to list vpc, %v", err)
 		return err
 	}
-	vpcNames := make([]string, 0, len(vpcs))
-	for _, s := range vpcs {
-		vpcNames = append(vpcNames, s.Name)
-	}
 
-	lrs, err := c.OVNNbClient.ListLogicalRouter(c.config.EnableExternalVpc, nil)
+	lrs, err := c.OVNNbClient.ListLogicalRouterNames(c.config.EnableExternalVpc, nil)
 	if err != nil {
 		klog.Errorf("failed to list logical router, %v", err)
 		return err
 	}
 
-	klog.Infof("lr in ovn %v", lrs)
-	klog.Infof("vpc in kubernetes %v", vpcNames)
+	vpcNames := set.New[string]()
+	for _, v := range vpcs {
+		vpcNames.Insert(v.Name)
+	}
+
+	klog.Infof("lr in ovn: %v", lrs)
+	klog.Infof("vpc in kubernetes: %v", vpcNames)
 
 	for _, lr := range lrs {
-		if lr.Name == c.config.ClusterRouter {
+		if lr == c.config.ClusterRouter || vpcNames.Has(lr) {
 			continue
 		}
-		if !slices.Contains(vpcNames, lr.Name) {
-			klog.Infof("gc router %s", lr.Name)
-			if err := c.deleteVpcRouter(lr.Name); err != nil {
-				klog.Errorf("failed to delete router %s, %v", lr.Name, err)
-				return err
-			}
+
+		klog.Infof("gc logical router %s", lr)
+		if err = c.deleteVpcRouter(lr); err != nil {
+			klog.Errorf("failed to gc logical router %q: %v", lr, err)
+			return err
 		}
 	}
 	return nil

pkg/controller/gc.go

@@ -32,6 +32,7 @@ type LogicalRouter interface {
 	LogicalRouterUpdateLoadBalancers(lrName string, op ovsdb.Mutator, lbNames ...string) error
 	GetLogicalRouter(lrName string, ignoreNotFound bool) (*ovnnb.LogicalRouter, error)
 	ListLogicalRouter(needVendorFilter bool, filter func(lr *ovnnb.LogicalRouter) bool) ([]ovnnb.LogicalRouter, error)
+	ListLogicalRouterNames(needVendorFilter bool, filter func(lr *ovnnb.LogicalRouter) bool) ([]string, error)
 	LogicalRouterExists(name string) (bool, error)
 }
 
@@ -80,6 +81,7 @@ type LogicalSwitch interface {
 	LogicalSwitchUpdateOtherConfig(lsName string, op ovsdb.Mutator, otherConfig map[string]string) error
 	DeleteLogicalSwitch(lsName string) error
 	ListLogicalSwitch(needVendorFilter bool, filter func(ls *ovnnb.LogicalSwitch) bool) ([]ovnnb.LogicalSwitch, error)
+	ListLogicalSwitchNames(needVendorFilter bool, filter func(ls *ovnnb.LogicalSwitch) bool) ([]string, error)
 	LogicalSwitchExists(lsName string) (bool, error)
 }
 

pkg/ovs/interface.go

@@ -130,8 +130,7 @@ func (c *OVNNbClient) ListLogicalRouter(needVendorFilter bool, filter func(lr *o
 	ctx, cancel := context.WithTimeout(context.Background(), c.Timeout)
 	defer cancel()
 
-	lrList := make([]ovnnb.LogicalRouter, 0)
-
+	var lrList []ovnnb.LogicalRouter
 	if err := c.ovsDbClient.WhereCache(func(lr *ovnnb.LogicalRouter) bool {
 		if needVendorFilter && (len(lr.ExternalIDs) == 0 || lr.ExternalIDs["vendor"] != util.CniTypeName) {
 			return false
@@ -150,6 +149,21 @@ func (c *OVNNbClient) ListLogicalRouter(needVendorFilter bool, filter func(lr *o
 	return lrList, nil
 }
 
+// ListLogicalRouterNames list logical router names
+func (c *OVNNbClient) ListLogicalRouterNames(needVendorFilter bool, filter func(lr *ovnnb.LogicalRouter) bool) ([]string, error) {
+	lrList, err := c.ListLogicalRouter(needVendorFilter, filter)
+	if err != nil {
+		klog.Error(err)
+		return nil, err
+	}
+
+	names := make([]string, 0, len(lrList))
+	for _, lr := range lrList {
+		names = append(names, lr.Name)
+	}
+	return names, nil
+}
+
 // LogicalRouterUpdateLoadBalancers add several lb to or from logical router once
 func (c *OVNNbClient) LogicalRouterUpdateLoadBalancers(lrName string, op ovsdb.Mutator, lbNames ...string) error {
 	if len(lbNames) == 0 {

pkg/ovs/ovn-nb-logical_router.go

@@ -7,9 +7,10 @@ import (
 
 	"github.com/ovn-org/libovsdb/model"
 	"github.com/ovn-org/libovsdb/ovsdb"
-	"github.com/stretchr/testify/require"
 	"k8s.io/klog/v2"
 
+	"github.com/stretchr/testify/require"
+
 	ovsclient "github.com/kubeovn/kube-ovn/pkg/ovsdb/client"
 	"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"
 	"github.com/kubeovn/kube-ovn/pkg/util"
@@ -70,7 +71,7 @@ func (suite *OvnClientTestSuite) testCreateLogicalRouter() {
 		require.NoError(t, err)
 
 		err = nbClient.CreateLogicalRouter(name)
-		require.ErrorContains(t, err, "more than one logical router with same name \"test-create-lr-more-than-one\"")
+		require.ErrorContains(t, err, fmt.Sprintf("more than one logical router with same name %q", name))
 	})
 }
 
@@ -154,7 +155,7 @@ func (suite *OvnClientTestSuite) testDeleteLogicalRouter() {
 		require.NoError(t, err)
 
 		err = nbClient.DeleteLogicalRouter(name)
-		require.ErrorContains(t, err, "more than one logical router with same name \"test-delete-lr-more-than-one\"")
+		require.ErrorContains(t, err, fmt.Sprintf("more than one logical router with same name %q", name))
 	})
 }
 
@@ -232,30 +233,55 @@ func (suite *OvnClientTestSuite) testListLogicalRouter() {
 		lrs, err := nbClient.ListLogicalRouter(true, nil)
 		require.NoError(t, err)
 
-		count := 0
+		count, names := 0, make([]string, 0, 3)
 		for _, lr := range lrs {
 			if strings.Contains(lr.Name, namePrefix) {
+				names = append(names, lr.Name)
 				count++
 			}
 		}
-		require.Equal(t, count, 3)
+		require.Equal(t, 3, count)
+
+		lrNames, err := nbClient.ListLogicalRouterNames(true, nil)
+		require.NoError(t, err)
+
+		filterdNames := make([]string, 0, len(names))
+		for _, lr := range lrNames {
+			if strings.Contains(lr, namePrefix) {
+				filterdNames = append(filterdNames, lr)
+			}
+		}
+		require.ElementsMatch(t, filterdNames, names)
 	})
 
 	t.Run("has custom filter", func(t *testing.T) {
 		t.Parallel()
-		lrs, err := nbClient.ListLogicalRouter(false, func(lr *ovnnb.LogicalRouter) bool {
-			return len(lr.ExternalIDs) == 0 || lr.ExternalIDs["vendor"] != util.CniTypeName
-		})
 
+		filter := func(lr *ovnnb.LogicalRouter) bool {
+			return len(lr.ExternalIDs) == 0 || lr.ExternalIDs["vendor"] != util.CniTypeName
+		}
+		lrs, err := nbClient.ListLogicalRouter(false, filter)
 		require.NoError(t, err)
 
-		count := 0
+		count, names := 0, make([]string, 0, 4)
 		for _, lr := range lrs {
 			if strings.Contains(lr.Name, namePrefix) {
+				names = append(names, lr.Name)
 				count++
 			}
 		}
-		require.Equal(t, count, 4)
+		require.Equal(t, 4, count)
+
+		lrNames, err := nbClient.ListLogicalRouterNames(false, filter)
+		require.NoError(t, err)
+
+		filterdNames := make([]string, 0, len(names))
+		for _, lr := range lrNames {
+			if strings.Contains(lr, namePrefix) {
+				filterdNames = append(filterdNames, lr)
+			}
+		}
+		require.ElementsMatch(t, filterdNames, names)
 	})
 }
 
@@ -328,9 +354,10 @@ func (suite *OvnClientTestSuite) testLogicalRouterUpdateLoadBalancers() {
 
 	t.Run("del lbs from logical router with more than one existing lb", func(t *testing.T) {
 		protocol := "tcp"
+		name := "test-del-lb-with-more-than-one-lb"
 		lb := &ovnnb.LoadBalancer{
 			UUID:     ovsclient.NamedUUID(),
-			Name:     "test-del-lb-with-more-than-one-lb",
+			Name:     name,
 			Protocol: &protocol,
 		}
 		ops, err := nbClient.ovsDbClient.Create(lb)
@@ -341,8 +368,8 @@ func (suite *OvnClientTestSuite) testLogicalRouterUpdateLoadBalancers() {
 		err = nbClient.Transact("lb-add", ops)
 		require.NoError(t, err)
 
-		err = nbClient.LogicalRouterUpdateLoadBalancers(lrName, ovsdb.MutateOperationDelete, []string{"test-del-lb-with-more-than-one-lb"}...)
-		require.ErrorContains(t, err, "more than one load balancer with same name \"test-del-lb-with-more-than-one-lb\"")
+		err = nbClient.LogicalRouterUpdateLoadBalancers(lrName, ovsdb.MutateOperationDelete, []string{name}...)
+		require.ErrorContains(t, err, fmt.Sprintf("more than one load balancer with same name %q", name))
 	})
 
 	t.Run("del lbs from non-exist logical router", func(t *testing.T) {

pkg/ovs/ovn-nb-logical_router_test.go

@@ -293,6 +293,21 @@ func (c *OVNNbClient) ListLogicalSwitch(needVendorFilter bool, filter func(ls *o
 	return lsList, nil
 }
 
+// ListLogicalSwitchNames list logical switch names
+func (c *OVNNbClient) ListLogicalSwitchNames(needVendorFilter bool, filter func(ls *ovnnb.LogicalSwitch) bool) ([]string, error) {
+	lsList, err := c.ListLogicalSwitch(needVendorFilter, filter)
+	if err != nil {
+		klog.Error(err)
+		return nil, err
+	}
+
+	names := make([]string, 0, len(lsList))
+	for i := range lsList {
+		names = append(names, lsList[i].Name)
+	}
+	return names, nil
+}
+
 // LogicalSwitchUpdatePortOp create operations add port to or delete port from logical switch
 func (c *OVNNbClient) LogicalSwitchUpdatePortOp(lsName, lspUUID string, op ovsdb.Mutator) ([]ovsdb.Operation, error) {
 	if len(lspUUID) == 0 {