topotests: support ExaBGP 5.x and update the in-tree installs to 5.0.9#22336
topotests: support ExaBGP 5.x and update the in-tree installs to 5.0.9#22336higebu wants to merge 4 commits into
Conversation
Greptile SummaryThis PR adds ExaBGP 5.x support to the topotests framework by auto-detecting the installed version and branching on 4.x vs 5.x behaviour, then updates all in-tree installs (both Dockerfiles and the developer doc) from a pinned 4.x commit to the
Confidence Score: 4/5Safe to merge; all behaviour changes are confined to the topotest harness and have been manually verified against both 4.2.21 and 5.0.9. The version-detection and caching logic is correct, the 5.x CLI branching matches the upstream migration guide, and the /dev/null redirect fix for the daemonize-blocking bug is sound. The only non-trivial note is the switch from an immutable commit SHA to a version tag for the pip install, which is a minor reproducibility trade-off but does not affect runtime correctness. All three Dockerfile / doc locations now pin to the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TopoExaBGP.start] --> B[get_exabgp_cmd]
B --> C{exabgp binary found?}
C -- yes --> D[probe: exabgp --version]
D --> E{regex match?}
E -- yes --> F[cache version, return exacmd]
E -- no --> G[probe: exabgp version]
G --> H{regex match?}
H -- yes --> F
H -- no --> I[try python3 -m exabgp ...]
I --> J{version ok?}
J -- yes --> F
J -- no --> K[return None - assert fails]
C -- no --> I
F --> L[version >= 4.2.11 confirmed]
L --> M[lookup _EXABGP_VERSION_CACHE]
M --> N{is 5.x?}
N -- no --> O[mkfifo /var/run/exabgp.in/.out - exabgp -e env cfg]
N -- yes --> P[skip mkfifo - EXABGP_ENVFILE=... - exabgp server cfg]
O --> Q[run + redirect stdio to /dev/null]
P --> Q
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
docker/ubuntu-ci/Dockerfile:110
**Git tag instead of pinned commit hash**
The previous reference used a full commit SHA (`@0659057837cd...`), which is immutable and guarantees the exact bytes installed. Switching to a tag (`@5.0.9`) means that if the tag is ever force-pushed or recreated on the upstream repo, a fresh `docker build` could silently pull in a different commit. The same change is made in `tests/topotests/Dockerfile` and `doc/developer/topotests.rst`. Consider using the resolved commit SHA for `5.0.9` to keep the same level of pinning.
Reviews (1): Last reviewed commit: "doc: update for exabgp 5" | Re-trigger Greptile |
| python3 -m pip install pyyaml && \ | ||
| python3 -m pip install xmltodict && \ | ||
| python3 -m pip install git+https://github.com/Exa-Networks/exabgp@0659057837cd6c6351579e9f0fa47e9fb7de7311 | ||
| python3 -m pip install git+https://github.com/Exa-Networks/exabgp@5.0.9 |
There was a problem hiding this comment.
Git tag instead of pinned commit hash
The previous reference used a full commit SHA (@0659057837cd...), which is immutable and guarantees the exact bytes installed. Switching to a tag (@5.0.9) means that if the tag is ever force-pushed or recreated on the upstream repo, a fresh docker build could silently pull in a different commit. The same change is made in tests/topotests/Dockerfile and doc/developer/topotests.rst. Consider using the resolved commit SHA for 5.0.9 to keep the same level of pinning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docker/ubuntu-ci/Dockerfile
Line: 110
Comment:
**Git tag instead of pinned commit hash**
The previous reference used a full commit SHA (`@0659057837cd...`), which is immutable and guarantees the exact bytes installed. Switching to a tag (`@5.0.9`) means that if the tag is ever force-pushed or recreated on the upstream repo, a fresh `docker build` could silently pull in a different commit. The same change is made in `tests/topotests/Dockerfile` and `doc/developer/topotests.rst`. Consider using the resolved commit SHA for `5.0.9` to keep the same level of pinning.
How can I resolve this? If you propose a fix, please make it concise.9106014 to
ea2c01d
Compare
|
I'm not terribly interested in having the topotests work with different versions of exabgp. Please upgrade everything to 5.whatever if that is what you want. This just increases our support load for exabgp, which I'm not terribly interested in doing. |
|
I think I'm with Donald - if the "current" version is 5, let's see about moving to 5. |
9b6d914 to
1494dbb
Compare
Probe and launch exabgp using the 5.x subcommands (`version`,
`server` with EXABGP_ENVFILE) and raise the minimum to 5.0.0. Drop
the /var/run/exabgp.{in,out} fifos, which 5.x refuses to start
against, and redirect the launch stdio to /dev/null so the daemonized
exabgp does not block self.run() on an open pipe.
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Install the ExaBGP 5.0.9 release. Newer address families used by the topotests (for example BGP-MUP) require ExaBGP 5.x. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Install the ExaBGP 5.0.9 release. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Point the topotests install instructions at the ExaBGP 5.0.9 release. Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
1494dbb to
8361645
Compare
|
@donaldsharp @mjstapp Thank you for your comments. Updated to target ExaBGP 5 only. |
The topotests only support ExaBGP 4.2, but its branch no longer
receives new features; newer address families such as BGP-MUP
(#22311) need ExaBGP 5.x.
This PR makes TopoExaBGP.start detect the installed ExaBGP version
and use the matching launch form (the 4.x behavior is unchanged),
and updates the in-tree installs (tests/topotests/Dockerfile,
docker/ubuntu-ci/Dockerfile, doc/developer/topotests.rst) from
4.2.21 to the 5.0.9 release. The NetDEF CI agents provision their
own ExaBGP separately and can switch independently: see #22320.
None of the 5.x breaking changes affect the in-tree tests: all
exabgp.env files use encoder=text, no test drives BGP-LS through
exabgp, and the prefix-sid tests announce the attribute as raw bytes.
Verified bgp_prefix_sid, bgp_vrf_md5_peering and bgp_peer_shut via
tests/topotests/docker/frr-topotests.sh with both 4.2.21 and 5.0.9:
5 passed each. As a side effect this fixes a hang when running the
topotests that way with any exabgp >= 4.2: frr-topotests.sh runs
pytest as the container's PID 1, and exabgp's daemonise() refuses
to detach from a PID 1 parent. No standard setup hits this today --
the published topotests image still ships ExaBGP 3.4.17, which the
version probe rejects fail-fast, and the docker/ubuntu-ci flow runs
with --init -- but a locally built topotests image (4.2.21)
reproduces it. See the first commit message for details.