diff --git a/internal/controller/loadbalancer/controller.go b/internal/controller/loadbalancer/controller.go index 9d56cb18..c93b8910 100644 --- a/internal/controller/loadbalancer/controller.go +++ b/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 diff --git a/internal/controller/loadbalancer/controller_test.go b/internal/controller/loadbalancer/controller_test.go index 1b8479e1..a7349f8f 100644 --- a/internal/controller/loadbalancer/controller_test.go +++ b/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() { diff --git a/internal/nfqlb/nfqlb.go b/internal/nfqlb/nfqlb.go index 246750ef..2bffb8a4 100644 --- a/internal/nfqlb/nfqlb.go +++ b/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. diff --git a/internal/nfqlb/nfqlb_test.go b/internal/nfqlb/nfqlb_test.go index 01c33174..04b16796 100644 --- a/internal/nfqlb/nfqlb_test.go +++ b/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 +}