Warm reboot: bring up ports for whole system warm reboot #653

qiluo-msft · 2018-10-23T02:27:27Z

The motivation here is to fix warm starting swss. Current implementation will

assume all netdev up
not set StateDB with state ok
not bring up the netdev

Actually SAI implementation will only create netdev, and keep them down

portsyncd should wait them created
portsyncd should set state ok in StateDB
portmgrd should bring them up

jipanyang · 2018-10-23T04:47:55Z

The existing implementation was targeting swss docker warm restart assuming stateDB and host netdev not touched.

As to system warm reboot, for going up path, we need to make sure syncd/libsai/SDK have finished their tasks ( one of tasks is to recreate host interfaces), probably syncd should set a flag in stateDB (maybe reuse WARM_RESTART_TABLE, state=reconciled). Then swss starts it tasks.

Since no real order control between portsyncd/portmgrd and orchagent, we should not re-apply the default config in port config file which may cause port state flapping and other unwanted consequence. While portmgrd has all up to date data for all config it had applied before shutdown, it is ok for it to replay the data.

As to portState in StateDB, it is an indication that netdev has been created. During warm reboot, libsai/SDK/syncd should have guaranteed that state, the easy way is to save and restore it during reboot. Note, swss needs the state reconciled signal from syncd in stateDB to proceed.

Just my 2cents about port state restore during system warm reboot. Of course there are other alternatives to the problem.
#Resolved

lguohan · 2018-10-23T09:12:15Z

tests/conftest.py

@@ -167,6 +167,9 @@ def __init__(self, name=None, keeptb=False):
                if ctn.id == ctn_sw_id or ctn.name == ctn_sw_id:
                    ctn_sw_name = ctn.name

+            # mount redis to base to unique directory
+            self.redis_sock = '/var/run/redis-vs' + '/' + "redis.sock"


can you rebase to master? #Pending

lguohan · 2018-10-23T09:15:27Z

host interface admin state (up or down) is not owned by sai/syncd/sdk, they cannot recover that. They will creates those interfaces in the kernel, but it is portmgrd's job to make sure they are in the correct admin state. #Resolved

lguohan · 2018-10-23T09:25:11Z

portsyncd/linksync.cpp

+        string cmd, res;
+        /* Bring down the existing kernel interfaces */
+        SWSS_LOG_INFO("Bring down old interface %s(%d)", key.c_str(), idx_p->if_index);
+        cmd = "ip link set " + key + " down";


in normal reboot (swss restart), it will see the previous netdev created in last run (syncd restart will recreate those interface). Therefore, it records all old interfaces and bring the down. In warm reboot (either swss docker restart or system reboot), the existing interface will be used, we should not bring them up. You can talk with prince on the previous normal restart flow. #Resolved

Ask to clarify, do you mean below?

In warm reboot (either swss docker restart or system reboot), the existing interface will be used, we should not bring them down.

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

lguohan · 2018-10-23T09:33:23Z

@jipanyang , I am not sure we need a flag to indicate syncd has finished it job.

after system reboot, syncd will start and wait for orchagent to all create_switch(). orchagent will start and call create_switch(). once create_switch is finished, SAI is finishing the warm reboot process. All SAI states are recovered and all host interfaces are created (not bring up). Then, portsyncd will enumerate all ports and put them into state db. portmgrd will bring them up.

In the code, bring the port down is an issue, I think he needs to figure out how to properly solve this one. But other than that, I think this is what you described, i do not see it is different from what qi propose. #Resolved

jipanyang · 2018-10-23T18:53:14Z

@lguohan @qiluo-msft
For the two processes: portsyncd and portmgrd, when they are up, logically we don't know whether libsai create_switch() has been finished or not, . There is no order control. Some mechanism is necessary to ensure that netdevs have been created, either some flag set by syncd or orchagent, or you check the existence of each netdev specifically.

Again, the default setting in port config file should not be applied again (as in portsyncd), it could cause ASIC port state flapping. We'll want to add more and more incremental update support for port attribute config in portmgrd process.

For port state in stateDB, either save and restore if we want to rely on the flag from lower layer, or portsyncd check the existence of each netdev and set the state again. Personally I prefer the first alternative, second alternative is unnecessarily complex and does't bring any extra benefit.
#Resolved

qiluo-msft · 2018-10-27T02:47:18Z

I fixed "the default setting in port config file should not be applied again". The only left one is admin_status, which I think it is safe.

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

jipanyang · 2018-10-27T06:53:26Z

How is it fixed?

Take autoneg as example: if its value is true in port_config.ini, and it was changed to false and applied to ASIC. After warm restart, with this change, autoneg may be enabled again in ASIC. #Resolved

jipanyang · 2018-10-27T07:06:40Z

Also there is possible race condition: since the RTM_NEWLINK message is triggered by libsai/SDK and you are relying on it to populated port state in stateDB, portsyncd may miss the message (no order control between syncd and portsyncd exists). #Resolved

