Dynamic port configuration - add port buffer cfg to the port ref counter #2022
Conversation
…ed only when port ref counter is 0
…ed only when port ref counter is 0
|
@praveen-li I am unable to assign code review to you. Could you please add yourself as the reviewer? |
|
@prsunny - Can you please add @praveen-li as a reviewer ? |
|
Do we really need this PR since if buffer cfg is on the port, it should fail when remove port with See: https://github.com/Azure/sonic-swss/blob/master/orchagent/portsorch.cpp#L3312 |
in this way we will receive endless orchagent/SAI error messages: In the suggested fix we don't have these errors, and the ref count warning message is printed only once Also, I noticed that the SAI permits to remove the port even when there are few buffer_pg configurations, for example: |
|
@zhenggen-xu Can you please review and approve this PR ? |
@zhenggen-xu - Can you ? |
|
As mentioned in sonic-net/SONiC#900 (comment), please update there too. |
orchagent/bufferorch.cpp
Outdated
| * so we added a map that will help us to know what was the last command for this port and priority - | ||
| * if the last command was set command then it is a modify command and we dont need to increase the buffer counter | ||
| * all other cases (no last command exist or del command was the last command) it means that we need to increase the ref counter */ | ||
| if (op == SET_COMMAND) { |
There was a problem hiding this comment.
Please follow the same coding style, put the { in separate line in all your changes .
orchagent/bufferorch.cpp
Outdated
| * if the last command was set command then it is a modify command and we dont need to increase the buffer counter | ||
| * all other cases (no last command exist or del command was the last command) it means that we need to increase the ref counter */ | ||
| if (op == SET_COMMAND) { | ||
| if (queue_port_flags[port_name][ind] != SET_COMMAND) { |
There was a problem hiding this comment.
Do you see any issues when first time the queue_port_flags[port_name][ind] was not set yet?
There was a problem hiding this comment.
I dont think we have an issue on the first time - when a key not exist on a map and we try to read it, the entry is created with null value (https://stackoverflow.com/questions/10124679/what-happens-if-i-read-a-maps-value-where-the-key-does-not-exist) - so in the first time it will enter to this if (" if (queue_port_flags[port_name][ind] != SET_COMMAND)")
| polling_config = PollingConfig(polling_interval = 1, timeout = 5.00, strict = True)) | ||
|
|
||
| # verify that the port was removed properly since all buffer configuration was removed also | ||
| assert len(num) == num_of_ports - 1 |
There was a problem hiding this comment.
Add test case to add the port back?
There was a problem hiding this comment.
the port is added at the end of the test case
other test cases of delete/create ports will be in the VS test PR:
#2047
There was a problem hiding this comment.
We need a test case to add the port back and with buffer cfg?
Also, How does this PR going to merge with the other one since you share some of the code? You will need make the other PR (2047) depending on this one, or I would think we just put that PR's content here and get everything in one shot?
change "if" coding style
change "if" coding style
|
@zhenggen-xu kindly reminder to review recent changes following your feedback |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run Azure.sonic-swss |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…net#2022) - What I did For SYSTEM READY feature. Currently, there is a booting stage in system health service to indicate that the system is loading SONiC component. This booting stage is no longer needed because SYSTEM READY feature will treat that stage as system "NOT READY". - How I did it 1. Remove booting stage 2. Adjust unit test cases - How to verify it Manual test, Unit test, sonic-mgmt Regression
I don't see this is addressed "We need a test case to add the port back and with buffer cfg?", again, since this PR has ref counter, we do need test cases to increase/decrease the ref counters. |
|
Hi @zhenggen-xu |
…ter (#2194) - What I did This PR replace PR #2022 Added increasing/decreasing to the port ref counter each time a port buffer configuration is added or removed Implemented according to the - sonic-net/SONiC#900 - Why I did it In order to avoid cases where a port is removed before the buffer configuration on this port were removed also - How I verified it VS Test was added in order to test it. we remove a port with buffer configuration and the port is not removed. only after all buffer configurations on this port were removed - this port will be removed.
…ter (#2194) - What I did This PR replace PR #2022 Added increasing/decreasing to the port ref counter each time a port buffer configuration is added or removed Implemented according to the - sonic-net/SONiC#900 - Why I did it In order to avoid cases where a port is removed before the buffer configuration on this port were removed also - How I verified it VS Test was added in order to test it. we remove a port with buffer configuration and the port is not removed. only after all buffer configurations on this port were removed - this port will be removed.
What I did
I added increasing/decreasing to the port ref counter each time a port buffer configuration is added or removed
Implemented according to the - Dynamic Port Cfg HLD
Why I did it
In order to avoid cases where a port is removed before the buffer configuration on this port were removed also
How I verified it
I added a vs test that test it.
we remove a port with buffer configuration and the port is not removed. only after all buffer configurations on this port were removed - this port will be removed.
Details if related