Linkmgrd subscribing State DB route event #13

zjswhhh · 2021-12-15T19:30:56Z

Description of PR

Summary:
Fixes # (issue)

This PR is to make linkmgrd subscribe events from ROUTE_TABLE in State DB, and react accordingly:

If any of the two default route state appears to be 'na', linkmgrd should switch to standby.
If both are 'ok', there will be a mux state probing and what happens next depends on linkmgrd state machine and the probing response.

Type of change

Bug fix
Test case(new/improvement)
New feature

Approach

What is the motivation for this PR?

To make linkmgrd subscribe state DB route event, and handle the switchovers.

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Related PR: sonic-net/sonic-swss#2009

Define handlers

…net#8) Description of PR Summary: Fixes # (issue) Fix unstable unit tests. People observed that linkmgrd unit tests appeared to be flaky, and kept failing PR checks. After checking the build logs, I found that in most of the failures, the state change handler didn't seem to be invoked: Linkmgrd's composite state remained as it was before the state change was post. This PR updates code to invoke io_service::reset(), sets the io_service to no longer be in a stopped state, allowing subsequent calls to run_one() to invoke handlers. (When io_service is stopped, any call to run_one() will return immediately without invoking any handler. ) Signed-off-by: Jing Zhang zhangjing@microsoft.com Type of change Bug fix Approach What is the motivation for this PR? Stabilize linkmgrd unit tests. How did you do it? Reset stopped io_service before calling to run_one(), to guarantee state change handlers are invoked as expected. How did you verify/test it? Tested building locally. This issue is hard to reproduce, the current build failure data is 2.98% (56/1877 in last 30 days). We should expect to see improvement after this change.

Add pull request template for linkmgrd repo. sign-off: Jing Zhang zhangjing@microsoft.com

This reverts commit 9d38f17.

yxieca · 2021-12-19T17:42:36Z

src/MuxManager.cpp