qiluo-msft · 2018-10-30T06:13:26Z

Good point. I refine the code, and bypass processing port_config.ini.

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

qiluo-msft · 2018-10-30T06:17:36Z

Good point. I add netlink.dumpRequest in portsyncd to prevent any missing.
~~I may need a timing control to let portsyncd get PortInitDone, then let orchagent warm start (TBD).~~

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

qiluo-msft · 2018-10-30T06:25:03Z

Existing implementation lacks one feature: Although we restore ConfigDB, containing admin_status up for all ports, after whole system reboot, portmgrd doesn't bring up the netdev.

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

jipanyang · 2018-10-30T06:53:18Z

Existing implementation lacks one feature: Although we restore ConfigDB, containing admin_status up for all ports, after whole system reboot, portmgrd doesn't bring up the netdev.

The existing code "https://github.com/Azure/sonic-swss/blob/master/cfgmgr/portmgr.cpp#L122 " should work?

I think we do need to move the regular port config https://github.com/Azure/sonic-swss/blob/65b015b17a42013c988b871654f9b4edfb75e749/portsyncd/portsyncd.cpp#L149 to portmgrd, it could be done in separate PR. #Resolved

jipanyang · 2018-10-30T07:01:32Z

Good point. I add netlink.dumpRequest in portsyncd to prevent any missing.
I may need a timing control to let portsyncd get PortInitDone, then let orchagent warm start (TBD).

We had that at the very initial portsyncd/orchagent warm restart implementation, the conclusion at that time is we don't want to couple portsyncd and orchagent. Why is it needed now? Orchagent has all the data and is self consistent at least for state restore phase.

I remember seeing some issue with netlink.dumpRequest() for cold restart (probably with swss only cold restart), but could not recall the exact problem. We may keep an eye on it.
#Resolved

qiluo-msft · 2018-10-30T16:46:36Z

The code you mentioned is working if ConfigDB has admin_status, set it in ApplDB. However, in portsycnd, code will remove ConfigDB.
check code block under /* clear the init port config buffer */.

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

qiluo-msft · 2018-10-30T16:51:40Z

Currently portsyncd is designed with responsible for waiting all port 'state ok', in both cold start or warm start. Here no new couple coming. Orchagent need to wait all port 'state ok' to begin the warm start logic.

I remember seeing some issue with netlink.dumpRequest() for cold restart

Let me know if you recall, thanks!

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

jipanyang · 2018-10-30T18:35:32Z

During warm reboot, orchagent doesn't need the port ok signal from portsyncd. The way orchagent communicates with syncd and the timing of switch create in syncd (at the very begining https://github.com/Azure/sonic-sairedis/blob/d655d20f9ae1b9918fc17317a203046299eba19a/syncd/syncd_hard_reinit.cpp#L1259) has guaranteed the existence of host netdev.

It is preferred that portsyncd is able to warm restart independently (it is now) without bothering orchagent. #Resolved

qiluo-msft · 2018-10-30T23:17:10Z

During warm reboot, orchagent doesn't need the port ok signal from portsyncd. The way orchagent communicates with syncd and the timing of switch create in syncd (at the very begining https://github.com/Azure/sonic-sairedis/blob/d655d20f9ae1b9918fc17317a203046299eba19a/syncd/syncd_hard_reinit.cpp#L1259) has guaranteed the existence of host netdev.

You are exactly correct!
So this PR is good, I strikethrough previous wrong statements, thanks!
And it is true that

portsyncd is able to warm restart independently (it is now) without bothering orchagent.

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

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

jipanyang

Looks good to me.

