diff --git a/config/main.py b/config/main.py index f1dc10f7c..a8766a00c 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..12c21a179 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}") @@ -112,7 +113,7 @@ def apply(self, patch, sort=True): # 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") @@ -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..63c8201be 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, 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): """ @@ -385,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 @@ -418,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) @@ -542,7 +602,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 4a1625b10..f7791db87 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 Any, IO, List, Optional, Tuple from .gu_common import OperationWrapper, OperationType, GenericConfigUpdaterError, \ JsonChange, PathAddressing, genericUpdaterLogging @@ -98,10 +99,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 @@ -322,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) @@ -465,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) @@ -497,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,8 +748,9 @@ 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): + 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 @@ -781,12 +787,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): @@ -817,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 @@ -831,10 +858,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: """ @@ -843,9 +870,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: """ @@ -860,7 +887,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 @@ -873,19 +900,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,15 +985,16 @@ 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): + 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 @@ -974,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 @@ -1020,15 +1049,16 @@ 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, + reject_on_unresolvable_ref=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 @@ -1095,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): @@ -1117,8 +1167,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 @@ -1156,10 +1206,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 @@ -1180,7 +1230,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 @@ -1195,9 +1245,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: """ @@ -1217,11 +1267,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): @@ -1252,13 +1302,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): @@ -1267,6 +1317,67 @@ 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) + # 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( + 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. @@ -1296,7 +1407,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)) @@ -1329,7 +1440,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)) @@ -1524,7 +1635,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) @@ -1536,7 +1647,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) @@ -1548,7 +1659,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 @@ -1575,55 +1686,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) + current_field = self.__fetch_path(current_config, tokens) + target_field = self.__fetch_path(target_config, tokens) - if table_to_check not in current_config: + # 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 - current_members = current_config[table_to_check] - if not current_members: - 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(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: @@ -1743,7 +1888,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): @@ -1753,7 +1901,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: @@ -1770,7 +1918,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): @@ -1780,7 +1928,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): @@ -1790,7 +1938,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: @@ -1809,7 +1960,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): @@ -1823,7 +1977,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() @@ -2004,12 +2161,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 [] @@ -2021,21 +2248,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([]) @@ -2058,7 +2304,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] @@ -2069,10 +2316,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(): @@ -2089,7 +2338,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): @@ -2111,11 +2361,12 @@ 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): - move_generators = [RemoveCreateOnlyDependencyMoveGenerator(self.path_addressing), - LowLevelMoveGenerator(self.path_addressing)] + 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 = [BulkKeyLevelMoveGenerator(self.path_addressing), + move_non_extendable_generators = [RemoveCreateOnlyDependencyMoveGenerator(self.path_addressing), + BulkLeafListMoveGenerator(self.path_addressing), + BulkKeyLevelMoveGenerator(self.path_addressing), KeyLevelMoveGenerator(self.path_addressing), BulkKeyGroupLowLevelMoveGenerator(self.path_addressing), BulkLowLevelMoveGenerator(self.path_addressing)] @@ -2134,11 +2385,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") @@ -2152,7 +2403,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 @@ -2160,7 +2411,7 @@ def sort(self, patch, algorithm=Algorithm.DFS): 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.") @@ -2170,7 +2421,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 @@ -2207,12 +2458,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. @@ -2343,9 +2599,9 @@ 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) + 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.") @@ -2380,7 +2636,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 '.'}") @@ -2403,16 +2659,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) + target_config = self.patch_wrapper.simulate_config_db_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/files/patch_sorter_test_success.json b/tests/generic_config_updater/files/patch_sorter_test_success.json index 4d8cb8a5a..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" + ] } ] ] @@ -737,27 +741,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", @@ -867,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" + ] } ] ] @@ -1017,42 +991,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 +1084,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 +1093,8 @@ [ { "op": "remove", - "path": "/VLAN_MEMBER" - } - ], - [ + "path": "/PORT/Ethernet3" + }, { "op": "remove", "path": "/PORT" @@ -1159,8 +1107,8 @@ "value": { "Ethernet0": { "alias": "Eth1", - "description": "Ethernet0 100G link", "lanes": "65, 66, 67, 68", + "description": "Ethernet0 100G link", "speed": "100000" } } @@ -1538,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" + ] } ] ] @@ -1753,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" + ] } ] ] @@ -2383,8 +2333,8 @@ "value": { "Ethernet0": { "alias": "Eth1", - "description": "Ethernet0 100G link", "lanes": "67", + "description": "Ethernet0 100G link", "speed": "100000" } } @@ -3879,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", @@ -4222,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", @@ -5412,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", @@ -5630,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", @@ -5692,5 +5658,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/generic_updater_test.py b/tests/generic_config_updater/generic_updater_test.py index 727c07b02..582b8e6ac 100644 --- a/tests/generic_config_updater/generic_updater_test.py +++ b/tests/generic_config_updater/generic_updater_test.py @@ -40,9 +40,9 @@ 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)]) + 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)]) @@ -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}) @@ -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/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 68be5e325..3ac4ea8e3 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() @@ -572,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/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/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 1f021b57a..3fda7c3f8 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) @@ -1239,21 +1246,93 @@ 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 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 +1342,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 +1354,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 +1369,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 +1384,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 +1398,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 +1412,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 +1424,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 +1455,66 @@ 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 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) @@ -1395,110 +1529,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 +1673,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 @@ -1993,16 +2133,100 @@ 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'] 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) @@ -2212,6 +2436,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__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=[]) + + 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() @@ -2525,8 +2834,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): @@ -2537,8 +2858,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): @@ -3283,12 +3616,13 @@ 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] + ps.BulkLowLevelMoveGenerator, + ps.BulkLeafListMoveGenerator] expected_extenders = [ps.RequiredValueMoveExtender, ps.UpperLevelMoveExtender, ps.DeleteInsteadOfReplaceMoveExtender, @@ -3334,12 +3668,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 +3681,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: @@ -3400,13 +3729,50 @@ 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' 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)]))] @@ -3672,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}) @@ -3765,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}) 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")