Skip to content

Support pinning demands to explicit routes (static paths) - #109

Merged
networmix merged 2 commits into
review/correctness-perf-docs-overhaulfrom
feature/static-paths
Aug 24, 2026
Merged

Support pinning demands to explicit routes (static paths)#109
networmix merged 2 commits into
review/correctness-perf-docs-overhaulfrom
feature/static-paths

Conversation

@networmix

@networmix networmix commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Stacked on #108. This branch builds on review/correctness-perf-docs-overhaul, which is this PR's base, so the diff below is the static-paths work only. GitHub retargets this PR to main automatically once #108 merges.

What this adds

A demand can name the routes its traffic must follow, instead of letting the flow policy choose them. This models MPLS-style LSPs.

demands:
  default:
    - source: "^A$"
      target: "^C$"
      volume: 10
      mode: pairwise
      flow_policy: SHORTEST_PATHS_WCMP
      static_paths:
        - ["A", "B", "C"]              # node names
        - links: ["A|D|0", "D|C|0"]    # or link ids, for a specific parallel link

One flow per route, created in the order listed. A route broken by a failure carries nothing rather than rerouting — that is what separates a pinned route from ordinary routing, and it is the property most of the tests pin down.

Node names read well; link ids are the escape hatch when parallel links connect the same pair. That form only became usable because link ids are deterministic (A|B|0) as of #108.

Why the design looks like this

Two constraints in netgraph-core shaped it, both verified in the C++ source rather than assumed:

  • PredDAG is read-only from Python — a bundle can only be built with from_edges, which takes one edge per hop. So a route is a strict explicit route. Where parallel links connect a pair, a node hop takes one of them; to model an LSP per parallel link, list one route per link using the links form. This is called out in the DSL reference.
  • Core rejects a max_flow_count that differs from the bundle count, so presets such as SHORTEST_PATHS_ECMP (count 1) would have thrown for a two-route demand. create_flow_policy now routes all five presets through one construction point and takes static_path_count.

Because routes run between two concrete nodes, a pinned demand needs mode: pairwise and selectors matching exactly one source and target. Combine mode routes through pseudo endpoints no operator route can start from, so it is rejected with that explanation.

Dependency bump

netgraph-core>=0.8.0set_static_paths and PredDAG.from_edges are 0.8.0 APIs. This is a correctness fix, not just a feature requirement: an earlier revision of this branch declared >=0.7.0, which would have resolved to a version without those APIs and failed with AttributeError the first time anyone used a pinned route.

0.8.0 also adds a max-flow completion phase that augments over the full residual graph, so max_flow can return more than it did on 0.7.x where the tier loop stopped below the true maximum. docs/reference/design.md is corrected accordingly: reverse residual arcs return previously placed flow, rather than serving only min-cut reachability as the previous text claimed. The restriction is documented and verified — the completion phase applies to PROPORTIONAL + require_capacity=True + shortest_path=False only.

Review found real bugs; they are fixed here

An adversarial review of this feature raised 15 findings. The ones that mattered:

Severity Bug Fix
High A node hop could pin to an administratively disabled link, so the route was pruned and the demand silently placed 0.0 while an enabled parallel link sat unused Disabled links are skipped; naming one explicitly is an error
Medium The Monte Carlo config round-trip rejected the list route form the DSL documents, raising AttributeError from a public API Accepts every form the YAML builder does
Medium A pinned demand whose selectors matched nothing was silently dropped, contradicting the documented guarantee Raises
Medium ${var} expansion could inject a non-string hop, bypassing the schema (which runs before expansion) Rejected, naming the value
Medium Bundles were rebuilt every iteration — 64% of pinned-iteration time — though they depend only on the static graph Resolved once per context; verified cached bundles still track per-iteration failures
Medium The docs claimed proportional presets "fill each route to its bottleneck". They fill in supply order, so route order is significant Corrected

My own code-walk separately found an O(E) scan per hop (3.3 ms → 0.28 ms via adjacency lookup) and a raw KeyError leaking where every sibling path raises a clear ValueError.

Known limitation

Two demands pinned between the same source, target and priority are rejected: Core assigns flow ids per policy starting at zero, so they would collide and corrupt placement. Python cannot offset them. Put every route on one demand, or separate by priority — the error says so, and the DSL reference documents it. Lifting this needs a Core change (a flow_id_base on FlowPolicyConfig).

