[config][generic-update] Implementing patch sorting #1599

ghooo · 2021-05-06T15:50:33Z

What I did

Implemented JSON Patch Ordering using YANG Models Design Doc

How I did it

How to verify it

Unit-Tests

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

lgtm-com · 2021-05-06T16:01:19Z

This pull request introduces 10 alerts when merging ba2b1d6bf6a1ddf011327887a697d89b778e1dea into 9a88cb6 - view on LGTM.com

new alerts:

4 for Unused import
3 for Unused local variable
2 for Testing equality to None
1 for Inconsistent equality and hashing

lgtm-com · 2021-05-17T20:44:44Z

This pull request introduces 10 alerts when merging 97bc24c4c75dc1b37ecf7bd9d79337c71aba7c08 into ad801bf - view on LGTM.com

new alerts:

5 for Unused import
2 for Testing equality to None
2 for Unused local variable
1 for Inconsistent equality and hashing

qiluo-msft · 2021-05-17T22:45:36Z

Please fix LGTM alerts first.

In reply to: 842690595

lgtm-com · 2021-05-18T08:09:15Z

This pull request introduces 1 alert when merging d61839670813af4455476e788c10fa70280cca82 into ad801bf - view on LGTM.com

new alerts:

1 for Unused import

qiluo-msft · 2021-05-21T22:40:12Z

generic_config_updater/patch_sorter.py

+from enum import Enum
+from .gu_common import OperationWrapper, OperationType, GenericConfigUpdaterError, JsonChange, PathAddressing
+
+# from gu_common import OperationWrapper, OperationType, GenericConfigUpdaterError, JsonChange, PathAddressing


Remove unused code or comments. #Closed

qiluo-msft · 2021-05-21T22:45:12Z

generic_config_updater/patch_sorter.py

+    def __hash__(self):
+        cc = json.dumps(self.current_config, sort_keys=True)
+        tc = json.dumps(self.target_config, sort_keys=True)
+        return hash(cc) ^ hash(tc)


hash(cc) ^ hash(tc)

How about hash ((cc, tc))?
The benefit is swapping input will result in different value. #Closed

good idea. updated

qiluo-msft · 2021-05-21T22:57:13Z

tests/generic_config_updater/gu_common_test.py

 import generic_config_updater.gu_common as gu_common

+# from gutest_helpers import create_side_effect_dict, Files
 # import sys
 # sys.path.insert(0,'../../generic_config_updater')
 # import gu_common


remove unused code. #Closed

qiluo-msft · 2021-05-26T22:48:17Z

generic_config_updater/gu_common.py

+        tokens = path.split(PathAddressing.PATH_SEPARATOR)[1:]
+
+        # Because the characters '~' (%x7E) and '/' (%x2F) have special
+        # meanings in JSON Pointer, '~' needs to be encoded as '~0' and '/'


JSON Pointer

# meanings in JSON Pointer, '~' needs to be encoded as '~0' and '/'

Did you try https://pypi.org/project/jsonpointer/ ? #Closed

still looking into this

qiluo-msft · 2021-05-26T22:52:12Z

generic_config_updater/gu_common.py

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


or ly.LYS_LEAFLIST == schema.nodetype()

return ly.LYS_LEAF == schema.nodetype() # or ly.LYS_LEAFLIST == schema.nodetype()

Remove unused code? #Closed

qiluo-msft · 2021-05-26T22:52:30Z

generic_config_updater/gu_common.py

+        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

Add blanks before and after == #Closed

qiluo-msft · 2021-05-26T22:53:19Z

generic_config_updater/gu_common.py

+        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.


# It is OK to have null backlinks if there is no backlinks.

Could you create a Github issue or submit a PR if you already know how to fix? #Closed

~~PR submitted: sonic-net/sonic-buildimage#7939~~

NOTE: unit-tests in the current PR (patch-sorting) will fail until the PR submitted above is completed

created PR on forked repo: sonic-net/sonic-buildimage#8187

qiluo-msft · 2021-05-26T22:54:45Z

generic_config_updater/gu_common.py

+        node = sy.root
+        try:
+            data_node = sy._find_data_node(data_xpath)
+        except Exception as e:


Exception

except Exception as e:

Could you use specific exception type? Here you swallow all the exception types and lose the exception context (like stack). #Closed

code removed locally

code removed

qiluo-msft · 2021-05-26T22:55:41Z

generic_config_updater/gu_common.py

+                          casted = data_set.subtype()
+                          if value == casted.value_str():
+                              ref_list.append(data_set.path())
+        except Exception as e:


Exception

