[fabricportsorch] Add fabric support #1459

ngoc-do · 2020-10-07T21:17:50Z

This code is to add support for fabric asics and NPU with fabric ports enabled.

What I did

Create FabricOrchDaemon for fabric asics, instead of using OrchDaemon which is used for NPU.
Create FabricPortsOrch to manage fabric ports. It collects information about port state, peer switch id and peer lane (stored in STATE_DB), sets up port stats and queue states. In future, it will also be used to enable/disable erroneous fabric ports.
Fabric port and queue stats are setup to be collected via FlexCounters.

One thing to note here: The code is waiting for #1431 to start fabric flow for fabric asic/NPU with enabled fabric ports. So right now, there is no code that creates objects for FabricOrchDaemon/FabricPortsOrch yet. We need to know asic type (and if asic needs fabric ports to be enabled) to do that. These information should come from orchagent/main.cpp, and will be available after #1431 merges.

Why I did it

Initialize fabric asics and support traffic across multiple NPUs.

How I verified it

Tested in systems with fabric asics (BCM 88790) and NPU with fabric ports (BCM 88690).

Details if related

This code will require to update a few other things:

(1) syncd: to ask FlexCounter to collect fabric counters (sonic-net/sonic-sairedis#669)

(2) docker-orchagent: disable orchagent-portsyncd dependency. In this pull request, after fabric asic is initialized (by SAI), all fabric ports will be enabled by default. So portsyncd is not used for fabric asic at all, and orchagent should not wait for portsyncd (sonic-net/sonic-buildimage#5569)

This pull request doesn't include:

Cli commands to show portstat and queuestat for fabric ports. Typically fabric is used in multi-asic environment, and portstat, queuestat seem not to be supported for multi asic systems yet. We will add them later in next requests.

The implementation is based on the design doc proposed in sonic-net/SONiC#668.

ghost · 2020-10-07T21:18:04Z

All CLA requirements met.

lgtm-com · 2020-10-07T22:07:02Z

This pull request introduces 1 alert and fixes 1 when merging 6162bbe into e4dfb37 - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

eswaranb · 2020-10-28T17:47:33Z

orchagent/fabricportsorch.cpp

+
+        if (attr.value.booldata)
+        {
+            attr.id = SAI_PORT_ATTR_FABRIC_ATTACHED_SWITCH_ID;


Add the crc error collection here.

Can we detect if a link flapped between the polling period? Is that a count that can be maintained in STATE_DB?

I will add counter list, one counter for one port, that counts the number of times the link flaps. I also maintain the last time the link flaps.

CRC error collection will be added in another PR. It's because sonic-sairedis now doesn't understand attribute type which is used for getting fabric port error list. So we will need to modify sonic-sairedis, and then add CRC in sonic-swss.

Ok. Sounds good to me.

anshuv-mfst · 2020-11-04T17:29:14Z

@minionatwork (Suresh) - please add as reviewer.

lguohan · 2020-11-04T17:34:32Z

wonder why this pr does not have swss vs test cases added?

lguohan

please add swss vs tests.

ngoc-do · 2020-11-05T18:22:34Z

please add swss vs tests.

Sure, I will look into that.

lgtm-com · 2020-11-10T22:54:00Z

This pull request introduces 1 alert and fixes 1 when merging 4770917 into b9084a7 - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

lgtm-com · 2021-02-08T16:24:31Z

This pull request introduces 1 alert and fixes 1 when merging 91df50a into 189a964 - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

ngoc-do · 2021-02-09T16:59:13Z

please add swss vs tests.

Hello @lguohan: vstest for fabric ports has been updated now.

There are PRs in sonic-sairedis which merged to add support fabric ports in vslib - sonic-net/sonic-sairedis#737 and sonic-net/sonic-sairedis#769.

The above PRs merged, but their code doesn't get invoked until sonic-net/sonic-buildimage#6185 merges. That is to set up fabric lane mapping, and so to create fabric ports. Because if we merge it, the following tests will fail:

test_virtual_chassis.TestVirtualChassis.test_connectivity
test_virtual_chassis.TestVirtualChassis.test_voq_switch
test_virtual_chassis.TestVirtualChassis.test_chassis_app_db_sync
test_virtual_chassis.TestVirtualChassis.test_chassis_system_interface
test_virtual_chassis.TestVirtualChassis.test_chassis_system_neigh

Why is that? Because now vslib is fed with fabric lane mapping, it will create 16 fabric ports for VOQ asic. Together with 32 front panel ports, VOQ asic will have 32 + 16 = 48 ports (type SAI_OBJECT_TYPE_PORT). But the tests expect only 32 ports only. So they fail.

conftest.py:490: in check_swss_ready
    asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_PORT", num_ports + 1)  # +1 CPU Port

In this PR's swss tests, I updated the expected number of ports with fabric ports. But since sonic-net/sonic-buildimage#6185 can't merge, this PR also has the above test failures (in the opposite way: it expects 48 ports, but only 32 ports created - no fabric).

So one possible solution is that in this PR, I won't update the expected number with fabric ports (keep 32 only for front panel ones). So the tests won't fail. And we will merge it up first. Once it merges, we disable the tests, merge sonic-net/sonic-buildimage#6185, and then make another PR to reenable the tests. sonic-net/sonic-buildimage#6185 is fairly simple, so it won't take much down time for the tests.

Please let me know what you think?

abdosi · 2021-03-01T20:42:29Z

can you add design document link to this PR in description

abdosi · 2021-03-01T20:47:03Z

orchagent/fabricportsorch.cpp

+};
+
+static const vector<sai_queue_stat_t> queue_stat_ids =
+{


are these valid for Fabric ports ?

Yes, those are valid for fabric ports. But currently we are not able to query those stats yet until jupiter-sai completely supports them. I keep them here though.

Those stats will be setup in generateQueueStats(), which is currently empty. For reference purpose, generateQueueStats() will do the below thing.

For each port:

// Maintain queue map and install flex counters for queue stats vector<FieldValueTuple> laneQueueMap; for (size_t q = 0; q < m_queue_ids.size(); q ++) { std::ostringstream name; name << "FabricPort" << lane << ":" << q; laneQueueMap.emplace_back(name.str(), sai_serialize_object_id(m_queue_ids[q])); std::unordered_set<string> counter_stats; for (const auto& it: queue_stat_ids) { counter_stats.emplace(sai_serialize_queue_stat(it)); } queue_stat_manager.setCounterIdList(m_queue_ids[q], CounterType::QUEUE, counter_stats); } m_laneQueueCounterTable->set("", laneQueueMap);

I'll update generateQueueStats() to use queue_stat_ids once we have fully support from jupiter-sai.

abdosi · 2021-03-01T20:48:03Z

orchagent/fabricportsorch.cpp

+        Orch(appl_db, tableNames),
+        port_stat_manager(FABRIC_PORT_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ,
+                          FABRIC_PORT_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS, true),
+        queue_stat_manager(FABRIC_QUEUE_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ,


i assume this is for queue_stat_ids ? So again is this needed for fabric port.

Yes, once jupiter-sai supports queue stats query, queue_stat_manager is used to set up queue_stat_ids in FLEX_COUNTER_DB. syncd periodically does its job by reading those counters and updating COUNTERS_DB.

abdosi · 2021-03-01T20:49:49Z

orchagent/fabricportsorch.cpp

+    m_stateTable = unique_ptr<Table>(new Table(m_state_db.get(), FABRIC_PORT_TABLE));
+
+    m_counter_db = shared_ptr<DBConnector>(new DBConnector("COUNTERS_DB", 0));
+    m_laneQueueCounterTable = unique_ptr<Table>(new Table(m_counter_db.get(), COUNTERS_QUEUE_NAME_MAP));


what does COUNTERS_QUEUE_NAME_MAP and COUNTERS_QUEUE_PORT_MAP looks like for fabric port ?
Can you add comment in the description.

Once jupiter-sai supports port and queue stats completely for fabric ports on both fabric and voq asic, this will look like below in COUNTERS_DB:

127.0.0.1:6379[2]> select 2 OK

127.0.0.1:6379[2]> HGETALL "COUNTERS_QUEUE_PORT_MAP" 1) "FabricPort0" 2) "oid:0x401000000000006"

127.0.0.1:6379[2]> HGETALL "COUNTERS:oid:0x401000000000006" 1) "SAI_PORT_STAT_IF_OUT_FABRIC_DATA_UNITS" 2) "0" 3) "SAI_PORT_STAT_IF_IN_ERRORS" 4) "0" 5) "SAI_PORT_STAT_IF_IN_OCTETS" 6) "0" 7) "SAI_PORT_STAT_IF_IN_FABRIC_DATA_UNITS" 8) "0" 9) "SAI_PORT_STAT_IF_IN_FEC_SYMBOL_ERRORS" 10) "0" 11) "SAI_PORT_STAT_IF_OUT_OCTETS" 12) "0" 13) "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES" 14) "0" 15) "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES" 16) "0"

127.0.0.1:6379[2]> HGETALL "COUNTERS_QUEUE_NAME_MAP" 1) "FabricPort0:0" 2) "oid:0x4150000000000c6"

127.0.0.1:6379[2]> HGETALL "COUNTERS:oid:0x4150000000000c6" 1) "SAI_QUEUE_STAT_CURR_OCCUPANCY_LEVEL" 2) "0" 3) "SAI_QUEUE_STAT_WATERMARK_LEVEL" 4) "24" 5) "SAI_QUEUE_STAT_CURR_OCCUPANCY_BYTES" 6) "0"

abdosi · 2021-03-01T20:55:42Z

orchagent/orchdaemon.cpp

@@ -49,6 +50,7 @@ OrchDaemon::OrchDaemon(DBConnector *applDb, DBConnector *configDb, DBConnector *
        m_chassisAppDb(chassisAppDb)
 {
    SWSS_LOG_ENTER();
+    m_select = new Select();


what are we doing here ?

what are we doing here ?

ok got it. we have moved to base class for select object initialization

Yes, I had to move it to base class since it's used for both npu and fabric.

abdosi · 2021-03-01T20:56:00Z

orchagent/orchdaemon.cpp

@@ -68,6 +70,7 @@ OrchDaemon::~OrchDaemon()
    for(; it != m_orchList.rend(); ++it) {
        delete(*it);
    }
+    delete m_select;


same as previous what is this change for ?

ok got it. we have moved to base class for select object initialization

Yes, I had to move it to base class since it's used for npu and fabric too.

abdosi · 2021-03-01T21:09:49Z

orchagent/fabricportsorch.cpp

+
+    if (m_getFabricPortListDone)
+    {
+        updateFabricPortState();


can we add link to the PR description where these State DB fields are define. Also looks like current VS test case added as part of this do not cover state db changes.

Sure. I updated the description with the link.

The VS test already verifies the number of fabric ports in STATE_DB which is 16 fabric ports created for voq asic. Please look at the test part:

# Verify that fabric ports are monitored in STATE_DB if metadata.get('switch_type', 'npu') in ['voq', 'fabric']: self.get_state_db() self.state_db.wait_for_n_keys("FABRIC_PORT_TABLE", 16)

Below is what we see from a container of a linecard created by a vs test.

root@a5959e6e0a4a:/# sonic-db-cli STATE_DB HGETALL 'FABRIC_PORT_TABLE|PORT10' {'STATUS': 'down'}

In addition to the change in the test that verifies STATE_DB, sonic-net/sonic-sairedis#737 merged to vslib to create fabric ports for voq asics.

Note that we don't test fabric asics because it will require significant changes in the tests - all tests now assume testing asic is NPU with front panel ports.

abdosi

Overall LGTM. Have some comments for clarification.

ngoc-do · 2021-03-10T18:27:08Z

Hello @abdosi, could you please check my responses, and see if you can approve?

One question is actually here that I will need to comment out 5 failing tests. Are you OK with that?

The reason is that there are PRs in sonic-sairedis which merged to add support fabric ports in vslib - sonic-net/sonic-sairedis#737 and sonic-net/sonic-sairedis#769.

The above PRs merged, but their code doesn't get invoked until sonic-net/sonic-buildimage#6185 merges. That is to set up fabric lane mapping, and so to create fabric ports. Because if we merge it, the following tests will fail:

test_virtual_chassis.TestVirtualChassis.test_connectivity
test_virtual_chassis.TestVirtualChassis.test_voq_switch
test_virtual_chassis.TestVirtualChassis.test_chassis_app_db_sync
test_virtual_chassis.TestVirtualChassis.test_chassis_system_interface
test_virtual_chassis.TestVirtualChassis.test_chassis_system_neigh

Why is that? Because now vslib is fed with fabric lane mapping, it will create 16 fabric ports for VOQ asic. Together with 32 front panel ports, VOQ asic will have 32 + 16 = 48 ports (type SAI_OBJECT_TYPE_PORT). But the tests expect only 32 ports only. So they fail.

conftest.py:490: in check_swss_ready
asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_PORT", num_ports + 1) # +1 CPU Port

In this PR's swss tests, I updated the expected number of ports with fabric ports. But since sonic-net/sonic-buildimage#6185 can't merge, this PR also has the above test failures (in the opposite way: it expects 48 ports, but only 32 ports created - no fabric).

So the solution is I will disable the above tests, merge this PR, and then merge sonic-net/sonic-buildimage#6185, and then reenable the tests.

Does it sound good to you? If yes, I will add change to disable the tests in this PR.

ngoc-do · 2021-03-31T22:17:20Z

retest vs please

lgtm-com · 2021-04-02T06:13:26Z

This pull request introduces 1 alert and fixes 1 when merging e368411 into cba6576 - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

ngoc-do · 2021-04-02T17:45:09Z

OK, so with the latest commit, I just disabled checking fabric port in vs test, instead of disabling all failing tests. Voq code is still under testing.

When sonic-net/sonic-buildimage#6185 merges, I will just make another PR that changes FABRIC_NUM_PORTS = 16, and fabric code testing will be covered.

@lguohan @abdosi please review. Thanks.

lgtm-com · 2021-04-28T23:41:47Z

This pull request introduces 1 alert and fixes 1 when merging cbff2cb into ee7a735 - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

ngoc-do · 2021-04-29T17:37:36Z

/azpw run

Signed-off-by: ngocdo <[email protected]>

lgtm-com · 2021-04-30T19:37:12Z

This pull request introduces 1 alert and fixes 1 when merging 03d9e26 into ee7a735 - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

anshuv-mfst · 2021-05-03T21:42:20Z

Hi @lguohan - could you please review again, comments taken care off. Thanks

lgtm-com · 2021-05-11T01:50:06Z

This pull request introduces 1 alert and fixes 1 when merging f054f8b into e29d566 - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

ngoc-do · 2021-05-11T15:53:25Z

retest vs please

anshuv-mfst · 2021-05-12T04:59:15Z

Hi @abdosi - could you please initiate azp run.

ngoc-do · 2021-05-13T15:56:28Z

@abdosi

@abdosi Could you please help rerun the tests?

lgtm-com · 2021-05-14T17:02:25Z

This pull request introduces 1 alert and fixes 1 when merging 38ddc7a into 73ffd5f - view on LGTM.com

new alerts:

1 for Non-virtual destructor in base class

fixed alerts:

1 for Resource not released in destructor

ngoc-do · 2021-05-14T21:33:35Z

@lguohan I just resolved conflict one more time, and the tests got run again. All passed.

Could you please approach it again? I think it's great if we can merge this PR asap. This PR recently keeps getting conflicts (twice last week).

abdosi · 2021-05-14T21:36:55Z

@ngoc-do

When Azure/sonic-buildimage#6185 merges, I will just make another PR that changes FABRIC_NUM_PORTS = 16, and fabric code testing will be covered.

have you done this or will do ?

ngoc-do · 2021-05-14T21:47:58Z

@ngoc-do

When Azure/sonic-buildimage#6185 merges, I will just make another PR that changes FABRIC_NUM_PORTS = 16, and fabric code testing will be covered.

have you done this or will do ?

I will do this. Thanks for merging it up.

…7629) This PR is actually #6185 that was merged and then reverted to avoid test failure in swss. Now sonic-net/sonic-swss#1459 merged, and so this PR could merge to enable fabric test in swss. Signed-off-by: ngocdo <[email protected]>

…onic-net#7629) This PR is actually sonic-net#6185 that was merged and then reverted to avoid test failure in swss. Now sonic-net/sonic-swss#1459 merged, and so this PR could merge to enable fabric test in swss. Signed-off-by: ngocdo <[email protected]>

This code is to add support for fabric asics and NPU with fabric ports enabled. What I did Create FabricOrchDaemon for fabric asics, instead of using OrchDaemon which is used for NPU. Create FabricPortsOrch to manage fabric ports. It collects information about port state, peer switch id and peer lane (stored in STATE_DB), sets up port stats and queue states. In future, it will also be used to enable/disable erroneous fabric ports. Fabric port and queue stats are setup to be collected via FlexCounters.

lguohan requested a review from rlhui October 8, 2020 13:35

eswaranb reviewed Oct 28, 2020

View reviewed changes

lguohan added the chassis label Nov 4, 2020

lguohan requested changes Nov 4, 2020

View reviewed changes

ngoc-do force-pushed the fabric branch from 6162bbe to 4770917 Compare November 10, 2020 22:12

rlhui requested a review from abdosi November 10, 2020 23:53

This was referenced Dec 10, 2020

Add fabric lane mapping to vs Force10-S6000 sai.profile sonic-net/sonic-buildimage#6185

Merged

Add fabric port test to vslib. sonic-net/sonic-sairedis#737

Merged

ngoc-do force-pushed the fabric branch from 4770917 to 8c3e3e7 Compare February 8, 2021 08:24

abdosi reviewed Mar 1, 2021

View reviewed changes

abdosi requested changes Mar 1, 2021

View reviewed changes

ngoc-do force-pushed the fabric branch from 91df50a to e368411 Compare April 2, 2021 05:18

Disable checking fabric ports ready until sonic-buildimage#6185 merges.

03d9e26

Signed-off-by: ngocdo <[email protected]>

ngoc-do force-pushed the fabric branch from cbff2cb to 03d9e26 Compare April 30, 2021 17:28

eswaranb previously approved these changes May 3, 2021

View reviewed changes

lguohan previously approved these changes May 10, 2021

View reviewed changes

Merge branch 'master' into fabric

f054f8b

rlhui removed their request for review May 11, 2021 02:59

Merge branch 'master' into fabric

38ddc7a

ngoc-do dismissed stale reviews from lguohan and eswaranb via 38ddc7a May 14, 2021 16:09

abdosi approved these changes May 14, 2021

View reviewed changes

abdosi merged commit fb06c32 into sonic-net:master May 14, 2021

ngoc-do mentioned this pull request May 17, 2021

Add fabric lane mapping to vs (sai.profile) and HWSKU Force10-S6000 sonic-net/sonic-buildimage#7629

Merged

4 tasks

vganesan-nokia mentioned this pull request Jun 29, 2021

[VS test stability] Chassis test #1809

Open

ngoc-do mentioned this pull request Jun 29, 2021

[vstest] VS test failure fix after fabric port orch PR merge #1811

Merged

judyjoseph removed the chassis label Aug 10, 2021

[fabricportsorch] Add fabric support #1459

[fabricportsorch] Add fabric support #1459

Conversation

ngoc-do commented Oct 7, 2020 • edited Loading

ghost commented Oct 7, 2020 • edited by ghost Loading

lgtm-com bot commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshuv-mfst commented Nov 4, 2020

lguohan commented Nov 4, 2020

lguohan left a comment

Choose a reason for hiding this comment

ngoc-do commented Nov 5, 2020

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Feb 8, 2021

ngoc-do commented Feb 9, 2021 • edited Loading

abdosi commented Mar 1, 2021

Choose a reason for hiding this comment

ngoc-do Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdosi left a comment

Choose a reason for hiding this comment

ngoc-do commented Mar 10, 2021

ngoc-do commented Mar 31, 2021

lgtm-com bot commented Apr 2, 2021

ngoc-do commented Apr 2, 2021

lgtm-com bot commented Apr 28, 2021

ngoc-do commented Apr 29, 2021

lgtm-com bot commented Apr 30, 2021

anshuv-mfst commented May 3, 2021

lgtm-com bot commented May 11, 2021

ngoc-do commented May 11, 2021

anshuv-mfst commented May 12, 2021

ngoc-do commented May 13, 2021

lgtm-com bot commented May 14, 2021

ngoc-do commented May 14, 2021

abdosi commented May 14, 2021

ngoc-do commented May 14, 2021

ngoc-do commented Oct 7, 2020 •

edited

Loading

ghost commented Oct 7, 2020 •

edited by ghost

Loading

ngoc-do commented Feb 9, 2021 •

edited

Loading

ngoc-do Mar 3, 2021 •

edited

Loading