[fast-reboot] Add a check for warmstart before cleaning up neigh table #1498

bingwang-ms · 2020-11-10T09:35:28Z

What I did
Fix sonic-net/sonic-buildimage#5841 and sonic-net/sonic-buildimage#5580

We found that neighbor table loaded by swssconfig from arp.json after fast-reboot is cleared by neighsyncd mistakenly at the initial stage. This PR adds a check for WarmStart before cleaning up, and only do that if WarmStart is enable.

Why I did it
This PR is to fix the issue that arp table is not recovered after fast-reboot.

How I verified it
Verified on Arista-7260, running 201911 image.

Run some test to populate ARP entries on DUT, such as test_fast_reboot
Issue a fast-reboot
Verify the arp.json backed up by fast-reboot-dump.py is loaded and NEIGH_TABLE is restored.

admin@str-7260cx3-acs-2:~$ redis-cli -n 0 keys '*NEIGH_TABLE*'
  1) "NEIGH_TABLE:Vlan1000:192.168.0.231"
  2) "NEIGH_TABLE:Vlan1000:192.168.0.106"
  3) "NEIGH_TABLE:Vlan1000:192.168.1.102"
  ....
509) "NEIGH_TABLE:Vlan1000:192.168.0.47"
510) "NEIGH_TABLE:Vlan1000:192.168.1.92"

py.test --inventory ../ansible/str,../ansible/veos --host-pattern str-7260cx3-acs-2 --module-path ../ansible --testbed vms7-t0-7260-2 --testbed_file ../ansible/testbed.csv --junit-xml=tr.xml --log-cli-level info --collect_techsupport=False --topology=t0,any,util platform_tests/test_advanced_reboot.py::test_fast_reboot
========================================================================================= test session starts =========================================================================================                                                                                                                        
platform_tests/test_advanced_reboot.py::test_fast_reboot 
------------------------------------------------------------------------------------------- live log setup --------------------------------------------------------------------------------------------
PASSED                                                                                                                                                                                          [100%]
------------------------------------------------------------------------------------------ live log teardown ------------------------------------------------------------------------------------------
===================================================================================== 1 passed in 404.24 seconds ======================================================================================

Details if related

…s enable. This commit is to address the issue that the NEIGH_TABLE loaded by swssconfig after fast-reboot is cleared by neighsyncd. Signed-off-by: bingwang <wang.bing@microsoft.com>

tahmed-dev · 2020-11-12T03:42:29Z

warmrestart/warmRestartAssist.cpp