except Exception as e:

Could you use specific exception type? Seems you know the exception is related to "Failed to find node or dependencies" #Closed

code removed locally

code removed

qiluo-msft · 2021-06-06T11:50:20Z

generic_config_updater/gu_common.py

+        # src: https://tools.ietf.org/html/rfc6901
+        mod_tokens = []
+        for token in tokens:
+            mod_token = str(token)


str

mod_token = str(token)

Why you need to str convert? #Closed

good catch, updated locally

qiluo-msft · 2021-06-06T11:55:38Z

generic_config_updater/gu_common.py

+                return [token]
+            list_config = config[token]
+            value=list_config[int(path_tokens[token_index+1])]
+            return [f"{token}[.='{value}']"]


{token}[.='{value}

It's difficult to read, could you add some comment? Is the . a mistake? #Closed

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

Added a comment to explain this '.' locally.

qiluo-msft · 2021-06-06T11:57:32Z

generic_config_updater/gu_common.py

+        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":


and clist['@name'] == token+"_LIST":

if isinstance(clist, dict): # and clist['@name'] == token+"_LIST":

Remove unused code? #Closed

qiluo-msft · 2021-06-06T11:58:34Z

generic_config_updater/gu_common.py

+        if len(xpath_tokens)-1 == token_index:
+            return path_tokens
+
+        nxt_token = xpath_tokens[token_index+1]


nxt_token

nxt_token = xpath_tokens[token_index+1]

Do you mean next_token? #Closed

qiluo-msft · 2021-06-06T12:00:35Z

generic_config_updater/patch_sorter.py

+    """
+    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.patch=jsonpatch.JsonPatch([operation])

Add a blank before and after operators. #Closed

qiluo-msft · 2021-06-06T12:05:15Z

generic_config_updater/patch_sorter.py

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


hash(self.op_type) ^ hash(self.path) ^ hash(json.dumps(self.value))

return hash(self.op_type) ^ hash(self.path) ^ hash(json.dumps(self.value))

hash((a, b, c)) ? #Closed

qiluo-msft

As comments

…t#1599)" This reverts commit 920bb87.

This reverts commit 920bb87.

lguohan · 2021-08-17T16:15:21Z

@ghooo , please note since this pr is causing unit testing failure, it has been reverted. Please check the error here. https://dev.azure.com/mssonic/build/_build/results?buildId=29578&view=logs&j=cef3d8a9-152e-5193-620b-567dc18af272&t=359769c4-8b5e-5976-a793-85da132e0a6f&s=ff05ad62-bb9a-53b6-ce9f-72f329a63e7c

please check attempt #1

…onic-net#1599)"" This reverts commit e6122e9.

) #### What I did Implemented [JSON Patch Ordering using YANG Models Design Doc](https://github.com/Azure/SONiC/blob/master/doc/config-generic-update-rollback/Json_Patch_Ordering_using_YANG_Models_Design.md) #### How to verify it Unit-Tests **NOTE: The code in this PR was [reverted](github.com/Azure/sonic-utilities/commit/0a145e8027380e8d4decb36bdfc647062c722612) before because of some [build issues](#1761). Build issues have been fixed [here](sonic-net/sonic-buildimage#8632). To check the original PR comments please go [here](#1599

…794) #### What I did Implemented [JSON Patch Ordering using YANG Models Design Doc](https://github.com/Azure/SONiC/blob/master/doc/config-generic-update-rollback/Json_Patch_Ordering_using_YANG_Models_Design.md) #### How to verify it Unit-Tests **NOTE: The code in this PR was [reverted](github.com/Azure/sonic-utilities/commit/0a145e8027380e8d4decb36bdfc647062c722612) before because of some [build issues](sonic-net/sonic-utilities#1761). Build issues have been fixed [here](sonic-net/sonic-buildimage#8632). To check the original PR comments please go [here](sonic-net/sonic-utilities#1599

ghooo force-pushed the dev/mghoneim/patch_sorter branch 3 times, most recently from 2cad759 to c20679f Compare May 20, 2021 11:10

qiluo-msft reviewed May 21, 2021

View reviewed changes

qiluo-msft reviewed May 26, 2021

View reviewed changes

qiluo-msft reviewed Jun 6, 2021

View reviewed changes

ghooo added 3 commits July 17, 2021 03:38

[config][generic-update] Implementing patch sorting

4da6ffa

addressing comments

7920963

addressing comments

c554f6d

ghooo force-pushed the dev/mghoneim/patch_sorter branch from d3d04d2 to c554f6d Compare July 17, 2021 10:38

using jsonpointer to get path parts

9590bdc

qiluo-msft approved these changes Aug 10, 2021

View reviewed changes

ghooo merged commit 920bb87 into sonic-net:master Aug 10, 2021

vivekrnv added a commit to vivekrnv/sonic-utilities that referenced this pull request Aug 14, 2021

Revert "[config][generic-update] Implementing patch sorting (sonic-ne…

e6122e9

…t#1599)" This reverts commit 920bb87.

lguohan added a commit that referenced this pull request Aug 17, 2021

Revert "[config][generic-update] Implementing patch sorting (#1599)"

20abc03

This reverts commit 920bb87.

lguohan added a commit that referenced this pull request Aug 17, 2021

Revert "[config][generic-update] Implementing patch sorting (#1599)"

0a145e8

This reverts commit 920bb87.

vivekrnv added a commit to vivekrnv/sonic-utilities that referenced this pull request Aug 19, 2021

Revert "Revert "[config][generic-update] Implementing patch sorting (s…

903a2f0

…onic-net#1599)"" This reverts commit e6122e9.

This was referenced Sep 1, 2021

[config][generic-update] Implementing patch sorting (2nd attempt) #1794

Merged

[sonic-utilities] Unit test failed when building sonic-utilities #1761

Closed

ghooo mentioned this pull request Jun 24, 2022

SONiC Generic Update and Rollback - HLD sonic-net/SONiC#736

Merged

[config][generic-update] Implementing patch sorting #1599

[config][generic-update] Implementing patch sorting #1599

Conversation

ghooo commented May 6, 2021

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

lgtm-com bot commented May 6, 2021

lgtm-com bot commented May 17, 2021

qiluo-msft commented May 17, 2021 • edited Loading

lgtm-com bot commented May 18, 2021

qiluo-msft May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 26, 2021 • edited Loading

Choose a reason for hiding this comment

# meanings in JSON Pointer, '~' needs to be encoded as '~0' and '/'

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 26, 2021 • edited Loading

Choose a reason for hiding this comment

or ly.LYS_LEAFLIST == schema.nodetype()

return ly.LYS_LEAF == schema.nodetype() # or ly.LYS_LEAFLIST == schema.nodetype()

Choose a reason for hiding this comment

qiluo-msft May 26, 2021 • edited Loading

Choose a reason for hiding this comment

if xpath=="/": # Point to Root element which contains all xpaths

Choose a reason for hiding this comment

qiluo-msft May 26, 2021 • edited Loading

Choose a reason for hiding this comment

# It is OK to have null backlinks if there is no backlinks.

ghooo Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 26, 2021 • edited Loading

Choose a reason for hiding this comment

except Exception as e:

ghooo Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 26, 2021 • edited Loading

Choose a reason for hiding this comment

except Exception as e:

ghooo Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

mod_token = str(token)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

and clist['@name'] == token+"_LIST":

if isinstance(clist, dict): # and clist['@name'] == token+"_LIST":

Choose a reason for hiding this comment

qiluo-msft Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

nxt_token = xpath_tokens[token_index+1]

Choose a reason for hiding this comment

qiluo-msft Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

self.patch=jsonpatch.JsonPatch([operation])

Choose a reason for hiding this comment

qiluo-msft Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

return hash(self.op_type) ^ hash(self.path) ^ hash(json.dumps(self.value))

qiluo-msft commented May 17, 2021 •

edited

Loading

qiluo-msft May 21, 2021 •

edited

Loading

qiluo-msft May 21, 2021 •

edited

Loading

qiluo-msft May 21, 2021 •

edited

Loading

qiluo-msft May 26, 2021 •

edited

Loading

qiluo-msft May 26, 2021 •

edited

Loading

qiluo-msft May 26, 2021 •

edited

Loading

qiluo-msft May 26, 2021 •

edited

Loading

ghooo Jun 22, 2021 •

edited

Loading

qiluo-msft May 26, 2021 •

edited

Loading

ghooo Jun 22, 2021 •

edited

Loading

qiluo-msft May 26, 2021 •

edited

Loading

ghooo Jun 22, 2021 •

edited

Loading

qiluo-msft Jun 6, 2021 •

edited

Loading

qiluo-msft Jun 6, 2021 •

edited

Loading

qiluo-msft Jun 6, 2021 •

edited

Loading

qiluo-msft Jun 6, 2021 •

edited

Loading

qiluo-msft Jun 6, 2021 •

edited

Loading

qiluo-msft Jun 6, 2021 •

edited

Loading

lguohan commented Aug 17, 2021 •

edited

Loading