Skip to content

Commit db85d4b

Browse files
authored
Merge pull request #403 from mssonicbld/sonicbld/202607-merge
```<br>* d636e0e - (HEAD -> 202607) Merge branch '202605' of https://github.com/sonic-net/sonic-utilities into 202607 (2026-07-14) [Sonic Automation] * 351be6e - (origin/202605) fix GCU patch sorter crash on create-only field change referenced by a leaf-list (#4681) (2026-07-13) [mssonicbld]<br>```
2 parents f28b4b1 + d636e0e commit db85d4b

2 files changed

Lines changed: 206 additions & 5 deletions

File tree

generic_config_updater/patch_sorter.py

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,7 @@ class RemoveCreateOnlyDependencyMoveValidator:
748748
def __init__(self, path_addressing):
749749
self.path_addressing = path_addressing
750750
self.create_only_filter = CreateOnlyFilter(path_addressing).get_filter()
751+
self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - RemoveCreateOnly")
751752

752753
def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]:
753754
# Note: group is not used by this validator
@@ -822,7 +823,28 @@ def _validate_member(self, tokens, member_name, current_config, target_config, s
822823
return False
823824

824825
member_path = f"/{table_to_check}/{member_name}"
825-
for ref_path in self.path_addressing.find_ref_paths(member_path, simulated_config, reload_config=reload_config):
826+
try:
827+
ref_paths = self.path_addressing.find_ref_paths(
828+
member_path, simulated_config, reload_config=reload_config)
829+
except (ValueError, KeyError) as e:
830+
# An unresolvable or malformed reference against the simulated intermediate config
831+
# raises here. The motivating case: a create-only field change (e.g. a PORT breakout
832+
# that rewrites lanes) has transiently removed this member from a multi-member leaf-list
833+
# (e.g. ACL_TABLE.ports) while a leafref to it is still present, so the dangling leafref
834+
# fails to resolve (list.index raises ValueError). Other resolution failures in the
835+
# xpath<->configdb-path conversion (schema/key-count mismatches raise ValueError; a table
836+
# with no YANG model raises KeyError) likewise indicate the reference cannot be resolved
837+
# against this intermediate config. In every case the ordering is an invalid intermediate
838+
# move, so reject it and let the sort backtrack rather than aborting the whole sort.
839+
# Scope is deliberately limited to reference-resolution errors: a loadData failure
840+
# (sonic_yang.SonicYangException) is not caught here because FullConfigMoveValidator has
841+
# already loaded this config into the sy singleton, so find_ref_paths skips loadData.
842+
self.logger.log_debug(
843+
f"Rejecting move: reference resolution failed against simulated config "
844+
f"for '{member_path}': {type(e).__name__}: {e}")
845+
return False
846+
847+
for ref_path in ref_paths:
826848
if not self.path_addressing.has_path(current_config, ref_path):
827849
return False
828850

@@ -963,6 +985,7 @@ class NoDependencyMoveValidator:
963985
def __init__(self, path_addressing, config_wrapper):
964986
self.path_addressing = path_addressing
965987
self.config_wrapper = config_wrapper
988+
self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - NoDependency")
966989

967990
def validate(self, group: JsonMoveGroup, diff, simulated_config) -> Tuple[bool, Optional[str]]:
968991
reload_config = True
@@ -979,7 +1002,8 @@ def __validate_move(self, move, diff, simulated_config, reload_config: bool = Tr
9791002

9801003
if operation_type == OperationType.ADD:
9811004
# For add operation, we check the simulated config has no dependencies between nodes under the added path
982-
if not self._validate_paths_config([path], simulated_config, reload_config):
1005+
if not self._validate_paths_config([path], simulated_config, reload_config,
1006+
reject_on_unresolvable_ref=True):
9831007
return False
9841008
elif operation_type == OperationType.REMOVE:
9851009
# For remove operation, we check the current config has no dependencies between nodes under the removed path
@@ -1030,7 +1054,8 @@ def _validate_replace(self, move, diff, simulated_config):
10301054
# so _currently_loaded_hash will match and find_ref_paths skips loadData.
10311055
# Then validate deleted_paths against current_config (requires a fresh loadData).
10321056
# This ordering gives 2 loadData calls instead of 3 for REPLACE operations.
1033-
if not self._validate_paths_config(added_paths, simulated_config, reload_config=True):
1057+
if not self._validate_paths_config(added_paths, simulated_config, reload_config=True,
1058+
reject_on_unresolvable_ref=True):
10341059
return False
10351060

10361061
if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True):
@@ -1100,11 +1125,31 @@ def _get_list_paths(self, current_list, target_list, tokens):
11001125

11011126
return deleted_paths, added_paths
11021127

1103-
def _validate_paths_config(self, paths, config, reload_config: bool = True):
1128+
def _validate_paths_config(self, paths, config, reload_config: bool = True,
1129+
reject_on_unresolvable_ref: bool = False):
11041130
"""
11051131
validates all config under paths do not have config and its references
1132+
1133+
reject_on_unresolvable_ref: set only when 'config' is the transient simulated intermediate
1134+
state. A dangling leafref in that state (e.g. a create-only PORT change that has transiently
1135+
removed the port from a leaf-list such as ACL_TABLE.ports while a reference to it lingers)
1136+
makes find_ref_paths raise ValueError; a table with no YANG model makes it raise KeyError.
1137+
Either way it is an invalid intermediate move, so reject it and let the sort backtrack rather
1138+
than aborting. When 'config' is diff.current_config (a valid committed state) the flag stays
1139+
False: such an error there is genuine and must surface instead of being silently swallowed.
1140+
Scope is limited to reference-resolution errors; a loadData failure
1141+
(sonic_yang.SonicYangException) is not caught because the config is already loaded into the sy
1142+
singleton by FullConfigMoveValidator, so find_ref_paths skips loadData for it.
11061143
"""
1107-
refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config)
1144+
try:
1145+
refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config)
1146+
except (ValueError, KeyError) as e:
1147+
if reject_on_unresolvable_ref:
1148+
self.logger.log_debug(
1149+
f"Rejecting move: reference resolution failed against simulated config "
1150+
f"for {paths}: {type(e).__name__}: {e}")
1151+
return False
1152+
raise
11081153
for ref in refs:
11091154
for path in paths:
11101155
if ref.startswith(path):

