-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Syncd graceful shutdown should not be ignored #14129
Syncd graceful shutdown should not be ignored #14129
Conversation
/azpw run Azure.sonic-buildimage |
/AzurePipelines run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-buildimage |
/AzurePipelines run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
sairedis PR has merged, pending submodule update PR #14235 |
Hi @saiarcot895, @qiluo-msft , could you please help review this? |
Hi @saiarcot895, @qiluo-msft , kindly reminder. |
@saiarcot895 @qiluo-msft would you please help to review? |
@@ -104,34 +104,32 @@ function stopplatform1() { | |||
debug "Stopped pmon service" | |||
fi | |||
|
|||
if [[ x$sonic_asic_platform != x"mellanox" ]] || [[ x$TYPE != x"cold" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @qiluo-msft , we intend to remove the whole if block because:
- if platform is not mellanox, it will trigger graceful shutdown flow no matter what type it is. It means that removing the condition won't affect any other vendor.
- if platform is mellanox, it will trigger graceful shutdown if type is not cold (old logic). This is exactly what we try to fix. We move the
type != cold
check to Syncd.cpp, see PR Ignore removing switch for mellanox platform due to known limitation sonic-sairedis#1216 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qiluo-msft would you please check the feedback from Junchao?
@Junchao-Mellanox IMO this fix is not relevant any more and the sairedis changes were reverted. |
Depends on sonic-net/sonic-sairedis#1216
Why I did this?
Due to a limitation in mellanox platform, sonic ignores syncs graceful shutdown in syncd.sh previously. However, ignoring graceful shutdown in syncd.sh also ignores closing other resources such as removing all counters.
This PR changes syncd shutdown flow for mellanox platform to make sure it execute graceful shutdown flow. There is another PR for sonic-sairedis to make sure mellanox platform does not call remove_switch for cold shutdown.
How I test this?
Running config reload on many platforms and check result.