Dynamic port configuration - add port buffer cfg to the port ref counter #2194

dprital · 2022-03-23T17:50:06Z

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.

Details if related

dprital · 2022-03-23T22:42:20Z

/azpw run

mssonicbld · 2022-03-23T22:42:22Z

/AzurePipelines run

azure-pipelines · 2022-03-23T22:42:32Z

Azure Pipelines successfully started running 1 pipeline(s).

zhenggen-xu · 2022-03-24T04:50:49Z

tests/test_port_add_remove.py

+@pytest.mark.usefixtures('dvs_port_manager')
+class TestPortAddRemove(object):
+
+    def test_remove_port_with_buffer_cfg(self, dvs, testlog):


we should remove_port, add_port, then remove_port (port will with buffer_cfg, when remove you will remove buffer cfg first, and when you add, you will add buffer cfg after) to see if things are as expected. This is to explicitly excise the ref counters during those operations. maybe we can have a test case like "test_remove_add_remove_port_with_buffer_cfg" ?

@zhenggen-xu
to me, this is a negative test that verifies a port will not be removed if there is a buffer configuration on it. We should keep this test.
So i would like to have the test test_remove_add_remove_port_with_buffer_cfg like this:

when removing the port for the first time, remove port first and then the buffer configuration. verify the port will not be removed without buffer configuration cleared.

when adding the port, add the port first and then the buffer configuration, as you suggested.

when removing the port for the second time, remove the buffer configuration and then the port, as you suggested.
By doing so, we are able to verify both orders.

How do you think?

@stephenxs sounds good to me.

Updated test case. It's a bit different compared with what I suggested yesterday.
What I suggested:

for the first removing, remove the port and then buffer configuration

for the second removing, remove the buffer configuration and then port

What I implemented:

for the first removing, remove the buffer configuration and then the port

for the second removing, remove the port and then buffer configuration

Compared with the original approach, we do not need to check whether the buffer configuration has been successfully re-added after it was added back when the port was recreated: if it is not successfully added, the following port removing can succeed without removing buffer configuration, which will fail the test. Neither do we lose coverage.
@zhenggen-xu can you please review it?

Sure, looks good to me.

orchagent/bufferorch.cpp

dprital · 2022-03-24T14:31:46Z

/azpw run

liat-grozovik · 2022-03-24T14:51:39Z

/azp run Azure.sonic-swss

azure-pipelines · 2022-03-24T14:52:02Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-03-24T21:16:16Z

/azp run Azure.sonic-swss

azure-pipelines · 2022-03-24T21:16:21Z

Commenter does not have sufficient privileges for PR 2194 in repo Azure/sonic-swss

dprital · 2022-03-27T08:18:57Z

/azpw run

mssonicbld · 2022-03-27T08:18:59Z

/AzurePipelines run

azure-pipelines · 2022-03-27T08:19:08Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-03-27T18:07:34Z

/azpw run

mssonicbld · 2022-03-27T18:07:35Z

/AzurePipelines run

azure-pipelines · 2022-03-27T18:07:46Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-03-28T08:27:01Z

/azpw run

mssonicbld · 2022-03-28T08:27:03Z

/AzurePipelines run

azure-pipelines · 2022-03-28T08:27:13Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-03-28T16:47:29Z

/azpw run

mssonicbld · 2022-03-28T16:47:31Z

/AzurePipelines run

azure-pipelines · 2022-03-28T16:47:41Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-03-30T20:02:45Z

/azpw run

mssonicbld · 2022-03-30T20:02:46Z

/AzurePipelines run

azure-pipelines · 2022-03-30T20:02:57Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-03-31T05:09:09Z

/azpw run

mssonicbld · 2022-03-31T05:09:11Z

/AzurePipelines run

azure-pipelines · 2022-03-31T05:09:21Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2022-05-16T17:32:44Z

/AzurePipelines run

azure-pipelines · 2022-05-16T17:32:54Z

Azure Pipelines successfully started running 1 pipeline(s).

…ort ref counter sonic-net#2022

…d port

Signed-off-by: Stephen Sun <[email protected]>

