fix(srv6): DelConnectivity の NLRI-key teardown・候補パス failover (+ SR Policy キャッシュ堅牢化)#8
Open
ryskn wants to merge 14 commits into
Open
fix(srv6): DelConnectivity の NLRI-key teardown・候補パス failover (+ SR Policy キャッシュ堅牢化)#8ryskn wants to merge 14 commits into
ryskn wants to merge 14 commits into
Conversation
…and candidate-path failover Replace the FIXME no-op SRv6Provider.DelConnectivity with a precise teardown path keyed by the RFC 9012 <Distinguisher, Color, Endpoint> NLRI tuple: - delSRPolicy matches cached tunnels by the NLRI key so a withdraw removes only the candidate path it identifies, never collaterally tearing down unrelated policies on the same endpoint. - Orphaned prefixes (those whose steering pointed at the deleted BSID) are re-steered onto the next-best surviving candidate of the matching behavior per RFC 9256 candidate-path failover, with the surviving policy installed on demand (it was masked by the higher-priority winner and never reached AddConnectivity's CreateSRv6Tunnnel call). - delPrefixSteering mirrors AddConnectivity's policy/localSid pool handling and prunes the steering rule + nodePrefixes cache symmetrically. common.SRv6Tunnel now carries Color and Distinguisher; getSRPolicy populates them on success, on withdraw short-circuit, and on mixed-behavior reject so the SRv6PolicyDeleted dispatch can be targeted. Adds unit tests covering the NLRI-key precision, the failover path (orphan collection + on-demand install + re-steering with dedupe), the no-survivor fallback, and the dispatcher type-assert. A small srv6VppAPI interface is extracted in the connectivity package so tests substitute a fake VppLink without spinning up the dataplane. Signed-off-by: Ryosuke Nakayama <ryosuke.nakayama@ryskn.com>
Calico-VPP's PeerWatcher.createBGPPeer hardcodes the AFI/SAFI list for external BGPPeers and omits ipv6-unicast on the IPv4-typed side, so an external IPv4 SR-TE controller cannot advertise pod prefixes with IPv6 next-hops via MP-BGP (extended-nexthop) over the IPv4 BGP session. This breaks the live verification of SRv6Provider.DelConnectivity candidate- path failover, where the surviving lower-priority candidate must be installed on demand for a known IPv6 pod prefix. Append BgpFamilyUnicastIPv6 to the afiSafis list when the peer address is IPv4. IPv6-typed peers already get IPv6 unicast via typ, so the append is gated to avoid duplicates. This is a test-scaffolding change kept on a dev/** branch and is not intended for the upstream DelConnectivity PR. Signed-off-by: Ryosuke Nakayama <ryosuke.nakayama@ryskn.com>
…rs as idempotent Two related issues surfaced during candidate-path failover (Step 3) verification: 1. nodePolices[endpoint].SRv6Tunnel was append-only. Re-advertisement of the same NLRI key <Distinguisher, Color, Endpoint> on the same endpoint (which happens on BGP path refresh, peer-state churn, etc.) silently grew the cache instead of replacing the prior candidate. Per RFC 9252, the NLRI key is the identity of a candidate path; same key MUST replace. 2. delSRPolicy iterated `matched` BSIDs without dedup. With (1), the same BSID could appear multiple times, and the second pass tried to delete a steering / SR policy that VPP had just removed on the first pass — producing NO_SUCH_INNER_FIB (-4) from DelSRv6Steering and UNSPECIFIED (-1) from DelSRv6Policy as warn-level noise. Functional outcome was correct; the warns were misleading. Fixes: - AddConnectivity (SRv6PolicyAdded branch): upsert by (Color, Distinguisher) instead of unconditional append. Same NLRI key replaces in place. - isAlreadyGoneOnDelete helper: recognises NO_SUCH_INNER_FIB (-4) on DelSRv6Steering and UNSPECIFIED (-1) on DelSRv6Policy as benign in a delete context, downgrading them from warn to debug. Other VPP errors still warn. Applied to delSRPolicy and delPrefixSteering. Tests: - TestAddConnectivity_ReAdvertiseSameNLRIKeyReplacesInPlace verifies same-key re-advertisement replaces in place. - TestAddConnectivity_DifferentNLRIKeysCoexist guards against the dedup over-applying to legitimate candidate paths. - TestDelSRPolicy_IdempotentWhenVPPAlreadyMissing verifies failover still completes when VPP returns NO_SUCH_INNER_FIB / UNSPECIFIED on delete.
…with a new BSID The previous commit moved nodePolices append to an in-place upsert keyed by NLRI <Distinguisher, Color, Endpoint>. That is RFC 9252-correct but had a gap: when BGP path refresh changes the BSID for an unchanged NLRI key, the prior BSID's SR Policy stays installed in VPP with no cache reference. A later withdraw matches only the new BSID, so the old one leaks indefinitely. The pre-upsert (append-only) behaviour incidentally cleaned up such leaks because matched accumulated every BSID ever advertised for the NLRI key. With upsert that safety net is gone; this commit replaces it with an explicit teardown on BSID change. On upsert, if the prior cache entry carried a different BSID, call DelSRv6Policy for that BSID. isAlreadyGoneOnDelete already covers the case where the prior candidate was never installed in VPP (e.g. lower priority than auto), so cleanup is idempotent. Steering does not need explicit cleanup here: the subsequent AddSRv6Steering inside CreateSRv6Tunnel hits VPP's update path and re-points the existing steering at the new sr_policy index. Tests: - TestAddConnectivity_BsidChangeOnUpsertCleansUpOldBsid: same NLRI key, different BSID → DelSRv6Policy(oldBsid) fires exactly once and cache holds only the new candidate. - TestAddConnectivity_SameBsidOnUpsertSkipsCleanup: same NLRI key and same BSID with only attribute changes (priority bump) does NOT trigger cleanup.
…ointed The prior commit cleaned up the orphaned BSID inline, right after the cache upsert. That fired DelSRv6Policy(oldBsid) BEFORE the subsequent CreateSRv6Tunnel loop re-pointed the steering at the new BSID, leaving a window where VPP's steering hash held steer_pl->sr_policy = a freed sr_policy pool index. Packets transiting the steering during that window would land on undefined sr_policy state. Move the DelSRv6Policy(oldBsid) call to a function-level defer. The deferred closure fires after the CreateSRv6Tunnel block has finished AddSRv6Steering(newBsid) — VPP's update path then overwrites the steering's sr_policy reference cleanly, so the subsequent policy delete only releases state nothing references anymore. Test: - TestAddConnectivity_BsidChangeCleansUpAfterRePoint pre-populates nodePrefixes so CreateSRv6Tunnel runs, then asserts the call log shows AddSRv6Steering(newBsid) strictly precedes DelSRv6Policy(oldBsid). The fakeSRv6VPP gained a callLog field that records the interleaved sequence of every method invocation.
The previous commit deferred DelSRv6Policy(oldBsid) until after the
CreateSRv6Tunnel loop above had re-pointed the steering. That handles the
happy path, but it fires unconditionally on any function exit, including
the cases where the re-point itself FAILED:
- getPolicyNode returned nil (e.g. no cached candidate of the matching
behavior), so CreateSRv6Tunnel never ran for this prefix.
- AddModSRv6Policy(newBsid) errored, so newBsid is not in VPP's sr_policies
hash and the subsequent AddSRv6Steering lookup falls back to -2.
- AddSRv6Steering errored, leaving steer_pl->sr_policy untouched.
In all three, VPP's steering still recursively resolves through the prior
BSID. Pulling its sr_policy out would drop traffic until the next
advertisement.
Gate the deferred DelSRv6Policy on the post-loop steering state from
ListSRv6Steering: if any entry still has Bsid == orphanedBsid, skip the
cleanup with a debug log; the BSID will be retried on the next
advertisement. The cost is one extra dump per BSID-change upsert, which is
infrequent enough to be acceptable.
Test:
- TestAddConnectivity_BsidChangeSkipsCleanupWhenStillReferenced seeds the
fake's ListSRv6Steering output with an entry pointing at the old BSID
after the upsert, then asserts DelSRv6Policy(oldBsid) was NOT called.
The previous defer-and-guard pattern had a leak: when the in-place NLRI-key upsert detected a BSID change but VPP still steered through the prior BSID, the cleanup logged "deferred" and dropped the BSID reference. The cache had already moved on, so subsequent upserts compared the NEW BSID against any newer advertisement — the original prior BSID was unreachable from cache and would never be retried. The SR Policy stayed installed in VPP forever. Add a small pendingBsidCleanup queue on SRv6Provider. When the upsert discovers the steering still resolves through the prior BSID, the BSID goes onto the queue instead of being silently dropped. drainPendingBsidCleanup walks the queue on every SR-policy AddConnectivity invocation and on delSRPolicy: a single ListSRv6Steering classifies each entry, the unreferenced ones get DelSRv6Policy, and entries still in use stay queued for the next event. Errors other than "already gone" re-queue too. Also: remove the early return from the SR-policy AddConnectivity branch when nodePrefixes is empty. The bottom CreateSRv6Tunnel block is already gated on nodePrefixes != nil and will skip itself; falling through is what lets the drain at function end still run. Test: - TestAddConnectivity_BsidChangePendingRetryOnNextEvent stages a failed re-point (fake's steering still pointing at oldBsid after the upsert), asserts oldBsid lands on pendingBsidCleanup, then unblocks VPP by flipping the fake's steering to otherBsid. A subsequent unrelated SR-policy event then drains the queue and finally fires DelSRv6Policy(oldBsid).
ryskn
changed the title
Skip cached candidates with a nil Policy (resteerOrphan dereferences policy.Bsid with only a nil-policy guard on the return value) and use a strict > with a found flag so a priority tie deterministically keeps the first candidate instead of letting a later, possibly nil-Policy entry win.
Address review feedback (nesting too deep) in delSRPolicy and delPrefixSteering. Extract the "delete + log already-gone at debug, real error at warn" branch into delSteering/delPolicy helpers and use early continue in the steering scan, dropping the loops from 4-5 levels of nesting to 2.
… helpers Replace the delSteering/delPolicy/reportDelete methods with a local logDel closure in delSRPolicy and inline equivalents in delPrefixSteering / drainPendingBsidCleanup. The closure selects the log function (warn, or debug when isAlreadyGoneOnDelete) rather than wrapping a switch in a separate method, keeping the delete-logging logic next to where it's used.
Invert the match condition into an early continue and collapse the two nested BSID-compare ifs into a single guard, dropping the upsert body from 4 levels of nesting to 2.
…ments Readability review follow-ups: - AddConnectivity used a named return `err` that was always shadowed by the block-local `prefix, err :=`, so the tail `return err` was just `nil` with extra cognitive load. Drop the named return and return nil explicitly. - getPolicyNode comment said "without an installed Policy", implying nil Policy == not-installed-in-VPP, which is false (a cached lower-priority candidate has a non-nil Policy but is not installed). Reword as a plain nil-deref guard. - Align the NLRI-key RFC citation in srv6.go with common.go / bgp_watcher.go (RFC 9012, not 9252).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
SRv6Provider.DelConnectivitywas a FIXME no-op. This implements SR Policy withdraw/teardown and the candidate-path failover that follows it (RFC 9252 / 9256).<Distinguisher, Color, Endpoint>, so it tears down only the candidate path it identifies and never collaterally removes other policies on the same endpoint.NO_SUCH_INNER_FIBandUNSPECIFIEDfrom VPP are treated as idempotent "already gone" in a delete context.Verification
From a worker, steer a remote-node prefix and have an external BGP speaker install SR Policy A, then withdraw it.
before:
after:
BSID change cleanup (re-advertise the same NLRI key with a new BSID):