test(e2e): make the LoadBalancer announcer detection failover-robust

The dataplane diagnostic's announcer-node detection reported the wrong
node in two failover situations and could match the wrong IP:

  - A node that announced an IP and then withdrew it (with no
    re-announce) was still reported as the announcer, because a withdraw
    did not retract a prior announce.
  - Speaker logs are concatenated per-pod, not globally time-sorted, so
    a stale former owner's announce could outrank the true current
    owner purely by its position in the input.
  - The IP was matched as a substring, so a query for 192.0.2.5 also
    matched a 192.0.2.50 line.

Reimplement the detection to report a node only when that node's LAST
own announce/withdraw event for the exact IP is an announce, which is
robust to both concat order and L2 failover, and anchor the IP match to
a whole IP-literal token instead of a substring.

Also fix the heavy-capture budget: the MAX_LBS cap counted every
enumerated LB, so a broken LB enumerated after a cap's worth of
reachable LBs was never captured. Count only LBs actually captured, via
a small pure budget helper, so reachable/skipped LBs do not consume the
budget. Document that this cap bounds work, not wall-clock -- at two or
more unreachable LBs the real wall-clock bound is the outer backstop at
the call site.

Add bats coverage pinning the fixed announcer behavior (withdraw,
concat-order, exact-IP) and the captured-budget accounting.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>

hack/capture-dataplane.bats

@@ -108,6 +108,54 @@ E2E_CAPTURE_DATAPLANE_LIB=1
   [ -z "$out" ]
 }
 
