[dual-tor] add missing SAI attribte in order to create IPNIP tunnel #2503
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ayurkiv-nvda
force-pushed
the
ip_n_ip_dualtor_attr_forked_public
branch
from
13170d5 to
3274baf
Compare
prsunny
reviewed
Nov 3, 2022
| @@ -212,6 +212,10 @@ static sai_object_id_t create_tunnel( | |||
| attr.value.s32 = SAI_TUNNEL_TTL_MODE_PIPE_MODEL; | |||
| tunnel_attrs.push_back(attr); | |||
|
|
|||
| attr.id = SAI_TUNNEL_ATTR_DECAP_TTL_MODE; | |||
There was a problem hiding this comment.
This tunnel is used only as nexthop tunnel for encap purposes. Why is this required here?
There was a problem hiding this comment.
Regarding SAI_TUNNEL_ATTR_DECAP_TTL_MODE
Problem:
Long time ago this attribute was defined as “mandatory on create” in SAI API, but at some point it was changed to “optional”.
SONiC “dual ToR” implementation, when it creates IP-in-IP tunnel, does not provide this attribute because now it is “optional”.
SAI team reported:
"We only support pipe, which is not the default API value of uniform, so user must provide explicit pipe
This has always been the ip in ip behavior and limitation, which is also documented in our RN"
So it is MLNX SAI implementation limitation that we support only “pipe” and since default value for this attribute is “uniform” we need to set it explicitly.
And in our case it is becomes “mandatory on create” attribute which must be set to “pipe” value.
As a result of this limitation we should update SONiC “Dual ToR” code in order to pass this attribute on IP-in-IP tunnel creation.
prsunny
previously approved these changes
Nov 9, 2022
bingwang-ms
previously approved these changes
Nov 10, 2022
|
LGTM. I need to watch the test result of master image to confirm the extra attribute does't cause issue. |
Signed-off-by: Andriy Yurkiv <ayurkiv@nvidia.com>
ayurkiv-nvda
dismissed stale reviews from bingwang-ms and prsunny
via
6b9e6a1
ayurkiv-nvda
force-pushed
the
ip_n_ip_dualtor_attr_forked_public
branch
from
3274baf to
6b9e6a1
Compare
|
@bingwang-ms @prsunny it looks like after branch alignment with latest master all checks passed successfully. |
bingwang-ms
approved these changes
Nov 23, 2022
|
can someone merge this PR to upstream? |
yxieca
pushed a commit
that referenced
this pull request
Nov 29, 2022
…2503) - What I did add SAI_TUNNEL_ATTR_DECAP_TTL_MODE and SAI_TUNNEL_ATTR_DECAP_DSCP_MODE - Why I did it Need to pass additional SAI attrib. Align encap end decap params 1 SAI_TUNNEL_ATTR_DECAP_DSCP_MODE Problem: Long time ago this attribute was defined as “mandatory on create” in SAI API, but at some point it was changed to “optional”. But MLNX SAI implementation didn’t change it and still expects this attribute to be always provided on tunnel creation. SONiC “dual ToR” implementation, when it creates IP-in-IP tunnel, does not provide this attribute because it is “optional”. So we get SAI error for creating tunnel because as mentioned above MLNX SAI implementation still expects this attribute as “mandatory on create”. Need to always set this attribute in “Dual ToR orchagent” on IP-in-IP tunnel creation 2 SAI_TUNNEL_ATTR_DECAP_TTL_MODE Problem: Long time ago this attribute was defined as “mandatory on create” in SAI API, but at some point it was changed to “optional”. SONiC “dual ToR” implementation, when it creates IP-in-IP tunnel, does not provide this attribute because now it is “optional”. SAI team reported: "We only support pipe, which is not the default API value of uniform, so user must provide explicit pipe This has always been the ip in ip behavior and limitation, which is also documented in our RN" So it is MLNX SAI implementation limitation that we support only “pipe” and since default value for this attribute is “uniform” we need to set it explicitly. And in our case it is becomes “mandatory on create” attribute which must be set to “pipe” value. As a result of this limitation we should update SONiC “Dual ToR” code in order to pass this attribute on IP-in-IP tunnel creation. - How I verified it tunnel successfully created Signed-off-by: Andriy Yurkiv <ayurkiv@nvidia.com>
StormLiangMS
pushed a commit
that referenced
this pull request
Mar 7, 2023
…2503) - What I did add SAI_TUNNEL_ATTR_DECAP_TTL_MODE and SAI_TUNNEL_ATTR_DECAP_DSCP_MODE - Why I did it Need to pass additional SAI attrib. Align encap end decap params 1 SAI_TUNNEL_ATTR_DECAP_DSCP_MODE Problem: Long time ago this attribute was defined as “mandatory on create” in SAI API, but at some point it was changed to “optional”. But MLNX SAI implementation didn’t change it and still expects this attribute to be always provided on tunnel creation. SONiC “dual ToR” implementation, when it creates IP-in-IP tunnel, does not provide this attribute because it is “optional”. So we get SAI error for creating tunnel because as mentioned above MLNX SAI implementation still expects this attribute as “mandatory on create”. Need to always set this attribute in “Dual ToR orchagent” on IP-in-IP tunnel creation 2 SAI_TUNNEL_ATTR_DECAP_TTL_MODE Problem: Long time ago this attribute was defined as “mandatory on create” in SAI API, but at some point it was changed to “optional”. SONiC “dual ToR” implementation, when it creates IP-in-IP tunnel, does not provide this attribute because now it is “optional”. SAI team reported: "We only support pipe, which is not the default API value of uniform, so user must provide explicit pipe This has always been the ip in ip behavior and limitation, which is also documented in our RN" So it is MLNX SAI implementation limitation that we support only “pipe” and since default value for this attribute is “uniform” we need to set it explicitly. And in our case it is becomes “mandatory on create” attribute which must be set to “pipe” value. As a result of this limitation we should update SONiC “Dual ToR” code in order to pass this attribute on IP-in-IP tunnel creation. - How I verified it tunnel successfully created Signed-off-by: Andriy Yurkiv <ayurkiv@nvidia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Andriy Yurkiv ayurkiv@nvidia.com
What I did
add SAI_TUNNEL_ATTR_DECAP_TTL_MODE and SAI_TUNNEL_ATTR_DECAP_DSCP_MODE
Why I did it
Need to pass additional SAI attrib. Align encap end decap params
1 SAI_TUNNEL_ATTR_DECAP_DSCP_MODE
Problem:
Long time ago this attribute was defined as “mandatory on create” in SAI API, but at some point it was changed to “optional”.
But MLNX SAI implementation didn’t change it and still expects this attribute to be always provided on tunnel creation.
SONiC “dual ToR” implementation, when it creates IP-in-IP tunnel, does not provide this attribute because it is “optional”.
So we get SAI error for creating tunnel because as mentioned above MLNX SAI implementation still expects this attribute as “mandatory on create”.
Need to always set this attribute in “Dual ToR orchagent” on IP-in-IP tunnel creation
2 SAI_TUNNEL_ATTR_DECAP_TTL_MODE
Problem:
Long time ago this attribute was defined as “mandatory on create” in SAI API, but at some point it was changed to “optional”.
SONiC “dual ToR” implementation, when it creates IP-in-IP tunnel, does not provide this attribute because now it is “optional”.
SAI team reported:
"We only support pipe, which is not the default API value of uniform, so user must provide explicit pipe
This has always been the ip in ip behavior and limitation, which is also documented in our RN"
So it is MLNX SAI implementation limitation that we support only “pipe” and since default value for this attribute is “uniform” we need to set it explicitly.
And in our case it is becomes “mandatory on create” attribute which must be set to “pipe” value.
As a result of this limitation we should update SONiC “Dual ToR” code in order to pass this attribute on IP-in-IP tunnel creation.
How I verified it
tunnel successfully created
Details if related