[routing-manager] stop PD if there is another prefix with higher preference in netdata (#10289)

This commit adds a new IDLE state to PdPrefixManager.

PdPrefixManager enters idle state when PD is enabled and there is
already a BR requesting PD prefix. When there are multiple BRs
publishing PD prefix at the same time, the one with lexcial smaller
prefix wins.

include/openthread/border_routing.h

@@ -165,6 +165,7 @@ typedef enum
     OT_BORDER_ROUTING_DHCP6_PD_STATE_DISABLED, ///< DHCPv6 PD is disabled on the border router.
     OT_BORDER_ROUTING_DHCP6_PD_STATE_STOPPED,  ///< DHCPv6 PD in enabled but won't try to request and publish a prefix.
     OT_BORDER_ROUTING_DHCP6_PD_STATE_RUNNING,  ///< DHCPv6 PD is enabled and will try to request and publish a prefix.
+    OT_BORDER_ROUTING_DHCP6_PD_STATE_IDLE,     ///< DHCPv6 PD is idle; Higher-prf prefix published by other BRs.
 } otBorderRoutingDhcp6PdState;
 
 /**

include/openthread/instance.h

@@ -52,7 +52,7 @@ extern "C" {
  *
  * @note This number versions both OpenThread platform and user APIs.
  */
-#define OPENTHREAD_API_VERSION (450)
+#define OPENTHREAD_API_VERSION (451)
 
 /**
  * @addtogroup api-instance

script/test

@@ -353,6 +353,7 @@ do_build_otbr_docker()
         "-DOT_UPTIME=ON"
         "-DOTBR_DNS_UPSTREAM_QUERY=ON"
         "-DOTBR_DUA_ROUTING=ON"
+        "-DOTBR_DHCP6_PD=ON"
     )
     local args=(
         "BORDER_ROUTING=${BORDER_ROUTING}"

src/cli/cli_br.cpp

@@ -564,6 +564,7 @@ template <> otError Br::Process<Cmd("pd")>(Arg aArgs[])
             "disabled", // (0) OT_BORDER_ROUTING_DHCP6_PD_STATE_DISABLED
             "stopped",  // (1) OT_BORDER_ROUTING_DHCP6_PD_STATE_STOPPED
             "running",  // (2) OT_BORDER_ROUTING_DHCP6_PD_STATE_RUNNING
+            "idle",     // (3) OT_BORDER_ROUTING_DHCP6_PD_STATE_IDLE
         };
 
         static_assert(0 == OT_BORDER_ROUTING_DHCP6_PD_STATE_DISABLED,
@@ -572,6 +573,8 @@ template <> otError Br::Process<Cmd("pd")>(Arg aArgs[])
                       "OT_BORDER_ROUTING_DHCP6_PD_STATE_STOPPED value is not expected!");
         static_assert(2 == OT_BORDER_ROUTING_DHCP6_PD_STATE_RUNNING,
                       "OT_BORDER_ROUTING_DHCP6_PD_STATE_RUNNING value is not expected!");
+        static_assert(3 == OT_BORDER_ROUTING_DHCP6_PD_STATE_IDLE,
+                      "OT_BORDER_ROUTING_DHCP6_PD_STATE_IDLE value is not expected!");
 
         OutputLine("%s", Stringify(otBorderRoutingDhcp6PdGetState(GetInstancePtr()), kDhcpv6PdStateStrings));
     }

src/core/border_router/routing_manager.cpp

@@ -462,6 +462,9 @@ void RoutingManager::EvaluateRoutingPolicy(void)
 #if OPENTHREAD_CONFIG_NAT64_BORDER_ROUTING_ENABLE
     mNat64PrefixManager.Evaluate();
 #endif
+#if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
+    mPdPrefixManager.Evaluate();
+#endif
 
     if (IsInitialPolicyEvaluationDone())
     {
@@ -2291,7 +2294,7 @@ void RoutingManager::OmrPrefixManager::Evaluate(void)
         {
             RemoveLocalFromNetData();
             mLocalPrefix.mPrefix         = Get<RoutingManager>().mPdPrefixManager.GetPrefix();
-            mLocalPrefix.mPreference     = RoutePreference::kRoutePreferenceMedium;
+            mLocalPrefix.mPreference     = PdPrefixManager::kPdRoutePreference;
             mLocalPrefix.mIsDomainPrefix = false;
             LogInfo("Setting local OMR prefix to PD prefix: %s", mLocalPrefix.GetPrefix().ToString().AsCString());
         }
@@ -3845,7 +3848,8 @@ exit:
 RoutingManager::PdPrefixManager::PdPrefixManager(Instance &aInstance)
     : InstanceLocator(aInstance)
     , mEnabled(false)
-    , mIsRunning(false)
+    , mIsStarted(false)
+    , mIsPaused(false)
     , mNumPlatformPioProcessed(0)
     , mNumPlatformRaReceived(0)
     , mLastPlatformRaTime(0)
@@ -3869,8 +3873,20 @@ void RoutingManager::PdPrefixManager::StartStop(bool aStart)
 {
     State oldState = GetState();
 
-    VerifyOrExit(aStart != mIsRunning);
+    VerifyOrExit(aStart != mIsStarted);
-    mIsRunning = aStart;
+    mIsStarted = aStart;
+    EvaluateStateChange(oldState);
+
+exit:
+    return;
+}
+
+void RoutingManager::PdPrefixManager::PauseResume(bool aPause)
+{
+    State oldState = GetState();
+
+    VerifyOrExit(aPause != mIsPaused);
+    mIsPaused = aPause;
     EvaluateStateChange(oldState);
 
 exit:
@@ -3883,12 +3899,22 @@ RoutingManager::PdPrefixManager::State RoutingManager::PdPrefixManager::GetState
 
     if (mEnabled)
     {
-        state = mIsRunning ? kDhcp6PdStateRunning : kDhcp6PdStateStopped;
+        state = mIsStarted ? (mIsPaused ? kDhcp6PdStateIdle : kDhcp6PdStateRunning) : kDhcp6PdStateStopped;
     }
 
     return state;
 }
 
+void RoutingManager::PdPrefixManager::Evaluate(void)
+{
+    const FavoredOmrPrefix &favoredPrefix = Get<RoutingManager>().mOmrPrefixManager.GetFavoredPrefix();
+
+    bool shouldPause = !favoredPrefix.IsEmpty() && favoredPrefix.GetPrefix() != mPrefix.GetPrefix() &&
+                       favoredPrefix.GetPreference() >= kPdRoutePreference;
+
+    PauseResume(/* aPause */ shouldPause);
+}
+
 void RoutingManager::PdPrefixManager::EvaluateStateChange(Dhcp6PdState aOldState)
 {
     State newState = GetState();
@@ -3900,12 +3926,17 @@ void RoutingManager::PdPrefixManager::EvaluateStateChange(Dhcp6PdState aOldState
     {
     case kDhcp6PdStateDisabled:
     case kDhcp6PdStateStopped:
+    case kDhcp6PdStateIdle:
         WithdrawPrefix();
         break;
     case kDhcp6PdStateRunning:
         break;
     }
 
+    // When the prefix is replaced, there will be a short period when the old prefix is still in the netdata, and PD
+    // manager will refuse to request the prefix.
+    // TODO: Either update the comment for the state callback or add a random delay when notifing the upper layer for
+    // state change.
     mStateCallback.InvokeIfSet(static_cast<otBorderRoutingDhcp6PdState>(newState));
 
 exit:
@@ -4129,11 +4160,13 @@ const char *RoutingManager::PdPrefixManager::StateToString(State aState)
         "Disabled", // (0) kDisabled
         "Stopped",  // (1) kStopped
         "Running",  // (2) kRunning
+        "Idle",     // (3) kIdle
     };
 
     static_assert(0 == kDhcp6PdStateDisabled, "kDhcp6PdStateDisabled value is incorrect");
     static_assert(1 == kDhcp6PdStateStopped, "kDhcp6PdStateStopped value is incorrect");
     static_assert(2 == kDhcp6PdStateRunning, "kDhcp6PdStateRunning value is incorrect");
+    static_assert(3 == kDhcp6PdStateIdle, "kDhcp6PdStateIdle value is incorrect");
 
     return kStateStrings[aState];
 }

