Enforce switch after config mux to active (#95)

lolyu · web-flow · commit 58d8aae2db7e · 2022-07-02T10:14:50.000+08:00
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) --&gt; config mux active --&gt; (unknown, active, up) --&gt; mux state unknown[as xcvrd could not setup gRPC connection] --&gt; (unknown, unknown, up) --&gt; probe mux state, return unknown --&gt; (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?
diff --git a/src/link_manager/LinkManagerStateMachineActiveActive.cpp b/src/link_manager/LinkManagerStateMachineActiveActive.cpp
@@ -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) {
@@ -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] =
@@ -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);
 //
@@ -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'") %
diff --git a/src/link_manager/LinkManagerStateMachineActiveActive.h b/src/link_manager/LinkManagerStateMachineActiveActive.h
@@ -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
      *
@@ -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
      *
@@ -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
@@ -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;
@@ -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;
diff --git a/test/LinkManagerStateMachineActiveActiveTest.cpp b/test/LinkManagerStateMachineActiveActiveTest.cpp
@@ -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 */
