From d508fcffc27d6d4a2c2ac092265c17438769251c Mon Sep 17 00:00:00 2001 From: rimunagala Date: Thu, 10 Sep 2026 16:40:53 -0700 Subject: [PATCH 1/7] Fix JsonMove._get_value to Support Both String and Integer List Indices (#4237) (#4811) What I did: Issue: #4221 Updated JsonMove._get_value to handle both string and integer indices when traversing lists in config data. Adjusted related unit tests to reflect the new behavior. How I did it: Modified the traversal logic to convert string tokens to integers when accessing lists, allowing both "1" and 1 as valid indices. Removed the test expecting a TypeError for integer indices and added assertions for both string and integer index access. How to verify it: Patched change in lab device, confirmed. admin@STR-SN5640-RDMA-1:~$ cat /usr/local/lib/python3.11/dist-packages/generic_config_updater/patch_sorter.py | grep -C 2 "int(token)" for token in tokens: if isinstance(config, list): token = int(token) config = config[token] admin@STR-SN5640-RDMA-1:~$ cat t_tc_to_queue_map_modify.json [ { "op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8" }, { "op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7" } ] admin@STR-SN5640-RDMA-1:~$ sudo config apply-patch -v t_tc_to_queue_map_modify.json Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}, {"op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Patch Applier: localhost: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: localhost: sorting patch updates. Patch Sorter - Strict: Validating patch is not making changes to tables without YANG models. Patch Sorter - Strict: Validating target config according to YANG models. Patch Sorter - Strict: Sorting patch updates. Patch Applier: The localhost patch was converted into 1 change: Patch Applier: localhost: applying 1 change in order: Patch Applier: * [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}, {"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}] Patch Applier: localhost: verifying patch updates are reflected on ConfigDB. Patch Applier: localhost patch application completed. Patch applied successfully. Also run the updated unit tests and all tests should pass, confirming the fix. (cherry picked from commit 40260d5b24293b3627fd4b46051cc6573e182cfe) Signed-off-by: rimunagala Co-authored-by: Xincun Li <147451452+xincunli-sonic@users.noreply.github.com> --- generic_config_updater/patch_sorter.py | 3 +- .../test_patch_sorter_get_value.py | 55 +++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 tests/generic_config_updater/test_patch_sorter_get_value.py diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 4a1625b10..6411dbba0 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -98,10 +98,9 @@ def _to_jsonpatch_operation(self, diff, op_type, current_config_tokens, target_c @staticmethod def _get_value(config, tokens): for token in tokens: - if isinstance(token, str) and token.isnumeric(): + if isinstance(config, list): token = int(token) config = config[token] - return copy.deepcopy(config) @staticmethod diff --git a/tests/generic_config_updater/test_patch_sorter_get_value.py b/tests/generic_config_updater/test_patch_sorter_get_value.py new file mode 100644 index 000000000..79d31ca69 --- /dev/null +++ b/tests/generic_config_updater/test_patch_sorter_get_value.py @@ -0,0 +1,55 @@ +import unittest +from generic_config_updater.patch_sorter import JsonMove + + +class TestJsonMoveGetValue(unittest.TestCase): + def setUp(self): + self.config = { + "table1": { + "key1": { + "field": "value1" + }, + "1": { + "field": "value2" + } + }, + "table2": [ + {"name": "item0"}, + {"name": "item1"} + ] + } + + def test_get_value_dict(self): + tokens = ["table1", "key1", "field"] + self.assertEqual(JsonMove._get_value(self.config, tokens), "value1") + + def test_get_value_dict_numeric_string(self): + tokens = ["table1", "1", "field"] + self.assertEqual(JsonMove._get_value(self.config, tokens), "value2") + + def test_get_value_list(self): + # Should allow both int and string index for list + tokens = ["table2", 1, "name"] + self.assertEqual(JsonMove._get_value(self.config, tokens), "item1") + tokens = ["table2", "1", "name"] + self.assertEqual(JsonMove._get_value(self.config, tokens), "item1") + + def test_get_value_missing_key(self): + tokens = ["table1", "not_exist"] + with self.assertRaises(KeyError): + JsonMove._get_value(self.config, tokens) + + def test_get_value_list_invalid_index(self): + tokens = ["table2", "10", "name"] + with self.assertRaises(IndexError): + JsonMove._get_value(self.config, tokens) + + def test_get_value_non_container(self): + tokens = ["table1", "key1", "field", "extra"] + with self.assertRaises(TypeError): + JsonMove._get_value(self.config, tokens) + + def test_get_value_dict_numeric_keys(self): + self.config["7"] = {"8": "30"} + tokens = ["7", "8"] + self.assertEqual(JsonMove._get_value(self.config, tokens), "30") From f27fda0ca028044b2ed2647356c370e10e00a93d Mon Sep 17 00:00:00 2001 From: rimunagala Date: Thu, 10 Sep 2026 22:32:40 -0700 Subject: [PATCH 2/7] GCU: Add path tracing support (#4317) (#4812) GCU uses a very complex set of generators and validators. When a suboptimal plan is created, it can be hard to determine why the path was generated the way it was and how best to optimize the generators. This patch adds the ability to pass an IO object to all attempted paths with annotations such as `valid`, `invalid`, `recurse_reject` (source and target were already attempted), and `path_issue` (patch was good, but a generator further down the hierarchy failed). It will also list the generator used for each patch, and if `invalid` it will list the validator which failed it with a possibly extended error message. For callers to `config replace` or `config apply-patch` a new command line option of `--path-trace` which takes a filename to dump the json output. This patch also increases the diff output shown during test run failures to make it easier to debug issues based on logging generated via CI/CD. (cherry picked from commit 369e703e41af6e5ab2bc380f1a97ebcbddd4e3a5) Signed-off-by: rimunagala Co-authored-by: Brad House - Nexthop --- config/main.py | 77 +++++- generic_config_updater/generic_updater.py | 104 +++++-- generic_config_updater/gu_common.py | 2 +- generic_config_updater/patch_sorter.py | 258 +++++++++++++----- tests/config_test.py | 256 ++++++++++++++++- tests/conftest.py | 3 + .../generic_updater_test.py | 41 ++- .../generic_config_updater/gutest_helpers.py | 4 +- .../patch_sorter_test.py | 212 +++++++------- 9 files changed, 721 insertions(+), 236 deletions(-) diff --git a/config/main.py b/config/main.py index 592b7284e..9454610c3 100644 --- a/config/main.py +++ b/config/main.py @@ -33,6 +33,7 @@ from sonic_py_common.general import getstatusoutput_noshell from sonic_py_common.interface import get_interface_table_name, get_port_table_name, get_intf_longname from sonic_yang_cfg_generator import SonicYangCfgDbGenerator +from typing import IO, Optional from utilities_common import util_base from swsscommon import swsscommon from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector, ConfigDBPipeConnector, \ @@ -1512,12 +1513,21 @@ def multiasic_save_to_singlefile(db, filename): os.fsync(file.fileno()) -def apply_patch_wrapper(args): - return apply_patch_for_scope(*args) +def apply_patch_wrapper(args, **kwargs): + return apply_patch_for_scope(*args, **kwargs) # Function to apply patch for a single ASIC. -def apply_patch_for_scope(scope_changes, results, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_path): +def apply_patch_for_scope( + scope_changes, + results, + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_path, + trace_io: Optional[IO] = None, +): scope, changes = scope_changes # Replace localhost to DEFAULT_NAMESPACE which is db definition of Host if scope.lower() == HOST_NAMESPACE or scope == "": @@ -1534,7 +1544,8 @@ def apply_patch_for_scope(scope_changes, results, config_format, verbose, dry_ru verbose, dry_run, ignore_non_yang_tables, - ignore_path) + ignore_path, + trace_io=trace_io) results[scope_for_log] = {"success": True, "message": "Success"} log.log_notice(f"'apply-patch' executed successfully for {scope_for_log} by {changes} in thread:{thread_id}") except Exception as e: @@ -2007,8 +2018,26 @@ def print_dry_run_message(dry_run): @click.option('-n', '--ignore-non-yang-tables', is_flag=True, default=False, help='ignore validation for tables without YANG models', hidden=True) @click.option('-i', '--ignore-path', multiple=True, help='ignore validation for config specified by given path which is a JsonPointer', hidden=True) @click.option('-v', '--verbose', is_flag=True, default=False, help='print additional details of what the operation is doing') +@click.option( + '-t', + '--path-trace', + type=click.Path(writable=True), + help='filename to write decision path trace for patch generation as JSON', + hidden=True, +) + @click.pass_context -def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang_tables, ignore_path, verbose): +def apply_patch( + ctx, + patch_file_path, + format, + dry_run, + parallel, + ignore_non_yang_tables, + ignore_path, + verbose, + path_trace, +): """Apply given patch of updates to Config. A patch is a JsonPatch which follows rfc6902. This command can be used do partial updates to the config with minimum disruption to running processes. It allows addition as well as deletion of configs. The patch file represents a diff of ConfigDb(ABNF) @@ -2023,6 +2052,10 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang patch_as_json = json.loads(text) patch_ops = patch_as_json + trace_io = None + if path_trace is not None: + trace_io = open(path_trace, 'w') + all_running_config = get_all_running_config() # Pre-process patch to append empty tables if required. @@ -2073,7 +2106,7 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang for scope_changes in changes_by_scope.items()] # Submit all tasks and wait for them to complete - futures = [executor.submit(apply_patch_wrapper, args) for args in arguments] + futures = [executor.submit(apply_patch_wrapper, args, trace_io=trace_io) for args in arguments] # Wait for all tasks to complete concurrent.futures.wait(futures) @@ -2084,11 +2117,15 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang config_format, verbose, dry_run, ignore_non_yang_tables, - ignore_path) + ignore_path, + trace_io=trace_io) # Check if any updates failed failures = [scope for scope, result in results.items() if not result['success']] + if trace_io is not None: + trace_io.close() + if failures: failure_messages = '\n'.join([f"- {failed_scope}: {results[failed_scope]['message']}" for failed_scope in failures]) raise GenericConfigUpdaterError(f"Failed to apply patch on the following scopes:\n{failure_messages}") @@ -2109,8 +2146,15 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang @click.option('-n', '--ignore-non-yang-tables', is_flag=True, default=False, help='ignore validation for tables without YANG models', hidden=True) @click.option('-i', '--ignore-path', multiple=True, help='ignore validation for config specified by given path which is a JsonPointer', hidden=True) @click.option('-v', '--verbose', is_flag=True, default=False, help='print additional details of what the operation is doing') +@click.option( + '-t', + '--path-trace', + type=click.Path(writable=True), + help='filename to output decision path trace for patch generation as JSON', + hidden=True, +) @click.pass_context -def replace(ctx, target_file_path, format, dry_run, ignore_non_yang_tables, ignore_path, verbose): +def replace(ctx, target_file_path, format, dry_run, ignore_non_yang_tables, ignore_path, verbose, path_trace): """Replace the whole config with the specified config. The config is replaced with minimum disruption e.g. if ACL config is different between current and target config only ACL config is updated, and other config/services such as DHCP will not be affected. @@ -2127,7 +2171,22 @@ def replace(ctx, target_file_path, format, dry_run, ignore_non_yang_tables, igno config_format = ConfigFormat[format.upper()] - GenericUpdater().replace(target_config, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_path) + trace_io = None + if path_trace is not None: + trace_io = open(path_trace, 'w') + + GenericUpdater().replace( + target_config, + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_path, + trace_io=trace_io, + ) + + if trace_io is not None: + trace_io.close() click.secho("Config replaced successfully.", fg="cyan", underline=True) except Exception as ex: diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index ac91f9ec0..b7a984455 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -5,6 +5,7 @@ from datetime import datetime, timezone from enum import Enum +from typing import IO, Optional from .gu_common import HOST_NAMESPACE, GenericConfigUpdaterError, EmptyTableError, ConfigWrapper, \ DryRunConfigWrapper, PatchWrapper, genericUpdaterLogging from .patch_sorter import StrictPatchSorter, NonStrictPatchSorter, ConfigSplitter, \ @@ -101,7 +102,7 @@ def __init__(self, self.patchsorter = patchsorter if patchsorter is not None else StrictPatchSorter(self.config_wrapper, self.patch_wrapper) self.changeapplier = changeapplier if changeapplier is not None else ChangeApplier(scope=self.scope) - def apply(self, patch, sort=True): + def apply(self, patch, sort=True, trace_io: Optional[IO] = None): scope = self.scope if self.scope else HOST_NAMESPACE self.logger.log_notice(f"{scope}: Patch application starting.") self.logger.log_notice(f"{scope}: Patch: {patch}") @@ -130,7 +131,7 @@ def apply(self, patch, sort=True): # Generate list of changes to apply if sort: self.logger.log_notice(f"{scope}: sorting patch updates.") - changes = self.patchsorter.sort(patch) + changes = self.patchsorter.sort(patch, trace_io=trace_io) else: self.logger.log_notice(f"{scope}: converting patch to JsonChange.") changes = [JsonChange(jsonpatch.JsonPatch([element])) for element in patch] @@ -166,7 +167,7 @@ def __init__(self, patch_applier=None, config_wrapper=None, patch_wrapper=None, self.config_wrapper = config_wrapper if config_wrapper is not None else ConfigWrapper(scope=self.scope) self.patch_wrapper = patch_wrapper if patch_wrapper is not None else PatchWrapper(scope=self.scope) - def replace(self, target_config): + def replace(self, target_config, trace_io: Optional[IO] = None): self.logger.log_notice("Config replacement starting.") self.logger.log_notice(f"Target config length: {len(json.dumps(target_config))}.") @@ -178,7 +179,7 @@ def replace(self, target_config): self.logger.log_debug(f"Generated patch: {patch}.") # debug since the patch will printed again in 'patch_applier.apply' self.logger.log_notice("Applying patch using 'Patch Applier'.") - self.patch_applier.apply(patch) + self.patch_applier.apply(patch, trace_io=trace_io) self.logger.log_notice("Verifying config replacement is reflected on ConfigDB.") new_config = self.config_wrapper.get_config_db_as_json() @@ -303,7 +304,7 @@ def __init__(self, self.scopelist = [HOST_NAMESPACE, *multi_asic.get_namespace_list()] super().__init__(patch_applier, config_wrapper, patch_wrapper, scope) - def replace(self, target_config): + def replace(self, target_config, trace_io: Optional[IO] = None): config_keys = set(target_config.keys()) missing_scopes = set(self.scopelist) - config_keys if missing_scopes: @@ -313,7 +314,7 @@ def replace(self, target_config): scope_config = target_config.pop(scope) if scope.lower() == HOST_NAMESPACE: scope = multi_asic.DEFAULT_NAMESPACE - ConfigReplacer(scope=scope).replace(scope_config) + ConfigReplacer(scope=scope).replace(scope_config, trace_io=trace_io) class MultiASICConfigRollbacker(FileSystemConfigRollbacker): @@ -420,11 +421,11 @@ def __init__(self, self.decorated_config_replacer = decorated_config_replacer self.decorated_config_rollbacker = decorated_config_rollbacker - def apply(self, patch): - self.decorated_patch_applier.apply(patch) + def apply(self, patch, sort=True, trace_io: Optional[IO] = None): + self.decorated_patch_applier.apply(patch, sort, trace_io=trace_io) - def replace(self, target_config): - self.decorated_config_replacer.replace(target_config) + def replace(self, target_config, trace_io: Optional[IO] = None): + self.decorated_config_replacer.replace(target_config, trace_io=trace_io) def rollback(self, checkpoint_name): self.decorated_config_rollbacker.rollback(checkpoint_name) @@ -451,13 +452,13 @@ def __init__(self, self.patch_wrapper = patch_wrapper self.config_wrapper = config_wrapper - def apply(self, patch): + def apply(self, patch, sort=True, trace_io: Optional[IO] = None): config_db_patch = self.patch_wrapper.convert_sonic_yang_patch_to_config_db_patch(patch) - Decorator.apply(self, config_db_patch) + Decorator.apply(self, config_db_patch, sort, trace_io=trace_io) - def replace(self, target_config): + def replace(self, target_config, trace_io: Optional[IO] = None): config_db_target_config = self.config_wrapper.convert_sonic_yang_to_config_db(target_config) - Decorator.replace(self, config_db_target_config) + Decorator.replace(self, config_db_target_config, trace_io=trace_io) class ConfigLockDecorator(Decorator): @@ -474,11 +475,11 @@ def __init__(self, scope=scope) self.config_lock = config_lock - def apply(self, patch, sort=True): - self.execute_write_action(Decorator.apply, self, patch) + def apply(self, patch, sort=True, trace_io: Optional[IO] = None): + self.execute_write_action(Decorator.apply, self, patch, sort, trace_io=trace_io) - def replace(self, target_config): - self.execute_write_action(Decorator.replace, self, target_config) + def replace(self, target_config, trace_io: Optional[IO] = None): + self.execute_write_action(Decorator.replace, self, target_config, trace_io=trace_io) def rollback(self, checkpoint_name): self.execute_write_action(Decorator.rollback, self, checkpoint_name) @@ -486,9 +487,9 @@ def rollback(self, checkpoint_name): def checkpoint(self, checkpoint_name): self.execute_write_action(Decorator.checkpoint, self, checkpoint_name) - def execute_write_action(self, action, *args): + def execute_write_action(self, action, *args, **kwargs): self.config_lock.acquire_lock() - action(*args) + action(*args, **kwargs) self.config_lock.release_lock() @@ -496,7 +497,14 @@ class GenericUpdateFactory: def __init__(self, scope=multi_asic.DEFAULT_NAMESPACE): self.scope = scope - def create_patch_applier(self, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_paths): + def create_patch_applier( + self, + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_paths, + ): self.init_verbose_logging(verbose) config_wrapper = self.get_config_wrapper(dry_run) change_applier = self.get_change_applier(dry_run, config_wrapper) @@ -523,7 +531,14 @@ def create_patch_applier(self, config_format, verbose, dry_run, ignore_non_yang_ return patch_applier - def create_config_replacer(self, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_paths): + def create_config_replacer( + self, + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_paths, + ): self.init_verbose_logging(verbose) config_wrapper = self.get_config_wrapper(dry_run) change_applier = self.get_change_applier(dry_run, config_wrapper) @@ -622,13 +637,44 @@ def __init__(self, generic_update_factory=None, scope=multi_asic.DEFAULT_NAMESPA self.generic_update_factory = \ generic_update_factory if generic_update_factory is not None else GenericUpdateFactory(scope=scope) - def apply_patch(self, patch, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_paths, sort=True): - patch_applier = self.generic_update_factory.create_patch_applier(config_format, verbose, dry_run, ignore_non_yang_tables, ignore_paths) - patch_applier.apply(patch, sort) - - def replace(self, target_config, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_paths): - config_replacer = self.generic_update_factory.create_config_replacer(config_format, verbose, dry_run, ignore_non_yang_tables, ignore_paths) - config_replacer.replace(target_config) + def apply_patch( + self, + patch, + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_paths, + sort=True, + trace_io: Optional[IO] = None, + ): + patch_applier = self.generic_update_factory.create_patch_applier( + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_paths, + ) + patch_applier.apply(patch, sort, trace_io=trace_io) + + def replace( + self, + target_config, + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_paths, + trace_io: Optional[IO] = None, + ): + config_replacer = self.generic_update_factory.create_config_replacer( + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_paths, + ) + config_replacer.replace(target_config, trace_io=trace_io) def rollback(self, checkpoint_name, verbose, dry_run, ignore_non_yang_tables, ignore_paths): config_rollbacker = self.generic_update_factory.create_config_rollbacker(verbose, dry_run, ignore_non_yang_tables, ignore_paths) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 4056e79ad..61aed505d 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -157,7 +157,7 @@ def validate_config_db_config(self, config_db_as_json): if not success: return success, error except sonic_yang.SonicYangException as ex: - return False, ex + return False, str(ex) return True, None diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 6411dbba0..807e1ad02 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -4,6 +4,7 @@ import sonic_yang from collections import deque, OrderedDict from enum import Enum +from typing import IO, Optional, Tuple from .gu_common import OperationWrapper, OperationType, GenericConfigUpdaterError, \ JsonChange, PathAddressing, genericUpdaterLogging @@ -321,7 +322,8 @@ class JsonMoveGroup: """ Group of JsonMove objects to be applied together """ - def __init__(self, move: JsonMove = None): + def __init__(self, generator_name: str, move: JsonMove = None): + self.generator_name = generator_name self.patches = [] if move is not None: self.append(move) @@ -464,14 +466,18 @@ def generate(self, diff): extended_moves.add(move) moves.extend(self._extend_moves(move, diff)) - def validate(self, move, diff): + def validate(self, move, diff) -> Tuple[bool, Optional[str]]: # Generate simulated config once, not once per validator as this performs # a deep copy simulated_config = move.apply(diff.current_config) for validator in self.move_validators: - if not validator.validate(move, diff, simulated_config): - return False - return True + success, errmsg = validator.validate(move, diff, simulated_config) + if not success: + error = f"{validator.__class__.__name__} failed" + if errmsg is not None: + error += ": " + errmsg + return False, error + return True, None def simulate(self, move, diff, in_place: bool = False): return diff.apply_move(move, in_place) @@ -496,7 +502,7 @@ def _extend_moves(self, moveGroup: JsonMoveGroup, diff) -> JsonMoveGroup: for move in moveGroup: for extender in self.move_extenders: for newmove in extender.extend(move, diff): - yield JsonMoveGroup(newmove) + yield JsonMoveGroup(f"{extender.__class__.__name__} extends {moveGroup.generator_name}", newmove) class JsonPointerFilter: """ @@ -743,7 +749,7 @@ def __init__(self, path_addressing): self.path_addressing = path_addressing self.create_only_filter = CreateOnlyFilter(path_addressing).get_filter() - def validate(self, group: JsonMoveGroup, diff, simulated_config): + def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]: # Note: group is not used by this validator current_config = diff.current_config target_config = diff.target_config # Final config after applying whole patch @@ -780,12 +786,12 @@ def validate(self, group: JsonMoveGroup, diff, simulated_config): if not self._validate_member(tokens, member_name, current_config, target_config, simulated_config, reload_config=reload_config): - return False + return False, None # After first call, no need to reload again reload_config = False - return True + return True, None def _validate_member(self, tokens, member_name, current_config, target_config, simulated_config, reload_config: bool = True): @@ -830,10 +836,10 @@ class DeleteWholeConfigMoveValidator: """ A class to validate not deleting whole config as it is not supported by JsonPatch lib. """ - def validate(self, group: JsonMoveGroup, diff, simulated_config): + def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]: if group.patches[0].op_type == OperationType.REMOVE and group.patches[0].path == "": - return False - return True + return False, None + return True, None class FullConfigMoveValidator: """ @@ -842,9 +848,9 @@ class FullConfigMoveValidator: def __init__(self, config_wrapper): self.config_wrapper = config_wrapper - def validate(self, move, diff, simulated_config): + def validate(self, move, diff, simulated_config) -> Tuple[bool, Optional[str]]: is_valid, error = self.config_wrapper.validate_config_db_config(simulated_config) - return is_valid + return is_valid, error class CreateOnlyMoveValidator: """ @@ -859,7 +865,7 @@ def __init__(self, path_addressing): # TODO: create-only fields are hard-coded for now, it should be moved to YANG models self.create_only_filter = CreateOnlyFilter(path_addressing).get_filter() - def validate(self, group: JsonMoveGroup, diff, simulated_config): + def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]: # NOTE: group not used by this validator # get create-only paths from current config, simulated config and also target config @@ -872,19 +878,19 @@ def validate(self, group: JsonMoveGroup, diff, simulated_config): for path in paths: tokens = self.path_addressing.get_path_tokens(path) if self._value_exist_but_different(tokens, diff.current_config, simulated_config): - return False + return False, None if self._value_added_but_parent_exist(tokens, diff.current_config, simulated_config): - return False + return False, None if self._value_removed_but_parent_remain(tokens, diff.current_config, simulated_config): - return False + return False, None # if parent of create-only field is added, create-only field should be the same as target # i.e. if field is deleted in target, it should be deleted in the move, or # if field is present in target, it should be present in the move if self._parent_added_child_not_as_target(tokens, diff.current_config, simulated_config, diff.target_config): - return False + return False, None - return True + return True, None def _parent_added_child_not_as_target(self, tokens, current_config, simulated_config, target_config): # if parent is not added, return false @@ -958,14 +964,14 @@ def __init__(self, path_addressing, config_wrapper): self.path_addressing = path_addressing self.config_wrapper = config_wrapper - def validate(self, group: JsonMoveGroup, diff, simulated_config): + def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]: reload_config = True # Note: all moves in a group are guaranteed to be the same operation type for move in group: if not self.__validate_move(move, diff, simulated_config, reload_config=reload_config): - return False + return False, None reload_config = False - return True + return True, None def __validate_move(self, move, diff, simulated_config, reload_config: bool = True): operation_type = move.op_type @@ -1116,8 +1122,8 @@ def __init__(self, path_addressing): def validate(self, group, diff, simulated_config): for move in group: if not self.__validate_move(move, diff, simulated_config): - return False - return True + return False, None + return True, None def __validate_move(self, move, diff, simulated_config): op_path = move.path @@ -1155,10 +1161,10 @@ def __init__(self, path_addressing): self.path_addressing = path_addressing self.identifier = RequiredValueIdentifier(path_addressing) - def validate(self, group: JsonMoveGroup, diff, simulated_config): + def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]: # ignore full config removal because it is not possible by JsonPatch lib if group.patches[0].op_type == OperationType.REMOVE and group.patches[0].path == "": - return + return False, None current_config = diff.current_config target_config = diff.target_config # Final config after applying whole patch @@ -1179,7 +1185,7 @@ def validate(self, group: JsonMoveGroup, diff, simulated_config): if actual_value is None: # current config does not have this value at all continue if actual_value != required_value: - return False + return False, None # If some changes to the requiring paths are still to take place and the move has changes # to the required path, reject the move @@ -1194,9 +1200,9 @@ def validate(self, group: JsonMoveGroup, diff, simulated_config): if simulated_value is None: # Simulated config does not have this value at all. continue if current_value != simulated_value and simulated_value != required_value: - return False + return False, None - return True + return True, None class TableLevelMoveGenerator: """ @@ -1216,11 +1222,11 @@ def __init__(self, path_addressing): def generate(self, diff): # Removing tables in current but not target for tokens in self._get_non_existing_tables_tokens(diff.current_config, diff.target_config, False): - yield JsonMoveGroup(JsonMove(diff, OperationType.REMOVE, tokens)) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.REMOVE, tokens)) # Adding tables in target but not current for tokens in self._get_non_existing_tables_tokens(diff.target_config, diff.current_config, True): - yield JsonMoveGroup(JsonMove(diff, OperationType.ADD, tokens, tokens)) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.ADD, tokens, tokens)) def _get_non_existing_tables_tokens(self, config1, config2, reverse): for table in self.path_addressing.configdb_sorted_keys_by_backlinks("/", config1, reverse=reverse): @@ -1251,13 +1257,13 @@ def generate(self, diff): table = tokens[0] # if table has a single key, delete the whole table because empty tables are not allowed in ConfigDB if len(diff.current_config[table]) == 1: - yield JsonMoveGroup(JsonMove(diff, OperationType.REMOVE, [table])) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.REMOVE, [table])) else: - yield JsonMoveGroup(JsonMove(diff, OperationType.REMOVE, tokens)) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.REMOVE, tokens)) # Adding keys in target but not current for tokens in self._get_non_existing_keys_tokens(diff.target_config, diff.current_config, reverse=True): - yield JsonMoveGroup(JsonMove(diff, OperationType.ADD, tokens, tokens)) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.ADD, tokens, tokens)) def _get_non_existing_keys_tokens(self, config1, config2, reverse): for table in self.path_addressing.configdb_sorted_keys_by_backlinks("/", config1, reverse=reverse): @@ -1295,7 +1301,7 @@ def generate(self, diff): prev_table = table prev_num_separators = num_separators if group is None: - group = JsonMoveGroup() + group = JsonMoveGroup(self.__class__.__name__) group.append(JsonMove(diff, OperationType.REMOVE, tokens)) @@ -1328,7 +1334,7 @@ def generate(self, diff): prev_table = table prev_num_separators = num_separators if group is None: - group = JsonMoveGroup() + group = JsonMoveGroup(self.__class__.__name__) group.append(JsonMove(diff, OperationType.ADD, tokens, tokens)) @@ -1523,7 +1529,7 @@ def __output_bulk_move(self, op, current_ptr, target_ptr, tokens, min_moves, res yield move def __output_bulk_add(self, current_ptr, target_ptr, tokens, min_moves, restricted_only): - group = JsonMoveGroup() + group = JsonMoveGroup(self.__class__.__name__,) for key in target_ptr: if current_ptr.get(key) is None and not self.__restricted_key(tokens, key, invert=restricted_only): tokens.append(key) @@ -1535,7 +1541,7 @@ def __output_bulk_add(self, current_ptr, target_ptr, tokens, min_moves, restrict yield group def __output_bulk_remove(self, current_ptr, target_ptr, tokens, min_moves, restricted_only): - group = JsonMoveGroup() + group = JsonMoveGroup(self.__class__.__name__,) for key in current_ptr: if target_ptr.get(key) is None and not self.__restricted_key(tokens, key, invert=restricted_only): tokens.append(key) @@ -1547,7 +1553,7 @@ def __output_bulk_remove(self, current_ptr, target_ptr, tokens, min_moves, restr yield group def __output_bulk_replace(self, current_ptr, target_ptr, tokens, min_moves, restricted_only): - group = JsonMoveGroup() + group = JsonMoveGroup(self.__class__.__name__,) for key in current_ptr: target_val = target_ptr.get(key) if (target_val is not None and target_val != current_ptr.get(key) and @@ -1614,7 +1620,7 @@ def generate(self, diff): for ref_path in self.path_addressing.find_ref_paths(member_path, current_config, reload_config=reload_config): - yield JsonMoveGroup(JsonMove(diff, OperationType.REMOVE, + yield JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.REMOVE, self.path_addressing.get_path_tokens(ref_path))) # No need to reload config after first call @@ -1742,7 +1748,10 @@ def _traverse_value(self, current_value, target_value, current_tokens, target_to if current_value == target_value: return - yield JsonMoveGroup(JsonMove(self.diff, OperationType.REPLACE, current_tokens, target_tokens)) + yield JsonMoveGroup( + self.__class__.__name__, + JsonMove(self.diff, OperationType.REPLACE, current_tokens, target_tokens), + ) def _traverse_current(self, ptr, current_tokens): if isinstance(ptr, list): @@ -1752,7 +1761,7 @@ def _traverse_current(self, ptr, current_tokens): if isinstance(ptr, dict): if len(ptr) == 0: - yield JsonMoveGroup(JsonMove(self.diff, OperationType.REMOVE, current_tokens)) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(self.diff, OperationType.REMOVE, current_tokens)) return for key in ptr: @@ -1769,7 +1778,7 @@ def _traverse_current(self, ptr, current_tokens): def _traverse_current_list(self, ptr, current_tokens): if len(ptr) <= 1: - yield JsonMoveGroup(JsonMove(self.diff, OperationType.REMOVE, current_tokens)) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(self.diff, OperationType.REMOVE, current_tokens)) return for index, val in enumerate(ptr): @@ -1779,7 +1788,7 @@ def _traverse_current_list(self, ptr, current_tokens): current_tokens.pop() def _traverse_current_value(self, val, current_tokens): - yield JsonMoveGroup(JsonMove(self.diff, OperationType.REMOVE, current_tokens)) + yield JsonMoveGroup(self.__class__.__name__, JsonMove(self.diff, OperationType.REMOVE, current_tokens)) def _traverse_target(self, ptr, current_tokens, target_tokens): if isinstance(ptr, list): @@ -1789,7 +1798,10 @@ def _traverse_target(self, ptr, current_tokens, target_tokens): if isinstance(ptr, dict): if len(ptr) == 0: - yield JsonMoveGroup(JsonMove(self.diff, OperationType.ADD, current_tokens, target_tokens)) + yield JsonMoveGroup( + self.__class__.__name__, + JsonMove(self.diff, OperationType.ADD, current_tokens, target_tokens), + ) return for key in ptr: @@ -1808,7 +1820,10 @@ def _traverse_target(self, ptr, current_tokens, target_tokens): def _traverse_target_list(self, ptr, current_tokens, target_tokens): if len(ptr) == 0: - yield JsonMoveGroup(JsonMove(self.diff, OperationType.ADD, current_tokens, target_tokens)) + yield JsonMoveGroup( + self.__class__.__name__, + JsonMove(self.diff, OperationType.ADD, current_tokens, target_tokens), + ) return for index, val in enumerate(ptr): @@ -1822,7 +1837,10 @@ def _traverse_target_list(self, ptr, current_tokens, target_tokens): current_tokens.pop() def _traverse_target_value(self, val, current_tokens, target_tokens): - yield JsonMoveGroup(JsonMove(self.diff, OperationType.ADD, current_tokens, target_tokens)) + yield JsonMoveGroup( + self.__class__.__name__, + JsonMove(self.diff, OperationType.ADD, current_tokens, target_tokens), + ) def _list_to_dict_with_count(self, items): counts = dict() @@ -2003,12 +2021,82 @@ def extend(self, move: JsonMove, diff): yield JsonMove(diff, OperationType.REMOVE, self.path_addressing.get_path_tokens(ref_path)) +class PatchStatus(str, Enum): + # Topmost Node + TOPLEVEL = "toplevel" + # During testing, should never see this in a final result + TESTING = "testing" + # This is the valid tree, the entire chain should be valid + VALID = "valid" + # Validation failed at this node + INVALID = "invalid" + # Validation failed at a child node + PATH_ISSUE = "path_issue" + # original and target diffs already seen, prevent recursion + RECURSE_REJECT = "recurse_reject" + + +class PatchSorterPath: + """Class containing path elements for debugging/tracking""" + def __init__(self, diff: Optional[Diff] = None, moves: Optional[JsonMoveGroup] = None): + if diff is not None: + self.status = PatchStatus.TOPLEVEL + self.patch = json.loads(jsonpatch.make_patch(diff.current_config, diff.target_config).to_string()) + self.children = [] + return + + if moves is None: + raise RuntimeError("Must specify diff or moves") + + self.generator = moves.generator_name + self.patch = json.loads(str(moves.get_jsonpatch())) + self.status = PatchStatus.TESTING + self.error = None + self.children = [] + + def __len__(self): + return len(self.children) + + def append(self, moves: JsonMoveGroup) -> 'PatchSorterPath': + item = PatchSorterPath(moves=moves) + self.children.append(item) + return item + + @staticmethod + def json_encoder(o): + if isinstance(o, PatchSorterPath): + if o.status == PatchStatus.TOPLEVEL: + mydict = { + "requested": o.patch, + "generated": o.children, + } + return mydict + + mydict = { + "generator": o.generator, + "status": o.status, + "patch": o.patch, + } + if o.children is not None and len(o.children): + mydict["children"] = o.children + if o.error is not None: + mydict["error"] = o.error + return mydict + raise TypeError(f'Type {type(o)} not serializable') + + class DfsSorter: - def __init__(self, move_wrapper): + def __init__(self, move_wrapper, path_trace: bool = False): self.visited = {} self.move_wrapper = move_wrapper + self.path_trace = path_trace + self.path_tracker = None + + def sort(self, diff, path_tracker: Optional[PatchSorterPath] = None): + if path_tracker is None and self.path_trace: + self.path_tracker = PatchSorterPath(diff=diff) + path_tracker = self.path_tracker - def sort(self, diff): if diff.has_no_diff(): return [] @@ -2020,21 +2108,40 @@ def sort(self, diff): moves = self.move_wrapper.generate(diff) for move in moves: - if self.move_wrapper.validate(move, diff): + path_item = None + if path_tracker is not None: + path_item = path_tracker.append(move) + + success, errmsg = self.move_wrapper.validate(move, diff) + if success: # NOTE: due to the recursive nature, we can't modify in-place as on error we will # receive "RuntimeError: dictionary changed size during iteration" new_diff = self.move_wrapper.simulate(move, diff, in_place=False) - new_moves = self.sort(new_diff) + new_moves = self.sort(new_diff, path_item) if new_moves is not None: + if path_item is not None: + path_item.status = PatchStatus.VALID return [move] + new_moves + else: + if path_item is not None: + if len(path_item.children): + path_item.status = PatchStatus.PATH_ISSUE + else: + path_item.status = PatchStatus.RECURSE_REJECT + else: + if path_item is not None: + path_item.status = PatchStatus.INVALID + path_item.error = errmsg return None class BfsSorter: - def __init__(self, move_wrapper): + def __init__(self, move_wrapper, path_trace: bool = False): self.visited = {} self.move_wrapper = move_wrapper + # Not currently supported + self.path_tracker = None def sort(self, diff): diff_queue = deque([]) @@ -2057,7 +2164,8 @@ def sort(self, diff): moves = self.move_wrapper.generate(diff) for move in moves: - if self.move_wrapper.validate(move, diff): + success, errmsg = self.move_wrapper.validate(move, diff) + if success: new_diff = self.move_wrapper.simulate(move, diff) new_prv_moves = prv_moves + [move] @@ -2068,10 +2176,12 @@ def sort(self, diff): class MemoizationSorter: - def __init__(self, move_wrapper): + def __init__(self, move_wrapper, path_trace: bool = False): self.visited = {} self.move_wrapper = move_wrapper self.mem = {} + # Not currently supported + self.path_tracker = None def sort(self, diff): if diff.has_no_diff(): @@ -2088,7 +2198,8 @@ def sort(self, diff): bst_moves = None for move in moves: - if self.move_wrapper.validate(move, diff): + success, errmsg = self.move_wrapper.validate(move, diff) + if success: new_diff = self.move_wrapper.simulate(move, diff) new_moves = self.sort(new_diff) if new_moves != None and (bst_moves is None or len(bst_moves) > len(new_moves)+1): @@ -2110,7 +2221,7 @@ def __init__(self, operation_wrapper, config_wrapper, path_addressing): self.config_wrapper = config_wrapper self.path_addressing = path_addressing - def create(self, algorithm=Algorithm.DFS): + def create(self, algorithm=Algorithm.DFS, path_trace: bool = False): move_generators = [RemoveCreateOnlyDependencyMoveGenerator(self.path_addressing), LowLevelMoveGenerator(self.path_addressing)] # TODO: Enable TableLevelMoveGenerator once it is confirmed whole table can be updated at the same time @@ -2133,11 +2244,11 @@ def create(self, algorithm=Algorithm.DFS): move_wrapper = MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, move_validators) if algorithm == Algorithm.DFS: - sorter = DfsSorter(move_wrapper) + sorter = DfsSorter(move_wrapper, path_trace=path_trace) elif algorithm == Algorithm.BFS: - sorter = BfsSorter(move_wrapper) + sorter = BfsSorter(move_wrapper, path_trace=path_trace) elif algorithm == Algorithm.MEMOIZATION: - sorter = MemoizationSorter(move_wrapper) + sorter = MemoizationSorter(move_wrapper, path_trace=path_trace) else: raise ValueError(f"Algorithm {algorithm} is not supported") @@ -2151,7 +2262,7 @@ def __init__(self, config_wrapper, patch_wrapper, inner_patch_sorter=None): self.patch_wrapper = patch_wrapper self.inner_patch_sorter = inner_patch_sorter if inner_patch_sorter else PatchSorter(config_wrapper, patch_wrapper) - def sort(self, patch, algorithm=Algorithm.DFS): + def sort(self, patch, algorithm=Algorithm.DFS, trace_io: Optional[IO] = None): current_config = self.config_wrapper.get_config_db_as_json() # Validate patch is only updating tables with yang models @@ -2169,7 +2280,7 @@ def sort(self, patch, algorithm=Algorithm.DFS): # Generate list of changes to apply self.logger.log_info("Sorting patch updates.") - changes = self.inner_patch_sorter.sort(patch, algorithm) + changes = self.inner_patch_sorter.sort(patch, algorithm, trace_io=trace_io) return changes @@ -2206,12 +2317,17 @@ def split_yang_non_yang_distinct_field_path(self, config): # Add to config_without_yang from config_with_yang tokens = self.path_addressing.get_path_tokens(path) - add_move = JsonMoveGroup(JsonMove(Diff(config_without_yang, config_with_yang), OperationType.ADD, tokens, - tokens)) + add_move = JsonMoveGroup( + self.__class__.__name__, + JsonMove(Diff(config_without_yang, config_with_yang), OperationType.ADD, tokens, tokens), + ) config_without_yang = add_move.apply(config_without_yang) # Remove from config_with_yang - remove_move = JsonMoveGroup(JsonMove(Diff(config_with_yang, {}), OperationType.REMOVE, tokens)) + remove_move = JsonMoveGroup( + self.__class__.__name__, + JsonMove(Diff(config_with_yang, {}), OperationType.REMOVE, tokens), + ) config_with_yang = remove_move.apply(config_with_yang) # Splitting the config based on 'ignore_paths_from_yang_list' can result in empty tables. @@ -2342,7 +2458,7 @@ def __init__(self, config_wrapper, patch_wrapper, config_splitter, change_wrappe self.change_wrapper = change_wrapper if change_wrapper else ChangeWrapper(patch_wrapper, config_splitter) self.inner_patch_sorter = patch_sorter if patch_sorter else PatchSorter(config_wrapper, patch_wrapper) - def sort(self, patch, algorithm=Algorithm.DFS): + def sort(self, patch, algorithm=Algorithm.DFS, trace_io: Optional[IO] = None): current_config = self.config_wrapper.get_config_db_as_json() target_config = self.patch_wrapper.simulate_patch(patch, current_config) @@ -2379,7 +2495,7 @@ def sort(self, patch, algorithm=Algorithm.DFS): # Generating changes associated with YANG covered configs self.logger.log_info("Sorting YANG-covered configs patch updates.") - yang_changes = self.inner_patch_sorter.sort(yang_patch, algorithm, current_config_yang) + yang_changes = self.inner_patch_sorter.sort(yang_patch, algorithm, current_config_yang, trace_io=trace_io) changes_len = len(yang_changes) self.logger.log_debug(f"The YANG covered config update was sorted into {changes_len} " \ f"change{'s' if changes_len != 1 else ''}{':' if changes_len > 0 else '.'}") @@ -2402,16 +2518,20 @@ def __init__(self, config_wrapper, patch_wrapper, sort_algorithm_factory=None): self.path_addressing = PathAddressing(self.config_wrapper) self.sort_algorithm_factory = sort_algorithm_factory if sort_algorithm_factory else \ SortAlgorithmFactory(self.operation_wrapper, config_wrapper, self.path_addressing) + self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter", print_all_to_console=True) - def sort(self, patch, algorithm=Algorithm.DFS, preloaded_current_config=None): + def sort(self, patch, algorithm=Algorithm.DFS, preloaded_current_config=None, trace_io: Optional[IO] = None): current_config = preloaded_current_config if preloaded_current_config else self.config_wrapper.get_config_db_as_json() target_config = self.patch_wrapper.simulate_patch(patch, current_config) diff = Diff(copy.deepcopy(current_config), target_config) - sort_algorithm = self.sort_algorithm_factory.create(algorithm) + sort_algorithm = self.sort_algorithm_factory.create(algorithm, path_trace=False if trace_io is None else True) moves = sort_algorithm.sort(diff) + if trace_io is not None: + json.dump(sort_algorithm.path_tracker, trace_io, default=PatchSorterPath.json_encoder, indent=2) + if moves is None: raise GenericConfigUpdaterError("There is no possible sorting") diff --git a/tests/config_test.py b/tests/config_test.py index 9736a571b..d12564071 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -2076,7 +2076,15 @@ def test_apply_patch__only_required_params__default_values_used_for_optional_par # Arrange expected_exit_code = 0 expected_output = "Patch applied successfully" - expected_call_with_default_values = mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, False, False, ()) + expected_call_with_default_values = mock.call( + mock.ANY, + ConfigFormat.CONFIGDB, + False, + False, + False, + (), + trace_io=None, + ) mock_generic_updater = mock.Mock() with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_patch_as_text)): @@ -2101,7 +2109,7 @@ def test_apply_patch__all_optional_params_non_default__non_default_values_used(s expected_output = "Patch applied successfully" expected_ignore_path_tuple = ('/ANY_TABLE', '/ANY_OTHER_TABLE/ANY_FIELD', '') expected_call_with_non_default_values = \ - mock.call(mock.ANY, ConfigFormat.SONICYANG, True, True, True, expected_ignore_path_tuple) + mock.call(mock.ANY, ConfigFormat.SONICYANG, True, True, True, expected_ignore_path_tuple, trace_io=None) mock_generic_updater = mock.Mock() with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_patch_as_text)): @@ -2150,19 +2158,67 @@ def test_apply_patch__exception_thrown__error_displayed_error_code_returned(self def test_apply_patch__optional_parameters_passed_correctly(self): self.validate_apply_patch_optional_parameter( ["--format", ConfigFormat.SONICYANG.name], - mock.call(mock.ANY, ConfigFormat.SONICYANG, False, False, False, ())) + mock.call(mock.ANY, ConfigFormat.SONICYANG, False, False, False, (), trace_io=None)) self.validate_apply_patch_optional_parameter( ["--verbose"], - mock.call(mock.ANY, ConfigFormat.CONFIGDB, True, False, False, ())) + mock.call(mock.ANY, ConfigFormat.CONFIGDB, True, False, False, (), trace_io=None)) self.validate_apply_patch_optional_parameter( ["--dry-run"], - mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, True, False, ())) + mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, True, False, (), trace_io=None)) self.validate_apply_patch_optional_parameter( ["--ignore-non-yang-tables"], - mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, False, True, ())) + mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, False, True, (), trace_io=None)) self.validate_apply_patch_optional_parameter( ["--ignore-path", "/ANY_TABLE"], - mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, False, False, ("/ANY_TABLE",))) + mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, False, False, ("/ANY_TABLE",), trace_io=None)) + + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) + def test_apply_patch__path_trace_option__trace_file_opened_and_passed(self): + # Arrange + import tempfile + expected_exit_code = 0 + expected_output = "Patch applied successfully" + mock_generic_updater = mock.Mock() + mock_file_handle = mock.MagicMock() + + # Create a temporary file for the trace output + with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as trace_file: + trace_file_path = trace_file.name + + try: + with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): + with mock.patch('builtins.open', mock.mock_open(read_data=self.any_patch_as_text)) as mock_open_func: + # Configure mock to return different handles for patch file and trace file + def open_side_effect(filename, mode='r'): + if filename == trace_file_path: + return mock_file_handle + else: + return mock.mock_open(read_data=self.any_patch_as_text).return_value + mock_open_func.side_effect = open_side_effect + + # Act + result = self.runner.invoke(config.config.commands["apply-patch"], + [self.any_path, "--path-trace", trace_file_path], + catch_exceptions=False) + + # Assert + self.assertEqual(expected_exit_code, result.exit_code) + self.assertIn(expected_output, result.output) + mock_generic_updater.apply_patch.assert_called_once() + # Verify that trace_io parameter is not None when --path-trace is used + call_args = mock_generic_updater.apply_patch.call_args + self.assertIsNotNone(call_args[1]['trace_io']) + # Verify the file handle was closed + mock_file_handle.close.assert_called_once() + finally: + # Clean up the temporary file + import os + if os.path.exists(trace_file_path): + os.unlink(trace_file_path) @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( communicate=mock.Mock(return_value=('{"some": "config"}', None)), @@ -2340,7 +2396,15 @@ def test_replace__only_required_params__default_values_used_for_optional_params( # Arrange expected_exit_code = 0 expected_output = "Config replaced successfully" - expected_call_with_default_values = mock.call(mock.ANY, ConfigFormat.CONFIGDB, False, False, False, ()) + expected_call_with_default_values = mock.call( + mock.ANY, + ConfigFormat.CONFIGDB, + False, + False, + False, + (), + trace_io=None + ) mock_generic_updater = mock.Mock() with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_target_config_as_text)): @@ -2360,7 +2424,15 @@ def test_replace__all_optional_params_non_default__non_default_values_used(self) expected_output = "Config replaced successfully" expected_ignore_path_tuple = ('/ANY_TABLE', '/ANY_OTHER_TABLE/ANY_FIELD', '') expected_call_with_non_default_values = \ - mock.call(self.any_target_config, ConfigFormat.SONICYANG, True, True, True, expected_ignore_path_tuple) + mock.call( + self.any_target_config, + ConfigFormat.SONICYANG, + True, + True, + True, + expected_ignore_path_tuple, + trace_io=None + ) mock_generic_updater = mock.Mock() with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_target_config_as_text)): @@ -2404,19 +2476,74 @@ def test_replace__exception_thrown__error_displayed_error_code_returned(self): def test_replace__optional_parameters_passed_correctly(self): self.validate_replace_optional_parameter( ["--format", ConfigFormat.SONICYANG.name], - mock.call(self.any_target_config, ConfigFormat.SONICYANG, False, False, False, ())) + mock.call(self.any_target_config, ConfigFormat.SONICYANG, False, False, False, (), trace_io=None)) self.validate_replace_optional_parameter( ["--verbose"], - mock.call(self.any_target_config, ConfigFormat.CONFIGDB, True, False, False, ())) + mock.call(self.any_target_config, ConfigFormat.CONFIGDB, True, False, False, (), trace_io=None)) self.validate_replace_optional_parameter( ["--dry-run"], - mock.call(self.any_target_config, ConfigFormat.CONFIGDB, False, True, False, ())) + mock.call(self.any_target_config, ConfigFormat.CONFIGDB, False, True, False, (), trace_io=None)) self.validate_replace_optional_parameter( ["--ignore-non-yang-tables"], - mock.call(self.any_target_config, ConfigFormat.CONFIGDB, False, False, True, ())) + mock.call(self.any_target_config, ConfigFormat.CONFIGDB, False, False, True, (), trace_io=None)) self.validate_replace_optional_parameter( ["--ignore-path", "/ANY_TABLE"], - mock.call(self.any_target_config, ConfigFormat.CONFIGDB, False, False, False, ("/ANY_TABLE",))) + mock.call( + self.any_target_config, + ConfigFormat.CONFIGDB, + False, + False, + False, + ("/ANY_TABLE",), + trace_io=None, + ) + ) + + def test_replace__path_trace_option__trace_file_opened_and_passed(self): + # Arrange + import tempfile + expected_exit_code = 0 + expected_output = "Config replaced successfully" + mock_generic_updater = mock.Mock() + mock_file_handle = mock.MagicMock() + + # Create a temporary file for the trace output + with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as trace_file: + trace_file_path = trace_file.name + + try: + with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): + with ( + mock.patch('builtins.open', mock.mock_open(read_data=self.any_target_config_as_text)) as + mock_open_func + ): + # Configure mock to return different handles for config file and trace file + def open_side_effect(filename, mode='r'): + if filename == trace_file_path: + return mock_file_handle + else: + return mock.mock_open(read_data=self.any_target_config_as_text).return_value + mock_open_func.side_effect = open_side_effect + + # Act + result = self.runner.invoke(config.config.commands["replace"], + [self.any_path, "--path-trace", trace_file_path], + catch_exceptions=False) + + # Assert + self.assertEqual(expected_exit_code, result.exit_code) + self.assertIn(expected_output, result.output) + mock_generic_updater.replace.assert_called_once() + # Verify that trace_io parameter is not None when --path-trace is used + call_args = mock_generic_updater.replace.call_args + self.assertIsNotNone(call_args[1]['trace_io']) + # Verify the file handle was closed + mock_file_handle.close.assert_called_once() + finally: + # Clean up the temporary file + import os + if os.path.exists(trace_file_path): + os.unlink(trace_file_path) def validate_replace_optional_parameter(self, param_args, expected_call): # Arrange @@ -2469,7 +2596,8 @@ def test_rollback__only_required_params__default_values_used_for_optional_params mock_generic_updater = mock.Mock() with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): # Act - result = self.runner.invoke(config.config.commands["rollback"], [self.any_checkpoint_name], catch_exceptions=False) + result = self.runner.invoke( + config.config.commands["rollback"], [self.any_checkpoint_name], catch_exceptions=False) # Assert self.assertEqual(expected_exit_code, result.exit_code) @@ -4947,6 +5075,104 @@ def test_delete_checkpoint_multiasic(self): self.assertEqual(result.exit_code, 0, "Command should succeed") self.assertIn("Checkpoint deleted successfully.", result.output) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) + def test_apply_patch__path_trace_option_multiasic__trace_file_opened_and_passed(self): + # Arrange + import tempfile + mock_file_handle = mock.MagicMock() + + # Create a temporary file for the trace output + with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as trace_file: + trace_file_path = trace_file.name + + try: + # Mock open to simulate file reading + with ( + patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as + mock_open_func + ): + # Configure mock to return different handles for patch file and trace file + def open_side_effect(filename, mode='r'): + if filename == trace_file_path: + return mock_file_handle + else: + return mock.mock_open(read_data=json.dumps(self.patch_content)).return_value + mock_open_func.side_effect = open_side_effect + + # Mock GenericUpdater to avoid actual patch application + with patch('config.main.GenericUpdater') as mock_generic_updater: + mock_generic_updater.return_value.apply_patch = MagicMock() + + print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) + # Invocation of the command with the CliRunner + result = self.runner.invoke(config.config.commands["apply-patch"], + [self.patch_file_path, "--path-trace", trace_file_path], + catch_exceptions=False) + + print("Exit Code: {}, output: {}".format(result.exit_code, result.output)) + # Assertions and verifications + self.assertEqual(result.exit_code, 0, "Command should succeed") + self.assertIn("Patch applied successfully.", result.output) + + # Verify the file handle was closed + mock_file_handle.close.assert_called_once() + finally: + # Clean up the temporary file + import os + if os.path.exists(trace_file_path): + os.unlink(trace_file_path) + + @patch('generic_config_updater.generic_updater.ConfigReplacer.replace', MagicMock()) + def test_replace__path_trace_option_multiasic__trace_file_opened_and_passed(self): + # Arrange + import tempfile + mock_file_handle = mock.MagicMock() + mock_replace_content = copy.deepcopy(self.all_config) + + # Create a temporary file for the trace output + with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as trace_file: + trace_file_path = trace_file.name + + try: + with ( + patch('builtins.open', mock_open(read_data=json.dumps(mock_replace_content)), create=True) as + mock_open_func + ): + # Configure mock to return different handles for config file and trace file + def open_side_effect(filename, mode='r'): + if filename == trace_file_path: + return mock_file_handle + else: + return mock.mock_open(read_data=json.dumps(mock_replace_content)).return_value + mock_open_func.side_effect = open_side_effect + + # Mock GenericUpdater to avoid actual replace operation + with patch('config.main.GenericUpdater') as mock_generic_updater: + mock_generic_updater.return_value.replace_all = MagicMock() + + print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) + # Invocation of the command with the CliRunner + result = self.runner.invoke(config.config.commands["replace"], + [self.replace_file_path, "--path-trace", trace_file_path], + catch_exceptions=False) + + print("Exit Code: {}, output: {}".format(result.exit_code, result.output)) + # Assertions and verifications + self.assertEqual(result.exit_code, 0, "Command should succeed") + self.assertIn("Config replaced successfully.", result.output) + + # Verify the file handle was closed + mock_file_handle.close.assert_called_once() + finally: + # Clean up the temporary file + import os + if os.path.exists(trace_file_path): + os.unlink(trace_file_path) + @classmethod def teardown_class(cls): print("TEARDOWN") diff --git a/tests/conftest.py b/tests/conftest.py index c1fd82a77..e64502c06 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,7 @@ import os import re import sys +import unittest from unittest import mock import pytest @@ -22,6 +23,8 @@ import utilities_common.constants as constants import config.main as config +unittest.TestCase.maxDiff = None + test_path = os.path.dirname(os.path.abspath(__file__)) modules_path = os.path.dirname(test_path) sys.path.insert(0, modules_path) diff --git a/tests/generic_config_updater/generic_updater_test.py b/tests/generic_config_updater/generic_updater_test.py index 727c07b02..203d43574 100644 --- a/tests/generic_config_updater/generic_updater_test.py +++ b/tests/generic_config_updater/generic_updater_test.py @@ -42,7 +42,7 @@ def test_apply__no_errors__update_successful(self): patch_applier.config_wrapper.get_config_db_as_json.assert_has_calls([call(), call()]) patch_applier.patch_wrapper.simulate_patch.assert_has_calls( [call(Files.MULTI_OPERATION_CONFIG_DB_PATCH, Files.CONFIG_DB_AS_JSON)]) - patch_applier.patchsorter.sort.assert_has_calls([call(Files.MULTI_OPERATION_CONFIG_DB_PATCH)]) + patch_applier.patchsorter.sort.assert_has_calls([call(Files.MULTI_OPERATION_CONFIG_DB_PATCH, trace_io=None)]) patch_applier.changeapplier.apply.assert_called() patch_applier.patch_wrapper.verify_same_json.assert_has_calls( [call(Files.CONFIG_DB_AFTER_MULTI_PATCH, Files.CONFIG_DB_AFTER_MULTI_PATCH)]) @@ -99,7 +99,9 @@ def test_replace__no_errors__update_successful(self): config_replacer.config_wrapper.get_config_db_as_json.assert_has_calls([call(), call()]) config_replacer.patch_wrapper.generate_patch.assert_has_calls( [call(Files.CONFIG_DB_AS_JSON, Files.CONFIG_DB_AFTER_MULTI_PATCH)]) - config_replacer.patch_applier.apply.assert_has_calls([call(Files.MULTI_OPERATION_CONFIG_DB_PATCH)]) + config_replacer.patch_applier.apply.assert_has_calls( + [call(Files.MULTI_OPERATION_CONFIG_DB_PATCH, trace_io=None)] + ) config_replacer.patch_wrapper.verify_same_json.assert_has_calls( [call(Files.CONFIG_DB_AFTER_MULTI_PATCH, Files.CONFIG_DB_AFTER_MULTI_PATCH)]) @@ -640,7 +642,7 @@ def test_apply_patch__creates_applier_and_apply(self): self.any_ignore_paths) # Assert - patch_applier.apply.assert_has_calls([call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH, True)]) + patch_applier.apply.assert_has_calls([call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH, True, trace_io=None)]) def test_replace__creates_replacer_and_replace(self): # Arrange @@ -667,7 +669,7 @@ def test_replace__creates_replacer_and_replace(self): self.any_ignore_paths) # Assert - config_replacer.replace.assert_has_calls([call(Files.SONIC_YANG_AS_JSON)]) + config_replacer.replace.assert_has_calls([call(Files.SONIC_YANG_AS_JSON, trace_io=None)]) def test_rollback__creates_rollbacker_and_rollback(self): # Arrange @@ -784,14 +786,16 @@ def test_apply__calls_decorated_applier(self): self.decorator.apply(Files.SINGLE_OPERATION_SONIC_YANG_PATCH) # Assert - self.decorated_patch_applier.apply.assert_has_calls([call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH)]) + self.decorated_patch_applier.apply.assert_has_calls( + [call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH, True, trace_io=None)] + ) def test_replace__calls_decorated_replacer(self): # Act self.decorator.replace(Files.SONIC_YANG_AS_JSON) # Assert - self.decorated_config_replacer.replace.assert_has_calls([call(Files.SONIC_YANG_AS_JSON)]) + self.decorated_config_replacer.replace.assert_has_calls([call(Files.SONIC_YANG_AS_JSON, trace_io=None)]) def test_rollback__calls_decorated_rollbacker(self): # Act @@ -835,9 +839,11 @@ def test_apply__converts_to_config_db_and_calls_decorated_class(self): # Assert sonic_yang_decorator.patch_wrapper.convert_sonic_yang_patch_to_config_db_patch.assert_has_calls( - [call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH)]) + [call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH)] + ) sonic_yang_decorator.decorated_patch_applier.apply.assert_has_calls( - [call(Files.SINGLE_OPERATION_CONFIG_DB_PATCH)]) + [call(Files.SINGLE_OPERATION_CONFIG_DB_PATCH, True, trace_io=None)] + ) def test_replace__converts_to_config_db_and_calls_decorated_class(self): # Arrange @@ -849,11 +855,15 @@ def test_replace__converts_to_config_db_and_calls_decorated_class(self): # Assert sonic_yang_decorator.config_wrapper.convert_sonic_yang_to_config_db.assert_has_calls( [call(Files.SONIC_YANG_AS_JSON)]) - sonic_yang_decorator.decorated_config_replacer.replace.assert_has_calls([call(Files.CONFIG_DB_AS_JSON)]) + sonic_yang_decorator.decorated_config_replacer.replace.assert_has_calls( + [call(Files.CONFIG_DB_AS_JSON, trace_io=None)] + ) def __create_sonic_yang_decorator(self): patch_applier = Mock() - patch_applier.apply.side_effect = create_side_effect_dict({(str(Files.SINGLE_OPERATION_CONFIG_DB_PATCH),): 0}) + patch_applier.apply.side_effect = create_side_effect_dict( + {(str(Files.SINGLE_OPERATION_CONFIG_DB_PATCH), 'True'): 0} + ) patch_wrapper = Mock() patch_wrapper.convert_sonic_yang_patch_to_config_db_patch.side_effect = \ @@ -886,7 +896,8 @@ def test_apply__lock_config(self): # Assert config_lock_decorator.config_lock.acquire_lock.assert_called_once() config_lock_decorator.decorated_patch_applier.apply.assert_has_calls( - [call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH)]) + [call(Files.SINGLE_OPERATION_SONIC_YANG_PATCH, True, trace_io=None)] + ) config_lock_decorator.config_lock.release_lock.assert_called_once() def test_replace__lock_config(self): @@ -898,7 +909,9 @@ def test_replace__lock_config(self): # Assert config_lock_decorator.config_lock.acquire_lock.assert_called_once() - config_lock_decorator.decorated_config_replacer.replace.assert_has_calls([call(Files.SONIC_YANG_AS_JSON)]) + config_lock_decorator.decorated_config_replacer.replace.assert_has_calls( + [call(Files.SONIC_YANG_AS_JSON, trace_io=None)] + ) config_lock_decorator.config_lock.release_lock.assert_called_once() def test_rollback__lock_config(self): @@ -929,7 +942,9 @@ def __create_config_lock_decorator(self): config_lock = Mock() patch_applier = Mock() - patch_applier.apply.side_effect = create_side_effect_dict({(str(Files.SINGLE_OPERATION_SONIC_YANG_PATCH),): 0}) + patch_applier.apply.side_effect = create_side_effect_dict( + {(str(Files.SINGLE_OPERATION_SONIC_YANG_PATCH), 'True'): 0} + ) config_replacer = Mock() config_replacer.replace.side_effect = create_side_effect_dict({(str(Files.SONIC_YANG_AS_JSON),): 0}) diff --git a/tests/generic_config_updater/gutest_helpers.py b/tests/generic_config_updater/gutest_helpers.py index b95812d96..b541d8346 100644 --- a/tests/generic_config_updater/gutest_helpers.py +++ b/tests/generic_config_updater/gutest_helpers.py @@ -11,7 +11,7 @@ class MockSideEffectDict: def __init__(self, map): self.map = map - def side_effect_func(self, *args): + def side_effect_func(self, *args, **kwargs): l = [str(arg) for arg in args] key = tuple(l) value = self.map.get(key) @@ -47,7 +47,7 @@ def side_effect_jsonmovegroup_func(self, *args): rv = [] for val in value: - rv.append(JsonMoveGroup(val)) + rv.append(JsonMoveGroup(self.__class__.__name__, val)) return rv diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 1f021b57a..e6d0f0027 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1,5 +1,7 @@ from collections import OrderedDict +import io import jsonpatch +import sys import unittest from unittest.mock import MagicMock, Mock import generic_config_updater.patch_sorter as ps @@ -490,11 +492,11 @@ def setUp(self): self.fail_move_validator = Mock() self.fail_move_validator.validate.side_effect = create_side_effect_skiplastarg_dict( - {(str(self.any_move), str(self.any_diff)): False}) + {(str(self.any_move), str(self.any_diff)): (False, None)}) self.success_move_validator = Mock() self.success_move_validator.validate.side_effect = create_side_effect_skiplastarg_dict( - {(str(self.any_move), str(self.any_diff)): True}) + {(str(self.any_move), str(self.any_diff)): (True, None)}) def test_ctor__assigns_values_correctly(self): # Arrange @@ -516,7 +518,7 @@ def test_generate__single_move_generator__single_move_returned(self): # Arrange move_generators = [self.single_move_generator] move_wrapper = ps.MoveWrapper(move_generators, [], [], []) - expected = [JsonMoveGroup(self.any_move)] + expected = [JsonMoveGroup("", self.any_move)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -528,8 +530,11 @@ def test_generate__multiple_move_generator__multiple_move_returned(self): # Arrange move_generators = [self.multiple_move_generator] move_wrapper = ps.MoveWrapper(move_generators, [], [], []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_other_move1), - JsonMoveGroup(self.any_other_move2)] + expected = [ + JsonMoveGroup("", self.any_move), + JsonMoveGroup("", self.any_other_move1), + JsonMoveGroup("", self.any_other_move2), + ] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -541,7 +546,7 @@ def test_generate__different_move_generators__different_moves_returned(self): # Arrange move_generators = [self.single_move_generator, self.another_single_move_generator] move_wrapper = ps.MoveWrapper(move_generators, [], [], []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_other_move1)] + expected = [JsonMoveGroup("", self.any_move), JsonMoveGroup("", self.any_other_move1)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -553,7 +558,7 @@ def test_generate__duplicate_generated_moves__unique_moves_returned(self): # Arrange move_generators = [self.single_move_generator, self.single_move_generator] move_wrapper = ps.MoveWrapper(move_generators, [], [], []) - expected = [JsonMoveGroup(self.any_move)] + expected = [JsonMoveGroup("", self.any_move)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -565,7 +570,7 @@ def test_generate__different_move_non_extendable_generators__different_moves_ret # Arrange move_non_extendable_generators = [self.single_move_generator, self.another_single_move_generator] move_wrapper = ps.MoveWrapper([], move_non_extendable_generators, [], []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_other_move1)] + expected = [JsonMoveGroup("", self.any_move), JsonMoveGroup("", self.any_other_move1)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -577,7 +582,7 @@ def test_generate__duplicate_generated_non_extendable_moves__unique_moves_return # Arrange move_non_extendable_generators = [self.single_move_generator, self.single_move_generator] move_wrapper = ps.MoveWrapper([], move_non_extendable_generators, [], []) - expected = [JsonMoveGroup(self.any_move)] + expected = [JsonMoveGroup("", self.any_move)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -590,7 +595,7 @@ def test_generate__duplicate_move_between_extendable_and_non_extendable_generato move_generators = [self.single_move_generator] move_non_extendable_generators = [self.single_move_generator] move_wrapper = ps.MoveWrapper(move_generators, move_non_extendable_generators, [], []) - expected = [JsonMoveGroup(self.any_move)] + expected = [JsonMoveGroup("", self.any_move)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -603,7 +608,7 @@ def test_generate__single_move_extender__one_extended_move_returned(self): move_generators = [self.single_move_generator] move_extenders = [self.single_move_extender] move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_extended_move)] + expected = [JsonMoveGroup("", self.any_move), JsonMoveGroup("", self.any_extended_move)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -616,8 +621,8 @@ def test_generate__multiple_move_extender__multiple_extended_move_returned(self) move_generators = [self.single_move_generator] move_extenders = [self.multiple_move_extender] move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_extended_move), - JsonMoveGroup(self.any_other_extended_move1), JsonMoveGroup(self.any_other_extended_move2)] + expected = [JsonMoveGroup("", self.any_move), JsonMoveGroup("", self.any_extended_move), + JsonMoveGroup("", self.any_other_extended_move1), JsonMoveGroup("", self.any_other_extended_move2)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -630,8 +635,8 @@ def test_generate__different_move_extenders__different_extended_moves_returned(s move_generators = [self.single_move_generator] move_extenders = [self.single_move_extender, self.another_single_move_extender] move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_extended_move), - JsonMoveGroup(self.any_other_extended_move1)] + expected = [JsonMoveGroup("", self.any_move), JsonMoveGroup("", self.any_extended_move), + JsonMoveGroup("", self.any_other_extended_move1)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -644,7 +649,7 @@ def test_generate__duplicate_extended_moves__unique_moves_returned(self): move_generators = [self.single_move_generator] move_extenders = [self.single_move_extender, self.single_move_extender] move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_extended_move)] + expected = [JsonMoveGroup("", self.any_move), JsonMoveGroup("", self.any_extended_move)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -657,11 +662,11 @@ def test_generate__mixed_extended_moves__unique_moves_returned(self): move_generators = [self.single_move_generator, self.another_single_move_generator] move_extenders = [self.mixed_move_extender] move_wrapper = ps.MoveWrapper(move_generators, [], move_extenders, []) - expected = [JsonMoveGroup(self.any_move), - JsonMoveGroup(self.any_other_move1), - JsonMoveGroup(self.any_extended_move), - JsonMoveGroup(self.any_other_extended_move1), - JsonMoveGroup(self.any_other_extended_move2)] + expected = [JsonMoveGroup("", self.any_move), + JsonMoveGroup("", self.any_other_move1), + JsonMoveGroup("", self.any_extended_move), + JsonMoveGroup("", self.any_other_extended_move1), + JsonMoveGroup("", self.any_other_extended_move2)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -674,7 +679,7 @@ def test_generate__multiple_non_extendable_moves__no_moves_extended(self): move_non_extendable_generators = [self.single_move_generator, self.another_single_move_generator] move_extenders = [self.mixed_move_extender] move_wrapper = ps.MoveWrapper([], move_non_extendable_generators, move_extenders, []) - expected = [JsonMoveGroup(self.any_move), JsonMoveGroup(self.any_other_move1)] + expected = [JsonMoveGroup("", self.any_move), JsonMoveGroup("", self.any_other_move1)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -688,9 +693,9 @@ def test_generate__mixed_extendable_non_extendable_moves__only_extendable_moves_ move_non_extendable_generators = [self.single_move_generator] # generates: any_move move_extenders = [self.mixed_move_extender] move_wrapper = ps.MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, []) - expected = [JsonMoveGroup(self.any_move), - JsonMoveGroup(self.any_other_move1), - JsonMoveGroup(self.any_other_extended_move1)] + expected = [JsonMoveGroup("", self.any_move), + JsonMoveGroup("", self.any_other_move1), + JsonMoveGroup("", self.any_other_extended_move1)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -704,8 +709,8 @@ def test_generate__move_is_extendable_and_non_extendable__move_is_extended(self) move_non_extendable_generators = [self.single_move_generator] move_extenders = [self.single_move_extender] move_wrapper = ps.MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, []) - expected = [JsonMoveGroup(self.any_move), - JsonMoveGroup(self.any_extended_move)] + expected = [JsonMoveGroup("", self.any_move), + JsonMoveGroup("", self.any_extended_move)] # Act actual = list(move_wrapper.generate(self.any_diff)) @@ -719,7 +724,7 @@ def test_validate__validation_fail__false_returned(self): move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert - self.assertFalse(move_wrapper.validate(self.any_move, self.any_diff)) + self.assertFalse(move_wrapper.validate(self.any_move, self.any_diff)[0]) def test_validate__validation_succeed__true_returned(self): # Arrange @@ -727,7 +732,7 @@ def test_validate__validation_succeed__true_returned(self): move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert - self.assertTrue(move_wrapper.validate(self.any_move, self.any_diff)) + self.assertTrue(move_wrapper.validate(self.any_move, self.any_diff)[0]) def test_validate__multiple_validators_last_fail___false_returned(self): # Arrange @@ -735,7 +740,7 @@ def test_validate__multiple_validators_last_fail___false_returned(self): move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert - self.assertFalse(move_wrapper.validate(self.any_move, self.any_diff)) + self.assertFalse(move_wrapper.validate(self.any_move, self.any_diff)[0]) def test_validate__multiple_validators_succeed___true_returned(self): # Arrange @@ -743,7 +748,7 @@ def test_validate__multiple_validators_succeed___true_returned(self): move_wrapper = ps.MoveWrapper([], [], [], move_validators) # Act and assert - self.assertTrue(move_wrapper.validate(JsonMoveGroup(self.any_move), self.any_diff)) + self.assertTrue(move_wrapper.validate(JsonMoveGroup("", self.any_move), self.any_diff)[0]) def test_simulate__applies_move(self): # Arrange @@ -904,7 +909,7 @@ def verify(self, operation_type, path, expected): move = ps.JsonMove.from_operation(operation) # Act - actual = self.validator.validate(JsonMoveGroup(move), self.any_diff, self.any_target_config) + actual, _ = self.validator.validate(JsonMoveGroup("", move), self.any_diff, self.any_target_config) # Assert self.assertEqual(expected, actual) @@ -927,7 +932,8 @@ def test_validate__invalid_config_db_after_applying_move__failure(self): validator = ps.FullConfigMoveValidator(config_wrapper) # Act and assert - self.assertFalse(validator.validate(JsonMoveGroup(self.any_move), self.any_diff, self.any_simulated_config)) + self.assertFalse( + validator.validate(JsonMoveGroup("", self.any_move), self.any_diff, self.any_simulated_config)[0]) def test_validate__valid_config_db_after_applying_move__success(self): # Arrange @@ -937,7 +943,8 @@ def test_validate__valid_config_db_after_applying_move__success(self): validator = ps.FullConfigMoveValidator(config_wrapper) # Act and assert - self.assertTrue(validator.validate(JsonMoveGroup(self.any_move), self.any_diff, self.any_simulated_config)) + self.assertTrue( + validator.validate(JsonMoveGroup("", self.any_move), self.any_diff, self.any_simulated_config)[0]) def test_validate__passes_quiet_true_to_config_wrapper(self): # Regression guard: gu_common.ConfigWrapper.validate_config_db_config @@ -1216,7 +1223,7 @@ def verify_parent_adding(self, added_parent_value, expected): diff = ps.Diff(current_config, target_config) move = ps.JsonMove.from_operation({"op":"add", "path":"/BGP_NEIGHBOR/10.0.0.57", "value": added_parent_value}) - actual = self.validator.validate(move, diff, move.apply(diff.current_config)) + actual, _ = self.validator.validate(move, diff, move.apply(diff.current_config)) self.assertEqual(expected, actual) @@ -1228,7 +1235,7 @@ def verify_diff(self, current_config, target_config, current_config_tokens=None, move = ps.JsonMove(diff, OperationType.REPLACE, current_config_tokens, target_config_tokens) # Act - actual = self.validator.validate(move, diff, move.apply(diff.current_config)) + actual, _ = self.validator.validate(move, diff, move.apply(diff.current_config)) # Assert self.assertEqual(expected, actual) @@ -1243,17 +1250,17 @@ def test_validate__add_full_config_has_dependencies__failure(self): # Arrange # CROPPED_CONFIG_DB_AS_JSON has dependencies between PORT and ACL_TABLE diff = ps.Diff(Files.EMPTY_CONFIG_DB, Files.CROPPED_CONFIG_DB_AS_JSON) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.ADD, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, [], [])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__add_full_config_no_dependencies__success(self): # Arrange diff = ps.Diff(Files.EMPTY_CONFIG_DB, Files.CONFIG_DB_NO_DEPENDENCIES) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.ADD, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, [], [])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__add_table_has_no_dependencies__success(self): # Arrange @@ -1263,10 +1270,10 @@ def test_validate__add_table_has_no_dependencies__success(self): {"op": "remove", "path":"/ACL_TABLE"} ])) diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__remove_table_has_no_dependencies__success(self): # Arrange @@ -1275,10 +1282,10 @@ def test_validate__remove_table_has_no_dependencies__success(self): {"op": "remove", "path":"/ACL_TABLE"} ])) diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__replace_whole_config_item_added_ref_added__failure(self): # Arrange @@ -1290,10 +1297,10 @@ def test_validate__replace_whole_config_item_added_ref_added__failure(self): ])) diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__replace_whole_config_item_removed_ref_removed__false(self): # Arrange @@ -1305,10 +1312,10 @@ def test_validate__replace_whole_config_item_removed_ref_removed__false(self): ])) diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__replace_whole_config_item_same_ref_added__true(self): # Arrange @@ -1319,10 +1326,10 @@ def test_validate__replace_whole_config_item_same_ref_added__true(self): ])) diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__replace_whole_config_item_same_ref_removed__true(self): # Arrange @@ -1333,10 +1340,10 @@ def test_validate__replace_whole_config_item_same_ref_removed__true(self): ])) diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__replace_whole_config_item_same_ref_same__true(self): # Arrange @@ -1345,10 +1352,10 @@ def test_validate__replace_whole_config_item_same_ref_same__true(self): target_config = current_config diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__replace_list_item_different_location_than_target_and_no_deps__true(self): # Arrange @@ -1376,11 +1383,18 @@ def test_validate__replace_list_item_different_location_than_target_and_no_deps_ diff = ps.Diff(current_config, target_config) # the target tokens point to location 0 which exist in target_config # but the replace operation is operating on location 1 in current_config - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, ["VLAN", "Vlan100", "dhcp_servers", 1], - ["VLAN", "Vlan100", "dhcp_servers", 0])) + move = JsonMoveGroup( + "", + ps.JsonMove( + diff, + OperationType.REPLACE, + ["VLAN", "Vlan100", "dhcp_servers", 1], + ["VLAN", "Vlan100", "dhcp_servers", 0] + ), + ) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def prepare_config(self, config, patch): return patch.apply(config) @@ -1395,110 +1409,116 @@ def test_validate__no_changes__success(self): current_config = {"some_table":{"key1":"value1", "key2":"value2"}} target_config = {"some_table":{"key1":"value1", "key2":"value22"}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key1"], ["some_table", "key1"])) + move = JsonMoveGroup( + "", + ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key1"], ["some_table", "key1"]) + ) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__change_but_no_empty_table__success(self): # Arrange current_config = {"some_table":{"key1":"value1", "key2":"value2"}} target_config = {"some_table":{"key1":"value1", "key2":"value22"}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key2"], ["some_table", "key2"])) + move = JsonMoveGroup( + "", + ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key2"], ["some_table", "key2"]), + ) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__single_empty_table__failure(self): # Arrange current_config = {"some_table":{"key1":"value1", "key2":"value2"}} target_config = {"some_table":{}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, ["some_table"], ["some_table"])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, ["some_table"], ["some_table"])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__whole_config_replace_single_empty_table__failure(self): # Arrange current_config = {"some_table":{"key1":"value1", "key2":"value2"}} target_config = {"some_table":{}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__whole_config_replace_mix_of_empty_and_non_empty__failure(self): # Arrange current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} target_config = {"some_table":{"key1":"value1"}, "other_table":{}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__whole_config_multiple_empty_tables__failure(self): # Arrange current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} target_config = {"some_table":{}, "other_table":{}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__remove_key_empties_a_table__failure(self): # Arrange current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} target_config = {"some_table":{"key1":"value1"}, "other_table":{}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], [])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__remove_key_but_table_has_other_keys__success(self): # Arrange current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2", "key3":"value3"}} target_config = {"some_table":{"key1":"value1"}, "other_table":{"key3":"value3"}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], [])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__remove_whole_table__success(self): # Arrange current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} target_config = {"some_table":{"key1":"value1"}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REMOVE, ["other_table"], [])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["other_table"], [])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__add_empty_table__failure(self): # Arrange current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} target_config = {"new_table":{}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"])) # Act and assert - self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertFalse(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def test_validate__add_non_empty_table__success(self): # Arrange current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} target_config = {"new_table":{"key3":"value3"}} diff = ps.Diff(current_config, target_config) - move = JsonMoveGroup(ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"])) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"])) # Act and assert - self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) class TestRequiredValueMoveValidator(unittest.TestCase): def setUp(self): @@ -1533,12 +1553,12 @@ def _run_single_test(self, test_case): # Arrange expected = test_case['expected'] current_config = test_case['config'] - move = JsonMoveGroup(test_case['move']) + move = JsonMoveGroup("", test_case['move']) target_config = test_case.get('target_config', move.apply(current_config)) diff = ps.Diff(current_config, target_config) # Act and Assert - self.assertEqual(expected, self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertEqual(expected, self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def _get_critical_port_change_test_cases(self): # port-up status-changing under-port port-exist verdict @@ -1997,12 +2017,12 @@ def _run_single_test(self, test_case): # Arrange expected = test_case['expected'] current_config = test_case['config'] - move = JsonMoveGroup(test_case['move']) + move = JsonMoveGroup("", test_case['move']) target_config = test_case.get('target_config', move.apply(current_config)) diff = ps.Diff(current_config, target_config) # Act and Assert - self.assertEqual(expected, self.validator.validate(move, diff, move.apply(diff.current_config))) + self.assertEqual(expected, self.validator.validate(move, diff, move.apply(diff.current_config))[0]) def _apply_operations(self, config, operations): return jsonpatch.JsonPatch(operations).apply(config) @@ -3334,12 +3354,11 @@ def test_patch_sorter_success(self): # . # } data = Files.PATCH_SORTER_TEST_SUCCESS - skip_exact_change_list_match = False for test_case_name in data: with self.subTest(name=test_case_name): - self.run_single_success_case(data[test_case_name], skip_exact_change_list_match) + self.run_single_success_case(test_case_name, data[test_case_name]) - def run_single_success_case(self, data, skip_exact_change_list_match): + def run_single_success_case(self, test_case_name, data): current_config = data["current_config"] patch = jsonpatch.JsonPatch(data["patch"]) expected_changes = [] @@ -3348,19 +3367,15 @@ def run_single_success_case(self, data, skip_exact_change_list_match): sorter = self.create_patch_sorter(current_config) - actual_changes = sorter.sort(patch) - - if not skip_exact_change_list_match: - self.assertEqual(expected_changes, actual_changes) + trace_io = io.StringIO() + actual_changes = sorter.sort(patch, trace_io=trace_io) + trace = trace_io.getvalue() + trace_io.close() - target_config = patch.apply(current_config) - simulated_config = current_config - for change in actual_changes: - simulated_config = change.apply(simulated_config) - is_valid, error = self.config_wrapper.validate_config_db_config(simulated_config) - self.assertTrue(is_valid, f"Change will produce invalid config. Error: {error}") + if expected_changes != actual_changes: + print(f"{test_case_name} failed, trace: \n{trace}", file=sys.stderr) - self.assertEqual(target_config, simulated_config) + self.assertEqual(expected_changes, actual_changes) def test_patch_sorter_failure(self): # Format of the JSON file containing the test-cases: @@ -3406,7 +3421,8 @@ def test_sort__does_not_remove_tables_without_yang_unintentionally_if_generated_ any_patch = Files.SINGLE_OPERATION_CONFIG_DB_PATCH target_config = any_patch.apply(current_config) sort_algorithm = Mock() - sort_algorithm.sort = lambda diff: [JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], []))] + sort_algorithm.sort = lambda diff: [JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], []))] + sort_algorithm.path_tracker = None patch_sorter = self.create_patch_sorter(current_config, sort_algorithm) expected = [JsonChange(jsonpatch.JsonPatch([OperationWrapper().create(OperationType.REPLACE, "", target_config)]))] From be04b07f4af16e2e0fdb78b154e3862c2112f4e8 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Fri, 11 Sep 2026 09:29:09 -0700 Subject: [PATCH 3/7] GCU generates suboptimal plan for CreateOnly paths (#4335) (#4813) * GCU generates suboptimal plan for CreateOnly paths When GCU hits a CreateOnly entry that has changed, it generates a suboptimal plan. One example is a simple change of: ``` [{"op": "replace", "path": "/MIRROR_SESSION/EVERFLOW_TUNNEL/dst_ip", "value": "200.1.1.203"}] ``` Should generate an optimal plan of: ``` [ [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}], [{"op": "remove", "path": "/MIRROR_SESSION"}], [{"op": "add", "path": "/MIRROR_SESSION", "value": {"EVERFLOW_TUNNEL": {"dscp": "8", "dst_ip": "200.1.1.203", "src_ip": "100.1.1.1", "ttl": "255", "type": "ERSPAN"}}}] [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}] ] ``` But instead generates this plan (which removes all ACLs): ``` [ [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}], [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}], [{"op": "remove", "path": "/ACL_RULE"}], [{"op": "add", "path": "/ACL_RULE", "value": {"EVERFLOW|RULE_1": {"PRIORITY": "1000", "IP_TYPE": "IP", "MIRROR_INGRESS_ACTION": "EVERFLOW_TUNNEL"}}}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1", "value": {"PRIORITY": "10"}}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/DST_IP", "value": "192.168.1.1/32"}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/IP_TYPE", "value": "IP"}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/L4_DST_PORT", "value": "22"}], [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}], [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}], [{"op": "remove", "path": "/ACL_RULE/DATAACL|RULE_1"}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1", "value": {"PRIORITY": "10"}}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/DST_IP", "value": "192.168.1.1/32"}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/IP_TYPE", "value": "IP"}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/PACKET_ACTION", "value": "DROP"}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/IP_TYPE", "value": "IP"}], [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION"}], [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}], [{"op": "remove", "path": "/ACL_RULE/DATAACL|RULE_1"}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/IP_TYPE", "value": "IP"}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1", "value": {"PRIORITY": "10"}}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/DST_IP", "value": "192.168.1.1/32"}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/IP_TYPE", "value": "IP"}], [{"op": "remove", "path": "/ACL_RULE/EVERFLOW|RULE_1"}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1", "value": {"PRIORITY": "1000"}}], [{"op": "remove", "path": "/MIRROR_SESSION"}], [{"op": "add", "path": "/MIRROR_SESSION", "value": {"EVERFLOW_TUNNEL": {"dscp": "8", "dst_ip": "200.1.1.203", "src_ip": "100.1.1.1", "ttl": "255", "type": "ERSPAN"}}}], [{"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/L4_DST_PORT", "value": "22"}, {"op": "add", "path": "/ACL_RULE/DATAACL|RULE_1/PACKET_ACTION", "value": "DROP"}], [{"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/IP_TYPE", "value": "IP"}, {"op": "add", "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", "value": "EVERFLOW_TUNNEL"}] ] ``` Modified`RemoveCreateOnlyDependencyMoveGenerator`: * it would previously short-circuit early due to only processing one child leaf in the same table. * it would previously attempt to iterate across all members of the table even though there was a complete path list. * it was missing logic to remove the create only path itself (and was relying on extenders to do that which was inefficient and wouldn't generate the right plan) * when removing dependents it wasn't recursing to ensure it would remove dependents of dependents Since this generator is now full and doesn't rely on any extenders, it has been moved to a non-extendable generator. These changes caused some existing (suboptimal) plans that got generated to change so those test cases have also been updated. Added test case to validate this behavior and ensure it does not regress. * Update generic_config_updater/patch_sorter.py Prevent possible infinite loop scenario Copilot identified, however it shouldn't be possible given self.__get_path_count() can't return 1 in that scenario to allow the loop to continue. But hardening isn't a bad practice. * Update generic_config_updater/patch_sorter.py Fix spelling / gramatical error caught by Copilot. * Code Review Comments 1. Add recursion depth protection in removing dependents because of a concern of recursive dependencies, which isn't actually possible with yang. None-the-less, implemented. 2. Remove a duplicate move that gets generated as when using it with a Depth-first sorter its not necessary though could be on other sorters. 3. The old code depended on 3 levels of depth for the create only leafs. Reworked the logic to not be dependent on depth. 4. __get_path_count() can no longer return a KeyError even though the caller paths would make that impossible, but future use cases may need it to not throw an exception when the path is not found. --------- (cherry picked from commit 1580ccced2ae08e2e65c5428aa6c85369ca01748) Signed-off-by: rimunagala Co-authored-by: Brad House - Nexthop --- generic_config_updater/patch_sorter.py | 114 ++++++---- .../files/patch_sorter_test_success.json | 197 ++++++++++++------ .../patch_sorter_test.py | 36 +++- 3 files changed, 239 insertions(+), 108 deletions(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 807e1ad02..fb2d779c1 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -4,7 +4,7 @@ import sonic_yang from collections import deque, OrderedDict from enum import Enum -from typing import IO, Optional, Tuple +from typing import Any, IO, List, Optional, Tuple from .gu_common import OperationWrapper, OperationType, GenericConfigUpdaterError, \ JsonChange, PathAddressing, genericUpdaterLogging @@ -1580,55 +1580,89 @@ def generate(self, diff): target_config = diff.target_config # Final config after applying whole patch reload_config = True - processed_tables = set() for path in self.create_only_filter.get_paths(current_config): tokens = self.path_addressing.get_path_tokens(path) - table_to_check, create_only_field = tokens[0], tokens[-1] - - if table_to_check in processed_tables: - continue - else: - processed_tables.add(table_to_check) - - if table_to_check not in current_config: - continue + current_field = self.__fetch_path(current_config, tokens) + target_field = self.__fetch_path(target_config, tokens) - current_members = current_config[table_to_check] - if not current_members: + # If field is deleted or created we don't care, we only care when it was + # already set and is changing value. + if current_field is None or target_field is None or current_field == target_field: continue - if table_to_check not in target_config: - continue + # Create only filters may reference an exact leaf, but it's really the parent that is the + # object we're after. + tokens.pop() - target_members = target_config[table_to_check] - if not target_members: - continue + # First see if there are any dependents for the exact path + for move in self.__remove_dependents(diff, tokens, reload_config=reload_config, + remove_parent=False): + yield move - for member_name in current_members: - if member_name not in target_members: - continue + # No need to reload config after first call + reload_config = False - current_field = self._get_create_only_field( - current_config, table_to_check, member_name, create_only_field) - target_field = self._get_create_only_field( - target_config, table_to_check, member_name, create_only_field) + yield self.__remove_nonempty(diff, tokens) - if current_field == target_field: - continue + # If that didn't work, likely the parents of the dependent path needs to be removed. + for move in self.__remove_dependents(diff, tokens, reload_config=reload_config, + remove_parent=True): + yield move - member_path = f"/{table_to_check}/{member_name}" + # Remove self again after removing the parents of dependents + # NOTE: When we use the DFS sorter this is irrelevant. Right now we only use DFS so we don't need it + # as it will be called again after it removed any parent dependents. Commenting out for now. + # + # yield self.__remove_nonempty(diff, tokens) - for ref_path in self.path_addressing.find_ref_paths(member_path, current_config, - reload_config=reload_config): - yield JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.REMOVE, - self.path_addressing.get_path_tokens(ref_path))) + def __fetch_path(self, config, tokens: List[str]) -> Any: + for token in tokens: + config = config.get(token) + if config is None: + return None + return config + + def __get_path_count(self, config, tokens: List[str]) -> int: + config = self.__fetch_path(config, tokens) + if config is None: + return 0 + return len(config) + + def __remove_nonempty(self, diff: Diff, tokens: List[str]): + remove_tokens = list(tokens) + # Only trim while there is at least one table token and one deeper level. + # This prevents generating an empty path ("") and avoids an infinite loop + # when the top-level config has a single table. + while len(remove_tokens) > 1 and \ + self.__get_path_count(diff.current_config, remove_tokens[:-1]) == 1: + remove_tokens = remove_tokens[:-1] + return JsonMoveGroup(self.__class__.__name__, JsonMove(diff, OperationType.REMOVE, remove_tokens)) + + def __remove_dependents( + self, + diff: Diff, + tokens: List[str], + reload_config: bool, + remove_parent: bool, + recursion_depth: int = 0, + ): + if recursion_depth >= 10: + return - # No need to reload config after first call - reload_config = False + config = diff.current_config + path = self.path_addressing.create_path(tokens) + ref_paths = self.path_addressing.find_ref_paths(path, config, reload_config) + for ref in ref_paths: + ref_tokens = self.path_addressing.get_path_tokens(ref) + if remove_parent: + ref_tokens.pop() + + # Recurse since there could be a dependency chain + for move in self.__remove_dependents(diff, ref_tokens, reload_config=False, + remove_parent=remove_parent, recursion_depth=recursion_depth+1): + yield move - def _get_create_only_field(self, config, table_to_check, - member_name, create_only_field): - return config[table_to_check][member_name].get(create_only_field, None) + yield self.__remove_nonempty(diff, ref_tokens) class LowLevelMoveGenerator: @@ -2222,10 +2256,10 @@ def __init__(self, operation_wrapper, config_wrapper, path_addressing): self.path_addressing = path_addressing def create(self, algorithm=Algorithm.DFS, path_trace: bool = False): - move_generators = [RemoveCreateOnlyDependencyMoveGenerator(self.path_addressing), - LowLevelMoveGenerator(self.path_addressing)] + move_generators = [LowLevelMoveGenerator(self.path_addressing)] # TODO: Enable TableLevelMoveGenerator once it is confirmed whole table can be updated at the same time - move_non_extendable_generators = [BulkKeyLevelMoveGenerator(self.path_addressing), + move_non_extendable_generators = [RemoveCreateOnlyDependencyMoveGenerator(self.path_addressing), + BulkKeyLevelMoveGenerator(self.path_addressing), KeyLevelMoveGenerator(self.path_addressing), BulkKeyGroupLowLevelMoveGenerator(self.path_addressing), BulkLowLevelMoveGenerator(self.path_addressing)] diff --git a/tests/generic_config_updater/files/patch_sorter_test_success.json b/tests/generic_config_updater/files/patch_sorter_test_success.json index 4d8cb8a5a..dc041684b 100644 --- a/tests/generic_config_updater/files/patch_sorter_test_success.json +++ b/tests/generic_config_updater/files/patch_sorter_test_success.json @@ -737,27 +737,6 @@ } ], "expected_changes": [ - [ - { - "op": "replace", - "path": "/PORT/Ethernet0/alias", - "value": "Eth1/1" - } - ], - [ - { - "op": "replace", - "path": "/PORT/Ethernet0/description", - "value": "" - } - ], - [ - { - "op": "replace", - "path": "/PORT/Ethernet0/speed", - "value": "10000" - } - ], [ { "op": "remove", @@ -1017,42 +996,37 @@ [ { "op": "remove", - "path": "/VLAN_MEMBER/Vlan100|Ethernet1" - }, - { - "op": "remove", - "path": "/VLAN_MEMBER/Vlan100|Ethernet2" - }, + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/0" + } + ], + [ { "op": "remove", - "path": "/VLAN_MEMBER/Vlan100|Ethernet3" + "path": "/VLAN_MEMBER/Vlan100|Ethernet0" } ], [ { "op": "remove", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/0" + "path": "/PORT/Ethernet0" } ], [ { - "op": "replace", - "path": "/PORT/Ethernet0/alias", - "value": "Eth1" + "op": "remove", + "path": "/VLAN_MEMBER/Vlan100|Ethernet1" } ], [ { - "op": "replace", - "path": "/PORT/Ethernet0/description", - "value": "Ethernet0 100G link" + "op": "remove", + "path": "/VLAN_MEMBER/Vlan100|Ethernet2" } ], [ { - "op": "replace", - "path": "/PORT/Ethernet0/speed", - "value": "100000" + "op": "remove", + "path": "/VLAN_MEMBER" } ], [ @@ -1115,25 +1089,6 @@ "path": "/PORT/Ethernet2" } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/0", - "value": "Ethernet0" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1" - } - ], - [ - { - "op": "remove", - "path": "/PORT/Ethernet3" - } - ], [ { "op": "remove", @@ -1143,10 +1098,8 @@ [ { "op": "remove", - "path": "/VLAN_MEMBER" - } - ], - [ + "path": "/PORT/Ethernet3" + }, { "op": "remove", "path": "/PORT" @@ -1159,8 +1112,8 @@ "value": { "Ethernet0": { "alias": "Eth1", - "description": "Ethernet0 100G link", "lanes": "65, 66, 67, 68", + "description": "Ethernet0 100G link", "speed": "100000" } } @@ -5692,5 +5645,125 @@ } ] ] + }, + "CREATE_ONLY_PATH_TEST_MIRROR_SESSION__SUCCESS": { + "desc": "Mirror changes should not remove ACL Table.", + "current_config": { + "MIRROR_SESSION": { + "EVERFLOW_TUNNEL": { + "dscp": "8", + "dst_ip": "200.1.1.200", + "src_ip": "100.1.1.1", + "ttl": "255", + "type": "ERSPAN" + } + }, + "ACL_TABLE": { + "DATAACL": { + "policy_desc": "DATAACL", + "ports": [ + "Ethernet4" + ], + "stage": "ingress", + "type": "L3" + }, + "EVERFLOW": { + "policy_desc": "EVERFLOW", + "ports": [ + "Ethernet8" + ], + "stage": "ingress", + "type": "MIRROR" + }, + "EVERFLOWV6": { + "policy_desc": "EVERFLOWV6", + "ports": [ + "Ethernet4", + "Ethernet8" + ], + "stage": "ingress", + "type": "MIRRORV6" + } + }, + "ACL_RULE": { + "DATAACL|RULE_1": { + "DST_IP": "192.168.1.1/32", + "IP_TYPE": "IP", + "L4_DST_PORT": "22", + "PACKET_ACTION": "DROP", + "PRIORITY": "10" + }, + "EVERFLOW|RULE_1": { + "PRIORITY": "1000", + "IP_TYPE": "IP", + "MIRROR_INGRESS_ACTION": "EVERFLOW_TUNNEL" + } + }, + "PORT": { + "Ethernet4": { + "admin_status": "up", + "alias": "fortyGigE0/4", + "description": "Servers0:eth0", + "index": "1", + "lanes": "29,30,31,32", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + }, + "Ethernet8": { + "admin_status": "up", + "alias": "fortyGigE0/8", + "description": "Servers1:eth0", + "index": "2", + "lanes": "33,34,35,36", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + } + } + }, + "patch": [ + { + "op": "replace", + "path": "/MIRROR_SESSION/EVERFLOW_TUNNEL/dst_ip", + "value": "200.1.1.203" + } + ], + "expected_changes": [ + [ + { + "op": "remove", + "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION" + } + ], + [ + { + "op": "remove", + "path": "/MIRROR_SESSION" + } + ], + [ + { + "op": "add", + "path": "/MIRROR_SESSION", + "value": { + "EVERFLOW_TUNNEL": { + "dscp": "8", + "dst_ip": "200.1.1.203", + "src_ip": "100.1.1.1", + "ttl": "255", + "type": "ERSPAN" + } + } + } + ], + [ + { + "op": "add", + "path": "/ACL_RULE/EVERFLOW|RULE_1/MIRROR_INGRESS_ACTION", + "value": "EVERFLOW_TUNNEL" + } + ] + ] } } diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index e6d0f0027..9ba0c2508 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -2545,8 +2545,20 @@ def test_generate__dpb_4_to_1_example(self): moves = list(self.generator.generate(diff)) # Assert + + # This is a proper output even though it looks wrong. + # Due to logic in the generator to ensure it removes the exact referenced + # leaves for dependents, then the CreateOnly path, followed by the parents + # of the dependent paths. Since this is a generator called by DFS it will + # be called recursively so the parent may not ever be removed in practice. + # Also since it is recursive and starts over, in practice if it did need + # to delete the parent path, it would emit another delete of the + # create-only attribute parent. self.verify_moves([{'op': 'remove', 'path': '/ACL_TABLE/NO-NSW-PACL-V4/ports/0'}, - {'op': 'remove', 'path': '/VLAN_MEMBER/Vlan100|Ethernet0'}], + {'op': 'remove', 'path': '/VLAN_MEMBER/Vlan100|Ethernet0'}, + {'op': 'remove', 'path': '/PORT/Ethernet0'}, + {'op': 'remove', 'path': '/ACL_TABLE/NO-NSW-PACL-V4/ports'}, + {'op': 'remove', 'path': '/VLAN_MEMBER'}], moves) def test_generate__dpb_1_to_4_example(self): @@ -2557,8 +2569,20 @@ def test_generate__dpb_1_to_4_example(self): moves = list(self.generator.generate(diff)) # Assert - self.verify_moves([{'op': 'remove', 'path': '/ACL_TABLE/NO-NSW-PACL-V4/ports/0'}, - {'op': 'remove', 'path': '/VLAN_MEMBER/Vlan100|Ethernet0'}], + + # This is a proper output even though it looks wrong on a couple of fronts. + # Due to logic in the generator to ensure it doesn't create empty tables, it + # will remove the parent if it removed the last entry in the table. In this + # case in each of the tables we are removing the only entry. Then the repetition + # is due to logic to remove the parent of a dependent if the prior generator + # failed to validate, which ends up resolving to the same path as the original + # due to the no-empty-table logic. Since no validators are run we see the same + # output twice. + self.verify_moves([{'op': 'remove', 'path': '/ACL_TABLE/NO-NSW-PACL-V4/ports'}, + {'op': 'remove', 'path': '/VLAN_MEMBER'}, + {'op': 'remove', 'path': '/PORT'}, + {'op': 'remove', 'path': '/ACL_TABLE/NO-NSW-PACL-V4/ports'}, + {'op': 'remove', 'path': '/VLAN_MEMBER'}], moves) def verify_moves(self, ops, moves): @@ -3303,9 +3327,9 @@ def verify(self, algo, algo_class): # Arrange config_wrapper = ConfigWrapper() factory = ps.SortAlgorithmFactory(OperationWrapper(), config_wrapper, PathAddressing(config_wrapper)) - expected_generators = [ps.RemoveCreateOnlyDependencyMoveGenerator, - ps.LowLevelMoveGenerator] - expected_non_extendable_generators = [ps.BulkKeyLevelMoveGenerator, + expected_generators = [ps.LowLevelMoveGenerator] + expected_non_extendable_generators = [ps.RemoveCreateOnlyDependencyMoveGenerator, + ps.BulkKeyLevelMoveGenerator, ps.KeyLevelMoveGenerator, ps.BulkKeyGroupLowLevelMoveGenerator, ps.BulkLowLevelMoveGenerator] From 5962f5cf215dcea23b2c790d2e2c737d44e51887 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Fri, 11 Sep 2026 11:15:12 -0700 Subject: [PATCH 4/7] GCU sort: batch leaf-list changes into single REPLACE move (#4478) (#4814) Add BulkLeafListMoveGenerator that produces a single REPLACE move for leaf-list fields whose items differ between current and target configs, instead of decomposing into N individual REMOVE/ADD moves. This is registered as a non-extendable generator (tried before individual moves in DFS). If validation fails, DFS falls through to per-item moves. Impact: For a 512-port ACL table where half the ports are removed, this reduces ~256 individual moves (each triggering 2 loadData calls at ~1.4s each = ~717s) to 1 move (1 loadData = ~1.4s). Conservative scope: - Only handles leaf-lists (lists of scalars, not lists of dicts) - Only replaces lists that exist in both current and target - Falls through to individual moves if the bulk replace fails validation (cherry picked from commit bfc67f56837f56a7e0fea1d22a6a653836d6e74a) Signed-off-by: rimunagala Co-authored-by: rookie-who --- generic_config_updater/patch_sorter.py | 57 ++++++++ .../files/patch_sorter_test_success.json | 129 ++++++++++-------- .../patch_sorter_test.py | 88 +++++++++++- 3 files changed, 215 insertions(+), 59 deletions(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index fb2d779c1..cc2343fbf 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -1272,6 +1272,62 @@ def _get_non_existing_keys_tokens(self, config1, config2, reverse): yield [table, key] +class BulkLeafListMoveGenerator: + """ + A non-extendable generator that produces a single REPLACE move for each + leaf-list field whose items differ between current and target configs. + + Instead of generating N individual REMOVE/ADD moves (one per list item), + this emits one REPLACE of the whole list. The DFS tries non-extendable + generators first, so if the bulk replace validates, we skip N-1 moves + and their associated loadData() calls. + + This is conservative: + - Only handles leaf-lists (lists of scalars, not lists of dicts) + - Only replaces lists that already exist in both current and target + - If this move fails validation, DFS continues to other generators + which produce per-item moves (no explicit fallback mechanism) + """ + def __init__(self, path_addressing): + self.path_addressing = path_addressing + + def generate(self, diff): + for move in self._traverse(diff, diff.current_config, diff.target_config, []): + yield move + + def _traverse(self, diff, current_ptr, target_ptr, tokens): + if not isinstance(current_ptr, dict) or not isinstance(target_ptr, dict): + return + + for key in current_ptr: + if key not in target_ptr: + continue + + tokens.append(key) + current_val = current_ptr[key] + target_val = target_ptr[key] + + if isinstance(current_val, list) and isinstance(target_val, list): + # Only handle leaf-lists (lists of scalars) + if (current_val != target_val and + self._is_leaf_list(current_val) and + self._is_leaf_list(target_val)): + yield JsonMoveGroup( + self.__class__.__name__, + JsonMove(diff, OperationType.REPLACE, list(tokens), list(tokens)), + ) + elif isinstance(current_val, dict) and isinstance(target_val, dict): + for move in self._traverse(diff, current_val, target_val, tokens): + yield move + + tokens.pop() + + @staticmethod + def _is_leaf_list(lst): + """Return True if lst contains only scalars (str, int, float, bool).""" + return all(isinstance(item, (str, int, float, bool)) for item in lst) + + class BulkKeyLevelMoveGenerator: """ Same concept as KeyLevelMoveGenerator, but groups additions and removals of sibling keys. @@ -2259,6 +2315,7 @@ def create(self, algorithm=Algorithm.DFS, path_trace: bool = False): move_generators = [LowLevelMoveGenerator(self.path_addressing)] # TODO: Enable TableLevelMoveGenerator once it is confirmed whole table can be updated at the same time move_non_extendable_generators = [RemoveCreateOnlyDependencyMoveGenerator(self.path_addressing), + BulkLeafListMoveGenerator(self.path_addressing), BulkKeyLevelMoveGenerator(self.path_addressing), KeyLevelMoveGenerator(self.path_addressing), BulkKeyGroupLowLevelMoveGenerator(self.path_addressing), diff --git a/tests/generic_config_updater/files/patch_sorter_test_success.json b/tests/generic_config_updater/files/patch_sorter_test_success.json index dc041684b..a63fc3f4b 100644 --- a/tests/generic_config_updater/files/patch_sorter_test_success.json +++ b/tests/generic_config_updater/files/patch_sorter_test_success.json @@ -537,9 +537,13 @@ "expected_changes": [ [ { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/ports/0", - "value": "Ethernet0" + "op": "replace", + "path": "/ACL_TABLE/EVERFLOWV6/ports", + "value": [ + "Ethernet0", + "Ethernet4", + "Ethernet8" + ] } ] ] @@ -846,23 +850,14 @@ ], [ { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/1", - "value": "Ethernet1" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/2", - "value": "Ethernet2" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports/3", - "value": "Ethernet3" + "op": "replace", + "path": "/ACL_TABLE/NO-NSW-PACL-V4/ports", + "value": [ + "Ethernet0", + "Ethernet1", + "Ethernet2", + "Ethernet3" + ] } ] ] @@ -1491,15 +1486,12 @@ "expected_changes": [ [ { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOWV6/ports/0" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/ports/0", - "value": "Ethernet0" + "op": "replace", + "path": "/ACL_TABLE/EVERFLOWV6/ports", + "value": [ + "Ethernet0", + "Ethernet8" + ] } ] ] @@ -1706,8 +1698,13 @@ "expected_changes": [ [ { - "op": "remove", - "path": "/VLAN/Vlan1000/dhcp_servers/0" + "op": "replace", + "path": "/VLAN/Vlan1000/dhcp_servers", + "value": [ + "192.0.0.2", + "192.0.0.3", + "192.0.0.4" + ] } ] ] @@ -2336,8 +2333,8 @@ "value": { "Ethernet0": { "alias": "Eth1", - "description": "Ethernet0 100G link", "lanes": "67", + "description": "Ethernet0 100G link", "speed": "100000" } } @@ -3832,6 +3829,28 @@ } ], "expected_changes": [ + [ + { + "op": "replace", + "path": "/ACL_TABLE/EVERFLOW/ports", + "value": [ + "Ethernet64", + "Ethernet68", + "Ethernet72" + ] + } + ], + [ + { + "op": "replace", + "path": "/ACL_TABLE/EVERFLOWV6/ports", + "value": [ + "Ethernet64", + "Ethernet68", + "Ethernet72" + ] + } + ], [ { "op": "add", @@ -4175,20 +4194,6 @@ "value": "up" } ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOW/ports/0", - "value": "Ethernet64" - } - ], - [ - { - "op": "add", - "path": "/ACL_TABLE/EVERFLOWV6/ports/0", - "value": "Ethernet64" - } - ], [ { "op": "add", @@ -5365,6 +5370,26 @@ } ], "expected_changes": [ + [ + { + "op": "replace", + "path": "/ACL_TABLE/EVERFLOW/ports", + "value": [ + "Ethernet68", + "Ethernet72" + ] + } + ], + [ + { + "op": "replace", + "path": "/ACL_TABLE/EVERFLOWV6/ports", + "value": [ + "Ethernet68", + "Ethernet72" + ] + } + ], [ { "op": "remove", @@ -5583,18 +5608,6 @@ "path": "/BGP_NEIGHBOR/fc00::a/admin_status" } ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOW/ports/0" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOWV6/ports/0" - } - ], [ { "op": "remove", diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 9ba0c2508..5e0eab677 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -2232,6 +2232,91 @@ def verify_moves(self, ops, moves): moves_ops.extend(move.get_jsonpatch()) self.assertCountEqual(ops, moves_ops) + +class TestBulkLeafListMoveGenerator(unittest.TestCase): + def setUp(self): + path_addressing = PathAddressing() + self.generator = ps.BulkLeafListMoveGenerator(path_addressing) + + def test_generate__leaf_list_items_removed__single_replace_move(self): + """Removing items from a leaf-list should produce one REPLACE move.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4", "Ethernet8"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet8"], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/EVERFLOW/ports", + "value": ["Ethernet8"]}]) + + def test_generate__leaf_list_items_added__single_replace_move(self): + """Adding items to a leaf-list should produce one REPLACE move.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4", "Ethernet8"], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/EVERFLOW/ports", + "value": ["Ethernet0", "Ethernet4", "Ethernet8"]}]) + + def test_generate__leaf_list_unchanged__no_moves(self): + """Identical leaf-lists should produce no moves.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4"], "type": "MIRROR"}}}, + ex_ops=[]) + + def test_generate__non_list_fields_differ__no_moves(self): + """Non-list field changes should not produce moves from this generator.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "L3"}}}, + ex_ops=[]) + + def test_generate__list_of_dicts__no_moves(self): + """Lists of dicts (not leaf-lists) should be skipped.""" + self.verify( + current={"TABLE": {"KEY": {"items": [{"a": 1}, {"b": 2}]}}}, + target={"TABLE": {"KEY": {"items": [{"a": 1}]}}}, + ex_ops=[]) + + def test_generate__list_only_in_current__no_moves(self): + """List exists in current but not target — not a REPLACE, skip.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"type": "MIRROR"}}}, + ex_ops=[]) + + def test_generate__list_only_in_target__no_moves(self): + """List exists in target but not current — handled by other generators, skip.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"type": "MIRROR", "ports": ["Ethernet0"]}}}, + ex_ops=[]) + + def test_generate__leaf_list_all_items_removed__single_replace_move(self): + """Removing all items from a leaf-list should produce one REPLACE with empty list.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": [], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/EVERFLOW/ports", "value": []}]) + + def test_generate__multiple_tables_with_leaf_lists__multiple_moves(self): + """Multiple differing leaf-lists should each get a REPLACE move.""" + self.verify( + current={"ACL_TABLE": { + "T1": {"ports": ["Ethernet0", "Ethernet4"], "type": "L3"}, + "T2": {"ports": ["Ethernet8", "Ethernet12"], "type": "MIRROR"}}}, + target={"ACL_TABLE": { + "T1": {"ports": ["Ethernet0"], "type": "L3"}, + "T2": {"ports": ["Ethernet12"], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/T1/ports", "value": ["Ethernet0"]}, + {"op": "replace", "path": "/ACL_TABLE/T2/ports", "value": ["Ethernet12"]}]) + + def verify(self, current, target, ex_ops): + diff = ps.Diff(current, target) + moves = list(self.generator.generate(diff)) + moves_ops = [] + for move in moves: + moves_ops.extend(move.get_jsonpatch()) + self.assertCountEqual(ex_ops, moves_ops) + + class TestLowLevelMoveGenerator(unittest.TestCase): def setUp(self): path_addressing = PathAddressing() @@ -3332,7 +3417,8 @@ def verify(self, algo, algo_class): ps.BulkKeyLevelMoveGenerator, ps.KeyLevelMoveGenerator, ps.BulkKeyGroupLowLevelMoveGenerator, - ps.BulkLowLevelMoveGenerator] + ps.BulkLowLevelMoveGenerator, + ps.BulkLeafListMoveGenerator] expected_extenders = [ps.RequiredValueMoveExtender, ps.UpperLevelMoveExtender, ps.DeleteInsteadOfReplaceMoveExtender, From 6c5a4fc144f0ae5409dae9612256b9018f5f34b0 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Fri, 11 Sep 2026 13:29:59 -0700 Subject: [PATCH 5/7] [generic_config_updater] Cache loadData() calls to reduce redundant YANG parsing (#4476) (#4815) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two caches in this PR target different layers: 1. _currently_loaded_hash in SonicYangCfg.loadData() — skips re-parsing when the same config (by content hash) is loaded consecutively. This helps when multiple validators call loadData() with identical config within a single move validation. 2. _validate_config_cache in ConfigWrapper.validate_config_db() — caches the validation result for a given config hash, so if the same config state is validated again later, it returns the cached pass/fail without calling loadData() at all. Per-operation analysis Operation Helps? Why REMOVE (individual) ❌ No Each DFS step removes one item → unique config at each step. Neither cache hits because every state is different. ADD ⚠️ Marginal Typically 1 move → few loadData calls total. Cache might save 1 call if FullConfigMoveValidator and NoDependencyMoveValidator validate the same state. REPLACE (scalar) ⚠️ Marginal Same as ADD — few moves, small absolute savings. REMOVE (batched via #4478) ✅ Yes #4478 collapses N individual REMOVEs into 1 bulk REPLACE move. That single move still triggers multiple validator calls with the same config. Cache deduplicates those, reducing loads/move from ~10.6x to ~7.7 --------- (cherry picked from commit 5d54e441ad9a7881ca4182a56e77f45a8e44300b) Signed-off-by: rimunagala --- generic_config_updater/gu_common.py | 39 ++++++++- generic_config_updater/patch_sorter.py | 14 +-- .../generic_config_updater/gu_common_test.py | 86 +++++++++++++++++++ .../patch_sorter_test.py | 48 +++++++++++ 4 files changed, 176 insertions(+), 11 deletions(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 61aed505d..b9cedc5ac 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -9,6 +9,7 @@ import copy import re import os +import hashlib from sonic_py_common import logger, multi_asic from enum import Enum from functools import cmp_to_key @@ -81,6 +82,8 @@ def __init__(self, yang_dir=YANG_DIR, scope=multi_asic.DEFAULT_NAMESPACE): self.scope = scope self.yang_dir = YANG_DIR self.sonic_yang_with_loaded_models = None + self._validate_config_cache = {} + self._currently_loaded_hash = None def get_config_db_as_json(self): return get_config_db_as_json(self.scope) @@ -138,6 +141,16 @@ def validate_sonic_yang_config(self, sonic_yang_as_json): return False, ex def validate_config_db_config(self, config_db_as_json): + # Cache validation results by config content hash. + # validate_config_db_config is a pure function: same config always produces + # the same result. Caching avoids redundant loadData() calls when the DFS + # revisits the same config state during backtracking. + _cache_key = hashlib.md5( + json.dumps(config_db_as_json, sort_keys=True).encode() + ).hexdigest() + if _cache_key in self._validate_config_cache: + return self._validate_config_cache[_cache_key] + sy = self.create_sonic_yang_with_loaded_models() # TODO: Move these validators to YANG models @@ -152,14 +165,22 @@ def validate_config_db_config(self, config_db_as_json): # tuple / SonicYangException, so callers retain full error # signal -- only the duplicate syslog spam is silenced. sy.loadData(config_db_as_json, quiet=True) + self._currently_loaded_hash = _cache_key for supplemental_yang_validator in supplemental_yang_validators: success, error = supplemental_yang_validator(config_db_as_json) if not success: - return success, error + result = (success, error) + self._validate_config_cache[_cache_key] = result + return result except sonic_yang.SonicYangException as ex: - return False, str(ex) + self._currently_loaded_hash = None + result = (False, str(ex)) + self._validate_config_cache[_cache_key] = result + return result - return True, None + result = (True, None) + self._validate_config_cache[_cache_key] = result + return result def validate_field_operation(self, old_config, target_config): """ @@ -542,7 +563,17 @@ def find_ref_paths(self, paths, config, reload_config: bool = True): # caller passes reload_config=False (e.g. the recursive # __remove_dependents path). Load whenever nothing is loaded yet. if reload_config or sy.root is None: - sy.loadData(config) + _config_hash = hashlib.md5( + json.dumps(config, sort_keys=True).encode() + ).hexdigest() + already_loaded = ( + self.config_wrapper is not None and + self.config_wrapper._currently_loaded_hash == _config_hash + ) + if not already_loaded: + sy.loadData(config) + if self.config_wrapper is not None: + self.config_wrapper._currently_loaded_hash = _config_hash # Force to be a list if not isinstance(paths, list): diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index cc2343fbf..de7e5e368 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -1025,15 +1025,15 @@ def _validate_replace(self, move, diff, simulated_config): """ deleted_paths, added_paths = self._get_paths(diff.current_config, simulated_config, []) - # Note: on replace operations we are loading both current and simulated configs so we have to - # load twice :( - - # For deleted paths, we check the current config has no dependencies between nodes under the removed path - if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True): + # Validate added_paths against simulated_config first: FullConfigMoveValidator has + # already loaded simulated_config into the sy singleton (via validate_config_db_config), + # so _currently_loaded_hash will match and find_ref_paths skips loadData. + # Then validate deleted_paths against current_config (requires a fresh loadData). + # This ordering gives 2 loadData calls instead of 3 for REPLACE operations. + if not self._validate_paths_config(added_paths, simulated_config, reload_config=True): return False - # For added paths, we check the simulated config has no dependencies between nodes under the added path - if not self._validate_paths_config(added_paths, simulated_config, reload_config=True): + if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True): return False return True diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 68be5e325..8619acc43 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -221,6 +221,92 @@ def test_validate_config_db_config__invalid_config__returns_false(self): self.assertEqual(expected, actual) self.assertIsNotNone(error) + def test_validate_config_db_config__same_config_called_twice__loadData_called_once(self): + """Cache hit: second call with same config must not call loadData again.""" + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + mock_sy.loadYangModel = MagicMock() + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + config = {"ACL_TABLE": {}} + + config_wrapper.validate_config_db_config(config) + config_wrapper.validate_config_db_config(config) + + # loadData should only be called once — second call hits the result cache + mock_sy.loadData.assert_called_once() + + def test_validate_config_db_config__different_configs__loadData_called_each_time(self): + """Cache miss: different configs must each call loadData.""" + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + config_a = {"ACL_TABLE": {"rule1": {}}} + config_b = {"ACL_TABLE": {"rule2": {}}} + + config_wrapper.validate_config_db_config(config_a) + config_wrapper.validate_config_db_config(config_b) + + self.assertEqual(2, mock_sy.loadData.call_count) + + def test_find_ref_paths__after_validate_same_config__loadData_skipped(self): + """ + Core optimization: if validate_config_db_config already loaded config into sy, + find_ref_paths with the same config must skip loadData entirely. + """ + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + mock_sy.root = MagicMock() + mock_sy.root.find_path = MagicMock(return_value=MagicMock(data=MagicMock(return_value=[]))) + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + path_addressing = gu_common.PathAddressing(config_wrapper) + config = {"ACL_TABLE": {}} + + # First: validate loads the config into sy and sets _currently_loaded_hash + config_wrapper.validate_config_db_config(config) + self.assertIsNotNone(config_wrapper._currently_loaded_hash, + "validate_config_db_config should have set _currently_loaded_hash") + load_count_after_validate = mock_sy.loadData.call_count + + # Second: find_ref_paths with same config should NOT call loadData again + path_addressing.find_ref_paths("/ACL_TABLE", config, reload_config=True) + load_count_after_find = mock_sy.loadData.call_count + + self.assertEqual(load_count_after_validate, load_count_after_find, + "find_ref_paths should skip loadData when validate already loaded same config") + + def test_find_ref_paths__after_validate_different_config__loadData_called(self): + """ + Cache miss in find_ref_paths: if validate loaded config_A, calling find_ref_paths + with config_B must still call loadData. + """ + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + mock_sy.root = MagicMock() + mock_sy.root.find_path = MagicMock(return_value=MagicMock(data=MagicMock(return_value=[]))) + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + path_addressing = gu_common.PathAddressing(config_wrapper) + config_a = {"ACL_TABLE": {"rule1": {}}} + config_b = {"ACL_TABLE": {"rule2": {}}} + + config_wrapper.validate_config_db_config(config_a) + self.assertIsNotNone(config_wrapper._currently_loaded_hash, + "validate_config_db_config should have set _currently_loaded_hash") + load_count_after_validate = mock_sy.loadData.call_count + + path_addressing.find_ref_paths("/ACL_TABLE", config_b, reload_config=True) + load_count_after_find = mock_sy.loadData.call_count + + self.assertEqual(load_count_after_find, load_count_after_validate + 1, + "find_ref_paths should call loadData exactly once when config differs") + def test_validate_bgp_peer_group__valid_non_intersecting_ip_ranges__returns_true(self): # Arrange config_wrapper = gu_common.ConfigWrapper() diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 5e0eab677..e31e6f242 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1396,6 +1396,54 @@ def test_validate__replace_list_item_different_location_than_target_and_no_deps_ # Act and assert self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))[0]) + def test_validate_replace__calls_find_ref_paths_simulated_config_before_current_config(self): + """ + _validate_replace must check added_paths/simulated_config FIRST, then deleted_paths/current_config. + This ordering lets find_ref_paths reuse the sy singleton already loaded with simulated_config + by FullConfigMoveValidator (via _currently_loaded_hash), saving one loadData call per REPLACE. + """ + config_wrapper = ConfigWrapper() + mock_pa = MagicMock(spec=PathAddressing) + + # Track which config is passed to find_ref_paths in call order + configs_seen = [] + + def track_find_ref_paths(paths, config, reload_config=True): + configs_seen.append(config) + return [] # no refs → validation passes + mock_pa.find_ref_paths = MagicMock(side_effect=track_find_ref_paths) + mock_pa.create_path = PathAddressing.create_path + + validator = ps.NoDependencyMoveValidator(mock_pa, config_wrapper) + + # Patch _get_paths on the validator instance (not on mock_pa — _get_paths is a method + # on NoDependencyMoveValidator, not on PathAddressing) + deleted_paths = ["/PORT/Ethernet0"] + added_paths = ["/PORT/Ethernet4"] + validator._get_paths = MagicMock(return_value=(deleted_paths, added_paths)) + + current_config = {"PORT": {"Ethernet0": {"lanes": "0"}}} + simulated_config = {"PORT": {"Ethernet4": {"lanes": "4"}}} + diff = ps.Diff(current_config, simulated_config) + + # Create a move mock with the right attributes, wrapped in a group mock + # that is iterable (validate() does `for move in group:`) + inner_move = MagicMock() + inner_move.op_type = OperationType.REPLACE + inner_move.path = "" + group = MagicMock() + group.__iter__ = MagicMock(return_value=iter([inner_move])) + + validator.validate(group, diff, simulated_config) + + # Assert: simulated_config must be checked first (for added_paths), + # then current_config (for deleted_paths) + self.assertEqual(len(configs_seen), 2, "find_ref_paths should be called exactly twice") + self.assertIs(configs_seen[0], simulated_config, + "First find_ref_paths call must use simulated_config (for added_paths)") + self.assertIs(configs_seen[1], current_config, + "Second find_ref_paths call must use current_config (for deleted_paths)") + def prepare_config(self, config, patch): return patch.apply(config) From befb0eb1704b05e8fdee52c442e77cc94a76271a Mon Sep 17 00:00:00 2001 From: rimunagala Date: Fri, 11 Sep 2026 14:21:21 -0700 Subject: [PATCH 6/7] fix GCU patch sorter crash on create-only field change referenced by a leaf-list (#4668) (#4816) #### Why I did it The GCU (`generic_config_updater`) patch sorter aborted with an unhandled `ValueError ("'' is not in list")` when a `config replace` / apply-patch changed a create-only PORT field (e.g. `lanes` during a breakout) while that port was a member of a multi-member leaf-list such as `ACL_TABLE.ports`. During move validation the sorter transiently removes the port from the leaf-list in the simulated intermediate config, but a still-present leafref to it then fails to resolve (`list.index` raises `ValueError`). The exception propagated out of `RemoveCreateOnlyDependencyMoveValidator._validate_member` and aborted the whole sort, so any GCU/config-replace-based breakout failed on a realistic switch config (auto-rollback). The native `config interface breakout` path avoids GCU and is unaffected. #### How I did it Treat an unresolvable reference in a simulated intermediate config as an *invalid move*: catch the error in `_validate_member` and return `False` so the DFS backtracks to a valid ordering instead of letting the exception propagate and abort the whole sort. #### How to verify it Added a regression test that reproduces the create-only PORT field change against a multi-member `ACL_TABLE.ports` leaf-list and asserts the move is rejected rather than raising: ``` python3 -m pytest tests/generic_config_updater/patch_sorter_test.py \ -k test_validate__unresolvable_ref_in_simulated_config__move_rejected ``` Verified the new test **fails** with the original code (raising the reported `ValueError`) and **passes** with the fix. (cherry picked from commit 0552d0d24f67b287d74ef82ef7922f389f715c5f) Signed-off-by: rimunagala Co-authored-by: Brad House - Nexthop --- generic_config_updater/patch_sorter.py | 55 +++++- .../patch_sorter_test.py | 156 ++++++++++++++++++ 2 files changed, 206 insertions(+), 5 deletions(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index de7e5e368..376cf60bc 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -748,6 +748,7 @@ class RemoveCreateOnlyDependencyMoveValidator: def __init__(self, path_addressing): self.path_addressing = path_addressing self.create_only_filter = CreateOnlyFilter(path_addressing).get_filter() + self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - RemoveCreateOnly") def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]: # Note: group is not used by this validator @@ -822,7 +823,28 @@ def _validate_member(self, tokens, member_name, current_config, target_config, s return False member_path = f"/{table_to_check}/{member_name}" - for ref_path in self.path_addressing.find_ref_paths(member_path, simulated_config, reload_config=reload_config): + try: + ref_paths = self.path_addressing.find_ref_paths( + member_path, simulated_config, reload_config=reload_config) + except (ValueError, KeyError) as e: + # An unresolvable or malformed reference against the simulated intermediate config + # raises here. The motivating case: a create-only field change (e.g. a PORT breakout + # that rewrites lanes) has transiently removed this member from a multi-member leaf-list + # (e.g. ACL_TABLE.ports) while a leafref to it is still present, so the dangling leafref + # fails to resolve (list.index raises ValueError). Other resolution failures in the + # xpath<->configdb-path conversion (schema/key-count mismatches raise ValueError; a table + # with no YANG model raises KeyError) likewise indicate the reference cannot be resolved + # against this intermediate config. In every case the ordering is an invalid intermediate + # move, so reject it and let the sort backtrack rather than aborting the whole sort. + # Scope is deliberately limited to reference-resolution errors: a loadData failure + # (sonic_yang.SonicYangException) is not caught here because FullConfigMoveValidator has + # already loaded this config into the sy singleton, so find_ref_paths skips loadData. + self.logger.log_debug( + f"Rejecting move: reference resolution failed against simulated config " + f"for '{member_path}': {type(e).__name__}: {e}") + return False + + for ref_path in ref_paths: if not self.path_addressing.has_path(current_config, ref_path): return False @@ -963,6 +985,7 @@ class NoDependencyMoveValidator: def __init__(self, path_addressing, config_wrapper): self.path_addressing = path_addressing self.config_wrapper = config_wrapper + self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - NoDependency") def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]: reload_config = True @@ -979,7 +1002,8 @@ def __validate_move(self, move, diff, simulated_config, reload_config: bool = Tr if operation_type == OperationType.ADD: # For add operation, we check the simulated config has no dependencies between nodes under the added path - if not self._validate_paths_config([path], simulated_config, reload_config): + if not self._validate_paths_config([path], simulated_config, reload_config, + reject_on_unresolvable_ref=True): return False elif operation_type == OperationType.REMOVE: # For remove operation, we check the current config has no dependencies between nodes under the removed path @@ -1030,7 +1054,8 @@ def _validate_replace(self, move, diff, simulated_config): # so _currently_loaded_hash will match and find_ref_paths skips loadData. # Then validate deleted_paths against current_config (requires a fresh loadData). # This ordering gives 2 loadData calls instead of 3 for REPLACE operations. - if not self._validate_paths_config(added_paths, simulated_config, reload_config=True): + if not self._validate_paths_config(added_paths, simulated_config, reload_config=True, + reject_on_unresolvable_ref=True): return False if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True): @@ -1100,11 +1125,31 @@ def _get_list_paths(self, current_list, target_list, tokens): return deleted_paths, added_paths - def _validate_paths_config(self, paths, config, reload_config: bool = True): + def _validate_paths_config(self, paths, config, reload_config: bool = True, + reject_on_unresolvable_ref: bool = False): """ validates all config under paths do not have config and its references + + reject_on_unresolvable_ref: set only when 'config' is the transient simulated intermediate + state. A dangling leafref in that state (e.g. a create-only PORT change that has transiently + removed the port from a leaf-list such as ACL_TABLE.ports while a reference to it lingers) + makes find_ref_paths raise ValueError; a table with no YANG model makes it raise KeyError. + Either way it is an invalid intermediate move, so reject it and let the sort backtrack rather + than aborting. When 'config' is diff.current_config (a valid committed state) the flag stays + False: such an error there is genuine and must surface instead of being silently swallowed. + Scope is limited to reference-resolution errors; a loadData failure + (sonic_yang.SonicYangException) is not caught because the config is already loaded into the sy + singleton by FullConfigMoveValidator, so find_ref_paths skips loadData for it. """ - refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config) + try: + refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config) + except (ValueError, KeyError) as e: + if reject_on_unresolvable_ref: + self.logger.log_debug( + f"Rejecting move: reference resolution failed against simulated config " + f"for {paths}: {type(e).__name__}: {e}") + return False + raise for ref in refs: for path in paths: if ref.startswith(path): diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index e31e6f242..0b2722727 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1246,6 +1246,78 @@ def setUp(self): path_addressing = ps.PathAddressing(config_wrapper) self.validator = ps.NoDependencyMoveValidator(path_addressing, config_wrapper) + def test_validate__unresolvable_ref_in_simulated_config__add_rejected(self): + # A dangling leafref in the transient simulated intermediate config makes find_ref_paths + # raise ValueError. For a simulated-config-facing check (ADD here), the validator must reject + # the move (return False) so the sort backtracks, rather than letting the exception abort the + # whole sort. + current_config = {"PORT": {"Ethernet0": {}}} + target_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("'Ethernet0' is not in list")) + + # Must not raise; the move is rejected so the DFS can backtrack. Pin the exact arguments so a + # regression swapping simulated_config <-> diff.current_config in the ADD branch is caught + # (simulated_config is value-distinct from current_config here: it carries the added ACL_TABLE). + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], simulated_config, reload_config=True) + + def test_validate__unresolvable_ref_in_simulated_config__replace_rejected(self): + # REPLACE is the operation type for a PORT breakout (rewriting lanes while an ACL reference + # lingers). _validate_replace validates added_paths against the simulated intermediate config, + # so an unresolvable reference there (KeyError here, e.g. a table with no YANG model) must + # reject the move rather than aborting the sort. + current_config = {"PORT": {"Ethernet0": {}}} + target_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], [])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=KeyError("ACL_TABLE")) + + # Must not raise; the move is rejected so the DFS can backtrack. Pin the config argument so a + # regression routing REPLACE added_paths through diff.current_config instead of the simulated + # config is caught (the two configs are value-distinct: simulated carries the added ACL_TABLE). + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], simulated_config, reload_config=True) + + def test_validate__value_error_on_current_config__propagates(self): + # A check against diff.current_config (a valid committed state) is not simulated, so a + # ValueError from find_ref_paths signals a genuine schema error and must propagate rather + # than being silently swallowed as a move rejection. + current_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + target_config = {"PORT": {"Ethernet0": {}}} + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("Keys in configDb not matching keys in SonicYang")) + + with self.assertRaises(ValueError): + self.validator.validate(move, diff, simulated_config) + # Pin that the REMOVE validation ran against diff.current_config (unguarded), not + # simulated_config: the re-raise must be the guaranteed-current-config path, not a lucky + # raise that a config swap could mask. Configs are value-distinct (current has ACL_TABLE). + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], diff.current_config, reload_config=True) + def test_validate__add_full_config_has_dependencies__failure(self): # Arrange # CROPPED_CONFIG_DB_AS_JSON has dependencies between PORT and ACL_TABLE @@ -2061,6 +2133,90 @@ def test_validate__lane_replacement_change(self): with self.subTest(name=test_case_name): self._run_single_test(test_cases[test_case_name]) + def test_validate__unresolvable_ref_in_simulated_config__move_rejected(self): + # A create-only PORT field change (breakout rewriting lanes) can transiently + # remove the port from a multi-member leaf-list (e.g. ACL_TABLE.ports) in the simulated + # intermediate config while a leafref to it is still present. Resolving that dangling + # leafref raises ValueError ("'' is not in list"). The validator must treat this as + # an invalid intermediate move (return False) so the sort backtracks, rather than letting + # the exception propagate and abort the whole sort. + current_config = { + "PORT": { + "Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]} + } + } + target_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet280"]} + } + } + # The transient intermediate config the sorter is validating: Ethernet312's create-only + # 'lanes' field has already been rewritten toward the target, but the ACL_TABLE leafref to + # it has not been removed yet, so the reference is momentarily dangling. Resolving it raises + # ValueError. Kept value-distinct from current_config (different lanes) so the argument + # assertion below would catch a regression that passed current_config to find_ref_paths. + simulated_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]} + } + } + + move = JsonMoveGroup("", Mock()) + diff = ps.Diff(current_config, target_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("'Ethernet312' is not in list")) + + # Must not raise; the move is rejected so the DFS can backtrack. + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + # Pin the assertion to the code path under test: the rejection must come from the guarded + # find_ref_paths call raising, resolved against the simulated (intermediate) config. Asserting + # the exact arguments also catches a regression that swapped simulated_config <-> current_config. + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + "/PORT/Ethernet312", simulated_config, reload_config=True) + + def test_validate__keyerror_in_simulated_config__move_rejected(self): + # find_ref_paths also raises KeyError (not just ValueError) when a referenced path's table + # has no YANG model in the simulated intermediate config. The guard must treat that the same + # way as an unresolvable reference: reject the move so the sort backtracks, not abort it. + current_config = { + "PORT": { + "Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"} + } + } + target_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + } + } + # Value-distinct from current_config (lanes already rewritten toward the target) so the + # argument assertion below catches a regression that passed current_config instead. + simulated_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + } + } + + move = JsonMoveGroup("", Mock()) + diff = ps.Diff(current_config, target_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=KeyError("ACL_TABLE")) + + # Must not raise; the move is rejected so the DFS can backtrack. + self.assertFalse(self.validator.validate(move, diff, simulated_config)[0]) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + "/PORT/Ethernet312", simulated_config, reload_config=True) + def _run_single_test(self, test_case): # Arrange expected = test_case['expected'] From 2fe029a1d17628101a6401cba77617a4f14bb349 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Fri, 11 Sep 2026 15:46:20 -0700 Subject: [PATCH 7/7] [generic_config_updater]: Remove empty leaf-lists from the simulated ConfigDB target (#4731) (#4817) Emptying a leaf-list through `config apply-patch` left an invalid value in CONFIG_DB and made the patch application fail. Removing the last item of e.g. BGP_ALLOWED_PREFIXES|DEPLOYMENT_ID|0 prefixes_v4 wrote `prefixes_v4@ = ""` and the command reported "after applying patch to config, there are still some parts not updated". Every later patch touching that field then failed YANG validation with `"" does not conform`. Root cause is that CONFIG_DB has no representation for an empty leaf-list. set_entry serializes [] to an empty string, which is read back as [""], so a target config asking for [] can never be satisfied and the final verify_same_json in PatchApplier.apply always mismatches. The canonical way to express "this leaf-list has no items" is for the field to be absent. Add PatchWrapper.simulate_config_db_patch, which simulates the patch and then drops empty leaf-lists, and use it everywhere the result is a ConfigDB target: PatchApplier.apply, all three sorters (StrictPatchSorter.sort, NonStrictPatchSorter.sort and PatchSorter.sort), and convert_config_db_patch_to_sonic_yang_patch. The sorter now produces a field level remove instead of a replace with an empty list, the change applier writes the entry without the field, and the readback matches the target. simulate_patch stays shape agnostic so the SonicYang format path is unaffected. In SonicYang json a list holds yang list entries rather than leaf-list items, so ConfigDB canonicalization must not be applied there. The guard in BulkLeafListMoveGenerator is kept as defence in depth so the generator never emits a bulk replace with an empty list. This intentionally reverses one assertion added by #4478. test_generate__leaf_list_all_items_removed__single_replace_move expected the generator to emit REPLACE with an empty list for an emptied leaf-list. That is the value CONFIG_DB cannot store, so the test is renamed to test_generate__leaf_list_all_items_removed__no_bulk_replace_move and now expects no bulk move, leaving the transition to the granular removal generators. The batching optimisation from #4478 is untouched for every non-empty leaf-list. Verified on docker-sonic-vs built from master using the reproduction steps from sonic-net/sonic-buildimage#27818. Before the change CONFIG_DB ended up with prefixes_v4@ = "" and apply-patch failed; after the change the field is removed and the patch applies successfully, including re-adding the leaf-list afterwards. The full generic_config_updater suite passes in the same container, 558 passed against a 554 passed baseline. (cherry picked from commit 4464c12241d3f87f8d43c71e60a57a438484aa0b) Signed-off-by: rimunagala --- generic_config_updater/generic_updater.py | 2 +- generic_config_updater/gu_common.py | 41 ++++++++++++- generic_config_updater/patch_sorter.py | 11 +++- .../generic_updater_test.py | 4 +- .../generic_config_updater/gu_common_test.py | 58 +++++++++++++++++++ .../multiasic_generic_updater_test.py | 2 +- .../patch_sorter_test.py | 46 +++++++++++++-- 7 files changed, 151 insertions(+), 13 deletions(-) diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index b7a984455..12c21a179 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -113,7 +113,7 @@ def apply(self, patch, sort=True, trace_io: Optional[IO] = None): # Generate target config self.logger.log_notice(f"{scope}: simulating the target full config after applying the patch.") - target_config = self.patch_wrapper.simulate_patch(patch, old_config) + target_config = self.patch_wrapper.simulate_config_db_patch(patch, old_config) # Validate all JsonPatch operations on specified fields self.logger.log_notice(f"{scope}: validating all JsonPatch operations are permitted on the specified fields") diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index b9cedc5ac..63c8201be 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -406,6 +406,36 @@ def _init_imitated_config_db_if_none(self): self.imitated_config_db = super().get_config_db_as_json() +def remove_empty_leaf_lists(config): + """Drop leaf-list fields whose value is an empty list. + + Expects ConfigDB shaped json, i.e. table -> key -> field. At that shape a + list valued field is always a leaf-list, so every empty list found here is + an empty leaf-list. Do not call this with SonicYang shaped json, where a + list is a yang list of entries rather than a leaf-list. + + ConfigDB has no representation for an empty leaf-list. set_entry serializes + [] to an empty string, which is then read back as [''], so a config that asks + for [] can never be satisfied. The canonical way to express "this leaf-list + has no items" is for the field to be absent. + + Normalizing the simulated target keeps the patch sorter, the change applier + and the final verification in agreement: the sorter emits a field-level + remove instead of a replace-with-empty-list that ConfigDB cannot store. + """ + for entries in config.values(): + if not isinstance(entries, dict): + continue + for fields in entries.values(): + if not isinstance(fields, dict): + continue + empty_leaf_lists = [field for field, value in fields.items() + if isinstance(value, list) and len(value) == 0] + for field in empty_leaf_lists: + del fields[field] + return config + + class PatchWrapper: def __init__(self, config_wrapper=None, scope=multi_asic.DEFAULT_NAMESPACE): self.scope = scope @@ -439,12 +469,21 @@ def generate_patch(self, current, target): def simulate_patch(self, patch, jsonconfig): return patch.apply(jsonconfig) + def simulate_config_db_patch(self, patch, config_db): + """Simulate a patch against ConfigDB shaped json. + + Use this instead of simulate_patch whenever the result is meant to be a + ConfigDB target, so that empty leaf-lists are normalized to absent + fields, which is the only way ConfigDB can express them. + """ + return remove_empty_leaf_lists(self.simulate_patch(patch, config_db)) + def convert_config_db_patch_to_sonic_yang_patch(self, patch): if not(self.validate_config_db_patch_has_yang_models(patch)): raise ValueError(f"Given patch is not valid") current_config_db = self.config_wrapper.get_config_db_as_json() - target_config_db = self.simulate_patch(patch, current_config_db) + target_config_db = self.simulate_config_db_patch(patch, current_config_db) current_yang = self.config_wrapper.convert_config_db_to_sonic_yang(current_config_db) target_yang = self.config_wrapper.convert_config_db_to_sonic_yang(target_config_db) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 376cf60bc..f7791db87 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -1354,7 +1354,12 @@ def _traverse(self, diff, current_ptr, target_ptr, tokens): if isinstance(current_val, list) and isinstance(target_val, list): # Only handle leaf-lists (lists of scalars) + # An empty target leaf-list is deliberately excluded: a bulk + # REPLACE with [] serializes to an empty string in CONFIG_DB, + # which is an invalid state. Those transitions are left to the + # granular removal generators, which remove the field instead. if (current_val != target_val and + len(target_val) > 0 and self._is_leaf_list(current_val) and self._is_leaf_list(target_val)): yield JsonMoveGroup( @@ -2406,7 +2411,7 @@ def sort(self, patch, algorithm=Algorithm.DFS, trace_io: Optional[IO] = None): if not(self.patch_wrapper.validate_config_db_patch_has_yang_models(patch)): raise ValueError(f"Given patch is not valid because it has changes to tables without YANG models") - target_config = self.patch_wrapper.simulate_patch(patch, current_config) + target_config = self.patch_wrapper.simulate_config_db_patch(patch, current_config) # Validate target config self.logger.log_info("Validating target config according to YANG models.") @@ -2596,7 +2601,7 @@ def __init__(self, config_wrapper, patch_wrapper, config_splitter, change_wrappe def sort(self, patch, algorithm=Algorithm.DFS, trace_io: Optional[IO] = None): current_config = self.config_wrapper.get_config_db_as_json() - target_config = self.patch_wrapper.simulate_patch(patch, current_config) + target_config = self.patch_wrapper.simulate_config_db_patch(patch, current_config) # Splitting current/target config based on YANG covered vs non-YANG covered configs self.logger.log_info("Splitting current/target config based on YANG covered vs non-YANG covered configs.") @@ -2658,7 +2663,7 @@ def __init__(self, config_wrapper, patch_wrapper, sort_algorithm_factory=None): def sort(self, patch, algorithm=Algorithm.DFS, preloaded_current_config=None, trace_io: Optional[IO] = None): current_config = preloaded_current_config if preloaded_current_config else self.config_wrapper.get_config_db_as_json() - target_config = self.patch_wrapper.simulate_patch(patch, current_config) + target_config = self.patch_wrapper.simulate_config_db_patch(patch, current_config) diff = Diff(copy.deepcopy(current_config), target_config) diff --git a/tests/generic_config_updater/generic_updater_test.py b/tests/generic_config_updater/generic_updater_test.py index 203d43574..582b8e6ac 100644 --- a/tests/generic_config_updater/generic_updater_test.py +++ b/tests/generic_config_updater/generic_updater_test.py @@ -40,7 +40,7 @@ def test_apply__no_errors__update_successful(self): # Assert patch_applier.config_wrapper.get_config_db_as_json.assert_has_calls([call(), call()]) - patch_applier.patch_wrapper.simulate_patch.assert_has_calls( + patch_applier.patch_wrapper.simulate_config_db_patch.assert_has_calls( [call(Files.MULTI_OPERATION_CONFIG_DB_PATCH, Files.CONFIG_DB_AS_JSON)]) patch_applier.patchsorter.sort.assert_has_calls([call(Files.MULTI_OPERATION_CONFIG_DB_PATCH, trace_io=None)]) patch_applier.changeapplier.apply.assert_called() @@ -59,7 +59,7 @@ def __create_patch_applier(self, create_side_effect_dict({(str(Files.CONFIG_DB_AFTER_MULTI_PATCH),): empty_tables}) patch_wrapper = Mock() - patch_wrapper.simulate_patch.side_effect = \ + patch_wrapper.simulate_config_db_patch.side_effect = \ create_side_effect_dict( {(str(Files.MULTI_OPERATION_CONFIG_DB_PATCH), str(Files.CONFIG_DB_AS_JSON)): Files.CONFIG_DB_AFTER_MULTI_PATCH}) diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 8619acc43..3ac4ea8e3 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -658,6 +658,64 @@ def test_simulate_patch__non_empty_patch__changes_applied(self): # Assert self.assertDictEqual(expected, actual) + def test_simulate_config_db_patch__empties_leaf_list__field_is_dropped(self): + # Arrange + patch_wrapper = gu_common.PatchWrapper() + config = { + "BGP_ALLOWED_PREFIXES": { + "DEPLOYMENT_ID|0": { + "prefixes_v4": ["10.20.0.0/16"], + "prefixes_v6": ["fc01:20::/64"], + } + } + } + patch = jsonpatch.JsonPatch( + [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/0"}]) + # ConfigDB cannot store an empty leaf-list, the field has to be absent + expected = {"BGP_ALLOWED_PREFIXES": {"DEPLOYMENT_ID|0": {"prefixes_v6": ["fc01:20::/64"]}}} + + # Act + actual = patch_wrapper.simulate_config_db_patch(patch, config) + + # Assert + self.assertDictEqual(expected, actual) + + def test_simulate_config_db_patch__leaf_list_still_has_items__field_is_kept(self): + # Arrange + patch_wrapper = gu_common.PatchWrapper() + config = { + "BGP_ALLOWED_PREFIXES": { + "DEPLOYMENT_ID|0": { + "prefixes_v4": ["10.20.0.0/16", "10.30.0.0/16"], + } + } + } + patch = jsonpatch.JsonPatch( + [{"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/0"}]) + expected = {"BGP_ALLOWED_PREFIXES": {"DEPLOYMENT_ID|0": {"prefixes_v4": ["10.30.0.0/16"]}}} + + # Act + actual = patch_wrapper.simulate_config_db_patch(patch, config) + + # Assert + self.assertDictEqual(expected, actual) + + def test_simulate_patch__sonic_yang_config__empty_yang_list_is_kept(self): + # Arrange + # simulate_patch must stay shape agnostic. In SonicYang json a list holds yang list + # entries, not leaf-list items, so it must not be treated as an empty leaf-list. + patch_wrapper = gu_common.PatchWrapper() + config = {"sonic-vlan:sonic-vlan": {"VLAN": {"VLAN_LIST": [{"name": "Vlan1000"}]}}} + patch = jsonpatch.JsonPatch( + [{"op": "remove", "path": "/sonic-vlan:sonic-vlan/VLAN/VLAN_LIST/0"}]) + expected = {"sonic-vlan:sonic-vlan": {"VLAN": {"VLAN_LIST": []}}} + + # Act + actual = patch_wrapper.simulate_patch(patch, config) + + # Assert + self.assertDictEqual(expected, actual) + def test_generate_patch__diff__non_empty_patch(self): # Arrange patch_wrapper = gu_common.PatchWrapper() diff --git a/tests/generic_config_updater/multiasic_generic_updater_test.py b/tests/generic_config_updater/multiasic_generic_updater_test.py index 5acdd391f..48d8cfcd1 100644 --- a/tests/generic_config_updater/multiasic_generic_updater_test.py +++ b/tests/generic_config_updater/multiasic_generic_updater_test.py @@ -16,7 +16,7 @@ class TestMultiAsicPatchApplier(unittest.TestCase): @patch('generic_config_updater.gu_common.ConfigWrapper.get_empty_tables', return_value=[]) @patch('generic_config_updater.gu_common.ConfigWrapper.get_config_db_as_json') - @patch('generic_config_updater.gu_common.PatchWrapper.simulate_patch') + @patch('generic_config_updater.gu_common.PatchWrapper.simulate_config_db_patch') @patch('generic_config_updater.generic_updater.ChangeApplier') def test_apply_patch_specific_namespace(self, mock_ChangeApplier, mock_simulate_patch, mock_get_config, mock_get_empty_tables): scope = "asic0" diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 0b2722727..3fda7c3f8 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -2493,12 +2493,12 @@ def test_generate__list_only_in_target__no_moves(self): target={"ACL_TABLE": {"EVERFLOW": {"type": "MIRROR", "ports": ["Ethernet0"]}}}, ex_ops=[]) - def test_generate__leaf_list_all_items_removed__single_replace_move(self): - """Removing all items from a leaf-list should produce one REPLACE with empty list.""" + def test_generate__leaf_list_all_items_removed__no_bulk_replace_move(self): + """Removing all items from a leaf-list should not bulk-replace with an empty list.""" self.verify( current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4"], "type": "MIRROR"}}}, target={"ACL_TABLE": {"EVERFLOW": {"ports": [], "type": "MIRROR"}}}, - ex_ops=[{"op": "replace", "path": "/ACL_TABLE/EVERFLOW/ports", "value": []}]) + ex_ops=[]) def test_generate__multiple_tables_with_leaf_lists__multiple_moves(self): """Multiple differing leaf-lists should each get a REPLACE move.""" @@ -3729,6 +3729,42 @@ def run_single_failure_case(self, data): if notfound_substrings: self.fail(f"Did not find the substrings {notfound_substrings} in the error: '{error}'") + def test_patch_sorter__remove_last_bgp_allowed_prefix__removes_field_instead_of_emptying_it(self): + current_config = { + "BGP_ALLOWED_PREFIXES": { + "DEPLOYMENT_ID|0": { + "deployment": "DEPLOYMENT_ID", + "id": 0, + "prefixes_v4": ["10.20.0.0/16"], + "prefixes_v6": ["fc00:f0::/64"], + } + } + } + patch = jsonpatch.JsonPatch([ + {"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4/0"} + ]) + sorter = self.create_patch_sorter(current_config) + + actual_changes = sorter.sort(patch) + # Removing the only item has to drop the whole field. ConfigDB cannot store an + # empty leaf-list, it would be written as "" and read back as [""]. + expected_changes = [ + JsonChange(jsonpatch.JsonPatch([ + {"op": "remove", "path": "/BGP_ALLOWED_PREFIXES/DEPLOYMENT_ID|0/prefixes_v4"} + ])) + ] + + self.assertEqual(expected_changes, actual_changes) + + target_config = PatchWrapper().simulate_config_db_patch(patch, current_config) + self.assertNotIn("prefixes_v4", target_config["BGP_ALLOWED_PREFIXES"]["DEPLOYMENT_ID|0"]) + simulated_config = current_config + for change in actual_changes: + simulated_config = change.apply(simulated_config) + is_valid, error = self.config_wrapper.validate_config_db_config(simulated_config) + self.assertTrue(is_valid, f"Change will produce invalid config. Error: {error}") + self.assertEqual(target_config, simulated_config) + def test_sort__does_not_remove_tables_without_yang_unintentionally_if_generated_change_replaces_whole_config(self): # Arrange current_config = Files.CONFIG_DB_AS_JSON # has a table without yang named 'TABLE_WITHOUT_YANG' @@ -4002,7 +4038,7 @@ def __create_patch_sorter(self, config_wrapper.get_config_db_as_json.side_effect = \ [any_current_config] - patch_wrapper.simulate_patch.side_effect = \ + patch_wrapper.simulate_config_db_patch.side_effect = \ create_side_effect_dict( {(str(patch), str(any_current_config)): any_target_config}) @@ -4095,7 +4131,7 @@ def __create_patch_sorter(self, config_wrapper.get_config_db_as_json.side_effect = \ [any_current_config, any_target_config] - patch_wrapper.simulate_patch.side_effect = \ + patch_wrapper.simulate_config_db_patch.side_effect = \ create_side_effect_dict( {(str(patch), str(any_current_config)): any_target_config})