liat-grozovik · 2022-06-14T07:10:25Z

/azp run Azure.sonic-swss

azure-pipelines · 2022-06-14T07:10:34Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2022-07-14T06:35:13Z

@dprital what is the status of this PR? do you have an ETA to fix the tests

dprital · 2022-08-23T23:30:37Z

@neethajohn , @zhenggen-xu - This is an old PR which was approved by you, as the checkers failed for some time, i fixed the test and now it pass. Would l ike to ask you to look at it (the code was not changed, only the test itself) and I would like you to approve so it will be able to be merged. Thanks in advance!

tests/test_port_add_remove.py

stephenxs

LGTM

liat-grozovik · 2022-08-29T13:24:00Z

Note: approved and merged following prev approval and only tests updates.
@dprital is it needed for 202205? if so please ensure it can be cherry picked and passing. if not creale new PR against 202205

dprital · 2022-08-29T14:12:45Z

Note: approved and merged following prev approval and only tests updates. @dprital is it needed for 202205? if so please ensure it can be cherry picked and passing. if not creale new PR against 202205

Needed for 202205 and can be taken.

Update sonic-swss submodule pointer to include the following: * Dynamic port configuration - add port buffer cfg to the port ref counter ([sonic-net#2194](sonic-net/sonic-swss#2194)) * tlm_teamd: Filter portchannel subinterface events from STATE_DB LAG_TABLE ([sonic-net#2408](sonic-net/sonic-swss#2408)) * [counters] Improve performance by polling only configured ports buffer queue/pg counters ([sonic-net#2360](sonic-net/sonic-swss#2360)) * added support for Xsight platform ([sonic-net#2426](sonic-net/sonic-swss#2426)) * [ci][asan] add DVS tests run with ASAN ([sonic-net#2363](sonic-net/sonic-swss#2363)) * Handle dual ToR neighbor miss scenario ([sonic-net#2151](sonic-net/sonic-swss#2151)) * Upstream new development on p4orch ([sonic-net#2237](sonic-net/sonic-swss#2237)) * [lgtm] Fix dependency ([sonic-net#2419](sonic-net/sonic-swss#2419)) * [muxorch] Returning true if nbr in skip_neighbor_ in isNeighborActive() ([sonic-net#2415](sonic-net/sonic-swss#2415)) * [macsec]: Set MTU for MACsec ([sonic-net#2398](sonic-net/sonic-swss#2398)) * Delete Invalid if condition in intfsorch.cpp ([sonic-net#2411](sonic-net/sonic-swss#2411)) Signed-off-by: dprital <[email protected]>

…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.

Update sonic-swss submodule pointer to include the following: * [BFD]Clean up state_db BFD entries on swss restart ([sonic-net#2434](sonic-net/sonic-swss#2434)) * Fix the Fec Mode Setting of gbsyncd ([sonic-net#2430](sonic-net/sonic-swss#2430)) * [neighsyncd] Enabling ipv4 link local entries for non-dualtor ([sonic-net#2427](sonic-net/sonic-swss#2427)) * tlm_teamd: Filter portchannel subinterface events from STATE_DB LAG_TABLE ([sonic-net#2408](sonic-net/sonic-swss#2408)) * PFCWD recovery changes using DLR_INIT ([sonic-net#2316](sonic-net/sonic-swss#2316)) * Dynamic port configuration - add port buffer cfg to the port ref counter ([sonic-net#2194](sonic-net/sonic-swss#2194)) Signed-off-by: dprital <[email protected]>

dprital requested a review from prsunny as a code owner March 23, 2022 17:50

dprital requested a review from zhenggen-xu March 23, 2022 17:51

dprital mentioned this pull request Mar 23, 2022

Dynamic port configuration - add port buffer cfg to the port ref counter #2022

Closed

zhenggen-xu reviewed Mar 24, 2022

View reviewed changes

dprital force-pushed the dynamic_port_config_buffers branch from a35cbfa to 5d262e4 Compare May 16, 2022 22:28

dprital and others added 4 commits May 26, 2022 18:33

Apply PR of Dynamic port configuration - add port buffer cfg to the p…

9b1e3b8

…ort ref counter sonic-net#2022

Update test due to comments: check admin status to set it back and ad…

eb5d3a1

…d port

Move { for line consistent due to comment on PR

51de623

Address comment: add a flow for port remove/add/remove

39aa90a

Signed-off-by: Stephen Sun <[email protected]>

dprital force-pushed the dynamic_port_config_buffers branch from 5d262e4 to 39aa90a Compare May 26, 2022 18:34

zhangyanzhao mentioned this pull request Jun 1, 2022

HLD document on add/remove ports dynamically feature sonic-net/SONiC#900

Merged

dprital added the Request for 202205 Branch label Jun 19, 2022

Fix test failures

0a5db6f

stephenxs reviewed Aug 24, 2022

View reviewed changes

tests/test_port_add_remove.py Outdated Show resolved Hide resolved

Update test due to review feedback, move mmu_set to common place

35b5dda

stephenxs approved these changes Aug 24, 2022

View reviewed changes

liat-grozovik merged commit a26b26a into sonic-net:master Aug 29, 2022

dprital mentioned this pull request Aug 29, 2022

[submodule] Advance sonic-swss pointer sonic-net/sonic-buildimage#11884

Closed

7 tasks

yxieca added the Included in 202205 Branch label Sep 1, 2022

dprital mentioned this pull request Sep 1, 2022

[202205][submodule] Advance sonic-swss pointer sonic-net/sonic-buildimage#11940

Merged

7 tasks

DavidZagury mentioned this pull request Sep 14, 2022

[sonic-swss] Update submodule sonic-net/sonic-buildimage#12075

Closed

7 tasks

dprital deleted the dynamic_port_config_buffers branch January 18, 2023 22:18

Dynamic port configuration - add port buffer cfg to the port ref counter #2194

Dynamic port configuration - add port buffer cfg to the port ref counter #2194

Conversation

dprital commented Mar 23, 2022

dprital commented Mar 23, 2022

mssonicbld commented Mar 23, 2022

azure-pipelines bot commented Mar 23, 2022

zhenggen-xu Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

stephenxs Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

zhenggen-xu Mar 29, 2022

Choose a reason for hiding this comment

stephenxs Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

zhenggen-xu Mar 31, 2022

Choose a reason for hiding this comment

dprital commented Mar 24, 2022

liat-grozovik commented Mar 24, 2022

azure-pipelines bot commented Mar 24, 2022

dprital commented Mar 24, 2022

azure-pipelines bot commented Mar 24, 2022

dprital commented Mar 27, 2022

mssonicbld commented Mar 27, 2022

azure-pipelines bot commented Mar 27, 2022

dprital commented Mar 27, 2022

mssonicbld commented Mar 27, 2022

azure-pipelines bot commented Mar 27, 2022

dprital commented Mar 28, 2022

mssonicbld commented Mar 28, 2022

azure-pipelines bot commented Mar 28, 2022

dprital commented Mar 28, 2022

mssonicbld commented Mar 28, 2022

azure-pipelines bot commented Mar 28, 2022

dprital commented Mar 30, 2022

mssonicbld commented Mar 30, 2022

azure-pipelines bot commented Mar 30, 2022

dprital commented Mar 31, 2022

mssonicbld commented Mar 31, 2022

azure-pipelines bot commented Mar 31, 2022

mssonicbld commented May 16, 2022

azure-pipelines bot commented May 16, 2022

liat-grozovik commented Jun 14, 2022

azure-pipelines bot commented Jun 14, 2022

liat-grozovik commented Jul 14, 2022

dprital commented Aug 23, 2022 • edited Loading

stephenxs left a comment

Choose a reason for hiding this comment

liat-grozovik commented Aug 29, 2022

dprital commented Aug 29, 2022

zhenggen-xu Mar 24, 2022 •

edited

Loading

stephenxs Mar 29, 2022 •

edited

Loading

stephenxs Mar 30, 2022 •

edited

Loading

dprital commented Aug 23, 2022 •

edited

Loading