Skip to content

[show] add new CLI to show tunnel route objects#2255

Merged
zjswhhh merged 9 commits intosonic-net:masterfrom
zjswhhh:asic_db_tunnel_route_master
Jul 7, 2022
Merged

[show] add new CLI to show tunnel route objects#2255
zjswhhh merged 9 commits intosonic-net:masterfrom
zjswhhh:asic_db_tunnel_route_master

Conversation

@zjswhhh
Copy link
Copy Markdown
Contributor

@zjswhhh zjswhhh commented Jul 6, 2022 •
edited
Loading

What I did

Add new CLI support to show tunnel route objects in ASIC DB.

sign-off: Jing Zhang zhangjing@microsoft.com

How I did it

  • Check if tunnel route object exists for server_ipv4, server_ipv6, soc_ipv4. If existing, print it out.
  • If not specifying port name, print all tunnel route objects.

How to verify it

  • Added unit tests.
  • Tested on dual testbeds:
    • show mux tunnel-route Ethernet140 
       PORT         DEST_TYPE    DEST_ADDRESS    
 -----------  -----------  ---------------  
 Ethernet140  server_ipv4  192.168.0.29/32
    • show mux tunnel-route --json Ethernet140 
       {
     "TUNNEL_ROUTE": {
         "Ethernet140": {
             "server_ipv4": {
                 "DEST": "192.168.0.29/32"
             }
         }
     }
 }

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)

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Jul 6, 2022

I tent to disagree with this change. First of all, Nexthop ID doesn't convey any meaningful information. Secondly, just "ip route show 192.168.0.29/32" will display if its tunnel or non-tunnel route.

@zjswhhh
Copy link
Copy Markdown
Contributor Author

zjswhhh commented Jul 6, 2022

I tent to disagree with this change. First of all, Nexthop ID doesn't convey any meaningful information. Secondly, just "ip route show 192.168.0.29/32" will display if its tunnel or non-tunnel route.

Hi Prince - Yes ip route show will have that information. It's just recently we have been doing static active-active (forcing both ToR to be active) on active-standby dualToRs, and to verify the tunnel route is removed, I first need to get server ip and then check the route. So I wanted to add a CLI to check in batches. But it's not a must-have.

@zjswhhh zjswhhh requested review from vdahiya12 and yxieca July 6, 2022 18:32
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jul 6, 2022

This pull request introduces 1 alert when merging 6609452 into c7389bd - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@zjswhhh zjswhhh force-pushed the asic_db_tunnel_route_master branch from 6609452 to 599215b Compare July 6, 2022 19:46
@zjswhhh zjswhhh force-pushed the asic_db_tunnel_route_master branch from 599215b to 86a9ba3 Compare July 6, 2022 19:48
@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Jul 6, 2022 •
edited
Loading

I tent to disagree with this change. First of all, Nexthop ID doesn't convey any meaningful information. Secondly, just "ip route show 192.168.0.29/32" will display if its tunnel or non-tunnel route.

Hi Prince - Yes ip route show will have that information. It's just recently we have been doing static active-active (forcing both ToR to be active) on active-standby dualToRs, and to verify the tunnel route is removed, I first need to get server ip and then check the route. So I wanted to add a CLI to check in batches. But it's not a must-have.

Agree, but was there an issue observed in transition that a tunnel route is not removed? I mean in active-active, there should not be any tunnel route. Just want to understand the motivation.

yxieca
yxieca previously approved these changes Jul 6, 2022
View reviewed changes
@zjswhhh
Copy link
Copy Markdown
Contributor Author

zjswhhh commented Jul 6, 2022 •
edited
Loading

I tent to disagree with this change. First of all, Nexthop ID doesn't convey any meaningful information. Secondly, just "ip route show 192.168.0.29/32" will display if its tunnel or non-tunnel route.

Hi Prince - Yes ip route show will have that information. It's just recently we have been doing static active-active (forcing both ToR to be active) on active-standby dualToRs, and to verify the tunnel route is removed, I first need to get server ip and then check the route. So I wanted to add a CLI to check in batches. But it's not a must-have.

Agree, but was there an issue observed in transition that a tunnel route is not removed? I mean in active-active, there should not be any tunnel route. Just want to understand the motivation.

There wasn't an issue like that. But show mux status is reflecting xcvrd/ycabled status only, and in current stage we don't care much about that, we want to have a quick way to confirm ToR is in active mode, in other words the tunnel route is removed.

