[vnetorch]: Set default VxLAN encap TTL value#980
[vnetorch]: Set default VxLAN encap TTL value#980liat-grozovik merged 1 commit intosonic-net:masterfrom
Conversation
Signed-off-by: Volodymyr Samotiy <[email protected]>
|
Should be merged with sonic-net/sonic-mgmt#1017 |
prsunny
left a comment
prsunny
left a comment
There was a problem hiding this comment.
Changes looks good to me
stcheng
left a comment
stcheng
left a comment
There was a problem hiding this comment.
Could you update the unit test to verify the value is correctly set? Thanks.
| sai_ip_address_t *src_ip, | ||
| sai_object_id_t underlay_rif) | ||
| sai_object_id_t underlay_rif, | ||
| sai_uint8_t encap_ttl=0) |
There was a problem hiding this comment.
no need to specify the default value here, as already specified in the header file
There was a problem hiding this comment.
This is static helper function to create tunnel in SAI, so we need default value here (in header file default value specified for "create tunnel" public APIs)
| } | ||
|
|
||
| bool createTunnel(MAP_T encap, MAP_T decap); | ||
| bool createTunnel(MAP_T encap, MAP_T decap, uint8_t encap_ttl=0); |
There was a problem hiding this comment.
why do you set the default argument with value 0 instead of VXLAN_ENCAP_TTL?
There was a problem hiding this comment.
As Prince mentioned in another comment, VxLAN tunnel is created for different scenarios and for the VNET use-case we have default TTL set to 128 but for other use-cases all should work as before.
| tunnel_attrs.push_back(attr); | ||
| } | ||
|
|
||
| if (encap_ttl != 0) |
There was a problem hiding this comment.
since you're not exposing any places for user to change the encap_ttl value, why do you need to expose the capability of setting it via different functions while providing all the functions with default value? could you directly set the TTL mode to pipe model and the value to the default value?
There was a problem hiding this comment.
@stcheng , the VxlanTunnel is created for different scenarios. For the VNET use-case, we would want the Tunnel to have default 128 TTL. So I think it is better to pass this as parameter.
stcheng
left a comment
stcheng
left a comment
There was a problem hiding this comment.
thanks for the explanation
|
retest this please |
Signed-off-by: Volodymyr Samotiy <[email protected]>
Added ICMP Echo hardware offload session table names
4 participants
Signed-off-by: Volodymyr Samotiy [email protected]
What I did
Implemented logic to set default VxLAN encap TTL value
Why I did it
On different platforms default VxLAN encap TTL value is different, so we need to set same value for all
How I verified it
Executed "VNET" ansible test and verified it passed.
Details if related