…ynchronous mode (sonic-net#653) * Prevent syncd from stopping swss in the presence of SAI failures in synchronous mode * Remove TODO message * Update log message

lguohan reviewed Oct 23, 2018

View reviewed changes

stcheng added the Enhancement ➕ label Oct 24, 2018

qiluo-msft force-pushed the qiluo/warm_admin_status branch from f5e17fa to 4504893 Compare October 27, 2018 02:43

qiluo-msft changed the title ~~Remove isWarmStart check~~ Warm reboot: handle admin_status in portsyncd Oct 29, 2018

qiluo-msft closed this Oct 29, 2018

qiluo-msft reopened this Oct 30, 2018

qiluo-msft requested review from lguohan, pavel-shirshov, prsunny, stcheng and yxieca October 30, 2018 18:04

qiluo-msft changed the title ~~Warm reboot: handle admin_status in portsyncd~~ Warm reboot: bring up ports in portsyncd for whole system warm reboot Oct 30, 2018

qiluo-msft changed the title ~~Warm reboot: bring up ports in portsyncd for whole system warm reboot~~ Warm reboot: bring up ports for whole system warm reboot Oct 30, 2018

qiluo-msft added 6 commits October 31, 2018 19:16

Remove isWarmStart check

0f3a932

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

No bring down in warm start

4dda918

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Protect doPortTask to apply repeated SET once

859450f

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Refine setPortMtu

b3ccff0

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

(comment)

ef7d200

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Fix warm reboot: port state ok, port admin up, port autoneg ignore

15afe73

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>

qiluo-msft force-pushed the qiluo/warm_admin_status branch from ab3265f to 15afe73 Compare October 31, 2018 19:16

jipanyang approved these changes Oct 31, 2018

View reviewed changes

lguohan merged commit 2c83b68 into sonic-net:master Oct 31, 2018

qiluo-msft deleted the qiluo/warm_admin_status branch October 31, 2018 23:22

prsunny mentioned this pull request Nov 22, 2018

MTU not set correctly to hardware sonic-net/sonic-buildimage#2293

Closed

mint570 mentioned this pull request Sep 18, 2023

Internal m_portCount incorrect in portsorch #2904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warm reboot: bring up ports for whole system warm reboot #653

Warm reboot: bring up ports for whole system warm reboot #653

qiluo-msft commented Oct 23, 2018 •

edited

Loading

jipanyang commented Oct 23, 2018 •

edited by qiluo-msft

Loading

lguohan Oct 23, 2018 •

edited by qiluo-msft

Loading

lguohan commented Oct 23, 2018 •

edited by qiluo-msft

Loading

lguohan Oct 23, 2018 •

edited by qiluo-msft

Loading

qiluo-msft Oct 24, 2018

lguohan commented Oct 23, 2018 •

edited by qiluo-msft

Loading

jipanyang commented Oct 23, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Oct 27, 2018 •

edited

Loading

jipanyang commented Oct 27, 2018 •

edited by qiluo-msft

Loading

jipanyang commented Oct 27, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Oct 30, 2018

qiluo-msft commented Oct 30, 2018 •

edited

Loading

qiluo-msft commented Oct 30, 2018

jipanyang commented Oct 30, 2018 •

edited by qiluo-msft

Loading

jipanyang commented Oct 30, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Oct 30, 2018

qiluo-msft commented Oct 30, 2018 •

edited

Loading

jipanyang commented Oct 30, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Oct 30, 2018

jipanyang left a comment

Warm reboot: bring up ports for whole system warm reboot #653

Warm reboot: bring up ports for whole system warm reboot #653

Conversation

qiluo-msft commented Oct 23, 2018 • edited Loading

jipanyang commented Oct 23, 2018 • edited by qiluo-msft Loading

lguohan Oct 23, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

lguohan commented Oct 23, 2018 • edited by qiluo-msft Loading

lguohan Oct 23, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

qiluo-msft Oct 24, 2018

Choose a reason for hiding this comment

lguohan commented Oct 23, 2018 • edited by qiluo-msft Loading

jipanyang commented Oct 23, 2018 • edited by qiluo-msft Loading

qiluo-msft commented Oct 27, 2018 • edited Loading

jipanyang commented Oct 27, 2018 • edited by qiluo-msft Loading

jipanyang commented Oct 27, 2018 • edited by qiluo-msft Loading

qiluo-msft commented Oct 30, 2018

qiluo-msft commented Oct 30, 2018 • edited Loading

qiluo-msft commented Oct 30, 2018

jipanyang commented Oct 30, 2018 • edited by qiluo-msft Loading

jipanyang commented Oct 30, 2018 • edited by qiluo-msft Loading

qiluo-msft commented Oct 30, 2018

qiluo-msft commented Oct 30, 2018 • edited Loading

jipanyang commented Oct 30, 2018 • edited by qiluo-msft Loading

qiluo-msft commented Oct 30, 2018

jipanyang left a comment

Choose a reason for hiding this comment

qiluo-msft commented Oct 23, 2018 •

edited

Loading

jipanyang commented Oct 23, 2018 •

edited by qiluo-msft

Loading

lguohan Oct 23, 2018 •

edited by qiluo-msft

Loading

lguohan commented Oct 23, 2018 •

edited by qiluo-msft

Loading

lguohan Oct 23, 2018 •

edited by qiluo-msft

Loading

lguohan commented Oct 23, 2018 •

edited by qiluo-msft

Loading

jipanyang commented Oct 23, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Oct 27, 2018 •

edited

Loading

jipanyang commented Oct 27, 2018 •

edited by qiluo-msft

Loading

jipanyang commented Oct 27, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Oct 30, 2018 •

edited

Loading

jipanyang commented Oct 30, 2018 •

edited by qiluo-msft

Loading

jipanyang commented Oct 30, 2018 •

edited by qiluo-msft

Loading

qiluo-msft commented Oct 30, 2018 •

edited

Loading

jipanyang commented Oct 30, 2018 •

edited by qiluo-msft

Loading