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

Question regarding SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL #8980

Closed
tbgowda opened this issue Sep 24, 2021 · 7 comments · Fixed by #9419
Closed

Question regarding SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL #8980

tbgowda opened this issue Sep 24, 2021 · 7 comments · Fixed by #9419

Comments

@tbgowda
Copy link
Contributor

tbgowda commented Sep 24, 2021

Hello,

From SAI spec, it looks like the above mentioned attribute is set to false for fast reboot.

    /**
     * @brief Uninitialize data plane upon removal of switch object
     *
     * Typical use case for tear down of the host adapter, is to remove the switch ID,
     * which will stop all data and control plane, as leaving data plane open without
     * control can be a security risk.
     * However, on some scenarios, such as fast boot, host adapter would like to set
     * this value to false, call remove switch, and have the data plane still running.
     *
     * @type bool
     * @flags CREATE_AND_SET
     * @default true
     */
    SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL,

From the Syncd implementation it looks like it is being sent only for Mellanox boxes.

https://github.com/Azure/sonic-sairedis/blob/f2075ffd339584d5250dfc66cd3dd3288d4ba3d5/syncd/Makefile.am#L71
https://github.com/Azure/sonic-sairedis/blob/1020de71b2f842396d87761a2bf7a725fbd0b8c1/syncd/Syncd.cpp#L4536

It isn't clear why it is being sent only for Mellanox and not for other vendors.

And there is an issue raised but with no conclusion : #4008

So, I am confused as to when, where and how will the above mentioned attribute be set. Any clarification is much appreciated.

@azure-pipelines-wrapper
Copy link

Thanks for opening this issue!

@kcudnik
Copy link
Contributor

kcudnik commented Sep 24, 2021

@yxieca can you explain ?

@yxieca
Copy link
Contributor

yxieca commented Sep 24, 2021

Interesting. I think maybe only Mellanox platform requires such indication? Other platforms' default behavior is to retain data plane.

@tbgowda
Copy link
Contributor Author

tbgowda commented Sep 24, 2021

should it be set to false for fast-reboot/warm-reboot irrespective of the platform type as suggested by SAI spec? Platforms whose default behavior is to retain the data plane, will do that anyway.

Context : We are running Sonic on our platform, we are exploring the usage of this attribute to see if it can be taken advantage of to reduce the convergence time for fast-reboot.

@tbgowda
Copy link
Contributor Author

tbgowda commented Sep 29, 2021

any more insight into this?

@prsunny prsunny transferred this issue from sonic-net/sonic-sairedis Oct 14, 2021
@lguohan
Copy link
Collaborator

lguohan commented Oct 28, 2021

so, the reason is that at that time, the capability query is not there. so, we made the macro. now i believe we can query the attribute if it is support or not. if yes, we call it, otherwise we do not call it. if cisco can make the code changes, msft can coordinate and make sure if won't cause regression on other platforms.

@tbgowda
Copy link
Contributor Author

tbgowda commented Nov 6, 2021

@lguohan , for clarification, are you suggesting we use SAI api : sai_query_attribute_capability to check if the attribute above is supported or not and then call make the subsequent call to set it?

yxieca pushed a commit that referenced this issue Feb 1, 2022
Why I did it
Fixes #8980 partly.

The corresponding changes in sonic-sairedis is here :
sonic-net/sonic-sairedis#975

How I did it
Include changes from both repos and build an image for verification.

How to verify it
Trigger fast-reboot with the changes, see the attribute SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL being set at the SAI level.

Signed-off-by: Thushar Gowda <24815472+tbgowda@users.noreply.github.com>
qiluo-msft pushed a commit that referenced this issue Feb 8, 2022
Why I did it
Fixes #8980 partly.

The corresponding changes in sonic-sairedis is here :
sonic-net/sonic-sairedis#975

How I did it
Include changes from both repos and build an image for verification.

How to verify it
Trigger fast-reboot with the changes, see the attribute SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL being set at the SAI level.

Signed-off-by: Thushar Gowda <24815472+tbgowda@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants