Skip to content

Commit

Permalink
addressing comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ghooo
ghooo committed Jul 17, 2021
1 parent 7920963 commit c554f6d
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 83 deletions.
67 changes: 20 additions & 47 deletions generic_config_updater/gu_common.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class JsonChange:
It provides a single function to apply the change to a given JSON object.
"""
def __init__(self, patch):
self.patch=patch
self.patch = patch

def apply(self, config):
return self.patch.apply(config)
Expand Down Expand Up @@ -230,7 +230,7 @@ def get_path_tokens(self, path):
# src: https://tools.ietf.org/html/rfc6901
mod_tokens = []
for token in tokens:
mod_token = str(token)
mod_token = token
mod_token = mod_token.replace("~0", "~")
mod_token = mod_token.replace("~1", "/")
mod_tokens.append(mod_token)
Expand Down Expand Up @@ -312,7 +312,7 @@ def _get_xpath_single_quote_str_end(self, start, xpath):
# libyang implements XPATH 1.0 which does not escape single quotes
# libyang src: https://netopeer.liberouter.org/doc/libyang/master/html/howtoxpath.html
# XPATH 1.0 src: https://www.w3.org/TR/1999/REC-xpath-19991116/#NT-Literal
idx=idx+1
idx = idx+1

return idx

Expand All @@ -324,7 +324,7 @@ def _get_xpath_double_quote_str_end(self, start, xpath):
# libyang implements XPATH 1.0 which does not escape double quotes
# libyang src: https://netopeer.liberouter.org/doc/libyang/master/html/howtoxpath.html
# XPATH 1.0 src: https://www.w3.org/TR/1999/REC-xpath-19991116/#NT-Literal
idx=idx+1
idx = idx+1

return idx

Expand Down Expand Up @@ -392,7 +392,7 @@ def _find_leafref_paths(self, path, config):

ref_xpaths = []
for xpath in leaf_xpaths:
ref_xpaths.extend(self._find_leaf_dependencies(sy, xpath))
ref_xpaths.extend(sy.find_data_dependencies(xpath))

ref_paths = []
for ref_xpath in ref_xpaths:
Expand All @@ -402,7 +402,7 @@ def _find_leafref_paths(self, path, config):
return set(ref_paths)

def _get_inner_leaf_xpaths(self, xpath, sy):
if xpath=="/": # Point to Root element which contains all xpaths
if xpath == "/": # Point to Root element which contains all xpaths
nodes = sy.root.tree_for()
else: # Otherwise get all nodes that match xpath
nodes = sy.root.find_path(xpath).data()
Expand All @@ -415,37 +415,7 @@ def _get_inner_leaf_xpaths(self, xpath, sy):

def _is_leaf_node(self, node):
schema = node.schema()
return ly.LYS_LEAF == schema.nodetype() # or ly.LYS_LEAFLIST == schema.nodetype()

# TODO: YANG SONiC lib has this function but with a bug in handling null backlinks.
# It is OK to have null backlinks if there is no backlinks.
def _find_leaf_dependencies(self, sy, data_xpath):
ref_list = []
node = sy.root
try:
data_node = sy._find_data_node(data_xpath)
except Exception as e:
print("find_data_dependencies(): Failed to find data node from xpath: {}".format(data_xapth))
return ref_list

try:
value = str(sy._find_data_node_value(data_xpath))

schema_node = ly.Schema_Node_Leaf(data_node.schema())
backlinks = schema_node.backlinks()
if backlinks is not None and backlinks.number() > 0:
for link in backlinks.schema():
node_set = node.find_path(link.path())
for data_set in node_set.data():
data_set.schema()
casted = data_set.subtype()
if value == casted.value_str():
ref_list.append(data_set.path())
except Exception as e:
raise GenericConfigUpdaterError("Failed to find node or dependencies for \
{}\n{}".format(data_xpath, str(e)))

return ref_list
return ly.LYS_LEAF == schema.nodetype()

def convert_path_to_xpath(self, path, config, sy):
"""
Expand Down Expand Up @@ -500,10 +470,10 @@ def _get_xpath_tokens_from_container(self, model, token_index, path_tokens, conf
def _get_xpath_tokens_from_list(self, model, token_index, path_tokens, config):
list_name = model['@name']

tableKey=path_tokens[token_index]
tableKey = path_tokens[token_index]
listKeys = model['key']['@value']
keyDict = self._extractKey(tableKey, listKeys)
keyTokens=[f"[{key}='{keyDict[key]}']" for key in keyDict]
keyTokens = [f"[{key}='{keyDict[key]}']" for key in keyDict]
item_token = f"{list_name}{''.join(keyTokens)}"

xpath_tokens = [item_token]
Expand All @@ -512,7 +482,7 @@ def _get_xpath_tokens_from_list(self, model, token_index, path_tokens, config):
# Example:
# path: /VLAN/Vlan1000
# xpath: /sonic-vlan:sonic-vlan/VLAN/VLAN_LIST[name='Vlan1000']
if len(path_tokens)-1==token_index:
if len(path_tokens)-1 == token_index:
return xpath_tokens

new_xpath_tokens = self._get_xpath_tokens_from_leaf(model, token_index+1, path_tokens,config[path_tokens[token_index]])
Expand Down Expand Up @@ -544,10 +514,13 @@ def _get_xpath_tokens_from_leaf(self, model, token_index, path_tokens, config):
# Example:
# path: /VLAN/Vlan1000/dhcp_servers
# xpath: /sonic-vlan:sonic-vlan/VLAN/VLAN_LIST[name='Vlan1000']/dhcp_servers
if len(path_tokens)-1==token_index:
if len(path_tokens)-1 == token_index:
return [token]
list_config = config[token]
value=list_config[int(path_tokens[token_index+1])]
value = list_config[int(path_tokens[token_index+1])]
# To get a leaf-list instance with the value 'val'
# /module-name:container/leaf-list[.='val']
# Source: Check examples in https://netopeer.liberouter.org/doc/libyang/master/html/howto_x_path.html
return [f"{token}[.='{value}']"]

raise ValueError("Token not found")
Expand All @@ -571,11 +544,11 @@ def _get_list_model(self, model, token_index, path_tokens):
clist = model.get('list')
# Container contains a single list, just return it
# TODO: check if matching also by name is necessary
if isinstance(clist, dict): # and clist['@name'] == token+"_LIST":
if isinstance(clist, dict):
return clist

if isinstance(clist, list):
configdb_values_str=path_tokens[token_index+1]
configdb_values_str = path_tokens[token_index+1]
# Format: "value1|value2|value|..."
configdb_values = configdb_values_str.split("|")
for list_model in clist:
Expand Down Expand Up @@ -663,13 +636,13 @@ def _get_path_tokens_from_list(self, model, token_index, xpath_tokens, config):
if len(xpath_tokens)-1 == token_index:
return path_tokens

nxt_token = xpath_tokens[token_index+1]
next_token = xpath_tokens[token_index+1]
# if the target node is a key, then it does not have a correspondene to path.
# Just return the current 'key1|key2|..' token as it already refers to the keys
# Example where the target node is 'name' which is a key in VLAN_MEMBER_LIST:
# xpath: /sonic-vlan:sonic-vlan/VLAN_MEMBER/VLAN_MEMBER_LIST[name='Vlan1000'][port='Ethernet8']/name
# path: /VLAN_MEMBER/Vlan1000|Ethernet8
if nxt_token in key_dict:
if next_token in key_dict:
return path_tokens

new_path_tokens = self._get_path_tokens_from_leaf(model, token_index+1, xpath_tokens, config[path_token])
Expand Down Expand Up @@ -730,7 +703,7 @@ def _extract_key_dict(self, list_token):
# the findall groups would be ('name', 'Vlan1000'), ('port', 'Ethernet8')
key_value_pattern = "\[([^=]+)='([^']*)'\]"
matches = re.findall(key_value_pattern, all_key_value)
key_dict={}
key_dict = {}
for item in matches:
key = item[0]
value = item[1]
Expand Down
72 changes: 36 additions & 36 deletions generic_config_updater/patch_sorter.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,19 @@ class JsonMove:
"""
def __init__(self, diff, op_type, current_config_tokens, target_config_tokens=None):
operation = JsonMove._to_jsonpatch_operation(diff, op_type, current_config_tokens, target_config_tokens)
self.patch=jsonpatch.JsonPatch([operation])
self.op_type=operation[OperationWrapper.OP_KEYWORD]
self.path=operation[OperationWrapper.PATH_KEYWORD]
self.value=operation.get(OperationWrapper.VALUE_KEYWORD, None)
self.patch = jsonpatch.JsonPatch([operation])
self.op_type = operation[OperationWrapper.OP_KEYWORD]
self.path = operation[OperationWrapper.PATH_KEYWORD]
self.value = operation.get(OperationWrapper.VALUE_KEYWORD, None)

self.op_type=op_type
self.current_config_tokens=current_config_tokens
self.target_config_tokens=target_config_tokens
self.op_type = op_type
self.current_config_tokens = current_config_tokens
self.target_config_tokens = target_config_tokens

@staticmethod
def _to_jsonpatch_operation(diff, op_type, current_config_tokens, target_config_tokens):
operation_wrapper=OperationWrapper()
path_addressing=PathAddressing()
operation_wrapper = OperationWrapper()
path_addressing = PathAddressing()

if op_type == OperationType.REMOVE:
path = path_addressing.create_path(current_config_tokens)
Expand All @@ -82,7 +82,7 @@ def _to_jsonpatch_operation(diff, op_type, current_config_tokens, target_config_
@staticmethod
def _get_value(config, tokens):
for token in tokens:
config=config[token]
config = config[token]

return copy.deepcopy(config)

Expand Down Expand Up @@ -113,8 +113,8 @@ def _to_jsonpatch_add_operation(diff, current_config_tokens, target_config_token
Instead we convert to:
{"op":"add", "path":"/dict1", "value":{"key11": "value11"}}
"""
operation_wrapper=OperationWrapper()
path_addressing=PathAddressing()
operation_wrapper = OperationWrapper()
path_addressing = PathAddressing()

# if path refers to whole config i.e. no tokens, then just create the operation
if not current_config_tokens:
Expand Down Expand Up @@ -208,7 +208,7 @@ def _to_jsonpatch_add_operation(diff, current_config_tokens, target_config_token
break
if not(was_list) and token not in current_ptr:
break
current_ptr=current_ptr[token]
current_ptr = current_ptr[token]

op_type = OperationType.ADD
new_path = path_addressing.create_path(new_tokens)
Expand All @@ -227,7 +227,7 @@ def from_patch(patch):

@staticmethod
def from_operation(operation):
path_addressing=PathAddressing()
path_addressing = PathAddressing()
op_type = OperationType[operation[OperationWrapper.OP_KEYWORD].upper()]
path = operation[OperationWrapper.PATH_KEYWORD]
if op_type in [OperationType.ADD, OperationType.REPLACE]:
Expand All @@ -242,18 +242,18 @@ def from_operation(operation):
current_config = {}
current_config_ptr = current_config
for token in tokens[:-1]:
target_config_ptr[token]={}
current_config_ptr[token]={}
target_config_ptr=target_config_ptr[token]
current_config_ptr=current_config_ptr[token]
target_config_ptr[token] = {}
current_config_ptr[token] = {}
target_config_ptr = target_config_ptr[token]
current_config_ptr = current_config_ptr[token]

if tokens:
target_config_ptr[tokens[-1]] = value
else:
# whole-config, just use value
target_config = value

current_config_tokens=tokens
current_config_tokens = tokens
if op_type in [OperationType.ADD, OperationType.REPLACE]:
target_config_tokens = tokens
else:
Expand All @@ -279,7 +279,7 @@ def __eq__(self, other):
return False

def __hash__(self):
return hash(self.op_type) ^ hash(self.path) ^ hash(json.dumps(self.value))
return hash((self.op_type, self.path, json.dumps(self.value)))

class MoveWrapper:
def __init__(self, move_generators, move_extenders, move_validators):
Expand Down Expand Up @@ -339,7 +339,7 @@ class FullConfigMoveValidator:
A class to validate that full config is valid according to YANG models after applying the move.
"""
def __init__(self, config_wrapper):
self.config_wrapper=config_wrapper
self.config_wrapper = config_wrapper

def validate(self, move, diff):
simulated_config = move.apply(diff.current_config)
Expand Down Expand Up @@ -376,7 +376,7 @@ class CreateOnlyMoveValidator:
a modified create-only field.
"""
def __init__(self, path_addressing):
self.path_addressing=path_addressing
self.path_addressing = path_addressing

def validate(self, move, diff):
if move.op_type != OperationType.REPLACE:
Expand Down Expand Up @@ -416,10 +416,10 @@ def _value_exist_but_different(self, tokens, current_config_ptr, simulated_confi
if mod_token not in simulated_config_ptr:
return False

current_config_ptr=current_config_ptr[mod_token]
simulated_config_ptr=simulated_config_ptr[mod_token]
current_config_ptr = current_config_ptr[mod_token]
simulated_config_ptr = simulated_config_ptr[mod_token]

return current_config_ptr!=simulated_config_ptr
return current_config_ptr != simulated_config_ptr

class NoDependencyMoveValidator:
"""
Expand All @@ -428,8 +428,8 @@ class NoDependencyMoveValidator:
way dependent configs are never updated together.
"""
def __init__(self, path_addressing, config_wrapper):
self.path_addressing=path_addressing
self.config_wrapper=config_wrapper
self.path_addressing = path_addressing
self.config_wrapper = config_wrapper

def validate(self, move, diff):
operation_type = move.op_type
Expand Down Expand Up @@ -579,7 +579,7 @@ class LowLevelMoveGenerator:
where the path of the move does not have children.
"""
def __init__(self, path_addressing):
self.path_addressing=path_addressing
self.path_addressing = path_addressing
def generate(self, diff):
single_run_generator = SingleRunLowLevelMoveGenerator(diff, self.path_addressing)
for move in single_run_generator.generate():
Expand All @@ -591,7 +591,7 @@ class SingleRunLowLevelMoveGenerator:
"""
def __init__(self, diff, path_addressing):
self.diff = diff
self.path_addressing=path_addressing
self.path_addressing = path_addressing

def generate(self):
current_ptr = self.diff.current_config
Expand Down Expand Up @@ -848,7 +848,7 @@ class DeleteRefsMoveExtender:
A class to extend the given DELETE move by adding DELETE moves to configs referring to the path in the move.
"""
def __init__(self, path_addressing):
self.path_addressing=path_addressing
self.path_addressing = path_addressing

def extend(self, move, diff):
operation_type = move.op_type
Expand Down Expand Up @@ -950,15 +950,15 @@ def rec(self, diff):
return bst_moves

class Algorithm(Enum):
DFS=1
BFS=2
MEMOIZATION=3
DFS = 1
BFS = 2
MEMOIZATION = 3

class SortAlgorithmFactory:
def __init__(self, operation_wrapper, config_wrapper, path_addressing):
self.operation_wrapper=operation_wrapper
self.config_wrapper=config_wrapper
self.path_addressing=path_addressing
self.operation_wrapper = operation_wrapper
self.config_wrapper = config_wrapper
self.path_addressing = path_addressing

def create(self, algorithm=Algorithm.DFS):
move_generators = [LowLevelMoveGenerator(self.path_addressing)]
Expand Down

0 comments on commit c554f6d

Please sign in to comment.