Dynamic FDB entries should be skipped during warm boot if aged out in h/w

Signed-off-by: Vishrant Vasavada <vvasavada@nexthop.ai>

Author: vvasavada-nexthop

fboss/agent/hw/sai/api/FdbApi.h

@@ -102,6 +102,12 @@ SAI_ATTRIBUTE_NAME(Fdb, Metadata)
 template <>
 struct IsSaiEntryStruct<SaiFdbTraits::FdbEntry> : public std::true_type {};
 
+// Dynamic FDB entries age out in hardware on their own, so an entry FBOSS
+// tracks - in the store, or in a warm boot state written moments earlier - may
+// already be gone from HW.
+template <>
+struct SaiObjectMayBeMissingInHw<SaiFdbTraits> : public std::true_type {};
+
 class FdbApi : public SaiApi<FdbApi> {
  public:
   static constexpr sai_api_t ApiType = SAI_API_FDB;

fboss/agent/hw/sai/api/Traits.h

@@ -488,6 +488,14 @@ concept SaiAttributeTuple =
 template <typename SaiObjectTraits>
 struct IsSaiObjectOwnedByAdapter : public std::false_type {};
 
+/*
+ * Hardware can delete objects of some types on its own, without the agent
+ * asking. For those, an object FBOSS still tracks may already be gone from
+ * hardware, so ITEM_NOT_FOUND is an expected outcome rather than a fatal one.
+ */
+template <typename SaiObjectTraits>
+struct SaiObjectMayBeMissingInHw : public std::false_type {};
+
 template <typename SaiObjectTraits>
 struct SaiObjectHasStats : public std::false_type {};
 

fboss/agent/hw/sai/store/SaiObject.h

@@ -536,7 +536,8 @@ class SaiObject {
   bool ownedByAdapter_{IsSaiObjectOwnedByAdapter<SaiObjectTraits>::value};
   // For some object types we can ignore missing in HW errors
   // on when deleting.
-  bool ignoreMissingInHwOnDelete_{false};
+  bool ignoreMissingInHwOnDelete_{
+      SaiObjectMayBeMissingInHw<SaiObjectTraits>::value};
   // For some object types we want to skip remove call when deleting
   bool skipRemove_{false};
   typename SaiObjectTraits::AdapterKey adapterKey_;

fboss/agent/hw/sai/store/SaiStore.h

@@ -184,10 +184,13 @@ class SaiObjectStore {
           keys.end());
     }
     for (const auto& k : keys) {
-      ObjectType obj = getObject(k, adapterKeys2AdapterHostKey);
-      auto adapterHostKey = obj.adapterHostKey();
-      XLOGF(DBG5, "SaiStore reloaded {}", obj);
-      auto ins = objects_.refOrInsert(adapterHostKey, std::move(obj));
+      auto obj = getObjectIfInHw(k, adapterKeys2AdapterHostKey);
+      if (!obj) {
+        continue;
+      }
+      auto adapterHostKey = obj->adapterHostKey();
+      XLOGF(DBG5, "SaiStore reloaded {}", *obj);
+      auto ins = objects_.refOrInsert(adapterHostKey, std::move(*obj));
       if (!ins.second) {
         XLOG(FATAL) << "[" << saiObjectTypeToString(SaiObjectTraits::ObjectType)
                     << "]" << " Unexpected duplicate adapterHostKey";
@@ -495,6 +498,37 @@ class SaiObjectStore {
         });
   }
 
+  /*
+   * Warm boot reloads objects from adapter keys saved at graceful exit. For
+   * object types hardware can delete on its own, that snapshot is only a best
+   * effort: e.g. a dynamic FDB entry can age out after the keys are written, or
+   * after an age event the agent never got to process. Skip such keys instead
+   * of letting the missing object abort warm boot - switch state replays
+   * whatever is still needed, and removeUnclaimedDynanicEntries() cleans up the
+   * rest.
+   */
+  std::optional<ObjectType> getObjectIfInHw(
+      const typename SaiObjectTraits::AdapterKey& key,
+      const folly::dynamic* adapterKeys2AdapterHostKey) {
+    if constexpr (SaiObjectMayBeMissingInHw<SaiObjectTraits>::value) {
+      try {
+        return getObject(key, adapterKeys2AdapterHostKey);
+      } catch (const SaiApiError& e) {
+        if (e.getSaiStatus() != SAI_STATUS_ITEM_NOT_FOUND) {
+          throw;
+        }
+        XLOGF(
+            WARN,
+            "[{}] skipping {} on reload, no longer present in hardware",
+            objectTypeName().str(),
+            key);
+        return std::nullopt;
+      }
+    } else {
+      return getObject(key, adapterKeys2AdapterHostKey);
+    }
+  }
+
   ObjectType getObject(
       typename ObjectTraits::AdapterKey key,
       const folly::dynamic* adapterKey2AdapterHostKey) {

fboss/agent/hw/sai/store/tests/FdbStoreTest.cpp

@@ -80,6 +80,33 @@ TEST_F(SaiStoreTest, fdbSetBridgePort) {
   EXPECT_EQ(GET_OPT_ATTR(Fdb, Metadata, obj->attributes()), 23);
 }
 
+// Hardware ages dynamic FDB entries out on its own, so an entry saved in warm
+// boot state can be gone by the time the store reloads it. Reload must skip it
+// rather than let ITEM_NOT_FOUND abort warm boot.
+TEST_F(SaiStoreTest, fdbAgedOutBeforeReload) {
+  auto& fdbApi = saiApiTable->fdbApi();
+  SaiFdbTraits::FdbEntry present(0, 10, folly::MacAddress{"42:42:42:42:42:42"});
+  SaiFdbTraits::FdbEntry aged(0, 10, folly::MacAddress{"42:42:42:42:42:43"});
+  fdbApi.create<SaiFdbTraits>(present, {SAI_FDB_ENTRY_TYPE_STATIC, 42, 24});
+  fdbApi.create<SaiFdbTraits>(aged, {SAI_FDB_ENTRY_TYPE_DYNAMIC, 42, 24});
+
+  // Warm boot state is written with both entries in it
+  folly::dynamic adapterKeys;
+  {
+    SaiStore preWarmBoot(0);
+    preWarmBoot.reload();
+    adapterKeys = preWarmBoot.adapterKeysFollyDynamic();
+  }
+  // ... and HW ages the dynamic one out while the agent is down
+  fdbApi.remove(aged);
+
+  SaiStore postWarmBoot(0);
+  postWarmBoot.reload(&adapterKeys);
+  auto& store = postWarmBoot.get<SaiFdbTraits>();
+  EXPECT_NE(store.get(present), nullptr);
+  EXPECT_EQ(store.get(aged), nullptr);
+}
+
 TEST_F(SaiStoreTest, fdbSerDeser) {
   auto& fdbApi = saiApiTable->fdbApi();
   folly::MacAddress mac{"42:42:42:42:42:42"};

fboss/agent/hw/sai/switch/SaiFdbManager.cpp

@@ -52,14 +52,14 @@ void ManagedFdbEntry::createObject(PublisherObjects objects) {
   SaiFdbTraits::CreateAttributes attributes{type_, bridgePortId, metadata_};
 
   auto fdbEntry = manager_->createSaiObject(entry, attributes, intfIDAndMac_);
-  // For FDB entry, on delete, Ignore error if entry was already removed from
-  // HW. One scenario where this can occur is the following
+  // Missing in HW is tolerated for FDB entries on delete and on warm boot
+  // reload - see SaiObjectMayBeMissingInHw<SaiFdbTraits>. One scenario where
+  // this can occur is the following
   // - We learn a MAC and install it in HW
   // - Later we get a state update to transform this into a STATIC FDB entry
   // - Meanwhile the dynamic MAC ages out and gets deleted
   // - While processing the state delta for changed MAC entry, we try to
   // delete the dynamic entry before adding static entry
-  fdbEntry->setIgnoreMissingInHwOnDelete(true);
   // If pending_L2_entry is not supported, dynamic l2 entry is already deleted
   // from HW table upon receiving the l2 age event. Therefore, no need to make
   // the remove call down to SDK layer.