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] Mark children of bgp_neighbor as create-only #2008

Merged
merged 1 commit into from
Jan 21, 2022
Merged

[GCU] Mark children of bgp_neighbor as create-only #2008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 36 additions & 15 deletions generic_config_updater/patch_sorter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,11 +393,23 @@ def __init__(self, path_addressing):
self.create_only_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"],
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

BGP_NEIGHBOR

Also applied to BGP_INTERNAL_NEIGHBOR, BGP_MONITORS, BGP_PEER_RANGE, BGP_VOQ_CHASSIS_NEIGHBOR ? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I do this in a separate PR? I need to close the current issue.

#2029

]

def validate(self, move, diff):
simulated_config = move.apply(diff.current_config)
paths = set(list(self._get_create_only_paths(diff.current_config)) + list(self._get_create_only_paths(simulated_config)))
# get create-only paths from current config, simulated config and also target config
# simulated config is the result of the move
# target config is the final config
paths = set(list(self._get_create_only_paths(diff.current_config)) +
list(self._get_create_only_paths(simulated_config)) +
list(self._get_create_only_paths(diff.target_config)))

for path in paths:
tokens = self.path_addressing.get_path_tokens(path)
Expand All @@ -408,8 +420,28 @@ def validate(self, move, diff):
if self._value_removed_but_parent_remain(tokens, diff.current_config, simulated_config):
return False

# if parent of create-only field is added, create-only field should be the same as target
# i.e. if field is deleted in target, it should be deleted in the move, or
# if field is present in target, it should be present in the move
if self._parent_added_child_not_as_target(tokens, diff.current_config, simulated_config, diff.target_config):
return False

return True

def _parent_added_child_not_as_target(self, tokens, current_config, simulated_config, target_config):
# if parent is not added, return false
if not self._exist_only_in_first(tokens[:-1], simulated_config, current_config):
return False

child_path = self.path_addressing.create_path(tokens)

# if child is in target, check if child is not in simulated
if self.path_addressing.has_path(target_config, child_path):
return not self.path_addressing.has_path(simulated_config, child_path)
else:
# if child is not in target, check if child is in simulated
return self.path_addressing.has_path(simulated_config, child_path)

def _get_create_only_paths(self, config):
for pattern in self.create_only_patterns:
for create_only_path in self._get_create_only_path_recursive(config, pattern, [], 0):
Expand Down Expand Up @@ -472,20 +504,9 @@ def _value_removed_but_parent_remain(self, tokens, current_config_ptr, simulated
return True

def _exist_only_in_first(self, tokens, first_config_ptr, second_config_ptr):
for token in tokens:
mod_token = int(token) if isinstance(first_config_ptr, list) else token

if mod_token not in second_config_ptr:
return True

if mod_token not in first_config_ptr:
return False

first_config_ptr = first_config_ptr[mod_token]
second_config_ptr = second_config_ptr[mod_token]

# tokens exist in both
return False
path = self.path_addressing.create_path(tokens)
return self.path_addressing.has_path(first_config_ptr, path) and \
not self.path_addressing.has_path(second_config_ptr, path)

class NoDependencyMoveValidator:
"""
Expand Down
80 changes: 80 additions & 0 deletions tests/generic_config_updater/files/patch_sorter_test_success.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2894,5 +2894,85 @@
}
]
]
},
"ADDING_BGP_NEIGHBORS": {
"current_config": {
"BGP_NEIGHBOR": {
"10.0.0.57": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.56",
"name": "ARISTA01T1",
"nhopself": "0",
"rrclient": "0"
}
}
},
"patch": [
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.59",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.58",
"name": "ARISTA02T1",
"nhopself": "0",
"rrclient": "0"
}
},
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.61",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.60",
"name": "ARISTA03T1",
"nhopself": "0",
"rrclient": "0"
}
}
],
"expected_changes": [
[
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.59",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.58",
"name": "ARISTA02T1",
"nhopself": "0",
"rrclient": "0"
}
}
],
[
{
"op": "add",
"path": "/BGP_NEIGHBOR/10.0.0.61",
"value": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.60",
"name": "ARISTA03T1",
"nhopself": "0",
"rrclient": "0"
}
}
]
]
}
}
71 changes: 69 additions & 2 deletions tests/generic_config_updater/patch_sorter_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,31 @@ def test_validate__removed_create_only_field_parent_doesnot_remain__success(self
["PORT"],
["PORT"])

def test_validate__parent_added_with_all_create_only_field_that_target_has__success(self):
added_parent_value = {
"admin_status": "up",
"asn": "64600", # <== create-only field
"holdtime": "50", # <== create-only field
}
self.verify_parent_adding(added_parent_value, True)

def test_validate__parent_added_with_create_only_field_but_target_does_not_have_the_field__failure(self):
added_parent_value = {
"admin_status": "up",
"asn": "64600", # <== create-only field
"holdtime": "50", # <== create-only field
"rrclient": "1", # <== create-only field but not in target-config
}
self.verify_parent_adding(added_parent_value, False)

def test_validate__parent_added_without_create_only_field_but_target_have_the_field__failure(self):
added_parent_value = {
"admin_status": "up",
"asn": "64600", # <== create-only field
# Not adding: "holdtime": "50"
}
self.verify_parent_adding(added_parent_value, False)

def test_hard_coded_create_only_paths(self):
config = {
"PORT": {
Expand All @@ -895,17 +920,59 @@ def test_hard_coded_create_only_paths(self):
"Loopback0":{"vrf_name":"vrf0"},
"Loopback1":{},
"Loopback2":{"vrf_name":"vrf1"},
}}
},
"BGP_NEIGHBOR": {
"10.0.0.57": {
"admin_status": "up",
"asn": "64600",
"holdtime": "10",
"keepalive": "3",
"local_addr": "10.0.0.56",
"name": "ARISTA01T1",
"nhopself": "0",
"rrclient": "0"
}
}
}
expected = [
"/PORT/Ethernet0/lanes",
"/PORT/Ethernet2/lanes",
"/LOOPBACK_INTERFACE/Loopback0/vrf_name",
"/LOOPBACK_INTERFACE/Loopback2/vrf_name"
"/LOOPBACK_INTERFACE/Loopback2/vrf_name",
"/BGP_NEIGHBOR/10.0.0.57/asn",
"/BGP_NEIGHBOR/10.0.0.57/holdtime",
"/BGP_NEIGHBOR/10.0.0.57/keepalive",
"/BGP_NEIGHBOR/10.0.0.57/local_addr",
"/BGP_NEIGHBOR/10.0.0.57/name",
"/BGP_NEIGHBOR/10.0.0.57/nhopself",
"/BGP_NEIGHBOR/10.0.0.57/rrclient",
]

actual = self.validator._get_create_only_paths(config)

self.assertCountEqual(expected, actual)

def verify_parent_adding(self, added_parent_value, expected):
current_config = {
"BGP_NEIGHBOR": {}
}

target_config = {
"BGP_NEIGHBOR": {
"10.0.0.57": {
"admin_status": "up",
"asn": "64600",
"holdtime": "50",
}
}
}
diff = ps.Diff(current_config, target_config)
move = ps.JsonMove.from_operation({"op":"add", "path":"/BGP_NEIGHBOR/10.0.0.57", "value": added_parent_value})

actual = self.validator.validate(move, diff)

self.assertEqual(expected, actual)

def verify_diff(self, current_config, target_config, current_config_tokens=None, target_config_tokens=None, expected=True):
# Arrange
current_config_tokens = current_config_tokens if current_config_tokens else []
Expand Down