Skip to content

Commit

Permalink
fixing required value validator, and adding replacelane generator
Browse files Browse the repository at this point in the history
  • Loading branch information
@ghooo
ghooo committed Jul 21, 2022
1 parent 1dacb7f commit 63278a8
Show file tree
Hide file tree
Showing 3 changed files with 200 additions and 6 deletions.
109 changes: 106 additions & 3 deletions generic_config_updater/patch_sorter.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,10 +399,10 @@ def _get_paths_recursive(self, config, pattern_tokens, matching_tokens, idx, com
if token == "*":
matching_keys = config.keys()
elif token.startswith("*|"):
suffix = token[2:]
suffix = token[1:] # the suffix will be `|...`
matching_keys = [key for key in config.keys() if key.endswith(suffix)]
elif token.endswith("|*"):
prefix = token[:-2]
prefix = token[:-1] # the prefix will be `...|`
matching_keys = [key for key in config.keys() if key.startswith(prefix)]
elif token in config:
matching_keys = [token]
Expand Down Expand Up @@ -544,6 +544,66 @@ def _get_default_value_from_settings(self, parent_path, field_name):

return None

class LaneReplacementMoveValidator:
def __init__(self, path_addressing):
self.path_addressing = path_addressing

def validate(self, move, diff):
current_config = diff.current_config
target_config = diff.target_config # Final config after applying whole patch

if "PORT" not in current_config:
return True

current_ports = current_config["PORT"]
if not current_ports:
return True

if "PORT" not in target_config:
return True

target_ports = target_config["PORT"]
if not target_ports:
return True

simulated_config = move.apply(current_config) # Config after applying just this move

for port_name in current_ports:
if port_name not in target_ports:
continue

if not self._validate_port(port_name, current_config, target_config, simulated_config):
return False

return True

def _validate_port(self, port_name, current_config, target_config, simulated_config):
current_lanes = self._get_lanes(current_config, port_name)
target_lanes = self._get_lanes(target_config, port_name)

if current_lanes == target_lanes:
return True

simulated_port = self.path_addressing.get_from_path(simulated_config, f"/PORT/{port_name}")

if simulated_port == None:
return True

current_admin_status = self.path_addressing.get_from_path(current_config, f"/PORT/{port_name}/admin_status")
simulated_admin_status = self.path_addressing.get_from_path(simulated_config, f"/PORT/{port_name}/admin_status")
if current_admin_status != simulated_admin_status and current_admin_status != "up":
return False

port_path = f"/PORT/{port_name}"
for ref_path in self.path_addressing.find_ref_paths(port_path, simulated_config):
if not self.path_addressing.has_path(current_config, ref_path):
return False

return True

def _get_lanes(self, config, port_name):
return config["PORT"][port_name].get("lanes", None)

class DeleteWholeConfigMoveValidator:
"""
A class to validate not deleting whole config as it is not supported by JsonPatch lib.
Expand Down Expand Up @@ -921,6 +981,8 @@ def validate(self, move, diff):
for required_path, required_value in data[path]:
current_value = self.identifier.get_value_or_default(current_config, required_path)
simulated_value = self.identifier.get_value_or_default(simulated_config, required_path)
if simulated_value == None: # Simulated config does not have this value at all.
continue
if current_value != simulated_value and simulated_value != required_value:
return False

Expand Down Expand Up @@ -1000,6 +1062,43 @@ def generate(self, diff):
for move in single_run_generator.generate():
yield move

class LaneReplacementMoveGenerator:
def __init__(self, path_addressing):
self.path_addressing = path_addressing

def generate(self, diff):
current_config = diff.current_config
target_config = diff.target_config # Final config after applying whole patch

current_ports = current_config["PORT"]
if not current_ports:
return

if "PORT" not in target_config:
return

target_ports = target_config["PORT"]
if not target_ports:
return

for port_name in current_ports:
if port_name not in target_ports:
continue

current_lanes = self._get_lanes(current_config, port_name)
target_lanes = self._get_lanes(target_config, port_name)

if current_lanes == target_lanes:
continue

port_path = f"/PORT/{port_name}"

for ref_path in self.path_addressing.find_ref_paths(port_path, current_config):
yield JsonMove(diff, OperationType.REMOVE, self.path_addressing.get_path_tokens(ref_path))

def _get_lanes(self, config, port_name):
return config["PORT"][port_name].get("lanes", None)

class SingleRunLowLevelMoveGenerator:
"""
A class that can only run once to assist LowLevelMoveGenerator with generating the moves.
Expand Down Expand Up @@ -1271,6 +1370,8 @@ def extend(self, move, diff):
for required_path, required_value in data[path]:
current_value = self.identifier.get_value_or_default(current_config, required_path)
simulated_value = self.identifier.get_value_or_default(simulated_config, required_path)
if simulated_value == None: # Simulated config does not have this value at all.
continue
if current_value != simulated_value and simulated_value != required_value:
flip_path_value_tuples.add((required_path, required_value))

Expand Down Expand Up @@ -1473,7 +1574,8 @@ def __init__(self, operation_wrapper, config_wrapper, path_addressing):
self.path_addressing = path_addressing

def create(self, algorithm=Algorithm.DFS):
move_generators = [LowLevelMoveGenerator(self.path_addressing)]
move_generators = [LaneReplacementMoveGenerator(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 = [KeyLevelMoveGenerator()]
move_extenders = [RequiredValueMoveExtender(self.path_addressing, self.operation_wrapper),
Expand All @@ -1485,6 +1587,7 @@ def create(self, algorithm=Algorithm.DFS):
NoDependencyMoveValidator(self.path_addressing, self.config_wrapper),
CreateOnlyMoveValidator(self.path_addressing),
RequiredValueMoveValidator(self.path_addressing),
LaneReplacementMoveValidator(self.path_addressing),
NoEmptyTableMoveValidator(self.path_addressing)]

move_wrapper = MoveWrapper(move_generators, move_non_extendable_generators, move_extenders, move_validators)
Expand Down
20 changes: 20 additions & 0 deletions tests/generic_config_updater/files/config_db_with_port_critical.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,23 @@
"lanes": "41,42,43,44",
"pfc_asym": "off",
"speed": "40000"
},
"Ethernet28": {
"alias": "fortyGigE0/28",
"description": "Servers5:eth0",
"index": "6",
"lanes": "53,54,55,56",
"pfc_asym": "off",
"speed": "40000"
},
"Ethernet32": {
"alias": "fortyGigE0/32",
"description": "Servers6:eth0",
"index": "7",
"lanes": "57,58,59,60",
"mtu": "9100",
"pfc_asym": "off",
"speed": "40000"
}
},
"BUFFER_PG": {
Expand All @@ -44,6 +61,9 @@
},
"Ethernet12|0": {
"profile": "ingress_lossy_profile"
},
"Ethernet28|0": {
"profile": "ingress_lossy_profile"
}
}
}
77 changes: 74 additions & 3 deletions tests/generic_config_updater/patch_sorter_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,41 @@ def test_simulate__applies_move(self):
# Assert
self.assertIs(self.any_diff, actual)

class TestJsonPointerFilter(unittest.TestCase):
def test_get_paths__common_prefix__exact_match_returned(self):
config = {
"BUFFER_PG": {
"Ethernet1|0": {},
"Ethernet12|0": {},
"Ethernet120|0": {}, # 'Ethernet12' is a common prefix with the previous line
},
}

filter = ps.JsonPointerFilter([["BUFFER_PG", "Ethernet12|*"]], PathAddressing())

expected_paths = ["/BUFFER_PG/Ethernet12|0"]

actual_paths = list(filter.get_paths(config))

self.assertCountEqual(expected_paths, actual_paths)

def test_get_paths__common_suffix__exact_match_returned(self):
config = {
"QUEUE": {
"Ethernet1|0": {},
"Ethernet1|10": {},
"Ethernet1|110": {}, # 10 is a common suffix with the previous line
},
}

filter = ps.JsonPointerFilter([["QUEUE", "*|10"]], PathAddressing())

expected_paths = ["/QUEUE/Ethernet1|10"]

actual_paths = list(filter.get_paths(config))

self.assertCountEqual(expected_paths, actual_paths)

class TestRequiredValueIdentifier(unittest.TestCase):
def test_hard_coded_required_value_data(self):
identifier = ps.RequiredValueIdentifier(PathAddressing())
Expand Down Expand Up @@ -1849,6 +1884,22 @@ def _get_critical_port_change_test_cases(self):
}
})
},
# Additional cases where the full port is getting deleted
# If port is getting deleted, there is no point in checking if there are critical changes depending on it
"PORT_UP__STATUS_CHANGING__UNDER_PORT__PORT_EXIST__PORT_DELETION": {
"expected": True,
"config": Files.CONFIG_DB_WITH_PORT_CRITICAL,
"move": ps.JsonMove.from_operation({ "op": "remove", "path": "/PORT/Ethernet32" })
},
"PORT_UP__STATUS_CHANGING__NOT_UNDER_PORT__PORT_EXIST__PORT_DELETION": {
"expected": True,
"config": Files.CONFIG_DB_WITH_PORT_CRITICAL,
"target_config": self._apply_operations(Files.CONFIG_DB_WITH_PORT_CRITICAL, [
{ "op": "remove", "path": "/PORT/Ethernet28" },
{ "op": "remove", "path": "/BUFFER_PG/Ethernet28|0" },
]),
"move": ps.JsonMove.from_operation({ "op": "remove", "path": "/PORT/Ethernet28" })
},
}

