Skip to content

[action] [PR:3831] Generic Configuration Updater (GCU) performance enhancements#254

Merged
mssonicbld merged 1 commit into
Azure:202412from
mssonicbld:cherry/msft-202412/3831
Nov 9, 2025
Merged

[action] [PR:3831] Generic Configuration Updater (GCU) performance enhancements#254
mssonicbld merged 1 commit into
Azure:202412from
mssonicbld:cherry/msft-202412/3831

Conversation

@mssonicbld

Copy link
Copy Markdown
Collaborator

Important: Please review each commit (including commit message) individually. Looking at the patch-set as a whole may cause confusion.

What I did

Generic Configuration Updater is extremely slow, using the python profiler it was possible to determine the worst offenders where changes could be made without affecting the overall algorithm and HLD design documentation.

Brief overview of changes:

  • Prevent copy.deepcopy() calls where possible
  • Don't run validation twice back to back
  • Move configdb path <> xpath conversion logic to sonic-yang-mgmt where it belongs and enhance it to support schema conversion (not just data) and add caching.
  • Sort table keys by the number of schema backlinks and must statements for the node to try better guess the right order of the patches to generate rather than doing it in alphabetical order which is likely to cause validation failures.
  • Add ability to Group patches together in some commits where its known they will not cause issues, these are things like grouping parameter updates under the same key.
  • When applying changes, do not re-read the configuration from redis twice between each applied patch (this is extremely slow, and actually hid a race condition). We are mutating the configuration and a lock is held so we know the expected before and after. There is still a final validation to ensure something didn't go sideways.

Dependencies:

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).

How I did it

Profiling via cProfiler

How to verify it

Run sonic-utilities test cases, they still pass

Previous command output (if the output of a command-line utility has changed)

N/A

New command output (if the output of a command-line utility has changed)

N/A

Fixes sonic-net/sonic-buildimage#22372

***Important***: Please review each commit (including commit message) individually.  Looking at the patch-set as a whole may cause confusion.

#### What I did

Generic Configuration Updater is extremely slow, using the python profiler it was possible to determine the worst offenders where changes could be made without affecting the overall algorithm and HLD design documentation.

Brief overview of changes:
* Prevent copy.deepcopy() calls where possible
* Don't run validation twice back to back
* Move configdb path <> xpath conversion logic to sonic-yang-mgmt where it belongs and enhance it to support schema conversion (not just data) and add caching.
* Sort table keys by the number of schema backlinks and must statements for the node to try better guess the right order of the patches to generate rather than doing it in alphabetical order which is likely to cause validation failures.
* Add ability to Group patches together in some commits where its known they will not cause issues, these are things like grouping parameter updates under the same key.
* When applying changes, do not re-read the configuration from redis twice between each applied patch (this is **extremely** slow, and actually hid a race condition).  We are mutating the configuration and a lock is held so we know the expected before and after.  There is still a final validation to ensure something didn't go sideways.

Dependencies:
 * sonic-yang-mgmt enhancements: sonic-net/sonic-buildimage#22254
 * sonic-yang-mgmt parse uses/grouping: sonic-net/sonic-buildimage#21907
 * sonic-utilities rely on sonic-yang-mgmt uses/grouping handling: sonic-net/sonic-utilities#3814

Stats below ... (stats need both this and the sonic-utilities PR to be relevant)...

