Skip to content

[show] Add 'show p4-table' command#2773

Merged
qiluo-msft merged 5 commits intosonic-net:masterfrom
royyi8:master_p4_table
Aug 15, 2023
Merged

[show] Add 'show p4-table' command#2773
qiluo-msft merged 5 commits intosonic-net:masterfrom
royyi8:master_p4_table

Conversation

@royyi8
Copy link
Copy Markdown
Contributor

@royyi8 royyi8 commented Apr 3, 2023

What I did

Add 'show p4-table' command to the CLI. This is a new command to display P4RT tables used by PINS (P4 Integrated Network Stack).

How I did it

Add the command in show/p4_table.py to output the P4RT table entries in the Redis application database.

How to verify it

Run unit tests, manually test

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

None

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

See command reference

@mint570
Copy link
Copy Markdown

mint570 commented Apr 19, 2023

@svshah-intel can you please review this PR?

svshah-intel
svshah-intel previously approved these changes Apr 23, 2023
- What I did
Add 'show p4-table' command to the CLI. This is a new command to
display P4RT tables used by PINS (P4 Integrated Network Stack).

- How I did it
Add the command in show/p4_table.py to output the P4RT table entries in
the redis application database.

- How to verify it
Run unit tests, manually test

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

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

Signed-off-by: Roy Yi <royyi@google.com>
@mint570
Copy link
Copy Markdown

mint570 commented Apr 26, 2023

@svshah-intel all test checks are passing now after the rebase. Can you re-approve the PR? Thanks.

@royyi8
Copy link
Copy Markdown
Contributor Author

royyi8 commented May 1, 2023

@qiluo-msft Could you please review this PR? Thanks.

@royyi8
Copy link
Copy Markdown
Contributor Author

royyi8 commented May 22, 2023

@qiluo-msft Friendly reminder to review this CL, so we can get it merged. Thanks!

@maipbui
Copy link
Copy Markdown
Contributor

maipbui commented Jun 29, 2023

@royyi8 Please merge latest master code to trigger Semgrep.

show/p4_table.py Outdated
@click.option('--verbose', is_flag=True, help='Enable verbose output')
def p4_table(table_name, verbose):
"""Display all P4RT tables"""
appDB = SonicV2Connector(host='127.0.0.1')
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2023

Choose a reason for hiding this comment

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

host

Could you use unix socket? #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the code to use unix socket.

show/p4_table.py Outdated
"""Display all P4RT tables"""
appDB = SonicV2Connector(host='127.0.0.1')

if appDB is None:
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2023

Choose a reason for hiding this comment

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

We prefer 4 spaces as indentation. #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed the indentation to 4 spaces.

show/p4_table.py Outdated
if table_name is None:
table_name = ''

keys = appDB.keys(db, f'P4RT_TABLE:{table_name}*')
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2023

Choose a reason for hiding this comment

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

P4RT_TABLE:

If you use Table class, you do not need to concatenate table name and entry name. #WontFix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

: as separator is defined in database_config.json

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to Table class in swsscommon? This class takes swsscommon.DBConnector as an argument rather than swsscommon.SonicV2Connector, so it appears that I cannot use the provided mock dbconnector in my unit test if I use this class.

Will it suffice to match on the separator defined in database_config.json?

"BGP_PROFILE_TABLE:FROM_SDN_SLB_ROUTES": {
"community_id" : "1234:1235"
},
"P4RT_TABLE:ACL_TABLE_DEFINITION_TABLE:ACL_ACL_PRE_INGRESS_TABLE": {
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 29, 2023

Choose a reason for hiding this comment

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

P4RT_TABLE

Could you reformat this json file? #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you please clarify what needs to be reformatted? I run the json file through a json beautifier and I see the same result.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean the json indentation. I see tabs/spaces mixed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I fixed the indentation to use spaces.

@qiluo-msft qiluo-msft merged commit 6322389 into sonic-net:master Aug 15, 2023
@mint570 mint570 mentioned this pull request Feb 5, 2024
cpackham-atlnz added a commit to cpackham-atlnz/sonic-utilities that referenced this pull request Oct 3, 2025
Commit 6322389 ("[show] Add 'show p4-table' command (sonic-net#2773)") added a
table of contents entry for the "PINS Show commands" but the link target
didn't match the generated anchor so the TOC entry didn't render
correctly. Update it to use the correct link target so the TOC entry
works as intended.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
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