Adding vrrporch module #3315
Adding vrrporch module #3315
Conversation
|
@philo-micas @madhupalu please help review |
|
Adam,
Sure, my team is already on it
…On Thu, Oct 3, 2024 at 3:12 PM Adam Yeung ***@***.***> wrote:
@philo-micas <https://github.com/philo-micas> @madhupalu
<https://github.com/madhupalu> please help review
—
Reply to this email directly, view it on GitHub
<#3315 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALEIQNC2V7WRADAVYOV3NNDZZW6NRAVCNFSM6AAAAABPKHBD5WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJSGQZDSMRVGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
<https://aviznetworks.com/>
Madhu Paluru
Director, Engineering.
+1 408-455-5653
***@***.***
aviznetworks.com
[image: linkedin.png]
<https://www.linkedin.com/company/aviz-networks/> [image:
twitter.png] <https://twitter.com/aviznetworks> [image: youtube.png]
<https://www.youtube.com/channel/UCS0o0Lwv5M893eGpfAdOBNA>
|
|
|
||
| bool operator== (const vrrp_key_t& rhs) const | ||
| { | ||
| if (!(ip_addr == rhs.ip_addr) || port_name != rhs.port_name) |
There was a problem hiding this comment.
Shouldn't "!(ip_addr == rhs.ip_addr)" be "(ip_addr != rhs.ip_addr)" similar to other case?
There was a problem hiding this comment.
IpPrefix base class has operator == and no operator !=. Hence the code is in this fashion.
| @@ -186,6 +186,8 @@ bool OrchDaemon::init() | |||
| gDirectory.set(vnet_rt_orch); | |||
| VRFOrch *vrf_orch = new VRFOrch(m_applDb, APP_VRF_TABLE_NAME, m_stateDb, STATE_VRF_OBJECT_TABLE_NAME); | |||
| gDirectory.set(vrf_orch); | |||
| VrrpOrch *vrrp_orch = new VrrpOrch(m_applDb, APP_VRRP_TABLE_NAME); | |||
There was a problem hiding this comment.
APP_VRRP_TABLE_NAME name is not defined. It needs to be defined in common/schema.h.
There was a problem hiding this comment.
Will add APP_VRRP_TABLE_NAME in common/schema.h.
There was a problem hiding this comment.
Philo has already added it.
sonic-net/sonic-swss-common#813
| if (!hasSameIpAddr (request.getKeyString(0), request.getKeyIpPrefix(1))) | ||
| { | ||
| sai_status_t vip_status = sai_route_api->remove_route_entry(&unicast_route_entry); | ||
| if (vip_status != SAI_STATUS_SUCCESS) | ||
| { | ||
| SWSS_LOG_ERROR("Failed to delete Vrrp Vip on interface %s, rv:%d", | ||
| port.m_alias.c_str(), vip_status); | ||
| throw runtime_error("Failed to remove Vrrp Vip on interface"); | ||
| } | ||
| } | ||
|
|
||
| sai_status_t vmac_status = sai_router_intfs_api->remove_router_interface (vrrp_table_[key].rifid); | ||
| if (vmac_status != SAI_STATUS_SUCCESS) |
There was a problem hiding this comment.
If the same address exists, the route entry will not be deleted; however, the router interface below will be deleted, which may cause the route to be bound to a VRRP router interface id that has already been deleted.
There was a problem hiding this comment.
Same prefix will be present only in VRRP owner case. While adding VIP in owner case, only the RIF is added. Route entry would not be added as the same route will be added because of base interface.
Hence while deleting VIP in owner case, only the RIF will be deleted.
| attr.value.s32 = SAI_PACKET_ACTION_FORWARD; | ||
| vip_attrs.push_back(attr); | ||
| attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; | ||
| attr.value.oid = vrrp_rif_id; |
There was a problem hiding this comment.
@venkatmahalingam @vvbrcm @philo-micas is it expected to have next hop id as it's own RIF? I am suspecting this might not be correct as in my testing I found that the VRRP master is unable to forward the packet coming from Router C to the underlying VRRP host A and B as VRRP master doesn't know it's next hop.
cc: @madhupalu
There was a problem hiding this comment.
@vvbrcm did you test this reverse scenario where traffic is coming from the Internet via Router C?
There was a problem hiding this comment.
Yes, reverse traffic is tested.
For VRRP the VIP route entry will have VRRP router interface as its nexthop. This is needed to punt the dest IP=VIP packets to CPU.
In your case, do you have subnet route entry? This would be needed to punt any traffic dest to host behind the VRRP master.
Once the host traffic is punted to the CPU, ARP resolution should trigger and an arp entry should be added to H/W.
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
No pipelines are associated with this pull request. |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-swss |
|
/AzurePipelines run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
No pipelines are associated with this pull request. |
What I did
Adding vrrporch module
Why I did it
To listen to APPL_DB and program VRRP entities in SAI
How I verified it
UT
Details if related