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.

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

Debug dump utility dash objects update #3387

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gpunathilell
Copy link

What I did

Added support for the following DASH objects for the dump utility:
dash_acl_group
dash_acl_out
dash_acl_rule
dash_appliance
dash_prefix_tag
dash_eni
dash_qos
dash_route
dash_route_rule
dash_vnet_mapping
dash_vnet

This PR also adds Match infra update in order to consider Dash objects, so new match requests can be created with the fields present in the dash objects to obtain the fields/keys.

How I did it

Added protobuf library and libdashapi to sonic-utilities, this is required as the dash objects are stored in the APPL_DB in protobuf format, and we need the .proto files which are available in the libdashapi package. We also use redis package instead of the SonicV2Connector since SonicV2Connector get_all function from the connector considers null terminated strings so the complete protobuf data is not obtained using get_all function

How to verify it

dump state all <dash_object>
Examples:

admin@sonic:~$ dump state dash_vnet Vnet1 -t
+-------------------+-----------+--------------------------------------------------------------------------------------------+
| dash_vnet_table   | DB_NAME   | DUMP                                                                                       |
+===================+===========+============================================================================================+
| Vnet1             | APPL_DB   | +-----------------------+----------------------------------------------------+             |
|                   |           | | Keys                  | field-value pairs                                  |             |
|                   |           | +=======================+====================================================+             |
|                   |           | | DASH_VNET_TABLE:Vnet1 | +---------+--------------------------------------+ |             |
|                   |           | |                       | | field   | value                                | |             |
|                   |           | |                       | |---------+--------------------------------------| |             |
|                   |           | |                       | | vni     | 50                                   | |             |
|                   |           | |                       | | guid    | 5526cce8-26ab-4193-b946-ccc0e8f930b0 | |             |
|                   |           | |                       | +---------+--------------------------------------+ |             |
|                   |           | +-----------------------+----------------------------------------------------+             |
+-------------------+-----------+--------------------------------------------------------------------------------------------+
| Vnet1             | ASIC_DB   | +------------------------------------------------------+---------------------------------+ |
|                   |           | | Keys                                                 | field-value pairs               | |
|                   |           | +======================================================+=================================+ |
|                   |           | | ASIC_STATE:SAI_OBJECT_TYPE_VNET:oid:0x7a000000000021 | +-------------------+---------+ | |
|                   |           | |                                                      | | field             |   value | | |
|                   |           | |                                                      | |-------------------+---------| | |
|                   |           | |                                                      | | SAI_VNET_ATTR_VNI |    100  | | |
|                   |           | |                                                      | +-------------------+---------+ | |
|                   |           | +------------------------------------------------------+---------------------------------+ |
|                   |           | +----------------------+--------------------+                                              |
|                   |           | | vid                  | rid                |                                              |
|                   |           | +======================+====================+                                              |
|                   |           | | oid:0x7a000000000021 | oid:0xffff70009130 |                                              |
|                   |           | +----------------------+--------------------+                                              |
+-------------------+-----------+--------------------------------------------------------------------------------------------+
admin@sonic:~$ dump state dash_acl_rule all
{
    "group1:rule1": {
        "APPL_DB": {
            "keys": [
                {
                    "DASH_ACL_RULE_TABLE:group1:rule1": {
                        "action": "ACTION_PERMIT",
                        "terminating": true,
                        "src_addr": [
                            "0.0.0.0/0"
                        ],
                        "dst_addr": [
                            "0.0.0.0/0"
                        ],
                        "src_port": [
                            {
                                "value": 80
                            }
                        ],
                        "dst_port": [
                            {
                                "value": 5005
                            }
                        ]
                    }
                }
            ],
            "tables_not_found": []
        }
    }
}

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)

@dgsudharsan
Copy link
Collaborator

@qiluo-msft Can you please approve the workflow for the pipeline?

@gpunathilell
Copy link
Author

@Pterosaur Please review

@Pterosaur Pterosaur self-requested a review July 10, 2024 00:59
@Pterosaur
Copy link
Contributor

