[active-standby] Fix default route handler race condition #254

lolyu · 2024-06-20T09:00:47Z

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
New feature
Doc/Design
Unit test

Approach

What is the motivation for this PR?

Fix the race condition of the default route notification.

This is similar to #104

If there are multiple default route notifications received by linkmgrd, the mux port posts the default route handlers wrapped by strand. But boost asio doesn't guarantee the execution order of the default route handlers, so the final state machine default route could be any intermediate default route state.

For example, for default route notifications like:

[2024-06-20 08:28:57.872911] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: na
[2024-06-20 08:28:57.872954] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: ok

The final state machine default route state could be "ok" if the handler for "ok" is executed after the handler for "na".
The final state machine default route state could be "na" if the handler for "ok" is executed before the handler for "na".

Signed-off-by: Longxiang Lyu [email protected]

Work item tracking

Microsoft ADO (number only): 28471183

How did you do it?

post the default route handlers directly through strand instead of using strand::wrap, so the handlers are executed in the same order as the handlers' post order.

How did you verify/test it?

without this PR, UT fail:

lolv@f8c780888096:/sonic/src/repo/sonic-linkmgrd$ ./linkmgrd-test --gtest_filter="*DefaultRouteStateRaceCondition*"
Note: Google Test filter = *DefaultRouteStateRaceCondition*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from LinkManagerStateMachineTest
[ RUN      ] LinkManagerStateMachineTest.DefaultRouteStateRaceCondition
test/LinkManagerStateMachineTest.cpp:1567: Failure
Expected equality of these values:
  mFakeMuxPort.getDefaultRoute()
    Which is: 4-byte object <01-00 00-00>
  link_manager::LinkManagerStateMachineBase::DefaultRoute::OK
    Which is: 4-byte object <02-00 00-00>
[  FAILED  ] LinkManagerStateMachineTest.DefaultRouteStateRaceCondition (11327 ms)
[----------] 1 test from LinkManagerStateMachineTest (11327 ms total)
 
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (11328 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] LinkManagerStateMachineTest.DefaultRouteStateRaceCondition
 
1 FAILED TEST

with this PR, UT pass:

lolv@f8c780888096:/sonic/src/repo/sonic-linkmgrd$ ./linkmgrd-test --gtest_filter="*DefaultRouteStateRaceCondition*"
Note: Google Test filter = *DefaultRouteStateRaceCondition*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from LinkManagerStateMachineTest
[ RUN      ] LinkManagerStateMachineTest.DefaultRouteStateRaceCondition
[       OK ] LinkManagerStateMachineTest.DefaultRouteStateRaceCondition (11496 ms)
[----------] 1 test from LinkManagerStateMachineTest (11496 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (11496 ms total)
[  PASSED  ] 1 test.

Any platform specific information?

Documentation

Signed-off-by: Longxiang Lyu <[email protected]>

) What is the motivation for this PR? Fix the race condition of the default route notification. This is similar to sonic-net#104 If there are multiple default route notifications received by linkmgrd, the mux port posts the default route handlers wrapped by strand. But boost asio doesn't guarantee the execution order of the default route handlers, so the final state machine default route could be any intermediate default route state. For example, for default route notifications like: [2024-06-20 08:28:57.872911] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: na [2024-06-20 08:28:57.872954] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: ok The final state machine default route state could be "ok" if the handler for "ok" is executed after the handler for "na". The final state machine default route state could be "na" if the handler for "ok" is executed before the handler for "na". Signed-off-by: Longxiang Lyu [email protected] Work item tracking Microsoft ADO (number only): 28471183 How did you do it? post the default route handlers directly through strand instead of using strand::wrap, so the handlers are executed in the same order as the handlers' post order. How did you verify/test it? without this PR, UT fail: Signed-off-by: Longxiang Lyu <[email protected]>

mssonicbld · 2024-06-21T17:35:17Z

Cherry-pick PR to 202311: #255

