[202405][GCU] Backport sonic-yang-mgmt + libyang perf-dep patches (co-req sonic-utilities.msft#417 — see merge order) - #2733
Conversation
When a uses clause imports a grouping, it was only processing leaf entries and ignoring leaf-list and choice clauses. That means that for instance in bgp route maps, route_map_in and route_map_out validations would fail. Honoring the uses refine clause is now also honored which is depended upon in sonic-utilities. This now precompiles the uses clause and integrates it as if the uses clause was not part of the schema as multiple end users were having to do this additional processing. This fixes that behavior and adds test cases to ensure it doesn't regress in the future. This is the proper fix, replacing #21078 that just worked around it. Removal of uses logic in sonic-utilities here: sonic-net/sonic-utilities#3814 Fixes #22382 Work item tracking Microsoft ADO (number only): How I did it Added leaf-list lookup. How to verify it See test cases pass Description for the changelog sonic-yang-mgmt: uses clause with leaf-list, choice not honored
…22254) Why I did it Generic Config Updater (GCU) is notoriously slow. These patches add some helpers for the GCU overhaul (mostly in sonic-utilities) in order to facilitate the optimizations. These changes are in sonic-yang-mgmt plus a patch to libyang 1. Changes include: Libyang v1 was not exposing must data for leaf nodes (like it does for other node types). Patch to correct this oversight. Loading of sonic configuration data should not mutate the user-provided data, this forces callers to know they need to deepcopy the data, plus in most instances data won't be mutated. sonic-yang-mgmt test models should more closely mimic real sonic yang models as we can't implement other features that assume this otherwise (sonic mandates that the top-level container has the same name as the module). Import sonic configdb<>yang xpath conversion from sonic-utilities as this should be shared code. 90% of this code is copied from the original source but does contain some bugfixes and enhancements including caching. Make find_schema_dependencies() public, plus add the ability to find dependencies recursively. This implementation is caching. Add new find_schema_must_count(), with recursive capabilities to find if a node (and its children) have must clauses. This implementation is caching. sonic-utilities PR: sonic-net/sonic-utilities#3831 Stats below ... (stats need both this and the sonic-utilities PR to be relevant)... Original Performance: Dry Run: time sudo config replace -d ./config_db.json ... real 2m51.588s user 2m23.777s sys 0m25.300s Full: time sudo config replace ./config_db.json ... real 14m53.772s user 12m2.376s sys 2m8.908s With Patch: Dry Run: time sudo config replace -d ./config_db.json ... real 0m59.602s user 0m56.434s sys 0m2.110s Full: time sudo config replace ./config_db.json ... real 1m54.303s user 0m58.482s sys 0m2.545s So that's roughly 3x improvement for dry-run, and 7.5x improvement for full commit. There is room for improvement on the full commit due to a sleep(1) being used between each patch because of a race condition found in the prior code (that was hidden due to a costly sanity check that has been removed). Work item tracking How I did it Gathered profiling data using cProfile and evaluated where the largest gains could be had. How to verify it This patch is standalone as it will not cause any issues in other projects which use sonic-yang-mgmt or libyang, however the performance benefits are in sonic-utilities. Apply both this commit and the sonic-utilities PR to a local branch, build and run sonic-utilities tests. Then create a full image, load it onto a DUT (with default configuration), and use the attached config_db.json to attempt a config replace operation (tested on Dell S5248F). Which release branch to backport (provide reason below if selected) 202411 Tested branch (Please provide the tested image version) master as of 20250521 Description for the changelog sonic-yang-mgmt: Generic Config Updater - performance dependencies Fixes #22372
loadData currently logs LOG_ERR "Data Loading Failed" to syslog on every exception (log-and-throw antipattern) before raising. This is undesirable for callers that use loadData speculatively to test many candidate configs for validity, e.g. generic_config_updater patch sorter (sonic-utilities) FullConfigMoveValidator — every forward leafref during the search emits a LOG_ERR to syslog even though the caller handles the exception and uses it as a prune signal. Add a quiet=False kwarg. When quiet=True, suppress the informational "Try to load Data" line and the LOG_ERR "Data Loading Failed" line, but still raise SonicYangException so the caller sees the failure. Default quiet=False preserves existing behavior for all current callers (including config apply-patch final validation, db_migrator upgrades, sonic-cfggen, and unit tests). Paired with sonic-utilities change that makes patch_sorter.FullConfigMoveValidator pass quiet=True. <!-- Please make sure you've read and understood our contributing guidelines: https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md failure_prs.log skip_prs.log Make sure all your commits include a signature generated with `git commit -s` ** If this is a bug fix, make sure your description includes "fixes #xxxx", or "closes #xxxx" or "resolves #xxxx" Please provide the following information: --> #### Why I did it On T0 devices, a single `config apply-patch` invocation leaks 100+ `Data Loading Failed` ERR lines to /var/log/syslog. These come from the GCU patch sorter's speculative `FullConfigMoveValidator`, which deliberately tests candidate moves that may transiently violate leafref constraints during the search — the sorter already handles the failure via the returned `(False, error)` tuple. The ERR lines trip `loganalyzer` in sonic-mgmt baseline tests (e.g. `test_dhcp_relay_with_non_default_vrf`, `test_dhcp_relay_with_different_non_default_vrf`) and obscure real errors in support bundles. The root cause is `loadData`'s log-and-throw antipattern: it emits a LOG_ERR *and* raises, forcing log noise on callers that are expected to recover. ##### Work item tracking - Microsoft ADO **(number only)**: #### How I did it Added a `quiet=False` kwarg to `SonicYangExtMixin.loadData`. When `True`, guard the informational `"Try to load Data"` sysLog and the `LOG_ERR "Data Loading Failed"` sysLog behind `if not quiet:`. `SonicYangException` is still raised on failure, so nothing about the control flow changes — only the syslog side-effect is suppressed. Default `quiet=False` preserves behavior for every current caller. #### How to verify it 1. Build `python3-sonic-yang-mgmt` wheel from this branch and install on a T0 DUT. 2. Pair with the sonic-utilities PR that threads `quiet=True` from `FullConfigMoveValidator` → `validate_config_db_config` → `loadData`. 3. Run `sonic-mgmt` generic_config_updater tests and/or `dhcp_server/test_dhcp_server_port_based_customize_options` on a T0 and count `"Data Loading Failed"` lines in `/var/log/syslog` across the window. <!-- If PR needs to be backported, then the PR must be tested against the base branch and the earliest backport release branch and provide tested image version on these two branches. For example, if the PR is requested for master, 202211 and 202012, then the requester needs to provide test results on master and 202012. --> #### Which release branch to backport (provide reason below if selected) <!-- - Note we only backport fixes to a release branch, *not* features! - Please also provide a reason for the backporting below. - e.g. - [x] 202006 --> - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 - [x] 202511 #### Tested branch (Please provide the tested image version) <!-- - Please provide tested image version - e.g. - [x] 20201231.100 --> - [ ] <!-- image version 1 --> - [ ] <!-- image version 2 --> #### Description for the changelog <!-- Write a short (one line) summary that describes the changes in this pull request for inclusion in the changelog: --> <!-- Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU. --> #### Link to config_db schema for YANG module changes <!-- Provide a link to config_db schema for the table for which YANG model is defined Link should point to correct section on https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md --> Signed-off-by: Sonic Build Admin <sonicbld@microsoft.com> #### A picture of a cute animal (not mandatory but encouraged)
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
rimunagala
changed the title
This is an interesting dimension to add and use as a reference for confidence to what change is fixing and what not. However, the it seems suboptimal that empty patch takes ~10s. Do we know exactly why no changes (empty patch) will still lead to performance issues? LGTM otherwise. |
|
This needs release manager review before merge. |
Yes — it's deliberate. When a patch yields no per-scope changes, the code explicitly fans it out to one apply per scope so that YANG validation still runs (the code comment says exactly that). An empty patch isn't "no work", it's "full validation with nothing to change". Per Once per invocation — Per scope inside GCU — 4 full ConfigDB dumps ( I verified those per-scope counts by instrumenting the real apply path with a stubbed ConfigDB. So ~12 dumps and ~6 full-config YANG validations for a patch that changes nothing. At the ~0.48 s/dump measured earlier, the dumps alone are ≈5.8 s of the ~10.5 s; the rest is YANG validation. This is identical upstream: master has the same empty-case fan-out and the same 4 dumps + 1 validation per scope, and the same instrumented path shows no measurable difference (0.375 s vs 0.377 s over 7 iterations, ConfigDB reads stubbed to isolate the CPU/YANG work). One genuine difference worth flagging: master's Possible follow-up, separate from this PR: |
6 participants
Backport GCU perf dependencies: sonic-yang-mgmt helpers + libyang
must_size()patch to 202405This is Layers 2 + 3 of a 2-PR coordinated backport of upstream GCU perf improvements. The Layer 1 code lives at Azure/sonic-utilities.msft#417 (now at parts 1-8, includes the #4335 400G config-apply fix — see below).
What's in this PR (3 upstream cherry-picks)
sonic-yang-mgmt: uses clause with leaf-list, choice not honored— required by the perf helpers to correctly walk grouped schema (leaf-list + choice underuses).sonic-yang-mgmt: Generic Config Updater - performance dependencies— the primary perf-dependency payload:sonic_yang_path.py— YANG path-manipulation helpersfind_schema_dependencies()— schema dep index with cachingfind_schema_must_count()— must-constraint counting (needs Layer 3)sonic-yang-mgmt: add quiet= kwarg to SonicYangExtMixin.loadData— quiets the logs on the hot loadData path invoked repeatedly by the sorter.Layer 3 —
src/libyang/patch/libyang-leaf-must.patch: exposesSchema_Node_Leaf::must_size()on the libyang 1.0.73 swig CPP binding. Without this, Layer 2'sfind_schema_must_count()raisesAttributeErrorat runtime.Cherry-pick provenance
These are clean cherry-picks — content-identical to upstream. Unlike the companion sonic-utilities.msft#417, where 202405's GCU had diverged far enough that each upstream change had to be re-expressed by hand, all three commits here applied unmodified.
7c9d704bb9cab5bd72c0dbc41a2e016924f47042bbf2edddd9baf8Verified file-by-file against those upstream commits: all 22 file diffs are content-identical, and every file's +/− line counts match upstream exactly. 15 are byte-identical including
@@hunk headers; the other 7 differ only in hunk line offsets, because the surrounding 202405 code sits at different line numbers. Each upstream PR was taken whole — no files or hunks dropped, and no 202405-specific code added.Why all three layers must land together
The three layers form a runtime dependency chain. Any partial combination breaks GCU on 202405:
AttributeError: 'SonicYang' object has no attribute 'find_schema_dependencies'on GCU initAttributeError: 'Schema_Node_Leaf' object has no attribute 'must_size'during patch validationTo guarantee the 202405 buildimage
HEADis never in a broken intermediate state, Layer 1 (#417) is merged first onsonic-utilities.msft, then this PR is rebased to bumpsrc/sonic-utilitiesto that merge SHA so Layers 1+2+3 land in one atomic buildimage commit. See "Merge order" below.Validation (all 3 layers combined — refreshed on parts 1-8 of #417)
Full test matrix (562 total checks, 0 failures) lives in Azure/sonic-utilities.msft#417. Headline verified numbers on
str3-7800-lc3-1(asic0, SONiC.20240532.59):scale_50_add(3-iter mean)scale_100_add(3-iter mean)scale_200_add(3-iter mean)scale_500scale_1000variance_100(5-iter mean)Small-op isolated coverage (3 iters each):
op_add_20,op_replace_20,op_remove_20,op_mixed_30— 12/12 pass post part 8.Real (non-dry-run) end-to-end patches: 8/8 pass on
str3-7800-lc3-1— including100g_config,400g_config,400g_add_v2,queue_add,multi_asic,multi_asic_revert,stress,stress_revert.Unit tests: 409 pytest + 81 subtests = 490 pass, 0 fail across all 8 files in
tests/generic_config_updater/.Malformed patches (invalid JSON, invalid op, dangling leafref, schema-value violation): all produce graceful rc=2 errors with no python tracebacks.
Real MOR add-cluster scaling (validates the combined 3-layer stack)
The full L1+L2+L3 stack was additionally measured against the real MOR add-cluster workflow using
test_max_mors_under_budgetfrom sonic-mgmt #26274,on
str3-7800-lc4-1under a 1800 s budget. Six full test executions (stock.59vs this stack,across two ASICs), all PASSED — full tables and analysis in Azure/sonic-utilities.msft#417.
.59applyMarginal cost per MOR drops 37.8 s → 0.89 s (42× slope reduction); GCU change count stays
flat at 20 from N=1 through N=10 while the input patch grows ~9×. The speedup widens with N and
is still climbing at N=14 — the advantage grows with cluster size rather than being a fixed
constant. An empty
[]patch costs ~10.4–10.8 s in both states, confirming the fixedoverhead is untouched and the gain is attributable to the sorter.
Because Layer 3 (
must_size()) is a hard prerequisite for the L1 sorter, these MOR numbers areonly obtainable with this PR merged — they are as much a validation of #2733 as of #417.
400G config-apply — resolved by #4335 (cherry-picked into #417 as part 6)
The earlier reported
KeyError('PORTCHANNEL_MEMBER')on 400G port-add viaconfig apply-patchis fixed. Root cause was upstream issue #24464 (leaf-list handling regression introduced by #3831); upstream fix #4335 was cherry-picked into #417 as part 6 and independently verified against the 400G real-config patches:400g_config.patch: 27 s / 25 moves, no regression400g_add_v2.patch: passes cleanlyNo open perf/correctness caveats remain for this backport.
Environment
Tested on str3-7800-lc3-1 (Arista 7800R3A-36DM2-D36 chassis LC, SONiC.20240532.59, Python 3.11.2, libyang 1.0.73 with this PR's patch applied). Wheel rebuilt via
make -C src/sonic-yang-mgmtand installed on DUT for perf runs.Files
19 files changed:
src/sonic-yang-mgmt/**(Layer 2 python + tests) andsrc/libyang/patch/libyang-leaf-must.patch+src/libyang/patch/series(Layer 3). Two extra test-fixture JSON edits undersrc/sonic-yang-models/tests/needed for the new dependency semantics.Merge order (single atomic buildimage merge)
sonic-utilities.msft:202405→ produces a merge SHAsrc/sonic-utilitiessubmodule bump to that merge SHA on top of the existing Layer 2 + Layer 3 commits202405picks up Layers 1 + 2 + 3 in one atomic commit; no image build off202405is ever in a broken intermediate state