Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GCU] Add RemoveCreateOnlyDependency Validator/Generator #2500

Merged
merged 3 commits into from
Dec 2, 2022
Merged

[GCU] Add RemoveCreateOnlyDependency Validator/Generator #2500

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43c05fb
[GCU] Add LaneReplacement Generator
wen587 Nov 15, 2022
c667eb5
Resolve comment:
wen587 Nov 25, 2022
9048e07
improve validator efficiency. RemoveDependency can be applied to all …
wen587 Nov 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 177 additions & 22 deletions generic_config_updater/patch_sorter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,119 @@ def _get_default_value_from_settings(self, parent_path, field_name):

return None

class CreateOnlyFilter:
def __init__(self, path_addressing):
# TODO: create-only fields are hard-coded for now, it should be moved to YANG model
self.path_addressing = path_addressing
self.patterns = [
["PORT", "*", "lanes"],
["LOOPBACK_INTERFACE", "*", "vrf_name"],
["BGP_NEIGHBOR", "*", "holdtime"],
["BGP_NEIGHBOR", "*", "keepalive"],
["BGP_NEIGHBOR", "*", "name"],
["BGP_NEIGHBOR", "*", "asn"],
["BGP_NEIGHBOR", "*", "local_addr"],
["BGP_NEIGHBOR", "*", "nhopself"],
["BGP_NEIGHBOR", "*", "rrclient"],
["BGP_PEER_RANGE", "*", "*"],
["BGP_MONITORS", "*", "holdtime"],
["BGP_MONITORS", "*", "keepalive"],
["BGP_MONITORS", "*", "name"],
["BGP_MONITORS", "*", "asn"],
["BGP_MONITORS", "*", "local_addr"],
["BGP_MONITORS", "*", "nhopself"],
["BGP_MONITORS", "*", "rrclient"],
["MIRROR_SESSION", "*", "*"],
]

def get_filter(self):
return JsonPointerFilter(self.patterns,
self.path_addressing)

class RemoveCreateOnlyDependencyFilter(CreateOnlyFilter):
def __init__(self, path_addressing):
super(RemoveCreateOnlyDependencyFilter, self).__init__(path_addressing)
self.patterns = [
["PORT", "*", "lanes"],
]

class RemoveCreateOnlyDependencyMoveValidator:
def __init__(self, path_addressing):
self.path_addressing = path_addressing
self.create_only_filter = RemoveCreateOnlyDependencyFilter(path_addressing).get_filter()

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

for path in self.create_only_filter.get_paths(current_config):
tokens = self.path_addressing.get_path_tokens(path)
table_to_check = tokens[0]

if table_to_check not in current_config:
continue

current_members = current_config[table_to_check]
if not current_members:
continue

if table_to_check not in target_config:
continue

target_members = target_config[table_to_check]
if not target_members:
continue

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

for member_name in current_members:
if member_name not in target_members:
continue

if not self._validate_member(tokens, member_name,
current_config, target_config, simulated_config):
return False

return True

def _validate_member(self, tokens, member_name, current_config, target_config, simulated_config):
table_to_check, create_only_field = tokens[0], tokens[-1]

current_field = self._get_create_only_field(
current_config, table_to_check, member_name, create_only_field)
target_field = self._get_create_only_field(
target_config, table_to_check, member_name, create_only_field)

if current_field == target_field:
return True

simulated_member = self.path_addressing.get_from_path(
simulated_config, f"/{table_to_check}/{member_name}")

if simulated_member is None:
return True

if table_to_check == "PORT":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @qiluo-msft , still need to confirm admin_status need to be set with "down" for PORT["lanes"] change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. PORT["lanes"] are created only field. If the user require to change this field, we need to delete port and create new port. It is already a disruptive change, set admin_status to down will make it super safe on all platform, and does not make things worse.

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

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

return True

def _get_create_only_field(self, config, table_to_check,
member_name, create_only_field):
return config[table_to_check][member_name].get(create_only_field, None)

class DeleteWholeConfigMoveValidator:
"""
A class to validate not deleting whole config as it is not supported by JsonPatch lib.
Expand Down Expand Up @@ -576,27 +689,7 @@ def __init__(self, path_addressing):
self.path_addressing = path_addressing

# TODO: create-only fields are hard-coded for now, it should be moved to YANG models
self.create_only_filter = JsonPointerFilter([
["PORT", "*", "lanes"],
["LOOPBACK_INTERFACE", "*", "vrf_name"],
["BGP_NEIGHBOR", "*", "holdtime"],
["BGP_NEIGHBOR", "*", "keepalive"],
["BGP_NEIGHBOR", "*", "name"],
["BGP_NEIGHBOR", "*", "asn"],
["BGP_NEIGHBOR", "*", "local_addr"],
["BGP_NEIGHBOR", "*", "nhopself"],
["BGP_NEIGHBOR", "*", "rrclient"],
["BGP_PEER_RANGE", "*", "*"],
["BGP_MONITORS", "*", "holdtime"],
["BGP_MONITORS", "*", "keepalive"],
["BGP_MONITORS", "*", "name"],
["BGP_MONITORS", "*", "asn"],
["BGP_MONITORS", "*", "local_addr"],
["BGP_MONITORS", "*", "nhopself"],
["BGP_MONITORS", "*", "rrclient"],
["MIRROR_SESSION", "*", "*"],
],
path_addressing)
self.create_only_filter = CreateOnlyFilter(path_addressing).get_filter()

def validate(self, move, diff):
simulated_config = move.apply(diff.current_config)
Expand Down Expand Up @@ -921,6 +1014,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 is 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 +1095,62 @@ def generate(self, diff):
for move in single_run_generator.generate():
yield move

class RemoveCreateOnlyDependencyMoveGenerator:
def __init__(self, path_addressing):
self.path_addressing = path_addressing
self.create_only_filter = RemoveCreateOnlyDependencyFilter(path_addressing).get_filter()

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

processed_tables = set()
for path in self.create_only_filter.get_paths(current_config):
tokens = self.path_addressing.get_path_tokens(path)
table_to_check, create_only_field = tokens[0], tokens[-1]

if table_to_check in processed_tables:
continue
else:
processed_tables.add(table_to_check)

if table_to_check not in current_config:
continue

current_members = current_config[table_to_check]
if not current_members:
continue

if table_to_check not in target_config:
continue

target_members = target_config[table_to_check]
if not target_members:
continue

for member_name in current_members:
if member_name not in target_members:
continue

current_field = self._get_create_only_field(
current_config, table_to_check, member_name, create_only_field)
target_field = self._get_create_only_field(
target_config, table_to_check, member_name, create_only_field)

if current_field == target_field:
continue

member_path = f"/{table_to_check}/{member_name}"

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

def _get_create_only_field(self, config, table_to_check,
member_name, create_only_field):
return config[table_to_check][member_name].get(create_only_field, None)


class SingleRunLowLevelMoveGenerator:
"""
A class that can only run once to assist LowLevelMoveGenerator with generating the moves.
Expand Down Expand Up @@ -1271,6 +1422,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 is 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 +1626,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 = [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 = [KeyLevelMoveGenerator()]
move_extenders = [RequiredValueMoveExtender(self.path_addressing, self.operation_wrapper),
Expand All @@ -1485,6 +1639,7 @@ def create(self, algorithm=Algorithm.DFS):
NoDependencyMoveValidator(self.path_addressing, self.config_wrapper),
CreateOnlyMoveValidator(self.path_addressing),
RequiredValueMoveValidator(self.path_addressing),
RemoveCreateOnlyDependencyMoveValidator(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
Open in desktop
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"
}
}
}
Loading