+    }
+
+    std::string nextState = "na";
+    if (mIpv4DefaultRouteState == "ok" && mIpv6DefaultRouteState == "ok") {


@lguohan do we have concern if either IPv4 or IPv6 BGP is not configured? Can we assume we will have them both eventually?

Updated per discussion in Gemini sync-up meeting.

yxieca · 2022-01-04T22:47:21Z

src/link_manager/LinkManagerStateMachine.cpp

+{
+    MUXLOGWARNING(boost::format("%s: state db default route state: %s") % mMuxPortConfig.getPortName() % routeState);
+
+    if(mComponentInitState.test(MuxStateComponent)) {


add space after if

Updated accordingly.

yxieca · 2022-01-04T22:48:16Z

src/link_manager/LinkManagerStateMachine.cpp

+
+    if(mComponentInitState.test(MuxStateComponent)) {
+        if (ms(mCompositeState) == mux_state::MuxState::Label::Active &&
+            routeState == "na") {


I suggest putting '{' on a new line, especially when if condition expands multiple lines. new line helps to identify the beginning of the if body.

Updated accordingly.

yxieca · 2022-01-04T22:59:26Z

src/MuxManager.cpp

+    }
+
+    PortMapIterator portMapIterator = mPortMap.begin();
+    while(portMapIterator != mPortMap.end()) {


space after while please.

Updated accordingly.

prsunny · 2022-01-12T22:54:20Z

src/link_manager/LinkManagerStateMachine.cpp

+    MUXLOGWARNING(boost::format("%s: state db default route state: %s") % mMuxPortConfig.getPortName() % routeState);
+
+    if (mComponentInitState.test(MuxStateComponent)) {
+        if (ms(mCompositeState) == mux_state::MuxState::Label::Active && routeState == "na") {


I'm thinking we should check for != Standby. What if mux is in a transient state and we got this event? What do you think?

~~Yes I think this is a good point. I will evaluate the behavior and update accordingly.~~
Updated, thanks!

prsunny · 2022-01-12T22:56:12Z

src/link_manager/LinkManagerStateMachine.cpp

+        if (ms(mCompositeState) == mux_state::MuxState::Label::Active && routeState == "na") {
+            CompositeState nextState = mCompositeState;
+            enterLinkProberState(nextState, link_prober::LinkProberState::Wait);
+            switchMuxState(nextState, mux_state::MuxState::Label::Standby);


How does it behave if both ToRs have default route as "na" ?

Both ToRs will switch to standby, the one switches first will be "active" at the end.

Do we expect both ToRs to be "standby" in this scenario? This is not supported currently by linkmgrd as I know, we might need find other workaround if so.

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

Signed-off-by: Ying Xie <ying.xie@microsoft.com>

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

This reverts commit 12b9951.

…c-linkmgrd into subscribeRouteEvent

zjswhhh · 2022-01-19T04:17:33Z

boost::asio::ip::make_address is unable to parse "::/0" and "0.0.0.0/0", and considering the key will always be these two strings, I updated the code and removed make_address(). Changes have been tested on virtual dual testbeds and worked as expected.

@prsunny could you review again? Thanks!

Summary: Fixes # (issue) This PR is to make linkmgrd subscribe events from ROUTE_TABLE in State DB, and react accordingly: - If any of the two default route state appears to be 'na', linkmgrd should switch to standby. - If both are 'ok', there will be a mux state probing and what happens next depends on linkmgrd state machine and the probing response. Type of change: New feature Motivation for this PR: To make linkmgrd subscribe state DB route event, and handle the switchovers. Documentation: Related PR: sonic-net/sonic-swss#2009

zjswhhh and others added 11 commits December 15, 2021 03:58

Declare handlers in mux manager mux port

ecfac00

Define handlers

Define handlers in linkmgrd

21c8202

Add pull request template for linkmgrd repo (sonic-net#9)

24a11ea

Add pull request template for linkmgrd repo. sign-off: Jing Zhang zhangjing@microsoft.com

Merge branch 'Azure:master' into subscribeRouteEvent

4bd2012

Update route table name

9d38f17

Revert "Update route table name"

88403cc

This reverts commit 9d38f17.

Add unit tests

2f9fed0

Fix syntax issue

12f3b97

Fix syntax issue

ba54965

Fix syntax issue

7d4d091

zjswhhh requested review from lguohan, prsunny, shi-su, theasianpianist and yxieca December 16, 2021 22:30

yxieca reviewed Dec 19, 2021

View reviewed changes

Check IPv4 default route state only.

cc60ba5

yxieca reviewed Jan 4, 2022

View reviewed changes

Coding conventions.

a0ddcf4

prsunny reviewed Jan 12, 2022

View reviewed changes

Force switch when mux state is not equal to standby

6c29483

zjswhhh requested a review from prsunny January 14, 2022 22:27

Merge branch 'Azure:master' into subscribeRouteEvent

d34cf11

prsunny approved these changes Jan 15, 2022

View reviewed changes

lolyu and others added 3 commits January 19, 2022 02:32

Improve PR template (sonic-net#16)

afd2ec8

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

[linkmgrd] update README, set coding style guidance (sonic-net#15)

a784541

Signed-off-by: Ying Xie <ying.xie@microsoft.com>

Add missing intermediate states (sonic-net#16)

550aec3

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

lolyu and others added 5 commits January 19, 2022 02:32

Add TLV support to ICMP payload (sonic-net#11)

5438347

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>

Revert "Add TLV support to ICMP payload (sonic-net#11)"

0dd8eda

This reverts commit 12b9951.

update make_adress to handle invalid ip received

650a3b4

Merge branch 'subscribeRouteEvent' of https://github.com/zjswhhh/soni…

fe583f0

…c-linkmgrd into subscribeRouteEvent

Remove make_address to avoid extra complexity.

e14a524

zjswhhh requested a review from prsunny January 19, 2022 17:15

zjswhhh merged commit 0c23756 into sonic-net:master Jan 19, 2022

zjswhhh deleted the subscribeRouteEvent branch January 19, 2022 20:59

zjswhhh added the Included in 202012 Branch label Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linkmgrd subscribing State DB route event #13

Linkmgrd subscribing State DB route event #13

zjswhhh commented Dec 15, 2021 •

edited

Loading

yxieca Dec 19, 2021

zjswhhh Jan 3, 2022

yxieca Jan 4, 2022

zjswhhh Jan 5, 2022

yxieca Jan 4, 2022

zjswhhh Jan 5, 2022

yxieca Jan 4, 2022

zjswhhh Jan 5, 2022

prsunny Jan 12, 2022

zjswhhh Jan 13, 2022 •

edited

Loading

prsunny Jan 12, 2022

zjswhhh Jan 13, 2022 •

edited

Loading

zjswhhh commented Jan 19, 2022

Linkmgrd subscribing State DB route event #13

Linkmgrd subscribing State DB route event #13

Conversation

zjswhhh commented Dec 15, 2021 • edited Loading

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

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

zjswhhh Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjswhhh Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

zjswhhh commented Jan 19, 2022

zjswhhh commented Dec 15, 2021 •

edited

Loading

zjswhhh Jan 13, 2022 •

edited

Loading

zjswhhh Jan 13, 2022 •

edited

Loading