From 5cfbcfcbf4cbf1e33cdeb00db9b7f8903374c4e2 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Wed, 22 Jun 2022 14:17:28 -0700 Subject: [PATCH] [netdata] update `Publisher` to allow change to previous entries (#7827) This commit updates `NetworkData::Publisher` so that a call to `PublishOnMeshPrefix()` or `PublishExternalRoute()` replaces a previous request for the same prefix. In particular, if the new call only changes the flags (e.g., preference level) and the prefix is already added in the Network Data, the change to flags is immediately reflected in the Network Data. This ensures that existing entries in the Network Data are not abruptly removed. Note that a change in the preference level can potentially later cause the entry to be removed from the Network Data after determining there are other nodes that are publishing the same prefix with the same or higher preference. This commit also updates `test_netdata_publisher.py` to test the newly added behavior. --- include/openthread/instance.h | 2 +- include/openthread/netdata_publisher.h | 16 ++++- src/core/thread/network_data_publisher.cpp | 72 +++++++++++++------ src/core/thread/network_data_publisher.hpp | 18 ++++- .../thread-cert/test_netdata_publisher.py | 22 +++++- 5 files changed, 102 insertions(+), 28 deletions(-) diff --git a/include/openthread/instance.h b/include/openthread/instance.h index 5f91d8165..ab4765afc 100644 --- a/include/openthread/instance.h +++ b/include/openthread/instance.h @@ -53,7 +53,7 @@ extern "C" { * @note This number versions both OpenThread platform and user APIs. * */ -#define OPENTHREAD_API_VERSION (219) +#define OPENTHREAD_API_VERSION (220) /** * @addtogroup api-instance diff --git a/include/openthread/netdata_publisher.h b/include/openthread/netdata_publisher.h index fa44686be..8b4af3e8d 100644 --- a/include/openthread/netdata_publisher.h +++ b/include/openthread/netdata_publisher.h @@ -192,12 +192,18 @@ void otNetDataUnpublishDnsSrpService(otInstance *aInstance); * * Only stable entries can be published (i.e.,`aConfig.mStable` MUST be TRUE). * + * A subsequent call to this method will replace a previous request for the same prefix. In particular, if the new call + * only changes the flags (e.g., preference level) and the prefix is already added in the Network Data, the change to + * flags is immediately reflected in the Network Data. This ensures that existing entries in the Network Data are not + * abruptly removed. Note that a change in the preference level can potentially later cause the entry to be removed + * from the Network Data after determining there are other nodes that are publishing the same prefix with the same or + * higher preference. + * * @param[in] aInstance A pointer to an OpenThread instance. * @param[in] aConfig The on-mesh prefix config to publish (MUST NOT be NULL). * * @retval OT_ERROR_NONE The on-mesh prefix is published successfully. * @retval OT_ERROR_INVALID_ARGS The @p aConfig is not valid (bad prefix, invalid flag combinations, or not stable). - * @retval OT_ERROR_ALREADY An entry with the same prefix is already in the published list. * @retval OT_ERROR_NO_BUFS Could not allocate an entry for the new request. Publisher supports a limited number * of entries (shared between on-mesh prefix and external route) determined by config * `OPENTHREAD_CONFIG_NETDATA_PUBLISHER_MAX_PREFIX_ENTRIES`. @@ -213,12 +219,18 @@ otError otNetDataPublishOnMeshPrefix(otInstance *aInstance, const otBorderRouter * * Only stable entries can be published (i.e.,`aConfig.mStable` MUST be TRUE). * + * A subsequent call to this method will replace a previous request for the same prefix. In particular, if the new call + * only changes the flags (e.g., preference level) and the prefix is already added in the Network Data, the change to + * flags is immediately reflected in the Network Data. This ensures that existing entries in the Network Data are not + * abruptly removed. Note that a change in the preference level can potentially later cause the entry to be removed + * from the Network Data after determining there are other nodes that are publishing the same prefix with the same or + * higher preference. + * * @param[in] aInstance A pointer to an OpenThread instance. * @param[in] aConfig The external route config to publish (MUST NOT be NULL). * * @retval OT_ERROR_NONE The external route is published successfully. * @retval OT_ERROR_INVALID_ARGS The @p aConfig is not valid (bad prefix, invalid flag combinations, or not stable). - * @retval OT_ERROR_ALREADY An entry with the same prefix is already in the published list. * @retval OT_ERROR_NO_BUFS Could not allocate an entry for the new request. Publisher supports a limited number * of entries (shared between on-mesh prefix and external route) determined by config * `OPENTHREAD_CONFIG_NETDATA_PUBLISHER_MAX_PREFIX_ENTRIES`. diff --git a/src/core/thread/network_data_publisher.cpp b/src/core/thread/network_data_publisher.cpp index af25ee8f2..8141c760a 100644 --- a/src/core/thread/network_data_publisher.cpp +++ b/src/core/thread/network_data_publisher.cpp @@ -89,13 +89,15 @@ void Publisher::SetPrefixCallback(PrefixCallback aCallback, void *aContext) Error Publisher::PublishOnMeshPrefix(const OnMeshPrefixConfig &aConfig) { - Error error; + Error error = kErrorNone; PrefixEntry *entry; VerifyOrExit(aConfig.IsValid(GetInstance()), error = kErrorInvalidArgs); VerifyOrExit(aConfig.mStable, error = kErrorInvalidArgs); - SuccessOrExit(error = AllocatePrefixEntry(aConfig.GetPrefix(), entry)); + entry = FindOrAllocatePrefixEntry(aConfig.GetPrefix()); + VerifyOrExit(entry != nullptr, error = kErrorNoBufs); + entry->Publish(aConfig); exit: @@ -104,13 +106,15 @@ exit: Error Publisher::PublishExternalRoute(const ExternalRouteConfig &aConfig) { - Error error; + Error error = kErrorNone; PrefixEntry *entry; VerifyOrExit(aConfig.IsValid(GetInstance()), error = kErrorInvalidArgs); VerifyOrExit(aConfig.mStable, error = kErrorInvalidArgs); - SuccessOrExit(error = AllocatePrefixEntry(aConfig.GetPrefix(), entry)); + entry = FindOrAllocatePrefixEntry(aConfig.GetPrefix()); + VerifyOrExit(entry != nullptr, error = kErrorNoBufs); + entry->Publish(aConfig); exit: @@ -145,23 +149,26 @@ exit: return error; } -Error Publisher::AllocatePrefixEntry(const Ip6::Prefix &aPrefix, PrefixEntry *&aEntry) +Publisher::PrefixEntry *Publisher::FindOrAllocatePrefixEntry(const Ip6::Prefix &aPrefix) { - Error error = kErrorNoBufs; + // Returns a matching prefix entry if found, otherwise tries + // to allocate a new entry. - VerifyOrExit(FindMatchingPrefixEntry(aPrefix) == nullptr, error = kErrorAlready); + PrefixEntry *prefixEntry = FindMatchingPrefixEntry(aPrefix); + + VerifyOrExit(prefixEntry == nullptr); for (PrefixEntry &entry : mPrefixEntries) { if (!entry.IsInUse()) { - aEntry = &entry; - ExitNow(error = kErrorNone); + prefixEntry = &entry; + ExitNow(); } } exit: - return error; + return prefixEntry; } Publisher::PrefixEntry *Publisher::FindMatchingPrefixEntry(const Ip6::Prefix &aPrefix) @@ -788,25 +795,50 @@ void Publisher::PrefixEntry::Publish(const OnMeshPrefixConfig &aConfig) { LogInfo("Publishing OnMeshPrefix %s", aConfig.GetPrefix().ToString().AsCString()); - mType = kTypeOnMeshPrefix; - mPrefix = aConfig.GetPrefix(); - mFlags = aConfig.ConvertToTlvFlags(); - - SetState(kToAdd); - - Process(); + Publish(aConfig.GetPrefix(), aConfig.ConvertToTlvFlags(), kTypeOnMeshPrefix); } void Publisher::PrefixEntry::Publish(const ExternalRouteConfig &aConfig) { LogInfo("Publishing ExternalRoute %s", aConfig.GetPrefix().ToString().AsCString()); - mType = kTypeExternalRoute; - mPrefix = aConfig.GetPrefix(); - mFlags = aConfig.ConvertToTlvFlags(); + Publish(aConfig.GetPrefix(), aConfig.ConvertToTlvFlags(), kTypeExternalRoute); +} + +void Publisher::PrefixEntry::Publish(const Ip6::Prefix &aPrefix, uint16_t aNewFlags, Type aNewType) +{ + if (GetState() != kNoEntry) + { + // If this is an existing entry, first we check that there is + // a change in either type or flags. We remove the old entry + // from Network Data if it was added. If the only change is + // to flags (e.g., change to the preference level) and the + // entry was previously added in Network Data, we re-add it + // with the new flags. This ensures that changes to flags are + // immediately reflected in the Network Data. + + State oldState = GetState(); + + VerifyOrExit((mType != aNewType) || (mFlags != aNewFlags)); + + Remove(/* aNextState */ kNoEntry); + + if ((mType == aNewType) && ((oldState == kAdded) || (oldState == kRemoving))) + { + mFlags = aNewFlags; + Add(); + } + } + + VerifyOrExit(GetState() == kNoEntry); + + mType = aNewType; + mPrefix = aPrefix; + mFlags = aNewFlags; SetState(kToAdd); +exit: Process(); } diff --git a/src/core/thread/network_data_publisher.hpp b/src/core/thread/network_data_publisher.hpp index c52283b9b..7d1179095 100644 --- a/src/core/thread/network_data_publisher.hpp +++ b/src/core/thread/network_data_publisher.hpp @@ -214,6 +214,13 @@ public: * * Only stable entries can be published (i.e.,`aConfig.mStable` MUST be `true`). * + * A subsequent call to this method will replace a previous request for the same prefix. In particular if the + * new call only changes the flags (e.g., preference level) and the prefix is already added in the Network Data, + * the change to flags is immediately reflected in the Network Data. This ensures that existing entries in the + * Network Data are not abruptly removed. Note that a change in the preference level can potentially later cause + * the entry to be removed from the Network Data after determining there are other nodes that are publishing the + * same prefix with the same or higher preference. + * * @param[in] aConfig The on-mesh prefix config to publish. * * @retval kErrorNone The on-mesh prefix is published successfully. @@ -232,11 +239,17 @@ public: * * Only stable entries can be published (i.e.,`aConfig.mStable` MUST be `true`). * + * A subsequent call to this method will replace a previous request for the same prefix. In particular if the + * new call only changes the flags (e.g., preference level) and the prefix is already added in the Network Data, + * the change to flags is immediately reflected in the Network Data. This ensures that existing entries in the + * Network Data are not abruptly removed. Note that a change in the preference level can potentially later cause + * the entry to be removed from the Network Data after determining there are other nodes that are publishing the + * same prefix with the same or higher preference. + * * @param[in] aConfig The external route config to publish. * * @retval kErrorNone The external route is published successfully. * @retval kErrorInvalidArgs The @p aConfig is not valid (bad prefix, invalid flag combinations, or not stable). - * @retval kErrorAlready An entry with the same prefix is already in the published list. * @retval kErrorNoBufs Could not allocate an entry for the new request. Publisher supports a limited number * of entries (shared between on-mesh prefix and external route) determined by config * `OPENTHREAD_CONFIG_NETDATA_PUBLISHER_MAX_PREFIX_ENTRIES`. @@ -418,6 +431,7 @@ private: kTypeExternalRoute, }; + void Publish(const Ip6::Prefix &aPrefix, uint16_t aNewFlags, Type aNewType); void Add(void); Error AddOnMeshPrefix(void); Error AddExternalRoute(void); @@ -437,7 +451,7 @@ private: #endif #if OPENTHREAD_CONFIG_BORDER_ROUTER_ENABLE - Error AllocatePrefixEntry(const Ip6::Prefix &aPrefix, PrefixEntry *&aEntry); + PrefixEntry * FindOrAllocatePrefixEntry(const Ip6::Prefix &aPrefix); PrefixEntry * FindMatchingPrefixEntry(const Ip6::Prefix &aPrefix); const PrefixEntry *FindMatchingPrefixEntry(const Ip6::Prefix &aPrefix) const; bool IsAPrefixEntry(const Entry &aEntry) const; diff --git a/tests/scripts/thread-cert/test_netdata_publisher.py b/tests/scripts/thread-cert/test_netdata_publisher.py index 3cd2f0c13..98d1ea224 100755 --- a/tests/scripts/thread-cert/test_netdata_publisher.py +++ b/tests/scripts/thread-cert/test_netdata_publisher.py @@ -57,10 +57,10 @@ END_DEV5 = 11 WAIT_TIME = 55 -ON_MESH_PREFIX = 'fd00:1234::/64' +ON_MESH_PREFIX = 'fd00:1234:0:0::/64' ON_MESH_FLAGS = 'paso' -EXTERNAL_ROUTE = 'fd00:abce::/64' +EXTERNAL_ROUTE = 'fd00:abce:0:0::/64' EXTERNAL_FLAGS = 's' ANYCAST_SEQ_NUM = 4 @@ -444,6 +444,8 @@ class NetDataPublisher(thread_cert.TestCase): #--------------------------------------------------------------------------------- # External route + # Publish same external route on all nodes with low preference. + num = 0 for node in nodes: node.netdata_publish_route(EXTERNAL_ROUTE, EXTERNAL_FLAGS, 'low') @@ -452,12 +454,26 @@ class NetDataPublisher(thread_cert.TestCase): routes = leader.get_routes() self.check_num_of_routes(routes, num, 0, 0) - leader.netdata_unpublish_prefix(EXTERNAL_ROUTE) + # Change the preference level of the existing entry on leader to high. + leader.netdata_publish_route(EXTERNAL_ROUTE, EXTERNAL_FLAGS, 'high') self.simulator.go(WAIT_TIME) routes = leader.get_routes() self.check_num_of_routes(routes, num - 1, 0, 1) + # Publish the same prefix on leader as an on-mesh prefix. Make + # sure it is removed from external routes and now seen in the + # prefix list. + + leader.netdata_publish_prefix(EXTERNAL_ROUTE, ON_MESH_FLAGS, 'low') + self.simulator.go(WAIT_TIME) + routes = leader.get_routes() + self.check_num_of_routes(routes, num - 1, 0, 0) + + prefixes = leader.get_prefixes() + print(prefixes) + self.assertIn(EXTERNAL_ROUTE, [prefix.split()[0] for prefix in prefixes]) + if __name__ == '__main__': unittest.main()