[generic-config-updater] Handling empty tables while sorting a patch #1923

ghooo · 2021-11-11T01:01:02Z

What I did

Fixing issue #1908

How I did it

When the given patch produces empty-table, reject it since ConfigDb cannot show empty tables. Achieved that by:
- Adding a validation step before patch-sorting
The patch-sorter should not produce steps which result in empty-tables because it is not possible in ConfigDb, we should instead delete the whole table. Achieved that by:
- Adding a new validator to reject moves producing empty tables
- No need to add a generator for deleting the whole table, current generators take care of that. They do by the following:
  - The move to empty a table is rejected by NoEmptyTableMoveValidator
  - The previous rejected move is used to generate moves using UpperLevelMoveExtender which produces either
    - Remove move for parent -- This is good, we are removing the table
    - Replace move for parent -- This later on will be extended to a Delete move using DeleteInsteadOfReplaceMoveExtender

The generators/validators are documented in the PatchSorter.py, check the documentation out.

How to verify it

unit-test

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

admin@vlab-01:~$ sudo config apply-patch empty-a-table.json-patch -v
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "replace", "path": "/DEVICE_METADATA", "value": {}}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config according to YANG models.
... sonig-yang-mgmt logs
Patch Applier: Sorting patch updates.
... sonig-yang-mgmt logs
Patch Applier: The patch was sorted into 11 changes:
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/bgp_asn"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/buffer_model"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/default_bgp_status"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/default_pfcwd_status"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/deployment_id"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/docker_routing_config_mode"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/hostname"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/hwsku"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/mac"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost/platform"}]
Patch Applier:   * [{"op": "remove", "path": "/DEVICE_METADATA/localhost"}]
Patch Applier: Applying changes in order.
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: After applying patch to config, there are still some parts not updated
admin@vlab-01:~$

The above error occurs because post the update, the whole DEVICE_METADATA table is deleted, not just showing as empty i.e. "DEVICE_METADATA": {}

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

admin@vlab-01:~$ sudo config apply-patch empty-a-table.json-patch -v
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "replace", "path": "/DEVICE_METADATA", "value": {}}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config does not have empty tables, since they do not show up in ConfigDb.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Given patch is not valid because it will result in empty tables which is not allowed in ConfigDb. Table: DEVICE_METADATA
admin@vlab-01:~$

lgtm-com · 2021-11-11T01:12:43Z

This pull request introduces 1 alert when merging e47e186b1b0996ca108e8bc95bcf28853404cf75 into 4bcaa60 - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

lgtm-com · 2021-11-15T05:22:38Z

This pull request introduces 1 alert when merging 5984d0fcbdbb3258e241a885875be32e306de01b into 00b6045 - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

qiluo-msft · 2021-11-15T19:55:47Z

generic_config_updater/generic_updater.py

+        # Validate target config does not have empty tables since they do not show up in ConfigDb
+        self.logger.log_notice("Validating target config does not have empty tables, " \
+                               "since they do not show up in ConfigDb.")
+        has_empty_tables, empty_tables = self.config_wrapper.has_empty_tables(target_config)


empty_tables

Not need to return bool, you can check len(empty_tables). #Closed

qiluo-msft · 2021-11-15T20:00:16Z

generic_config_updater/patch_sorter.py

+
+    def _validate_table(self, table, config):
+        is_empty = table in config and not(config[table])
+        return not(is_empty)


not

too many not. How about table not in config or config[table] #Closed

It will look like this:

def _validate_table(self, table, config): return table not in config or config[table]

This means the table is either not in config, or table has content. Why does this mean the table is not empty? It will take future developer a while to understand it. I think it is easier to have a flag is_empty and revert the flag on return.

Either way is okay to me.
Could you add function level comment "the only invalid case is if table exist and is empty"?

[offline discussion] will update and add a small comment

qiluo-msft

As comments

[generic-config-updater] Handling empty tables while sorting a patch

31fe18a

ghooo force-pushed the dev/mghoneim/sorter_empty_table branch from 38fd9a2 to 31fe18a Compare November 15, 2021 18:16

qiluo-msft reviewed Nov 15, 2021

View reviewed changes

renukamanavalan previously approved these changes Nov 16, 2021

View reviewed changes

returning just tables instead of flag and tables

d8a78e6

ghooo dismissed renukamanavalan’s stale review via d8a78e6 November 17, 2021 15:40

ghooo added 2 commits November 17, 2021 08:52

fixing unit-test

b480152

simplyfing conditions

1d9db52

qiluo-msft approved these changes Nov 17, 2021

View reviewed changes

ghooo merged commit 2ec47a5 into sonic-net:master Nov 18, 2021

ghooo mentioned this pull request Dec 8, 2021

JSON change: remove op for empty table is invalid #1908

Closed

ghooo mentioned this pull request Jun 24, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[generic-config-updater] Handling empty tables while sorting a patch #1923

[generic-config-updater] Handling empty tables while sorting a patch #1923

ghooo commented Nov 11, 2021 •

edited

Loading

lgtm-com bot commented Nov 11, 2021

lgtm-com bot commented Nov 15, 2021

qiluo-msft Nov 15, 2021 •

edited

Loading

ghooo Nov 17, 2021

qiluo-msft Nov 15, 2021 •

edited

Loading

ghooo Nov 17, 2021 •

edited

Loading

qiluo-msft Nov 17, 2021

ghooo Nov 17, 2021

ghooo Nov 17, 2021

qiluo-msft left a comment

[generic-config-updater] Handling empty tables while sorting a patch #1923

[generic-config-updater] Handling empty tables while sorting a patch #1923

Conversation

ghooo commented Nov 11, 2021 • edited Loading

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 Nov 11, 2021

lgtm-com bot commented Nov 15, 2021

qiluo-msft Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

ghooo Nov 17, 2021

Choose a reason for hiding this comment

qiluo-msft Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

ghooo Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Nov 17, 2021

Choose a reason for hiding this comment

ghooo Nov 17, 2021

Choose a reason for hiding this comment

ghooo Nov 17, 2021

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

ghooo commented Nov 11, 2021 •

edited

Loading

qiluo-msft Nov 15, 2021 •

edited

Loading

qiluo-msft Nov 15, 2021 •

edited

Loading

ghooo Nov 17, 2021 •

edited

Loading