What is the motivation for this PR? Fix the race condition of the default route notification. This is similar to #104 If there are multiple default route notifications received by linkmgrd, the mux port posts the default route handlers wrapped by strand. But boost asio doesn't guarantee the execution order of the default route handlers, so the final state machine default route could be any intermediate default route state. For example, for default route notifications like: [2024-06-20 08:28:57.872911] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: na [2024-06-20 08:28:57.872954] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: ok The final state machine default route state could be "ok" if the handler for "ok" is executed after the handler for "na". The final state machine default route state could be "na" if the handler for "ok" is executed before the handler for "na". Signed-off-by: Longxiang Lyu [email protected] Work item tracking Microsoft ADO (number only): 28471183 How did you do it? post the default route handlers directly through strand instead of using strand::wrap, so the handlers are executed in the same order as the handlers' post order. How did you verify/test it? without this PR, UT fail: Signed-off-by: Longxiang Lyu <[email protected]>

) What is the motivation for this PR? Fix the race condition of the default route notification. This is similar to sonic-net#104 If there are multiple default route notifications received by linkmgrd, the mux port posts the default route handlers wrapped by strand. But boost asio doesn't guarantee the execution order of the default route handlers, so the final state machine default route could be any intermediate default route state. For example, for default route notifications like: [2024-06-20 08:28:57.872911] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: na [2024-06-20 08:28:57.872954] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: ok The final state machine default route state could be "ok" if the handler for "ok" is executed after the handler for "na". The final state machine default route state could be "na" if the handler for "ok" is executed before the handler for "na". Signed-off-by: Longxiang Lyu [email protected] Work item tracking Microsoft ADO (number only): 28471183 How did you do it? post the default route handlers directly through strand instead of using strand::wrap, so the handlers are executed in the same order as the handlers' post order. How did you verify/test it? without this PR, UT fail: Signed-off-by: Longxiang Lyu <[email protected]>

mssonicbld · 2024-08-06T07:10:57Z

Cherry-pick PR to 202305: #265

What is the motivation for this PR? Fix the race condition of the default route notification. This is similar to #104 If there are multiple default route notifications received by linkmgrd, the mux port posts the default route handlers wrapped by strand. But boost asio doesn't guarantee the execution order of the default route handlers, so the final state machine default route could be any intermediate default route state. For example, for default route notifications like: [2024-06-20 08:28:57.872911] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: na [2024-06-20 08:28:57.872954] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: ok The final state machine default route state could be "ok" if the handler for "ok" is executed after the handler for "na". The final state machine default route state could be "na" if the handler for "ok" is executed before the handler for "na". Signed-off-by: Longxiang Lyu [email protected] Work item tracking Microsoft ADO (number only): 28471183 How did you do it? post the default route handlers directly through strand instead of using strand::wrap, so the handlers are executed in the same order as the handlers' post order. How did you verify/test it? without this PR, UT fail: Signed-off-by: Longxiang Lyu <[email protected]>

StormLiangMS · 2024-08-21T15:27:44Z

@bingwang-ms you may want to cherry pick this one?

) What is the motivation for this PR? Fix the race condition of the default route notification. This is similar to sonic-net#104 If there are multiple default route notifications received by linkmgrd, the mux port posts the default route handlers wrapped by strand. But boost asio doesn't guarantee the execution order of the default route handlers, so the final state machine default route could be any intermediate default route state. For example, for default route notifications like: [2024-06-20 08:28:57.872911] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: na [2024-06-20 08:28:57.872954] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: ok The final state machine default route state could be "ok" if the handler for "ok" is executed after the handler for "na". The final state machine default route state could be "na" if the handler for "ok" is executed before the handler for "na". Signed-off-by: Longxiang Lyu [email protected] Work item tracking Microsoft ADO (number only): 28471183 How did you do it? post the default route handlers directly through strand instead of using strand::wrap, so the handlers are executed in the same order as the handlers' post order. How did you verify/test it? without this PR, UT fail: Signed-off-by: Longxiang Lyu <[email protected]>

bingwang-ms · 2024-08-21T16:26:46Z

@bingwang-ms you may want to cherry pick this one?

Done. Thanks for the reminding.

mssonicbld · 2024-08-21T16:26:52Z

Cherry-pick PR to 202405: #267