<ins>**Original Performance:**</ins>
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
```

<ins>**With Patch**:</ins>
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).

#### How I did it

Profiling via cProfiler

#### How to verify it

Run sonic-utilities test cases, they still pass

#### Previous command output (if the output of a command-line utility has changed)

N/A

#### New command output (if the output of a command-line utility has changed)

N/A

Fixes sonic-net/sonic-buildimage#22372
@mssonicbld

Copy link
Copy Markdown
Collaborator Author

Original PR: sonic-net/sonic-utilities#3831

@mssonicbld

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
mssonicbld merged commit 3967604 into Azure:202412 Nov 9, 2025
4 of 6 checks passed
rimunagala added a commit to rimunagala/sonic-utilities.msft that referenced this pull request Jul 21, 2026
…he + batch find_ref_paths

Backport of the gu_common.py PathAddressing portion of upstream
sonic-net/sonic-utilities PR #3831 to 202405, mirroring 202412
auto-backport (Azure/sonic-utilities.msft PR Azure#254).

Motivation:
On 202405, PathAddressing rebuilds its sonic-yang xpath token index
from scratch on every conversion call. At MOR scale (14 hosts *
27+ paths per patch = several hundred conversions per apply) this
produces the O(N) yang-tree-walk cost observed as the second-largest
contributor to GCU wall-clock time after the double get_config_db_as_json
addressed in part 1.

Also, find_ref_paths(path, ...) took a single path and had to reload
config for every call. The batched find_ref_paths(paths, ...) walks
config once per batch.

This change:
  * PathAddressing xpath cache rewrite: retire the ~500 lines of
    _get_xpath_tokens_from_* / _get_path_tokens_from_* walkers +
    _get_*_model / _find_leafref_paths helpers. Replace with a
    hash-keyed loadData cache so repeated conversions on the same
    config skip the yang reload. Public convert_path_to_xpath,
    convert_xpath_to_path, get_path_tokens, get_xpath_tokens,
    create_path, has_path, get_from_path, is_config_different
    signatures unchanged.
  * find_ref_paths(path, ...) -> find_ref_paths(paths, ...) batch
    API. Accepts either a single path (str) or a list of paths.
  * New helpers: configdb_sorted_keys_by_backlinks and configdb_sort_cmp
    used by patch_sorter's Bulk generators (added in part 3).
  * DryRunConfigWrapper.apply_change_to_config_db signature retained
    from part 1 (current_config_db threaded, returns updated dict).
  * JsonChange.apply(config, in_place=False) retained from part 1.

Test file wholesale-replaced from 202412 (adds caching-behavior tests
test_validate_config_db_config__same_config_called_twice__loadData_called_once,
test_find_ref_paths__after_validate_same_config__loadData_skipped, etc;
retires create_xpath / walker tests whose helpers are being deleted).

Deliberately excluded (not part of #3831):
  * ConfigWrapper.illegal_dataacl_check (from upstream PR #3668).

Reference: Azure/sonic-utilities.msft PR Azure#254 (202412 auto-backport of
upstream sonic-net/sonic-utilities #3831).

Signed-off-by: Rithvick Reddy Munagala <rimunagala@microsoft.com>
rimunagala added a commit to rimunagala/sonic-utilities.msft that referenced this pull request Jul 21, 2026
…oup + Bulk generators + validator refactor

Backport of the patch_sorter.py portion of upstream sonic-net/sonic-utilities
PR #3831 to 202405, mirroring 202412 auto-backport (Azure/sonic-utilities.msft PR Azure#254).

Motivation:
On 202405 the move-generation engine emits one JsonMove per individual
field/key delta, and each JsonMove goes through full-graph validation
independently. At MOR scale (14 hosts) the sorter emits several hundred
moves, and validators reload sonic-yang xpath state on every call.
Combined with the per-move DB roundtrip removed in part 1, this is the
second-largest contributor to observed GCU wall-clock time.

This change ports the sorter/validator rewrite:

New:
  * JsonMoveGroup - a mergeable group of JsonMoves. apply()/undo()
    fold a whole group into config in one pass, in_place-aware.
  * BulkLowLevelMoveGenerator, BulkKeyLevelMoveGenerator,
    BulkKeyGroupLowLevelMoveGenerator - emit JsonMoveGroups that
    coalesce many single-field/key deltas into one add/remove/replace
    move per subtree, with restricted-only fallback for correctness.
  * MoveWrapper.apply_move / undo_move now take in_place: bool.
  * Simulator.simulate / undo_simulate now take in_place: bool.

Refactor:
  * All *MoveValidator.validate signatures unified to
    (group: JsonMoveGroup, diff, simulated_config) - the simulated
    config is now computed once by the caller and passed in, instead
    of each validator recomputing it.
  * _extend_moves(moveGroup) and target_in_required_pattern helpers.
  * KeyLevelMoveGenerator / LowLevelMoveGenerator retained but now
    delegate to the Bulk variants under the hood via generate_groups
    with restricted_only fallback.
  * find_ref_paths now called with a paths batch and reload_config
    hint at PatchSorter._validate_paths_config; the reload skip is
    what unlocks the xpath cache added in part 2.

Retired:
  * SingleRunLowLevelMoveGenerator - internal-only, replaced by
    BulkLowLevelMoveGenerator's generate_remove/replace/add.

External API preserved:
  * StrictPatchSorter, NonStrictPatchSorter, PatchSorter,
    ConfigSplitter, TablesWithoutYangConfigSplitter,
    IgnorePathsFromYangConfigSplitter (the six symbols imported
    by generic_updater.py) - all signatures unchanged.

Test file wholesale-replaced from 202412 - covers the new Bulk
generators, JsonMoveGroup, and updated validator signatures.

Reference: Azure/sonic-utilities.msft PR Azure#254 (202412 auto-backport of
upstream sonic-net/sonic-utilities #3831).

Signed-off-by: Rithvick Reddy Munagala <rimunagala@microsoft.com>
rimunagala added a commit to rimunagala/sonic-utilities.msft that referenced this pull request Jul 21, 2026
…+ missing imports + missing test fixtures

Follow-up to parts 1-3 after cross-checking against PR Azure#254 file list.
No functional changes to the ported feature - only completes the port.

Fixes:

1. gu_common.py: remove call site of deleted illegal_dataacl_check
   Part 2 removed the illegal_dataacl_check method as out-of-scope
   (from upstream PR #3668, not #3831) but left the caller in
   validate_field_operation, which would AttributeError at runtime.

2. generic_updater.py: add missing 'import jsonpatch' and add
   'JsonChange' to the .gu_common import list. This is a pre-existing
   latent bug on 202405: PatchApplier.apply(patch, sort=False) uses
   JsonChange(jsonpatch.JsonPatch([element])) but neither symbol was
   imported. NameError only surfaces when the sort=False path is
   exercised. Upstream 202412 already imports both.

3. tests/generic_config_updater/gutest_helpers.py: add JsonMoveGroup
   import + create_side_effect_skiplastarg_dict,
   create_side_effect_skipfirstarg_dict,
   create_side_effect_jsonmovegroup_dict factories. These helpers
   are referenced by the 202412-shape patch_sorter_test.py added in
   part 3 - without them, patch_sorter_test collection fails.

4. tests/generic_config_updater/gcu_feature_patch_application_test.py:
   drop @patch('generic_config_updater.change_applier.get_config_db_as_json',
   ...) decorators. Part 1 dropped the get_config_db_as_json re-export
   from change_applier.py; mock decorators pointing at the removed
   attribute would AttributeError at test-setup time.

5. tests/generic_config_updater/files/patch_sorter_test_success.json:
   refresh golden fixture. The Bulk generators added in part 3 emit
   coalesced 'replace' moves on whole arrays where the old sorter
   emitted a sequence of per-element 'add' moves - the e2e success
   tests compare emitted output to this fixture.

6. tests/generic_config_updater/change_applier_test.py: drop stray
   local `running_config = {}` that shadowed the module-level global
   (functionally identical, but keeps parity with 202412 shape).

After these fixes, our branch's diff vs Azure/sonic-utilities.msft:202412
for the ported prod files is:
  change_applier.py: 0 (identical)
  patch_sorter.py:   0 (identical)
  gu_common.py:      -45 (illegal_dataacl_check method only)
  generic_updater.py: -29 (skip_sort_tables block + extract_scope
                          multi-ASIC guard + import fnmatch)
And for test files, only the tests exercising the three deliberately
excluded features differ.

Signed-off-by: Rithvick Reddy Munagala <rimunagala@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant