[neighorch] VOQ encap index change handling #1729

vganesan-nokia · 2021-04-27T21:27:04Z

What I did

For the remote neighbors in the VOQ systems when there is change in the
encap index (due to syncd restart), the previously programmed remote
neighs should be re-written with the new changed encap index. The
current voq system neigh handling api in neighorch only checks for the
change of mac address. Because of this the change in the encap index is
ignored. This causes mismatch of encap index in the neighbor records in
owner asic and the remote asics after syncd restart situations like
config reload or failure recovery. This results in packet forwarding
failures for the packets forwarded across fabric and egressing in
different asics than ingress asics. The encap index that was previously
allocated by SAI and synced to the CHASSIS_APP_DB may change for the
same neighbor when syncd is restarted after config reload or any failure
recovery situations. When syncd restarts, the SAI may undergo full
re-initialization and may loose the memory of previous encap index
allocation for the neighbors. During reprogramming of the neighbors they
may not get the same encap index that were allocated before restart.

This fix is to store the allocated encap index in the orchagent
(neighorch) and compare with received encap index if the neighbor entry
already exits. This comparison of encap index is new to voq chassis.
This is done in addition to checking for mac change. For any received
(synced) neigh (received from CHASSIS_APP_DB) if neigh entry already
exists and if there is change in encap index, the current neigh entry
is removed and re-added. As of now, since current SAI does not support
change of encap index (i.e, set of operaton of encap index attribute for
neigh records) del and re-add is done.

Why I did it

To fix the traffic loss problem in voq chassis after config reload. The problem is described in issue sonic-net/sonic-buildimage#7451

How I verified it

vs test for test_virtual_chassis.py fully pass.
With setup as mentioned in issue Traffic forwarding faliure in voq chassis after config reload sonic-buildimage#7451, performed config reload multiple times, and verified that the traffic recovers fully after the line card becomes stable and operational.

Details if related

anshuv-mfst · 2021-04-28T15:24:56Z

@abdosi - could you please take a look, thanks.

anshuv-mfst · 2021-05-12T17:29:48Z

@ysmanman please take a look.

ysmanman · 2021-05-13T04:03:27Z

orchagent/neighorch.cpp

+                // Handle encap index change. SAI does not support change of encap index for
+                // existing neighbors. Remove the neighbor but do not errase from consumer sync
+                // buffer. The next iteration will add the neighbor back with new encap index


Deleing neigh and adding it back may churn routers resolving over that neigh. So routes get deleted and added back. I guess encap index change is common. Probably should be seen in syncd restart, as you mentioned, or linecard reload? Ideally, encap index change should be handled by SAI just like how it handles MAC change today.

typo: i meant to say encap index change is NOT common.

I agree. SAI should handle the encap index change similar to mac change. But current SAI does not support it. Till SAI supports it, we will have to live with this delete and re-add approach. Once SAI is enhanced for the encap index change handling, we can change this.

@vganesan-nokia can you create issue in SONiC for tracking SAI not supporting set operation

@vganesan-nokia can you create issue in SONiC for tracking SAI not supporting set operation

Yes. Created issue sonic-net/sonic-buildimage#7879. Thanks

i was discussing with @prsunny and we think better option will be call SAI api with set and if it fails (which it will) then we should do remove/add neigh. That way once the attribute is supported we don't need any sonic change going forward.

There is a problem with this approach. In current SAI (w.r.t brcm SAI 5.0), when set is done for encap index attribute in the neighbor record, SAI's neighbor entry set attribute API returns SAI_STATUS_FAILURE. When there is not SUCCESS from SAI, syncd sends orchagent shut down.

Fixed. Since syncd is not sending orchagent shutdown for SET failures anymore, changes are done as suggested.

ysmanman · 2021-05-13T22:06:29Z

orchagent/neighorch.cpp

@@ -1133,8 +1133,30 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer)
            }

            if (m_syncdNeighbors.find(neighbor_entry) == m_syncdNeighbors.end() ||
-                    m_syncdNeighbors[neighbor_entry].mac != mac_address)
+                    m_syncdNeighbors[neighbor_entry].mac != mac_address ||


Why do we check mac for remote neighbor? Isn't checking encap sufficient?

No. For mac change, the encap index is not changed. If we only check encap index, we'll not be processing the mac change.

ysmanman · 2021-05-13T22:28:41Z

orchagent/neighorch.cpp

+                        //neigh successfully deleted from SAI. Set STATE DB to signal to remove entries from kernel
+                        m_stateSystemNeighTable->del(state_key);


Why we only do this when removeNeighbor succeeds? If removeNeighbor fails for some reasons, kernel entries are not removed and then traffic may still be forwarded with wrong encap index. Should we signal removing kernel entries always when encap index changes?

We do this for neat re-trying. Removal of kernel neigh without successful removal of neigh from SAI is partial processing. This will create problems in retries without additional logic.

anshuv-mfst · 2021-06-02T15:54:36Z

@neethajohn - please review and approve.

abdosi

as added comment.

Signed-off-by: vedganes <[email protected]> For the remote neighbors in the VOQ systems when there is change in the encap index (due to syncd restart), the previously programmed remote neighs should be re-written with the new changed encap index. The current voq system neigh handling api in neighorch only checks for the change of mac address. Because of this the change in the encap index is ignored. This causes mismatch of encap index in the neighbor records in owner asic and the remote asics after syncd restart situations like config reload or failure recovery. This results in packet forwarding failures for the packets forwarded across fabric and egressing in different asics than ingress asics. The encap index that was previously allocated by SAI and synced to the CHASSIS_APP_DB may change for the same neighbor when syncd is restarted after config reload or any failure recovery situations. When syncd restarts, the SAI may undergo full re-initialization and may loose the memory of previous encap index allocation for the neighbors. During reprogramming of the neighbors they may not get the same encap index that were allocated before restart. This fix is to store the allocated encap index in the orchagent (neighorch) and compare with received encap index if the neighbor entry already exits. This comparison of encap index is new to voq chassis. This is done in addition to checking for mac change. For any received (synced) neigh (received from CHASSIS_APP_DB) if neigh entry already exists and if there is change in encap index, the current neigh entry is removed and re-added. As of now, since current SAI does not support change of encap index (i.e, set of operaton of encap index attribute for neigh records) del and re-add is done.

Changes done to handle the encap index change by first try to SET and if there is failure from SAI for the SET of encap index attribute in neighbor entry, do remove and re-add the whole neighbor entry. Signed-off-by: vedganes <[email protected]>

Signed-off-by: vedganes <[email protected]>

[flex-counters] Delay flex counters stats init for faster boot time (sonic-net/sonic-swss#1803) [mirror] Detach session dst ip from route orch LPM calculation regardless of session status at session CONFIG DB removal (sonic-net/sonic-swss#1800) [Dynamic Buffer Calc] Support dynamic buffer calculation on top of port auto negotiation (sonic-net/sonic-swss#1762) [neighorch] VOQ encap index change handling (sonic-net/sonic-swss#1729) [neighorch] Mac for voq neighbors in VS platforms (sonic-net/sonic-swss#1724) [acl mirror action] Mirror session ref count fix at acl rule attachment (sonic-net/sonic-swss#1761)

For the remote neighbors in the VOQ systems when there is change in the encap index (due to syncd restart), the previously programmed remote neighs should be re-written with the new changed encap index. The current voq system neigh handling api in neighorch only checks for the change of mac address. Because of this the change in the encap index is ignored.

vganesan-nokia mentioned this pull request Apr 27, 2021

Traffic forwarding faliure in voq chassis after config reload sonic-net/sonic-buildimage#7451

Closed

ysmanman reviewed May 13, 2021

View reviewed changes

ysmanman previously approved these changes May 25, 2021

View reviewed changes

anshuv-mfst requested a review from abdosi June 2, 2021 16:44

vganesan-nokia mentioned this pull request Jun 15, 2021

[voq] SAI support for changing encap index in voq neighbor attribute programming (set function) sonic-net/sonic-buildimage#7879

Closed

abdosi requested changes Jun 15, 2021

View reviewed changes

judyjoseph added the Request for 202106 Branch label Jun 22, 2021

vganesan-nokia dismissed ysmanman’s stale review via 62e2c4e June 29, 2021 02:24

vganesan-nokia force-pushed the voq-encapidx-chg branch from b27a47d to 62e2c4e Compare June 29, 2021 02:24

vganesan-nokia requested a review from prsunny as a code owner June 29, 2021 02:24

vedganes added 2 commits June 29, 2021 13:27

[voq] Code review comments fix - 1

481cf18

Changes done to handle the encap index change by first try to SET and if there is failure from SAI for the SET of encap index attribute in neighbor entry, do remove and re-add the whole neighbor entry. Signed-off-by: vedganes <[email protected]>

vganesan-nokia force-pushed the voq-encapidx-chg branch from 62e2c4e to 481cf18 Compare June 29, 2021 17:55

[neighorch] Log message change for voq neigh encap index update

8cceec2

Signed-off-by: vedganes <[email protected]>

abdosi approved these changes Jul 2, 2021

View reviewed changes

abdosi merged commit 8a169fc into sonic-net:master Jul 2, 2021

shlomibitton mentioned this pull request Jul 4, 2021

[submodule] Update sonic-swss submodule sonic-net/sonic-buildimage#8057

Merged

judyjoseph added the Included in 202106 Branch label Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[neighorch] VOQ encap index change handling #1729

[neighorch] VOQ encap index change handling #1729

vganesan-nokia commented Apr 27, 2021

anshuv-mfst commented Apr 28, 2021

anshuv-mfst commented May 12, 2021

ysmanman May 13, 2021

ysmanman May 13, 2021

vganesan-nokia May 13, 2021

abdosi Jun 15, 2021

vganesan-nokia Jun 15, 2021

abdosi Jun 15, 2021

vganesan-nokia Jun 15, 2021

vganesan-nokia Jun 29, 2021

ysmanman May 13, 2021

vganesan-nokia May 18, 2021

ysmanman May 13, 2021

vganesan-nokia May 18, 2021

anshuv-mfst commented Jun 2, 2021

abdosi left a comment

		//neigh successfully deleted from SAI. Set STATE DB to signal to remove entries from kernel
		m_stateSystemNeighTable->del(state_key);

[neighorch] VOQ encap index change handling #1729

[neighorch] VOQ encap index change handling #1729

Conversation

vganesan-nokia commented Apr 27, 2021

anshuv-mfst commented Apr 28, 2021

anshuv-mfst commented May 12, 2021

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

anshuv-mfst commented Jun 2, 2021

abdosi left a comment

Choose a reason for hiding this comment