Pterosaur commented Jul 10, 2024

Is it possible to directly leverage or refer the utils from sonic-dash-api to serialize/deserialize the pb object to target json format?
https://github.com/sonic-net/sonic-dash-api/blob/master/misc/utils.h
We did also provide the python library, please refer to: https://github.com/sonic-net/sonic-dash-api/blob/5809048abbc52ce1762f3ed29eb77483051176dd/misc/dash_api_utils#L37

I don't think listing all protobuf message formats in the plugin forlder is maintainable.

@gpunathilell
Copy link
Author

Is it possible to directly leverage or refer the utils from sonic-dash-api to serialize/deserialize the pb object to target json format? https://github.com/sonic-net/sonic-dash-api/blob/master/misc/utils.h We did also provide the python library, please refer to: https://github.com/sonic-net/sonic-dash-api/blob/5809048abbc52ce1762f3ed29eb77483051176dd/misc/dash_api_utils#L37

I don't think listing all protobuf message formats in the plugin forlder is maintainable.

The .py files in the plugin folders are separate in order to maintain the current debug dump utility architecture of having a separate module for displaying the information separately for different modules (for example: the DASH_ACL_RULE table is treated as a module and displays only information relevant to that specific table using the command dump state dash_acl_rule all command.
For serialization/deserialization we use the ParseFromString and MessageToDict functions. This is used instead of the sonic-dash-api provided PbBinarytoJsonString because we need to do further processing of some types such as IpAddress IpPrefix and Guid to display in a readable format(to display IpAddress as 0.0.0.0 instead of just a number) , by leveraging the ParseFromString any new dash objects which are added which contain these types(IpAddress or IpPrefix) would have these fields in a readable format if we extend the dump utility, since the type of the field in the message can not be obtained using the provided PbBinarytoJsonString (in order to do further processing to display these fields in a readable formats) is the reason why the provided api in utils.h is not used.

dump/dash_util.py Outdated Show resolved Hide resolved
dump/main.py Outdated Show resolved Hide resolved
@Pterosaur
Copy link
Contributor

Pterosaur commented Jul 24, 2024

Is it possible to directly leverage or refer the utils from sonic-dash-api to serialize/deserialize the pb object to target json format? https://github.com/sonic-net/sonic-dash-api/blob/master/misc/utils.h We did also provide the python library, please refer to: https://github.com/sonic-net/sonic-dash-api/blob/5809048abbc52ce1762f3ed29eb77483051176dd/misc/dash_api_utils#L37
I don't think listing all protobuf message formats in the plugin forlder is maintainable.

The .py files in the plugin folders are separate in order to maintain the current debug dump utility architecture of having a separate module for displaying the information separately for different modules (for example: the DASH_ACL_RULE table is treated as a module and displays only information relevant to that specific table using the command dump state dash_acl_rule all command. For serialization/deserialization we use the ParseFromString and MessageToDict functions. This is used instead of the sonic-dash-api provided PbBinarytoJsonString because we need to do further processing of some types such as IpAddress IpPrefix and Guid to display in a readable format(to display IpAddress as 0.0.0.0 instead of just a number) , by leveraging the ParseFromString any new dash objects which are added which contain these types(IpAddress or IpPrefix) would have these fields in a readable format if we extend the dump utility, since the type of the field in the message can not be obtained using the provided PbBinarytoJsonString (in order to do further processing to display these fields in a readable formats) is the reason why the provided api in utils.h is not used.

OK, I agree.
IMHO, there are too many duplicated or similar codes and structures under the plugin folder.
I'm not sure whether we can leverage TypeResolver to dynamically infer or register the message type and generate subclasses of Executor. Like what I did at here: https://github.com/sonic-net/sonic-dash-api/blob/5809048abbc52ce1762f3ed29eb77483051176dd/misc/utils.cpp#L29-L67

I agree that there are some specific codes for some specific message types, but the most difference between the dash types are metadata or data name.

Because I worry that it's hard to maintain if we keep two concrete type codes at the sonic-dash-api and here.

@gpunathilell
Copy link
Author

Is it possible to directly leverage or refer the utils from sonic-dash-api to serialize/deserialize the pb object to target json format? https://github.com/sonic-net/sonic-dash-api/blob/master/misc/utils.h We did also provide the python library, please refer to: https://github.com/sonic-net/sonic-dash-api/blob/5809048abbc52ce1762f3ed29eb77483051176dd/misc/dash_api_utils#L37
I don't think listing all protobuf message formats in the plugin forlder is maintainable.

The .py files in the plugin folders are separate in order to maintain the current debug dump utility architecture of having a separate module for displaying the information separately for different modules (for example: the DASH_ACL_RULE table is treated as a module and displays only information relevant to that specific table using the command dump state dash_acl_rule all command. For serialization/deserialization we use the ParseFromString and MessageToDict functions. This is used instead of the sonic-dash-api provided PbBinarytoJsonString because we need to do further processing of some types such as IpAddress IpPrefix and Guid to display in a readable format(to display IpAddress as 0.0.0.0 instead of just a number) , by leveraging the ParseFromString any new dash objects which are added which contain these types(IpAddress or IpPrefix) would have these fields in a readable format if we extend the dump utility, since the type of the field in the message can not be obtained using the provided PbBinarytoJsonString (in order to do further processing to display these fields in a readable formats) is the reason why the provided api in utils.h is not used.

OK, I agree. IMHO, there are too many duplicated or similar codes and structures under the plugin folder. I'm not sure whether we can leverage TypeResolver to dynamically infer or register the message type and generate subclasses of Executor. Like what I did at here: https://github.com/sonic-net/sonic-dash-api/blob/5809048abbc52ce1762f3ed29eb77483051176dd/misc/utils.cpp#L29-L67

I agree that there are some specific codes for some specific message types, but the most difference between the dash types are metadata or data name.

Because I worry that it's hard to maintain if we keep two concrete type codes at the sonic-dash-api and here.

the issue with this approach would be that there are some differences between the implemented plugins, so the debug dump utility provides a unified view across multiple DBs for a module in the plugin folder(For example DASH_VNET_TABLE in APPL_DB will have a corresponding SAI_OBJECT_TYPE_VNET in ASIC_DB) which can be matched using the vni obtained from DASH_VNET_TABLE and then matching it with a SAI_OBJECT_TYPE_VNET table which has the same vni SAI_VNET_ATTR_VNI field
seen here
, this matching between multiple DBs is unique for each plugin, which is why plugins are different, if there are any other methods/improvements which can be done please let me know

Pterosaur
Pterosaur previously approved these changes Aug 21, 2024
@gpunathilell
Copy link
Author

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-utilities

Copy link

Azure Pipelines failed to run 1 pipeline(s).

qiluo-msft
qiluo-msft previously approved these changes Sep 9, 2024
dump/main.py Outdated Show resolved Hide resolved
dump/match_infra.py Outdated Show resolved Hide resolved
try:
importlib.import_module('.' + name, __package__)
except ImportError as e:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

pass

swallow exception blindly is dangerous. Do you really need it?

Copy link
Author

@gpunathilell gpunathilell Oct 7, 2024

Choose a reason for hiding this comment

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

This is used to print the modules available for the dump utility :

admin@sonic:~$ dump state --show
Module              Identifier
------------------  ---------------------
acl_rule            acl_rule_name
acl_table           acl_table_name
copp                trap_id
evpn                Remote VNI
fdb                 Vlan:fdb_entry
interface           intf_name
port                port_name
portchannel         portchannel_name
portchannel_member  portchannel_member
route               destination_network
vlan                vlan_name
vlan_member         vlan_member_name
vxlan_tunnel        vxlan_tunnel_name
vxlan_tunnel_map    vxlan_tunnel_map_name
admin@sonic:~$

I did not want to add anything else here so that on platforms where the newly added dash modules are not present there is no indication of error

@qiluo-msft
Copy link
Contributor

/azp run Azure.sonic-utilities

Copy link

Azure Pipelines failed to run 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants