Repository navigation
Conversation
gmelikov
changed the title
0650571 to
e87f6a6
Compare
|
Reworked fail-static to actually check for end_of_rib_received if mp_graceful_restart supported, otherwise |
|
@exordy /review |
exordy
left a comment
exordy
left a comment
There was a problem hiding this comment.
exo-review: 1 medium, 1 low, nothing blocking
Intent: Make CI actually run unit tests on 3.8/3.14, fix pins, add VXLAN-GBP group propagation and fail-static flow retention.
Verify before merge
ovs-vsctl --may-exist add-port br vxlan_out -- remove Interface vxlan_out options extson a stand: that removing an absent key is a no-op and the chained command applies.- The py2.7 band still resolves:
pip download grpcio==1.39.0 protobuf==3.17.3 --python-version 27 --only-binary :all:— CI covers 3.8 and 3.14 only. - gobgp clears end_of_rib_received on session reset: flap the RR and read
gobgp neighbor <rr> -j— settle_sec masks it, but converged would be wrong if it is sticky.
Note: fail_static defaults on with min_peers=0 (all peers): one peer down for maintenance suspends every flow deletion on that node.
Note: PR body is "See separate commit descriptions" — a squash merge would drop four detailed bodies.
Coverage: 9 axes clean · 5 checks run · mode shipped
Clean axes: security — no API/tenancy/secret surface; GBP underlay trust documented; scope — every hunk traces to one of the four commits; contracts — new OvSClient/service kwargs additive, defaults preserve behaviour; compat — no migration, API, manifest or target-resource surface; reconciliation — union keeps the fresh action; snapshot not overwritten while degraded; GBP coverage — all 4 tunnel-output sites set it, all 3 recovery sites are tunnel-ingress-only; tests — new behaviour covered; nose-style setup/teardown gone repo-wide; prod-readiness — no TODO/debug/process comments added; hygiene — English, (): subjects, no generated artefacts
Checks run:
- grep TODO/FIXME/print/sleep over the diff — none
- grep change-narrating comments over the diff — none
- grep nose-style setup/teardown under tests/ — none left
- grep tun_dst producers in objects.py — all 4 carry MARK_TO_GBP
- read every _ovs_filter_match — GBP_TO_MARK sites all match in_port=vxlan
23 turns · 510s
|
@exordy /review |
exordy
left a comment
exordy
left a comment
There was a problem hiding this comment.
exo-review: LGTM · since e87f6a6: 0 fixed, 0 still open, 0 new, 2 dropped after you answered
Verify before merge
- Whether /tmp survives a host reboot there: if it does, the restored snapshot re-installs pre-reboot flows for the settle window. Check tmpfiles.d on a node.
- Whether guest ofports are pinned across a reboot (ofport_request in the client configs); if not, a retained flow can output to a renumbered port.
Note: PR body defers to the commit bodies; those do carry mechanism and stand evidence.
Note: No sibling PR is involved; this one merges on its own.
Coverage: 10 axes clean · 6 checks run · mode shipped
Clean axes: security — no API/tenancy/secret surface; GBP underlay trust documented; scope — every hunk traces to one of the four commits; contracts — new kwargs additive, gbp and fail_static both default off; compat — no migration, API, manifest or target-resource surface; reconciliation — union keeps the fresh action; snapshot held while degraded; restart — snapshot read back from the tmp flow file; round-trip is exact; peer health — uptime is a Timestamp, EoR only where GR was negotiated; tests — restart, no-peer settle and both GBP states covered; prod-readiness — no TODO/debug/process comments; comments now terse; hygiene — English, (): subjects, no generated artefacts
Checks run:
- git diff e87f6a6..HEAD — delta is snapshot read-back + no-peer settle
- read OvsFlow/BaseObj — equality by match, so the union cannot duplicate
- grep match builders — no spaces, so the flow-file round-trip is exact
- grep gobgp_pb2 descriptors — uptime Timestamp, end_of_rib_received present
- read create_tun_port + tests — remove-by-key on the options map is valid
- checked commit claims — pins, py factors, metrics, defaults all match
28 turns · 545s
When the route reflector dies, gobgp withdraws the reflected routes (it flushes on the shutdown NOTIFICATION even with graceful-restart) and replace-flows tears down the matching OVS flows, so an RR outage breaks existing connectivity. New [gobgp] fail_static (default: false): while too few peers have converged, sync the union of the freshly computed flows and the last known good snapshot; deletions resume once the sessions are back. A union only adds, so local additions and changes still land. [gobgp] fail_static_min_peers (default: 0 — all of them) says how many have to converge; 1 fits redundant route reflectors, where a single session still carries the whole RIB. Converged is stricter than ESTABLISHED, which only says the session is up while the peer is still streaming its table. A peer counts once End-of-RIB (RFC 4724) has arrived for every family that negotiated graceful-restart, and once the session has been up for two sync steps, which is all there is to wait for when nothing reports the end. Measured on a pair of gobgp 3.37 with 20k EVPN routes over a veth: 0.90s between ESTABLISHED and End-of-RIB, against a default 3s step. An empty peer list counts as degraded for the settle time, being what a restarted gobgp looks like before it has read its config; past it the node has no peers and its RIB is trusted. A peer list that cannot be read is raised as a local fault instead. The snapshot is read back from the tmp flow file at startup, so a connector restarted during an outage retains the flows as well. Metrics: fail_static_active, fail_static_retained_cnt. Validated on a 3-node stand: a 2-minute RR outage with flows and connectivity retained, clean release on recovery.
A rule that names a group of workloads rather than a prefix needs the sender's identity to travel with the packet, or every host enforcing it has to be told who the members are and told again on every change. VXLAN-GBP has 16 bits for it, but a tunnel field is cleared crossing a patch port in both directions. The skb mark survives that hop, so the fabric copies between the two at the tunnel and interprets neither. Every path onto the wire carries it (switched, routed, flooded), and off the wire it is recovered on every flow a tunnel ingress can hit — the Type 2 one, VirtNet's stand-in for a missing Type 2 announce, and the VRF's. Never on local traffic, which had no header and whose mark reading one would erase. [ovs] gbp (default: false) is read once and handed to both the tunnel and the flows, so they cannot disagree, and turning it off unmakes an existing GBP tunnel, so the flag is not one-way. Proven on two real hosts (gcl_sdk sdn_fabric tier).
oslo.config 3.22 reads collections.Mapping, removed in python 3.10, so the service died on import; protobuf 3.14 and grpcio 1.26 have no wheels for a current interpreter. Split by python version, and bound each new pin from above as the rest of this file does. The pre-3.10 band keeps netaddr and sentry-sdk as they were; grpcio and protobuf collapse their <3.8 and >=3.8 splits onto one pin each, 1.39.0 and 3.17.3. setuptools is pinned for all of them: pbr reads the package version through pkg_resources, which setuptools 81 dropped and a fresh venv no longer provides. pbr itself cannot move past it, loopster caps it at 5.8.1.
The unit job ran `tox -e 3.8`, which is not an environment this tox.ini defines: its commands are bound to the py27/py38 factors, so an env named `3.8` matched none of them and the job passed in 0.02s having run nothing. The matrix now names real environments, the unit command is bound to every python factor from py27 to py314, and 3.14 is covered. Lint stays on 3.8. That surfaced two failures the job had never been in a position to see: pytest 8 no longer calls nose-style setup/teardown (renamed to setup_method/teardown_method, which every pytest since 2.x accepts), and pbr needs pkg_resources, which a fresh venv does not ship. Both also fail on master with the same invocation.
|
LGTM |
3 participants
See separate commit descriptions.