tests: bgp_community_change_update: use receivedPrefixDup counter

[enkechen-panw]

Replace the grep-based duplicate-update detection with the receivedPrefixDup counter from "show bgp neighbors json". This avoids reliance on log file content.

Also explicitly enable bgp suppress-duplicates in x1/bgpd.conf, and disable debug bgp updates in both c1 and x1 configs.

[greptile-apps]

Greptile Summary

This PR replaces the brittle grep-on-log approach for detecting duplicate BGP UPDATE messages with the receivedPrefixDup JSON counter from show bgp neighbors. It also explicitly enables bgp suppress-duplicates in x1/bgpd.conf and comments out verbose debug logging in both c1 and x1 configs. The change is a clear robustness improvement — avoiding log-file path assumptions and using a structured, observable metric instead.

Confidence Score: 5/5

Safe to merge; all findings are P2 robustness/style suggestions with no blocking issues.

No P0 or P1 issues found. The two P2 comments cover an unguarded dict key access and a bare sleep that could be fragile on slow CI, but neither causes incorrect test logic or data corruption.

test_bgp_community_change_update.py — minor robustness improvements suggested for _bgp_get_dup_count and the bare sleep.

Important Files Changed

Filename	Overview
tests/topotests/bgp_community_change_update/test_bgp_community_change_update.py	Replaces grep-on-log duplicate detection with receivedPrefixDup JSON counter; two P2 robustness concerns: unguarded key access in _bgp_get_dup_count and a bare sleep(2.5) for the suppress check.
tests/topotests/bgp_community_change_update/x1/bgpd.conf	Debug logging commented out and bgp suppress-duplicates added explicitly — aligns config with what the test was already assuming at runtime.
tests/topotests/bgp_community_change_update/c1/bgpd.conf	Only change is commenting out debug bgp updates; no functional impact.

Sequence Diagram

sequenceDiagram
    participant Test
    participant c1
    participant x1
    participant y1

    Test->>c1: show bgp neighbors 10.0.1.2 json
    c1-->>Test: dup_before = receivedPrefixDup (baseline)

    Test->>y1: ip link set dev y1-eth1 down
    Test->>y1: show ip bgp nei 10.0.3.2 json (wait Active)
    Test->>Test: sleep(2.5)
    Test->>c1: show bgp neighbors 10.0.1.2 json
    c1-->>Test: assert receivedPrefixDup == dup_before (suppress active)

    Test->>x1: no bgp suppress-duplicates
    Test->>c1: show bgp neighbors 10.0.1.2 json
    c1-->>Test: dup_before = receivedPrefixDup (new baseline)

    Test->>y1: ip link set dev y1-eth1 up
    Test->>y1: show ip bgp nei 10.0.3.2 json (wait Established)

    loop run_and_expect (count=10, wait=1.5)
        Test->>c1: show bgp neighbors 10.0.1.2 json
        c1-->>Test: receivedPrefixDup > dup_before?
    end
    Test->>Test: assert result is True (duplicate seen)

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
tests/topotests/bgp_community_change_update/test_bgp_community_change_update.py:139-143
**`receivedPrefixDup` key access is unguarded**

`_bgp_get_dup_count()` performs a raw dict traversal into `receivedPrefixDup` with no fallback. If that key is absent (e.g., on an older FRR build where the counter is omitted when zero, or during a transient neighbor state), both the bare call at line 145 and the one at line 172 will raise an unhandled `KeyError`, producing a cryptic traceback instead of a useful assertion message. The call inside `_bgp_check_for_duplicate_updates` at line 195 would also surface as a `run_and_expect` exception rather than a clean test failure.

```python
    def _bgp_get_dup_count():
        output = json.loads(
            tgen.gears["c1"].vtysh_cmd("show bgp neighbors 10.0.1.2 json")
        )
        return output.get("10.0.1.2", {}).get("addressFamilyInfo", {}).get(
            "ipv4Unicast", {}
        ).get("receivedPrefixDup", 0)
```

### Issue 2 of 2
tests/topotests/bgp_community_change_update/test_bgp_community_change_update.py:160
**Bare `sleep` may be fragile on slow CI**

The unconditional `sleep(2.5)` replaces a loop that verified the absence of a log line; now it just hopes 2.5 s is enough for any in-flight duplicate UPDATE to arrive and be counted before reading the counter. On a heavily loaded CI host the BGP UPDATE processing could still be queued. Consider wrapping the counter check in its own short `run_and_expect` with an inverted condition (counter still equals `dup_before`), mirroring the pattern used for the positive check below.

_{Reviews (2): Last reviewed commit: "tests: bgp_community_change_update: use ..." | Re-trigger Greptile}

[enkechen-panw]

@greptileai review

[ton31337]

Can we drop this at all since we use run_and_expect()?

[enkechen-panw]

We can not drop the sleep() there as it serves a different purpose from
the run_and_expect above it. The run_and_expect only confirms that y1
detected the link going down (its BGP session to y2 went Active). It says
nothing about whether c1 has received and processed x1's resulting UPDATE.

The sleep() gives time for any potential duplicate UPDATE from x1 to
actually arrive at c1 before we assert the counter hasn't moved.