[sairedis]vs SAI support for voq neighbor #725
Conversation
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> Changes include SAI support for voq neighbor. VOQ neighbor requires an additional attribute to set the encap index. For local neighbors the encap index iis assigned by SAI, if not imposed. For remote neighbors, the encap index is required to be supplised by the upper layer. The changes validate these conditions for remote neighbors.
vslib/src/SwitchStateBase.cpp
Outdated
| return SAI_STATUS_FAILURE; | ||
| } | ||
|
|
||
| if(!is_local && encap_index == 0) |
There was a problem hiding this comment.
space after if, and remove empty line 2751
There was a problem hiding this comment.
Thanks. I'll fix them
| // The only requirement for the encap index is it must be locally | ||
| // unique in asic. Lower 32 bits of the ip address is used as encap index | ||
|
|
||
| encap_index = nbr_entry.ip_address.addr.ip4; |
There was a problem hiding this comment.
according to:
119 /**
120 * @brief Encapsulation Index
121 *
122 * Defines the neighbor's encapsulation index
123 *
124 * @type sai_uint32_t
125 * @flags CREATE_AND_SET
126 * @default 0
127 */
128 SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_INDEX,
if encap index is not provided default value is zero and not ipv4
There was a problem hiding this comment.
When we create local neighbors, we either set this to 0 or do not send this attribute at all. Imposing encap index is optional for local neighbors. In SAI, when we see encap index = 0 and if the neighbor is local SAI knows that it has to assign the encap index by itself. This is what I do in here. If it was remote neighbor, encap index = 0 is an error, because for remote neighbor a valid encap index must be imposed. This is what is done in bcm SAI code, which I also emulate here.
Please also note that using last 32 bits of ip address as the encap index is vs test thing. I just wanted to have a locally unique number. We can use any mechanism to get a unique number. I just chose IP address since under normal conditions, it is expected that host ip address attached to any given asic must be unique. In actual SAI, encap index will be a unique number starting from a base number representing the FEC (used in egress encapsulation, depending on platfrom).
There was a problem hiding this comment.
issue i have here is that this attribute is set internally, by headers definition this value is 0, so possibly when we create this neighbor, this value will be not zero, but current sonic code will expect zero, if attribute is not provided, seems like this default value should be set to "inernal" in SAI headers ?
There was a problem hiding this comment.
if user will pass this attribute would it be set to user value or also assigned internally ?
There was a problem hiding this comment.
The encap index is not set by user but assigned internally by SAI. The normal work flow is: When a neighbor is learnt, it is sent to SAI without encap index (since it is local). For VOQ switch, SAI assigns a unique encap index to the neighbor internally. The upper layer (orchagent), retrieves this encap index after neighbor is created and syncs the neighbor info including the retrieved encap index with other asics (via chassis app db in supervisor card). In other asics, this neighbor is remote neighbor. For VOQ switches, all asics should have equivalent neighbor record for every neighbor from other asics to have proper forwarding for hosts from different asics. So when an asic creates neighbor record for remote neighbors, it uses the same exact encap index synced and the encap index attribute is sent to SAI along with attributes for is local flag = false and impose index flag = true.
There was a problem hiding this comment.
Sorry. I gave the line #2834 after I rebased to resolve conflict. The correct line number (from commit 1) is #2732.
There was a problem hiding this comment.
but when it's at line 2759, it could still mean SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_IMPOSE_INDEX is true, and doing the encap_index assignment when (encap_index == 0) is not the intended usage of the SAI API.
There was a problem hiding this comment.
Yes. impose encap index == true and encap_index == 0 is invalid combination. We'll return error (based on check in line #2732) even before hitting #2759.
There was a problem hiding this comment.
Changed to check for impose encap index flag instead of ecap index value == 0 to decide on allocating unique encap index. This is to align with SAI API intended usage as suggested.
There was a problem hiding this comment.
need fixing: opencomputeproject/SAI#1165
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> Code review comments fix 1
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> Test case added for testing voq neighbor.
|
@eswaranb can you please review/approve this PR. |
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> Code review comments fix 2
Changes include SAI support for voq neighbor. VOQ neighbor requires an additional attribute to set the encap index. For local neighbors the encap index iis assigned by SAI, if not imposed. For remote neighbors, the encap index is required to be supplised by the upper layer. The changes validate these conditions for remote neighbors.
Changes include SAI support for voq neighbor. VOQ neighbor requires an additional attribute to set the encap index. For local neighbors the encap index iis assigned by SAI, if not imposed. For remote neighbors, the encap index is required to be supplised by the upper layer. The changes validate these conditions for remote neighbors.
Signed-off-by: vedganes vedavinayagam.ganesan@nokia.com
Changes include SAI support for voq neighbor. VOQ neighbor requires an
additional attribute to set the encap index. For local neighbors the
encap index is assigned by SAI, if not imposed. For remote neighbors,
the encap index is required to be supplied by the upper layer. The
changes validate these conditions for remote neighbors.