[debug dump util] Module implementation Logic and Port Module

[vivekrnv]

Signed-off-by: Vivek Reddy Karri [email protected]

DEBUG DUMP UTIL PR SEQ: 2

NOTE: PLEASE MERGE #1666 BEFORE MERGING THIS

What I did

HLD for Dump Utility: HLD.
For More Info on adding new modules, Check this section in the HLD:
MatchInfra

How I did it

How to verify it

tests/dump_tests/module_tests/port_test.py::TestPortModule::test_all_args PASSED [ 86%]
tests/dump_tests/module_tests/port_test.py::TestPortModule::test_missing_asic_hostif PASSED [ 86%]
tests/dump_tests/module_tests/port_test.py::TestPortModule::test_missing_asic_port PASSED [ 86%]
tests/dump_tests/module_tests/port_test.py::TestPortModule::test_missing_state_and_appl PASSED [ 86%]
tests/dump_tests/module_tests/port_test.py::TestPortModule::test_no_port PASSED [ 86%]
tests/dump_tests/module_tests/port_test.py::TestPortModule::test_working_state PASSED [ 86%]

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)

admin@sonic:~$ dump state port Ethernet0
{
    "Ethernet0": {
        "CONFIG_DB": {
            "keys": [
                {
                    "PORT|Ethernet0": {
                        "admin_status": "up",
                        "alias": "etp1",
                        "index": "1",
                        "lanes": "0",
                        "speed": "25000"
                    }
                }
            ],
            "tables_not_found": []
        },
        "APPL_DB": {
            "keys": [
                {
                    "PORT_TABLE:Ethernet0": {
                        "admin_status": "up",
                        "alias": "etp1",
                        "description": "",
                        "index": "1",
                        "lanes": "0",
                        "mtu": "9100",
                        "oper_status": "up",
                        "speed": "25000"
                    }
                }
            ],
            "tables_not_found": []
        },
        "ASIC_DB": {
            "keys": [
                {
                    "ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF:oid:0xd000000000a21": {
                        "SAI_HOSTIF_ATTR_NAME": "Ethernet0",
                        "SAI_HOSTIF_ATTR_OBJ_ID": "oid:0x10000000008d4",
                        "SAI_HOSTIF_ATTR_OPER_STATUS": "true",
                        "SAI_HOSTIF_ATTR_TYPE": "SAI_HOSTIF_TYPE_NETDEV"
                    }
                },
                {
                    "ASIC_STATE:SAI_OBJECT_TYPE_PORT:oid:0x10000000008d4": {
                        "NULL": "NULL",
                        "SAI_PORT_ATTR_ADMIN_STATE": "true",
                        "SAI_PORT_ATTR_MTU": "9122",
                        "SAI_PORT_ATTR_SPEED": "25000"
                    }
                }
            ],
            "tables_not_found": [],
            "vidtorid": {
                "oid:0xd000000000a21": "oid:0xd",
                "oid:0x10000000008d4": "oid:0x1690000000001"
            }
        },
        "STATE_DB": {
            "keys": [
                {
                    "PORT_TABLE|Ethernet0": {
                        "netdev_oper_status": "up",
                        "state": "ok"
                    }
                }
            ],
            "tables_not_found": []
        }
    }
}

[lgtm-com]

This pull request introduces 1 alert when merging 6d76514 into a425ca2 - view on LGTM.com

new alerts:

• 1 for Unused import

[liat-grozovik]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vivekrnv]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 1667 in repo Azure/sonic-utilities

[liat-grozovik]

/app run

[vivekrnv]

Request for 202106

[arlakshm]

As comments

[arlakshm]

Why we need to introduce a new class for mocking Sonicv2connector. Can we add new functionality to dbconnector.py

[vivekrnv]

Testing Use case for these modules is to read from a separate json files as opposed to other tests which read from the default json files inside mock_tables directory.

I did see there is a dedicated_db list used inside one of the dbconnector methods

global dedicated_dbs
if dedicated_dbs and dedicated_dbs.get(db_name):
        self.dbintf.redis_kwargs['db_name'] = dedicated_dbs[db_name]
else:
        self.dbintf.redis_kwargs['db_name'] = db_name

The only test which uses this is db_migrator_test.py. However in that test, it was initializing the SonicV2Connector class directly inside the test, whereas in dump_module_tests it is being initialized in the methods defined in the actual classes.

And probably because of the complex redirection used in the dbconnector.py to mock the SonicV2Connector, i was not able to get the dump_module_tests tests running on custom json files. I've also worked with stephan who introduced that change but couldn't get it running.

Hence, decided to add a new mock class.

[arlakshm]

I found a couple of examples of using the dedicated_dbs recently added recently

https://github.com/Azure/sonic-utilities/blob/master/tests/config_int_ip_test.py
https://github.com/Azure/sonic-utilities/blob/5002745beb89a99a1cdf420f34a495c2c0cb31bd/tests/mpls_test.py#L60-L67

Can you check if we can follow a similar approach here?
I feel we should try and avoid duplicate mock classes if possible

[vivekrnv]

Thanks for the info. I was able to remove the extra mock and use the default mock. I couldn't get it to directly read the custom json files, but i've handled it nevertheless.

Regarding the connection pooling optimization, i've recently observed when operating at scale, (i.e. route, had around 10k routes to parse for the dump utility) i've observed a significant amount of time spent in connect calls. With this optimization, the number of connect calls can be effectively reduced from (number_of_routes*number_of_calls_per_route) to 4 (number of db's the module is looking into) and thus offering a significant improvement in performance.
This design change infact helped me to use the default mock as well.

@SuvarnaMeenakshi and @arlakshm , please provide your comments on the changes.

[SuvarnaMeenakshi]

As comments.

[lgtm-com]

This pull request introduces 3 alerts when merging 0082da2 into 29f4a16 - view on LGTM.com

new alerts:

• 2 for First parameter of a method is not named 'self'
• 1 for Unused import

[SuvarnaMeenakshi]

lgtm

[dgsudharsan]

@arlakshm Can you please sign off if the changes are good?

[dgsudharsan]

@SuvarnaMeenakshi Can we merge this?