Skip to content

Commit

Permalink
Enforce switch after config mux to active (#95)
Browse files Browse the repository at this point in the history 
Approach
What is the motivation for this PR?
After config mux state active EthernetXX, the first toggle might not be able to be succeeded as xcvrd has a chance to fail to setup the gRPC connection as the route changed by muxorch needs time to work.

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

How did you do it?
The state change sequence after linkmgrd boots up and receives a mux config active:

(unknown, unknown, up) --> config mux active --> (unknown, active, up) --> mux state unknown[as xcvrd could not setup gRPC connection] --> (unknown, unknown, up) --> probe mux state, return unknown --> (unknown, unknown, up)

Once linkmgrd reaches (unknown, unknown, up), it will keep probing the mux till xcvrd returns either active or standby:
if the probe result is active, we need an extra toggle to active to let show mux status shows active.
if the probe result is standby, we also need an extra toggle to active to notify the gRPC server the port is standby and let show mux status shows active.

How did you verify/test it?
  • Loading branch information
@lolyu @yxieca
lolyu authored and yxieca committed Aug 11, 2022
1 parent 15dbc30 commit e9ede7d
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 11 deletions.
38 changes: 35 additions & 3 deletions src/link_manager/LinkManagerStateMachineActiveActive.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ void ActiveActiveStateMachine::handleMuxStateNotification(mux_state::MuxState::L
MUXLOGWARNING(boost::format("%s: state db mux state: %s") % mMuxPortConfig.getPortName() % mMuxStateName[label]);

mWaitTimer.cancel();
mLastMuxStateNotification = label;

if (mComponentInitState.all()) {
if (mMuxStateMachine.getWaitStateCause() != mux_state::WaitState::WaitStateCause::SwssUpdate) {
Expand Down Expand Up @@ -543,6 +544,15 @@ void ActiveActiveStateMachine::initializeTransitionFunctionTable()
boost::placeholders::_1
);

mStateTransitionHandler[link_prober::LinkProberState::Label::Unknown]
[mux_state::MuxState::Label::Standby]
[link_state::LinkState::Label::Up] =
boost::bind(
&ActiveActiveStateMachine::LinkProberUnknownMuxStandbyLinkUpTransitionFunction,
this,
boost::placeholders::_1
);

mStateTransitionHandler[link_prober::LinkProberState::Label::Active]
[mux_state::MuxState::Label::Unknown]
[link_state::LinkState::Label::Up] =
Expand Down Expand Up @@ -621,9 +631,29 @@ void ActiveActiveStateMachine::LinkProberActiveMuxStandbyLinkUpTransitionFunctio
void ActiveActiveStateMachine::LinkProberUnknownMuxActiveLinkUpTransitionFunction(CompositeState &nextState)
{
MUXLOGINFO(mMuxPortConfig.getPortName());
switchMuxState(nextState, mux_state::MuxState::Label::Standby);
if (mMuxPortConfig.getMode() == common::MuxPortConfig::Mode::Active && mLastMuxStateNotification != mux_state::MuxState::Label::Active) {
// last switch mux state to active failed, try again
switchMuxState(nextState, mux_state::MuxState::Label::Active, true);
} else {
switchMuxState(nextState, mux_state::MuxState::Label::Standby);
}
}

//
// ---> LinkProberUnknownMuxStandbyLinkUpTransitionFunction(CompositeState &nextState);
//
// transition function when entering {LinkProberUnknown, MuxStandby, LinkUp} state
//
void ActiveActiveStateMachine::LinkProberUnknownMuxStandbyLinkUpTransitionFunction(CompositeState &nextState)
{
MUXLOGINFO(mMuxPortConfig.getPortName());
if (mMuxPortConfig.getMode() == common::MuxPortConfig::Mode::Active && mLastMuxStateNotification != mux_state::MuxState::Label::Active) {
// last switch mux state to active failed, try again
switchMuxState(nextState, mux_state::MuxState::Label::Active, true);
}
}


//
// ---> LinkProberActiveMuxUnknownLinkUpTransitionFunction(CompositeState &nextState);
//
Expand Down Expand Up @@ -781,10 +811,12 @@ void ActiveActiveStateMachine::enterPeerLinkProberState(link_prober::LinkProberS
//
void ActiveActiveStateMachine::switchMuxState(
CompositeState &nextState,
mux_state::MuxState::Label label
mux_state::MuxState::Label label,
bool forceSwitch
)
{
if (mMuxPortConfig.getMode() == common::MuxPortConfig::Mode::Auto ||
if (forceSwitch ||
mMuxPortConfig.getMode() == common::MuxPortConfig::Mode::Auto ||
mMuxPortConfig.getMode() == common::MuxPortConfig::Mode::Detached) {
MUXLOGWARNING(
boost::format("%s: Switching MUX state to '%s'") %
Expand Down
27 changes: 19 additions & 8 deletions src/link_manager/LinkManagerStateMachineActiveActive.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,15 @@ class ActiveActiveStateMachine : public LinkManagerStateMachineBase,

/**
* @method handleDefaultRouteStateNotification(const DefaultRoute routeState)
*
*
* @brief handle default route state notification from routeorch
*
*
* @param routeState
*
*
* @return none
*/
*/
void handleDefaultRouteStateNotification(const DefaultRoute routeState) override;

/**
*@method handleGetServerMacNotification
*
Expand Down Expand Up @@ -272,6 +272,15 @@ class ActiveActiveStateMachine : public LinkManagerStateMachineBase,
*/
void LinkProberUnknownMuxActiveLinkUpTransitionFunction(CompositeState &nextState);

/**
* @method LinkProberUnknownMuxStandbyLinkUpTransitionFunction
*
* @brief transition function when entering {LinkProberUnknown, MuxStandby, LinkUp} state
*
* @param nextState reference to composite state
*/
void LinkProberUnknownMuxStandbyLinkUpTransitionFunction(CompositeState &nextState);

/**
* @method LinkProberUnknownMuxUnknownLinkUpTransitionFunction
*
Expand Down Expand Up @@ -374,8 +383,9 @@ class ActiveActiveStateMachine : public LinkManagerStateMachineBase,
*
* @param nextState reference to composite state
* @param label new MuxState label to switch to
* @param forceSwitch force switch mux state, used to match the driver state only
*/
inline void switchMuxState(CompositeState &nextState, mux_state::MuxState::Label label);
inline void switchMuxState(CompositeState &nextState, mux_state::MuxState::Label label, bool forceSwitch = false);

/**
* @method switchPeerMuxState
Expand Down Expand Up @@ -467,9 +477,9 @@ class ActiveActiveStateMachine : public LinkManagerStateMachineBase,

/**
* @method shutdownOrRestartLinkProberOnDefaultRoute()
*
*
* @brief shutdown or restart link prober based on default route state
*
*
* @return none
*/
void shutdownOrRestartLinkProberOnDefaultRoute() override;
Expand Down Expand Up @@ -558,6 +568,7 @@ class ActiveActiveStateMachine : public LinkManagerStateMachineBase,
private: // peer link prober state and mux state
link_prober::LinkProberState::Label mPeerLinkProberState = link_prober::LinkProberState::Label::PeerWait;
mux_state::MuxState::Label mPeerMuxState = mux_state::MuxState::Label::Wait;
mux_state::MuxState::Label mLastMuxStateNotification = mux_state::MuxState::Label::Unknown;

private:
uint32_t mMuxProbeBackoffFactor = 1;
Expand Down
72 changes: 72 additions & 0 deletions test/LinkManagerStateMachineActiveActiveTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,4 +496,76 @@ TEST_F(LinkManagerStateMachineActiveActiveTest, LinkmgrdBootupSequenceHeartBeatF
EXPECT_EQ(mDbInterfacePtr->mLastSetMuxState, mux_state::MuxState::Label::Active);
}

TEST_F(LinkManagerStateMachineActiveActiveTest, LinkmgrdBootupSequenceMuxConfigActiveProbeActive)
{
activateStateMachine();
VALIDATE_STATE(Wait, Wait, Down);

postLinkEvent(link_state::LinkState::Up);
VALIDATE_STATE(Wait, Wait, Up);

postLinkProberEvent(link_prober::LinkProberState::Unknown, 3);
VALIDATE_STATE(Unknown, Standby, Up);
EXPECT_EQ(mDbInterfacePtr->mSetMuxStateInvokeCount, 1);
EXPECT_EQ(mDbInterfacePtr->mLastSetMuxState, mux_state::MuxState::Label::Standby);

handleMuxState("unknown", 3);
VALIDATE_STATE(Unknown, Unknown, Up);

handleMuxConfig("active", 1);
VALIDATE_STATE(Unknown, Active, Up);
EXPECT_EQ(mDbInterfacePtr->mSetMuxStateInvokeCount, 2);
EXPECT_EQ(mDbInterfacePtr->mLastSetMuxState, mux_state::MuxState::Label::Active);

handleMuxState("unknown", 5);
VALIDATE_STATE(Unknown, Unknown, Up);

handleProbeMuxState("unknown", 3);
VALIDATE_STATE(Unknown, Unknown, Up);

// xcvrd now answers the mux probe
handleProbeMuxState("active", 3);
VALIDATE_STATE(Unknown, Active, Up);
EXPECT_EQ(mDbInterfacePtr->mSetMuxStateInvokeCount, 3);
EXPECT_EQ(mDbInterfacePtr->mLastSetMuxState, mux_state::MuxState::Label::Active);

handleMuxState("active", 3);
VALIDATE_STATE(Unknown, Active, Up);
}

TEST_F(LinkManagerStateMachineActiveActiveTest, LinkmgrdBootupSequenceMuxConfigActiveProbeStandby)
{
activateStateMachine();
VALIDATE_STATE(Wait, Wait, Down);

postLinkEvent(link_state::LinkState::Up);
VALIDATE_STATE(Wait, Wait, Up);

postLinkProberEvent(link_prober::LinkProberState::Unknown, 3);
VALIDATE_STATE(Unknown, Standby, Up);
EXPECT_EQ(mDbInterfacePtr->mSetMuxStateInvokeCount, 1);
EXPECT_EQ(mDbInterfacePtr->mLastSetMuxState, mux_state::MuxState::Label::Standby);

handleMuxState("unknown", 3);
VALIDATE_STATE(Unknown, Unknown, Up);

handleMuxConfig("active", 1);
VALIDATE_STATE(Unknown, Active, Up);
EXPECT_EQ(mDbInterfacePtr->mSetMuxStateInvokeCount, 2);
EXPECT_EQ(mDbInterfacePtr->mLastSetMuxState, mux_state::MuxState::Label::Active);

handleMuxState("unknown", 5);
VALIDATE_STATE(Unknown, Unknown, Up);

handleProbeMuxState("unknown", 3);
VALIDATE_STATE(Unknown, Unknown, Up);

// xcvrd now answers the mux probe
handleProbeMuxState("standby", 3);
EXPECT_EQ(mDbInterfacePtr->mSetMuxStateInvokeCount, 3);
EXPECT_EQ(mDbInterfacePtr->mLastSetMuxState, mux_state::MuxState::Label::Active);
handleMuxState("active", 3);
VALIDATE_STATE(Unknown, Active, Up);
}

} /* namespace test */

0 comments on commit e9ede7d

Please sign in to comment.