Skip to content

Commit 3c0d849

Browse files
zhangzujianzhangzujian
authored
fix logging in ovn lr/ls gc (#5014)
we should print lr/ls names instead of ovn nb objects. Signed-off-by: zhangzujian <[email protected]>
1 parent 91b9698 commit 3c0d849

9 files changed

+202
-57
lines changed

‎mocks/pkg/ovs/interface.go

+60
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/controller/gc.go

+33-31
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"k8s.io/apimachinery/pkg/labels"
1818
"k8s.io/apimachinery/pkg/types"
1919
"k8s.io/klog/v2"
20+
"k8s.io/utils/set"
2021

2122
kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1"
2223
"github.com/kubeovn/kube-ovn/pkg/ovs"
@@ -127,34 +128,35 @@ func (c *Controller) gcLogicalSwitch() error {
127128
klog.Errorf("failed to list subnet, %v", err)
128129
return err
129130
}
130-
subnetNames := strset.NewWithSize(len(subnets))
131-
subnetMap := make(map[string]*kubeovnv1.Subnet, len(subnets))
132-
for _, s := range subnets {
133-
subnetMap[s.Name] = s
134-
subnetNames.Add(s.Name)
135-
}
136131

137-
lss, err := c.OVNNbClient.ListLogicalSwitch(c.config.EnableExternalVpc, nil)
132+
lss, err := c.OVNNbClient.ListLogicalSwitchNames(c.config.EnableExternalVpc, nil)
138133
if err != nil {
139-
klog.Errorf("list logical switch: %v", err)
134+
klog.Errorf("failed to list logical switch: %v", err)
140135
return err
141136
}
142137

143-
klog.Infof("ls in ovn %v", lss)
144-
klog.Infof("subnet in kubernetes %v", subnetNames)
138+
subnetNames := set.New[string]()
139+
subnetMap := make(map[string]*kubeovnv1.Subnet, len(subnets))
140+
for _, s := range subnets {
141+
subnetMap[s.Name] = s
142+
subnetNames.Insert(s.Name)
143+
}
144+
145+
klog.Infof("logical switch in ovn: %v", lss)
146+
klog.Infof("subnet in kubernetes: %v", subnetNames)
145147
for _, ls := range lss {
146-
if ls.Name == util.InterconnectionSwitch ||
147-
ls.Name == util.ExternalGatewaySwitch ||
148-
ls.Name == c.config.ExternalGatewaySwitch {
148+
if ls == util.InterconnectionSwitch ||
149+
ls == util.ExternalGatewaySwitch ||
150+
ls == c.config.ExternalGatewaySwitch {
149151
continue
150152
}
151-
if s := subnetMap[ls.Name]; s != nil && isOvnSubnet(s) {
153+
if s := subnetMap[ls]; s != nil && isOvnSubnet(s) {
152154
continue
153155
}
154156

155-
klog.Infof("gc subnet %s", ls.Name)
156-
if err := c.handleDeleteLogicalSwitch(ls.Name); err != nil {
157-
klog.Errorf("failed to gc subnet %s, %v", ls.Name, err)
157+
klog.Infof("gc logical switch %s", ls)
158+
if err = c.handleDeleteLogicalSwitch(ls); err != nil {
159+
klog.Errorf("failed to gc logical switch %q: %v", ls, err)
158160
return err
159161
}
160162
}
@@ -188,30 +190,30 @@ func (c *Controller) gcCustomLogicalRouter() error {
188190
klog.Errorf("failed to list vpc, %v", err)
189191
return err
190192
}
191-
vpcNames := make([]string, 0, len(vpcs))
192-
for _, s := range vpcs {
193-
vpcNames = append(vpcNames, s.Name)
194-
}
195193

196-
lrs, err := c.OVNNbClient.ListLogicalRouter(c.config.EnableExternalVpc, nil)
194+
lrs, err := c.OVNNbClient.ListLogicalRouterNames(c.config.EnableExternalVpc, nil)
197195
if err != nil {
198196
klog.Errorf("failed to list logical router, %v", err)
199197
return err
200198
}
201199

202-
klog.Infof("lr in ovn %v", lrs)
203-
klog.Infof("vpc in kubernetes %v", vpcNames)
200+
vpcNames := set.New[string]()
201+
for _, v := range vpcs {
202+
vpcNames.Insert(v.Name)
203+
}
204+
205+
klog.Infof("lr in ovn: %v", lrs)
206+
klog.Infof("vpc in kubernetes: %v", vpcNames)
204207

205208
for _, lr := range lrs {
206-
if lr.Name == c.config.ClusterRouter {
209+
if lr == c.config.ClusterRouter || vpcNames.Has(lr) {
207210
continue
208211
}
209-
if !slices.Contains(vpcNames, lr.Name) {
210-
klog.Infof("gc router %s", lr.Name)
211-
if err := c.deleteVpcRouter(lr.Name); err != nil {
212-
klog.Errorf("failed to delete router %s, %v", lr.Name, err)
213-
return err
214-
}
212+
213+
klog.Infof("gc logical router %s", lr)
214+
if err = c.deleteVpcRouter(lr); err != nil {
215+
klog.Errorf("failed to gc logical router %q: %v", lr, err)
216+
return err
215217
}
216218
}
217219
return nil

‎pkg/ovs/interface.go

+2
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ type LogicalRouter interface {
3232
LogicalRouterUpdateLoadBalancers(lrName string, op ovsdb.Mutator, lbNames ...string) error
3333
GetLogicalRouter(lrName string, ignoreNotFound bool) (*ovnnb.LogicalRouter, error)
3434
ListLogicalRouter(needVendorFilter bool, filter func(lr *ovnnb.LogicalRouter) bool) ([]ovnnb.LogicalRouter, error)
35+
ListLogicalRouterNames(needVendorFilter bool, filter func(lr *ovnnb.LogicalRouter) bool) ([]string, error)
3536
LogicalRouterExists(name string) (bool, error)
3637
}
3738

@@ -80,6 +81,7 @@ type LogicalSwitch interface {
8081
LogicalSwitchUpdateOtherConfig(lsName string, op ovsdb.Mutator, otherConfig map[string]string) error
8182
DeleteLogicalSwitch(lsName string) error
8283
ListLogicalSwitch(needVendorFilter bool, filter func(ls *ovnnb.LogicalSwitch) bool) ([]ovnnb.LogicalSwitch, error)
84+
ListLogicalSwitchNames(needVendorFilter bool, filter func(ls *ovnnb.LogicalSwitch) bool) ([]string, error)
8385
LogicalSwitchExists(lsName string) (bool, error)
8486
}
8587

‎pkg/ovs/ovn-nb-logical_router.go

+16-2
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,7 @@ func (c *OVNNbClient) ListLogicalRouter(needVendorFilter bool, filter func(lr *o
130130
ctx, cancel := context.WithTimeout(context.Background(), c.Timeout)
131131
defer cancel()
132132

133-
lrList := make([]ovnnb.LogicalRouter, 0)
134-
133+
var lrList []ovnnb.LogicalRouter
135134
if err := c.ovsDbClient.WhereCache(func(lr *ovnnb.LogicalRouter) bool {
136135
if needVendorFilter && (len(lr.ExternalIDs) == 0 || lr.ExternalIDs["vendor"] != util.CniTypeName) {
137136
return false
@@ -150,6 +149,21 @@ func (c *OVNNbClient) ListLogicalRouter(needVendorFilter bool, filter func(lr *o
150149
return lrList, nil
151150
}
152151

152+
// ListLogicalRouterNames list logical router names
153+
func (c *OVNNbClient) ListLogicalRouterNames(needVendorFilter bool, filter func(lr *ovnnb.LogicalRouter) bool) ([]string, error) {
154+
lrList, err := c.ListLogicalRouter(needVendorFilter, filter)
155+
if err != nil {
156+
klog.Error(err)
157+
return nil, err
158+
}
159+
160+
names := make([]string, 0, len(lrList))
161+
for _, lr := range lrList {
162+
names = append(names, lr.Name)
163+
}
164+
return names, nil
165+
}
166+
153167
// LogicalRouterUpdateLoadBalancers add several lb to or from logical router once
154168
func (c *OVNNbClient) LogicalRouterUpdateLoadBalancers(lrName string, op ovsdb.Mutator, lbNames ...string) error {
155169
if len(lbNames) == 0 {

‎pkg/ovs/ovn-nb-logical_router_test.go

+40-13
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,10 @@ import (
77

88
"github.com/ovn-org/libovsdb/model"
99
"github.com/ovn-org/libovsdb/ovsdb"
10-
"github.com/stretchr/testify/require"
1110
"k8s.io/klog/v2"
1211

12+
"github.com/stretchr/testify/require"
13+
1314
ovsclient "github.com/kubeovn/kube-ovn/pkg/ovsdb/client"
1415
"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"
1516
"github.com/kubeovn/kube-ovn/pkg/util"
@@ -70,7 +71,7 @@ func (suite *OvnClientTestSuite) testCreateLogicalRouter() {
7071
require.NoError(t, err)
7172

7273
err = nbClient.CreateLogicalRouter(name)
73-
require.ErrorContains(t, err, "more than one logical router with same name \"test-create-lr-more-than-one\"")
74+
require.ErrorContains(t, err, fmt.Sprintf("more than one logical router with same name %q", name))
7475
})
7576
}
7677

@@ -154,7 +155,7 @@ func (suite *OvnClientTestSuite) testDeleteLogicalRouter() {
154155
require.NoError(t, err)
155156

156157
err = nbClient.DeleteLogicalRouter(name)
157-
require.ErrorContains(t, err, "more than one logical router with same name \"test-delete-lr-more-than-one\"")
158+
require.ErrorContains(t, err, fmt.Sprintf("more than one logical router with same name %q", name))
158159
})
159160
}
160161

@@ -232,30 +233,55 @@ func (suite *OvnClientTestSuite) testListLogicalRouter() {
232233
lrs, err := nbClient.ListLogicalRouter(true, nil)
233234
require.NoError(t, err)
234235

235-
count := 0
236+
count, names := 0, make([]string, 0, 3)
236237
for _, lr := range lrs {
237238
if strings.Contains(lr.Name, namePrefix) {
239+
names = append(names, lr.Name)
238240
count++
239241
}
240242
}
241-
require.Equal(t, count, 3)
243+
require.Equal(t, 3, count)
244+
245+
lrNames, err := nbClient.ListLogicalRouterNames(true, nil)
246+
require.NoError(t, err)
247+
248+
filterdNames := make([]string, 0, len(names))
249+
for _, lr := range lrNames {
250+
if strings.Contains(lr, namePrefix) {
251+
filterdNames = append(filterdNames, lr)
252+
}
253+
}
254+
require.ElementsMatch(t, filterdNames, names)
242255
})
243256

244257
t.Run("has custom filter", func(t *testing.T) {
245258
t.Parallel()
246-
lrs, err := nbClient.ListLogicalRouter(false, func(lr *ovnnb.LogicalRouter) bool {
247-
return len(lr.ExternalIDs) == 0 || lr.ExternalIDs["vendor"] != util.CniTypeName
248-
})
249259

260+
filter := func(lr *ovnnb.LogicalRouter) bool {
261+
return len(lr.ExternalIDs) == 0 || lr.ExternalIDs["vendor"] != util.CniTypeName
262+
}
263+
lrs, err := nbClient.ListLogicalRouter(false, filter)
250264
require.NoError(t, err)
251265

252-
count := 0
266+
count, names := 0, make([]string, 0, 4)
253267
for _, lr := range lrs {
254268
if strings.Contains(lr.Name, namePrefix) {
269+
names = append(names, lr.Name)
255270
count++
256271
}
257272
}
258-
require.Equal(t, count, 4)
273+
require.Equal(t, 4, count)
274+
275+
lrNames, err := nbClient.ListLogicalRouterNames(false, filter)
276+
require.NoError(t, err)
277+
278+
filterdNames := make([]string, 0, len(names))
279+
for _, lr := range lrNames {
280+
if strings.Contains(lr, namePrefix) {
281+
filterdNames = append(filterdNames, lr)
282+
}
283+
}
284+
require.ElementsMatch(t, filterdNames, names)
259285
})
260286
}
261287

@@ -328,9 +354,10 @@ func (suite *OvnClientTestSuite) testLogicalRouterUpdateLoadBalancers() {
328354

329355
t.Run("del lbs from logical router with more than one existing lb", func(t *testing.T) {
330356
protocol := "tcp"
357+
name := "test-del-lb-with-more-than-one-lb"
331358
lb := &ovnnb.LoadBalancer{
332359
UUID: ovsclient.NamedUUID(),
333-
Name: "test-del-lb-with-more-than-one-lb",
360+
Name: name,
334361
Protocol: &protocol,
335362
}
336363
ops, err := nbClient.ovsDbClient.Create(lb)
@@ -341,8 +368,8 @@ func (suite *OvnClientTestSuite) testLogicalRouterUpdateLoadBalancers() {
341368
err = nbClient.Transact("lb-add", ops)
342369
require.NoError(t, err)
343370

344-
err = nbClient.LogicalRouterUpdateLoadBalancers(lrName, ovsdb.MutateOperationDelete, []string{"test-del-lb-with-more-than-one-lb"}...)
345-
require.ErrorContains(t, err, "more than one load balancer with same name \"test-del-lb-with-more-than-one-lb\"")
371+
err = nbClient.LogicalRouterUpdateLoadBalancers(lrName, ovsdb.MutateOperationDelete, []string{name}...)
372+
require.ErrorContains(t, err, fmt.Sprintf("more than one load balancer with same name %q", name))
346373
})
347374

348375
t.Run("del lbs from non-exist logical router", func(t *testing.T) {

‎pkg/ovs/ovn-nb-logical_switch.go

+15
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,21 @@ func (c *OVNNbClient) ListLogicalSwitch(needVendorFilter bool, filter func(ls *o
293293
return lsList, nil
294294
}
295295

296+
// ListLogicalSwitchNames list logical switch names
297+
func (c *OVNNbClient) ListLogicalSwitchNames(needVendorFilter bool, filter func(ls *ovnnb.LogicalSwitch) bool) ([]string, error) {
298+
lsList, err := c.ListLogicalSwitch(needVendorFilter, filter)
299+
if err != nil {
300+
klog.Error(err)
301+
return nil, err
302+
}
303+
304+
names := make([]string, 0, len(lsList))
305+
for i := range lsList {
306+
names = append(names, lsList[i].Name)
307+
}
308+
return names, nil
309+
}
310+
296311
// LogicalSwitchUpdatePortOp create operations add port to or delete port from logical switch
297312
func (c *OVNNbClient) LogicalSwitchUpdatePortOp(lsName, lspUUID string, op ovsdb.Mutator) ([]ovsdb.Operation, error) {
298313
if len(lspUUID) == 0 {

0 commit comments

Comments
 (0)