src/core/border_router/routing_manager.hpp

@@ -145,6 +145,7 @@ public:
         kDhcp6PdStateDisabled = OT_BORDER_ROUTING_DHCP6_PD_STATE_DISABLED, ///< Disabled.
         kDhcp6PdStateStopped  = OT_BORDER_ROUTING_DHCP6_PD_STATE_STOPPED,  ///< Enabled, but currently stopped.
         kDhcp6PdStateRunning  = OT_BORDER_ROUTING_DHCP6_PD_STATE_RUNNING,  ///< Enabled, and running.
+        kDhcp6PdStateIdle     = OT_BORDER_ROUTING_DHCP6_PD_STATE_IDLE,     ///< Enabled, but is not requesting prefix.
     };
 
     /**
@@ -1457,6 +1458,8 @@ private:
 
         typedef Dhcp6PdState State;
 
+        static constexpr RoutePreference kPdRoutePreference = RoutePreference::kRoutePreferenceMedium;
+
         explicit PdPrefixManager(Instance &aInstance);
 
         void               SetEnabled(bool aEnabled);
@@ -1473,6 +1476,7 @@ private:
         Error GetProcessedRaInfo(PdProcessedRaInfo &aPdProcessedRaInfo) const;
         void  HandleTimer(void) { WithdrawPrefix(); }
         void  SetStateCallback(PdCallback aCallback, void *aContext) { mStateCallback.Set(aCallback, aContext); }
+        void  Evaluate(void);
 
     private:
         class PrefixEntry : public OnLinkPrefix
@@ -1489,14 +1493,17 @@ private:
         void EvaluateStateChange(State aOldState);
         void WithdrawPrefix(void);
         void StartStop(bool aStart);
+        void PauseResume(bool aPause);
 
         static const char *StateToString(State aState);
 
         using PrefixTimer   = TimerMilliIn<RoutingManager, &RoutingManager::HandlePdPrefixManagerTimer>;
         using StateCallback = Callback<PdCallback>;
 
-        bool          mEnabled;
+        bool mEnabled;   // Whether PdPrefixManager is enabled. (guards the overall PdPrefixManager functions)
-        bool          mIsRunning;
+        bool mIsStarted; // Whether PdPrefixManager is started. (monitoring prefixes)
+        bool mIsPaused;  // Whether PdPrefixManager is paused. (when there is another BR advertising another prefix)
+
         uint32_t      mNumPlatformPioProcessed;
         uint32_t      mNumPlatformRaReceived;
         TimeMilli     mLastPlatformRaTime;

src/posix/platform/netif.cpp

@@ -1095,7 +1095,8 @@ exit:
     }
 }
 
-#if OPENTHREAD_CONFIG_NAT64_TRANSLATOR_ENABLE || OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
+#if OPENTHREAD_CONFIG_NAT64_TRANSLATOR_ENABLE || \
+    (OPENTHREAD_CONFIG_BORDER_ROUTING_ENABLE && OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE)
 static constexpr uint8_t kIpVersion4 = 4;
 static constexpr uint8_t kIpVersion6 = 6;
 
@@ -1111,7 +1112,7 @@ static uint8_t getIpVersion(const uint8_t *data)
 }
 #endif
 
-#if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
+#if OPENTHREAD_CONFIG_BORDER_ROUTING_ENABLE && OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
 
 /**
  * Returns nullptr if data does not point to a valid ICMPv6 RA message.
@@ -1156,7 +1157,7 @@ static otError tryProcessIcmp6RaMessage(otInstance *aInstance, const uint8_t *da
 exit:
     return error;
 }
-#endif // OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
+#endif // OPENTHREAD_CONFIG_BORDER_ROUTING_ENABLE && OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
 
 #ifdef __linux__
 /**
@@ -1222,7 +1223,7 @@ static void processTransmit(otInstance *aInstance)
     }
 #endif
 
-#if OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
+#if OPENTHREAD_CONFIG_BORDER_ROUTING_ENABLE && OPENTHREAD_CONFIG_BORDER_ROUTING_DHCP6_PD_ENABLE
     if (tryProcessIcmp6RaMessage(aInstance, reinterpret_cast<uint8_t *>(&packet[offset]), rval) == OT_ERROR_NONE)
     {
         ExitNow();

tests/scripts/thread-cert/border_router/test_dhcp6pd.py

@@ -0,0 +1,124 @@
+#!/usr/bin/env python3
+#
+#  Copyright (c) 2024, The OpenThread Authors.
+#  All rights reserved.
+#
+#  Redistribution and use in source and binary forms, with or without
+#  modification, are permitted provided that the following conditions are met:
+#  1. Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer.
+#  2. Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in the
+#     documentation and/or other materials provided with the distribution.
+#  3. Neither the name of the copyright holder nor the
+#     names of its contributors may be used to endorse or promote products
+#     derived from this software without specific prior written permission.
+#
+#  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+#  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+#  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+#  ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+#  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+#  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+#  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+#  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+#  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+#  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+#  POSSIBILITY OF SUCH DAMAGE.
+#
+import ipaddress
+import typing
+import unittest
+
+import config
+import thread_cert
+
+# Test description:
+#
+#   This test verifies DHCP6 PD pauses requesting PD prefix when there is another BR advertising PD prefix.
+#
+
+LEADER = 1
+ROUTER = 2
+
+
+class TestDhcp6Pd(thread_cert.TestCase):
+    SUPPORT_NCP = False
+    USE_MESSAGE_FACTORY = False
+
+    TOPOLOGY = {
+        LEADER: {
+            'name': 'Leader',
+            'allowlist': [ROUTER],
+            'is_otbr': True,
+            'mode': 'rdn',
+        },
+        ROUTER: {
+            'name': 'Router',
+            'allowlist': [LEADER],
+            'is_otbr': True,
+            'mode': 'rdn',
+        },
+    }
+
+    def test(self):
+        leader = self.nodes[LEADER]
+        router = self.nodes[ROUTER]
+
+        #---------------------------------------------------------------
+        # Start the server & client devices.
+
+        # Case 1: Only 1 BR receives PD prefix
+
+        leader.start()
+        self.simulator.go(config.LEADER_STARTUP_DELAY)
+        self.assertEqual(leader.get_state(), 'leader')
+        leader.pd_set_enabled(True)
+
+        router.start()
+        self.simulator.go(config.ROUTER_STARTUP_DELAY)
+        self.assertEqual(router.get_state(), 'router')
+        router.pd_set_enabled(True)
+
+        leader.start_pd_radvd_service("2001:db8:abcd:1234::/64")
+        self.simulator.go(30)
+
+        self.assertSetEqual({leader.pd_state, router.pd_state}, {"running", "idle"})
+
+        self.assertEqual(leader.pd_get_prefix(), "2001:db8:abcd:1234::/64")
+
+        # Case 2: More than one BR receives PD prefix
+        # The BR with "smaller" PD prefix wins
+
+        # Clean up and ensure no prefix is currently published.
+        leader.pd_set_enabled(False)
+        router.pd_set_enabled(False)
+        self.simulator.go(30)
+
+        leader.start_pd_radvd_service("2001:db8:abcd:1234::/64")
+        router.start_pd_radvd_service("2001:db8:1234:abcd::/64")
+        leader.pd_set_enabled(True)
+        router.pd_set_enabled(True)
+        self.simulator.go(30)
+
+        self.assertSetEqual({leader.pd_state, router.pd_state}, {"running", "idle"})
+
+        # Case 3: When the other BR lost PD prefix, the remaining BR should try to request one.
+
+        if leader.pd_state == 'running':
+            br_to_stop = leader
+            br_to_continue = router
+            expected_prefix = "2001:db8:1234:abcd::/64"
+        else:
+            br_to_stop = router
+            br_to_continue = leader
+            expected_prefix = "2001:db8:abcd:1234::/64"
+        br_to_stop.pd_set_enabled(False)
+        self.simulator.go(30)
+
+        self.assertEqual(br_to_continue.pd_state, "running")
+        self.assertEqual(br_to_continue.pd_get_prefix(), expected_prefix)
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/scripts/thread-cert/node.py

@@ -2310,6 +2310,20 @@ class NodeImpl:
         self.send_command(cmd)
         return self._expect_command_output()[0]
 
+    def pd_get_prefix(self):
+        cmd = 'br pd omrprefix'
+        self.send_command(cmd)
+        return self._expect_command_output()[0].split(" ")[0]
+
+    def pd_set_enabled(self, enable):
+        self.send_command('br pd {}'.format("enable" if enable else "disable"))
+        self._expect_done()
+
+    @property
+    def pd_state(self):
+        self.send_command('br pd state')
+        return self._expect_command_output()[0].strip()
+
     def get_netdata_non_nat64_routes(self):
         nat64_routes = []
         routes = self.get_routes()
@@ -4097,6 +4111,33 @@ EOF
         self.bash('service radvd start')
         self.bash('service radvd status')  # Make sure radvd service is running
 
+    def start_pd_radvd_service(self, prefix):
+        self.bash("""cat >/etc/radvd.conf <<EOF
+interface wpan0
+{
+    AdvSendAdvert on;
+
+    AdvReachableTime 20;
+    AdvRetransTimer 20;
+    AdvDefaultLifetime 180;
+    MinRtrAdvInterval 120;
+    MaxRtrAdvInterval 180;
+    AdvDefaultPreference low;
+
+    prefix %s
+    {
+        AdvOnLink on;
+        AdvAutonomous on;
+        AdvRouterAddr off;
+        AdvPreferredLifetime 180;
+        AdvValidLifetime 180;
+    };
+};
+EOF
+""" % (prefix,))
+        self.bash('service radvd start')
+        self.bash('service radvd status')  # Make sure radvd service is running
+
     def stop_radvd_service(self):
         self.bash('service radvd stop')
 

tests/unit/test_routing_manager.cpp

@@ -4213,9 +4213,17 @@ void TestBorderRoutingProcessPlatfromGeneratedNd(void)
         AdvanceTime(1000000);
         VerifyPdOmrPrefix(raPrefix);
 
+        sExpectedRios.Add(newRaPrefix);
+
+        // When the prefix is replaced, there will be a short period when the old prefix is still in the netdata, and PD
+        // manager will refuse to request the prefix.
         SendRouterAdvertToBorderRoutingProcessIcmp6Ra(
             {Pio(raPrefix, 0, 0), Pio(newRaPrefix, kValidLitime, kPreferredLifetime)});
-        sExpectedRios.Add(newRaPrefix);
+        // Advance a short period of time to wait for a stable PD state.
+        AdvanceTime(5000);
+        VerifyOrQuit(sInstance->Get<BorderRouter::RoutingManager>().GetDhcp6PdState() ==
+                     BorderRouter::RoutingManager::kDhcp6PdStateRunning);
+        SendRouterAdvertToBorderRoutingProcessIcmp6Ra({Pio(newRaPrefix, kValidLitime, kPreferredLifetime)});
 
         AdvanceTime(1000000);
         VerifyOrQuit(sExpectedRios.SawAll());