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

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

Merged
merged 9 commits into from
Jul 7, 2022

Conversation

zjswhhh
Copy link
Contributor

@zjswhhh zjswhhh commented Jul 6, 2022

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
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
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

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
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.

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
@zjswhhh
Copy link
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.

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.

show/muxcable.py Outdated Show resolved Hide resolved
@zjswhhh zjswhhh force-pushed the asic_db_tunnel_route_master branch from aa7a2f8 to df94706 Compare July 7, 2022 00:18
Copy link
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
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>
yxieca pushed a commit that referenced this pull request Jul 7, 2022
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
Contributor

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
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 added a commit that referenced this pull request Feb 8, 2023
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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants