Warm reboot for PortsOrch by qiluo-msft · Pull Request #551 · sonic-net/sonic-swss

qiluo-msft · 2018-07-27T02:38:20Z

The non-warm reboot behavior is backward compatible, and tested in lab.

The idea is best effort warm reboot based on left over entries in PORT_TABLE.

During a cold reboot, the whole table is empty, so keep original behavior
During a warm reboot, the previous port entries are left in the table, together with event entries such as PortConfigDone, PortInitDone. So PortsOrch will add them into PortsOrch's consumer queue to propagate to downstream syncd and ASIC.
During a warm reboot, if any corruption found in the table, clean up the table and fallback to cold reboot.

The warm reboot is not end-to-end tested.

lguohan · 2018-07-27T05:19:18Z

please fix vs test failure. #Resolved

lguohan · 2018-07-27T18:03:29Z

can you explain the idea in the commit message, it will be better for us to understand and do the review #Resolved

lguohan · 2018-07-27T18:05:33Z

portsyncd/portsyncd.cpp

    DBConnector cfgDb(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
    DBConnector appl_db(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
    DBConnector state_db(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
-    ProducerStateTable p(&appl_db, APP_PORT_TABLE_NAME);


why change to producer state table? #Resolved

Actually change to ProducerTable. The entries in APP_PORT_TABLE_NAME are sequence sensitive because there are event entries.

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

what event entries? #Resolved

PortConfigDone, PortInitDone

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

lguohan · 2018-07-27T18:31:37Z

orchagent/orch.cpp

 }

+// TODO: Table should be const
+void Orch::addExistingData(Table *table)


can you add one line description what addExistingData do? #Resolved

Actually it is in the header file.

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

lguohan · 2018-07-27T18:34:10Z

		notify(SUBJECT_TYPE_PORT_CHANGE, static_cast<void *>(&update));

change tab to spaces. #Resolved

Refers to: orchagent/portsorch.cpp:1163 in d363262. [](commit_id = d363262, deletion_comment = False)

jipanyang

Another change needed for portorch warm start is bool PortsOrch::initializePort(Port &p). If it is warm start, db oper_status should not be set down. We may use bool isWarmStart() common function as that for other orchagent as needed.

jipanyang · 2018-07-27T18:46:20Z

portsyncd/portsyncd.cpp

    DBConnector cfgDb(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
    DBConnector appl_db(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
    DBConnector state_db(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
-    ProducerStateTable p(&appl_db, APP_PORT_TABLE_NAME);


Could you do the ProducerStateTable to ProducerTable change in a separate PR? It is not strictly related to warm reboot. #Resolved

I could, however, this PR will depend on that new one. ProducerTable is correct choice for port table.

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

Let's start another PR for it. Please help #556

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

Your first suggestion is actually pretty good. Let's keep ProducerStateTable in this PR.

In reply to: 206734165 [](ancestors = 206734165,205866896)

jipanyang · 2018-07-27T18:50:09Z

orchagent/orch.cpp

+
+    while (it != m_consumerMap.end())
+    {
+        consumer = (Consumer*)(it->second.get());


The change overlap with part of #546 . One of the problem here is that other types of class may be in m_consumerMap too and they don't have getTableName() methods. casting them to Consumer may cause problem.
The other reason to add getName()/setName() for Executor is to facilitate debugging. #Resolved

Actually the name is inside ConsumerMap keys. Refine the code so the conversion is safe now.

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

jipanyang · 2018-07-27T18:56:16Z

orchagent/portsorch.cpp

+    if (!foundPortConfigDone || !foundPortInitDone)
+    {
+        SWSS_LOG_NOTICE("No port table, fallback to cold start");
+        cleanPortTable(keys);


There is #547 to have common function for all swss processes to check whether a process is doing warm start .
We agreed to discuss the schema design. The basic idea there is to assume that for cold start, the corresponding WARM_RESTART_TABLE entry cleared, then we create the entry and set restart_count to 0.
Every time the process is warm restarted, restart_count is incremented by 1.

Not sure about the purpose of cleanPortTable() here.

#Resolved

We could still discuss that design. No confliction here.

Not sure about the purpose of cleanPortTable() here.
If the redis leftover data for warm reboot is corrupted, clean up everything left and fallback to cold start.

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

jipanyang · 2018-07-30T04:46:36Z

orchagent/portsorch.cpp

+        return false;
+    }
+
+    addExistingData(m_portTable.get());


portorch also handling LAG, LAG member, vlan and vlan members:

addExistingData(db, APP_LAG_TABLE_NAME); addExistingData(db, APP_LAG_MEMBER_TABLE_NAME); addExistingData(db, APP_VLAN_TABLE_NAME); addExistingData(db, APP_VLAN_MEMBER_TABLE_NAME); #Resolved

jipanyang · 2018-07-30T06:17:58Z

orchagent/orch.h

    vector<Selectable*> getSelectables();

+    // add the existing table data (left by warm reboot) to the consumer todo task list.
+    bool addExistingData(Table *table);


There are quite a few places to use addExistingData() where only table name is available. It should be better to use table name directly. #Resolved

jipanyang · 2018-07-30T06:19:05Z

orchagent/portsorch.cpp

+    if (m_portCount != keys.size() - 2)
+    {
+        // Invalid port table
+        SWSS_LOG_ERROR("Invalid port table: m_portCount");


In what scenario will this case be hit? #Resolved

Redis database corrupted.

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

If redis database is corrupted, should not a whole system cold boot be performed instead of doing partial fix here and putting the system into unknown state? Supporting Individual process cold boot alone is another area. #Pending

It is another design choice, and I am ok with it.
If it will become our final decision, I am happy to iterate on it.

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

jipanyang · 2018-07-30T20:31:11Z

orchagent/portsorch.cpp

    Orch::addExecutor("PORT_STATUS_NOTIFICATIONS", portStatusNotificatier);
+
+    // Try warm start
+    bake();


Within PortsOrch::initializePort(Port &p), for warm start, oper_status should not be changed. #Resolved

I checked code in void PortsOrch::doPortTask(Consumer &consumer).

During warm start, m_portListLaneMap will have all existing ports from SAI query. So there is no new port created, and no new port initialized.

The function PortsOrch::initializePort(Port &p) will not be called.

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

Not sure if there is misunderstanding here, the warm restart testing I have done so far didn't skip PortsOrch::initializePort(Port &p). Even for cold start, m_portListLaneMap will have all the exiting physical ports. #Resolved

I am talking about this code snippet:

for (auto it = m_lanesAliasSpeedMap.begin(); it != m_lanesAliasSpeedMap.end();) { bool port_created = false; if (m_portListLaneMap.find(it->first) == m_portListLaneMap.end()) {

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

This is for port breakout. PortsOrch::initPort() still will be called. #Resolved

Thanks! Got your idea, and it is really a bug. I am using another PR to fix it. Please help #555

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

jipanyang · 2018-08-03T04:30:17Z

orchagent/orch.cpp

+}
+
+// TODO: Table should be const
+void Consumer::refillToSync(Table* table)


It looks to me if we remove "bool Orch::addExistingData(Table table)" and use bool Orch::addExistingData(const string& tableName) only,
void Consumer::refillToSync(Table table) is not needed and quite a few line of redundant code function may be eliminated. #Resolved

void Consumer::refillToSync(Table* table)

is used for optimization if we have a table object already.

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

jipanyang · 2018-08-03T04:45:55Z

orchagent/portsorch.cpp

+    }
+
+    doPortConfigDoneTask(tuples);
+    if (m_portCount != keys.size() - 2)


After "PortConfigDone", for some platform, port may be removed with removePort(it->second), could it cause this check invalid in case of warm start?

if (platform && (strstr(platform, BFN_PLATFORM_SUBSTRING) || strstr(platform, MLNX_PLATFORM_SUBSTRING))) { if (!removePort(it->second)) { throw runtime_error("PortsOrch initialization failure."); } } #Resolved

removePort() will never del entry in PORT_TABLE. So the answer is no.

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

In your change, doPortConfigDoneTask() is also called at construct phase,
having m_portConfigDone set prematurely will change the processing flow of void PortsOrch::doPortTask(Consumer &consumer).

/* Once all ports received, go through the each port and perform appropriate actions: * 1. Remove ports which don't exist anymore * 2. Create new ports * 3. Initialize all ports */ if (m_portConfigDone && (m_lanesAliasSpeedMap.size() == m_portCount)) <--- will be false before all of the ports handled. { } if (!m_portConfigDone) <------- m_portConfigDone is true. it continue the flow. { it = consumer.m_toSync.erase(it); <----- While if m_portConfigDone is false, the request will be deleted, not good for the one phase warm restore. continue; } ........ Port p; if (!getPort(alias, p)) { SWSS_LOG_ERROR("Failed to get port id by alias:%s", alias.c_str()); <--- will hit here, since initPort not called yet. }

Why do you want to mess with the port init orders? We might hit more subtle sequence problems.

This also reminds me that the previous switching from ProducerState/ConsumerState to Producer/Consumer for portTable might have some potential issue for cold boot,
the processing after "PortConfigDone" will not get all fields of the port and make the config like fec, AN void. The old ProducerState/ConsumerState happened to get all
fields of a port key every time. #Resolved

You are correct. I removed calling doPortConfigDoneTask in ctor. You are talking about other PR, please follow up in that PR page so I can follow up and iterate. Thanks!

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

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

jipanyang

Though I have different opinions on some of the function implementation details as listed in previous comments, I didn't see logical issue with current version, functionally it should work.

With the latest PortsOrch::doPortTask(), one of the side effects we probably missed is that:
After PortConfigDone, while waiting for "PortInitDone" and the first gBufferOrch->isPortReady(alias),
the complete m_lanesAliasSpeedMap may be populated again, so initPort() will be called more than once for the same port.
We may verify that via enabling INFO level debug, likely "SWSS_LOG_INFO("Port has already been initialized before alias:%s", alias.c_str());" will be hit. It is not fatal.

jipanyang · 2018-08-08T07:45:19Z

orchagent/portsorch.h

    map<string, Port>& getAllPorts();
+    bool bake();
+    void cleanPortTable(const vector<string>& keys);
+    void doPortConfigDoneTask(const vector<FieldValueTuple>& tuples);


Unused function signature. #Resolved

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

…nic-net#551) * [scripts]: add support to db_migrator for non-default unix socket * add '-s' option to db_migrator Signed-off-by: Lawrence Lee <[email protected]> * [scripts]: make db_migrator follow python convention * change variable names Signed-off-by: Lawrence Lee <[email protected]>

* Fix addExistingData consumer converstion * Add more addExistingData() * Warm reboot for PortsOrch * Remove calling doPortConfigDoneTask in ctor * Remove unused function signature

…t#551) Adding missing FABRIC_PORT_TABLE, COUNTERS_FABRIC_QUEUE_NAME_MAP and COUNTERS_FABRIC_PORT_NAME_MAP to schema.h Signed-off-by: Maxime Lorrillere [email protected] Signed-off-by: Maxime Lorrillere <[email protected]>

lguohan reviewed Jul 27, 2018

View reviewed changes

jipanyang reviewed Jul 27, 2018

View reviewed changes

jipanyang suggested changes Jul 30, 2018

View reviewed changes

jipanyang mentioned this pull request Jul 30, 2018

Warm reboot: Orchagent state restore #554

Merged

jipanyang suggested changes Jul 30, 2018

View reviewed changes

qiluo-msft force-pushed the qiluo/warmport branch 2 times, most recently from ebdaba9 to 27e0b6b Compare August 1, 2018 01:44

sonic-net deleted a comment from lguohan Aug 1, 2018

qiluo-msft force-pushed the qiluo/warmport branch from 27e0b6b to c394548 Compare August 3, 2018 00:28

jipanyang reviewed Aug 3, 2018

View reviewed changes

qiluo-msft force-pushed the qiluo/warmport branch 2 times, most recently from 41ac079 to 995b195 Compare August 8, 2018 00:01

qiluo-msft added 6 commits August 8, 2018 04:49

Warm reboot for PortsOrch

2ef7661

(comment)

dfaf72a

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

(reformat)

a45c5ee

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

Fix addExistingData consumer converstion

094da81

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

Add more addExistingData()

eb4d898

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

Remove calling doPortConfigDoneTask in ctor

2840988

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

qiluo-msft force-pushed the qiluo/warmport branch from 995b195 to 2840988 Compare August 8, 2018 04:49

jipanyang approved these changes Aug 8, 2018

View reviewed changes

qiluo-msft added 2 commits August 8, 2018 19:18

Remove unused function signature

a390b52

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

(add TODO comment)

f5c2b05

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

lguohan approved these changes Aug 8, 2018

View reviewed changes

qiluo-msft merged commit bfce363 into sonic-net:master Aug 8, 2018

qiluo-msft deleted the qiluo/warmport branch August 8, 2018 21:47

jipanyang mentioned this pull request Aug 8, 2018

Warm reboot: Extract the addToSync() function from Consumer::execute(), and new Or… #546

Closed

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

Update .gitignore (sonic-net#551)

e991281

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

lguohan Jul 27, 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 Jul 27, 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 Jul 27, 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 commented Jul 27, 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 Jul 27, 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!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Aug 8, 2018 • edited 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 Jul 27, 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 Jul 27, 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 Jul 30, 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 Jul 30, 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 Jul 30, 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 Jul 31, 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 Aug 3, 2018 • edited 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 Jul 27, 2018 •

edited

Loading

lguohan commented Jul 27, 2018 •

edited by qiluo-msft

Loading

lguohan commented Jul 27, 2018 •

edited by qiluo-msft

Loading

lguohan Jul 27, 2018 •

edited by qiluo-msft

Loading

lguohan Jul 27, 2018 •

edited by qiluo-msft

Loading

lguohan Jul 27, 2018 •

edited by qiluo-msft

Loading

lguohan commented Jul 27, 2018 •

edited by qiluo-msft

Loading

jipanyang Jul 27, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Aug 8, 2018 •

edited

Loading

jipanyang Jul 27, 2018 •

edited by qiluo-msft

Loading

jipanyang Jul 27, 2018 •

edited by qiluo-msft

Loading

jipanyang Jul 30, 2018 •

edited by qiluo-msft

Loading

jipanyang Jul 30, 2018 •

edited by qiluo-msft

Loading

jipanyang Jul 30, 2018 •

edited by qiluo-msft

Loading

jipanyang Jul 31, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Aug 3, 2018 •

edited

Loading

jipanyang Jul 30, 2018 •

edited by qiluo-msft

Loading

jipanyang Jul 31, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Jul 31, 2018 •

edited

Loading

jipanyang Jul 31, 2018 •

edited by qiluo-msft

Loading

jipanyang Aug 3, 2018 •

edited by qiluo-msft

Loading

jipanyang Aug 3, 2018 •

edited by qiluo-msft

Loading

jipanyang Aug 3, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Aug 4, 2018 •

edited

Loading

jipanyang Aug 8, 2018 •

edited by qiluo-msft

Loading