-
Notifications
You must be signed in to change notification settings - Fork 510
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
Port configuration incremental update support #2305
Port configuration incremental update support #2305
Conversation
…ne may directly change APP DB
Conflicts: tests/mock_tests/mock_table.cpp
Conflicts: tests/mock_tests/Makefile.am
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
* We should not call any ip command if portOk=false. However, it is | ||
* valid to put port configuration to APP DB which will trigger port creation in kernel. | ||
*/ | ||
bool portOk = isPortStateOk(alias); |
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.
I dont think we need to change this? portsyncd does the handlePortConfigFromConfigDB
. So portmgrd can still wait and keep the current flow.
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 @prsunny , handlePortConfigFromConfigDB
is only called once at init stage. It cannot handle case like dynamic port breakout. So, I suppose we need keep this change. What do you think?
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.
ok. so the logic here retries for some part of the configuration (kernel), but write to app_db in the first place.
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.
yes
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.
My concern is, developer has to make sure kernel writes cannot be attempted if portOk is false which is different from the previous logic. Secondly, it keeps re-writing app_db during the wait period. We need to revisit this logic
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.
how about doing this in the if case
and keep the original logic.
for (auto &entry : field_values)
{
writeConfigToAppDb(alias, fvField(entry), fvValue(entry));
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.
First let me confirm your solution:
if (!isPortStateOk(alias))
{
SWSS_LOG_INFO("Port %s is not ready, pending...", alias.c_str());
for (auto& i : kfvFieldsValues(t))
{
writeConfigToAppDb(alias, fvField(i), fvValue(i));
}
it++;
continue;
}
// rest of the code keeps as original one
This solution makes code much clear and simple. But It will repeatedly put the configuration to APP DB until the port state turns "ok".
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.
But its the same with your current implementation as well, as you are looping through all retry attributes and writing to app_db, correct?
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.
no, for the retry process there are 3 cases:
- port turns ok, mtu / admin_status will be configured into kernel, other fields will not be put into APP DB again
- port is still not ok and mtu/admin_status were changed by user, it will only put the new mtu / admin_status to APP DB
- port is still not ok and mtu / admin_status were not changed, nothing will be put into APP DB
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.
How about this? You can use similar to 'configured' below. Apply it to APP_DB only first time in this else
part. I would like to simplify the code. I dont think we need to worry the 2nd case (port is still not ok and mtu/admin_status changed). The new values get anyways applied once the port is ready.
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.
changes lgtm, one comment to address
cfgmgr/portmgr.cpp
Outdated
for (auto &entry : field_values) | ||
{ | ||
writeConfigToAppDb(alias, fvField(entry), fvValue(entry)); | ||
SWSS_LOG_NOTICE("Configure %s %s to %s", alias.c_str(), fvField(entry).c_str(), fvValue(entry).c_str()); |
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.
i'm confused by this. It seems like we are anyways writing all the fields to APP DB, so why individual writes above like mtu, tpid?
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.
I will fix this
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.
Left a comment above to move this section to top. Also please have this as INFO logs as we don't want to keep logging during retries.
* We should not call any ip command if portOk=false. However, it is | ||
* valid to put port configuration to APP DB which will trigger port creation in kernel. | ||
*/ | ||
bool portOk = isPortStateOk(alias); |
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.
My concern is, developer has to make sure kernel writes cannot be attempted if portOk is false which is different from the previous logic. Secondly, it keeps re-writing app_db during the wait period. We need to revisit this logic
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
lgtm
@Junchao-Mellanox , the VS test failure is tracke by #2365. Please retry once this is fixed. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Update sonic-swss submodule pointer to include the following: * Port configuration incremental update support ([sonic-net#2305](sonic-net/sonic-swss#2305)) * [VS Test] Skip failing subport tests ([sonic-net#2370](sonic-net/sonic-swss#2370)) * [teammgr]: Waiting MACsec ready before doLagMemberTask ([sonic-net#2286](sonic-net/sonic-swss#2286)) * [vnetorch] [vxlanorch] fix a set of memory usage issues ([sonic-net#2352](sonic-net/sonic-swss#2352)) * [tests] [asan] add graceful stop flag ([sonic-net#2347](sonic-net/sonic-swss#2347)) * [asan] suppress the static variable leaks ([sonic-net#2354](sonic-net/sonic-swss#2354)) * Add support for IP interface loopback action ([sonic-net#2307](sonic-net/sonic-swss#2307)) * [orchagent]: srv6orch support for uSID ([sonic-net#2335](sonic-net/sonic-swss#2335)) Signed-off-by: dprital <drorp@nvidia.com>
Update sonic-swss submodule pointer to include the following: * Port configuration incremental update support ([#2305](sonic-net/sonic-swss#2305)) * [VS Test] Skip failing subport tests ([#2370](sonic-net/sonic-swss#2370)) * [teammgr]: Waiting MACsec ready before doLagMemberTask ([#2286](sonic-net/sonic-swss#2286)) * [vnetorch] [vxlanorch] fix a set of memory usage issues ([#2352](sonic-net/sonic-swss#2352)) * [tests] [asan] add graceful stop flag ([#2347](sonic-net/sonic-swss#2347)) * [asan] suppress the static variable leaks ([#2354](sonic-net/sonic-swss#2354)) * Add support for IP interface loopback action ([#2307](sonic-net/sonic-swss#2307)) * [orchagent]: srv6orch support for uSID ([#2335](sonic-net/sonic-swss#2335)) Signed-off-by: dprital <drorp@nvidia.com>
*portsyncd no longer handle port configuration change and put it to APP DB *Implement incremental configuration change in portmgr *Adjust portsorch to meet incremental configuration change requirement
*portsyncd no longer handle port configuration change and put it to APP DB *Implement incremental configuration change in portmgr *Adjust portsorch to meet incremental configuration change requirement
HLD: sonic-net/SONiC#985
What I did
Why I did it
How I verified it
Details if related