What is the motivation for this PR? Fix the race condition of the default route notification. This is similar to #104 If there are multiple default route notifications received by linkmgrd, the mux port posts the default route handlers wrapped by strand. But boost asio doesn't guarantee the execution order of the default route handlers, so the final state machine default route could be any intermediate default route state. For example, for default route notifications like: [2024-06-20 08:28:57.872911] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: na [2024-06-20 08:28:57.872954] [warning] MuxPort.cpp:365 handleDefaultRouteState: port: EtherTest01, state db default route state: ok The final state machine default route state could be "ok" if the handler for "ok" is executed after the handler for "na". The final state machine default route state could be "na" if the handler for "ok" is executed before the handler for "na". Signed-off-by: Longxiang Lyu [email protected] Work item tracking Microsoft ADO (number only): 28471183 How did you do it? post the default route handlers directly through strand instead of using strand::wrap, so the handlers are executed in the same order as the handlers' post order. How did you verify/test it? without this PR, UT fail: Signed-off-by: Longxiang Lyu <[email protected]>

Approach What is the motivation for this PR? Fix the UT failure introduced by #254. The failure is due to that, the wait time for the two default route handlers to finish is 10ms, which is not sufficient on some build image agents which has limited CPU resource. Signed-off-by: Longxiang Lyu [email protected] Work item tracking Microsoft ADO (number only): 28471183 How did you do it? Let's increase the wait time to 8s. How did you verify/test it? UT passed. Any platform specific information? Documentation

Fix default route handler race condition

c75ff07

Signed-off-by: Longxiang Lyu <[email protected]>

lolyu requested review from yxieca and lguohan June 20, 2024 09:01

lolyu added Request for 202305 Branch Request for 202311 Branch Request for 202405 Branch labels Jun 20, 2024

lolyu requested a review from bingwang-ms June 20, 2024 09:01

Fix UT

72e8feb

Signed-off-by: Longxiang Lyu <[email protected]>

yxieca approved these changes Jun 21, 2024

View reviewed changes

yxieca merged commit d0124f5 into sonic-net:master Jun 21, 2024
9 checks passed

yxieca added the Approved for 202311 Branch label Jun 21, 2024

mssonicbld mentioned this pull request Jun 21, 2024

[action] [PR:254] [active-standby] Fix default route handler race condition (#254) #255

Merged

mssonicbld added the Created PR to 202311 Branch label Jun 21, 2024

mssonicbld added Included in 202311 Branch and removed Created PR to 202311 Branch labels Jun 21, 2024

lolyu mentioned this pull request Jun 26, 2024

Fix default route race condition UT #258

Merged

4 tasks

StormLiangMS added the Approved for 202305 Branch label Aug 6, 2024

mssonicbld mentioned this pull request Aug 6, 2024

[action] [PR:254] [active-standby] Fix default route handler race condition (#254) #265

Merged

mssonicbld added the Created PR to 202305 Branch label Aug 6, 2024

mssonicbld added Included in 202305 Branch and removed Created PR to 202305 Branch labels Aug 6, 2024

bingwang-ms added the Approved for 202405 Branch label Aug 21, 2024

mssonicbld mentioned this pull request Aug 21, 2024

[action] [PR:254] [active-standby] Fix default route handler race condition (#254) #267

Merged

mssonicbld added the Created PR to 202405 Branch label Aug 21, 2024

mssonicbld added Included in 202405 Branch and removed Created PR to 202405 Branch labels Aug 21, 2024

lolyu mentioned this pull request Aug 23, 2024

Fix default route race condition UT (#258) #268

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[active-standby] Fix default route handler race condition #254

[active-standby] Fix default route handler race condition #254

lolyu commented Jun 20, 2024 •

edited

Loading

mssonicbld commented Jun 21, 2024

mssonicbld commented Aug 6, 2024

StormLiangMS commented Aug 21, 2024

bingwang-ms commented Aug 21, 2024

mssonicbld commented Aug 21, 2024

[active-standby] Fix default route handler race condition #254

[active-standby] Fix default route handler race condition #254

Conversation

lolyu commented Jun 20, 2024 • edited Loading

Description of PR

Type of change

Approach

What is the motivation for this PR?

Work item tracking

How did you do it?

How did you verify/test it?

Any platform specific information?

Documentation

mssonicbld commented Jun 21, 2024

mssonicbld commented Aug 6, 2024

StormLiangMS commented Aug 21, 2024

bingwang-ms commented Aug 21, 2024

mssonicbld commented Aug 21, 2024

lolyu commented Jun 20, 2024 •

edited

Loading