Skip to content

Commit 6f15735

Browse files
authored
fix metallb underlay lflow rule is deleted unexpected (#5723)
* fix metallb underlay lflow rule is deleted unexpected Signed-off-by: clyi <clyi@alauda.io> * add log Signed-off-by: clyi <clyi@alauda.io> --------- Signed-off-by: clyi <clyi@alauda.io>
1 parent 6000a7a commit 6f15735

File tree

3 files changed

+189
-39
lines changed

3 files changed

+189
-39
lines changed

pkg/ovs/ovn-nb-load_balancer.go

Lines changed: 113 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -467,68 +467,153 @@ func (c *OVNNbClient) LoadBalancerAddIPPortMapping(lbName, vipEndpoint string, m
467467
return nil
468468
}
469469

470-
// LoadBalancerDeleteIPPortMapping delete load balancer ip port mapping
470+
// LoadBalancerDeleteIPPortMapping deletes IP port mappings for a specific VIP from a load balancer.
471+
// This function ensures that only backend IPs that are no longer referenced by any VIP are removed.
471472
func (c *OVNNbClient) LoadBalancerDeleteIPPortMapping(lbName, vipEndpoint string) error {
472-
var (
473-
ops []ovsdb.Operation
474-
lb *ovnnb.LoadBalancer
475-
mappings map[string]string
476-
vip string
477-
err error
478-
)
473+
lb, err := c.getLoadBalancerForDeletion(lbName)
474+
if err != nil {
475+
klog.Errorf("failed to get load balancer for deletion: %v", err)
476+
return err
477+
}
478+
if lb == nil {
479+
return nil
480+
}
479481

480-
if lb, err = c.GetLoadBalancer(lbName, true); err != nil {
481-
klog.Errorf("failed to get lb health check: %v", err)
482+
targetBackendIPs, err := c.extractBackendIPsFromVIP(lb, vipEndpoint)
483+
if err != nil {
484+
klog.Errorf("failed to extract backend IPs from VIP: %v", err)
482485
return err
483486
}
487+
if len(targetBackendIPs) == 0 {
488+
return nil
489+
}
490+
491+
unusedBackendIPs := c.findUnusedBackendIPs(lb, vipEndpoint, targetBackendIPs)
492+
return c.deleteUnusedIPPortMappings(lbName, vipEndpoint, unusedBackendIPs)
493+
}
494+
495+
// getLoadBalancerForDeletion retrieves the load balancer and performs initial validation
496+
func (c *OVNNbClient) getLoadBalancerForDeletion(lbName string) (*ovnnb.LoadBalancer, error) {
497+
lb, err := c.GetLoadBalancer(lbName, true)
498+
if err != nil {
499+
klog.Errorf("failed to get load balancer %s: %v", lbName, err)
500+
return nil, err
501+
}
484502

485503
if lb == nil {
486-
klog.Infof("lb %s already deleted", lbName)
487-
return nil
504+
klog.Infof("load balancer %s already deleted", lbName)
505+
return nil, nil
488506
}
507+
489508
if len(lb.IPPortMappings) == 0 {
490-
klog.Infof("lb %s has no ip port mapping", lbName)
491-
return nil
509+
klog.Infof("load balancer %s has no IP port mappings", lbName)
510+
return nil, nil
492511
}
493512

494-
if vip, _, err = net.SplitHostPort(vipEndpoint); err != nil {
495-
err := fmt.Errorf("failed to split host port: %w", err)
496-
klog.Error(err)
497-
return err
513+
return lb, nil
514+
}
515+
516+
// extractBackendIPsFromVIP extracts the backend IPs that are used by a specific VIP
517+
func (c *OVNNbClient) extractBackendIPsFromVIP(lb *ovnnb.LoadBalancer, vipEndpoint string) (map[string]bool, error) {
518+
vipBackends, exists := lb.Vips[vipEndpoint]
519+
if !exists {
520+
klog.Infof("VIP %s not found in load balancer", vipEndpoint)
521+
return nil, nil
498522
}
499523

500-
mappings = lb.IPPortMappings
501-
for portIP, portMapVip := range lb.IPPortMappings {
502-
splits := strings.Split(portMapVip, ":")
503-
if len(splits) == 2 && splits[1] == vip {
504-
delete(mappings, portIP)
524+
backendIPs := make(map[string]bool)
525+
526+
for backend := range strings.SplitSeq(vipBackends, ",") {
527+
if backendIP, _, err := net.SplitHostPort(backend); err == nil {
528+
backendIPs[backendIP] = true
505529
}
506530
}
507531

508-
if ops, err = c.LoadBalancerOp(
532+
klog.V(4).Infof("VIP %s uses %d backend IPs: %v", vipEndpoint, len(backendIPs), getMapKeys(backendIPs))
533+
return backendIPs, nil
534+
}
535+
536+
// findUnusedBackendIPs identifies which backend IPs are no longer used by any other VIP
537+
func (c *OVNNbClient) findUnusedBackendIPs(lb *ovnnb.LoadBalancer, targetVIP string, targetBackendIPs map[string]bool) map[string]string {
538+
unusedBackendIPs := make(map[string]string)
539+
540+
for backendIP := range targetBackendIPs {
541+
if !c.isBackendIPStillUsed(lb, targetVIP, backendIP) {
542+
if portMapping, exists := lb.IPPortMappings[backendIP]; exists {
543+
unusedBackendIPs[backendIP] = portMapping
544+
}
545+
}
546+
}
547+
548+
klog.V(4).Infof("Found %d unused backend IPs for VIP %s", len(unusedBackendIPs), targetVIP)
549+
return unusedBackendIPs
550+
}
551+
552+
// isBackendIPStillUsed checks if a backend IP is still referenced by any other VIP
553+
func (c *OVNNbClient) isBackendIPStillUsed(lb *ovnnb.LoadBalancer, targetVIP, backendIP string) bool {
554+
for otherVIP, otherBackends := range lb.Vips {
555+
if otherVIP == targetVIP {
556+
continue
557+
}
558+
559+
for otherBackend := range strings.SplitSeq(otherBackends, ",") {
560+
if otherBackendIP, _, err := net.SplitHostPort(otherBackend); err == nil && otherBackendIP == backendIP {
561+
klog.V(5).Infof("Backend IP %s is still used by VIP %s", backendIP, otherVIP)
562+
return true
563+
}
564+
}
565+
}
566+
567+
klog.V(5).Infof("Backend IP %s is no longer used by any other VIP", backendIP)
568+
return false
569+
}
570+
571+
// deleteUnusedIPPortMappings performs the actual deletion of unused IP port mappings
572+
func (c *OVNNbClient) deleteUnusedIPPortMappings(lbName, vipEndpoint string, unusedBackendIPs map[string]string) error {
573+
if len(unusedBackendIPs) == 0 {
574+
klog.Infof("no unused backend IPs to delete for VIP %s in load balancer %s", vipEndpoint, lbName)
575+
return nil
576+
}
577+
578+
ops, err := c.LoadBalancerOp(
509579
lbName,
510580
func(lb *ovnnb.LoadBalancer) []model.Mutation {
511581
return []model.Mutation{
512582
{
513583
Field: &lb.IPPortMappings,
514-
Value: mappings,
584+
Value: unusedBackendIPs,
515585
Mutator: ovsdb.MutateOperationDelete,
516586
},
517587
}
518588
},
519-
); err != nil {
589+
)
590+
if err != nil {
520591
klog.Error(err)
521-
return fmt.Errorf("failed to generate operations when deleting ip port mapping %s from load balancers %s: %w", vipEndpoint, lbName, err)
592+
return fmt.Errorf("failed to generate operations when deleting IP port mappings for VIP %s from load balancer %s: %w", vipEndpoint, lbName, err)
522593
}
594+
523595
if len(ops) == 0 {
524596
return nil
525597
}
598+
526599
if err = c.Transact("lb-del", ops); err != nil {
527-
return fmt.Errorf("failed to delete ip port mappings %s from load balancer %s: %w", vipEndpoint, lbName, err)
600+
return fmt.Errorf("failed to delete IP port mappings for VIP %s from load balancer %s: %w", vipEndpoint, lbName, err)
528601
}
602+
603+
klog.Infof("successfully deleted %d unused backend IPs for VIP %s from load balancer %s",
604+
len(unusedBackendIPs), vipEndpoint, lbName)
529605
return nil
530606
}
531607

608+
// getMapKeys returns the keys of a map as a slice
609+
func getMapKeys(m map[string]bool) []string {
610+
keys := make([]string, 0, len(m))
611+
for k := range m {
612+
keys = append(keys, k)
613+
}
614+
return keys
615+
}
616+
532617
// LoadBalancerUpdateIPPortMapping update load balancer ip port mapping
533618
func (c *OVNNbClient) LoadBalancerUpdateIPPortMapping(lbName, vipEndpoint string, ipPortMappings map[string]string) error {
534619
ops, err := c.LoadBalancerOp(

pkg/ovs/ovn-nb-load_balancer_test.go

Lines changed: 54 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -781,7 +781,7 @@ func (suite *OvnClientTestSuite) testLoadBalancerDeleteIPPortMapping() {
781781
require.Contains(t, lb.IPPortMappings, backend)
782782
}
783783

784-
err = nbClient.LoadBalancerDeleteIPPortMapping(lbName, vip)
784+
err = nbClient.LoadBalancerDeleteIPPortMapping(lbName, vhost)
785785
require.NoError(t, err)
786786

787787
lb, err = nbClient.GetLoadBalancer(lbName, false)
@@ -807,10 +807,29 @@ func (suite *OvnClientTestSuite) testLoadBalancerDeleteIPPortMapping() {
807807
t.Run("delete ip port mappings from load balancer repeatedly",
808808
func(t *testing.T) {
809809
for vip, backends := range vips {
810-
list := strings.Split(backends, ",")
810+
var (
811+
list []string
812+
vhost, host string
813+
)
814+
list = strings.Split(backends, ",")
811815
mappings = make(map[string]string)
812816

813-
err = nbClient.LoadBalancerDeleteIPPortMapping(lbName, vip)
817+
for _, backend := range list {
818+
host, _, err = net.SplitHostPort(backend)
819+
require.NoError(t, err)
820+
821+
mappings[host] = host
822+
}
823+
824+
vhost, _, err = net.SplitHostPort(vip)
825+
require.NoError(t, err)
826+
err = nbClient.LoadBalancerAddVip(lbName, vhost, list...)
827+
require.NoError(t, err)
828+
829+
err = nbClient.LoadBalancerAddIPPortMapping(lbName, vhost, mappings)
830+
require.NoError(t, err)
831+
832+
err = nbClient.LoadBalancerDeleteIPPortMapping(lbName, vhost)
814833
require.NoError(t, err)
815834

816835
lb, err = nbClient.GetLoadBalancer(lbName, false)
@@ -827,16 +846,45 @@ func (suite *OvnClientTestSuite) testLoadBalancerDeleteIPPortMapping() {
827846
)
828847

829848
vips = map[string]string{
830-
"[fd00:10:96::e86f]:8080": "",
849+
"[fd00:10:96::e86f]:8080": "[fc00::af4:a]:8080,[fc00::af4:b]:8080,[fc00::af4:c]:8080",
831850
}
832851
t.Run("delete ip port mappings from load balancer repeatedly",
833852
func(t *testing.T) {
834-
for vip := range vips {
835-
err = nbClient.LoadBalancerDeleteIPPortMapping(lbName, vip)
853+
for vip, backends := range vips {
854+
var (
855+
list []string
856+
vhost, host string
857+
)
858+
list = strings.Split(backends, ",")
859+
mappings = make(map[string]string)
860+
861+
for _, backend := range list {
862+
host, _, err = net.SplitHostPort(backend)
863+
require.NoError(t, err)
864+
865+
mappings[host] = host
866+
}
867+
868+
vhost, _, err = net.SplitHostPort(vip)
869+
require.NoError(t, err)
870+
err = nbClient.LoadBalancerAddVip(lbName, vhost, list...)
871+
require.NoError(t, err)
872+
873+
err = nbClient.LoadBalancerAddIPPortMapping(lbName, vhost, mappings)
874+
require.NoError(t, err)
875+
876+
err = nbClient.LoadBalancerDeleteIPPortMapping(lbName, vhost)
836877
require.NoError(t, err)
837878

838879
lb, err = nbClient.GetLoadBalancer(lbName, false)
839880
require.NoError(t, err)
881+
882+
for _, backend := range list {
883+
backend, _, err = net.SplitHostPort(backend)
884+
require.NoError(t, err)
885+
886+
require.NotContains(t, lb.IPPortMappings, backend)
887+
}
840888
}
841889
},
842890
)

test/e2e/metallb/e2e_test.go

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ var _ = framework.Describe("[group:metallb]", func() {
258258
if f.HasIPv4() {
259259
underlayCidr = append(underlayCidr, cidrV4)
260260
gateway = append(gateway, gatewayV4)
261-
for index := range 5 {
261+
for index := range 10 {
262262
startIP := strings.Split(cidrV4, "/")[0]
263263
ip, _ := ipam.NewIP(startIP)
264264
metallbVIPv4s = append(metallbVIPv4s, ip.Add(100+int64(index)).String())
@@ -268,7 +268,7 @@ var _ = framework.Describe("[group:metallb]", func() {
268268
if f.HasIPv6() {
269269
underlayCidr = append(underlayCidr, cidrV6)
270270
gateway = append(gateway, gatewayV6)
271-
for index := range 5 {
271+
for index := range 10 {
272272
startIP := strings.Split(cidrV6, "/")[0]
273273
ip, _ := ipam.NewIP(startIP)
274274
metallbVIPv6s = append(metallbVIPv6s, ip.Add(100+int64(index)).String())
@@ -338,7 +338,7 @@ var _ = framework.Describe("[group:metallb]", func() {
338338
deploy.Spec.Template.Spec.Containers[0].Args = args
339339
_ = deployClient.CreateSync(deploy)
340340

341-
ginkgo.By("Creating a service for the deployment")
341+
ginkgo.By("Creating the first service for the deployment")
342342
ports := []corev1.ServicePort{
343343
{
344344
Name: "http",
@@ -351,13 +351,30 @@ var _ = framework.Describe("[group:metallb]", func() {
351351
service.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal
352352
_ = serviceClient.CreateSync(service, func(s *corev1.Service) (bool, error) {
353353
return len(s.Status.LoadBalancer.Ingress) != 0, nil
354-
}, "lb service ip is not empty")
354+
}, "first lb service ip is not empty")
355355

356-
ginkgo.By("Checking the service is reachable")
356+
ginkgo.By("Creating the second service for the same deployment")
357+
serviceName2 := "service2-" + framework.RandomSuffix()
358+
service2 := framework.MakeService(serviceName2, corev1.ServiceTypeLoadBalancer, nil, podLabels, ports, "")
359+
service2.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal
360+
_ = serviceClient.CreateSync(service2, func(s *corev1.Service) (bool, error) {
361+
return len(s.Status.LoadBalancer.Ingress) != 0, nil
362+
}, "second lb service ip is not empty")
363+
364+
ginkgo.By("Checking both services are reachable")
357365
service = f.ServiceClient().Get(serviceName)
366+
service2 = f.ServiceClient().Get(serviceName2)
358367
lbsvcIP := service.Status.LoadBalancer.Ingress[0].IP
368+
lbsvcIP2 := service2.Status.LoadBalancer.Ingress[0].IP
359369

360370
checkReachable(f, containerID, clientip, lbsvcIP, "80", clusterName, true)
371+
checkReachable(f, containerID, clientip, lbsvcIP2, "80", clusterName, true)
372+
373+
ginkgo.By("Deleting the first service")
374+
serviceClient.DeleteSync(serviceName)
375+
376+
ginkgo.By("Checking the second service is still reachable after first service deletion")
377+
checkReachable(f, containerID, clientip, lbsvcIP2, "80", clusterName, true)
361378
})
362379
})
363380

0 commit comments

Comments
 (0)