[bulk mode] Fix bulk conflict when in case there are both remove and set operations #2071

shi-su · 2021-12-08T06:45:07Z

What I did
Check if there are items pending removal in bulk before calling bulk set API.

Why I did it
When there are items pending removal in bulk before calling set API, it means the item will be removed before the set and it should do create instead.

How I verified it

Details if related

qiluo-msft · 2021-12-08T21:41:57Z

auto it = consumer.m_toSync.begin();

I have another idea. For route orch, if m_toSync gets a DEL/SET, we can safely merge it into a SET. Then it will simplify the bulking logic. his is possible because the new SET has covered all the fields compared the existing fields before DEL.
And this will lead to a general solution.

In reply to: 989219096

Refers to: orchagent/routeorch.cpp:468 in 2a72407. [](commit_id = 2a72407, deletion_comment = False)

qiluo-msft · 2021-12-08T23:59:51Z

orchagent/routeorch.cpp

     */
-    if (it_route == m_syncdRoutes.at(vrf_id).end())
+    if (it_route == m_syncdRoutes.at(vrf_id).end() || gRouteBulker.bulk_entry_pending_removal(route_entry))


Add a unit test? #Closed

Added a unit test for the bulker function

stephenxs

This logic works only if removing a routing entry always succeeds.
If removing a routing entry can fail and the routing entry is still in SAI table in that case, the logic will be broken. Can that happen?

stephenxs · 2021-12-09T14:40:34Z

auto it = consumer.m_toSync.begin();
I have another idea. For route orch, if m_toSync gets a DEL/SET, we can safely merge it into a SET. Then it will simplify the bulking logic. his is possible because the new SET has covered all the fields compared the existing fields before DEL. And this will lead to a general solution.

Sounds good but I don't know what happens if the routing entry switches from a normal nexthop/nhg to some special thing like a tunnel/MPLS. In that case, is removing the old entry a must or it can just override the attributes?
In addToSync there is similar logic to merge multiple operations on the same key, but it doesn't merge a removal to a setting (S, D => D). It only merges a setting to a removal.

shi-su · 2021-12-09T21:58:34Z

This logic works only if removing a routing entry always succeeds. If removing a routing entry can fail and the routing entry is still in SAI table in that case, the logic will be broken. Can that happen?

This is a valid concern. This logic is based on the assumption that the remove operation is successful. In the current stage, the removal failure would cause orchagent exit, thus the situation should not happen. Also, the original behavior does not seem to be working. Let's assume the remove operation fails, the set operation would become a set_entry_attribute. In case that the set is successful and the remove gets retried, it might end up with the route getting removed though it is supposed to be there.

shi-su · 2021-12-09T22:02:13Z

auto it = consumer.m_toSync.begin();
I have another idea. For route orch, if m_toSync gets a DEL/SET, we can safely merge it into a SET. Then it will simplify the bulking logic. his is possible because the new SET has covered all the fields compared the existing fields before DEL. And this will lead to a general solution.
Sounds good but I don't know what happens if the routing entry switches from a normal nexthop/nhg to some special thing like a tunnel/MPLS. In that case, is removing the old entry a must or it can just override the attributes? In addToSync there is similar logic to merge multiple operations on the same key, but it doesn't merge a removal to a setting (S, D => D). It only merges a setting to a removal.

I have some concerns about making merging DEL and SET into one SET a general solution. For some SAI APIs, some attributes may only be configurable on creation. In such cases, we should keep both operations and strictly do a DEL and then CREATE. Yet I do agree that it is a valid solution specific to route orch. And it could be an additional performance improvement on top of this fix.

stephenxs · 2021-12-10T06:37:32Z

This logic works only if removing a routing entry always succeeds. If removing a routing entry can fail and the routing entry is still in SAI table in that case, the logic will be broken. Can that happen?

This is a valid concern. This logic is based on the assumption that the remove operation is successful. In the current stage, the removal failure would cause orchagent exit, thus the situation should not happen. Also, the original behavior does not seem to be working. Let's assume the remove operation fails, the set operation would become a set_entry_attribute. In case that the set is successful and the remove gets retried, it might end up with the route getting removed though it is supposed to be there.

Ok. That means it doesn't introduce new risks or degradation.

dprital · 2021-12-13T10:42:31Z

@shi-su, Can you please retrigger the checkers for this PR ?

shi-su · 2021-12-13T22:27:00Z

/azp run

azure-pipelines · 2021-12-13T22:27:11Z

Azure Pipelines successfully started running 1 pipeline(s).

stephenxs · 2021-12-14T01:45:13Z

vs test failed due to ##[error]Artifact sonic-gcov already exists for build 58142.

shi-su · 2021-12-14T01:50:53Z

vs test failed due to ##[error]Artifact sonic-gcov already exists for build 58142.

I retriggered the tests. I believe the status of sonic-gcov will be refreshed after this run if it passes.