Verification

  • 31 new tests, including one per review finding.
  • Randomized pressure test: 120 trials over random layered topologies, route sets, presets and volumes — placement never exceeds the demand, never exceeds the summed bottlenecks of the pinned routes, and never oversubscribes a link.
  • Monte Carlo: 200 iterations with single-link failures — outcomes are only the surviving route's capacity, never a reroute, and occurrence counts sum to the iteration count.
  • The YAML example in the DSL reference executes verbatim.
  • make check-ci green: 1217 tests, coverage 91.89%. make validate and make docs green.

A demand can name the routes its traffic must follow instead of letting the
flow policy choose them, modelling MPLS-style LSPs:

    static_paths:
      - ["A", "B", "C"]              # node names
      - links: ["A|D|0", "D|C|0"]    # or link ids, for a specific parallel link

One flow is created per route, in the order listed. A route broken by a
failure carries nothing rather than rerouting, which is what distinguishes a
pinned route from ordinary routing.

Built on netgraph-core 0.8.0's FlowPolicy.set_static_paths and
PredDAG.from_edges, so the dependency floor moves to 0.8.0. That release also
adds a max-flow completion phase, so max_flow can return more than it did on
0.7.x; the design reference is updated to describe reverse residual arcs
returning placed flow rather than serving only min-cut reachability.

Because routes run between two concrete nodes, a pinned demand must use
mode: pairwise with selectors matching exactly one source and one target.
Combine mode routes through pseudo endpoints that no operator-supplied route
can start from, so it is rejected with that explanation.

Resolution details:

- A node hop takes the cheapest enabled link between the pair, ties broken by
  link id, so the choice is stable across identical scenario builds. Disabled
  links are never chosen, and naming one explicitly is an error: a route
  pinned to a disabled link could never carry traffic.
- Hops resolve through the graph's adjacency rows rather than a scan of every
  edge, so cost is proportional to node degree rather than graph size.
- Bundles depend only on the static graph, not on per-iteration masks, so they
  are resolved once per analysis context and reused across Monte Carlo
  iterations and MSD probes.

Known limitation: two demands pinned between the same source, target and
priority are rejected, because netgraph-core assigns flow ids per policy
starting at zero and they would collide. List every route on one demand, or
separate the demands by priority.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@networmix
networmix changed the base branch from main to review/correctness-perf-docs-overhaul August 24, 2026 00:06
@networmix
networmix marked this pull request as ready for review August 24, 2026 00:33

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4c188d5aba

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ngraph/model/demand/spec.py
Publishing netgraph-core 0.8.0 made several statements in this release
inaccurate. An audit that ran ngraph against 0.7.2 and 0.8.0 side by side,
cross-checked against networkx.maximum_flow_value on thousands of random
topologies, found 0.7.2 understates max flow on cost-asymmetric graphs and
0.8.0 is exact -- so the completion phase is a correctness fix, and the docs
should describe it rather than the tier loop alone.

The min-cut duality guarantee announced under BREAKING was unqualified. It
holds for the default max-flow configuration (PROPORTIONAL, require_capacity,
not shortest_path) -- the same gate the C++ completion phase uses -- and not
for the placement models. Scoped in the changelog and in api.md.

Also corrected, all wrong on both this branch and its parent:

- cost_distribution keys: completion-phase entries are marginal costs (forward
  edge costs minus the cancelled flow's cost), so a key need not match any
  traversable path. design.md and the MaxFlowResult docstring said "path cost
  tier", which is only true of the tier loop.
- The MAX_FLOW pseudocode ended at the tier loop, so a reader implementing it
  would reproduce 0.7.x's smaller answer. Added the completion phase.
- The complexity bound was justified by "placed flow is never removed from an
  edge", which the completion phase does. The bound itself still holds --
  Edmonds-Karp at O(VE^2) is dominated -- so only the justification changed.
- "Does not re-route previously placed flow" now says which phase does not.

No behavior change; docs and changelog only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@networmix
networmix merged commit faaf052 into review/correctness-perf-docs-overhaul Aug 24, 2026
10 checks passed
@networmix
networmix deleted the feature/static-paths branch August 24, 2026 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant