[202405][GCU perf backport #3831] Config-threading refactor (part 1/N)#417
Open
rimunagala wants to merge 4 commits into
Open
[202405][GCU perf backport #3831] Config-threading refactor (part 1/N)#417rimunagala wants to merge 4 commits into
rimunagala wants to merge 4 commits into
Conversation
…r: fetch config once per patch, thread through ChangeApplier Backport of the config-threading portion of upstream sonic-net/sonic-utilities PR #3831 to the 202405 branch, mirroring the 202412 auto-backport (PR Azure#254). Motivation: On 202405 the ChangeApplier.apply() path calls get_config_db_as_json() twice per JsonChange (once to seed the deepcopy, once for the post-apply sanity check). At MOR scale (N=14 => 381 changes) this dominates wall clock time and yields the flat ~1.47s/change floor observed in the sonic-mgmt scaling test. This change: * JsonChange.apply(config) -> JsonChange.apply(config, in_place=False) * DryRunConfigWrapper.apply_change_to_config_db(change) -> apply_change_to_config_db(current_config_db, change) and returns the updated dict. * ChangeApplier.apply(change) -> apply(current_configdb, change) -> dict - drops the deepcopy (uses in_place=False on JsonChange.apply) - drops the redundant post-apply get_config_db_as_json + jsondiff sanity check; replaces it with a 1s sleep race-guard (matches upstream #3831 behavior; upstream SONiC issue tracks proper fix). - returns the updated in-memory config dict. * DryRunChangeApplier.apply likewise threads and returns config. * PatchApplier.apply now fetches old_config once, seeds current_config from it, and threads current_config = self.changeapplier.apply( current_config, change) through the change loop. Tests updated to match new signatures: * change_applier_test.py: mock_obj.apply(config, in_place), applier call sites pass current_config, DryRunChangeApplier test updated. * multiasic_change_applier_test.py: @patch targets moved from change_applier.get_config_db_as_json to gu_common.get_config_db_as_json (since change_applier no longer re-exports it), applier call sites updated, `import copy` added. * generic_updater_test.py: TestPatchApplier now asserts changeapplier.apply.assert_called() and the mock side_effect accepts (current_config, change) and returns the config through. * gu_common_test.py: DryRunConfigWrapper multi-call test threads the returned config through the loop. Deliberately out of scope for this commit (will land in follow-ups): * skip_sort_tables.txt handling (perpetuates a shortcut we want to eliminate on 202412; not required for perf win). * illegal_dataacl_check (comes from upstream PR #3668, not #3831). * PathAddressing xpath cache rewrite and find_ref_paths batch API. * BulkLowLevelMoveGenerator family (patch_sorter additive changes). * extract_scope multi-ASIC guard. 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>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
…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>
…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>
…+ 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>
rimunagala
marked this pull request as ready for review
July 22, 2026 19:34
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
rimunagala
force-pushed
the
rimunagala/202405-gcu-3831-port
branch
from
July 23, 2026 16:32
0acf307 to
a205f2b
Compare
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.
Summary
Backports upstream sonic-utilities
#3831(GCU perf refactor) to202405in 4 parts:Diff vs
Azure/sonic-utilities.msft:202412for ported prod files:change_applier.py,patch_sorter.py: 0 (identical)gu_common.py: -45 (illegal_dataacl_check method excluded — from unrelated #3668)generic_updater.py: -29 (skip_sort_tables + extract_scope multi-ASIC guard excluded — out of scope)Requires coordinating buildimage-msft PR #2733 (3-layer YANG DFS enhancement).
Perf validation (str3-7800-lc3-1, single asic0 scope)
Mini-battery (3 iters × 3 scenarios: portchannel_add, mirror_session_add, acl_table_add):
Extended battery (13 scenarios × 5 iters):
scale_add_50015.2×,real_apply_314.8×,op_replace_2012.1×400G bring-up (
C_400g_config_noBufferPG.json): 58.5s (vs stock 109.7s), see caveat below.Testing done
sudo config apply-patchround-trips OK for portchannel/mirror/ACL/queue/buffer-profileextract_scopeconfig replaceon real_rollback_3 succeedsKnown caveat — 400G config-apply
400G port-add via
config apply-patchfails withKeyError('PORTCHANNEL_MEMBER')inRemoveCreateOnlyDependencyMoveValidator._validate_member. Root cause is DFS visit order inpatch_sorter.pyon 400G's larger move set (8 lanes vs 100G's 2) — the validator inspectsPORTCHANNEL_MEMBERbefore the parent table is populated.Applying the natural fix (upstream #4668 try/except) makes the DFS backtrack instead of aborting, but on 400G the backtrack space is exponential — process hangs at 99% CPU (py-spy shows 50+ deep recursion, each node full-YANG re-parse via
FullConfigMoveValidator). Full 5-PR chain (+#4118 +#26563 backport) has same DFS explosion.100G/breakout/all other scenarios unaffected. 400G port additions must currently go through the native
config interface breakoutpath (which does not use GCU) until the follow-up fix lands.Follow-up: separate PR planned to address DFS-explosion class of bug (needs deeper upstream fix chain analysis).
Signed-off-by: Rithvick Reddy Munagala rimunagala@microsoft.com