lib: northbound: distinguish unknown schema node from key mismatch

[reinaldosaraiva]

Summary

mgmtd currently emits "List keys in xpath and data tree are different" for two distinct edit-config failure modes:

• the xpath names a schema node that does not exist (typo, e.g. interfac instead of interface); and
• the xpath resolves to a valid schema node, but the key predicates do not match the keys carried in the payload.

The combined message makes the first case hard to debug — operators and external controllers cannot tell the schema name is wrong without cross-referencing the YANG tree by hand.

This patch calls lys_find_path() on the schema context only on the error path: if the xpath does not resolve to any schema node, emit "Unknown schema node in xpath: <xpath>"; otherwise keep the existing message. Happy path is unaffected.

Test plan

• tools/checkpatch.pl --no-tree -g HEAD → 0 errors, 0 warnings
• Existing tests/topotests/mgmt_tests/test_yang_mgmt.py:500 still passes — it asserts the key-mismatch message which is preserved verbatim
• Empirical probes on mgmtd 10.7-dev standalone:
  • mgmt edit replace /frr-interface:lib/interfac[name='eth0'] ... → % Unknown schema node in xpath: /frr-interface:lib/interfac[name='eth0']
  • mgmt edit replace /frr-interface:lib/interface[name='eth0'] ... (payload with name='eth1') → % List keys in xpath and data tree are different

Why this is safe

• Changes only the error message text; return code (NB_ERR) is unchanged.
• lys_find_path runs exclusively when lyd_find_path already failed.
• No ABI or API change; no new includes.

[greptile-apps]

Greptile Summary

This PR improves mgmtd error diagnostics by distinguishing two distinct edit-config failure modes that previously shared the same message. On the error path of nb_candidate_edit_tree_add, a lys_find_path schema-only lookup is added to differentiate a non-existent schema node (typo in xpath) from a key predicate mismatch, emitting a more actionable message in the former case. The change is error-path-only, return codes are unchanged, and a new topotest covers the new code path.

Confidence Score: 5/5

Safe to merge — error-path-only change with no impact on the happy path, return codes, or ABI.

No P0 or P1 findings. The change is narrowly scoped to an existing error branch, lys_find_path is already used elsewhere in the codebase for the same purpose (e.g. yang.c:343, yang.c:1010), the existing key-mismatch message is preserved verbatim, and a new topotest covers the added code path.

No files require special attention.

Important Files Changed

Filename	Overview
lib/northbound.c	Adds lys_find_path schema lookup exclusively on the error path to emit a distinct "Unknown schema node" message vs. "List keys…different"; logic is sound and well-commented.
tests/topotests/mgmt_tests/test_yang_mgmt.py	Adds a topotest case using a typo'd xpath (interfac) to verify the new "Unknown schema node in xpath" error message is emitted correctly.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["nb_candidate_edit_tree_add()"] --> B{"root || !REPLACE && !MODIFY?"}
    B -- Yes --> C[Skip key check]
    B -- No --> D["lyd_find_path(tree, xpath)"]
    D -- Success --> E[Continue normal path]
    D -- Failure --> F["lys_find_path(ly_native_ctx, NULL, xpath)"]
    F -- NULL\nschema node not found --> G["errmsg: 'Unknown schema node in xpath: <xpath>'\nret = NB_ERR\ngoto done"]
    F -- non-NULL\nschema node exists --> H["errmsg: 'List keys in xpath and data tree are different'\nret = NB_ERR\ngoto done"]

Loading

_{Reviews (2): Last reviewed commit: "lib: northbound: distinguish unknown sch..." | Re-trigger Greptile}

[reinaldosaraiva]

Additional validation — re-ran the two probes inside a real FRR deployment (podman-in-podman chassis, image localhost/mgc-connect/frr:mgc-bgpd-lite-dev) after injecting the rebuilt libfrr.so.0.0.0 + mgmtd with all backends registered:

[TKR4Z-MFN07] Backend daemon: mgmtd registers with mgmtd (client-id: 0)
[TKR4Z-MFN07] Backend daemon: bgpd registers with mgmtd (client-id: 1)
[TKR4Z-MFN07] Backend daemon: staticd registers with mgmtd (client-id: 2)
[TKR4Z-MFN07] Backend daemon: zebra registers with mgmtd (client-id: 3)

Probes via vtysh -d mgmtd:

• mgmt edit replace /frr-interface:lib/interfac[name='eth0'] lock commit {"frr-interface:interface":[{"name":"eth0","description":"d"}]} → % Unknown schema node in xpath: /frr-interface:lib/interfac[name='eth0']
• mgmt edit replace /frr-interface:lib/interface[name='eth0'] lock commit {"frr-interface:interface":[{"name":"eth1","description":"d"}]} → % List keys in xpath and data tree are different

Schema typo detected distinctly; key-mismatch message preserved verbatim (matching the existing assertion at tests/topotests/mgmt_tests/test_yang_mgmt.py:500).

[reinaldosaraiva]

Full validation suite for the amended commit (adds the new topotest case).

T1 — Existing topotest suite passes

cd tests/topotests && sudo python3 -m pytest mgmt_tests/test_yang_mgmt.py -v
...
======================== 7 passed, 2 warnings in 25.91s ========================

