From be57e378b80b2cd75e3ef8fa274145f1a171fb33 Mon Sep 17 00:00:00 2001 From: Xincun Li <147451452+xincunli-sonic@users.noreply.github.com> Date: Wed, 25 Mar 2026 09:47:57 -0700 Subject: [PATCH 1/8] Create GCU wheel (#4310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Create GCU wheel Signed-off-by: Xincun Li * fix test Signed-off-by: Xincun Li * Refactor Signed-off-by: Xincun Li * Fixed show vxlan remotemac ambiguity (#4121) Summary: When users type ambiguous commands like show vxlan remote, they see a Python traceback instead of a clean error message because the CLI finds multiple matching commands and throws an backtrace and an exception. Root Cause The AliasedGroup.get_command() method calls ctx.fail() which raises a UsageError exception that propagates through Click's bash completion system, causing the traceback. Approach: Implement context-aware error handling in the [AliasedGroup.get_command() method to differentiate between: Bash completion context: Where tracebacks should be suppressed Normal command execution context: Where clean error messages should be displayed How did you do it? Added environment variable detection to handle bash completion context differently Command execution results in a clear error message without any tracebacks No changes to the existing CLI functionality Signed-off-by: Gnanapriya Sethuramarajan Signed-off-by: Xincun Li * Fix spelling typos across utilities_common, config plugins, and misc modules (#4264) * Fix spelling typos across utilities_common, config plugins, and misc modules Fix misspellings found via codespell across 37 files: utilities_common/: Neighbhor, seperate, Contants, Explicity, classs, retreive/retreived, wont, fileds, statisitics crm/: recources, neigbor flow_counter_util/: formated sonic_cli_gen/: separeted sonic_installer/: Excpetion, commond, necessarry, threhold, Verifing, orignal, reconcilation sonic_package_manager/: wether, componenets, infromation, spliting, Seperator, Wether pfcwd/: explicitely, Paramter sfputil/: EEEPROM pcieutil/: Vender acl_loader/: overriden dump/: Multipe, incluing, orignal, recieved, Proceding generic_config_updater/: relevent, acending, confing, happend config/: Defualt, configurtion, patter, seperated, obect, cummulative rcli/: commmand sonic-utilities-data/: obect (template) Signed-off-by: Rustiqly * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Signed-off-by: Rustiqly Co-authored-by: Rustiqly Co-authored-by: Lihua Yuan Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xincun Li * In route_check.py, Convey the IJSON Backend using an env variable (#4294) * Fix route_check.py to not hog a lot of memory This diff modifies the route_check.py to not invoke "show" and rather invoke the vtysh cmd directly. It then attempt to interpret one route at a time in a paginated manner. This prevents a sudden transient memory buildup. The zebra process already does the right thing and backs off when the output socket buffers are full. There is probably scope to improve that further (Refer to https://sonicfoundation.dev/2025-sonic-hackathon-most-impactful-award-spotlight-optimizing-output-buffer-memory-for-show-commands/) Signed-off-by: Venkit Kasiviswanathan * Fix merge conflicts related test failure from upstream Signed-off-by: Venkit Kasiviswanathan * Fix precommit check failure Signed-off-by: Venkit Kasiviswanathan * Revert back to using the TIMEOUT from the earlier code. Signed-off-by: Venkit Kasiviswanathan * Fixed review comments from upstream Signed-off-by: Venkit Kasiviswanathan * Removed CHUNK_SIZE as it is not used any more Signed-off-by: Venkit Kasiviswanathan * Fix multi asic connection creation (#4109) - What I did Create a cache for the SonicV2Connector objects which are created, because currently we are creating n interfaces * m namespace amount of connectors in case of multi asic implementation, which is very high and would lead to the show interface counters command to crash root@sonic:/home/admin# show interfaces counters Traceback (most recent call last): File "/usr/local/bin/portstat", line 168, in main() File "/usr/local/bin/portstat", line 158, in main portstat.cnstat_diff_print(cnstat_dict, {}, ratestat_dict, intf_list, use_json, print_all, errors_only, File "/usr/local/lib/python3.11/dist-packages/utilities_common/portstat.py", line 572, in cnstat_diff_print port_speed = self.get_port_speed(key) ^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/utilities_common/portstat.py", line 373, in get_port_speed self.db = multi_asic.connect_to_all_dbs_for_ns(ns) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/sonic_py_common/multi_asic.py", line 81, in connect_to_all_dbs_for_ns db.connect(db_id) File "/usr/lib/python3/dist-packages/swsscommon/swsscommon.py", line 2069, in connect return _swsscommon.SonicV2Connector_Native_connect(self, db_name, retry_on) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RuntimeError: Unable to connect to redis - Cannot assign requested address(1): Cannot assign requested address - How I did it Cache the connectors in a dictionary - How to verify it Run show interfaces counters command Signed-off-by: gpunathilell Signed-off-by: Venkit Kasiviswanathan * Add q3d SKUs to gcu_field_operation_validators.conf.json (#4201) Signed-off-by: arista-hpandya Signed-off-by: Venkit Kasiviswanathan * sonic-utilities: Support for clearing aggregate VOQ counters(#2001) (#4044) * Caching the current counters when sonic-clear queuecounters is executed. * Calculating and displaying the difference in counter values when the show command is run. * Providing clear CLI messaging to indicate the behavior when run from supervisor(clear aggregate VOQ counters only). * Unit test for clear aggregate VOQ counters is added verifying the data is cached and counters are cleared properly. Signed-off-by: manish Signed-off-by: Venkit Kasiviswanathan * [multi-asic][Mellanox] Add multi-ASIC support for generate_dump and update FW upgrade script (#4192) - What I did Add multi-ASIC support for generate_dump and update FW upgrade script - How I did it 1. Refactor collect_mellanox() to support multi-ASIC architecture 2. Add collect_mellanox_sai_sdk_dump() function to collect SAI SDK dumps per ASIC 3. Process CMIS host management files for each ASIC instance separately 4. Collect SAI SDK dumps in parallel for all ASICs using background processes 5. Update fast-reboot to use mlnx-fw-manager instead of mlnx-fw-upgrade.sh 6. Fix file paths to be relative to SKU folder for multi-ASIC setups 7. Support namespace-aware command execution for multi-ASIC environments - How to verify it Run regression tests Signed-off-by: Oleksandr Ivantsiv Signed-off-by: Venkit Kasiviswanathan * Added counterpoll CLI support (#4106) * Added counterpoll CLI support (enable/disable/interval/show) Signed-off-by: dhanasekar-arista * change port_attr to port_phy_attr Signed-off-by: dhanasekar-arista * add unit tests for counterpoll phy configs Signed-off-by: dhanasekar-arista --------- Signed-off-by: dhanasekar-arista Signed-off-by: Venkit Kasiviswanathan * Add current and configured frequency to DOM CLI (#4209) * Add current and configured frequency to DOM CLI Signed-off-by: Ariz Zubair * Update unit test for 400ZR. Signed-off-by: Ariz Zubair * Fix the parameter name. Signed-off-by: Ariz Zubair * Update the command reference doc. Signed-off-by: Ariz Zubair * Redact vendor details. Signed-off-by: Ariz Zubair * Added requested tx power to dom output Signed-off-by: Ariz Zubair * Update command reference. Signed-off-by: Ariz Zubair * Fix unit test. Signed-off-by: Ariz Zubair * Fix linting error. Signed-off-by: Ariz Zubair * Undo the output changes. Signed-off-by: Ariz Zubair --------- Signed-off-by: Ariz Zubair Signed-off-by: Venkit Kasiviswanathan * Fix multi asic initialization for dump command (#4108) - What I did To add initializeGlobalConfig for dump command in case of multi asic implementation, This is to prevent the error: root@dut:/home/admin# dump state interface Ethernet0 -n asic0 Traceback (most recent call last): File "/usr/local/bin/dump", line 8, in sys.exit(dump()) ^^^^^^ File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 764, in __call__ return self.main(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 717, in main rv = self.invoke(ctx) ^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 1137, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 956, in invoke return ctx.invoke(self.callback, **ctx.params) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 555, in invoke return callback(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/click/decorators.py", line 17, in new_func return f(get_current_context(), *args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/dump/main.py", line 96, in state collected_info = populate_fv(collected_info, module, namespace, ctx.obj.conn_pool, obj.return_pb2_obj()) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/dump/main.py", line 159, in populate_fv conn_pool.get(db_name, namespace) File "/usr/local/lib/python3.11/dist-packages/dump/match_infra.py", line 316, in get self.cache[ns][CONN] = self.initialize_connector(ns) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/dump/match_infra.py", line 298, in initialize_connector return SonicV2Connector(namespace=ns, use_unix_socket_path=True) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/swsscommon/swsscommon.py", line 2138, in __init__ for db_name in self.get_db_list(): ^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/swsscommon/swsscommon.py", line 2075, in get_db_list return _swsscommon.SonicV2Connector_Native_get_db_list(self) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RuntimeError: :- validateNamespace: Initialize global DB config using API SonicDBConfig::initializeGlobalConfig On multi asic system - How I did it Initialize global config - How to verify it Run unit test Signed-off-by: gpunathilell Signed-off-by: Venkit Kasiviswanathan * Fix issue that namespace is not correctly fetched in Multi ASIC environment for mirror capability checking (#4159) - What I did Fix issue sonic-net/sonic-mgmt#21690 - How I did it The logic to check the mirror capability is: orchagent exposes capability to SWITCH_CAPABILITY table in STATE_DB during initialization CLI (config mirror) fetches capability from the table when a CLI command is issued by a user. On the multi ASIC environment, the table is in ASIC's namespace. But the CLI command fetches the capability from the host. As a result it always treats mirror is unsupported and fails the test. Fixed by checking the mirror capability from the namespaces based on source and destination ports. - How to verify it Manual test. Signed-off-by: Stephen Sun Signed-off-by: Venkit Kasiviswanathan * Fix the PSU show command error message on platform without psu at all (#4151) What I did de-escalate the message when no psu had been detected at all from error to more moderate info. - How I did it simply change the print output and remove the redundance ones - How to verify it UT as well as manual test - Previous command output (if the output of a command-line utility has changed) Error: Failed to get the number of PSUs Error: Failed to get PSU status Error: failed to get PSU status from state DB - New command output (if the output of a command-line utility has changed) PSU not detected Signed-off-by: Yuanzhe Liu Signed-off-by: Venkit Kasiviswanathan * Update bash completions for sonic-utilities commands (#4163) What I did Update the bash completion files for all sonic-utilities commands to make them compatible with the current Click version. Fixes sonic-net/sonic-buildimage#24594. How I did it Use Click's documentation to generate the bash completion script for each command that is packaged from sonic-utilities and uses Click. How to verify it Tested in KVM in Trixie image. admin@vlab-01:~$ sonic-package-manager install list manifests migrate repository reset show uninstall update admin@vlab-01:~$ sonic-package-manager install list manifests migrate repository reset show uninstall update admin@vlab-01:~$ sonic-package-manager install list manifests migrate repository reset show uninstall update admin@vlab-01:~$ spm install list manifests migrate repository reset show uninstall update admin@vlab-01:~$ spm ^C admin@vlab-01:~$ show Display all 105 possibilities? (y or n) aaa buffer_pool environment icmp macsec passw-hardening runningconfiguration suppress-fib-pending vlan acl chassis event-counters interfaces management_interface pbh serial_console switch vnet arp clock fabric ip mgmt-vrf pfc services switch-hash vrf asic-sdk-health-event copp feature ipv6 mirror_session pfcwd sflow switch-trimming vrrp auto-techsupport dhcp4relay-counters fg-nhg kdump mmu platform snmpagentaddress syslog vrrp6 auto-techsupport-feature dhcp6relay_counters fg-nhg-member kubernetes muxcable policer snmptrap system-health vxlan banner dhcp_relay fg-nhg-prefix ldap nat priority-group spanning-tree system-memory warm_restart bfd dhcp_server fgnhg ldap-server ndp processes srv6 tacacs watermark bgp dhcprelay_helper flowcnt-route line ntp queue ssh techsupport ztp bmp dns flowcnt-trap lldp nvgre-tunnel radius startupconfiguration uptime boot dropcounters headroom-pool logging nvgre-tunnel-map reboot-cause storm-control users buffer ecn history mac p4-table route-map subinterfaces version admin@vlab-01:~$ config aaa cbf dropcounters interface_naming_mode loopback nvgre-tunnel-map reload spanning-tree unique-ip acl chassis ecn ipv6 macsec override-config-table replace ssh vlan apply-patch checkpoint fabric kdump mclag passw-hardening rollback subinterface vnet asic-sdk-health-event clock feature kubernetes member pbh route suppress-fib-pending vrf auto-techsupport console fg-nhg ldap mirror_session pfcwd save switch-hash vxlan auto-techsupport-feature delete-checkpoint fg-nhg-member ldap-server mmu platform serial_console switch-trimming warm_restart banner dhcp_relay fg-nhg-prefix list-checkpoints muxcable portchannel sflow switchport watermark bgp dhcp_server flowcnt-route load nat qos snmp synchronous_mode yang_config_validation bmp dhcpv4_relay hostname load_mgmt_config ntp radius snmpagentaddress syslog ztp buffer dns interface load_minigraph nvgre-tunnel rate snmptrap tacac Note that these commands don't have a completion script generated, likely because an exception is being raised when just importing that module: Cannot generate completion for counterpoll.main:cli! Cannot generate completion for debug.main:cli! Cannot generate completion for fwutil.main:cli! Cannot generate completion for psuutil.main:cli! Cannot generate completion for sfputil.main:cli! Cannot generate completion for undebug.main:cli! Signed-off-by: Venkit Kasiviswanathan * [GCU] Update WRED_PROFILE and BUFFER_POOL validators for GCU (#4219) What I did Remove strict validation for WRED_PROFILE changes Add stricter controls on BUFFER_POOL changes Other RDMA tables do not need strict validators How I did it Modify the allowlist of ops and fields How to verify it Tested on lab device admin@STR-SN5640-RDMA-1:~$ sudo config apply-patch -v buffer_pool_allowed_replace.json Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [{"op": "replace", "path": "/BUFFER_POOL/ingress_lossless_pool/size", "value": "136200192"}, {"op": "replace", "path": "/BUFFER_POOL/egress_lossy_pool/size", "value": "136200192"}] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Failed to apply patch due to: Failed to apply patch on the following scopes: - localhost: Modification of BUFFER_POOL table is illegal- validating function generic_config_updater.field_operation_validators.rdma_config_update_validator returned False Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH Try "config apply-patch -h" for help. Error: Failed to apply patch on the following scopes: - localhost: Modification of BUFFER_POOL table is illegal- validating function generic_config_updater.field_operation_validators.rdma_config_update_validator returned False Validation for RDMA tables | Table | GCU Supported | Validator Present | Allowed Ops | Notes | |---------------------------------|---------------|-------------------|-------------------------------------|-------| | WRED_PROFILE | βœ… Yes | ❌ Removed | add, replace, remove | YANG-only enforcement is sufficient | | BUFFER_POOL | 🚫 No | βœ… Yes | none (blocked) | Blocked due to potential unintended ASIC impact | | BUFFER_PROFILE | ⚠️ Limited | βœ… Yes | replace, add (field-specific) | Strictly allow-listed by validator. Only `dynamic_th` field change allowed on this table | | BUFFER_QUEUE | βœ… Yes | ❌ No | add, replace, remove (entry-level) | Field-level remove of profile is invalid (leafref β†’ "0"); entry-level remove works | | BUFFER_PG | βœ… Yes | ❌ No | add, replace, remove (entry-level) | Field-level remove of profile is invalid (leafref β†’ "0"); entry-level remove works | | BUFFER_PORT_EGRESS_PROFILE_LIST | βœ… Yes | ❌ No | add, replace, remove | No RDMA-specific validator | | BUFFER_PORT_INGRESS_PROFILE_LIST| βœ… Yes | ❌ No | add, replace, remove | No RDMA-specific validator | | QUEUE | βœ… Yes | ❌ No | add, replace, remove | Used to bind scheduler and wred_profile per (port\|queue). Remove likely unsafe unless entry-level delete is supported by YANG | | PORT_QOS_MAP | βœ… Yes | ❌ No | add, replace | Bindings only (`dscp_to_tc_map`, `tc_to_pg_map`, `tc_to_queue_map`, `tc_to_dscp_map`). Ignore PFC/PFCWD for this SKU | | SCHEDULER | βœ… Yes | ❌ No | replace | Update weight for DWRR schedulers only. Type changes not permitted | | DSCP_TO_TC_MAP | 🚫 No (blocked)| ❌ No | none (blocked) | Observed failure: config apply-patch fails at β€œPatch Sorter - Strict … scopes” (YANG/scope enforcement). Treat as no-ops allowed for now | | TC_TO_QUEUE_MAP | 🚫 No (blocked)| ❌ No | none (blocked) | Observed failure: β€œFailed to apply patch on scopes …” β†’ treat as no-ops allowed for now | | TC_TO_PRIORITY_GROUP_MAP | 🚫 No (blocked)| ❌ No | none (blocked) | Same class of failure as mapping tables above | Signed-off-by: Venkit Kasiviswanathan * generate_dump: add interface FEC stats (#4093) Add FEC stats to the tarball produced by "show tech". The stats can be found in files named "interface.counters.fec-stats_$idx". Signed-off-by: Fraser Gordon Signed-off-by: Venkit Kasiviswanathan * [sfputil] Fix issue: should not do low power mode or reset for non-present ports (#4206) - What I did Ignore get_lpmode, set_lpmode, reset for ports that with no module present - How I did it Check module presence before calling get_lpmode, set_lpmode, reset - How to verify it New unit test - PASSED Manual test - PASSED Signed-off-by: Junchao-Mellanox Signed-off-by: Venkit Kasiviswanathan * Use Singleton PlatformDataProvider to reduce module import time (#4183) - What I did For fwutil show command which displays the usage/help message reduce the time taken by lazily importing PlatformDataProvider. This reduced the average time taken by ~50%. - How I did it Use a singleton PlatformDataProvider in fwutil/main.py - How to verify it Before the change Running 'fwutil show' 10 times (gap 5s)... Run 1: 972 ms Run 2: 1058 ms Run 3: 948 ms Run 4: 1213 ms Run 5: 1507 ms Run 6: 1235 ms Run 7: 1553 ms Run 8: 1037 ms Run 9: 1000 ms Run 10: 1037 ms ---- fwutil show stats ---- Avg: 1156 ms Min: 948 ms Max: 1553 ms After the change Running 'fwutil show' 10 times (gap 5s)... Run 1: 496 ms Run 2: 482 ms Run 3: 466 ms Run 4: 445 ms Run 5: 482 ms Run 6: 463 ms Run 7: 780 ms Run 8: 662 ms Run 9: 653 ms Run 10: 659 ms ---- fwutil show stats ---- Avg: 558 ms Min: 445 ms Max: 780 ms Signed-off-by: Hemanth Kumar Tirupati Signed-off-by: Venkit Kasiviswanathan * [Fast-linkup] Added CLIs for config/show (#4182) HLD: fast-link-up-hld.md What I did Implemented CLI for Fast-linkup feature including: config feature parameters enable/disable the feature per-port show feature parameters show interfaces feature status How I did it By adding the new command support to config and show CLI How to verify it Run Fast-linkup CLIs Which release branch to backport (provide reason below if selected) 202511 New command output (if the output of a command-line utility has changed) admin@sonic:/home/admin# show switch-fast-linkup global +---------------+---------+ | Field | Value | +===============+=========+ | ber_threshold | 10 | +---------------+---------+ | guard_time | 15 | +---------------+---------+ | polling_time | 60 | +---------------+---------+ admin@sonic:/home/admin# show interfaces fast-linkup status +-------------+---------------+ | Interface | fast_linkup | +=============+===============+ | Ethernet0 | true | | Ethernet4 | true | | Ethernet8 | true | | Ethernet12 | false | | Ethernet16 | false | | Ethernet20 | false | | Ethernet24 | false | | Ethernet28 | false | | Ethernet32 | false | | Ethernet36 | false | | Ethernet40 | false | | Ethernet44 | false | | Ethernet48 | false | | Ethernet52 | false | | Ethernet56 | false | | Ethernet60 | false | | Ethernet64 | false | | Ethernet68 | false | | Ethernet72 | false | | Ethernet76 | false | | Ethernet80 | false | | Ethernet84 | false | | Ethernet88 | false | | Ethernet92 | false | | Ethernet96 | false | | Ethernet100 | false | | Ethernet104 | false | | Ethernet108 | false | | Ethernet112 | false | | Ethernet116 | false | | Ethernet120 | false | | Ethernet124 | false | +-------------+---------------+ Signed-off-by: Venkit Kasiviswanathan * Update the error message for sfputil debug loopback command (#4224) * Update the error message for sfputil debug loopback command when diag pages are not supported. Signed-off-by: Ariz Zubair * Update unit tests. Signed-off-by: Ariz Zubair * Fix flake8 error. Signed-off-by: Ariz Zubair * Fix unit test. Signed-off-by: Ariz Zubair --------- Signed-off-by: Ariz Zubair Signed-off-by: Venkit Kasiviswanathan * refactor: enhance show bfd summary command (#4242) Update show bfd summary to aggregate BFD sessions across all ASIC namespaces when no -n is provided. Extend multi-ASIC BFD tests and expected output for the all-ASIC summary. Signed-off-by: Chenyang Wang Signed-off-by: Venkit Kasiviswanathan * Fix JsonMove._get_value to Support Both String and Integer List Indices (#4237) What I did: Issue: #4221 Updated JsonMove._get_value to handle both string and integer indices when traversing lists in config data. Adjusted related unit tests to reflect the new behavior. How I did it: Modified the traversal logic to convert string tokens to integers when accessing lists, allowing both "1" and 1 as valid indices. Removed the test expecting a TypeError for integer indices and added assertions for both string and integer index access. How to verify it: Patched change in lab device, confirmed. admin@STR-SN5640-RDMA-1:~$ cat /usr/local/lib/python3.11/dist-packages/generic_config_updater/patch_sorter.py | grep -C 2 "int(token)" for token in tokens: if isinstance(config, list): token = int(token) config = config[token] admin@STR-SN5640-RDMA-1:~$ cat t_tc_to_queue_map_modify.json [ { "op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8" }, { "op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7" } ] admin@STR-SN5640-RDMA-1:~$ sudo config apply-patch -v t_tc_to_queue_map_modify.json Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}, {"op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Patch Applier: localhost: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: localhost: sorting patch updates. Patch Sorter - Strict: Validating patch is not making changes to tables without YANG models. Patch Sorter - Strict: Validating target config according to YANG models. Patch Sorter - Strict: Sorting patch updates. Patch Applier: The localhost patch was converted into 1 change: Patch Applier: localhost: applying 1 change in order: Patch Applier: * [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}, {"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}] Patch Applier: localhost: verifying patch updates are reflected on ConfigDB. Patch Applier: localhost patch application completed. Patch applied successfully. Also run the updated unit tests and all tests should pass, confirming the fix. Signed-off-by: Xincun Li Signed-off-by: Venkit Kasiviswanathan * Fix j2 files not getting packaged (#4250) What I did Signed-off-by: Venkit Kasiviswanathan * Fix failure with ijson library There was a failure when sonic-mgmt tests were run in a KVM. The failure appears to be due to the environment where it is running. It seems like on this environment ijson is not able to find the C-libraries required to set a default backend. Force a python backend to iterm. Signed-off-by: Venkit Kasiviswanathan * Incorporate feedback from Sai Signed-off-by: Venkit Kasiviswanathan * Pick the python backend for ijson The alternative C backend has an issue that is best described by a comment from saiarcot895 in https://github.com/sonic-net/sonic-utilities/pull/4205 Signed-off-by: Venkit Kasiviswanathan * Add multi-asic support for sonic-clear queue wredcounters and counter poll , --nonzero support for show queue wredcounters (#4152) * Add multi-asic support for sonic-clear queue wredcounters and counterpoll , --nonzero support for show queue wredcounters * Add multi-asic support for sonic-clear queue wredcounters Signed-off-by: saksarav * Fix the flake8 error Signed-off-by: saksarav --------- Signed-off-by: saksarav Signed-off-by: Venkit Kasiviswanathan * [Mellanox] Add restricted sysfs to fw control list (#4240) - What I did Add interrupt sysfs to restricted fw control sysfs list, and took hw_present value only if control == 1. - How I did it Updated generate_dump script - How to verify it run show techsupport on switch Signed-off-by: noaOrMlnx Signed-off-by: Venkit Kasiviswanathan * Clearing /tmp/tmp* is unsafe with parallel builds (#4268) * Clearing /tmp/tmp* is unsafe with parallel builds Many tests for various packages use /tmp/tmp.XXXXXXXX or /tmp/tmpi_XXXXX as the temporary file or directory pattern for mktemp. Since the same slave container is used for multiple simultaneous builds, destroying an in-progress build's temporary file or directory will cause those builds to fail. While this has existed for a year, it appears the introduction of Trixie has reordered the builds a bit so that packages using the temp file patterns impacted are built simultaneously. Signed-off-by: Brad House * subprocess does not need to invoke the shell glob pattern is no longer used so we don't need to spawn a shell to interpret. Signed-off-by: Brad House --------- Signed-off-by: Brad House Co-authored-by: Brad House Signed-off-by: Venkit Kasiviswanathan * Fix dump port state CLI command crash on multi-asic platforms (#4229) * Fix masic dump port state crash The error occurs because the code checks if any database configuration is loaded, but multi-ASIC systems specifically need the global database configuration to be loaded. Fixed it by using isGlobalInit() check for multi-ASIC and isInit() for single-ASIC to ensure the correct DB configuration is loaded before creating connectors. Signed-off-by: setu * Fix masic dump port state crash The error occurs because the code checks if any database configuration is loaded, but multi-ASIC systems specifically need the global database configuration to be loaded. Fixed it by calling load_db_config helper function to ensure the correct DB configuration is loaded before creating connectors. Signed-off-by: setu --------- Signed-off-by: setu Signed-off-by: Venkit Kasiviswanathan * Add .github/copilot-instructions.md for AI-assisted development (#4271) Signed-off-by: Rustiqly Co-authored-by: Rustiqly Signed-off-by: Venkit Kasiviswanathan * Add filesystem sync after plugin installation (#4251) - Why I did it In some scenarios, after install plugin then power cycle, file content might lost. Before power cycle, file size is 205, also can found register function in python file, but after power cycle, this file size is 0, so assume this is caused by page cache didn't write back to disk on time, when power cycle happen. Before power cycle: 2026 Feb 3 10:34:16.156531 sonic-testbed INFO [DIAGNOSTIC] Starting CLI plugins installation for package: cpu-report 2026 Feb 3 10:34:16.157013 sonic-testbed INFO [DIAGNOSTIC] Installing CLI plugin: package=cpu-report, command=show, src=/show.py, dst=/usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py 2026 Feb 3 10:34:16.157177 sonic-testbed INFO [DIAGNOSTIC] Starting extract: image=sha256:1230c222517c88863253c94dba34a788b580604618373fff24ab737a7d519c3f, src=/show.py, dst=/usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py 2026 Feb 3 10:34:16.267834 sonic-testbed INFO [DIAGNOSTIC] Tar buffer size: 2048 bytes, MD5: b0b48780efda61d230dc2e3592cc3ba6 2026 Feb 3 10:34:16.268709 sonic-testbed INFO [DIAGNOSTIC] Tar member: name=show.py, size=205, isfile=True 2026 Feb 3 10:34:16.269652 sonic-testbed INFO [DIAGNOSTIC] File extracted successfully: path=/usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py, size=205, MD5=f2f3ca5258fd0685adf2cc44567934fb, elapsed=0.112s 2026 Feb 3 10:34:16.270313 sonic-testbed INFO [DIAGNOSTIC] Python syntax validation: PASS for /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py 2026 Feb 3 10:34:16.270820 sonic-testbed INFO [DIAGNOSTIC] Plugin file verification after extract: path=/usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py, size=205, MD5=f2f3ca5258fd0685adf2cc44567934fb, mtime=1684332898.0, extract_time=0.113s 2026 Feb 3 10:34:16.271351 sonic-testbed INFO [DIAGNOSTIC] Python syntax check: PASS for /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py 2026 Feb 3 10:34:16.271638 sonic-testbed INFO [DIAGNOSTIC] Found "def register" in plugin file: /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py 2026 Feb 3 10:34:16.271918 sonic-testbed INFO [DIAGNOSTIC] Completed CLI plugins installation for package: cpu-report, elapsed=0.115s After power cycle: admin@sonic-testbed:~$ show version 2>&1 failed to import plugin show.plugins.cpu-report: module 'show.plugins.cpu-report' has no attribute 'register' admin@sonic-testbed:~$ ls -lih /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py 830572 -rw-r--r-- 1 root root 0 May 17 2023 /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py admin@sonic-testbed:~$ sudo md5sum /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py d41d8cd98f00b204e9800998ecf8427e /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py admin@sonic-testbed:~$ sudo stat /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py File: /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py Size: 0 Blocks: 0 IO Block: 4096 regular empty file Device: 0,27 Inode: 830572 Links: 1 Access: (0644/-rw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root) Access: 2026-02-03 10:34:16.266593882 +0200 Modify: 2023-05-17 17:14:58.000000000 +0300 Change: 2026-02-03 10:34:16.262593831 +0200 Birth: 2026-02-03 10:34:16.262593831 +0200 admin@sonic-testbed:~$ cat /usr/local/lib/python3.13/dist-packages/show/plugins/cpu-report.py admin@sonic-testbed:~$ - What I did Fix intermittent plugin corruption after power cycle by adding os.sync() to flush filesystem buffers after all CLI plugins are installed. This prevents incomplete plugin files that cause 'module has no attribute 'register'' errors in show commands after system reboot. - How I did it Added os.sync() system call in PackageManager._install_cli_plugins() method after all CLI plugin files are extracted and installed. This ensures that: All plugin file data is flushed from the OS page cache to disk File metadata and data are both persisted before the method returns Plugin files remain intact even if an abrupt power loss occurs shortly after installation - How to verify it 1. Install cpu-report package: sonic-package-manager install cpu-report==1.0.0 -y 2. Enable feature: config feature state cpu-report enabled 3. Upgrade package: sonic-package-manager install cpu-report==1.0.7 -y 4. Upgrade again: sonic-package-manager install cpu-report==1.0.8 -y Immediately perform power cycle 5. After reboot, run: show version If there is problem, error is: failed to import plugin show.plugins.cpu-report: module 'show.plugins.cpu-report' has no attribute 'register'. Signed-off-by: Jianyue Wu Signed-off-by: Venkit Kasiviswanathan * [multi-asic][warm_restart] add Multi-ASIC support for warm_restart commands (#4200) - What I did Added Multi-ASIC support for warm_restart commands. - How I did it Updated the warm restart commands to operate per ASIC namespace and handle multi-ASIC execution consistently. - How to verify it Run warm_restart commands on a Multi-ASIC system and confirm per-ASIC namespaces are handled. Verify warm restart flags/status are correct per namespace. Signed-off-by: Stepan Blyschak Signed-off-by: Venkit Kasiviswanathan * [multi-asic][warm-reboot] Support warm-reboot on Multi-ASIC systems (#4199) - What I did Implement warm-reboot script support for Multi-ASIC systems. - How I did it Modified warm-reboot script. - How to verify it 1. Verified on Multi-ASIC KVM with 4 ASICs 2. On boot SAI started in warm boot mode 3. Tested on single-ASIC real HW to ensure flow is as was before --------- Signed-off-by: Stepan Blyschak Signed-off-by: Yair Raviv Signed-off-by: Venkit Kasiviswanathan * [centralize_database] Add --namespace option (#4198) - What I did Added --namespace option to centralize_database script - How I did it Added --namespace option to centralize_database script - How to verify it Run centralize_database script with --namespace option Signed-off-by: Stepan Blyschak Signed-off-by: Venkit Kasiviswanathan * [check_db_integrity] Add NETNS environment (#4197) - What I did Renamed DB dump files to include database name and namespace. - How I did it Adjusted the dump file naming to ".json" to uniquely identify per-ASIC/namespace outputs. - How to verify it Run the DB dump command with and without a namespace. Confirm the output file name matches DBNAME plus NETNS (when provided). Ensure dumps are still created successfully. Signed-off-by: Stepan Blyschak Signed-off-by: Venkit Kasiviswanathan * [warm/fast-reboot] check per-ASIC FW upgrade status (#4196) - What I did Added per-ASIC firmware upgrade status checks during warm/fast reboot. - How I did it Updated the warm/fast reboot flow to query and validate FW upgrade status per ASIC namespace instead of relying on a single/global check. - How to verify it Trigger warm/fast reboot on a Multi-ASIC system with mixed FW upgrade states and confirm the per-ASIC check reflects each namespace. Confirm reboot proceeds only when all ASICs report FW upgrade completion. Run existing warm reboot tests and ensure they pass. Signed-off-by: Stepan Blyschak Signed-off-by: Venkit Kasiviswanathan * [teamd_retry_count] Add support for --namespace parameter (#4195) - What I did Added support for --namespace parameter in both config portchannel retry-count CLI as well as teamd_increase_retry_count.py script to support Multi-ASIC systems. - How I did it Pass namespace to DB interfaces and CLI commands, in teamd_increase_retry_count.py script - switch to network namespace to perform network operations within that namespace. - How to verify it Manual test. Signed-off-by: Stepan Blyschak Signed-off-by: Venkit Kasiviswanathan * [lag_keepalive] add `--namespace` option (#4194) - What I did Added --namespace option to lag_keepalive.py. - How I did it Added --namespace option to lag_keepalive.py. - How to verify it Run lag_keepalive.py with --namepsace option. Signed-off-by: Stepan Blyschak Signed-off-by: Venkit Kasiviswanathan * [fast-reboot] Remove teamsyncd timer override by fast-boot (#4233) Timer override to 1 sec was used to speed up kernel IP configuration on PortChannel as a W/A. This PR reopened this PR - #3996 - What I did Remove teamsyncd 1 sec timer override. It was used to speed up kernel IP configuration on PortChannel as a W/A. Original issue is solved by sonic-net/sonic-swss#4170 - How I did it Remove teamsyncd 1 sec timer override. - How to verify it Ran fast-boot and warm-boot tests. Signed-off-by: Stepan Blyschak Signed-off-by: Venkit Kasiviswanathan * Prevent early exit of reboot status (#4282) Signed-off-by: gpunathilell Signed-off-by: Venkit Kasiviswanathan * [multi-asic] fix utilities_common Db helper (#4273) - What I did This is to fix the utilities_common.Db() helper class. Using it now in the multi-asic environment leads to an error: RuntimeError: :- validateNamespace: Initialize global DB config using API SonicDBConfig::initializeGlobalConfig This impacts the counterpoll switch CLI command. - How I did it Added a proper DB config initialization - How to verify it Manual test for the Db() helper Running counterpoll switch disable in multi-asic environment Signed-off-by: Yakiv Huryk Signed-off-by: Venkit Kasiviswanathan * Convey the IJSON Backend using an env variable Signed-off-by: Venkit Kasiviswanathan * Revert "Convey the IJSON Backend using an env variable" This reverts commit 916442c9df260653783f14dcebfa65aa7f1ed393. Signed-off-by: Venkit Kasiviswanathan * Convey the IJSON Backend using an env variable Signed-off-by: Venkit Kasiviswanathan * Fix flake8 error Signed-off-by: Venkit Kasiviswanathan * Fix flake8 errors Signed-off-by: Venkit Kasiviswanathan * Fix merge conflict error Signed-off-by: Venkit Kasiviswanathan --------- Signed-off-by: Venkit Kasiviswanathan Signed-off-by: gpunathilell Signed-off-by: arista-hpandya Signed-off-by: manish Signed-off-by: Oleksandr Ivantsiv Signed-off-by: dhanasekar-arista Signed-off-by: Ariz Zubair Signed-off-by: Stephen Sun Signed-off-by: Yuanzhe Liu Signed-off-by: Fraser Gordon Signed-off-by: Junchao-Mellanox Signed-off-by: Hemanth Kumar Tirupati Signed-off-by: Chenyang Wang Signed-off-by: Xincun Li Signed-off-by: saksarav Signed-off-by: noaOrMlnx Signed-off-by: Brad House Signed-off-by: setu Signed-off-by: Rustiqly Signed-off-by: Jianyue Wu Signed-off-by: Stepan Blyschak Signed-off-by: Yair Raviv Signed-off-by: Yakiv Huryk Co-authored-by: Gagan Punathil Ellath Co-authored-by: HP Co-authored-by: manish1-arista Co-authored-by: Oleksandr Ivantsiv Co-authored-by: Dhanasekar Rathinavel Co-authored-by: Ariz Zubair <5427064+az-pz@users.noreply.github.com> Co-authored-by: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Co-authored-by: Yuanzhe <150663541+yuazhe@users.noreply.github.com> Co-authored-by: Saikrishna Arcot Co-authored-by: Dev Ojha <47282568+developfast@users.noreply.github.com> Co-authored-by: Fraser Gordon Co-authored-by: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Co-authored-by: Hemanth Kumar Tirupati Co-authored-by: Yair Raviv <73100906+YairRaviv@users.noreply.github.com> Co-authored-by: Chenyang Wang <49756587+cyw233@users.noreply.github.com> Co-authored-by: Xincun Li <147451452+xincunli-sonic@users.noreply.github.com> Co-authored-by: saksarav-nokia Co-authored-by: Noa Or <58519608+noaOrMlnx@users.noreply.github.com> Co-authored-by: Brad House - NextHop Co-authored-by: Brad House Co-authored-by: Setu Patel <171176331+arista-setu@users.noreply.github.com> Co-authored-by: rustiqly <245760149+rustiqly@users.noreply.github.com> Co-authored-by: Rustiqly Co-authored-by: Jianyue Wu Co-authored-by: Yakiv Huryk <62013282+Yakiv-Huryk@users.noreply.github.com> Signed-off-by: Xincun Li * Fix spelling typos in config/nat.py (#4258) Fix repeated misspelling of 'configuration' (configutation) throughout NAT configuration commands, plus 'suported' -> 'supported' and 'Enbale' -> 'Enable'. Signed-off-by: Rustiqly Co-authored-by: Rustiqly Signed-off-by: Xincun Li * Fix spelling typos in config/config_mgmt.py (#4260) Fix misspellings: managment, Seperator, dependecies, delets, sucessful, relavant, compitible. Signed-off-by: Rustiqly Co-authored-by: Rustiqly Signed-off-by: Xincun Li * Fix spelling typos in show/ and clear/ modules (#4263) Fix misspellings in show and clear commands: - dislay -> display (bgp_common.py) - lastest -> latest (kdump.py) - continous -> continuous (show/main.py, clear/main.py) - deafult -> default (interfaces/__init__.py) - Erorrs -> Errors (interfaces/__init__.py) - fomatted -> formatted (5 plugin files) - cummulative -> cumulative (auto_techsupport.py) Signed-off-by: Rustiqly Co-authored-by: Rustiqly Signed-off-by: Xincun Li * Fix spelling typos in scripts/ (#4262) Signed-off-by: Rustiqly Co-authored-by: Rustiqly Signed-off-by: Xincun Li * Fix spelling typos in config/main.py (#4261) Fix the following spelling errors in comments and string literals: - relavent -> relevant - retreive -> retrieve - cant -> can't - environmnet -> environment - funtion -> function - dependecy -> dependency - overriden -> overridden (2 occurrences) - exmaple -> example - sepcified -> specified (5 occurrences) - Interation -> Iteration - Remvoe -> Remove - transciever -> transceiver (2 occurrences) - Disble -> Disable - doesnt exists -> doesn't exist (2 occurrences) - doesnt exist -> doesn't exist - doesnot exist -> does not exist (2 occurrences) - cant delete -> can't delete (2 occurrences) Signed-off-by: Rustiqly Co-authored-by: Rustiqly Signed-off-by: Xincun Li * Fix spelling typos in muxcable modules (#4259) Fix 'retreive' -> 'retrieve', 'cant' -> 'can\'t', 'standy' -> 'standby', and 'detemine' -> 'determine' in config/muxcable.py and show/muxcable.py. Signed-off-by: Rustiqly Co-authored-by: Rustiqly Signed-off-by: Xincun Li * Fix unit test assertions broken by spelling typo PRs (#4321) What is the motivation for this PR Fix unit test assertions broken by recent spelling correction PRs, and revert the 'Neighbhor' β†’ 'Neighbor' header change which is intentionally preserved for backward compatibility. How did you do it Updated test expected strings to match corrected source messages and restored the 'Neighbhor' header in bgp_util.py. How did you verify/test it Not provided in PR description. Signed-off-by: Rustiqly Signed-off-by: Xincun Li * Add fsync to config save to persist config across power cycle (#4313) What I did Fixed config_db.json not persisting across power cycle. Config changes (e.g., FEC) were lost after power cycle because data stayed in page cache and was never flushed to disk. How I did it Added flush() and os.fsync() after json.dump() to ensures config is written to disk before returning, so it survives power cycle. How to verify it config interface fec Ethernet0 auto config save -y cat /etc/sonic/config_db.json | grep -i fec Signed-off-by: Xincun Li * [LACP retry-count] Syntax Fix for Trixie (#4274) Signed-off-by: Yair Raviv Signed-off-by: Xincun Li * fix scapy delayed import when we have large routes (#4315) * Fix delayed scapy import in teamd retry count script Signed-off-by: Hemanth Kumar Tirupati * fix scapy delayed import. Signed-off-by: Hemanth Kumar Tirupati --------- Signed-off-by: Hemanth Kumar Tirupati Signed-off-by: Xincun Li * fix: skip PORT_INGRESS/EGRESS_MIRROR_CAPABLE check for ERSPAN mirror sessions (#4323) * fix: skip PORT_INGRESS/EGRESS_MIRROR_CAPABLE check for ERSPAN sessions ERSPAN sessions (direction=None) use source/destination IPs, not ports. The PORT_INGRESS_MIRROR_CAPABLE and PORT_EGRESS_MIRROR_CAPABLE capability flags in STATE_DB only apply to SPAN (port mirror) sessions. Checking these flags for ERSPAN incorrectly blocks session creation on platforms that do not populate these STATE_DB keys (e.g., multi-ASIC T1 KVM). Changes: - Return True immediately when direction=None (ERSPAN) in is_port_mirror_capability_supported(), bypassing the capability check - Treat absent STATE_DB keys (None value) as 'supported' for backward compatibility on platforms that don't populate SWITCH_CAPABILITY table Fixes: https://github.com/sonic-net/sonic-mgmt/issues/21690 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bing Wang * fix: skip PORT_INGRESS/EGRESS_MIRROR_CAPABLE check for ERSPAN sessions ERSPAN sessions use src/dst IPs (GRE tunnel), not ports. The capability flags PORT_INGRESS_MIRROR_CAPABLE and PORT_EGRESS_MIRROR_CAPABLE in STATE_DB SWITCH_CAPABILITY|switch only apply to SPAN (port mirror) sessions. Root cause: platforms that do not populate these STATE_DB keys return None, which != 'true', so is_port_mirror_capability_supported() incorrectly returns False and blocks ERSPAN session creation. Fix: - In validate_mirror_session_config(): skip the capability check entirely for ERSPAN sessions (dst_port=None is always passed by the ERSPAN code path) - In is_port_mirror_capability_supported(): treat absent STATE_DB keys (None) as 'supported' for backward compatibility; direction=None now correctly checks both ingress and egress capabilities for SPAN sessions Fixes: https://github.com/sonic-net/sonic-mgmt/issues/21690 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bing Wang --------- Signed-off-by: Bing Wang Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Xincun Li * Fix 'show version' KeyError when sonic_version.yml has missing fields (#4324) 'show version' crashes with KeyError when debian_version or kernel_version are missing from sonic_version.yml. This happens in docker-sonic-vs containers where the version file is generated without these fields (they are only set during full image builds). Use .get() with sensible runtime fallbacks: - debian_version: 'N/A' (not available in container context) - kernel_version: os.uname().release (actual host kernel at runtime) - build_version: 'N/A' - sonic_os_version: 'N/A' Fixes sonic-net/sonic-buildimage#25765 Signed-off-by: securely1g Signed-off-by: Xincun Li * Modified dualtor_neighbor_check to use mux neighbor_mode (#4227) What I did Adjusted the dualtor_neighbor_check.py based on mux neighbor_mode described in HLD : sonic-net/SONiC#2176 Output of dualtor_neighbor_check will now depend on neighbor_mode set in STATE_DB|MUX_CABLE_TABLE How I did it How to verify it Previous command output (if the output of a command-line utility has changed) NEIGHBOR MAC PORT MUX_STATE IN_MUX_TOGGLE NEIGHBOR_IN_ASIC TUNNEL_IN_ASIC HWSTATUS ------------ ----------------- ----------- ----------- --------------- ------------------ ---------------- ---------- 192.168.0.3 16:8d:06:da:8d:0d Ethernet8 active no yes no consistent 192.168.0.5 42:85:ce:ff:2b:7a Ethernet16 active no yes no consistent New command output (if the output of a command-line utility has changed) ================================================================================ Neighbors in PREFIX-ROUTE mode: ================================================================================ NEIGHBOR MAC PORT MUX_STATE IN_MUX_TOGGLE NEIGHBOR_IN_ASIC PREFIX_ROUTE NEXTHOP_TYPE HWSTATUS ------------ ----------------- ----------- ----------- --------------- ------------------ -------------- -------------- ---------- 192.168.0.7 5e:9d:89:07:66:83 Ethernet24 active no yes yes NEIGHBOR consistent 192.168.0.9 e2:2a:a8:65:1e:50 Ethernet32 active no yes yes NEIGHBOR consistent ================================================================================ Neighbors in HOST-ROUTE mode: ================================================================================ NEIGHBOR MAC PORT MUX_STATE IN_MUX_TOGGLE NEIGHBOR_IN_ASIC TUNNEL_IN_ASIC HWSTATUS ----------- ----------------- ---------- ----------- --------------- ------------------ ---------------- ---------- 192.168.0.3 16:8d:06:da:8d:0d Ethernet8 active no yes no consistent 192.168.0.5 42:85:ce:ff:2b:7a Ethernet16 active no yes no consistent Signed-off-by: Xincun Li * [tests/gcu]: Improve code coverage for generic_config_updater/main.py and config/main.py Add a new test module tests/generic_config_updater/main_test.py with comprehensive unit tests covering: - validate_patch_format: all valid ops, non-list/non-dict/missing-field/ invalid-op branches - get_all_running_config: success return, nonzero returncode exception - filter_duplicate_patch_operations: no-leaf-list fast path, duplicate removal, non-duplicate unchanged, dict config input - append_emptytables_if_required: no-insert, single/multiple missing tables, op-without-path skip, asic-scoped paths - validate_patch: ImportError skip, True/False validation, unexpected exception, multi-asic all-asics loop and per-asic failure - apply_patch_for_scope: success, exception -> failure, HOST_NAMESPACE scope mapping - apply_patch_from_file: invalid format, no-preprocess success, preprocess helper invocations, preprocess validation failure, parallel threadpool dispatch, scope failure aggregation, empty patch single/multi-asic - print_error / print_success output targets - multiasic_save_to_singlefile: host + asic configs - Sub-command functions: create_checkpoint, delete_checkpoint, list_checkpoints, apply_patch, replace_config, save_config, rollback_config (success, verbose output, failure -> sys.exit) - build_parser: all 7 sub-commands with default and non-default flags - main(): no-command help, all commands dispatched, missing-file exit Extend tests/config_test.py (TestGenericUpdateCommands) to cover the previously uncovered lines in config/main.py: - print_dry_run_message: dry_run=True banner / dry_run=False no output - run_gcu_standalone: basic cmd construction, non-default format flag, all optional flags (--dry-run, --parallel, --ignore-non-yang-tables, --ignore-path, --verbose), return value pass-through - apply-patch GCU standalone redirect: success (returncode 0) and failure (returncode != 0 -> ctx.fail) branches Signed-off-by: Xincun Li * [tests/gcu]: Fix flake8 lint errors in main_test.py - Remove unused 'jsonpatch' import (F401) - Add '# noqa: E402' to imports that follow sys.path.insert calls (E402) Signed-off-by: Xincun Li * [GCU/config]: Port path-trace support from PR #4317 Adopt the --path-trace option added in upstream PR #4317. config/main.py: - Add import jsonpatch and validate_patch from generic_config_updater.main - Update run_gcu_standalone() to forward --path-trace to the standalone binary - In apply_patch(): when --path-trace is set, open the trace file and call GenericUpdater.apply_patch() directly with trace_io parameter, closing the file in a finally block; for the non-trace path delegate unchanged to _gcu_apply_patch_from_file() generic_config_updater/main.py: - Add trace_io=None parameter to apply_patch_for_scope() and apply_patch_from_file(), propagating it through parallel and serial dispatch - Document the new parameter in the docstring - Remove the incorrect open() call inside apply_patch_from_file() that would have leaked file handles and conflated file-path strings with IO objects tests/config_test.py: - Add test_apply_patch__path_trace_option__trace_file_opened_and_passed - Update existing assertion helpers to include trace_io=None for the no-trace path Signed-off-by: Xincun Li * [tests/config]: Update run_gcu_standalone test calls to pass path_trace=None After adding the path_trace parameter to run_gcu_standalone(), existing test call sites need to be updated to supply the new argument so the function signature matches and --path-trace absence is explicitly asserted. Signed-off-by: Xincun Li * [config/GCU]: Cleanup imports and fix multi-asic mock in apply_patch tests config/main.py: remove unused validate_patch import from generic_config_updater.main (apply_patch no longer calls it directly after the path_trace refactor). tests/generic_config_updater/main_test.py: add mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False) around the apply_patch_from_file test so it does not attempt real multi-ASIC detection when running in a unit-test environment. Signed-off-by: Xincun Li * [tests/config]: Fix validate_patch mock target after import cleanup After removing the validate_patch re-import from config.main, the @patch decorator in the path_trace tests must target the canonical location generic_config_updater.main.validate_patch instead of config.main.validate_patch. Signed-off-by: Xincun Li * [config/apply-patch]: Guard standalone GCU redirect against infinite loop The redirect to gcu-standalone used os.path.exists(GCU_STANDALONE_BIN) as the sole condition. If the standalone binary itself ever re-enters this code path (e.g. it also delegates to GCU_STANDALONE_BIN, or is replaced by a wrapper that calls 'config apply-patch'), the process would recurse or loop indefinitely. Fix: set GCU_STANDALONE_ACTIVE=1 in the subprocess environment inside run_gcu_standalone(), and add 'not os.environ.get(GCU_STANDALONE_ACTIVE)' to the redirect guard. This ensures at most one level of delegation regardless of what the standalone binary does. Signed-off-by: Xincun Li * [config/apply-patch]: Always delegate to _gcu_apply_patch_from_file, pass trace_io directly Signed-off-by: Xincun Li * [config,generic_config_updater,utilities_common]: Fix stale comment and add sync warnings for DEFAULT_SUPPORTED_FECS_LIST Signed-off-by: Xincun Li * [tests/config]: Fix GenericUpdater mock target for path_trace tests after GCU refactor Signed-off-by: Xincun Li * [gcu]: Add explanatory comment at top of setup.py Clarifies why gcu/ exists as a separate build context for the sonic-gcu wheel, how gcu-standalone relates to sonic-utilities, and why setup.py and pytest.ini must stay in gcu/ rather than being moved into generic_config_updater/. Signed-off-by: Xincun Li * [GCU/standalone]: Add --path-trace support to gcu-standalone apply-patch The apply-patch subparser in build_parser() was missing the -t/--path-trace argument, so any invocation that included --path-trace would be rejected with 'unrecognised arguments' by the standalone binary. - Add '-t'/'--path-trace' argument to the apply-patch subparser - Wire it through apply_patch(args) by opening the file and passing the handle as trace_io= to apply_patch_from_file(), closing it in a finally block to avoid resource leaks Signed-off-by: Xincun Li * [GCU]: Fix AttributeError for path_trace in apply_patch and test helper apply_patch() accessed args.path_trace directly, causing AttributeError when called from test Namespace objects or older callers that predate the attribute. Use getattr(args, 'path_trace', None) for backward compat. TestApplyPatchSubcommand._make_args() also lacked path_trace; add it with default=None so tests pass now and new path_trace tests can use it. Signed-off-by: Xincun Li * [GCU]: Minor fixes across config and generic_config_updater config/main.py: fix stale comment referencing .gcu module; the function is imported from generic_config_updater.main. generic_config_updater/main.py: gcu-standalone apply_patch() now uses preprocess=True (default) so running-config fetch, empty-table insertion, duplicate filtering and YANG validation are all performed, matching the behaviour of the host config apply-patch path. generic_config_updater/generic_updater.py: add missing 'import jsonpatch' and import JsonChange from gu_common. Signed-off-by: Xincun Li * [generic_config_updater]: Move GCU wheel build context from gcu/ to generic_config_updater/ - Move setup.py, pytest.ini, .coveragerc from gcu/ into generic_config_updater/ - Remove copytree logic from setup.py (no longer needed, setup.py is co-located) - Remove sonic_dependencies pre-flight check and install_requires (wheel is installed with --no-deps into a --system-site-packages venv) - Fix testpaths in pytest.ini (same relative depth, ../tests/generic_config_updater) Signed-off-by: Xincun Li * [azure-pipelines/generic_config_updater]: Update GCU build context path from gcu/ to generic_config_updater/ - azure-pipelines.yml: update pushd and publish paths to generic_config_updater/ - main.py: remove stale references to scripts/gcu.py in comments Signed-off-by: Xincun Li * [config]: Remove GCU standalone redirect from apply-patch The shim approach (container-side PATH override) is the correct mechanism for redirecting apply-patch to the GCU container binary on all SONiC versions, including older ones that lack this host-side redirect code. Remove the host-side redirect as it is out of scope for this PR and redundant given the shim. - Remove GCU_STANDALONE_BIN constant - Remove run_gcu_standalone() helper function - Remove standalone redirect block from apply_patch() - Remove all related unit tests Signed-off-by: Xincun Li * [generic_config_updater]: Fix package_dir for in-tree setup.py setup.py is co-located with source files inside generic_config_updater/. Add package_dir={'generic_config_updater': '.'} so setuptools resolves the package source from the current directory instead of looking for a non-existent subdirectory named generic_config_updater. Signed-off-by: Xincun Li --------- Signed-off-by: Xincun Li Signed-off-by: Gnanapriya Sethuramarajan Signed-off-by: Rustiqly Signed-off-by: Venkit Kasiviswanathan Signed-off-by: gpunathilell Signed-off-by: arista-hpandya Signed-off-by: manish Signed-off-by: Oleksandr Ivantsiv Signed-off-by: dhanasekar-arista Signed-off-by: Ariz Zubair Signed-off-by: Stephen Sun Signed-off-by: Yuanzhe Liu Signed-off-by: Fraser Gordon Signed-off-by: Junchao-Mellanox Signed-off-by: Hemanth Kumar Tirupati Signed-off-by: Chenyang Wang Signed-off-by: saksarav Signed-off-by: noaOrMlnx Signed-off-by: Brad House Signed-off-by: setu Signed-off-by: Jianyue Wu Signed-off-by: Stepan Blyschak Signed-off-by: Yair Raviv Signed-off-by: Yakiv Huryk Signed-off-by: Bing Wang Signed-off-by: securely1g Co-authored-by: Gnanapriya [Marvell] Co-authored-by: rustiqly <245760149+rustiqly@users.noreply.github.com> Co-authored-by: Rustiqly Co-authored-by: Lihua Yuan Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: venkit-nexthop Co-authored-by: Gagan Punathil Ellath Co-authored-by: HP Co-authored-by: manish1-arista Co-authored-by: Oleksandr Ivantsiv Co-authored-by: Dhanasekar Rathinavel Co-authored-by: Ariz Zubair <5427064+az-pz@users.noreply.github.com> Co-authored-by: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Co-authored-by: Yuanzhe <150663541+yuazhe@users.noreply.github.com> Co-authored-by: Saikrishna Arcot Co-authored-by: Dev Ojha <47282568+developfast@users.noreply.github.com> Co-authored-by: Fraser Gordon Co-authored-by: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Co-authored-by: Hemanth Kumar Tirupati Co-authored-by: Yair Raviv <73100906+YairRaviv@users.noreply.github.com> Co-authored-by: Chenyang Wang <49756587+cyw233@users.noreply.github.com> Co-authored-by: saksarav-nokia Co-authored-by: Noa Or <58519608+noaOrMlnx@users.noreply.github.com> Co-authored-by: Brad House - NextHop Co-authored-by: Brad House Co-authored-by: Setu Patel <171176331+arista-setu@users.noreply.github.com> Co-authored-by: Jianyue Wu Co-authored-by: Yakiv Huryk <62013282+Yakiv-Huryk@users.noreply.github.com> Co-authored-by: bingwang-ms <66248323+bingwang-ms@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: securely1g Co-authored-by: manamand2020 <68087238+manamand2020@users.noreply.github.com> (cherry picked from commit 02b9e4856789337f9f69efeeb8ba7d9677d22a7c) --- azure-pipelines.yml | 136 +++ config/main.py | 158 +-- generic_config_updater/.coveragerc | 6 + .../field_operation_validators.py | 7 +- generic_config_updater/main.py | 822 +++++++++++++++ generic_config_updater/pytest.ini | 5 + generic_config_updater/setup.py | 54 + tests/config_test.py | 668 +++++++++++- tests/generic_config_updater/main_test.py | 965 ++++++++++++++++++ utilities_common/constants.py | 4 + 10 files changed, 2655 insertions(+), 170 deletions(-) create mode 100644 generic_config_updater/.coveragerc create mode 100644 generic_config_updater/main.py create mode 100644 generic_config_updater/pytest.ini create mode 100644 generic_config_updater/setup.py create mode 100644 tests/generic_config_updater/main_test.py diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 885f6cb73..4aacfb262 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -175,3 +175,139 @@ stages: - publish: '$(System.DefaultWorkingDirectory)/dist/' artifact: wheels displayName: "Publish Python wheels" + + - script: | + set -e + pushd generic_config_updater + python3 -m build -n + displayName: 'Build Python 3 wheel for GCU' + + - publish: '$(System.DefaultWorkingDirectory)/generic_config_updater/dist/' + artifact: gcu_wheels + displayName: "Publish Python wheels for GCU" + +- stage: BuildData + + jobs: + - job: + displayName: "Build sonic-utilities-data" + pool: + vmImage: ubuntu-24.04 + + container: + image: sonicdev-microsoft.azurecr.io:443/sonic-slave-bookworm:$(BUILD_BRANCH) + + steps: + - script: | + set -ex + sudo apt-get update + sudo apt-get install -y python3-pip + sudo apt-get install -y python3-protobuf + displayName: "Install dependencies" + + - script: | + sourceBranch=$(Build.SourceBranchName) + if [[ "$(Build.Reason)" == "PullRequest" ]];then + sourceBranch=$(System.PullRequest.TargetBranch) + fi + echo "Download artifact branch: $sourceBranch" + echo "##vso[task.setvariable variable=sourceBranch]$sourceBranch" + displayName: "Get correct artifact downloading branch" + + - task: DownloadPipelineArtifact@2 + inputs: + source: specific + project: build + pipeline: 142 + artifact: sonic-buildimage.vs + runVersion: 'latestFromBranch' + runBranch: 'refs/heads/$(sourceBranch)' + patterns: | + **/*.deb + **/*.whl + displayName: "Download artifacts from latest sonic-buildimage build" + + - script: | + set -xe + sudo apt-get -y purge libnl-3-dev libnl-route-3-dev || true + sudo dpkg -i libnl-3-200_*.deb + sudo dpkg -i libnl-genl-3-200_*.deb + sudo dpkg -i libnl-route-3-200_*.deb + sudo dpkg -i libnl-nf-3-200_*.deb + sudo dpkg -i libyang_1.0.73_amd64.deb + sudo dpkg -i libyang-cpp_1.0.73_amd64.deb + sudo dpkg -i python3-yang_1.0.73_amd64.deb + workingDirectory: $(Pipeline.Workspace)/target/debs/bookworm/ + displayName: 'Install Debian dependencies' + + - task: DownloadPipelineArtifact@2 + inputs: + source: specific + project: build + pipeline: 9 + artifact: sonic-swss-common-bookworm + runVersion: 'latestFromBranch' + runBranch: 'refs/heads/$(sourceBranch)' + displayName: "Download sonic swss common deb packages" + + - script: | + set -xe + sudo dpkg -i libswsscommon_1.0.0_amd64.deb + sudo dpkg -i python3-swsscommon_1.0.0_amd64.deb + workingDirectory: $(Pipeline.Workspace)/ + displayName: 'Install swss-common dependencies' + + - task: DownloadPipelineArtifact@2 + inputs: + source: specific + project: build + pipeline: sonic-net.sonic-dash-api + artifact: sonic-dash-api + runVersion: 'latestFromBranch' + runBranch: 'refs/heads/$(BUILD_BRANCH)' + path: $(Build.ArtifactStagingDirectory)/download + patterns: | + libdashapi*.deb + displayName: "Download dash api" + + - script: | + set -xe + sudo apt-get update + sudo dpkg -i $(Build.ArtifactStagingDirectory)/download/libdashapi_*.deb + workingDirectory: $(Pipeline.Workspace)/ + displayName: 'Install libdashapi libraries' + + - script: | + set -xe + sudo pip3 install swsssdk-2.0.1-py3-none-any.whl + sudo pip3 install sonic_py_common-1.0-py3-none-any.whl + sudo pip3 install sonic_yang_mgmt-1.0-py3-none-any.whl + sudo pip3 install sonic_yang_models-1.0-py3-none-any.whl + sudo pip3 install sonic_config_engine-1.0-py3-none-any.whl + sudo pip3 install sonic_platform_common-1.0-py3-none-any.whl + workingDirectory: $(Pipeline.Workspace)/target/python-wheels/bookworm/ + displayName: 'Install Python dependencies' + + - task: DownloadPipelineArtifact@2 + inputs: + artifact: wheels + path: $(Build.ArtifactStagingDirectory)/download + displayName: "Download pre-stage built sonic-utilities" + + - script: | + sudo pip3 install sonic_utilities-1.2-py3-none-any.whl + workingDirectory: $(Build.ArtifactStagingDirectory)/download + displayName: 'Install sonic-utilities wheel' + + - script: | + set -xe + cd sonic-utilities-data + dpkg-buildpackage -b + cd .. + mkdir -p dist + mv sonic-utilities-data_*.deb dist/ + displayName: 'Build sonic-utilities-data package' + + - publish: '$(System.DefaultWorkingDirectory)/dist/' + artifact: sonic_utilities_data + displayName: "Publish sonic-utilities-data package" diff --git a/config/main.py b/config/main.py index 6420e4ae4..ab705b8d2 100644 --- a/config/main.py +++ b/config/main.py @@ -1,12 +1,9 @@ #!/usr/sbin/env python -import threading import click -import concurrent.futures import datetime import ipaddress import json -import jsonpatch import netaddr import netifaces import os @@ -22,8 +19,11 @@ from jsonpatch import JsonPatchConflict from jsonpointer import JsonPointerException from collections import OrderedDict -from generic_config_updater.generic_updater import GenericUpdater, ConfigFormat, extract_scope -from generic_config_updater.gu_common import HOST_NAMESPACE, GenericConfigUpdaterError +from generic_config_updater.generic_updater import GenericUpdater, ConfigFormat +from generic_config_updater.gu_common import HOST_NAMESPACE +from generic_config_updater.main import ( + apply_patch_from_file as _gcu_apply_patch_from_file +) from minigraph import parse_device_desc_xml, minigraph_encoder from natsort import natsorted from portconfig import get_child_ports @@ -31,7 +31,6 @@ from sonic_py_common import device_info, multi_asic 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 utilities_common import util_base from swsscommon import swsscommon from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector, ConfigDBPipeConnector, \ @@ -141,6 +140,10 @@ # Helper functions # + +# get_all_running_config is imported from generic_config_updater.main + + # Sort nested dict def sort_dict(data): """ Sort of 1st level and 2nd level dict of data naturally by its key @@ -1236,64 +1239,6 @@ def multiasic_save_to_singlefile(db, filename): json.dump(all_current_config, file, indent=4) -def apply_patch_wrapper(args): - return apply_patch_for_scope(*args) - - -# 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): - scope, changes = scope_changes - # Replace localhost to DEFAULT_NAMESPACE which is db definition of Host - if scope.lower() == HOST_NAMESPACE or scope == "": - scope = multi_asic.DEFAULT_NAMESPACE - - scope_for_log = scope if scope else HOST_NAMESPACE - thread_id = threading.get_ident() - log.log_notice(f"apply_patch_for_scope started for {scope_for_log} by {changes} in thread:{thread_id}") - - try: - # Call apply_patch with the ASIC-specific changes and predefined parameters - GenericUpdater(scope=scope).apply_patch(jsonpatch.JsonPatch(changes), - config_format, - verbose, - dry_run, - ignore_non_yang_tables, - ignore_path) - 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: - results[scope_for_log] = {"success": False, "message": str(e)} - log.log_error(f"'apply-patch' executed failed for {scope_for_log} by {changes} due to {str(e)}") - - -def validate_patch(patch): - try: - command = ["show", "runningconfiguration", "all"] - proc = subprocess.Popen(command, text=True, stdout=subprocess.PIPE) - all_running_config, returncode = proc.communicate() - if returncode: - log.log_notice(f"Fetch all runningconfiguration failed as output:{all_running_config}") - return False - - # Structure validation and simulate apply patch. - all_target_config = patch.apply(json.loads(all_running_config)) - - # Verify target config by YANG models - target_config = all_target_config.pop(HOST_NAMESPACE) if multi_asic.is_multi_asic() else all_target_config - target_config.pop("bgpraw", None) - if not SonicYangCfgDbGenerator().validate_config_db_json(target_config): - return False - - if multi_asic.is_multi_asic(): - for asic in multi_asic.get_namespace_list(): - target_config = all_target_config.pop(asic) - target_config.pop("bgpraw", None) - if not SonicYangCfgDbGenerator().validate_config_db_json(target_config): - return False - - return True - except Exception as e: - raise GenericConfigUpdaterError(f"Validate json patch: {patch} failed due to:{e}") def multiasic_validate_single_file(filename): @@ -1603,6 +1548,7 @@ def print_dry_run_message(dry_run): if dry_run: click.secho("** DRY RUN EXECUTION **", fg="yellow", underline=True) + @config.command('apply-patch') @click.argument('patch-file-path', type=str, required=True) @click.option('-f', '--format', type=click.Choice([e.name for e in ConfigFormat]), @@ -1622,78 +1568,32 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang format or SonicYang format. : Path to the patch file on the file-system.""" - try: - print_dry_run_message(dry_run) - with open(patch_file_path, 'r') as fh: - text = fh.read() - patch_as_json = json.loads(text) - patch = jsonpatch.JsonPatch(patch_as_json) + print_dry_run_message(dry_run) - if not validate_patch(patch): - raise GenericConfigUpdaterError(f"Failed validating patch:{patch}") - - results = {} - config_format = ConfigFormat[format.upper()] - # Initialize a dictionary to hold changes categorized by scope - changes_by_scope = {} - - # Iterate over each change in the JSON Patch - for change in patch: - scope, modified_path = extract_scope(change["path"]) - - # Modify the 'path' in the change to remove the scope - change["path"] = modified_path - - # Check if the scope is already in our dictionary, if not, initialize it - if scope not in changes_by_scope: - changes_by_scope[scope] = [] - - # Add the modified change to the appropriate list based on scope - changes_by_scope[scope].append(change) + trace_io = None + try: + if path_trace: + trace_io = open(path_trace, 'w') + _gcu_apply_patch_from_file( + patch_file_path, + config_format_name=format, + verbose=verbose, + dry_run=dry_run, + parallel=parallel, + ignore_non_yang_tables=ignore_non_yang_tables, + ignore_path=ignore_path, + trace_io=trace_io, + ) - # Empty case to force validate YANG model. - if not changes_by_scope: - asic_list = [multi_asic.DEFAULT_NAMESPACE] - if multi_asic.is_multi_asic(): - asic_list.extend(multi_asic.get_namespace_list()) - for asic in asic_list: - changes_by_scope[asic] = [] - - # Apply changes for each scope - if parallel: - with concurrent.futures.ThreadPoolExecutor() as executor: - # Prepare the argument tuples - arguments = [(scope_changes, results, config_format, - verbose, dry_run, ignore_non_yang_tables, ignore_path) - 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] - - # Wait for all tasks to complete - concurrent.futures.wait(futures) - else: - for scope_changes in changes_by_scope.items(): - apply_patch_for_scope(scope_changes, - results, - config_format, - verbose, dry_run, - ignore_non_yang_tables, - ignore_path) - - # Check if any updates failed - failures = [scope for scope, result in results.items() if not result['success']] - - 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}") - - log.log_notice(f"Patch applied successfully for {patch}.") + log.log_notice("Patch applied successfully.") click.secho("Patch applied successfully.", fg="cyan", underline=True) except Exception as ex: click.secho("Failed to apply patch due to: {}".format(ex), fg="red", underline=True, err=True) ctx.fail(ex) + finally: + if trace_io: + trace_io.close() @config.command() @click.argument('target-file-path', type=str, required=True) diff --git a/generic_config_updater/.coveragerc b/generic_config_updater/.coveragerc new file mode 100644 index 000000000..d947f08ae --- /dev/null +++ b/generic_config_updater/.coveragerc @@ -0,0 +1,6 @@ +[run] +branch = True +source = generic_config_updater +omit = + .eggs/* + tests/* diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index ebc1cd6ad..4f97aede5 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -6,7 +6,12 @@ from sonic_py_common import device_info from .gu_common import GenericConfigUpdaterError from swsscommon import swsscommon -from utilities_common.constants import DEFAULT_SUPPORTED_FECS_LIST + +# Default FEC modes when STATE_DB does not advertise supported_fecs for a port. +# Kept local to avoid pulling utilities_common into the GCU wheel. +# NOTE: A duplicate of this list exists in utilities_common/constants.py. +# If you update this list, update that copy too. +DEFAULT_SUPPORTED_FECS_LIST = ['rs', 'fc', 'none', 'auto'] SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" diff --git a/generic_config_updater/main.py b/generic_config_updater/main.py new file mode 100644 index 000000000..4bf878b76 --- /dev/null +++ b/generic_config_updater/main.py @@ -0,0 +1,822 @@ +""" +GCU (Generic Config Updater) β€” apply-patch orchestration +========================================================= + +This module is the **single source of truth** for all apply-patch logic. +It is consumed by: + +* ``config apply-patch`` (config/main.py β€” thin entry-point / standalone redirect) +* ``gcu-standalone`` (console_scripts entry point installed by the GCU wheel + into the GCU container virtual-env at + /opt/sonic/gcu/current/bin/gcu-standalone) + +No caller should re-implement scope extraction, parallel execution, +pre-processing, or per-scope dispatch β€” they should call the helpers +exposed here instead. +""" + +import copy +import json +import logging +import os +import sys +import argparse +import subprocess +import threading +import concurrent.futures + +import jsonpatch +import jsonpointer + +from generic_config_updater.generic_updater import ( + GenericUpdater, + ConfigFormat, + extract_scope, +) +from generic_config_updater.gu_common import ( + HOST_NAMESPACE, + GenericConfigUpdaterError, +) +from sonic_py_common import multi_asic + +logger = logging.getLogger(__name__) + +# Constants +DEFAULT_CONFIG_DB_FILE = '/etc/sonic/config_db.json' + + +# --------------------------------------------------------------------------- +# Lightweight JSON-Patch format validation (RFC 6902) +# --------------------------------------------------------------------------- + +def validate_patch_format(patch): + """Return *True* if *patch* is a structurally valid JSON Patch list.""" + try: + if not isinstance(patch, list): + return False + for change in patch: + if not isinstance(change, dict): + return False + if 'op' not in change or 'path' not in change: + return False + if change['op'] not in ( + 'add', 'remove', 'replace', 'move', 'copy', 'test', + ): + return False + return True + except Exception: + return False + + +# --------------------------------------------------------------------------- +# Running-config retrieval +# --------------------------------------------------------------------------- + +def get_all_running_config(): + """Fetch all running configuration as a JSON string via ``show``.""" + command = ["show", "runningconfiguration", "all"] + proc = subprocess.Popen(command, text=True, stdout=subprocess.PIPE) + all_running_config, _ = proc.communicate() + if proc.returncode: + raise GenericConfigUpdaterError( + f"Fetch all runningconfiguration failed with rc={proc.returncode}" + ) + return all_running_config + + +# --------------------------------------------------------------------------- +# Patch pre-processing helpers +# --------------------------------------------------------------------------- + +def filter_duplicate_patch_operations(patch_ops, all_running_config): + """Remove leaf-list ``add`` ops that would create duplicate entries.""" + if not any(op.get("path", "").endswith("/-") for op in patch_ops): + return patch_ops + + config = ( + json.loads(all_running_config) + if isinstance(all_running_config, str) + else all_running_config + ) + + patch_copy = jsonpatch.JsonPatch([copy.deepcopy(op) for op in patch_ops]) + all_target_config = patch_copy.apply(config) + + def _find_duplicate_entries(cfg): + duplicates = {} + + def _check(obj, path=""): + if isinstance(obj, dict): + for k, v in obj.items(): + _check(v, f"{path}/{k}" if path else f"/{k}") + elif isinstance(obj, list): + seen, dups = set(), set() + for item in obj: + (dups if item in seen else seen).add(item) + if dups: + duplicates[path] = list(dups) + for idx, item in enumerate(obj): + _check(item, f"{path}[{idx}]") + + _check(cfg) + return duplicates + + dups = _find_duplicate_entries(all_target_config) + if not dups: + return patch_ops + + ops_to_remove = set() + for list_path, dup_values in dups.items(): + for op_idx, op in enumerate(patch_ops): + if ( + op.get("op") == "add" + and op.get("path", "").endswith("/-") + and op.get("path").startswith(list_path) + and op.get("value") in dup_values + ): + ops_to_remove.add(op_idx) + + return [op for idx, op in enumerate(patch_ops) if idx not in ops_to_remove] + + +def append_emptytables_if_required(patch_ops, all_running_config): + """Insert ``add`` ops for missing top-level tables before the first + reference to each table so that subsequent ops don't fail.""" + config = ( + json.loads(all_running_config) + if isinstance(all_running_config, str) + else all_running_config + ) + missing_tables = set() + patch_ops_copy = [copy.deepcopy(op) for op in patch_ops] + + for operation in patch_ops_copy: + if 'path' not in operation: + continue + path_parts = operation['path'].strip('/').split('/') + if not path_parts: + continue + + if path_parts[0].startswith('asic') or path_parts[0] == HOST_NAMESPACE: + if len(path_parts) < 2: + continue + table_path = f"/{path_parts[0]}/{path_parts[1]}" + else: + table_path = f"/{path_parts[0]}" + + try: + jsonpointer.resolve_pointer(config, table_path) + except jsonpointer.JsonPointerException: + missing_tables.add(table_path) + + if not missing_tables: + return patch_ops_copy + + for table in missing_tables: + insert_idx = None + for idx, op in enumerate(patch_ops_copy): + if 'path' in op and op['path'].startswith(table): + insert_idx = idx + break + empty_table_patch = {"op": "add", "path": table, "value": {}} + if insert_idx is not None: + patch_ops_copy.insert(insert_idx, empty_table_patch) + else: + patch_ops_copy.append(empty_table_patch) + + return patch_ops_copy + + +# --------------------------------------------------------------------------- +# Full YANG validation of a patch against running config +# --------------------------------------------------------------------------- + +def validate_patch(patch_ops, all_running_config): + """Simulate applying *patch_ops* to *all_running_config* and validate + the result against YANG models. Returns ``True`` on success. + + Raises ``GenericConfigUpdaterError`` on unexpected failures. + """ + try: + from sonic_yang_cfg_generator import SonicYangCfgDbGenerator + except ImportError: + # In environments without sonic_yang_cfg_generator (e.g. minimal + # standalone venv), skip YANG validation. + logger.warning( + "sonic_yang_cfg_generator not available; skipping YANG validation" + ) + return True + + try: + config = ( + json.loads(all_running_config) + if isinstance(all_running_config, str) + else all_running_config + ) + patch_copy = jsonpatch.JsonPatch( + [copy.deepcopy(op) for op in patch_ops] + ) + all_target_config = patch_copy.apply(config) + + target_config = ( + all_target_config.pop(HOST_NAMESPACE) + if multi_asic.is_multi_asic() + else all_target_config + ) + target_config.pop("bgpraw", None) + if not SonicYangCfgDbGenerator().validate_config_db_json( + target_config + ): + return False + + if multi_asic.is_multi_asic(): + for asic in multi_asic.get_namespace_list(): + target_config = all_target_config.pop(asic) + target_config.pop("bgpraw", None) + if not SonicYangCfgDbGenerator().validate_config_db_json( + target_config + ): + return False + + return True + except Exception as e: + raise GenericConfigUpdaterError( + f"Validate json patch: {patch_ops} failed due to: {e}" + ) + + +# --------------------------------------------------------------------------- +# Per-scope dispatch +# --------------------------------------------------------------------------- + +def apply_patch_for_scope(scope_changes, results, config_format, + verbose, dry_run, + ignore_non_yang_tables, ignore_path, + trace_io=None): + """Apply a patch for a single ASIC scope and record the outcome in + *results* (a shared dict).""" + scope, changes = scope_changes + if scope.lower() == HOST_NAMESPACE or scope == "": + scope = multi_asic.DEFAULT_NAMESPACE + + scope_for_log = scope if scope else HOST_NAMESPACE + thread_id = threading.get_ident() + logger.info( + "apply_patch_for_scope started for %s with %d changes in thread %s", + scope_for_log, len(changes), thread_id, + ) + + try: + GenericUpdater(scope=scope).apply_patch( + jsonpatch.JsonPatch(changes), + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_path, + trace_io=trace_io, + ) + results[scope_for_log] = {"success": True, "message": "Success"} + logger.info("apply-patch succeeded for %s", scope_for_log) + except Exception as e: + results[scope_for_log] = {"success": False, "message": str(e)} + logger.error("apply-patch failed for %s: %s", scope_for_log, e) + + +def _apply_patch_wrapper(args): + """Thin wrapper so ``ThreadPoolExecutor.submit`` can unpack a tuple.""" + return apply_patch_for_scope(*args) + + +# --------------------------------------------------------------------------- +# Top-level apply-patch orchestrator +# --------------------------------------------------------------------------- + +def apply_patch_from_file(patch_file_path, config_format_name, verbose, + dry_run, parallel, ignore_non_yang_tables, + ignore_path, preprocess=True, trace_io=None): + """Read a JSON-Patch file and apply it β€” the single implementation + used by all entry points. + + Parameters + ---------- + patch_file_path : str + Path to the JSON-Patch file. + config_format_name : str + ``"CONFIGDB"`` or ``"SONICYANG"``. + verbose : bool + dry_run : bool + parallel : bool + If *True*, apply per-ASIC changes in parallel threads. + ignore_non_yang_tables : bool + ignore_path : tuple/list of str + preprocess : bool + When *True* (default), fetch running config and run + ``append_emptytables_if_required``, ``filter_duplicate_patch_operations`` + and ``validate_patch``. Callers that already performed these steps + (or intentionally want to skip them) can pass *False*. + trace_io : IO, optional + Writable file-like object for writing the patch-sorter decision path + trace as JSON. ``None`` (default) disables tracing. + + Raises + ------ + GenericConfigUpdaterError + On validation failure or any per-scope failure. + """ + # 1. Read & validate patch file + with open(patch_file_path, 'r') as fh: + patch_json = json.loads(fh.read()) + + if not validate_patch_format(patch_json): + raise GenericConfigUpdaterError( + f"Invalid patch format in file: {patch_file_path}" + ) + + patch_ops = patch_json + config_format = ConfigFormat[config_format_name.upper()] + + # 2. Optional pre-processing (running-config fetch + YANG validation) + if preprocess: + all_running_config = get_all_running_config() + patch_ops = append_emptytables_if_required( + patch_ops, all_running_config + ) + patch_ops = filter_duplicate_patch_operations( + patch_ops, all_running_config + ) + if not validate_patch(patch_ops, all_running_config): + raise GenericConfigUpdaterError( + f"Failed validating patch: {patch_ops}" + ) + + # 3. Build a JsonPatch and split by scope + patch = jsonpatch.JsonPatch(patch_ops) + changes_by_scope = {} + + for change in patch: + scope, modified_path = extract_scope(change["path"]) + change["path"] = modified_path + changes_by_scope.setdefault(scope, []).append(change) + + # Empty case β€” still force YANG validation per scope + if not changes_by_scope: + asic_list = [multi_asic.DEFAULT_NAMESPACE] + if multi_asic.is_multi_asic(): + asic_list.extend(multi_asic.get_namespace_list()) + for asic in asic_list: + changes_by_scope[asic] = [] + + # 4. Dispatch + results = {} + if parallel: + with concurrent.futures.ThreadPoolExecutor() as executor: + arguments = [ + (sc, results, config_format, verbose, dry_run, + ignore_non_yang_tables, ignore_path, trace_io) + for sc in changes_by_scope.items() + ] + futures = [ + executor.submit(_apply_patch_wrapper, arg) + for arg in arguments + ] + concurrent.futures.wait(futures) + else: + for scope_changes in changes_by_scope.items(): + apply_patch_for_scope( + scope_changes, results, config_format, + verbose, dry_run, ignore_non_yang_tables, ignore_path, + trace_io, + ) + + # 5. Aggregate results + failures = [s for s, r in results.items() if not r['success']] + if failures: + msgs = '\n'.join( + f"- {s}: {results[s]['message']}" for s in failures + ) + raise GenericConfigUpdaterError( + f"Failed to apply patch on the following scopes:\n{msgs}" + ) + + +# --------------------------------------------------------------------------- +# Helper utilities (used by the gcu-standalone entry point) +# --------------------------------------------------------------------------- + +def multiasic_save_to_singlefile(filename): + """Save all ASIC configurations to a single file in multi-asic mode.""" + all_configs = {} + + # Get host configuration + cmd = ["sonic-cfggen", "-d", "--print-data"] + result = subprocess.run(cmd, capture_output=True, text=True, check=True) + host_config = json.loads(result.stdout) + all_configs['localhost'] = host_config + + # Get each ASIC configuration + for namespace in multi_asic.get_namespace_list(): + cmd = ["sonic-cfggen", "-d", "--print-data", "-n", namespace] + result = subprocess.run( + cmd, capture_output=True, text=True, check=True + ) + asic_config = json.loads(result.stdout) + all_configs[namespace] = asic_config + + # Save to file + with open(filename, 'w') as f: + json.dump(all_configs, f, indent=2) + + +def print_error(message): + """Print error message to stderr.""" + print(f"Error: {message}", file=sys.stderr) + + +def print_success(message): + """Print success message.""" + print(message) + + +# --------------------------------------------------------------------------- +# Sub-command implementations (used by gcu-standalone) +# --------------------------------------------------------------------------- + +def create_checkpoint(args): + """Create a checkpoint of the current configuration.""" + try: + if args.verbose: + print(f"Creating checkpoint: {args.checkpoint_name}") + + updater = GenericUpdater() + updater.checkpoint(args.checkpoint_name, args.verbose) + + print_success( + f"Checkpoint '{args.checkpoint_name}' created successfully." + ) + except Exception as ex: + print_error( + f"Failed to create checkpoint '{args.checkpoint_name}': {ex}" + ) + sys.exit(1) + + +def delete_checkpoint(args): + """Delete a checkpoint.""" + try: + if args.verbose: + print(f"Deleting checkpoint: {args.checkpoint_name}") + + updater = GenericUpdater() + updater.delete_checkpoint(args.checkpoint_name, args.verbose) + + print_success( + f"Checkpoint '{args.checkpoint_name}' deleted successfully." + ) + except Exception as ex: + print_error( + f"Failed to delete checkpoint '{args.checkpoint_name}': {ex}" + ) + sys.exit(1) + + +def list_checkpoints(args): + """List all available checkpoints.""" + try: + updater = GenericUpdater() + checkpoints = updater.list_checkpoints(args.time, args.verbose) + + if not checkpoints: + print("No checkpoints found.") + return + + if args.time and isinstance(checkpoints[0], dict): + print("Available checkpoints:") + for checkpoint in checkpoints: + print( + f" - {checkpoint['name']} " + f"(Last Modified: {checkpoint['time']})" + ) + else: + print("Available checkpoints:") + for checkpoint in checkpoints: + print(f" - {checkpoint}") + except Exception as ex: + print_error(f"Failed to list checkpoints: {ex}") + sys.exit(1) + + +def apply_patch(args): + """Apply a configuration patch β€” delegates to apply_patch_from_file.""" + trace_file = None + try: + if args.verbose: + print(f"Applying patch from: {args.patch_file}") + print(f"Format: {args.format}") + if args.dry_run: + print("** DRY RUN EXECUTION **") + + if getattr(args, 'path_trace', None): + trace_file = open(args.path_trace, 'w') + + apply_patch_from_file( + patch_file_path=args.patch_file, + config_format_name=args.format, + verbose=args.verbose, + dry_run=args.dry_run, + parallel=args.parallel, + ignore_non_yang_tables=args.ignore_non_yang_tables, + ignore_path=args.ignore_path, + preprocess=True, + trace_io=trace_file, + ) + + print_success("Patch applied successfully.") + except Exception as ex: + print_error(f"Failed to apply patch: {ex}") + sys.exit(1) + finally: + if trace_file: + trace_file.close() + + +def replace_config(args): + """Replace the entire configuration with a new configuration.""" + try: + if args.verbose: + print(f"Replacing configuration from: {args.config_file}") + print(f"Format: {args.format}") + + with open(args.config_file, 'r') as f: + target_config = json.loads(f.read()) + + config_format = ConfigFormat[args.format.upper()] + updater = GenericUpdater() + updater.replace( + target_config, config_format, args.verbose, False, + args.ignore_non_yang_tables, args.ignore_path, + ) + + print_success("Configuration replaced successfully.") + except Exception as ex: + print_error(f"Failed to replace configuration: {ex}") + sys.exit(1) + + +def save_config(args): + """Save the current configuration to a file.""" + try: + filename = args.filename if args.filename else DEFAULT_CONFIG_DB_FILE + + if args.verbose: + print(f"Saving configuration to: {filename}") + + if multi_asic.is_multi_asic(): + multiasic_save_to_singlefile(filename) + else: + cmd = ["sonic-cfggen", "-d", "--print-data"] + result = subprocess.run( + cmd, capture_output=True, text=True, check=True + ) + config_to_save = json.loads(result.stdout) + with open(filename, 'w') as f: + json.dump(config_to_save, f, indent=2) + + print_success(f"Configuration saved successfully to '{filename}'.") + except subprocess.CalledProcessError as e: + print_error(f"Failed to get current configuration: {e}") + sys.exit(1) + except Exception as ex: + print_error(f"Failed to save configuration: {ex}") + sys.exit(1) + + +def rollback_config(args): + """Rollback configuration to a checkpoint.""" + try: + if args.verbose: + print(f"Rolling back to checkpoint: {args.checkpoint_name}") + + updater = GenericUpdater() + updater.rollback( + args.checkpoint_name, args.verbose, False, + args.ignore_non_yang_tables, args.ignore_path, + ) + + print_success( + f"Configuration rolled back to " + f"'{args.checkpoint_name}' successfully." + ) + except Exception as ex: + print_error( + f"Failed to rollback to checkpoint " + f"'{args.checkpoint_name}': {ex}" + ) + sys.exit(1) + + +# --------------------------------------------------------------------------- +# Argument parser (shared by gcu-standalone entry point) +# --------------------------------------------------------------------------- + +def build_parser(): + """Build and return the argument parser.""" + parser = argparse.ArgumentParser( + description=( + 'GCU - Generic Config Updater for SONiC configuration management' + ), + formatter_class=argparse.RawDescriptionHelpFormatter, + epilog=""" +Examples: + %(prog)s create-checkpoint my-checkpoint + %(prog)s apply-patch patch.json + %(prog)s apply-patch patch.json --dry-run + %(prog)s replace config.json + %(prog)s save backup.json + """, + ) + + subparsers = parser.add_subparsers( + dest='command', help='Available commands' + ) + + # ---- create-checkpoint ---- + p = subparsers.add_parser( + 'create-checkpoint', + help='Create a checkpoint of the current configuration', + ) + p.add_argument('checkpoint_name', help='Name for the checkpoint') + p.add_argument( + '-v', '--verbose', action='store_true', + help='Print additional details', + ) + + # ---- delete-checkpoint ---- + p = subparsers.add_parser( + 'delete-checkpoint', help='Delete a checkpoint', + ) + p.add_argument( + 'checkpoint_name', help='Name of the checkpoint to delete', + ) + p.add_argument( + '-v', '--verbose', action='store_true', + help='Print additional details', + ) + + # ---- list-checkpoints ---- + p = subparsers.add_parser( + 'list-checkpoints', help='List all available checkpoints', + ) + p.add_argument( + '-t', '--time', action='store_true', + help='Include last modified time for each checkpoint', + ) + p.add_argument( + '-v', '--verbose', action='store_true', + help='Print additional details', + ) + + # ---- apply-patch ---- + p = subparsers.add_parser( + 'apply-patch', help='Apply a configuration patch', + ) + p.add_argument('patch_file', help='Path to the JSON patch file') + p.add_argument( + '-f', '--format', choices=['CONFIGDB', 'SONICYANG'], + default='CONFIGDB', + help='Format of the patch file (default: CONFIGDB)', + ) + p.add_argument( + '-v', '--verbose', action='store_true', + help='Print additional details', + ) + p.add_argument( + '-d', '--dry-run', action='store_true', default=False, + help='Test out the command without affecting config state', + ) + p.add_argument( + '-p', '--parallel', action='store_true', + help='Apply changes to all ASICs in parallel (multi-asic only)', + ) + p.add_argument( + '-n', '--ignore-non-yang-tables', action='store_true', + help='Ignore validation for tables without YANG models', + ) + p.add_argument( + '-i', '--ignore-path', action='append', default=[], + help='Ignore validation for config specified by given path ' + '(JsonPointer)', + ) + p.add_argument( + '-t', '--path-trace', + metavar='FILE', + default=None, + help='Filename to write decision path trace for patch generation as JSON', + ) + + # ---- replace ---- + p = subparsers.add_parser( + 'replace', help='Replace the entire configuration', + ) + p.add_argument('config_file', help='Path to the configuration file') + p.add_argument( + '-f', '--format', choices=['CONFIGDB', 'SONICYANG'], + default='CONFIGDB', + help='Format of the configuration file (default: CONFIGDB)', + ) + p.add_argument( + '-v', '--verbose', action='store_true', + help='Print additional details', + ) + p.add_argument( + '-n', '--ignore-non-yang-tables', action='store_true', + help='Ignore validation for tables without YANG models', + ) + p.add_argument( + '-i', '--ignore-path', action='append', default=[], + help='Ignore validation for config specified by given path ' + '(JsonPointer)', + ) + + # ---- save ---- + p = subparsers.add_parser( + 'save', help='Save the current configuration to a file', + ) + p.add_argument( + 'filename', nargs='?', + help=f'Output filename (default: {DEFAULT_CONFIG_DB_FILE})', + ) + p.add_argument( + '-v', '--verbose', action='store_true', + help='Print additional details', + ) + + # ---- rollback ---- + p = subparsers.add_parser( + 'rollback', help='Rollback configuration to a checkpoint', + ) + p.add_argument( + 'checkpoint_name', + help='Name of the checkpoint to rollback to', + ) + p.add_argument( + '-v', '--verbose', action='store_true', + help='Print additional details', + ) + p.add_argument( + '-n', '--ignore-non-yang-tables', action='store_true', + help='Ignore validation for tables without YANG models', + ) + p.add_argument( + '-i', '--ignore-path', action='append', default=[], + help='Ignore validation for config specified by given path ' + '(JsonPointer)', + ) + + return parser + + +# --------------------------------------------------------------------------- +# Main entry point (used by gcu-standalone console_scripts) +# --------------------------------------------------------------------------- + +def main(): + """Main entry point for the gcu-standalone console script.""" + parser = build_parser() + args = parser.parse_args() + + if not args.command: + parser.print_help() + return + + # Validate file paths if provided + if hasattr(args, 'patch_file') and args.patch_file: + if not os.path.exists(args.patch_file): + print_error(f"Patch file not found: {args.patch_file}") + sys.exit(1) + + if hasattr(args, 'config_file') and args.config_file: + if not os.path.exists(args.config_file): + print_error(f"Config file not found: {args.config_file}") + sys.exit(1) + + command_functions = { + 'create-checkpoint': create_checkpoint, + 'delete-checkpoint': delete_checkpoint, + 'list-checkpoints': list_checkpoints, + 'apply-patch': apply_patch, + 'replace': replace_config, + 'save': save_config, + 'rollback': rollback_config, + } + + if args.command in command_functions: + command_functions[args.command](args) + else: + print_error(f"Unknown command: {args.command}") + parser.print_help() + sys.exit(1) + + +if __name__ == '__main__': + main() diff --git a/generic_config_updater/pytest.ini b/generic_config_updater/pytest.ini new file mode 100644 index 000000000..bf901aa5c --- /dev/null +++ b/generic_config_updater/pytest.ini @@ -0,0 +1,5 @@ +[pytest] +addopts = --cov-config=.coveragerc --cov --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -vv +testpaths = ../tests/generic_config_updater + + diff --git a/generic_config_updater/setup.py b/generic_config_updater/setup.py new file mode 100644 index 000000000..2f0b0aa6d --- /dev/null +++ b/generic_config_updater/setup.py @@ -0,0 +1,54 @@ +# generic_config_updater/ β€” Standalone GCU wheel build context +# +# This setup.py builds the 'sonic-gcu' Python wheel, published independently +# from the main 'sonic-utilities' wheel. The gcu-standalone binary installed +# from this wheel is placed at /opt/sonic/gcu/current/bin/gcu-standalone and +# allows the GCU container to deliver fixes to generic_config_updater without +# touching the host sonic-utilities package. +# +# pytest.ini and .coveragerc in this directory configure test runs scoped to +# GCU only (see azure-pipelines.yml 'Build Python 3 wheel for GCU' step). +# +# Dependencies are intentionally absent from install_requires: the GCU venv is +# created with --system-site-packages and the wheel is installed with --no-deps, +# so all runtime dependencies (SONiC packages and third-party libs) are +# inherited from the host SONiC environment. +from setuptools import setup + + +setup( + name='sonic-gcu', + version='1.0.0', + description='GCU package for SONiC', + license='Apache 2.0', + author='SONiC Team', + author_email='linuxnetdev@microsoft.com', + url='https://github.com/Azure/sonic-utilities/generic_config_updater', + maintainer='Xincun Li', + maintainer_email='xincun.li@microsoft.com', + package_dir={'generic_config_updater': '.'}, + packages=[ + 'generic_config_updater', + ], + package_data={ + 'generic_config_updater': ['gcu_services_validator.conf.json', 'gcu_field_operation_validators.conf.json'] + }, + entry_points={ + 'console_scripts': [ + 'gcu-standalone=generic_config_updater.main:main', + ] + }, + classifiers=[ + 'Development Status :: 3 - Alpha', + 'Environment :: Console', + 'Intended Audience :: Developers', + 'Intended Audience :: Information Technology', + 'Intended Audience :: System Administrators', + 'License :: OSI Approved :: Apache Software License', + 'Natural Language :: English', + 'Operating System :: POSIX :: Linux', + 'Programming Language :: Python :: 3.7', + 'Topic :: Utilities', + ], + keywords='SONiC GCU package' +) diff --git a/tests/config_test.py b/tests/config_test.py index 7ba574b06..56401d919 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -1817,8 +1817,9 @@ def setUp(self): self.any_checkpoint_name = "any_checkpoint_name" self.any_checkpoints_list = ["checkpoint1", "checkpoint2", "checkpoint3"] self.any_checkpoints_list_as_text = json.dumps(self.any_checkpoints_list, indent=4) + self.any_checkpoints_list_with_time_as_text = json.dumps(self.any_checkpoints_list_with_time, indent=4) - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__no_params__get_required_params_error_msg(self): # Arrange unexpected_exit_code = 0 @@ -1831,7 +1832,7 @@ def test_apply_patch__no_params__get_required_params_error_msg(self): self.assertNotEqual(unexpected_exit_code, result.exit_code) self.assertTrue(expected_output in result.output) - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__help__gets_help_msg(self): # Arrange expected_exit_code = 0 @@ -1844,14 +1845,18 @@ def test_apply_patch__help__gets_help_msg(self): self.assertEqual(expected_exit_code, result.exit_code) self.assertTrue(expected_output in result.output) - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__only_required_params__default_values_used_for_optional_params(self): # Arrange expected_exit_code = 0 expected_output = "Patch applied successfully" expected_call_with_default_values = mock.call(self.any_patch, ConfigFormat.CONFIGDB, False, False, False, ()) mock_generic_updater = mock.Mock() - with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_patch_as_text)): # Act @@ -1863,7 +1868,11 @@ def test_apply_patch__only_required_params__default_values_used_for_optional_par mock_generic_updater.apply_patch.assert_called_once() mock_generic_updater.apply_patch.assert_has_calls([expected_call_with_default_values]) - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__all_optional_params_non_default__non_default_values_used(self): # Arrange expected_exit_code = 0 @@ -1872,7 +1881,7 @@ def test_apply_patch__all_optional_params_non_default__non_default_values_used(s expected_call_with_non_default_values = \ mock.call(self.any_patch, ConfigFormat.SONICYANG, True, True, True, expected_ignore_path_tuple) mock_generic_updater = mock.Mock() - with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_patch_as_text)): # Act @@ -1893,14 +1902,18 @@ def test_apply_patch__all_optional_params_non_default__non_default_values_used(s mock_generic_updater.apply_patch.assert_called_once() mock_generic_updater.apply_patch.assert_has_calls([expected_call_with_non_default_values]) - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__exception_thrown__error_displayed_error_code_returned(self): # Arrange unexpected_exit_code = 0 any_error_message = "any_error_message" mock_generic_updater = mock.Mock() mock_generic_updater.apply_patch.side_effect = Exception(any_error_message) - with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_patch_as_text)): # Act @@ -1929,13 +1942,65 @@ def test_apply_patch__optional_parameters_passed_correctly(self): ["--ignore-path", "/ANY_TABLE"], mock.call(self.any_patch, ConfigFormat.CONFIGDB, False, False, False, ("/ANY_TABLE",))) - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.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('generic_config_updater.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)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def validate_apply_patch_optional_parameter(self, param_args, expected_call): # Arrange expected_exit_code = 0 expected_output = "Patch applied successfully" mock_generic_updater = mock.Mock() - with mock.patch('config.main.GenericUpdater', return_value=mock_generic_updater): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_generic_updater): with mock.patch('builtins.open', mock.mock_open(read_data=self.any_patch_as_text)): # Act @@ -1949,6 +2014,150 @@ def validate_apply_patch_optional_parameter(self, param_args, expected_call): mock_generic_updater.apply_patch.assert_called_once() mock_generic_updater.apply_patch.assert_has_calls([expected_call]) + def test_filter_duplicate_patch_operations_basic(self): + from generic_config_updater.main import filter_duplicate_patch_operations + # Patch tries to add duplicate port to ACL_TABLE leaf-list + patch_ops = [ + {"op": "add", "path": "/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet1"}, + {"op": "add", "path": "/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet2"}, + {"op": "add", "path": "/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet3"} + ] + config = { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet1", "Ethernet2"] + } + } + } + filtered_patch_ops = filter_duplicate_patch_operations(patch_ops, json.dumps(config)) + # Only the non-duplicate add ops should remain + self.assertEqual(len(filtered_patch_ops), 1, "Only Ethernet3 add op should remain") + self.assertEqual(filtered_patch_ops[0]['value'], "Ethernet3", "Only Ethernet3 add op should remain") + + def test_filter_duplicate_patch_operations_no_duplicates(self): + from generic_config_updater.main import filter_duplicate_patch_operations + # Patch does not contain any duplicate ops + patch_ops = [ + {"op": "add", "path": "/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet3"}, + {"op": "remove", "path": "/ACL_TABLE/MY_ACL_TABLE/ports/0"}, + {"op": "replace", "path": "/ACL_TABLE/MY_ACL_TABLE/description", "value": "New description"} + ] + config = { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet1", "Ethernet2"], + "description": "Old description" + } + } + } + filtered_patch_ops = filter_duplicate_patch_operations(patch_ops, json.dumps(config)) + # All ops should remain as there are no duplicates + self.assertEqual(len(filtered_patch_ops), len(patch_ops), "All patch should remain as no duplicates") + self.assertEqual(filtered_patch_ops, patch_ops, "Filtered ops should match original ops") + + def test_filter_duplicate_patch_operations_non_list_field(self): + from generic_config_updater.main import filter_duplicate_patch_operations + # Patch tries to add duplicate entries to a non-list field + patch_ops = [ + {"op": "add", "path": "/PORT/Ethernet0/description", "value": "Desc1"}, + {"op": "add", "path": "/PORT/Ethernet0/description", "value": "Desc2"} + ] + config = { + "PORT": { + "Ethernet0": { + "description": "Existing description" + } + } + } + filtered_patch_ops = filter_duplicate_patch_operations(patch_ops, json.dumps(config)) + # Both ops should remain as description is not a list field + self.assertEqual(len(filtered_patch_ops), len(patch_ops), "Both add ops should remain for non-list field") + self.assertEqual(filtered_patch_ops, patch_ops, "Filtered ops should match original ops") + + def test_filter_duplicate_patch_operations_empty_config(self): + from generic_config_updater.main import filter_duplicate_patch_operations + patch_ops = [ + {"op": "add", "path": "/PORT/Ethernet0/allowed_vlans/-", "value": "100"}, + {"op": "add", "path": "/PORT/Ethernet0/allowed_vlans/-", "value": "200"} + ] + config = { + "PORT": { + "Ethernet0": { + "allowed_vlans": [] + } + } + } + filtered_patch_ops = filter_duplicate_patch_operations(patch_ops, json.dumps(config)) + # All ops should remain as config is empty and has no existing entries + self.assertEqual(len(filtered_patch_ops), len(patch_ops), "All add ops should remain for empty list") + self.assertEqual(filtered_patch_ops, patch_ops, "Filtered ops should match original ops") + + def test_append_emptytables_if_required_basic_config(self): + from generic_config_updater.main import append_emptytables_if_required + + patch_ops = [ + {"op": "add", "path": "/ACL_TABLE2/ports", "value": ["Ethernet1", "Ethernet2"]} + ] + config = { + "ACL_TABLE": { + "ports": ["Ethernet3"] + } + } + updated_patch_ops = append_emptytables_if_required(patch_ops, json.dumps(config)) + assert len(updated_patch_ops) == 2, "Patch should have 2 operations after appending empty tables" + assert updated_patch_ops[0]['path'] == "/ACL_TABLE2", "First op should create ACL_TABLE2" + assert updated_patch_ops[0]['op'] == "add", "First op should be an add operation" + assert updated_patch_ops[1] == patch_ops[0], "Second op should be the original add operation" + + def test_append_emptytables_if_required_no_action_needed(self): + from generic_config_updater.main import append_emptytables_if_required + patch_ops = [ + {"op": "add", "path": "/ACL_TABLE/ports", "value": ["Ethernet1", "Ethernet2"]} + ] + config = { + "ACL_TABLE": { + "ports": ["Ethernet3"] + } + } + updated_patch_ops = append_emptytables_if_required(patch_ops, json.dumps(config)) + assert len(updated_patch_ops) == 1, "Patch should remain unchanged with 1 operation" + assert updated_patch_ops[0] == patch_ops[0], "Patch operation should remain unchanged" + + def test_append_emptytables_if_required_multiple_tables(self): + from generic_config_updater.main import append_emptytables_if_required + + patch_ops = [ + {"op": "add", "path": "/TABLE1/field", "value": "value1"}, + {"op": "add", "path": "/TABLE2/field", "value": "value2"} + ] + config = {} + updated_patch_ops = append_emptytables_if_required(patch_ops, json.dumps(config)) + assert len(updated_patch_ops) == 4, "Patch should have 4 operations after appending empty tables" + assert updated_patch_ops[0]['path'] == "/TABLE1", "First op should create TABLE1" + assert updated_patch_ops[1] == patch_ops[0], "Second op should be the original TABLE1 add operation" + assert updated_patch_ops[2]['path'] == "/TABLE2", "Third op should create TABLE2" + assert updated_patch_ops[3] == patch_ops[1], "Fourth op should be the original TABLE2 add operation" + + # ------------------------------------------------------------------ + # print_dry_run_message + # ------------------------------------------------------------------ + + def test_print_dry_run_message_dry_run_true_prints_banner(self): + """print_dry_run_message with dry_run=True should echo a banner.""" + from config.main import print_dry_run_message + result = self.runner.invoke( + click.command()(lambda: print_dry_run_message(True)) + ) + self.assertIn("DRY RUN", result.output) + + def test_print_dry_run_message_dry_run_false_no_output(self): + """print_dry_run_message with dry_run=False should produce no output.""" + from config.main import print_dry_run_message + result = self.runner.invoke( + click.command()(lambda: print_dry_run_message(False)) + ) + self.assertEqual(result.output.strip(), "") + def test_replace__no_params__get_required_params_error_msg(self): # Arrange unexpected_exit_code = 0 @@ -3497,12 +3706,16 @@ def setUp(self): self.all_config["asic1"] = data self.all_config["asic1"]["bgpraw"] = "" - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch_multiasic(self): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) @@ -3517,12 +3730,16 @@ def test_apply_patch_multiasic(self): # Verify mocked_open was called as expected mocked_open.assert_called_with(self.patch_file_path, 'r') - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch_dryrun_multiasic(self): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() # Mock ConfigDBConnector to ensure it's not called during dry-run @@ -3552,13 +3769,17 @@ def test_apply_patch_dryrun_multiasic(self): # Ensure ConfigDBConnector was never instantiated or called mock_config_db_connector.assert_not_called() - @patch('config.main.validate_patch', mock.Mock(return_value=True)) - @patch('config.main.concurrent.futures.wait', autospec=True) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.concurrent.futures.wait', autospec=True) def test_apply_patch_dryrun_parallel_multiasic(self, MockThreadPoolWait): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() # Mock ConfigDBConnector to ensure it's not called during dry-run @@ -3592,13 +3813,17 @@ def test_apply_patch_dryrun_parallel_multiasic(self, MockThreadPoolWait): # Ensure ConfigDBConnector was never instantiated or called mock_config_db_connector.assert_not_called() - @patch('config.main.validate_patch', mock.Mock(return_value=True)) - @patch('config.main.concurrent.futures.wait', autospec=True) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.concurrent.futures.wait', autospec=True) def test_apply_patch_check_running_in_parallel_multiasic(self, MockThreadPoolWait): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() # Mock ConfigDBConnector to ensure it's not called during dry-run @@ -3631,13 +3856,17 @@ def test_apply_patch_check_running_in_parallel_multiasic(self, MockThreadPoolWai # Ensure ConfigDBConnector was never instantiated or called mock_config_db_connector.assert_not_called() - @patch('config.main.validate_patch', mock.Mock(return_value=True)) - @patch('config.main.apply_patch_wrapper') + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) + @patch('generic_config_updater.main._apply_patch_wrapper') def test_apply_patch_check_apply_call_parallel_multiasic(self, mock_apply_patch): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() # Mock ConfigDBConnector to ensure it's not called during dry-run @@ -3672,13 +3901,17 @@ def test_apply_patch_check_apply_call_parallel_multiasic(self, mock_apply_patch) # Ensure ConfigDBConnector was never instantiated or called mock_config_db_connector.assert_not_called() - @patch('config.main.validate_patch', mock.Mock(return_value=True)) - @patch('config.main.concurrent.futures.wait', autospec=True) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.concurrent.futures.wait', autospec=True) def test_apply_patch_check_running_in_not_parallel_multiasic(self, MockThreadPoolWait): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() # Mock ConfigDBConnector to ensure it's not called during dry-run @@ -3710,12 +3943,16 @@ def test_apply_patch_check_running_in_not_parallel_multiasic(self, MockThreadPoo # Ensure ConfigDBConnector was never instantiated or called mock_config_db_connector.assert_not_called() - @patch('config.main.validate_patch', mock.Mock(return_value=True)) + @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( + communicate=mock.Mock(return_value=('{"some": "config"}', None)), + returncode=0 + ))) + @patch('generic_config_updater.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch_parallel_with_error_multiasic(self): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() # Mock ConfigDBConnector to ensure it's not called during dry-run @@ -3746,8 +3983,261 @@ def test_apply_patch_parallel_with_error_multiasic(self): # Ensure ConfigDBConnector was never instantiated or called mock_config_db_connector.assert_not_called() - @patch('config.main.subprocess.Popen') - @patch('config.main.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) + def test_filter_duplicate_patch_operations_basic_multiasic(self): + from generic_config_updater.main import filter_duplicate_patch_operations + import jsonpatch + # Multi-ASIC config: each ASIC has its own ACL_TABLE + config = { + "localhost": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet1", "Ethernet2"] + } + } + }, + "asic0": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet3", "Ethernet4"] + } + } + }, + "asic1": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet5", "Ethernet6"] + } + } + } + } + # Patch tries to add duplicate ports to each ASIC's ACL_TABLE leaf-list + patch_ops = [ + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet1"}, + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet2"}, + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet3"}, + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet4"}, + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet5"}, + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet6"} + ] + patch = jsonpatch.JsonPatch(patch_ops) + filtered_patch = filter_duplicate_patch_operations(patch, config) + filtered_ops = list(filtered_patch) + self.assertEqual(len(filtered_ops), 0, "All adds are duplicates, should be filtered out in multi-asic config") + + def test_filter_duplicate_patch_operations_no_duplicates_multiasic(self): + from generic_config_updater.main import filter_duplicate_patch_operations + import jsonpatch + # Multi-ASIC config: each ASIC has its own ACL_TABLE + config = { + "localhost": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet1", "Ethernet2"] + } + } + }, + "asic0": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet3", "Ethernet4"] + } + } + }, + "asic1": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet5", "Ethernet6"] + } + } + } + } + # Patch tries to add new ports to each ASIC's ACL_TABLE leaf-list + patch_ops = [ + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet7"}, + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet8"}, + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet9"}, + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet10"}, + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet11"}, + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet12"} + ] + patch = jsonpatch.JsonPatch(patch_ops) + filtered_patch = filter_duplicate_patch_operations(patch, config) + filtered_ops = list(filtered_patch) + self.assertEqual( + len(filtered_ops), + len(patch_ops), + "No adds are duplicates, none should be filtered out in multi-asic config" + ) + + def test_filter_duplicate_patch_operations_mixed_multiasic(self): + from generic_config_updater.main import filter_duplicate_patch_operations + import jsonpatch + # Multi-ASIC config: each ASIC has its own ACL_TABLE + config = { + "localhost": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet1", "Ethernet2"] + } + } + }, + "asic0": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet3", "Ethernet4"] + } + } + }, + "asic1": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": ["Ethernet5", "Ethernet6"] + } + } + } + } + # Patch tries to add some duplicate and some new ports to each ASIC's ACL_TABLE leaf-list + patch_ops = [ + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet1"}, # duplicate + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet8"}, # new + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet3"}, # duplicate + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet10"}, # new + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet5"}, # duplicate + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet12"} # new + ] + patch = jsonpatch.JsonPatch(patch_ops) + filtered_patch = filter_duplicate_patch_operations(patch, config) + filtered_ops = list(filtered_patch) + self.assertEqual( + len(filtered_ops), + 3, + "Three adds are duplicates, three are new and should remain in multi-asic config" + ) + + def test_filter_duplicate_patch_operations_empty_config_multiasic(self): + from generic_config_updater.main import filter_duplicate_patch_operations + import jsonpatch + config = { + "localhost": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": [] + } + } + }, + "asic0": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": [] + } + } + }, + "asic1": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "ports": [] + } + } + } + } + # Patch tries to add ports to each ASIC's ACL_TABLE leaf-list + patch_ops = [ + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet1"}, + {"op": "add", "path": "/localhost/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet2"}, + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet3"}, + {"op": "add", "path": "/asic0/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet4"}, + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet5"}, + {"op": "add", "path": "/asic1/ACL_TABLE/MY_ACL_TABLE/ports/-", "value": "Ethernet6"} + ] + patch = jsonpatch.JsonPatch(patch_ops) + filtered_patch = filter_duplicate_patch_operations(patch, config) + filtered_ops = list(filtered_patch) + self.assertEqual( + len(filtered_ops), + len(patch_ops), + "No adds are duplicates in empty list, " + "none should be filtered out in multi-asic config" + ) + + def test_test_append_emptytables_if_required_basic_config_multiasic(self): + from generic_config_updater.main import append_emptytables_if_required + # Multi-ASIC config: each ASIC has its own PORT table + config = { + "localhost": { + "PORT": { + "Ethernet0": { + "mtu": "9100" + } + } + }, + "asic0": { + "PORT": { + "Ethernet1": { + "mtu": "9100" + } + } + }, + "asic1": { + "PORT": { + "Ethernet2": { + "mtu": "9100" + } + } + } + } + # Patch does not include PORT table for asic1 + patch_ops = [ + {"op": "add", "path": "/localhost/BGP_NEIGHBOR/ARISTA01T1", "value": "10.0.0.1"}, + {"op": "add", "path": "/asic0/BGP_NEIGHBOR/ARISTA02T1", "value": "10.0.0.2"}, + {"op": "add", "path": "/asic1/BGP_NEIGHBOR/ARISTA02T1", "value": "10.0.0.3"} + ] + updated_patch = append_emptytables_if_required(patch_ops, config) + updated_patch_list = list(updated_patch) + assert len(updated_patch_list) == 6, "BGP_NEIGHBOR table for each namespace should be added to the patch" + assert updated_patch_list[0] == {"op": "add", "path": "/localhost/BGP_NEIGHBOR", "value": {}} + assert updated_patch_list[2] == {"op": "add", "path": "/asic0/BGP_NEIGHBOR", "value": {}} + assert updated_patch_list[4] == {"op": "add", "path": "/asic1/BGP_NEIGHBOR", "value": {}} + assert updated_patch_list[1] == patch_ops[0] + assert updated_patch_list[3] == patch_ops[1] + assert updated_patch_list[5] == patch_ops[2] + + def test_test_append_emptytables_if_required_no_additional_tables_multiasic(self): + from generic_config_updater.main import append_emptytables_if_required + # Multi-ASIC config: each ASIC has its own PORT table + config = { + "localhost": { + "PORT": { + "Ethernet0": { + "mtu": "9100" + } + } + }, + "asic0": { + "PORT": { + "Ethernet1": { + "mtu": "9100" + } + } + }, + "asic1": { + "PORT": { + "Ethernet2": { + "mtu": "9100" + } + } + } + } + # Patch already includes PORT table for each namespace + patch_ops = [ + {"op": "add", "path": "/localhost/PORT/Ethernet0/mtu", "value": "9200"}, + {"op": "add", "path": "/asic0/PORT/Ethernet1/mtu", "value": "9200"}, + {"op": "add", "path": "/asic1/PORT/Ethernet2/mtu", "value": "9200"} + ] + updated_patch = append_emptytables_if_required(patch_ops, config) + assert len(updated_patch) == len(patch_ops), "No additional tables should be added to the patch" + + @patch('generic_config_updater.main.subprocess.Popen') + @patch('sonic_yang_cfg_generator.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) def test_apply_patch_validate_patch_multiasic(self, mock_subprocess_popen): mock_instance = MagicMock() mock_instance.communicate.return_value = (json.dumps(self.all_config), 0) @@ -3756,7 +4246,7 @@ def test_apply_patch_validate_patch_multiasic(self, mock_subprocess_popen): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) @@ -3773,8 +4263,8 @@ def test_apply_patch_validate_patch_multiasic(self, mock_subprocess_popen): # Verify mocked_open was called as expected mocked_open.assert_called_with(self.patch_file_path, 'r') - @patch('config.main.subprocess.Popen') - @patch('config.main.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.subprocess.Popen') + @patch('sonic_yang_cfg_generator.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) def test_apply_patch_validate_patch_with_badpath_multiasic(self, mock_subprocess_popen): mock_instance = MagicMock() mock_instance.communicate.return_value = (json.dumps(self.all_config), 0) @@ -3793,7 +4283,7 @@ def test_apply_patch_validate_patch_with_badpath_multiasic(self, mock_subprocess # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(bad_patch)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) @@ -3810,8 +4300,8 @@ def test_apply_patch_validate_patch_with_badpath_multiasic(self, mock_subprocess # Verify mocked_open was called as expected mocked_open.assert_called_with(self.patch_file_path, 'r') - @patch('config.main.subprocess.Popen') - @patch('config.main.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.subprocess.Popen') + @patch('sonic_yang_cfg_generator.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) def test_apply_patch_parallel_badpath_multiasic(self, mock_subprocess_popen): mock_instance = MagicMock() mock_instance.communicate.return_value = (json.dumps(self.all_config), 0) @@ -3830,7 +4320,7 @@ def test_apply_patch_parallel_badpath_multiasic(self, mock_subprocess_popen): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(bad_patch)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) @@ -3848,8 +4338,8 @@ def test_apply_patch_parallel_badpath_multiasic(self, mock_subprocess_popen): # Verify mocked_open was called as expected mocked_open.assert_called_with(self.patch_file_path, 'r') - @patch('config.main.subprocess.Popen') - @patch('config.main.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) + @patch('generic_config_updater.main.subprocess.Popen') + @patch('sonic_yang_cfg_generator.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) def test_apply_patch_validate_patch_with_wrong_fetch_config(self, mock_subprocess_popen): mock_instance = MagicMock() mock_instance.communicate.return_value = (json.dumps(self.all_config), 2) @@ -3858,7 +4348,7 @@ def test_apply_patch_validate_patch_with_wrong_fetch_config(self, mock_subproces # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: # Mock GenericUpdater to avoid actual patch application - with patch('config.main.GenericUpdater') as mock_generic_updater: + with patch('generic_config_updater.main.GenericUpdater') as mock_generic_updater: mock_generic_updater.return_value.apply_patch = MagicMock() print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) @@ -4009,6 +4499,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('generic_config_updater.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('generic_config_updater.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/generic_config_updater/main_test.py b/tests/generic_config_updater/main_test.py new file mode 100644 index 000000000..3a61e2504 --- /dev/null +++ b/tests/generic_config_updater/main_test.py @@ -0,0 +1,965 @@ +import io +import json +import os +import subprocess +import sys +import unittest +from argparse import Namespace +from unittest import mock + +# Make sure the repo root is on the path +_TEST_DIR = os.path.dirname(os.path.abspath(__file__)) +_ROOT_DIR = os.path.dirname(os.path.dirname(_TEST_DIR)) +sys.path.insert(0, _ROOT_DIR) + +from generic_config_updater.generic_updater import ConfigFormat # noqa: E402 +from generic_config_updater.gu_common import GenericConfigUpdaterError # noqa: E402 +import generic_config_updater.main as gcu_main # noqa: E402 + + +# --------------------------------------------------------------------------- +# validate_patch_format +# --------------------------------------------------------------------------- + +class TestValidatePatchFormat(unittest.TestCase): + + def test_valid_patch_returns_true(self): + patch = [{"op": "add", "path": "/TABLE/key", "value": "v"}] + self.assertTrue(gcu_main.validate_patch_format(patch)) + + def test_valid_all_ops(self): + for op in ("add", "remove", "replace", "move", "copy", "test"): + patch = [{"op": op, "path": "/X"}] + self.assertTrue(gcu_main.validate_patch_format(patch)) + + def test_empty_list_is_valid(self): + self.assertTrue(gcu_main.validate_patch_format([])) + + def test_not_a_list_returns_false(self): + self.assertFalse(gcu_main.validate_patch_format({"op": "add", "path": "/X"})) + + def test_item_not_dict_returns_false(self): + self.assertFalse(gcu_main.validate_patch_format(["not_a_dict"])) + + def test_missing_op_returns_false(self): + self.assertFalse(gcu_main.validate_patch_format([{"path": "/X"}])) + + def test_missing_path_returns_false(self): + self.assertFalse(gcu_main.validate_patch_format([{"op": "add"}])) + + def test_invalid_op_returns_false(self): + self.assertFalse( + gcu_main.validate_patch_format([{"op": "invalid_op", "path": "/X"}]) + ) + + def test_none_input_returns_false(self): + self.assertFalse(gcu_main.validate_patch_format(None)) + + def test_multiple_valid_changes(self): + patch = [ + {"op": "add", "path": "/A", "value": {}}, + {"op": "remove", "path": "/B"}, + {"op": "replace", "path": "/C", "value": 1}, + ] + self.assertTrue(gcu_main.validate_patch_format(patch)) + + def test_one_invalid_in_list_returns_false(self): + patch = [ + {"op": "add", "path": "/A", "value": {}}, + {"op": "BAD", "path": "/B"}, + ] + self.assertFalse(gcu_main.validate_patch_format(patch)) + + +# --------------------------------------------------------------------------- +# get_all_running_config +# --------------------------------------------------------------------------- + +class TestGetAllRunningConfig(unittest.TestCase): + + def _make_popen(self, stdout, returncode): + proc = mock.Mock() + proc.communicate.return_value = (stdout, None) + proc.returncode = returncode + return proc + + def test_success_returns_config_string(self): + cfg = '{"PORT": {}}' + with mock.patch('subprocess.Popen', return_value=self._make_popen(cfg, 0)): + result = gcu_main.get_all_running_config() + self.assertEqual(result, cfg) + + def test_nonzero_returncode_raises(self): + with mock.patch('subprocess.Popen', + return_value=self._make_popen('', 1)): + with self.assertRaises(GenericConfigUpdaterError): + gcu_main.get_all_running_config() + + +# --------------------------------------------------------------------------- +# filter_duplicate_patch_operations +# --------------------------------------------------------------------------- + +class TestFilterDuplicatePatchOperations(unittest.TestCase): + + def test_no_leaf_list_ops_returned_unchanged(self): + patch_ops = [{"op": "add", "path": "/TABLE/key", "value": "v"}] + config = {} + result = gcu_main.filter_duplicate_patch_operations(patch_ops, json.dumps(config)) + self.assertEqual(result, patch_ops) + + def test_removes_duplicate_leaf_list_add(self): + config = {"ACL_TABLE": {"MY_ACL": {"ports": ["Eth0", "Eth1"]}}} + patch_ops = [ + {"op": "add", "path": "/ACL_TABLE/MY_ACL/ports/-", "value": "Eth0"}, + {"op": "add", "path": "/ACL_TABLE/MY_ACL/ports/-", "value": "Eth2"}, + ] + result = gcu_main.filter_duplicate_patch_operations(patch_ops, json.dumps(config)) + paths_values = [(op["path"], op["value"]) for op in result] + self.assertNotIn(("/ACL_TABLE/MY_ACL/ports/-", "Eth0"), paths_values) + self.assertIn(("/ACL_TABLE/MY_ACL/ports/-", "Eth2"), paths_values) + + def test_no_duplicates_nothing_removed(self): + config = {"ACL_TABLE": {"MY_ACL": {"ports": []}}} + patch_ops = [ + {"op": "add", "path": "/ACL_TABLE/MY_ACL/ports/-", "value": "Eth0"}, + {"op": "add", "path": "/ACL_TABLE/MY_ACL/ports/-", "value": "Eth1"}, + ] + result = gcu_main.filter_duplicate_patch_operations(patch_ops, json.dumps(config)) + self.assertEqual(len(result), 2) + + def test_accepts_dict_config(self): + config = {"ACL_TABLE": {"MY_ACL": {"ports": ["Eth0"]}}} + patch_ops = [ + {"op": "add", "path": "/ACL_TABLE/MY_ACL/ports/-", "value": "Eth0"}, + ] + result = gcu_main.filter_duplicate_patch_operations(patch_ops, config) + self.assertEqual(len(result), 0) + + +# --------------------------------------------------------------------------- +# append_emptytables_if_required +# --------------------------------------------------------------------------- + +class TestAppendEmptyTablesIfRequired(unittest.TestCase): + + def test_no_missing_tables_returned_unchanged(self): + config = {"TABLE1": {}} + patch_ops = [{"op": "add", "path": "/TABLE1/key", "value": "v"}] + result = gcu_main.append_emptytables_if_required(patch_ops, json.dumps(config)) + self.assertEqual(result, patch_ops) + + def test_missing_table_prepended(self): + config = {} + patch_ops = [{"op": "add", "path": "/TABLE1/key", "value": "v"}] + result = gcu_main.append_emptytables_if_required(patch_ops, json.dumps(config)) + self.assertEqual(result[0], {"op": "add", "path": "/TABLE1", "value": {}}) + self.assertEqual(result[1], patch_ops[0]) + + def test_multiple_missing_tables(self): + config = {} + patch_ops = [ + {"op": "add", "path": "/TABLE1/field", "value": "v1"}, + {"op": "add", "path": "/TABLE2/field", "value": "v2"}, + ] + result = gcu_main.append_emptytables_if_required(patch_ops, json.dumps(config)) + created_paths = [op["path"] for op in result if op.get("value") == {}] + self.assertIn("/TABLE1", created_paths) + self.assertIn("/TABLE2", created_paths) + + def test_accepts_dict_config(self): + config = {} + patch_ops = [{"op": "add", "path": "/TABLE1/key", "value": "v"}] + result = gcu_main.append_emptytables_if_required(patch_ops, config) + self.assertEqual(result[0]["path"], "/TABLE1") + + def test_op_without_path_skipped(self): + config = {} + patch_ops = [{"op": "add", "value": "v"}] + result = gcu_main.append_emptytables_if_required(patch_ops, config) + # Should not crash, just return the ops with no empty table inserted + self.assertEqual(len(result), 1) + + def test_empty_path_parts_skipped(self): + config = {} + patch_ops = [{"op": "add", "path": "/", "value": "v"}] + # Should not raise + result = gcu_main.append_emptytables_if_required(patch_ops, config) + self.assertIsInstance(result, list) + + def test_asic_scoped_table_path(self): + """Paths starting with asic0/TABLE should resolve two-level pointer.""" + config = {"asic0": {}} + patch_ops = [{"op": "add", "path": "/asic0/NEW_TABLE/key", "value": "v"}] + result = gcu_main.append_emptytables_if_required(patch_ops, json.dumps(config)) + created_paths = [op["path"] for op in result if op.get("value") == {}] + self.assertIn("/asic0/NEW_TABLE", created_paths) + + +# --------------------------------------------------------------------------- +# validate_patch +# --------------------------------------------------------------------------- + +class TestValidatePatch(unittest.TestCase): + + def _simple_ops(self): + return [{"op": "add", "path": "/TABLE/key", "value": "v"}] + + def test_returns_true_when_yang_not_available(self): + """When sonic_yang_cfg_generator is not importable, validation is skipped.""" + with mock.patch.dict('sys.modules', {'sonic_yang_cfg_generator': None}): + result = gcu_main.validate_patch([], json.dumps({})) + self.assertTrue(result) + + def test_returns_true_on_valid_config(self): + mock_generator = mock.Mock() + mock_generator.validate_config_db_json.return_value = True + mock_module = mock.Mock() + mock_module.SonicYangCfgDbGenerator.return_value = mock_generator + + with mock.patch.dict('sys.modules', {'sonic_yang_cfg_generator': mock_module}): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + result = gcu_main.validate_patch([], json.dumps({})) + self.assertTrue(result) + + def test_returns_false_when_validation_fails(self): + mock_generator = mock.Mock() + mock_generator.validate_config_db_json.return_value = False + mock_module = mock.Mock() + mock_module.SonicYangCfgDbGenerator.return_value = mock_generator + + with mock.patch.dict('sys.modules', {'sonic_yang_cfg_generator': mock_module}): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + result = gcu_main.validate_patch([], json.dumps({})) + self.assertFalse(result) + + def test_raises_on_unexpected_exception(self): + mock_module = mock.Mock() + mock_module.SonicYangCfgDbGenerator.side_effect = RuntimeError("boom") + + with mock.patch.dict('sys.modules', {'sonic_yang_cfg_generator': mock_module}): + with self.assertRaises(GenericConfigUpdaterError): + gcu_main.validate_patch([], json.dumps({})) + + def test_multiasic_validates_all_asics(self): + mock_generator = mock.Mock() + mock_generator.validate_config_db_json.return_value = True + mock_module = mock.Mock() + mock_module.SonicYangCfgDbGenerator.return_value = mock_generator + + config = {"localhost": {}, "asic0": {}, "asic1": {}} + + with mock.patch.dict('sys.modules', {'sonic_yang_cfg_generator': mock_module}): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=True): + with mock.patch('sonic_py_common.multi_asic.get_namespace_list', + return_value=['asic0', 'asic1']): + result = gcu_main.validate_patch([], json.dumps(config)) + self.assertTrue(result) + # Called once per host + once per asic + self.assertEqual(mock_generator.validate_config_db_json.call_count, 3) + + def test_multiasic_returns_false_when_asic_fails(self): + call_count = [0] + + def side_effect(_cfg): + call_count[0] += 1 + # Fail on the second call (first asic) + return call_count[0] != 2 + + mock_generator = mock.Mock() + mock_generator.validate_config_db_json.side_effect = side_effect + mock_module = mock.Mock() + mock_module.SonicYangCfgDbGenerator.return_value = mock_generator + + config = {"localhost": {}, "asic0": {}} + + with mock.patch.dict('sys.modules', {'sonic_yang_cfg_generator': mock_module}): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=True): + with mock.patch('sonic_py_common.multi_asic.get_namespace_list', + return_value=['asic0']): + result = gcu_main.validate_patch([], json.dumps(config)) + self.assertFalse(result) + + +# --------------------------------------------------------------------------- +# apply_patch_for_scope +# --------------------------------------------------------------------------- + +class TestApplyPatchForScope(unittest.TestCase): + + def test_success_records_success(self): + mock_updater = mock.Mock() + results = {} + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + gcu_main.apply_patch_for_scope( + ("", [{"op": "add", "path": "/T/k", "value": "v"}]), + results, ConfigFormat.CONFIGDB, False, False, False, () + ) + self.assertTrue(results[gcu_main.HOST_NAMESPACE]["success"]) + + def test_exception_records_failure(self): + mock_updater = mock.Mock() + mock_updater.apply_patch.side_effect = Exception("scope error") + results = {} + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + gcu_main.apply_patch_for_scope( + ("", [{"op": "add", "path": "/T/k", "value": "v"}]), + results, ConfigFormat.CONFIGDB, False, False, False, () + ) + key = list(results.keys())[0] + self.assertFalse(results[key]["success"]) + self.assertIn("scope error", results[key]["message"]) + + def test_host_namespace_scope_mapping(self): + mock_updater = mock.Mock() + results = {} + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + gcu_main.apply_patch_for_scope( + (gcu_main.HOST_NAMESPACE, []), + results, ConfigFormat.CONFIGDB, False, False, False, () + ) + # HOST_NAMESPACE scope should map correctly + self.assertIn(gcu_main.HOST_NAMESPACE, results) + + +# --------------------------------------------------------------------------- +# apply_patch_from_file +# --------------------------------------------------------------------------- + +class TestApplyPatchFromFile(unittest.TestCase): + + def _make_patch_file(self, patch_ops): + return mock.mock_open(read_data=json.dumps(patch_ops)) + + def test_invalid_format_raises(self): + bad_patch = {"op": "add"} # not a list + with mock.patch('builtins.open', mock.mock_open(read_data=json.dumps(bad_patch))): + with self.assertRaises(GenericConfigUpdaterError): + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, False, False, () + ) + + def test_success_no_preprocess(self): + patch_ops = [{"op": "add", "path": "/TABLE/key", "value": "v"}] + mock_updater = mock.Mock() + with mock.patch('builtins.open', self._make_patch_file(patch_ops)): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('generic_config_updater.main.extract_scope', + return_value=('', '/TABLE/key')): + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, False, False, (), preprocess=False + ) + mock_updater.apply_patch.assert_called_once() + + def test_preprocess_path_runs_helpers(self): + patch_ops = [{"op": "add", "path": "/TABLE/key", "value": "v"}] + running_cfg = json.dumps({"TABLE": {}}) + mock_updater = mock.Mock() + + with mock.patch('builtins.open', self._make_patch_file(patch_ops)): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + with mock.patch('generic_config_updater.main.get_all_running_config', + return_value=running_cfg): + with mock.patch('generic_config_updater.main.append_emptytables_if_required', + return_value=patch_ops) as mock_append: + with mock.patch('generic_config_updater.main.filter_duplicate_patch_operations', + return_value=patch_ops) as mock_filter: + with mock.patch('generic_config_updater.main.validate_patch', + return_value=True) as mock_validate: + with mock.patch('generic_config_updater.main.GenericUpdater', + return_value=mock_updater): + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, False, False, (), preprocess=True + ) + mock_append.assert_called_once() + mock_filter.assert_called_once() + mock_validate.assert_called_once() + + def test_preprocess_validation_failure_raises(self): + patch_ops = [{"op": "add", "path": "/TABLE/key", "value": "v"}] + running_cfg = json.dumps({}) + + with mock.patch('builtins.open', self._make_patch_file(patch_ops)): + with mock.patch('generic_config_updater.main.get_all_running_config', + return_value=running_cfg): + with mock.patch('generic_config_updater.main.append_emptytables_if_required', + return_value=patch_ops): + with mock.patch('generic_config_updater.main.filter_duplicate_patch_operations', + return_value=patch_ops): + with mock.patch('generic_config_updater.main.validate_patch', + return_value=False): + with self.assertRaises(GenericConfigUpdaterError): + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, False, False, (), preprocess=True + ) + + def test_parallel_dispatches_with_threadpool(self): + patch_ops = [{"op": "add", "path": "/TABLE/key", "value": "v"}] + mock_updater = mock.Mock() + + with mock.patch('builtins.open', self._make_patch_file(patch_ops)): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('generic_config_updater.main.extract_scope', + return_value=('', '/TABLE/key')): + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, True, False, (), preprocess=False + ) + mock_updater.apply_patch.assert_called_once() + + def test_scope_failure_raises(self): + patch_ops = [{"op": "add", "path": "/TABLE/key", "value": "v"}] + mock_updater = mock.Mock() + mock_updater.apply_patch.side_effect = Exception("boom") + + with mock.patch('builtins.open', self._make_patch_file(patch_ops)): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('generic_config_updater.main.extract_scope', + return_value=('', '/TABLE/key')): + with self.assertRaises(GenericConfigUpdaterError) as ctx: + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, False, False, (), preprocess=False + ) + self.assertIn("Failed to apply patch", str(ctx.exception)) + + def test_empty_patch_still_validates(self): + """Empty patch ops triggers per-asic validation loop.""" + patch_ops = [] + mock_updater = mock.Mock() + + with mock.patch('builtins.open', self._make_patch_file(patch_ops)): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, False, False, (), preprocess=False + ) + mock_updater.apply_patch.assert_called_once() + + def test_empty_patch_multiasic(self): + """Empty patch in multiasic triggers all asic namespaces.""" + patch_ops = [] + mock_updater = mock.Mock() + + with mock.patch('builtins.open', self._make_patch_file(patch_ops)): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=True): + with mock.patch('sonic_py_common.multi_asic.get_namespace_list', + return_value=['asic0', 'asic1']): + gcu_main.apply_patch_from_file( + '/fake/patch.json', 'CONFIGDB', + False, False, False, False, (), preprocess=False + ) + self.assertEqual(mock_updater.apply_patch.call_count, 3) # default + asic0 + asic1 + + +# --------------------------------------------------------------------------- +# print_error / print_success +# --------------------------------------------------------------------------- + +class TestPrintHelpers(unittest.TestCase): + + def test_print_error_writes_to_stderr(self): + captured = io.StringIO() + with mock.patch('sys.stderr', captured): + gcu_main.print_error("something went wrong") + self.assertIn("something went wrong", captured.getvalue()) + + def test_print_success_writes_to_stdout(self): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + gcu_main.print_success("all good") + self.assertIn("all good", captured.getvalue()) + + +# --------------------------------------------------------------------------- +# multiasic_save_to_singlefile +# --------------------------------------------------------------------------- + +class TestMultiasicSaveToSinglefile(unittest.TestCase): + + def test_saves_host_and_asic_configs(self): + host_config = {"PORT": {}} + asic_config = {"VLAN": {}} + + def fake_run(cmd, **kwargs): + result = mock.Mock() + if "-n" in cmd: + result.stdout = json.dumps(asic_config) + else: + result.stdout = json.dumps(host_config) + return result + + mock_open_obj = mock.mock_open() + with mock.patch('subprocess.run', side_effect=fake_run): + with mock.patch('sonic_py_common.multi_asic.get_namespace_list', + return_value=['asic0']): + with mock.patch('builtins.open', mock_open_obj): + gcu_main.multiasic_save_to_singlefile('/tmp/all_config.json') + + written = ''.join( + call.args[0] + for call in mock_open_obj().write.call_args_list + ) + saved = json.loads(written) + self.assertIn('localhost', saved) + self.assertIn('asic0', saved) + + +# --------------------------------------------------------------------------- +# Sub-command functions +# --------------------------------------------------------------------------- + +class TestCreateCheckpoint(unittest.TestCase): + + def _make_args(self, name='cp1', verbose=False): + return Namespace(checkpoint_name=name, verbose=verbose) + + def test_success(self): + mock_updater = mock.Mock() + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('generic_config_updater.main.print_success') as mock_ps: + gcu_main.create_checkpoint(self._make_args()) + mock_updater.checkpoint.assert_called_once_with('cp1', False) + mock_ps.assert_called_once() + + def test_success_verbose(self): + mock_updater = mock.Mock() + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + with mock.patch('generic_config_updater.main.print_success'): + gcu_main.create_checkpoint(self._make_args(verbose=True)) + self.assertIn('cp1', captured.getvalue()) + + def test_failure_calls_sys_exit(self): + mock_updater = mock.Mock() + mock_updater.checkpoint.side_effect = Exception("fail") + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with self.assertRaises(SystemExit): + gcu_main.create_checkpoint(self._make_args()) + + +class TestDeleteCheckpoint(unittest.TestCase): + + def _make_args(self, name='cp1', verbose=False): + return Namespace(checkpoint_name=name, verbose=verbose) + + def test_success(self): + mock_updater = mock.Mock() + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('generic_config_updater.main.print_success') as mock_ps: + gcu_main.delete_checkpoint(self._make_args()) + mock_updater.delete_checkpoint.assert_called_once_with('cp1', False) + mock_ps.assert_called_once() + + def test_success_verbose(self): + mock_updater = mock.Mock() + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + with mock.patch('generic_config_updater.main.print_success'): + gcu_main.delete_checkpoint(self._make_args(verbose=True)) + self.assertIn('cp1', captured.getvalue()) + + def test_failure_calls_sys_exit(self): + mock_updater = mock.Mock() + mock_updater.delete_checkpoint.side_effect = Exception("fail") + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with self.assertRaises(SystemExit): + gcu_main.delete_checkpoint(self._make_args()) + + +class TestListCheckpoints(unittest.TestCase): + + def _make_args(self, time=False, verbose=False): + return Namespace(time=time, verbose=verbose) + + def test_no_checkpoints(self): + mock_updater = mock.Mock() + mock_updater.list_checkpoints.return_value = [] + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + gcu_main.list_checkpoints(self._make_args()) + self.assertIn('No checkpoints', captured.getvalue()) + + def test_list_without_time(self): + mock_updater = mock.Mock() + mock_updater.list_checkpoints.return_value = ['cp1', 'cp2'] + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + gcu_main.list_checkpoints(self._make_args()) + output = captured.getvalue() + self.assertIn('cp1', output) + self.assertIn('cp2', output) + + def test_list_with_time(self): + mock_updater = mock.Mock() + mock_updater.list_checkpoints.return_value = [ + {'name': 'cp1', 'time': '2025-01-01T00:00:00Z'}, + ] + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + gcu_main.list_checkpoints(self._make_args(time=True)) + output = captured.getvalue() + self.assertIn('cp1', output) + self.assertIn('2025-01-01', output) + + def test_failure_calls_sys_exit(self): + mock_updater = mock.Mock() + mock_updater.list_checkpoints.side_effect = Exception("fail") + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with self.assertRaises(SystemExit): + gcu_main.list_checkpoints(self._make_args()) + + +class TestApplyPatchSubcommand(unittest.TestCase): + + def _make_args(self, patch_file='/fake/p.json', fmt='CONFIGDB', + verbose=False, dry_run=False, parallel=False, + ignore_non_yang_tables=False, ignore_path=None, + path_trace=None): + return Namespace( + patch_file=patch_file, + format=fmt, + verbose=verbose, + dry_run=dry_run, + parallel=parallel, + ignore_non_yang_tables=ignore_non_yang_tables, + ignore_path=ignore_path or [], + path_trace=path_trace, + ) + + def test_success(self): + with mock.patch('generic_config_updater.main.apply_patch_from_file') as mock_apf: + with mock.patch('generic_config_updater.main.print_success') as mock_ps: + gcu_main.apply_patch(self._make_args()) + mock_apf.assert_called_once() + mock_ps.assert_called_once() + + def test_verbose_prints_details(self): + with mock.patch('generic_config_updater.main.apply_patch_from_file'): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + with mock.patch('generic_config_updater.main.print_success'): + gcu_main.apply_patch(self._make_args(verbose=True, dry_run=True)) + output = captured.getvalue() + self.assertIn('/fake/p.json', output) + self.assertIn('DRY RUN', output) + + def test_failure_calls_sys_exit(self): + with mock.patch('generic_config_updater.main.apply_patch_from_file', + side_effect=Exception("oops")): + with self.assertRaises(SystemExit): + gcu_main.apply_patch(self._make_args()) + + +class TestReplaceConfigSubcommand(unittest.TestCase): + + def _make_args(self, config_file='/fake/cfg.json', fmt='CONFIGDB', + verbose=False, ignore_non_yang_tables=False, + ignore_path=None): + return Namespace( + config_file=config_file, + format=fmt, + verbose=verbose, + ignore_non_yang_tables=ignore_non_yang_tables, + ignore_path=ignore_path or [], + ) + + def test_success(self): + mock_updater = mock.Mock() + cfg = json.dumps({"PORT": {}}) + with mock.patch('builtins.open', mock.mock_open(read_data=cfg)): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('generic_config_updater.main.print_success') as mock_ps: + gcu_main.replace_config(self._make_args()) + mock_updater.replace.assert_called_once() + mock_ps.assert_called_once() + + def test_verbose_prints_details(self): + mock_updater = mock.Mock() + cfg = json.dumps({}) + with mock.patch('builtins.open', mock.mock_open(read_data=cfg)): + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + with mock.patch('generic_config_updater.main.print_success'): + gcu_main.replace_config(self._make_args(verbose=True)) + self.assertIn('/fake/cfg.json', captured.getvalue()) + + def test_failure_calls_sys_exit(self): + with mock.patch('builtins.open', side_effect=Exception("no file")): + with self.assertRaises(SystemExit): + gcu_main.replace_config(self._make_args()) + + +class TestSaveConfigSubcommand(unittest.TestCase): + + def _make_args(self, filename=None, verbose=False): + return Namespace(filename=filename, verbose=verbose) + + def test_save_defaults_to_config_db_file(self): + fake_cfg = json.dumps({"PORT": {}}) + + def fake_run(cmd, **kwargs): + r = mock.Mock() + r.stdout = fake_cfg + return r + + mock_open_obj = mock.mock_open() + with mock.patch('subprocess.run', side_effect=fake_run): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + with mock.patch('builtins.open', mock_open_obj): + with mock.patch('generic_config_updater.main.print_success') as mock_ps: + gcu_main.save_config(self._make_args()) + mock_ps.assert_called_once() + + def test_save_with_explicit_filename_verbose(self): + fake_cfg = json.dumps({}) + + def fake_run(cmd, **kwargs): + r = mock.Mock() + r.stdout = fake_cfg + return r + + mock_open_obj = mock.mock_open() + with mock.patch('subprocess.run', side_effect=fake_run): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + with mock.patch('builtins.open', mock_open_obj): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + with mock.patch('generic_config_updater.main.print_success'): + gcu_main.save_config(self._make_args(filename='/tmp/out.json', verbose=True)) + self.assertIn('/tmp/out.json', captured.getvalue()) + + def test_save_multiasic(self): + with mock.patch('generic_config_updater.main.multiasic_save_to_singlefile') as mock_save: + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=True): + with mock.patch('generic_config_updater.main.print_success'): + gcu_main.save_config(self._make_args(filename='/tmp/out.json')) + mock_save.assert_called_once_with('/tmp/out.json') + + def test_subprocess_error_exits(self): + with mock.patch('subprocess.run', + side_effect=subprocess.CalledProcessError(1, 'cmd')): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + with self.assertRaises(SystemExit): + gcu_main.save_config(self._make_args()) + + def test_generic_exception_exits(self): + with mock.patch('subprocess.run', side_effect=RuntimeError("oops")): + with mock.patch('sonic_py_common.multi_asic.is_multi_asic', return_value=False): + with self.assertRaises(SystemExit): + gcu_main.save_config(self._make_args()) + + +class TestRollbackConfigSubcommand(unittest.TestCase): + + def _make_args(self, name='cp1', verbose=False, + ignore_non_yang_tables=False, ignore_path=None): + return Namespace( + checkpoint_name=name, + verbose=verbose, + ignore_non_yang_tables=ignore_non_yang_tables, + ignore_path=ignore_path or [], + ) + + def test_success(self): + mock_updater = mock.Mock() + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with mock.patch('generic_config_updater.main.print_success') as mock_ps: + gcu_main.rollback_config(self._make_args()) + mock_updater.rollback.assert_called_once() + mock_ps.assert_called_once() + + def test_verbose_prints_details(self): + mock_updater = mock.Mock() + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + captured = io.StringIO() + with mock.patch('sys.stdout', captured): + with mock.patch('generic_config_updater.main.print_success'): + gcu_main.rollback_config(self._make_args(verbose=True)) + self.assertIn('cp1', captured.getvalue()) + + def test_failure_calls_sys_exit(self): + mock_updater = mock.Mock() + mock_updater.rollback.side_effect = Exception("fail") + with mock.patch('generic_config_updater.main.GenericUpdater', return_value=mock_updater): + with self.assertRaises(SystemExit): + gcu_main.rollback_config(self._make_args()) + + +# --------------------------------------------------------------------------- +# build_parser +# --------------------------------------------------------------------------- + +class TestBuildParser(unittest.TestCase): + + def setUp(self): + self.parser = gcu_main.build_parser() + + def test_parser_returns_argparse_parser(self): + import argparse + self.assertIsNotNone(self.parser) + self.assertIsInstance(self.parser, argparse.ArgumentParser) + + def test_create_checkpoint_command(self): + args = self.parser.parse_args(['create-checkpoint', 'mycp']) + self.assertEqual(args.command, 'create-checkpoint') + self.assertEqual(args.checkpoint_name, 'mycp') + self.assertFalse(args.verbose) + + def test_create_checkpoint_verbose(self): + args = self.parser.parse_args(['create-checkpoint', 'mycp', '--verbose']) + self.assertTrue(args.verbose) + + def test_delete_checkpoint_command(self): + args = self.parser.parse_args(['delete-checkpoint', 'mycp']) + self.assertEqual(args.command, 'delete-checkpoint') + self.assertEqual(args.checkpoint_name, 'mycp') + + def test_list_checkpoints_command(self): + args = self.parser.parse_args(['list-checkpoints']) + self.assertEqual(args.command, 'list-checkpoints') + self.assertFalse(args.time) + + def test_list_checkpoints_with_time(self): + args = self.parser.parse_args(['list-checkpoints', '--time']) + self.assertTrue(args.time) + + def test_apply_patch_command_defaults(self): + args = self.parser.parse_args(['apply-patch', 'my.json']) + self.assertEqual(args.command, 'apply-patch') + self.assertEqual(args.patch_file, 'my.json') + self.assertEqual(args.format, 'CONFIGDB') + self.assertFalse(args.dry_run) + self.assertFalse(args.parallel) + self.assertFalse(args.ignore_non_yang_tables) + self.assertEqual(args.ignore_path, []) + + def test_apply_patch_all_flags(self): + args = self.parser.parse_args([ + 'apply-patch', 'my.json', + '--format', 'SONICYANG', + '--dry-run', + '--parallel', + '--ignore-non-yang-tables', + '--ignore-path', '/T1', + '--ignore-path', '/T2', + '--verbose', + ]) + self.assertEqual(args.format, 'SONICYANG') + self.assertTrue(args.dry_run) + self.assertTrue(args.parallel) + self.assertTrue(args.ignore_non_yang_tables) + self.assertEqual(args.ignore_path, ['/T1', '/T2']) + self.assertTrue(args.verbose) + + def test_replace_command_defaults(self): + args = self.parser.parse_args(['replace', 'cfg.json']) + self.assertEqual(args.command, 'replace') + self.assertEqual(args.config_file, 'cfg.json') + self.assertEqual(args.format, 'CONFIGDB') + + def test_save_command_default_filename(self): + args = self.parser.parse_args(['save']) + self.assertEqual(args.command, 'save') + self.assertIsNone(args.filename) + + def test_save_command_explicit_filename(self): + args = self.parser.parse_args(['save', '/tmp/out.json']) + self.assertEqual(args.filename, '/tmp/out.json') + + def test_rollback_command(self): + args = self.parser.parse_args(['rollback', 'cp1']) + self.assertEqual(args.command, 'rollback') + self.assertEqual(args.checkpoint_name, 'cp1') + + +# --------------------------------------------------------------------------- +# main() +# --------------------------------------------------------------------------- + +class TestMain(unittest.TestCase): + + def _run_main(self, argv): + with mock.patch('sys.argv', ['gcu-standalone'] + argv): + try: + gcu_main.main() + except SystemExit: + pass + + def test_no_command_prints_help(self): + captured = io.StringIO() + with mock.patch('sys.argv', ['gcu-standalone']): + with mock.patch('sys.stdout', captured): + gcu_main.main() + # argparse prints usage/help on no subcommand + # main() calls parser.print_help() and returns + + def test_create_checkpoint_dispatched(self): + with mock.patch('generic_config_updater.main.create_checkpoint') as mock_fn: + with mock.patch('sys.argv', ['gcu', 'create-checkpoint', 'mycp']): + gcu_main.main() + mock_fn.assert_called_once() + + def test_delete_checkpoint_dispatched(self): + with mock.patch('generic_config_updater.main.delete_checkpoint') as mock_fn: + with mock.patch('sys.argv', ['gcu', 'delete-checkpoint', 'mycp']): + gcu_main.main() + mock_fn.assert_called_once() + + def test_list_checkpoints_dispatched(self): + with mock.patch('generic_config_updater.main.list_checkpoints') as mock_fn: + with mock.patch('sys.argv', ['gcu', 'list-checkpoints']): + gcu_main.main() + mock_fn.assert_called_once() + + def test_save_dispatched(self): + with mock.patch('generic_config_updater.main.save_config') as mock_fn: + with mock.patch('sys.argv', ['gcu', 'save']): + gcu_main.main() + mock_fn.assert_called_once() + + def test_rollback_dispatched(self): + with mock.patch('generic_config_updater.main.rollback_config') as mock_fn: + with mock.patch('sys.argv', ['gcu', 'rollback', 'cp1']): + gcu_main.main() + mock_fn.assert_called_once() + + def test_apply_patch_missing_file_exits(self): + with mock.patch('sys.argv', ['gcu', 'apply-patch', '/nonexistent/file.json']): + with self.assertRaises(SystemExit): + gcu_main.main() + + def test_replace_missing_file_exits(self): + with mock.patch('sys.argv', ['gcu', 'replace', '/nonexistent/cfg.json']): + with self.assertRaises(SystemExit): + gcu_main.main() + + def test_apply_patch_existing_file_dispatched(self, ): + with mock.patch('generic_config_updater.main.apply_patch') as mock_fn: + with mock.patch('os.path.exists', return_value=True): + with mock.patch('sys.argv', ['gcu', 'apply-patch', '/fake/patch.json']): + gcu_main.main() + mock_fn.assert_called_once() + + def test_replace_existing_file_dispatched(self): + with mock.patch('generic_config_updater.main.replace_config') as mock_fn: + with mock.patch('os.path.exists', return_value=True): + with mock.patch('sys.argv', ['gcu', 'replace', '/fake/cfg.json']): + gcu_main.main() + mock_fn.assert_called_once() + + +if __name__ == '__main__': + unittest.main() diff --git a/utilities_common/constants.py b/utilities_common/constants.py index 25858b785..a0ef0a391 100644 --- a/utilities_common/constants.py +++ b/utilities_common/constants.py @@ -1,6 +1,10 @@ #All the constant used in sonic-utilities DEFAULT_NAMESPACE = '' + +# NOTE: A duplicate of this list exists in generic_config_updater/field_operation_validators.py +# (kept separate to avoid a utilities_common dependency in the GCU wheel). +# If you update this list, update that copy too. DEFAULT_SUPPORTED_FECS_LIST = [ 'rs', 'fc', 'none', 'auto'] DISPLAY_ALL = 'all' DISPLAY_EXTERNAL = 'frontend' From 015eacbd52d67ed010d9c03cd0a72e237f8e6601 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Wed, 29 Apr 2026 03:21:16 -0400 Subject: [PATCH 2/8] [generic_config_updater] Cache loadData() calls to reduce redundant YANG parsing (#4476) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two caches in this PR target different layers: 1. _currently_loaded_hash in SonicYangCfg.loadData() β€” skips re-parsing when the same config (by content hash) is loaded consecutively. This helps when multiple validators call loadData() with identical config within a single move validation. 2. _validate_config_cache in ConfigWrapper.validate_config_db() β€” caches the validation result for a given config hash, so if the same config state is validated again later, it returns the cached pass/fail without calling loadData() at all. Per-operation analysis Operation Helps? Why REMOVE (individual) ❌ No Each DFS step removes one item β†’ unique config at each step. Neither cache hits because every state is different. ADD ⚠️ Marginal Typically 1 move β†’ few loadData calls total. Cache might save 1 call if FullConfigMoveValidator and NoDependencyMoveValidator validate the same state. REPLACE (scalar) ⚠️ Marginal Same as ADD β€” few moves, small absolute savings. REMOVE (batched via #4478) βœ… Yes #4478 collapses N individual REMOVEs into 1 bulk REPLACE move. That single move still triggers multiple validator calls with the same config. Cache deduplicates those, reducing loads/move from ~10.6x to ~7.7 --------- Signed-off-by: Rithvick Reddy Munagala (cherry picked from commit 5d54e441ad9a7881ca4182a56e77f45a8e44300b) --- generic_config_updater/gu_common.py | 39 ++++++++- generic_config_updater/patch_sorter.py | 14 +-- .../generic_config_updater/gu_common_test.py | 86 +++++++++++++++++++ .../patch_sorter_test.py | 48 +++++++++++ 4 files changed, 176 insertions(+), 11 deletions(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 0c209a563..cc6bb7ed4 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 @@ -147,14 +160,22 @@ def validate_config_db_config(self, config_db_as_json): try: # Loading data automatically does full validation sy.loadData(config_db_as_json) + 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): """ @@ -525,7 +546,17 @@ def find_ref_paths(self, paths, config, reload_config: bool = True): sy = self._create_sonic_yang_with_loaded_models() if reload_config: - 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 2895c1982..3641339d7 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -1019,15 +1019,15 @@ def _validate_replace(self, move, diff, simulated_config): """ deleted_paths, added_paths = self._get_paths(diff.current_config, simulated_config, []) - # Note: on replace operations we are loading both current and simulated configs so we have to - # load twice :( - - # For deleted paths, we check the current config has no dependencies between nodes under the removed path - if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True): + # Validate added_paths against simulated_config first: FullConfigMoveValidator has + # already loaded simulated_config into the sy singleton (via validate_config_db_config), + # so _currently_loaded_hash will match and find_ref_paths skips loadData. + # Then validate deleted_paths against current_config (requires a fresh loadData). + # This ordering gives 2 loadData calls instead of 3 for REPLACE operations. + if not self._validate_paths_config(added_paths, simulated_config, reload_config=True): return False - # For added paths, we check the simulated config has no dependencies between nodes under the added path - if not self._validate_paths_config(added_paths, simulated_config, reload_config=True): + if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True): return False return True diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index a2df2a148..fb26c5261 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -221,6 +221,92 @@ def test_validate_config_db_config__invalid_config__returns_false(self): self.assertEqual(expected, actual) self.assertIsNotNone(error) + def test_validate_config_db_config__same_config_called_twice__loadData_called_once(self): + """Cache hit: second call with same config must not call loadData again.""" + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + mock_sy.loadYangModel = MagicMock() + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + config = {"ACL_TABLE": {}} + + config_wrapper.validate_config_db_config(config) + config_wrapper.validate_config_db_config(config) + + # loadData should only be called once β€” second call hits the result cache + mock_sy.loadData.assert_called_once() + + def test_validate_config_db_config__different_configs__loadData_called_each_time(self): + """Cache miss: different configs must each call loadData.""" + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + config_a = {"ACL_TABLE": {"rule1": {}}} + config_b = {"ACL_TABLE": {"rule2": {}}} + + config_wrapper.validate_config_db_config(config_a) + config_wrapper.validate_config_db_config(config_b) + + self.assertEqual(2, mock_sy.loadData.call_count) + + def test_find_ref_paths__after_validate_same_config__loadData_skipped(self): + """ + Core optimization: if validate_config_db_config already loaded config into sy, + find_ref_paths with the same config must skip loadData entirely. + """ + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + mock_sy.root = MagicMock() + mock_sy.root.find_path = MagicMock(return_value=MagicMock(data=MagicMock(return_value=[]))) + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + path_addressing = gu_common.PathAddressing(config_wrapper) + config = {"ACL_TABLE": {}} + + # First: validate loads the config into sy and sets _currently_loaded_hash + config_wrapper.validate_config_db_config(config) + self.assertIsNotNone(config_wrapper._currently_loaded_hash, + "validate_config_db_config should have set _currently_loaded_hash") + load_count_after_validate = mock_sy.loadData.call_count + + # Second: find_ref_paths with same config should NOT call loadData again + path_addressing.find_ref_paths("/ACL_TABLE", config, reload_config=True) + load_count_after_find = mock_sy.loadData.call_count + + self.assertEqual(load_count_after_validate, load_count_after_find, + "find_ref_paths should skip loadData when validate already loaded same config") + + def test_find_ref_paths__after_validate_different_config__loadData_called(self): + """ + Cache miss in find_ref_paths: if validate loaded config_A, calling find_ref_paths + with config_B must still call loadData. + """ + config_wrapper = gu_common.ConfigWrapper() + mock_sy = MagicMock() + mock_sy.loadData = MagicMock() + mock_sy.root = MagicMock() + mock_sy.root.find_path = MagicMock(return_value=MagicMock(data=MagicMock(return_value=[]))) + config_wrapper.sonic_yang_with_loaded_models = mock_sy + + path_addressing = gu_common.PathAddressing(config_wrapper) + config_a = {"ACL_TABLE": {"rule1": {}}} + config_b = {"ACL_TABLE": {"rule2": {}}} + + config_wrapper.validate_config_db_config(config_a) + self.assertIsNotNone(config_wrapper._currently_loaded_hash, + "validate_config_db_config should have set _currently_loaded_hash") + load_count_after_validate = mock_sy.loadData.call_count + + path_addressing.find_ref_paths("/ACL_TABLE", config_b, reload_config=True) + load_count_after_find = mock_sy.loadData.call_count + + self.assertEqual(load_count_after_find, load_count_after_validate + 1, + "find_ref_paths should call loadData exactly once when config differs") + def test_validate_bgp_peer_group__valid_non_intersecting_ip_ranges__returns_true(self): # Arrange config_wrapper = gu_common.ConfigWrapper() diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index d33cf5644..73d9082f4 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1365,6 +1365,54 @@ def test_validate__replace_list_item_different_location_than_target_and_no_deps_ # Act and assert self.assertTrue(self.validator.validate(move, diff, move.apply(diff.current_config))) + def test_validate_replace__calls_find_ref_paths_simulated_config_before_current_config(self): + """ + _validate_replace must check added_paths/simulated_config FIRST, then deleted_paths/current_config. + This ordering lets find_ref_paths reuse the sy singleton already loaded with simulated_config + by FullConfigMoveValidator (via _currently_loaded_hash), saving one loadData call per REPLACE. + """ + config_wrapper = ConfigWrapper() + mock_pa = MagicMock(spec=PathAddressing) + + # Track which config is passed to find_ref_paths in call order + configs_seen = [] + + def track_find_ref_paths(paths, config, reload_config=True): + configs_seen.append(config) + return [] # no refs β†’ validation passes + mock_pa.find_ref_paths = MagicMock(side_effect=track_find_ref_paths) + mock_pa.create_path = PathAddressing.create_path + + validator = ps.NoDependencyMoveValidator(mock_pa, config_wrapper) + + # Patch _get_paths on the validator instance (not on mock_pa β€” _get_paths is a method + # on NoDependencyMoveValidator, not on PathAddressing) + deleted_paths = ["/PORT/Ethernet0"] + added_paths = ["/PORT/Ethernet4"] + validator._get_paths = MagicMock(return_value=(deleted_paths, added_paths)) + + current_config = {"PORT": {"Ethernet0": {"lanes": "0"}}} + simulated_config = {"PORT": {"Ethernet4": {"lanes": "4"}}} + diff = ps.Diff(current_config, simulated_config) + + # Create a move mock with the right attributes, wrapped in a group mock + # that is iterable (validate() does `for move in group:`) + inner_move = MagicMock() + inner_move.op_type = OperationType.REPLACE + inner_move.path = "" + group = MagicMock() + group.__iter__ = MagicMock(return_value=iter([inner_move])) + + validator.validate(group, diff, simulated_config) + + # Assert: simulated_config must be checked first (for added_paths), + # then current_config (for deleted_paths) + self.assertEqual(len(configs_seen), 2, "find_ref_paths should be called exactly twice") + self.assertIs(configs_seen[0], simulated_config, + "First find_ref_paths call must use simulated_config (for added_paths)") + self.assertIs(configs_seen[1], current_config, + "Second find_ref_paths call must use current_config (for deleted_paths)") + def prepare_config(self, config, patch): return patch.apply(config) From 8bedc0a9301e075221af45a31ee7cd2606068ed9 Mon Sep 17 00:00:00 2001 From: rookie-who Date: Tue, 28 Apr 2026 12:09:23 -0700 Subject: [PATCH 3/8] GCU sort: batch leaf-list changes into single REPLACE move (#4478) Add BulkLeafListMoveGenerator that produces a single REPLACE move for leaf-list fields whose items differ between current and target configs, instead of decomposing into N individual REMOVE/ADD moves. This is registered as a non-extendable generator (tried before individual moves in DFS). If validation fails, DFS falls through to per-item moves. Impact: For a 512-port ACL table where half the ports are removed, this reduces ~256 individual moves (each triggering 2 loadData calls at ~1.4s each = ~717s) to 1 move (1 loadData = ~1.4s). Conservative scope: - Only handles leaf-lists (lists of scalars, not lists of dicts) - Only replaces lists that exist in both current and target - Falls through to individual moves if the bulk replace fails validation Signed-off-by: vaibhavhd Co-authored-by: rookie-who (cherry picked from commit bfc67f56837f56a7e0fea1d22a6a653836d6e74a) --- generic_config_updater/patch_sorter.py | 60 +++++++- .../files/patch_sorter_test_success.json | 129 ++++++++++-------- .../patch_sorter_test.py | 88 +++++++++++- 3 files changed, 217 insertions(+), 60 deletions(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 3641339d7..5640e3b2e 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -1266,6 +1266,62 @@ def _get_non_existing_keys_tokens(self, config1, config2, reverse): yield [table, key] +class BulkLeafListMoveGenerator: + """ + A non-extendable generator that produces a single REPLACE move for each + leaf-list field whose items differ between current and target configs. + + Instead of generating N individual REMOVE/ADD moves (one per list item), + this emits one REPLACE of the whole list. The DFS tries non-extendable + generators first, so if the bulk replace validates, we skip N-1 moves + and their associated loadData() calls. + + This is conservative: + - Only handles leaf-lists (lists of scalars, not lists of dicts) + - Only replaces lists that already exist in both current and target + - If this move fails validation, DFS continues to other generators + which produce per-item moves (no explicit fallback mechanism) + """ + def __init__(self, path_addressing): + self.path_addressing = path_addressing + + def generate(self, diff): + for move in self._traverse(diff, diff.current_config, diff.target_config, []): + yield move + + def _traverse(self, diff, current_ptr, target_ptr, tokens): + if not isinstance(current_ptr, dict) or not isinstance(target_ptr, dict): + return + + for key in current_ptr: + if key not in target_ptr: + continue + + tokens.append(key) + current_val = current_ptr[key] + target_val = target_ptr[key] + + if isinstance(current_val, list) and isinstance(target_val, list): + # Only handle leaf-lists (lists of scalars) + if (current_val != target_val and + self._is_leaf_list(current_val) and + self._is_leaf_list(target_val)): + yield JsonMoveGroup( + self.__class__.__name__, + JsonMove(diff, OperationType.REPLACE, list(tokens), list(tokens)), + ) + elif isinstance(current_val, dict) and isinstance(target_val, dict): + for move in self._traverse(diff, current_val, target_val, tokens): + yield move + + tokens.pop() + + @staticmethod + def _is_leaf_list(lst): + """Return True if lst contains only scalars (str, int, float, bool).""" + return all(isinstance(item, (str, int, float, bool)) for item in lst) + + class BulkKeyLevelMoveGenerator: """ Same concept as KeyLevelMoveGenerator, but groups additions and removals of sibling keys. @@ -2114,7 +2170,9 @@ def create(self, algorithm=Algorithm.DFS): move_generators = [RemoveCreateOnlyDependencyMoveGenerator(self.path_addressing), LowLevelMoveGenerator(self.path_addressing)] # TODO: Enable TableLevelMoveGenerator once it is confirmed whole table can be updated at the same time - 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)] 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..896cdbe05 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" + ] } ] ] @@ -867,23 +871,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" + ] } ] ] @@ -1538,15 +1533,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 +1745,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 +2380,8 @@ "value": { "Ethernet0": { "alias": "Eth1", - "description": "Ethernet0 100G link", "lanes": "67", + "description": "Ethernet0 100G link", "speed": "100000" } } @@ -3879,6 +3876,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 +4241,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 +5417,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 +5655,6 @@ "path": "/BGP_NEIGHBOR/fc00::a/admin_status" } ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOW/ports/0" - } - ], - [ - { - "op": "remove", - "path": "/ACL_TABLE/EVERFLOWV6/ports/0" - } - ], [ { "op": "remove", diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 73d9082f4..bb9e61ba7 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -2243,6 +2243,91 @@ def verify_moves(self, ops, moves): moves_ops.extend(move.get_jsonpatch()) self.assertCountEqual(ops, moves_ops) + +class TestBulkLeafListMoveGenerator(unittest.TestCase): + def setUp(self): + path_addressing = PathAddressing() + self.generator = ps.BulkLeafListMoveGenerator(path_addressing) + + def test_generate__leaf_list_items_removed__single_replace_move(self): + """Removing items from a leaf-list should produce one REPLACE move.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4", "Ethernet8"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet8"], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/EVERFLOW/ports", + "value": ["Ethernet8"]}]) + + def test_generate__leaf_list_items_added__single_replace_move(self): + """Adding items to a leaf-list should produce one REPLACE move.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4", "Ethernet8"], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/EVERFLOW/ports", + "value": ["Ethernet0", "Ethernet4", "Ethernet8"]}]) + + def test_generate__leaf_list_unchanged__no_moves(self): + """Identical leaf-lists should produce no moves.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4"], "type": "MIRROR"}}}, + ex_ops=[]) + + def test_generate__non_list_fields_differ__no_moves(self): + """Non-list field changes should not produce moves from this generator.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "L3"}}}, + ex_ops=[]) + + def test_generate__list_of_dicts__no_moves(self): + """Lists of dicts (not leaf-lists) should be skipped.""" + self.verify( + current={"TABLE": {"KEY": {"items": [{"a": 1}, {"b": 2}]}}}, + target={"TABLE": {"KEY": {"items": [{"a": 1}]}}}, + ex_ops=[]) + + def test_generate__list_only_in_current__no_moves(self): + """List exists in current but not target β€” not a REPLACE, skip.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"type": "MIRROR"}}}, + ex_ops=[]) + + def test_generate__list_only_in_target__no_moves(self): + """List exists in target but not current β€” handled by other generators, skip.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"type": "MIRROR", "ports": ["Ethernet0"]}}}, + ex_ops=[]) + + def test_generate__leaf_list_all_items_removed__single_replace_move(self): + """Removing all items from a leaf-list should produce one REPLACE with empty list.""" + self.verify( + current={"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0", "Ethernet4"], "type": "MIRROR"}}}, + target={"ACL_TABLE": {"EVERFLOW": {"ports": [], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/EVERFLOW/ports", "value": []}]) + + def test_generate__multiple_tables_with_leaf_lists__multiple_moves(self): + """Multiple differing leaf-lists should each get a REPLACE move.""" + self.verify( + current={"ACL_TABLE": { + "T1": {"ports": ["Ethernet0", "Ethernet4"], "type": "L3"}, + "T2": {"ports": ["Ethernet8", "Ethernet12"], "type": "MIRROR"}}}, + target={"ACL_TABLE": { + "T1": {"ports": ["Ethernet0"], "type": "L3"}, + "T2": {"ports": ["Ethernet12"], "type": "MIRROR"}}}, + ex_ops=[{"op": "replace", "path": "/ACL_TABLE/T1/ports", "value": ["Ethernet0"]}, + {"op": "replace", "path": "/ACL_TABLE/T2/ports", "value": ["Ethernet12"]}]) + + def verify(self, current, target, ex_ops): + diff = ps.Diff(current, target) + moves = list(self.generator.generate(diff)) + moves_ops = [] + for move in moves: + moves_ops.extend(move.get_jsonpatch()) + self.assertCountEqual(ex_ops, moves_ops) + + class TestLowLevelMoveGenerator(unittest.TestCase): def setUp(self): path_addressing = PathAddressing() @@ -3319,7 +3404,8 @@ def verify(self, algo, algo_class): expected_non_extendable_generators = [ps.BulkKeyLevelMoveGenerator, ps.KeyLevelMoveGenerator, ps.BulkKeyGroupLowLevelMoveGenerator, - ps.BulkLowLevelMoveGenerator] + ps.BulkLowLevelMoveGenerator, + ps.BulkLeafListMoveGenerator] expected_extenders = [ps.RequiredValueMoveExtender, ps.UpperLevelMoveExtender, ps.DeleteInsteadOfReplaceMoveExtender, From a810a59536c23a91fe26158940884e906a82d435 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Wed, 20 May 2026 15:30:22 -0400 Subject: [PATCH 4/8] fix(gcu): initialize SonicDBConfig in gcu-standalone for multi-ASIC support (#4554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gcu-standalone entry point (main()) does not call load_db_config(), causing ConfigDBConnector(namespace='asicN') to fail with: swsscommon.SonicDBException: validateNamespace: Initialize global DB config on multi-ASIC platforms (e.g., Nokia 7250 IXR chassis). Root Cause When config apply-patch runs, the config Click group callback in config/main.py calls load_db_config() before dispatching β€” so the DB config is already initialized. However, gcu-standalone invokes generic_config_updater/main.py:main() directly, bypassing that initialization entirely. Fix Add load_db_config() as the first call in main(). This matches the established pattern used by every other standalone entry point in sonic-utilities (config/main.py, show/main.py, scripts/port2alias, scripts/portconfig, acl_loader/main.py, etc.). load_db_config() is idempotent (guarded by isGlobalInit()/isInit() checks) and a no-op on single-ASIC devices where DB is already initialized. Testing Verified on Nokia 7250 IXR multi-ASIC chassis (2 ASICs: asic0, asic1) gcu-standalone apply-patch with asic0-scoped patch proceeds past DB init (previously crashed) Single-ASIC regression: no behavior change (idempotent no-op) (cherry picked from commit 5734b264e79c2a1e307b7818aa98c89881ab9d9e) --- generic_config_updater/main.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/generic_config_updater/main.py b/generic_config_updater/main.py index 4bf878b76..75cdd2325 100644 --- a/generic_config_updater/main.py +++ b/generic_config_updater/main.py @@ -38,6 +38,7 @@ GenericConfigUpdaterError, ) from sonic_py_common import multi_asic +from utilities_common.general import load_db_config logger = logging.getLogger(__name__) @@ -782,6 +783,8 @@ def build_parser(): def main(): """Main entry point for the gcu-standalone console script.""" + load_db_config() + parser = build_parser() args = parser.parse_args() From 21ddca6af4564e2d03d9510d304f1f21d5245f77 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Thu, 28 May 2026 19:53:02 +0000 Subject: [PATCH 5/8] fix: remove --path-trace usage not yet backported to 202412 Upstream PR #4317 (GCU path tracing) is not in this backport scope. Strip the path_trace/trace_io references from apply_patch and remove the 3 corresponding --path-trace test methods. Fixes flake8 F821 undefined name path_trace at config/main.py:1576-1577 introduced by cherry-pick of #4310. --- config/main.py | 7 --- tests/config_test.py | 142 ------------------------------------------- 2 files changed, 149 deletions(-) diff --git a/config/main.py b/config/main.py index ab705b8d2..0806ac26d 100644 --- a/config/main.py +++ b/config/main.py @@ -1571,10 +1571,7 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang print_dry_run_message(dry_run) - trace_io = None try: - if path_trace: - trace_io = open(path_trace, 'w') _gcu_apply_patch_from_file( patch_file_path, config_format_name=format, @@ -1583,7 +1580,6 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang parallel=parallel, ignore_non_yang_tables=ignore_non_yang_tables, ignore_path=ignore_path, - trace_io=trace_io, ) log.log_notice("Patch applied successfully.") @@ -1591,9 +1587,6 @@ def apply_patch(ctx, patch_file_path, format, dry_run, parallel, ignore_non_yang except Exception as ex: click.secho("Failed to apply patch due to: {}".format(ex), fg="red", underline=True, err=True) ctx.fail(ex) - finally: - if trace_io: - trace_io.close() @config.command() @click.argument('target-file-path', type=str, required=True) diff --git a/tests/config_test.py b/tests/config_test.py index 56401d919..323bf8310 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -1942,54 +1942,6 @@ def test_apply_patch__optional_parameters_passed_correctly(self): ["--ignore-path", "/ANY_TABLE"], mock.call(self.any_patch, ConfigFormat.CONFIGDB, False, False, False, ("/ANY_TABLE",))) - @patch('subprocess.Popen', mock.Mock(return_value=mock.Mock( - communicate=mock.Mock(return_value=('{"some": "config"}', None)), - returncode=0 - ))) - @patch('generic_config_updater.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('generic_config_updater.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)), returncode=0 @@ -4504,100 +4456,6 @@ def test_delete_checkpoint_multiasic(self): returncode=0 ))) @patch('generic_config_updater.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('generic_config_updater.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") os.environ['UTILITIES_UNIT_TESTING'] = "0" From 47e51dafb9b55ac8b78cb83e34a34a01ce9993d3 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Tue, 2 Jun 2026 00:22:03 +0000 Subject: [PATCH 6/8] fix(gcu): complete --path-trace removal started in 21ddca6a Commit 21ddca6a removed --path-trace from the outer 'config apply-patch' click command but left trace_io plumbing in generic_config_updater/main.py (the gcu-standalone entry point) and the corresponding -t/--path-trace argparse option. With GenericUpdater.apply_patch (the receiver in generic_updater.py) not accepting trace_io, every 'config apply-patch' invocation on 202412 fails: TypeError: GenericUpdater.apply_patch() got an unexpected keyword argument 'trace_io' Discovered during on-image validation on KVM SONiC-OS-20241212.58. This commit removes: - trace_io kwarg from apply_patch_for_scope / apply_patch_from_file - trace_io kwarg from the GenericUpdater().apply_patch() call - trace_file open/close logic in the apply_patch CLI handler - -t/--path-trace argparse option on gcu-standalone apply-patch - Stale trace_io docstring entry The --path-trace observability feature depends on upstream PR #4317 which is intentionally out of scope for this 202412 backport (whose goal is GCU sidecar container delivery for MRC isolation). A separate followup backport can introduce path-trace cleanly when needed. --- generic_config_updater/main.py | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/generic_config_updater/main.py b/generic_config_updater/main.py index 75cdd2325..20c9bb2f1 100644 --- a/generic_config_updater/main.py +++ b/generic_config_updater/main.py @@ -252,8 +252,7 @@ def validate_patch(patch_ops, all_running_config): def apply_patch_for_scope(scope_changes, results, config_format, verbose, dry_run, - ignore_non_yang_tables, ignore_path, - trace_io=None): + ignore_non_yang_tables, ignore_path): """Apply a patch for a single ASIC scope and record the outcome in *results* (a shared dict).""" scope, changes = scope_changes @@ -275,7 +274,6 @@ def apply_patch_for_scope(scope_changes, results, config_format, dry_run, ignore_non_yang_tables, ignore_path, - trace_io=trace_io, ) results[scope_for_log] = {"success": True, "message": "Success"} logger.info("apply-patch succeeded for %s", scope_for_log) @@ -295,7 +293,7 @@ def _apply_patch_wrapper(args): def apply_patch_from_file(patch_file_path, config_format_name, verbose, dry_run, parallel, ignore_non_yang_tables, - ignore_path, preprocess=True, trace_io=None): + ignore_path, preprocess=True): """Read a JSON-Patch file and apply it β€” the single implementation used by all entry points. @@ -316,9 +314,6 @@ def apply_patch_from_file(patch_file_path, config_format_name, verbose, ``append_emptytables_if_required``, ``filter_duplicate_patch_operations`` and ``validate_patch``. Callers that already performed these steps (or intentionally want to skip them) can pass *False*. - trace_io : IO, optional - Writable file-like object for writing the patch-sorter decision path - trace as JSON. ``None`` (default) disables tracing. Raises ------ @@ -374,7 +369,7 @@ def apply_patch_from_file(patch_file_path, config_format_name, verbose, with concurrent.futures.ThreadPoolExecutor() as executor: arguments = [ (sc, results, config_format, verbose, dry_run, - ignore_non_yang_tables, ignore_path, trace_io) + ignore_non_yang_tables, ignore_path) for sc in changes_by_scope.items() ] futures = [ @@ -387,7 +382,6 @@ def apply_patch_from_file(patch_file_path, config_format_name, verbose, apply_patch_for_scope( scope_changes, results, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_path, - trace_io, ) # 5. Aggregate results @@ -509,7 +503,6 @@ def list_checkpoints(args): def apply_patch(args): """Apply a configuration patch β€” delegates to apply_patch_from_file.""" - trace_file = None try: if args.verbose: print(f"Applying patch from: {args.patch_file}") @@ -517,9 +510,6 @@ def apply_patch(args): if args.dry_run: print("** DRY RUN EXECUTION **") - if getattr(args, 'path_trace', None): - trace_file = open(args.path_trace, 'w') - apply_patch_from_file( patch_file_path=args.patch_file, config_format_name=args.format, @@ -529,16 +519,12 @@ def apply_patch(args): ignore_non_yang_tables=args.ignore_non_yang_tables, ignore_path=args.ignore_path, preprocess=True, - trace_io=trace_file, ) print_success("Patch applied successfully.") except Exception as ex: print_error(f"Failed to apply patch: {ex}") sys.exit(1) - finally: - if trace_file: - trace_file.close() def replace_config(args): @@ -708,12 +694,6 @@ def build_parser(): help='Ignore validation for config specified by given path ' '(JsonPointer)', ) - p.add_argument( - '-t', '--path-trace', - metavar='FILE', - default=None, - help='Filename to write decision path trace for patch generation as JSON', - ) # ---- replace ---- p = subparsers.add_parser( From 320afd98f1ea8511cb3a1ba893183b2afa5da47f Mon Sep 17 00:00:00 2001 From: rimunagala Date: Tue, 2 Jun 2026 14:11:01 +0000 Subject: [PATCH 7/8] fix(gcu): strip --time from list-checkpoints (upstream #3746 not backported) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #4310's gcu-standalone main.py calls updater.list_checkpoints(args.time, args.verbose) which assumes the 3-arg signature added by upstream PR sonic-net/sonic-utilities#3746 (Enhance list-checkpoints CLI, merged 2025-03-05). #3746 is NOT backported to 202412, so the 202412 receiver still has the original 2-arg signature list_checkpoints(self, verbose), causing: Error: Failed to list checkpoints: GenericUpdater.list_checkpoints() takes 2 positional arguments but 3 were given Same class of incomplete-cherry-pick as the trace_io cleanup in 47e51daf β€” feature plumbing brought in by #4310, but its dependency PR was never backported. Same approach as 21ddca6a (--path-trace strip): drop the unbackported feature surface from gcu-standalone, leave the underlying receiver untouched. - Removes -t/--time argparse option - Simplifies list_checkpoints() output flow (no time-dict branch) - Caller now matches 202412 receiver's 2-arg signature Diagnostic-only loss: --time was UX sugar for last-modified timestamps; operators can still 'stat' checkpoint files directly. MRC apply-patch path unaffected. --- generic_config_updater/main.py | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/generic_config_updater/main.py b/generic_config_updater/main.py index 20c9bb2f1..ce0c1a99f 100644 --- a/generic_config_updater/main.py +++ b/generic_config_updater/main.py @@ -479,23 +479,15 @@ def list_checkpoints(args): """List all available checkpoints.""" try: updater = GenericUpdater() - checkpoints = updater.list_checkpoints(args.time, args.verbose) + checkpoints = updater.list_checkpoints(args.verbose) if not checkpoints: print("No checkpoints found.") return - if args.time and isinstance(checkpoints[0], dict): - print("Available checkpoints:") - for checkpoint in checkpoints: - print( - f" - {checkpoint['name']} " - f"(Last Modified: {checkpoint['time']})" - ) - else: - print("Available checkpoints:") - for checkpoint in checkpoints: - print(f" - {checkpoint}") + print("Available checkpoints:") + for checkpoint in checkpoints: + print(f" - {checkpoint}") except Exception as ex: print_error(f"Failed to list checkpoints: {ex}") sys.exit(1) @@ -654,10 +646,6 @@ def build_parser(): p = subparsers.add_parser( 'list-checkpoints', help='List all available checkpoints', ) - p.add_argument( - '-t', '--time', action='store_true', - help='Include last modified time for each checkpoint', - ) p.add_argument( '-v', '--verbose', action='store_true', help='Print additional details', From abab28a7b951974fa7df269c52e7a4de190dc581 Mon Sep 17 00:00:00 2001 From: rimunagala Date: Tue, 2 Jun 2026 15:56:52 +0000 Subject: [PATCH 8/8] fix(gcu): adapt #4478 BulkLeafListMoveGenerator to 202412 JsonMoveGroup signature Upstream PR #4478 (BulkLeafListMoveGenerator, master commit bfc67f56) was written against masters JsonMoveGroup(generator_name, move) signature introduced by upstream PR #3831 (Generic Configuration Updater performance enhancements, merged 2025-11-03). When #3831 was backported to Azure 202412 as PR #254 (merge 3967604d, merged 2025-11-09), the generator_name diagnostic parameter was deliberately omitted. All 21 existing call sites were adapted to the 1-arg form JsonMoveGroup(move). Our cherry-pick of #4478 (8bedc0a9) added a 22nd call site in BulkLeafListMoveGenerator._traverse but missed the same adaptation, leaving: yield JsonMoveGroup( self.__class__.__name__, JsonMove(diff, OperationType.REPLACE, list(tokens), list(tokens)), ) This fails at runtime the moment any leaf-list REPLACE is sorted (e.g. ACL table ports replace): JsonMoveGroup.__init__() takes from 1 to 2 positional arguments but 3 were given Drop the generator_name argument to match #254s convention. No functional impact: generator_name is unused on 202412 (the field does not exist on the class) and the leaf-list batching optimization itself is preserved. Validated on KVM SONiC-OS-20241212.58: leaf-list REPLACE applies cleanly via a single bulk move. Signed-off-by: Ramana Munagala --- generic_config_updater/patch_sorter.py | 1 - 1 file changed, 1 deletion(-) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 5640e3b2e..75f20849c 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -1307,7 +1307,6 @@ def _traverse(self, diff, current_ptr, target_ptr, tokens): 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):