Warm reboot: store FDB entries and warm start by qiluo-msft · Pull Request #615 · sonic-net/sonic-swss

qiluo-msft · 2018-09-12T02:19:21Z

Inspired by #558
The motivation of this PR:

orchagent should not directly read ASIC DB
orchagent should not assume that syncd recording FDB entries at all. It should store FDB entries based on observation on FDB notifications, and use them during warm start
We believe orchagent holds the ground truth of FDB entries during warm start
Orchagent will create_fdb_entry for dynamic ones, so sync don’t need special treatment on them during comparison logic
Orchagent will receive fdb notifications, mapping bridge_port_id to port name and store it in StateDB for warm restore
Orchagent will stop FDB learning/moving/aging before warm stop. (TBD in another PR)

Tested:

vs test: store FDB notification in StateDB
vs test: warm start with FDB in StateDB

lguohan · 2018-09-12T07:36:10Z

where do you use refreshFdbEntries? #Resolved

jipanyang

How are the FDB entries retstored? It looks FDB entries are saved in stateDB.

Could you separate code restructuring with the function implementation in general?

There are quite some changes not related to FDB restore.

jipanyang · 2018-09-12T21:41:24Z

orchagent/fdborch.cpp

 }

-void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_object_id_t bridge_port_id)
+void FdbOrch::refreshFdbEntries()


If it is for FDB reconciliation, separate PR should be prepared.
FDB reconciliation requires stale entry removal, which may be done in syncd implicitly or explicitly in orchagent. #Resolved

we plan to this reconciliation by disabling hw learn during warm-reboot on the asic. #Resolved

orchagent/orchdaemon.cpp

orchagent/fdborch.cpp

qiluo-msft · 2018-09-14T21:09:47Z

To answer @jipanyang's question

How are the FDB entries retstored? It looks FDB entries are saved in stateDB.
Could you separate code restructuring with the function implementation in general?
There are quite some changes not related to FDB restore.

The function refreshFdbEntries() will restore FDB from StateDB. The real actions are actually deserializing and update observing orchs.
Other code changes are just refactoring. #Closed

jipanyang · 2018-09-14T22:44:59Z

@qiluo-msft I didn't find the place where refreshFdbEntries() is called. Mixing code refactoring with new feature implementation makes it pretty hard to do code review.

@lguohan "we plan to this reconciliation by disabling hw learn during warm-reboot on the asic", in this case, unplanned warm restart won't work. swss won't be able to signal asic for unplanned restart. #Resolved

lguohan · 2018-09-17T07:43:27Z

@jipanyang , unplanned restart is not a goal here. It is very difficult to recover the system start from a crashed scenario. #Resolved

lguohan · 2018-09-17T07:46:09Z

@qiluo-msft , please indicate us to do the code review when you use the refreshfdbentries, both jipan and me asked the same question. not much to review for now. #Resolved

qiluo-msft · 2018-09-19T02:41:13Z

In latest iteration, all warm start logic is moved into bake(), which called by orchdaemon.

In reply to: 420544357 [](ancestors = 420544357)

qiluo-msft · 2018-09-19T02:42:00Z

In latest iteration, all warm start logic is moved into bake(), which called by orchdaemon. Refactoring removed.

In reply to: 421503216 [](ancestors = 421503216)

qiluo-msft · 2018-09-19T07:00:03Z

Please help review and meanwhile I will fix the unit test. #Closed

jipanyang · 2018-09-19T20:29:21Z

orchagent/orchdaemon.cpp

    }
    WarmStart::setWarmStartState("orchagent", WarmStart::RESTORED);
-    return true;
+    return ts.empty();


true is 1. "return !ts.empty();" or do further update? #Resolved

If ts empty, warm restore is successful.

In reply to: 218952757 [](ancestors = 218952757)

Got it, thanks #Resolved

orchagent/orchdaemon.cpp

jipanyang · 2018-09-19T21:09:47Z

orchagent/fdborch.cpp

+        std::vector<FieldValueTuple> fvs;
+        fvs.push_back(FieldValueTuple("port", portName));
+        fvs.push_back(FieldValueTuple("type", "dynamic"));
+        m_fdbStateTable.set(key, fvs);


If FDB entries are saved to stateDB. During system warm reboot, will stateDB be restored? #Resolved

Do you mean will stateDB in redis persistent during warm reboot? Yes, it should be.

Or do you mean if fdborch will restore from stateDB? Yes, restored in bake().

In reply to: 218964895 [](ancestors = 218964895)

stateDB will be cleared during state warm reboot. it is a problem. #Resolved

I will save the fdb entries in state db in the warm-reboot script and recover it in the going up path. @jipanyang , thanks for the catch. #Pending

Would it be easier if the data is put in a separate table in appDB so no special handling is needed for stateDB? #Resolved

I agree easier because of the implementation detail. The design of stateDB is for SWSS state, and applDB is very old which is originally designed for inter-process communication between SWSS applications. So I made the decision to store in stateDB.

BTW, there is already a FDB_TABLE (ProviderStateTable) in applDB for user custom FDB.

In reply to: 218993140 [](ancestors = 218993140)

jipanyang · 2018-09-20T02:47:46Z

tests/test_fdb.py

+                        [],
+                        [("SAI_FDB_ENTRY_ATTR_TYPE", "SAI_FDB_ENTRY_TYPE_DYNAMIC"),
+                         ("SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID", iface_2_bridge_port_id["Ethernet64"]),
+                        ]


Was the entry removed during orchagent restart? If not, probably other check like CRM counter value might be better. #Resolved

If you are asking entry in ASIC DB, no. orchagent warm start will create dynamic fdb, and syncd apply view will match it with old view, and keep it.
I will add CRM test.

In reply to: 219020615 [](ancestors = 219020615)

jipanyang · 2018-09-20T02:48:56Z

tests/test_fdb.py

+
+    finally:
+        # disable warm restart
+        # TODO: use cfg command to config it


"config warm_restart enable swss" is available. #Pending

I am following tests in test_warm_reboot.py. I will update later if those tests updated.

In reply to: 219020740 [](ancestors = 219020740)

jipanyang · 2018-09-20T02:51:01Z

tests/test_fdb.py

+
+    try:
+        # restart orchagent
+        dvs.runcmd(['sh', '-c', 'supervisorctl restart orchagent'])


Would it be better to do swss start/stop? If it is for warm restart testing, test_warm_reboot.py might be a good place for all related test cases.

# start processes in SWSS def start_swss(dvs): dvs.runcmd(['sh', '-c', 'supervisorctl start orchagent; supervisorctl start portsyncd; supervisorctl start intfsyncd; \ supervisorctl start neighsyncd; supervisorctl start intfmgrd; supervisorctl start vlanmgrd; \ supervisorctl start buffermgrd; supervisorctl start arp_update']) # stop processes in SWSS def stop_swss(dvs): dvs.runcmd(['sh', '-c', 'supervisorctl stop orchagent; supervisorctl stop portsyncd; supervisorctl stop intfsyncd; \ supervisorctl stop neighsyncd; supervisorctl stop intfmgrd; supervisorctl stop vlanmgrd; \ supervisorctl stop buffermgrd; supervisorctl stop arp_update']) ``` #Resolved

I change it to restart swss.
It is also for fdb test, so I keep it here.

In reply to: 219020965 [](ancestors = 219020965)

lguohan · 2018-09-21T00:46:48Z

orchagent/fdborch.cpp

+    string portName = port.m_alias;
+
+    // ref: https://github.com/Azure/sonic-swss/blob/master/doc/swss-schema.md#fdb_table
+    string key = "Vlan" + to_string(vlan_id) + ":" + mac.to_string();


: [](start = 48, length = 1)

should this be '|' #Resolved

Currently there is a trick. All entries stored in StateDB FDB_TABLE will be fed into ApplDB during warm starting. So I keep them the same pattern.

In reply to: 219357862 [](ancestors = 219357862)

I still feel it is more convenient to save the data in appDB using a new table like ASIC_FDB_TABLE, so to eliminate all the exception handling (stateDB save/restore during system warm reboot, and the special ":" separator in stateDB).

lguohan · 2018-09-21T08:33:48Z

orchagent/fdborch.cpp

+
+    size_t refilled = consumer->refillToSync(&m_fdbStateTable);
+    SWSS_LOG_NOTICE("Add warm input FDB State: %s, %zd", APP_FDB_TABLE_NAME, refilled);
+    return true;


does this call fdb notification? here the consumer is the app db? how can the fdb entries in state db got translated into fdb notificastions? #Resolved

Are you asking syncd receive this fdb entry and send orchagent notification? No.

Yes, the consumer is the app db

Never translated.

In reply to: 219421390 [](ancestors = 219421390)

Signed-off-by: Qi Luo <[email protected]>

…sumer Signed-off-by: Qi Luo <[email protected]>

Signed-off-by: Qi Luo <[email protected]>

Signed-off-by: Guohan Lu <[email protected]>

* Refactor Signed-off-by: Qi Luo <[email protected]> * Refactor Signed-off-by: Qi Luo <[email protected]> * Refactor Signed-off-by: Qi Luo <[email protected]> * Refactor Signed-off-by: Qi Luo <[email protected]> * Store fdb notification immediately after popping from NotificationConsumer Signed-off-by: Qi Luo <[email protected]> * Add syncUpFdb() Signed-off-by: Qi Luo <[email protected]> * Refactor Signed-off-by: Qi Luo <[email protected]> * Add vlan ping test Signed-off-by: Qi Luo <[email protected]> * Refine test: verify AsicDB and StateDB on fdb entries Signed-off-by: Qi Luo <[email protected]> * Refactor test * Add FDB state sync up and test Signed-off-by: Qi Luo <[email protected]> * Revert many refactoring, and store FDB notification with port/vlan info Signed-off-by: Qi Luo <[email protected]> * Fix test Signed-off-by: Qi Luo <[email protected]> * Refine macro name Signed-off-by: Qi Luo <[email protected]> * Add test on CRM counters Signed-off-by: Qi Luo <[email protected]> * Restart swss in test Signed-off-by: Qi Luo <[email protected]> * Fix bugs: increase iterator before erase, no double dec CRM counter Signed-off-by: Qi Luo <[email protected]> * Fix test Signed-off-by: Qi Luo <[email protected]> * Split fdb tests into 2 files Signed-off-by: Qi Luo <[email protected]>

jipanyang reviewed Sep 12, 2018

View reviewed changes

qiluo-msft force-pushed the qiluo/fdbstate branch from 4e6dd87 to 1be9cfc Compare September 19, 2018 02:36

sonic-net deleted a comment from lguohan Sep 19, 2018

jipanyang reviewed Sep 19, 2018

View reviewed changes

jipanyang reviewed Sep 20, 2018

View reviewed changes

lguohan reviewed Sep 21, 2018

View reviewed changes

qiluo-msft added 15 commits September 21, 2018 17:40

Refactor

961de03

Signed-off-by: Qi Luo <[email protected]>

Refactor

6918f8d

Signed-off-by: Qi Luo <[email protected]>

Refactor

8129570

Signed-off-by: Qi Luo <[email protected]>

Refactor

a3b436e

Signed-off-by: Qi Luo <[email protected]>

Store fdb notification immediately after popping from NotificationCon…

5c52b6c

…sumer Signed-off-by: Qi Luo <[email protected]>

Add syncUpFdb()

05e8e24

Signed-off-by: Qi Luo <[email protected]>

Refactor

5ff6401

Signed-off-by: Qi Luo <[email protected]>

Add vlan ping test

e92fd5f

Signed-off-by: Qi Luo <[email protected]>

Refine test: verify AsicDB and StateDB on fdb entries

c3f8a93

Signed-off-by: Qi Luo <[email protected]>

Refactor test

162aef9

Add FDB state sync up and test

b358021

Signed-off-by: Qi Luo <[email protected]>

Revert many refactoring, and store FDB notification with port/vlan info

25632a4

Signed-off-by: Qi Luo <[email protected]>

Fix test

a98a474

Signed-off-by: Qi Luo <[email protected]>

Refine macro name

41de268

Signed-off-by: Qi Luo <[email protected]>

Add test on CRM counters

9ffca45

Signed-off-by: Qi Luo <[email protected]>

qiluo-msft force-pushed the qiluo/fdbstate branch from 5953a91 to 9ffca45 Compare September 21, 2018 19:09

Restart swss in test

9b42775

Signed-off-by: Qi Luo <[email protected]>

lguohan approved these changes Sep 21, 2018

View reviewed changes

qiluo-msft added 4 commits September 22, 2018 01:36

Fix bugs: increase iterator before erase, no double dec CRM counter

9627c54

Signed-off-by: Qi Luo <[email protected]>

Fix test

4ba2adf

Signed-off-by: Qi Luo <[email protected]>

Merge branch 'master' into qiluo/fdbstate

e6f9235

Split fdb tests into 2 files

97eb09c

Signed-off-by: Qi Luo <[email protected]>

sonic-net deleted a comment from lguohan Sep 23, 2018

lguohan merged commit f1e1109 into sonic-net:master Sep 24, 2018

qiluo-msft deleted the qiluo/fdbstate branch September 24, 2018 17:56

qiluo-msft restored the qiluo/fdbstate branch September 24, 2018 17:56

stcheng added the Enhancement ➕ label Sep 25, 2018

qiluo-msft mentioned this pull request Sep 27, 2018

Warm reboot: FDB sync up #558

Closed

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023

[build]: support lgtm build for python library (sonic-net#615)

ebdf4ff

Signed-off-by: Guohan Lu <[email protected]>

Conversation

qiluo-msft commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lguohan commented Sep 12, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jipanyang left a comment

Choose a reason for hiding this comment

Uh oh!

jipanyang Sep 12, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan Sep 14, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

qiluo-msft commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jipanyang commented Sep 14, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lguohan commented Sep 17, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lguohan commented Sep 17, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiluo-msft commented Sep 19, 2018

Uh oh!

qiluo-msft commented Sep 19, 2018

Uh oh!

qiluo-msft commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jipanyang Sep 19, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jipanyang Sep 19, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jipanyang Sep 19, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan Sep 19, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan Sep 19, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jipanyang Sep 19, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jipanyang Sep 20, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jipanyang Sep 20, 2018 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft commented Sep 12, 2018 •

edited

Loading

lguohan commented Sep 12, 2018 •

edited by qiluo-msft

Loading

jipanyang Sep 12, 2018 •

edited by qiluo-msft

Loading

lguohan Sep 14, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Sep 14, 2018 •

edited

Loading

jipanyang commented Sep 14, 2018 •

edited by qiluo-msft

Loading

lguohan commented Sep 17, 2018 •

edited by qiluo-msft

Loading

lguohan commented Sep 17, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Sep 19, 2018 •

edited

Loading

jipanyang Sep 19, 2018 •

edited by qiluo-msft

Loading

jipanyang Sep 19, 2018 •

edited by qiluo-msft

Loading

jipanyang Sep 19, 2018 •

edited by qiluo-msft

Loading

lguohan Sep 19, 2018 •

edited by qiluo-msft

Loading

lguohan Sep 19, 2018 •

edited by qiluo-msft

Loading

jipanyang Sep 19, 2018 •

edited by qiluo-msft

Loading

jipanyang Sep 20, 2018 •

edited by qiluo-msft

Loading

jipanyang Sep 20, 2018 •

edited by qiluo-msft

Loading

jipanyang Sep 20, 2018 •

edited by qiluo-msft

Loading

lguohan Sep 21, 2018 •

edited by qiluo-msft

Loading

lguohan Sep 21, 2018 •

edited by qiluo-msft

Loading