-    psTable->clear();
+    if (m_warmStartInProgress)
+    {
+        psTable->clear();


Looks like this change will affect both neighsyncd and natsyncd. @lguohan is it OK to not clear the table for natsyncd when the dut is warm-rebooting? If not and to limit the change to limit neighsyncd, we can also check the table name to be neighsyncd

Wouldn't nat hit same issue if this protection is not there?

It is not clear if it is required for NAT tables to be cleared unconditionally or not. Needs a bit of digging into how NAT is using this shared class. NAT is using this method for 4 table here

This is a bug introduced by #1126 where the refactoring of the library to support multiple tables was calling this psTable->clear unconditionally. The way the fix doing here should be correct. NAT tables or any client should not use the library to flush producer state table in non warm-reboot cases.

btw: The reason we use psTable clear was to make sure the relevant table wouldn’t change in some corner cases after we dumped it to memory, this is required only in warm-reboot/restart case before we dump the table. Also, since the daemon using the library was the producer itself, it was safe to do so. However, this assumption was broken if we use swssconfig to load the table at the same time, which is the case for non warm-reboot cases for arp, nat tables etc. In those cases, the library cleared the requests from swssconfig incorrectly and cause the issues reported.

zhenggen-xu · 2020-11-18T01:08:34Z

warmrestart/warmRestartAssist.cpp

-    psTable->clear();
+    if (m_warmStartInProgress)
+    {
+        psTable->clear();


This is a bug introduced by #1126 where the refactoring of the library to support multiple tables was calling this psTable->clear unconditionally. The way the fix doing here should be correct. NAT tables or any client should not use the library to flush producer state table in non warm-reboot cases.

…s enable. (#1498) This commit is to address the issue that the NEIGH_TABLE loaded by swssconfig after fast-reboot is cleared by neighsyncd. **What I did** Fix sonic-net/sonic-buildimage#5841 and sonic-net/sonic-buildimage#5580 We found that neighbor table loaded by ```swssconfig``` from ```arp.json``` after ```fast-reboot``` is cleared by ```neighsyncd``` mistakenly at the initial stage. This PR adds a check for ```WarmStart``` before cleaning up, and only do that if ```WarmStart``` is enable. **Why I did it** This PR is to fix the issue that arp table is not recovered after fast-reboot. **How I verified it** Verified on Arista-7260, running 201911 image. 1. Run some test to populate ARP entries on DUT, such as ```test_fast_reboot``` 2. Issue a fast-reboot 3. Verify the ```arp.json``` backed up by ```fast-reboot-dump.py``` is loaded and NEIGH_TABLE is restored.

…s enable. (sonic-net#1498) This commit is to address the issue that the NEIGH_TABLE loaded by swssconfig after fast-reboot is cleared by neighsyncd. **What I did** Fix sonic-net/sonic-buildimage#5841 and sonic-net/sonic-buildimage#5580 We found that neighbor table loaded by ```swssconfig``` from ```arp.json``` after ```fast-reboot``` is cleared by ```neighsyncd``` mistakenly at the initial stage. This PR adds a check for ```WarmStart``` before cleaning up, and only do that if ```WarmStart``` is enable. **Why I did it** This PR is to fix the issue that arp table is not recovered after fast-reboot. **How I verified it** Verified on Arista-7260, running 201911 image. 1. Run some test to populate ARP entries on DUT, such as ```test_fast_reboot``` 2. Issue a fast-reboot 3. Verify the ```arp.json``` backed up by ```fast-reboot-dump.py``` is loaded and NEIGH_TABLE is restored.

Add a check for warm-restart, and do a clear only when warm-restart i…

cbfcc76

…s enable. This commit is to address the issue that the NEIGH_TABLE loaded by swssconfig after fast-reboot is cleared by neighsyncd. Signed-off-by: bingwang <wang.bing@microsoft.com>

bingwang-ms mentioned this pull request Nov 10, 2020

[fast-reboot] Fix arp entries will be cleared after swssconfig restores. sonic-net/sonic-buildimage#5842

Open

3 tasks

bingwang-ms requested a review from lguohan November 10, 2020 09:42

bingwang-ms added the request_for_201911_branch label Nov 10, 2020

bingwang-ms requested a review from yxieca November 10, 2020 09:43

liat-grozovik requested a review from tahmed-dev November 10, 2020 17:25

tahmed-dev reviewed Nov 12, 2020

View reviewed changes

tahmed-dev requested a review from zhenggen-xu November 17, 2020 19:18

zhenggen-xu approved these changes Nov 18, 2020

View reviewed changes

qiluo-msft approved these changes Nov 18, 2020

View reviewed changes

qiluo-msft merged commit fcb6c9d into sonic-net:master Nov 18, 2020

bingwang-ms mentioned this pull request Nov 19, 2020

[fast-reboot] ARP table is not restored after fast-reboot sonic-net/sonic-buildimage#5580

Closed

abdosi mentioned this pull request Dec 4, 2020

Merged latest 201911 to 201911-t0 sonic-net/sonic-buildimage#6133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fast-reboot] Add a check for warmstart before cleaning up neigh table #1498

[fast-reboot] Add a check for warmstart before cleaning up neigh table #1498

bingwang-ms commented Nov 10, 2020

tahmed-dev Nov 12, 2020

yxieca Nov 13, 2020

tahmed-dev Nov 13, 2020

zhenggen-xu Nov 18, 2020

zhenggen-xu Nov 18, 2020

zhenggen-xu Nov 18, 2020

[fast-reboot] Add a check for warmstart before cleaning up neigh table #1498

[fast-reboot] Add a check for warmstart before cleaning up neigh table #1498

Conversation

bingwang-ms commented Nov 10, 2020

tahmed-dev Nov 12, 2020

Choose a reason for hiding this comment

yxieca Nov 13, 2020

Choose a reason for hiding this comment

tahmed-dev Nov 13, 2020

Choose a reason for hiding this comment

zhenggen-xu Nov 18, 2020

Choose a reason for hiding this comment

zhenggen-xu Nov 18, 2020

Choose a reason for hiding this comment

zhenggen-xu Nov 18, 2020

Choose a reason for hiding this comment