tests/generic_config_updater/patch_sorter_test.py

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1246,6 +1246,78 @@ def setUp(self):
12461246
path_addressing = ps.PathAddressing(config_wrapper)
12471247
self.validator = ps.NoDependencyMoveValidator(path_addressing, config_wrapper)
12481248

1249+
def test_validate__unresolvable_ref_in_simulated_config__add_rejected(self):
1250+
# A dangling leafref in the transient simulated intermediate config makes find_ref_paths
1251+
# raise ValueError. For a simulated-config-facing check (ADD here), the validator must reject
1252+
# the move (return False) so the sort backtracks, rather than letting the exception abort the
1253+
# whole sort.
1254+
current_config = {"PORT": {"Ethernet0": {}}}
1255+
target_config = {
1256+
"PORT": {"Ethernet0": {}},
1257+
"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}}
1258+
}
1259+
diff = ps.Diff(current_config, target_config)
1260+
move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"]))
1261+
simulated_config = move.apply(diff.current_config)
1262+
1263+
self.validator.path_addressing.find_ref_paths = Mock(
1264+
side_effect=ValueError("'Ethernet0' is not in list"))
1265+
1266+
# Must not raise; the move is rejected so the DFS can backtrack. Pin the exact arguments so a
1267+
# regression swapping simulated_config <-> diff.current_config in the ADD branch is caught
1268+
# (simulated_config is value-distinct from current_config here: it carries the added ACL_TABLE).
1269+
self.assertFalse(self.validator.validate(move, diff, simulated_config)[0])
1270+
self.validator.path_addressing.find_ref_paths.assert_called_once_with(
1271+
["/ACL_TABLE"], simulated_config, reload_config=True)
1272+
1273+
def test_validate__unresolvable_ref_in_simulated_config__replace_rejected(self):
1274+
# REPLACE is the operation type for a PORT breakout (rewriting lanes while an ACL reference
1275+
# lingers). _validate_replace validates added_paths against the simulated intermediate config,
1276+
# so an unresolvable reference there (KeyError here, e.g. a table with no YANG model) must
1277+
# reject the move rather than aborting the sort.
1278+
current_config = {"PORT": {"Ethernet0": {}}}
1279+
target_config = {
1280+
"PORT": {"Ethernet0": {}},
1281+
"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}}
1282+
}
1283+
diff = ps.Diff(current_config, target_config)
1284+
move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REPLACE, [], []))
1285+
simulated_config = move.apply(diff.current_config)
1286+
1287+
self.validator.path_addressing.find_ref_paths = Mock(
1288+
side_effect=KeyError("ACL_TABLE"))
1289+
1290+
# Must not raise; the move is rejected so the DFS can backtrack. Pin the config argument so a
1291+
# regression routing REPLACE added_paths through diff.current_config instead of the simulated
1292+
# config is caught (the two configs are value-distinct: simulated carries the added ACL_TABLE).
1293+
self.assertFalse(self.validator.validate(move, diff, simulated_config)[0])
1294+
self.validator.path_addressing.find_ref_paths.assert_called_once_with(
1295+
["/ACL_TABLE"], simulated_config, reload_config=True)
1296+
1297+
def test_validate__value_error_on_current_config__propagates(self):
1298+
# A check against diff.current_config (a valid committed state) is not simulated, so a
1299+
# ValueError from find_ref_paths signals a genuine schema error and must propagate rather
1300+
# than being silently swallowed as a move rejection.
1301+
current_config = {
1302+
"PORT": {"Ethernet0": {}},
1303+
"ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}}
1304+
}
1305+
target_config = {"PORT": {"Ethernet0": {}}}
1306+
diff = ps.Diff(current_config, target_config)
1307+
move = JsonMoveGroup("", ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"]))
1308+
simulated_config = move.apply(diff.current_config)
1309+
1310+
self.validator.path_addressing.find_ref_paths = Mock(
1311+
side_effect=ValueError("Keys in configDb not matching keys in SonicYang"))
1312+
1313+
with self.assertRaises(ValueError):
1314+
self.validator.validate(move, diff, simulated_config)
1315+
# Pin that the REMOVE validation ran against diff.current_config (unguarded), not
1316+
# simulated_config: the re-raise must be the guaranteed-current-config path, not a lucky
1317+
# raise that a config swap could mask. Configs are value-distinct (current has ACL_TABLE).
1318+
self.validator.path_addressing.find_ref_paths.assert_called_once_with(
1319+
["/ACL_TABLE"], diff.current_config, reload_config=True)
1320+
12491321
def test_validate__add_full_config_has_dependencies__failure(self):
12501322
# Arrange
12511323
# CROPPED_CONFIG_DB_AS_JSON has dependencies between PORT and ACL_TABLE
@@ -2061,6 +2133,90 @@ def test_validate__lane_replacement_change(self):
20612133
with self.subTest(name=test_case_name):
20622134
self._run_single_test(test_cases[test_case_name])
20632135

2136+
def test_validate__unresolvable_ref_in_simulated_config__move_rejected(self):
2137+
# A create-only PORT field change (breakout rewriting lanes) can transiently
2138+
# remove the port from a multi-member leaf-list (e.g. ACL_TABLE.ports) in the simulated
2139+
# intermediate config while a leafref to it is still present. Resolving that dangling
2140+
# leafref raises ValueError ("'<port>' is not in list"). The validator must treat this as
2141+
# an invalid intermediate move (return False) so the sort backtracks, rather than letting
2142+
# the exception propagate and abort the whole sort.
2143+
current_config = {
2144+
"PORT": {
2145+
"Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"}
2146+
},
2147+
"ACL_TABLE": {
2148+
"DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]}
2149+
}
2150+
}
2151+
target_config = {
2152+
"PORT": {
2153+
"Ethernet312": {"lanes": "305", "admin_status": "up"}
2154+
},
2155+
"ACL_TABLE": {
2156+
"DATAACL": {"type": "L3", "ports": ["Ethernet280"]}
2157+
}
2158+
}
2159+
# The transient intermediate config the sorter is validating: Ethernet312's create-only
2160+
# 'lanes' field has already been rewritten toward the target, but the ACL_TABLE leafref to
2161+
# it has not been removed yet, so the reference is momentarily dangling. Resolving it raises
2162+
# ValueError. Kept value-distinct from current_config (different lanes) so the argument
2163+
# assertion below would catch a regression that passed current_config to find_ref_paths.
2164+
simulated_config = {
2165+
"PORT": {
2166+
"Ethernet312": {"lanes": "305", "admin_status": "up"}
2167+
},
2168+
"ACL_TABLE": {
2169+
"DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]}
2170+
}
2171+
}
2172+
2173+
move = JsonMoveGroup("", Mock())
2174+
diff = ps.Diff(current_config, target_config)
2175+
2176+
self.validator.path_addressing.find_ref_paths = Mock(
2177+
side_effect=ValueError("'Ethernet312' is not in list"))
2178+
2179+
# Must not raise; the move is rejected so the DFS can backtrack.
2180+
self.assertFalse(self.validator.validate(move, diff, simulated_config)[0])
2181+
# Pin the assertion to the code path under test: the rejection must come from the guarded
2182+
# find_ref_paths call raising, resolved against the simulated (intermediate) config. Asserting
2183+
# the exact arguments also catches a regression that swapped simulated_config <-> current_config.
2184+
self.validator.path_addressing.find_ref_paths.assert_called_once_with(
2185+
"/PORT/Ethernet312", simulated_config, reload_config=True)
2186+
2187+
def test_validate__keyerror_in_simulated_config__move_rejected(self):
2188+
# find_ref_paths also raises KeyError (not just ValueError) when a referenced path's table
2189+
# has no YANG model in the simulated intermediate config. The guard must treat that the same
2190+
# way as an unresolvable reference: reject the move so the sort backtracks, not abort it.
2191+
current_config = {
2192+
"PORT": {
2193+
"Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"}
2194+
}
2195+
}
2196+
target_config = {
2197+
"PORT": {
2198+
"Ethernet312": {"lanes": "305", "admin_status": "up"}
2199+
}
2200+
}
2201+
# Value-distinct from current_config (lanes already rewritten toward the target) so the
2202+
# argument assertion below catches a regression that passed current_config instead.
2203+
simulated_config = {
2204+
"PORT": {
2205+
"Ethernet312": {"lanes": "305", "admin_status": "up"}
2206+
}
2207+
}
2208+
2209+
move = JsonMoveGroup("", Mock())
2210+
diff = ps.Diff(current_config, target_config)
2211+
2212+
self.validator.path_addressing.find_ref_paths = Mock(
2213+
side_effect=KeyError("ACL_TABLE"))
2214+
2215+
# Must not raise; the move is rejected so the DFS can backtrack.
2216+
self.assertFalse(self.validator.validate(move, diff, simulated_config)[0])
2217+
self.validator.path_addressing.find_ref_paths.assert_called_once_with(
2218+
"/PORT/Ethernet312", simulated_config, reload_config=True)
2219+
20642220
def _run_single_test(self, test_case):
20652221
# Arrange
20662222
expected = test_case['expected']

0 commit comments

Comments
 (0)