T2 — New test case for the new branch

Added in tests/topotests/mgmt_tests/test_yang_mgmt.py::test_mgmt_edit_config, right after the existing key-mismatch assertion. Exercises mgmt edit replace with a misspelled schema node (interfac instead of interface) and asserts "Unknown schema node in xpath". Both cases now covered by the same test.

T3 — Chassis-real validation

Already included in the earlier comment: patched libfrr.so.0.0.0 (sha256 28f16311...) + patched mgmtd (sha256 6314e7af...) running inside a podman-in-podman chassis with all four backends registered (mgmtd, bgpd, staticd, zebra). Probes via vtysh -d mgmtd returned the expected distinct messages.

T4 — Multiple entry points

nb_candidate_edit_tree_add is common code shared by vtysh mgmt edit, NETCONF, gNMI, and RESTCONF via the mgmtd FE socket. The topotest uses vtysh -c "mgmt edit replace ..." which, per FRR's architecture, goes through the FE native protocol — the same path any external controller uses. No entry-point-specific branch exists in the patched code, so a single-path test covers the common case.

T5 — No regression under load

Ran reconciler-bench against the patched mgmtd (100 concurrent writers, overlap=1.0):

{
  "successes": 100, "failures": 0, "success_pct": 100,
  "total_retries": 0, "max_retries": 0,
  "p50_ms": 53.26, "p95_ms": 72.91, "p99_ms": 74.65, "max_ms": 74.97
}

Happy-path unaffected as expected — the extra lys_find_path call runs only on the error path (after lyd_find_path has already failed). Bench artifact and topotest output preserved locally.

[reinaldosaraiva]

Raw artifact (chassis probes + sha256 attestations) preserved as gist for reproducibility: https://gist.github.com/reinaldosaraiva/33b4a402eb6dccbd4e4dee450600ebb7

[reinaldosaraiva]

Thanks @greptile-apps — good catch on the lys_find_path semantics. After review, keeping the current implementation for this specific call-site, with rationale:

Edit-config xpaths are constrained to canonical instance-identifiers. nb_candidate_edit_tree_add is invoked from the mgmtd FE protocol path, whose four entry points all enforce canonical paths:

• vtysh mgmt edit — parses simple key-predicate form
• NETCONF <edit-config> — RFC 6241 instance-identifier (RFC 7950 §9.13)
• RESTCONF — RFC 8040 §3.5.3 data resource identifier (RFC 7951 §6.11)
• gNMI SetRequest — gNMI path (no XPath axes/functions)

None of these can legally carry //, XPath functions, multi-step predicates, or or/and expressions. The warning at lib/yang.c:313 applies to yang_resolve_snode_xpath, which has a broader contract (it is used by show-state and arbitrary query paths, not edit).

yang_resolve_snode_xpath correctly uses the two-step fallback because it must accept arbitrary user XPaths. nb_candidate_edit_tree_add does not, so the simpler lys_find_path matches the input contract exactly and avoids a redundant libyang lookup on the error path.

If a future change relaxes the edit-config xpath contract (e.g. to permit XPath predicates), this site should adopt the fallback then. Happy to add an inline comment noting this constraint if a maintainer prefers.

[greptile-apps]

That reasoning is sound. The distinction you've drawn — that yang_resolve_snode_xpath's two-step fallback exists for callers with an arbitrary-XPath contract, and this call site has a strictly narrower canonical instance-identifier contract — is the right framing.

The inline comment would be a good addition. Something like:

+			/*
+			 * lyd_find_path fails for two distinct reasons: the
+			 * xpath names a schema node that does not exist (typo),
+			 * or the xpath resolves but its key predicates diverge
+			 * from the keys carried in the payload. Distinguish
+			 * them via a schema-only lookup so the operator sees
+			 * which case happened.
+			 *
+			 * lys_find_path (not yang_resolve_snode_xpath) is
+			 * intentional: edit-config xpaths are canonical
+			 * instance-identifiers (vtysh, NETCONF, RESTCONF,
+			 * gNMI), not arbitrary XPath expressions, so the
+			 * two-step fallback in yang_resolve_snode_xpath is
+			 * unnecessary here.
+			 */

That documents the constraint so future readers don't second-guess the choice or inadvertently "fix" it.

[reinaldosaraiva]

Thanks @greptile-apps — applied the inline comment verbatim. Force-pushed as ec981fd9ea (comment-only amendment, no code or behavior change).

The extended comment now documents both what the schema-only lookup distinguishes and why lys_find_path is intentional vs yang_resolve_snode_xpath at this call-site (canonical instance-identifier contract via vtysh/NETCONF/RESTCONF/gNMI). Future readers get the reasoning without having to re-derive it from the PR thread.

checkpatch.pl --no-tree -g HEAD: 0 errors, 0 warnings, 38 lines.

[reinaldosaraiva]

Proactively applied the frrbot style diff — force-pushed as 7c4498d4dd. Pure line-folding in lib/northbound.c:954-957 (one snprintf call folded onto fewer lines), no behavior change. checkpatch.pl --no-tree -g HEAD: 0 errors, 0 warnings, 9 lines checked. frrbot status was NEUTRAL; this should flip it green.