+@test "lb_announcer_node excludes a node whose last own event is a withdraw" {
+  # Case A: node-a announces the IP then withdraws it and never re-announces.
+  # Its last own IP-event is the withdraw, so it is NOT the current announcer and
+  # nothing must be reported. The pre-polish logic kept node-a (a withdraw did
+  # not retract a prior announce), so this pins the failover fix.
+  logs="$(printf '%s\t%s\n' \
+    'node-a' '{"event":"serviceAnnounced","ips":["192.0.2.50"],"node":"node-a"}' \
+    'node-a' '{"event":"serviceWithdrawn","ips":["192.0.2.50"],"node":"node-a"}')"
+
+  out="$(printf '%s\n' "$logs" | lb_announcer_node '192.0.2.50')"
+
+  [ -z "$out" ]
+}
+
+@test "lb_announcer_node ignores a stale owner that appears later in concat order" {
+  # Case B: node-c is the true current owner (announce, never withdrawn). Later
+  # in the input -- speaker logs are concatenated per-pod, NOT globally time-
+  # sorted -- a stale node-a announce+withdraw block appears. node-a's last own
+  # event is a withdraw, so the announcer is node-c, NOT the later-in-input
+  # node-a. The pre-polish logic returned node-a (most recent announce by concat
+  # order), so this pins robustness to concat order.
+  logs="$(printf '%s\t%s\n' \
+    'node-c' '{"event":"serviceAnnounced","ips":["192.0.2.50"],"node":"node-c"}' \
+    'node-a' '{"event":"serviceAnnounced","ips":["192.0.2.50"],"node":"node-a"}' \
+    'node-a' '{"event":"serviceWithdrawn","ips":["192.0.2.50"],"node":"node-a"}')"
+
+  out="$(printf '%s\n' "$logs" | lb_announcer_node '192.0.2.50')"
+
+  [ "$out" = "node-c" ]
+}
+
+@test "lb_announcer_node matches the IP as a whole token not a substring" {
+  # Case C: querying 192.0.2.5 must NOT match a 192.0.2.50 announce (the
+  # pre-polish index()/substring match returned node-d here). The exact 192.0.2.5
+  # announce on node-e is matched and wins.
+  logs="$(printf '%s\t%s\n' \
+    'node-d' '{"event":"serviceAnnounced","ips":["192.0.2.50"],"node":"node-d"}' \
+    'node-e' '{"event":"serviceAnnounced","ips":["192.0.2.5"],"node":"node-e"}')"
+
+  no_substr="$(printf '%s\n' "$logs" | lb_announcer_node '192.0.2.5')"
+  no_match="$(printf '%s\t%s\n' \
+    'node-d' '{"event":"serviceAnnounced","ips":["192.0.2.50"],"node":"node-d"}' \
+    | lb_announcer_node '192.0.2.5')"
+
+  [ "$no_substr" = "node-e" ]
+  [ -z "$no_match" ]
+}
+
 @test "lb_capture_decision returns capture when every probe failed" {
   [ "$(printf 'fail\nfail\nfail\n' | lb_capture_decision)" = "capture" ]
   [ "$(printf 'fail\n' | lb_capture_decision)" = "capture" ]
@@ -121,3 +169,56 @@ E2E_CAPTURE_DATAPLANE_LIB=1
 @test "lb_capture_decision returns skip when no probe ran at all" {
   [ "$(printf '' | lb_capture_decision)" = "skip" ]
 }
+
+@test "lb_budget_ok yes below the cap and no once it is reached" {
+  [ "$(lb_budget_ok 0 6)" = "yes" ]
+  [ "$(lb_budget_ok 5 6)" = "yes" ]
+  [ "$(lb_budget_ok 6 6)" = "no" ]
+  [ "$(lb_budget_ok 7 6)" = "no" ]
+}
+
+@test "capture budget counts captured LBs so a late broken LB is still captured" {
+  # The MAX_LBS cap must bound LBs actually CAPTURED, not LBs enumerated: reachable
+  # (skipped) LBs must not consume the budget, so a broken LB enumerated after many
+  # reachable ones is still characterised. This mirrors the per-LB loop in
+  # capture_lb_datapath, which only consults lb_budget_ok / increments the counter
+  # on the capture branch -- here driven by a fixed decision sequence so no cluster
+  # is needed. With the pre-polish "increment per enumerated LB" logic and max=2,
+  # the loop would have broken at the 3rd LB and never reached the broken one.
+  max=2
+  captured=0
+  last=""
+  for decision in skip skip skip skip skip capture; do
+    if [ "$decision" != "capture" ]; then
+      last="skip"
+      continue
+    fi
+    if [ "$(lb_budget_ok "$captured" "$max")" != "yes" ]; then
+      last="dropped"
+      continue
+    fi
+    captured=$((captured + 1))
+    last="captured"
+  done
+
+  [ "$captured" -eq 1 ]
+  [ "$last" = "captured" ]
+}
+
+@test "capture budget stops after the cap worth of broken LBs" {
+  # Once MAX_LBS broken LBs are captured, further unreachable LBs are not
+  # characterised (left to the outer wall-clock backstop). max=2, three broken.
+  max=2
+  captured=0
+  dropped=0
+  for decision in capture capture capture; do
+    if [ "$(lb_budget_ok "$captured" "$max")" != "yes" ]; then
+      dropped=$((dropped + 1))
+      continue
+    fi
+    captured=$((captured + 1))
+  done
+
+  [ "$captured" -eq 2 ]
+  [ "$dropped" -eq 1 ]
+}

hack/e2e-capture-dataplane.sh

@@ -128,13 +128,44 @@ lb_first_ready_endpoint() {
 
 # lb_announcer_node <lbip>: stdin = MetalLB speaker logs, each line prefixed with
 # the emitting speaker pod's node and a TAB (`<node>\t<logline>`). The speaker is
-# hostNetwork and only the elected L2 owner logs an announce for the IP, so the
-# node of the most recent announce line (announce, not withdraw) is the current
-# announcer. Emits that node, or nothing when no announce for the IP is found.
+# hostNetwork and only the elected L2 owner currently announces the IP. Reports a
+# node ONLY IF that node's LAST own IP-event for this exact IP is an announce: per
+# node we track its most recent announce/withdraw line for the IP, so a node that
+# announced then withdrew (last event = withdraw) is excluded and a node that is
+# still announcing (last event = announce) qualifies. This is robust to the
+# per-pod (not globally time-sorted) concat order of the speaker logs and to L2
+# failover. If more than one node still qualifies (should not happen for an L2
+# announce), the one whose last announce appears latest in the input wins.
+# The IP is matched as a whole token (not a substring), so a query for
+# `192.0.2.5` does NOT match a `192.0.2.50` line. Emits the announcer node, or
+# nothing when no node currently announces the IP.
 lb_announcer_node() {
   awk -F'\t' -v ip="$1" '
-    index($0, ip) && /[Aa]nnounc/ && !/[Ww]ithdraw/ { node = $1 }
-    END { if (node != "") print node }'
+    BEGIN {
+      # Match the IP as a maximal IP-literal token: not flanked by another
+      # IP char (digit / dot / hex / colon), so `192.0.2.5` != `192.0.2.50`.
+      # `[.]` escapes each dot portably (no gsub-replacement backslash games).
+      ipre = ip
+      gsub(/\./, "[.]", ipre)
+      re = "(^|[^0-9A-Fa-f:.])" ipre "([^0-9A-Fa-f:.]|$)"
+    }
+    $0 ~ re {
+      if      ($0 ~ /[Ww]ithdraw/) ev = "withdraw"
+      else if ($0 ~ /[Aa]nnounc/)  ev = "announce"
+      else                         next
+      last_ev[$1]   = ev
+      last_line[$1] = NR
+    }
+    END {
+      best = ""; bestline = -1
+      for (n in last_ev) {
+        if (last_ev[n] == "announce" && last_line[n] > bestline) {
+          bestline = last_line[n]
+          best = n
+        }
+      }
+      if (best != "") print best
+    }'
 }
 
 # lb_capture_decision: stdin = one probe outcome token per line ("ok" / "fail").
@@ -154,6 +185,18 @@ lb_capture_decision() {
     }'
 }
 
+# lb_budget_ok <captured-so-far> <max>: gate for the heavy per-LB capture budget.
+# Emits "yes" while fewer than <max> LBs have been CAPTURED, "no" once the cap is
+# reached. The cap bounds LBs actually captured -- the caller increments only on
+# the capture branch, so reachable/skipped LBs never consume it and a broken LB
+# enumerated after many reachable ones is still characterised. The cap bounds
+# WORK, not wall-clock: at >=2 unreachable LBs (each heavy capture takes tens of
+# seconds) the real wall-clock bound is the outer `timeout -k 30 600` backstop at
+# the call site, not this cap.
+lb_budget_ok() {
+  if [ "$1" -lt "$2" ]; then echo yes; else echo no; fi
+}
+
 # Sourcing guard: hack/capture-dataplane.bats sets E2E_CAPTURE_DATAPLANE_LIB and
 # sources this file purely to reach the helpers above; return before touching $1
 # or running any capture so the unit test never needs a cluster. The script's
@@ -180,8 +223,11 @@ MAX_PODS="${COZY_DATAPLANE_MAX_PODS:-12}"
 METALLB_NS="${COZY_METALLB_NS:-cozy-metallb}"
 SPEAKER_SELECTOR="${COZY_METALLB_SPEAKER_SELECTOR:-app.kubernetes.io/component=speaker}"
 GENEVE_IFACE="${COZY_GENEVE_IFACE:-genev_sys_6081}"
-# Cap failing-LB heavy captures so a cluster with many broken LBs cannot blow the
-# call-site wall-clock backstop; per-command timeouts are the other bound.
+# Cap how many UNREACHABLE LBs get the heavy datapath capture; reachable/skipped
+# LBs never count toward it (see the captured-budget gate in capture_lb_datapath).
+# This bounds WORK, not wall-clock: at >=2 unreachable LBs the real wall-clock
+# bound is the outer `timeout -k 30 600` backstop at the call site (truncate +
+# hard-kill), since each heavy capture itself takes tens of seconds.
 MAX_LBS="${COZY_DATAPLANE_MAX_LBS:-6}"
 
 command -v kubectl >/dev/null 2>&1 || exit 0
@@ -532,7 +578,7 @@ capture_lb_datapath() {
   fi
 
   _lbcount=$(printf '%s\n' "$_lbs" | wc -l | tr -d ' ')
-  log "characterising up to $MAX_LBS of $_lbcount LoadBalancer service(s) -> $OUT/lb-*.txt"
+  log "probing $_lbcount LoadBalancer service(s); heavy-capturing up to $MAX_LBS unreachable one(s) -> $OUT/lb-*.txt"
 
   # MetalLB speaker logs once, prefixed with each speaker's node (hostNetwork, so
   # .spec.nodeName IS the announcing node). Kept as an artifact file that
@@ -550,15 +596,14 @@ capture_lb_datapath() {
     done
   fi
 
-  _i=0
+  # Counts LBs actually CAPTURED (heavy capture performed), not enumerated, so the
+  # MAX_LBS cap only bounds failing LBs -- reachable/skipped ones below never
+  # increment it and a broken LB enumerated after many reachable ones still gets
+  # captured. The gate lives on the capture branch (lb_budget_ok), not here.
+  _captured=0
   printf '%s\n' "$_lbs" | {
     while IFS='|' read -r _ns _name _type _lbip _port _np _etp; do
       [ -n "$_lbip" ] || continue
-      _i=$((_i + 1))
-      if [ "$_i" -gt "$MAX_LBS" ]; then
-        log "reached MAX_LBS=$MAX_LBS cap; $((_lbcount - MAX_LBS)) more LB(s) NOT characterised"
-        break
-      fi
       _lbport="${_port:-0}"
 
       # EndpointSlice backend for this Service (first ready, addressed endpoint).
@@ -604,6 +649,16 @@ capture_lb_datapath() {
         continue
       fi
 
+      # Heavy-capture budget: only failing LBs consume MAX_LBS (reachable ones
+      # skipped above without counting). Stop once the cap's worth of broken LBs
+      # have been captured; remaining ones are left to the outer wall-clock bound.
+      if [ "$(lb_budget_ok "$_captured" "$MAX_LBS")" != "yes" ]; then
+        echo "probe: LB IP unreachable from node $_probenode but MAX_LBS=$MAX_LBS captured cap reached -- NOT characterised" >> "$_of" 2>&1 || true
+        log "reached MAX_LBS=$MAX_LBS captured cap; further unreachable LB(s) NOT characterised"
+        break
+      fi
+      _captured=$((_captured + 1))
+
       log "LB $_ns/$_name ($_lbip) UNREACHABLE -- capturing announcer/endpoint datapath"
       {
         echo