691c37b [Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk (sonic-net/sonic-swss#2086) a4c80c3 patch for issue sonic-net/sonic-swss#1971 - enable Rx Drop handling for cisco-8000 (sonic-net/sonic-swss#2041) 71751d1 [macsec] Support setting IPG by gearbox_config.json (sonic-net/sonic-swss#2051) 5d5c169 [bulk mode] Fix bulk conflict when in case there are both remove and set operations (sonic-net/sonic-swss#2071) 8bbdbd2 Fix SRV6 NHOP CRM object type (sonic-net/sonic-swss#2072) ef5b35f [vstest] VS test failure fix after fabric port orch PR merge (sonic-net/sonic-swss#1811) 89ea538 Supply the missing ingress/egress port profile list in document (sonic-net/sonic-swss#2064) 8123437 [pfc_detect] fix RedisReply errors (sonic-net/sonic-swss#2040) b38f527 [swss][CRM][MPLS] MPLS CRM Nexthop - switch back to using SAI OBJECT rather than SWITCH OBJECT ae061e5 create debug_shell_enable config to enable debug shell (sonic-net/sonic-swss#2060) 45e446d [cbf] Fix max FC value (sonic-net/sonic-swss#2049) b1b5b29 Initial p4orch pytest code. (sonic-net/sonic-swss#2054) d352d5a Update default route status to state DB (sonic-net/sonic-swss#2009) 24a64d6 Orchagent: Integrate P4Orch (sonic-net/sonic-swss#2029) 15a3b6c Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled (sonic-net/sonic-swss#1897) ed783e1 [orchagent] Add trap flow counter support (sonic-net/sonic-swss#1951) e9b05a3 [vnetorch] ECMP for vnet tunnel routes with endpoint health monitor (sonic-net/sonic-swss#1955) bcb7d61 P4Orch: inital add of source (sonic-net/sonic-swss#1997) f6f6f86 [mclaglink] fix acl out ports (sonic-net/sonic-swss#2026) fd887bf [Reclaim buffer] Reclaim unused buffer for dynamic buffer model (sonic-net/sonic-swss#1910) 9258978 [orchagent, cfgmgr] Add response publisher and state recording (sonic-net/sonic-swss#1992) 3d862a7 Fixing subport vs test script for subport under VNET (sonic-net/sonic-swss#2048) fb0a5fd Don't handle buffer pool watermark during warm reboot reconciling (sonic-net/sonic-swss#1987) 16d4bcd Routed subinterface enhancements (sonic-net/sonic-swss#1907) 9639db7 [vstest/subintf] Add vs test to validate sub interface ingress to a vnet (sonic-net/sonic-swss#1642) Signed-off-by: Stephen Sun [email protected]

…set operations (sonic-net#2071) What I did Check if there are items pending removal in bulk before calling bulk set API. Fixes sonic-net/sonic-buildimage#9434 Why I did it When there are items pending removal in bulk before calling set API, it means the item will be removed before the set and it should do create instead.

Shi Su added 2 commits December 8, 2021 06:42

Fix bulk conflict removal and set

b892221

Add comment

2a72407

shi-su mentioned this pull request Dec 8, 2021

Routeorch calls SAI API set attribute for a routing entry which was just removed from SAI in the same bulk operation. Should recreate it in such case. sonic-net/sonic-buildimage#9434

Closed

shi-su marked this pull request as ready for review December 8, 2021 17:37

shi-su requested a review from prsunny as a code owner December 8, 2021 17:37

shi-su requested a review from qiluo-msft December 8, 2021 17:38

qiluo-msft requested a review from stephenxs December 8, 2021 20:34

qiluo-msft reviewed Dec 8, 2021

View reviewed changes

stephenxs reviewed Dec 9, 2021

View reviewed changes

Add unit test

cb09853

stephenxs approved these changes Dec 10, 2021

View reviewed changes

qiluo-msft approved these changes Dec 13, 2021

View reviewed changes

shi-su merged commit 5d5c169 into sonic-net:master Dec 14, 2021

shi-su deleted the bulk_conflict branch December 14, 2021 06:50

stephenxs mentioned this pull request Dec 14, 2021

[sonic-swss] Submodule update sonic-net/sonic-buildimage#9397

Merged

5 tasks

stephenxs mentioned this pull request Dec 22, 2021

[Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk #2086

Merged

stephenxs mentioned this pull request Jan 6, 2022

[sonic-swss][202111] Submodule update (#9397) sonic-net/sonic-buildimage#9685

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bulk mode] Fix bulk conflict when in case there are both remove and set operations #2071

[bulk mode] Fix bulk conflict when in case there are both remove and set operations #2071

shi-su commented Dec 8, 2021 •

edited

Loading

qiluo-msft commented Dec 8, 2021 •

edited

Loading

qiluo-msft Dec 8, 2021 •

edited

Loading

shi-su Dec 9, 2021

stephenxs left a comment

stephenxs commented Dec 9, 2021

shi-su commented Dec 9, 2021

shi-su commented Dec 9, 2021

stephenxs commented Dec 10, 2021

dprital commented Dec 13, 2021

shi-su commented Dec 13, 2021

azure-pipelines bot commented Dec 13, 2021

stephenxs commented Dec 14, 2021

shi-su commented Dec 14, 2021

[bulk mode] Fix bulk conflict when in case there are both remove and set operations #2071

[bulk mode] Fix bulk conflict when in case there are both remove and set operations #2071

Conversation

shi-su commented Dec 8, 2021 • edited Loading

qiluo-msft commented Dec 8, 2021 • edited Loading

qiluo-msft Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

shi-su Dec 9, 2021

Choose a reason for hiding this comment

stephenxs left a comment

Choose a reason for hiding this comment

stephenxs commented Dec 9, 2021

shi-su commented Dec 9, 2021

shi-su commented Dec 9, 2021

stephenxs commented Dec 10, 2021

dprital commented Dec 13, 2021

shi-su commented Dec 13, 2021

azure-pipelines bot commented Dec 13, 2021

stephenxs commented Dec 14, 2021

shi-su commented Dec 14, 2021

shi-su commented Dec 8, 2021 •

edited

Loading

qiluo-msft commented Dec 8, 2021 •

edited

Loading

qiluo-msft Dec 8, 2021 •

edited

Loading