[swss]: Restore cached FDB and ARP entries after config reload for dual ToR #6912

theasianpianist · 2021-02-26T19:42:01Z

Only performed after config reload on dual ToR systems

In conjuction with: sonic-net/sonic-utilities#1465
Signed-off-by: Lawrence Lee lawlee@microsoft.com

Why I did it

On dual ToR systems, we need to quickly restore the ARP table after config reload so the standby ToR is able to continue normal operation.

How I did it

Check for the /host/config-reload directory, which is created by the linked PR. If found, copy the ARP and FDB caches to SWSS. Restore FDB entries to APP_DB. Then wait for the VLAN interface to come up, and restore ARP table to the kernel.

How to verify it

Ran config reload -y on a dual ToR testbed including changes from the linked PR, confirmed that ARP table on standby ToR was restored after.

Which release branch to backport (provide reason below if selected)

201811
201911
202006
202012

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

* Only performed after config reload on dual ToR systems Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

* Only runs on dual ToR systems after config reload Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

prsunny · 2021-03-01T16:37:36Z

dockers/docker-orchagent/kernel_arp_restore.sh

+    INTF_NAME=$1
+
+    until [[ `ip link show $INTF_NAME | grep 'state UP'` ]]; do
+        sleep 0.1;


This is too minute. I would suggest wait 1 sec. Also, if its never up, we need a way to exit the script after say 5mts or so.

Ack, will look into a way to timeout

prsunny · 2021-03-01T16:42:07Z

dockers/docker-orchagent/swssconfig.sh

+
+      if [[ -f /arp.json ]];
+      then
+        swssconfig /arp.json


I think, may be we should not restore the arp.json entries to APP_DB since we are directly adding to kernel. If we add to APP_DB, arp_refresh script kick in and can result in failed entries temporarily.

Ack, will fix

prsunny · 2021-03-01T16:43:07Z

dockers/docker-orchagent/kernel_arp_restore.sh

+        IP=`echo $KEY | jq -r ". / \":\" | .[2]"`
+
+        wait_for_intf_up $DEVICE
+        ip neigh replace "$IP" dev "$DEVICE" lladdr "$MAC" nud stale


if we are restoring IPv6 entries, this should be "ip -6 neigh ..."

I thought ip neigh by default could accept both IPv4 and IPv6 entries?

* Timeout kernel ARP restore process after 5 minutes * Reduce sleep interval when checking interface status * Don't restoret APP_DB neighbor table when restoring kernel neighbor table Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

tahmed-dev

LGTM

dockers/docker-orchagent/kernel_arp_restore.sh

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

lguohan · 2021-03-05T18:46:53Z

dockers/docker-orchagent/supervisord.conf

@@ -71,6 +71,16 @@ stderr_logfile=syslog
 dependent_startup=true
 dependent_startup_wait_for=orchagent:running

+[program:kernel_arp_restore]


what is the different between kernel_arp_restore and restore_neighbors below? it seems the similiar things.

restore_neighbors is executed only during warmboot and restoration is by programming and sending arp requests. kernel_arp_restore is only programming to kernel (as dynamic entries) and done for config_reload scenario. Do you prefer we should combine the script?

i recall for fast reboot we also restore the arp entries, which script is that? do we restart kernel arp entries for fast reboot?

AFAIK, for fastboot, we only restore to APP_DB so that orchangent programs those ARP entry to ASIC and the arp_update script will send out requests to resolve arp.

It looks like restore_neighbors is used primarily during warmboot. It looks like it does accomplish the same thing, but I am hesitant to change anything warmboot related in case it has other side effects.

why fast_reboot cannot use the this kernel_arp_restore script? why only use this script in config reload scenario?

it is fine not to combine with restore_neighbors, but why this arp_restore cannot be combined into swssconfig.sh and also use it for fast reboot?

The issue with placing this in swssconfig.sh is that these kernel entries can only be programmed after the VLAN interface comes up, which happens after swssconfig.sh has exited

it is ok not be combine. but can this be used for fast-reboot as well?

lguohan · 2021-03-05T19:19:14Z

dockers/docker-orchagent/swssconfig.sh

@@ -29,6 +29,25 @@ function fast_reboot {
  esac
 }