def test_validate__no_critical_port_changes(self):
Expand Down Expand Up @@ -1922,7 +1973,7 @@ def test_generate__all_tables_exist__no_moves(self):
self.verify(current = {"ExistingTable1": { "Key1": "Value1" }, "ExistingTable2": {}},
target = {"ExistingTable1": {}, "ExistingTable2": { "Key2": "Value2" }},
ex_ops = [])

def test_generate__multiple_cases__deletion_precedes_addition(self):
self.verify(current = {"CommonTable": { "Key1": "Value1" }, "CurrentTable": {}},
target = {"CommonTable": {}, "TargetTable": { "Key2": "Value2" }},
Expand Down Expand Up @@ -2390,7 +2441,7 @@ def test_extend__multi_port_turn_up_and_critical_move__multi_flip_to_turn_down(s
{"op":"add", "path":"/PORT/Ethernet16/admin_status", "value":"up"},
{"op":"add", "path":"/PORT/Ethernet12/mtu", "value":"9000"},
# Will not be part of the move, only in the final target config
{"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"},
{"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"},
])
move = ps.JsonMove.from_operation({
"op":"replace",
Expand Down Expand Up @@ -2444,7 +2495,7 @@ def test_extend__multiple_changes__multiple_extend_moves(self):
{"op":"add", "path":"/PORT/Ethernet16/admin_status", "value":"up"},
{"op":"add", "path":"/PORT/Ethernet12/mtu", "value":"9000"},
# Will not be part of the move, only in the final target config
{"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"},
{"op":"add", "path":"/PORT/Ethernet16/mtu", "value":"9000"},
])
move = ps.JsonMove.from_operation({
"op":"replace",
Expand Down Expand Up @@ -2508,6 +2559,26 @@ def test_extend__multiple_changes__multiple_extend_moves(self):
# Assert
self._verify_moves(expected, actual)

def test_extend__port_deletion__no_extension(self):
# Arrange
move = ps.JsonMove.from_operation({
"op":"remove",
"path":"/PORT/Ethernet28"
})
current_config = Files.CONFIG_DB_WITH_PORT_CRITICAL
target_config = self._apply_operations(Files.CONFIG_DB_WITH_PORT_CRITICAL, [
{ "op": "remove", "path": "/PORT/Ethernet28" },
{ "op": "remove", "path": "/BUFFER_PG/Ethernet28|0" }
])
diff = ps.Diff(current_config, target_config)
expected = []

# Act
actual = self.extender.extend(move, diff)

# Assert
self._verify_moves(expected, actual)

def _verify_moves(self, ex_ops, moves):
moves_ops = [list(move.patch)[0] for move in moves]
self.assertCountEqual(ex_ops, moves_ops)
Expand Down

0 comments on commit 63278a8

Please sign in to comment.