fix(loadbalancer): clean up NFQLB targets before unlinking shm

internal/controller/loadbalancer/controller.go

@@ -18,6 +18,7 @@ package loadbalancer
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 
@@ -192,22 +193,38 @@ func (c *Controller) cleanupDistributionGroup(ctx context.Context, distGroupName
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
-	// Cleanup NFQLB service
+	var errs []error
+
+	// Cleanup NFQLB service. Only drop our own tracking (c.instances/targets/flows)
+	// once DeleteInstance actually succeeds — if it fails, the NFQLB layer keeps the
+	// instance (and its fwmark offset) reserved for retry, so forgetting it here would
+	// mean nothing ever calls DeleteInstance again for this DG (reconcileNFQLBInstance
+	// only acts when the DG is absent from c.instances, and a deleted DG never
+	// reappears to re-trigger creation).
 	if _, exists := c.instances[distGroupName]; exists {
 		logr.Info("Deleting NFQLB service", "distGroup", distGroupName)
 		if err := c.NFQLB.DeleteInstance(ctx, distGroupName); err != nil {
-			logr.Error(err, "Failed to delete NFQLB service", "distGroup", distGroupName)
+			logr.Error(err, "Failed to delete NFQLB service, will retry", "distGroup", distGroupName)
+			errs = append(errs, fmt.Errorf("failed to delete NFQLB service for %q: %w", distGroupName, err))
+		} else {
+			delete(c.instances, distGroupName)
+			delete(c.targets, distGroupName)
+			delete(c.flows, distGroupName)
 		}
-		delete(c.instances, distGroupName)
-		delete(c.targets, distGroupName)
-		delete(c.flows, distGroupName)
 	}
 
 	// Note: nftables manager is shared, not cleaned up per-DG
 
-	// Remove readiness file
+	// Remove readiness file. Attempted independently of the NFQLB cleanup above —
+	// a stuck DeleteInstance must not keep this DG's VIP advertised via BGP
+	// (readiness.Manager.IsReady() is keyed off file presence) while cleanup retries.
 	if err := c.Readiness.Remove(distGroupName); err != nil {
 		logr.Error(err, "Failed to remove readiness file", "distGroup", distGroupName)
+		errs = append(errs, fmt.Errorf("failed to remove readiness file for %q: %w", distGroupName, err))
+	}
+
+	if len(errs) > 0 {
+		return ctrl.Result{}, errors.Join(errs...)
 	}
 
 	return ctrl.Result{}, nil

internal/controller/loadbalancer/controller_test.go

@@ -92,6 +92,11 @@ func newTestLBEPS(dg *meridio2v1alpha1.DistributionGroup, endpoints []meridio2v1
 type mockNFQLB struct {
 	instances  map[string]*mockNFQLBInstance
 	fwmarkBase int
+	// deleteInstanceErr, when set, is returned by DeleteInstance instead of
+	// succeeding, without removing the instance from m.instances — mirroring
+	// the real NFQueueLoadBalancer, which keeps a failed deletion's instance
+	// (and its fwmark offset) reserved so the next reconcile retries it.
+	deleteInstanceErr error
 }
 
 func newMockNFQLB() *mockNFQLB {
@@ -112,6 +117,9 @@ func (m *mockNFQLB) AddInstance(_ context.Context, name string, _ ...nfqlb.Insta
 }
 
 func (m *mockNFQLB) DeleteInstance(_ context.Context, name string) error {
+	if m.deleteInstanceErr != nil {
+		return m.deleteInstanceErr
+	}
 	delete(m.instances, name)
 	return nil
 }
@@ -470,6 +478,42 @@ var _ = Describe("LoadBalancer Controller", func() {
 			Expect(controller.reconcileNFQLBInstance(ctx, dg4)).To(Succeed())
 			Expect(controller.instances).To(HaveKey("dg-4"))
 		})
+
+		It("keeps the instance tracked for retry when DeleteInstance fails, "+
+			"but still removes the readiness file independently", func() {
+			controller.Readiness = readiness.NewManager(GinkgoT().TempDir())
+
+			dg := &meridio2v1alpha1.DistributionGroup{
+				ObjectMeta: metav1.ObjectMeta{Name: "dg-1", Namespace: namespace},
+			}
+			Expect(controller.reconcileNFQLBInstance(ctx, dg)).To(Succeed())
+			Expect(controller.Readiness.Set("dg-1")).To(Succeed())
+			Expect(controller.Readiness.IsReady()).To(BeTrue())
+
+			mockNfqlb.deleteInstanceErr = errors.New("shm busy")
+
+			_, err := controller.cleanupDistributionGroup(ctx, "dg-1")
+			Expect(err).To(HaveOccurred(),
+				"a failed NFQLB cleanup must be returned so the reconcile is requeued, "+
+					"not logged and dropped")
+
+			Expect(controller.instances).To(HaveKey("dg-1"),
+				"a failed DeleteInstance must keep the DG tracked so the next "+
+					"reconcile retries cleanup instead of forgetting it — nothing else "+
+					"would ever call DeleteInstance for a DG that is already deleted")
+
+			Expect(controller.Readiness.IsReady()).To(BeFalse(),
+				"the readiness file must be removed even though NFQLB cleanup is "+
+					"still stuck retrying — otherwise this DG's VIP keeps being "+
+					"advertised via BGP for a DG that no longer exists")
+
+			// Once the underlying delete succeeds, retrying cleanup must finish
+			// the job and drop tracking.
+			mockNfqlb.deleteInstanceErr = nil
+			_, err = controller.cleanupDistributionGroup(ctx, "dg-1")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(controller.instances).ToNot(HaveKey("dg-1"))
+		})
 	})
 
 	Describe("reconcileTargets", func() {

internal/nfqlb/nfqlb.go

@@ -321,8 +321,20 @@ func (nfqlb *NFQueueLoadBalancer) AddInstance(ctx context.Context,
 }
 
 // DeleteInstance deletes a nfqlb instance and all related configuration (targets and flows).
+// Cleanup order is: targets/routes, then flows, then the shm segment unlink last.
+// The instance is removed from nfqlb.instances (releasing its fwmark offset for reuse)
+// only after every step succeeds. If any step fails, the instance remains tracked (so
+// its offset cannot be handed to a different DG while stale routes for it may still
+// exist) and the caller is expected to retry DeleteInstance on the next reconcile.
+//
+// The ordering is load-bearing, not cosmetic: `nfqlb deactivate` has to mmap the shm
+// segment, so unlinking it first makes every deactivate call fail with
+// "FAILED mapSharedData". deleteTargetNoLock returns early on that error, before its
+// policy-route cleanup, which leaked an ip rule and routing table per target on every
+// deletion of a DistributionGroup that had active targets.
 func (nfqlb *NFQueueLoadBalancer) DeleteInstance(ctx context.Context, name string) error {
 	nfqlb.mu.Lock()
+	defer nfqlb.mu.Unlock()
 
 	nfqlbInstance, exists := nfqlb.instances[name]
 	if !exists {
@@ -331,30 +343,9 @@ func (nfqlb *NFQueueLoadBalancer) DeleteInstance(ctx context.Context, name strin
 
 	ctrl.LoggerFrom(ctx).Info("nfqlb: delete instance", "instance", name)
 
-	delete(nfqlb.instances, name)
-
-	// Safe to release nfqlb.mu before locking instance.mu: the instance was
-	// already removed from the map above, so heal() (which iterates
-	// nfqlb.instances under nfqlb.mu) will no longer see it.
-	nfqlb.mu.Unlock()
-
 	nfqlbInstance.mu.Lock()
 	defer nfqlbInstance.mu.Unlock()
 
-	// unlink the shared mem file
-	//nolint:gosec
-	cmd := exec.CommandContext(
-		ctx,
-		nfqlb.nfqlbPath,
-		"delete",
-		fmt.Sprintf("--shm=%s", name),
-	)
-
-	stdoutStderr, err := cmd.CombinedOutput()
-	if err != nil {
-		return fmt.Errorf("failed deleting nfqlb instance ; %w; %s", err, stdoutStderr)
-	}
-
 	var errs []error
 
 	for targetIdentifier := range nfqlbInstance.targets {
@@ -366,20 +357,43 @@ func (nfqlb *NFQueueLoadBalancer) DeleteInstance(ctx context.Context, name strin
 	flows, err := nfqlb.flowList(ctx)
 	if err != nil {
 		errs = append(errs, fmt.Errorf("list flows: %w", err))
-		return errors.Join(errs...)
-	}
-
-	for _, flow := range flows {
-		if flow.ServerName == name {
-			if err := nfqlbInstance.DeleteFlow(ctx, flow); err != nil {
-				errs = append(errs, fmt.Errorf("delete flow %s: %w", flow.GetName(), err))
+	} else {
+		for _, flow := range flows {
+			if flow.ServerName == name {
+				if err := nfqlbInstance.DeleteFlow(ctx, flow); err != nil {
+					errs = append(errs, fmt.Errorf("delete flow %s: %w", flow.GetName(), err))
+				}
 			}
 		}
 	}
 
+	if len(errs) > 0 {
+		// Targets/flows are not fully cleaned up: keep the instance (and its
+		// offset) reserved rather than unlinking shm and losing track of it.
+		return fmt.Errorf("failed cleaning up nfqlb instance %q, shm not deleted: %w", name, errors.Join(errs...))
+	}
+
+	// unlink the shared mem file
+	//nolint:gosec
+	cmd := exec.CommandContext(
+		ctx,
+		nfqlb.nfqlbPath,
+		"delete",
+		fmt.Sprintf("--shm=%s", name),
+	)
+
+	stdoutStderr, err := cmd.CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("failed deleting nfqlb instance ; %w; %s", err, stdoutStderr)
+	}
+
+	// Only release the offset for reuse once targets, flows, and the shm
+	// segment are all confirmed gone.
+	delete(nfqlb.instances, name)
+
 	ctrl.LoggerFrom(ctx).Info("nfqlb: instance deleted", "instance", name)
 
-	return errors.Join(errs...)
+	return nil
 }
 
 // AddFlow adds/updates a Flow selecting the associated nfqlb instance.

internal/nfqlb/nfqlb_test.go

@@ -17,6 +17,12 @@ limitations under the License.
 package nfqlb
 
 import (
+	"context"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
@@ -204,3 +210,132 @@ var _ = Describe("New", func() {
 		Expect(err.Error()).To(ContainSubstring("invalid queue"))
 	})
 })
+
+// DeleteInstance must clean up targets (and their policy routes) and flows
+// before unlinking the shm segment, and must only release the instance's
+// fwmark offset once all of that succeeded.
+//
+// `nfqlb deactivate` has to mmap the shm segment, so unlinking it first makes
+// every deactivate call fail with "FAILED mapSharedData". deleteTargetNoLock
+// returns early on that error, before its policy-route cleanup — which leaked
+// an ip rule and routing table per target on every deletion of a
+// DistributionGroup that had active targets. Releasing the offset at the same
+// time compounds it: the next DistributionGroup can be allocated the same
+// fwmark range and inherit live routes pointing at the previous DG's Pod IPs.
+var _ = Describe("DeleteInstance", func() {
+	It("cleans up policy routes and keeps the offset reserved when the shm unlink fails", func() {
+		ctx := context.Background()
+
+		lb, err := New(WithNFQLBPath("/bin/true"))
+		Expect(err).ToNot(HaveOccurred())
+		lb.running.Store(true)
+
+		inst, err := lb.AddInstance(ctx, "dg-1", WithMaxTargets(4))
+		Expect(err).ToNot(HaveOccurred())
+
+		var routesDeleted []int
+		inst.routeCreate = func(int, string) error { return nil }
+		inst.routeDelete = func(fwmark int, _ string) error {
+			routesDeleted = append(routesDeleted, fwmark)
+			return nil
+		}
+		inst.execCmd = func(context.Context, ...string) ([]byte, error) { return nil, nil }
+		Expect(inst.AddTarget(ctx, []string{"10.0.0.1"}, 0)).To(Succeed())
+
+		firstOffset := inst.offset
+
+		// `nfqlb delete --shm=` fails (shm busy, EACCES, binary error, ...)
+		lb.nfqlbPath = "/bin/false"
+		err = lb.DeleteInstance(ctx, "dg-1")
+		Expect(err).To(HaveOccurred())
+
+		Expect(routesDeleted).ToNot(BeEmpty(),
+			"policy routes/rules must be cleaned up even when the shm unlink fails")
+
+		lb.nfqlbPath = "/bin/true"
+		inst2, addErr := lb.AddInstance(ctx, "dg-2", WithMaxTargets(4))
+		Expect(addErr).ToNot(HaveOccurred())
+		Expect(inst2.offset).ToNot(Equal(firstOffset),
+			"a failed deletion must not free its offset for reuse while "+
+				"leftover routes for it still exist")
+	})
+
+	It("unlinks the shm segment only after targets and flows are cleaned up", func() {
+		ctx := context.Background()
+
+		// Record the real call order. The shm unlink and flowList both shell out
+		// via exec.CommandContext (neither is routed through the injectable
+		// execCmd), so the only way to observe the unlink's position relative to
+		// the deactivate is to have the fake binary log its own invocations.
+		// execCmd is deliberately NOT injected here so `deactivate` also goes
+		// through the script and lands in the same ordered log.
+		dir := GinkgoT().TempDir()
+		callLog := filepath.Join(dir, "calls")
+		scriptPath := filepath.Join(dir, "nfqlb-stub.sh")
+		script := fmt.Sprintf("#!/bin/sh\necho \"$1\" >> %s\necho '[]'\n", callLog)
+		Expect(os.WriteFile(scriptPath, []byte(script), 0o755)).To(Succeed())
+
+		readCalls := func() []string {
+			data, err := os.ReadFile(callLog)
+			if err != nil {
+				return nil
+			}
+			var out []string
+			for l := range strings.SplitSeq(strings.TrimSpace(string(data)), "\n") {
+				if l != "" {
+					out = append(out, l)
+				}
+			}
+			return out
+		}
+
+		lb, err := New(WithNFQLBPath(scriptPath))
+		Expect(err).ToNot(HaveOccurred())
+		lb.running.Store(true)
+
+		inst, err := lb.AddInstance(ctx, "dg-order", WithMaxTargets(4))
+		Expect(err).ToNot(HaveOccurred())
+
+		inst.routeCreate = func(int, string) error { return nil }
+		inst.routeDelete = func(int, string) error {
+			f, e := os.OpenFile(callLog, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600)
+			Expect(e).ToNot(HaveOccurred())
+			defer f.Close() //nolint:errcheck
+			_, e = f.WriteString("route-delete\n")
+			Expect(e).ToNot(HaveOccurred())
+			return nil
+		}
+		Expect(inst.AddTarget(ctx, []string{"10.0.0.1"}, 0)).To(Succeed())
+
+		// Drop everything logged during setup (init, activate).
+		Expect(os.WriteFile(callLog, nil, 0o600)).To(Succeed())
+
+		Expect(lb.DeleteInstance(ctx, "dg-order")).To(Succeed())
+
+		calls := readCalls()
+		Expect(calls).To(ContainElement("deactivate"))
+		Expect(calls).To(ContainElement("delete"))
+		Expect(calls).To(ContainElement("route-delete"))
+
+		// The unlink ("delete") must come last. On the previous ordering it ran
+		// first, which made `nfqlb deactivate` fail with "FAILED mapSharedData"
+		// and caused deleteTargetNoLock to return before its route cleanup.
+		Expect(indexOf(calls, "delete")).To(BeNumerically(">", indexOf(calls, "deactivate")),
+			"the shm segment must be unlinked only after its targets are deactivated; got %v", calls)
+		Expect(indexOf(calls, "delete")).To(BeNumerically(">", indexOf(calls, "route-delete")),
+			"the shm segment must be unlinked only after policy routes are removed; got %v", calls)
+
+		Expect(lb.instances).ToNot(HaveKey("dg-order"),
+			"a fully successful cleanup must release the instance and its offset")
+	})
+})
+
+// indexOf returns the position of want in calls, or -1.
+func indexOf(calls []string, want string) int {
+	for i, c := range calls {
+		if c == want {
+			return i
+		}
+	}
+	return -1
+}