prsunny
prsunny requested changes Jul 6, 2022
View reviewed changes
@zjswhhh zjswhhh force-pushed the asic_db_tunnel_route_master branch from aa7a2f8 to df94706 Compare July 7, 2022 00:18
prsunny
prsunny approved these changes Jul 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

lgtm

@zjswhhh zjswhhh merged commit df54138 into sonic-net:master Jul 7, 2022
@zjswhhh zjswhhh deleted the asic_db_tunnel_route_master branch July 7, 2022 17:39
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Jul 7, 2022
@dprital
[submodule] Update sonic-utilities submodule
6ee9933 
Update sonic-utilities submodule pointer to include the following:
* [show] add new CLI to show tunnel route objects ([sonic-net#2255](sonic-net/sonic-utilities#2255))
* Add support for IP interface loopback action ([sonic-net#2192](sonic-net/sonic-utilities#2192))
* Update load minigraph to load backend acl ([sonic-net#2236](sonic-net/sonic-utilities#2236))
* show linkmgrd status in  ([sonic-net#2254](sonic-net/sonic-utilities#2254))

Signed-off-by: dprital <drorp@nvidia.com>
@dprital dprital mentioned this pull request Jul 7, 2022
Closed
7 tasks
yxieca pushed a commit that referenced this pull request Jul 7, 2022
@zjswhhh @yxieca
[show] add new CLI to show tunnel route objects (#2255)
a0b04de 
What I did
Add new CLI support to show tunnel route objects in ASIC DB.
sign-off: Jing Zhang zhangjing@microsoft.com

How I did it
Check if tunnel route object exists for server_ipv4, server_ipv6, soc_ipv4. If existing, print it out.
If not specifying port name, print all tunnel route objects.

How to verify it
Added unit tests.
Tested on dual testbed.
@qiluo-msft
Copy link
Copy Markdown
Contributor

qiluo-msft commented Feb 1, 2023

This commit could not be cleanly cherry-picked to 202012. Please submit another PR.

zjswhhh added a commit to zjswhhh/sonic-utilities that referenced this pull request Feb 1, 2023
@zjswhhh
[show] add new CLI to show tunnel route objects (sonic-net#2255)
eb04d02 
What I did
Add new CLI support to show tunnel route objects in ASIC DB.
sign-off: Jing Zhang zhangjing@microsoft.com

How I did it
Check if tunnel route object exists for server_ipv4, server_ipv6, soc_ipv4. If existing, print it out.
If not specifying port name, print all tunnel route objects.

How to verify it
Added unit tests.
Tested on dual testbed.
@zjswhhh zjswhhh mentioned this pull request Feb 8, 2023
Merged
zjswhhh added a commit that referenced this pull request Feb 8, 2023
@zjswhhh
[202012][show] add new CLI to show tunnel route objects #2255 (#2659)
b3ebba2 
What I did
Add new CLI support to show tunnel route objects in APP DB.

sign-off: Jing Zhang zhangjing@microsoft.com

How I did it
Check if tunnel route object exists for server_ipv4, server_ipv6. If existing, print it out.
If not specifying port name, print all tunnel route objects.
How to verify it
Added unit tests.
Tested on dual testbeds:
show mux tunnel-route Ethernet140
 PORT         DEST_TYPE    DEST_ADDRESS    
 -----------  -----------  ---------------  
 Ethernet140  server_ipv4  192.168.0.29/32  
show mux tunnel-route --json Ethernet140
 {
     "TUNNEL_ROUTE": {
         "Ethernet140": {
             "server_ipv4": {
                 "DEST": "192.168.0.29/32"
             }
         }
     }
 }
@zjswhhh zjswhhh added the Cherry Pick PR created_202012 Label for a manually cherry-pick PR created for 202012 cherry-pick conflict PR. label Feb 8, 2023
@liuh-80
Copy link
Copy Markdown
Contributor

liuh-80 commented Mar 1, 2023

Cherry-pick PR merged: #2659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prsunny prsunny prsunny approved these changes

@yxieca yxieca yxieca approved these changes

@vdahiya12 vdahiya12 Awaiting requested review from vdahiya12

Assignees

No one assigned

Labels

Cherry Pick Conflict_202012 Cherry Pick PR created_202012 Label for a manually cherry-pick PR created for 202012 cherry-pick conflict PR. Included in 202012 Branch Included in 202205 Branch Request for 202012 Branch Request for 202205 Branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zjswhhh @prsunny @qiluo-msft @liuh-80 @yxieca