+function config_reload {


can you rename this function as restator_neighbors, and then rename the fast_reboot script as restore default_routes. basically, i think we should try to reuse the code here and not to create duplication.

I can refactor this to re-use more existing code, but not by much since we don't want a lot of overlapping behavior between fast-reboot and config-reload here. E.g. for fast-reboot, we want to restore the ARP table to APP_DB as well as the default routes, but for config reload we do not want to restore the default routes, and we only need the ARP table to be restored to the kernel (if we restored to APP_DB here, there was some concern about having these entries be FAILED if the ToR attempted an ARP refresh).

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

lguohan · 2021-03-16T17:30:44Z

dockers/docker-orchagent/swssconfig.sh

-    *)
-      ;;
-  esac
+function signal_kernel_restore {


why is this only for config reload? can this be used for fast reboot as well?

Fast reboot already restores the neighbor table to APP_DB, and to my knowledge does not need it restored to the kernel.

theasianpianist · 2022-10-28T19:54:28Z

Closing since we are reverting the prerequisite PR sonic-net/sonic-utilities#2460

theasianpianist added 2 commits February 25, 2021 20:44

[docker_image_ctl]: Copy cached ARP tables to SWSS docker

049b833

* Only performed after config reload on dual ToR systems Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

[docker-orchagent]: Create process to restore arp to kernel

b069745

* Only runs on dual ToR systems after config reload Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist added the Request for 202012 Branch label Feb 26, 2021

theasianpianist requested review from lguohan, prsunny and tahmed-dev February 26, 2021 19:42

theasianpianist mentioned this pull request Feb 27, 2021

[garp_service]: Only send GARPs once at startup sonic-net/sonic-mgmt#3053

Closed

3 tasks

prsunny reviewed Mar 1, 2021

View reviewed changes

[swss]: Address review comments:

d58bae8

* Timeout kernel ARP restore process after 5 minutes * Reduce sleep interval when checking interface status * Don't restoret APP_DB neighbor table when restoring kernel neighbor table Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist changed the title ~~[docker_image_ctl]: Copy cached ARP tables to SWSS docker~~ [swss]: Restore cached FDB and ARP entries after config reload for dual ToR Mar 1, 2021

tahmed-dev previously approved these changes Mar 1, 2021

View reviewed changes

dockers/docker-orchagent/kernel_arp_restore.sh Outdated Show resolved Hide resolved

dockers/docker-orchagent/kernel_arp_restore.sh Outdated Show resolved Hide resolved

[kernel_arp_restore]: Add newline at EOF

566d40a

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist dismissed tahmed-dev’s stale review via 566d40a March 1, 2021 23:19

[swss]: Search for NEIGH_TABLE key before restoring

dcebde3

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist requested review from prsunny and tahmed-dev March 2, 2021 19:16

lguohan reviewed Mar 5, 2021

View reviewed changes

tahmed-dev previously approved these changes Mar 5, 2021

View reviewed changes

lguohan reviewed Mar 5, 2021

View reviewed changes

[swssconfig]: Increase code re-use

afc3ce5

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist dismissed tahmed-dev’s stale review via afc3ce5 March 9, 2021 20:25

theasianpianist requested a review from lguohan March 9, 2021 20:25

lguohan reviewed Mar 16, 2021

View reviewed changes

theasianpianist requested a review from lguohan March 17, 2021 01:00

zjswhhh mentioned this pull request Oct 14, 2022

NIC simulator not receiving SetAdminForwardingPortState request for 2 min after peer config reload sonic-net/sonic-utilities#2485

Closed

theasianpianist closed this Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[swss]: Restore cached FDB and ARP entries after config reload for dual ToR #6912

[swss]: Restore cached FDB and ARP entries after config reload for dual ToR #6912

theasianpianist commented Feb 26, 2021 •

edited

Loading

prsunny Mar 1, 2021

theasianpianist Mar 1, 2021

prsunny Mar 1, 2021

theasianpianist Mar 1, 2021

prsunny Mar 1, 2021

theasianpianist Mar 1, 2021

tahmed-dev left a comment

lguohan Mar 5, 2021

prsunny Mar 5, 2021

lguohan Mar 5, 2021

prsunny Mar 5, 2021

theasianpianist Mar 5, 2021

lguohan Mar 5, 2021

lguohan Mar 5, 2021

theasianpianist Mar 5, 2021

lguohan Mar 16, 2021

lguohan Mar 5, 2021

theasianpianist Mar 5, 2021

lguohan Mar 16, 2021

theasianpianist Mar 16, 2021

theasianpianist commented Oct 28, 2022 •

edited

Loading

[swss]: Restore cached FDB and ARP entries after config reload for dual ToR #6912

[swss]: Restore cached FDB and ARP entries after config reload for dual ToR #6912

Conversation

theasianpianist commented Feb 26, 2021 • edited Loading

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

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

tahmed-dev left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theasianpianist commented Oct 28, 2022 • edited Loading

theasianpianist commented Feb 26, 2021 •

edited

Loading

theasianpianist commented Oct 28, 2022 •

edited

Loading