From afe2a0d29299128d314b93c9e4726e62d3116948 Mon Sep 17 00:00:00 2001 From: Kamil Cudnik Date: Wed, 30 Oct 2019 16:54:49 +0100 Subject: [PATCH] [vs]: Fix learn fdb events after fdb flush event (#524) Before this fix if flush event is received, vlan id in fdb_info was always set to zero, and it should be extracted from bv_id field in fdb_entry, this was causing to not find exact fdb entry in g_fdb_info_set and since those entries were always there, they were not "learned" again --- lib/src/sai_redis_fdb.cpp | 4 +- saiplayer/saiplayer.cpp | 90 +++++++++++++++++++++++++++++++++++ syncd/syncd_notifications.cpp | 22 +++++++++ vslib/src/sai_vs_fdb.cpp | 28 ++++++++++- vslib/src/sai_vs_hostintf.cpp | 19 ++++++++ 5 files changed, 160 insertions(+), 3 deletions(-) diff --git a/lib/src/sai_redis_fdb.cpp b/lib/src/sai_redis_fdb.cpp index 4ed7c889198b..684a72d6241f 100644 --- a/lib/src/sai_redis_fdb.cpp +++ b/lib/src/sai_redis_fdb.cpp @@ -19,7 +19,7 @@ sai_status_t internal_redis_flush_fdb_entries( std::string key = str_object_type + ":" + sai_serialize_object_id(switch_id); - SWSS_LOG_DEBUG("flush key: %s, fields: %lu", key.c_str(), entry.size()); + SWSS_LOG_NOTICE("flush key: %s, fields: %lu", key.c_str(), entry.size()); if (g_record) { @@ -76,7 +76,7 @@ sai_status_t internal_redis_flush_fdb_entries( recordLine("F|" + str_status); } - SWSS_LOG_DEBUG("flush status: %d", status); + SWSS_LOG_NOTICE("flush status: %d", status); return status; } diff --git a/saiplayer/saiplayer.cpp b/saiplayer/saiplayer.cpp index 267ce8831446..bc678981cecd 100644 --- a/saiplayer/saiplayer.cpp +++ b/saiplayer/saiplayer.cpp @@ -857,6 +857,76 @@ void performNotifySyncd(const std::string& request, const std::string response) // OK } +void performFdbFlush( + _In_ const std::string& request, + _In_ const std::string response) +{ + SWSS_LOG_ENTER(); + + // 2017-05-13.20:47:24.883499|f|SAI_OBJECT_TYPE_FDB_FLUSH:oid:0x21000000000000| + // 2017-05-13.20:47:24.883499|F|SAI_STATUS_SUCCESS + + // timestamp|action|data + auto r = swss::tokenize(request, '|'); + auto R = swss::tokenize(response, '|'); + + if (r[1] != "f" || R[1] != "F") + { + SWSS_LOG_THROW("invalid fdb flush request/response %s/%s", request.c_str(), response.c_str()); + } + + if (r.size() > 3 && r[3].size() != 0) + { + SWSS_LOG_NOTICE("%zu %zu, %s", r.size(), r[3].size(), r[3].c_str()); + // TODO currently we support only flush fdb entries with no attributes + SWSS_LOG_THROW("currently fdb flush supports only no attributes, but some given: %s", request.c_str()); + } + + // objecttype:objectid (object id may contain ':') + auto& data = r[2]; + auto start = data.find_first_of(":"); + auto str_object_type = data.substr(0, start); + auto str_object_id = data.substr(start + 1); + + sai_object_type_t ot = deserialize_object_type(str_object_type); + + if (ot != SAI_OBJECT_TYPE_FDB_FLUSH) + { + SWSS_LOG_THROW("expected object type %s, but got: %s on %s", + sai_serialize_object_type(SAI_OBJECT_TYPE_FDB_FLUSH).c_str(), + str_object_type.c_str(), + request.c_str()); + } + + sai_object_id_t local_switch_id; + sai_deserialize_object_id(str_object_id, local_switch_id); + + if (sai_switch_id_query(local_switch_id) != local_switch_id) + { + SWSS_LOG_THROW("fdb flush object is not switch id: %s, switch_id_query: %s", + str_object_id.c_str(), + sai_serialize_object_id(sai_switch_id_query(local_switch_id)).c_str()); + } + + auto switch_id = translate_local_to_redis(local_switch_id); + + // TODO currently we support only flush fdb entries with no attributes + sai_status_t status = sai_metadata_sai_fdb_api->flush_fdb_entries(switch_id, 0, NULL); + + // check status + sai_status_t expected_status; + sai_deserialize_status(R[2], expected_status); + + if (status != expected_status) + { + SWSS_LOG_THROW("fdb flush got status %s, but expecting: %s", + sai_serialize_status(status).c_str(), + R[2].c_str()); + } + + // fdb flush OK +} + std::vector tokenize( _In_ std::string input, _In_ const std::string &delim) @@ -1189,6 +1259,25 @@ int replay(int argc, char **argv) performNotifySyncd(line, response); } continue; + + case 'f': + { + std::string response; + + do + { + // this line may be notification, we need to skip + if (!std::getline(infile, response)) + { + SWSS_LOG_THROW("failed to read next file from file, previous: %s", line.c_str()); + } + } + while (response[response.find_first_of("|") + 1] == 'n'); + + performFdbFlush(line, response); + } + continue; + case '@': performSleep(line); continue; @@ -1217,6 +1306,7 @@ int replay(int argc, char **argv) continue; // skip over query responses case '#': case 'n': + SWSS_LOG_INFO("skipping op %c line %s", op, line.c_str()); continue; // skip comment and notification default: diff --git a/syncd/syncd_notifications.cpp b/syncd/syncd_notifications.cpp index 0d751018cb2f..1954ee9be4df 100644 --- a/syncd/syncd_notifications.cpp +++ b/syncd/syncd_notifications.cpp @@ -324,6 +324,23 @@ bool check_fdb_event_notification_data( return result; } +bool contains_fdb_flush_event( + _In_ uint32_t count, + _In_ const sai_fdb_event_notification_data_t *data) +{ + SWSS_LOG_ENTER(); + + sai_mac_t mac = { 0, 0, 0, 0, 0, 0 }; + + for (uint32_t idx = 0; idx < count; idx++) + { + if (memcmp(mac, data[idx].fdb_entry.mac_address, sizeof(mac)) == 0) + return true; + } + + return false; +} + void process_on_fdb_event( _In_ uint32_t count, _In_ sai_fdb_event_notification_data_t *data) @@ -468,6 +485,11 @@ void handle_fdb_event( sai_deserialize_fdb_event_ntf(data, count, &fdbevent); + if (contains_fdb_flush_event(count, fdbevent)) + { + SWSS_LOG_NOTICE("got fdb flush event: %s", data.c_str()); + } + process_on_fdb_event(count, fdbevent); sai_deserialize_free_fdb_event_ntf(count, fdbevent); diff --git a/vslib/src/sai_vs_fdb.cpp b/vslib/src/sai_vs_fdb.cpp index 3a6974d25abd..0f273180ea73 100644 --- a/vslib/src/sai_vs_fdb.cpp +++ b/vslib/src/sai_vs_fdb.cpp @@ -135,16 +135,42 @@ sai_status_t internal_vs_flush_fdb_entries( fdb_info_t fi; + // will set vlan to 0, we need to recreate that information memset(&fi, 0, sizeof(fi)); + // If fdb entry has bv_id set to vlan object type then we can try to get vlan number from + // that object and populate vlan_id in fdb_info. If bv_id is bridge object type then vlan + // must be zero, since there can be only 1 (assuming) mac address on a given bridge. + sai_deserialize_fdb_entry(it->first, fi.fdb_entry); + if (sai_object_type_query(fi.fdb_entry.bv_id) == SAI_OBJECT_TYPE_VLAN) + { + sai_attribute_t attr; + + attr.id = SAI_VLAN_ATTR_VLAN_ID; + + sai_status_t status = vs_generic_get(SAI_OBJECT_TYPE_VLAN, fi.fdb_entry.bv_id, 1, &attr); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("failed to get vlan_id for vlan object: %s", + sai_serialize_object_id(fi.fdb_entry.bv_id).c_str()); + + return SAI_STATUS_FAILURE; + } + + SWSS_LOG_INFO("got vlan id %d", attr.value.u16); + + fi.vlan_id = attr.value.u16; + } + auto fit = g_fdb_info_set.find(fi); if (fit == g_fdb_info_set.end()) { // this may happen if vlan is invalid - SWSS_LOG_ERROR("failed to find fdb entry in info set: %s", it->first.c_str()); + SWSS_LOG_ERROR("failed to find fdb entry in info set: %s, learn for this MAC will be disabled", it->first.c_str()); } else { diff --git a/vslib/src/sai_vs_hostintf.cpp b/vslib/src/sai_vs_hostintf.cpp index 718dcd3e48fd..c59f956b9cf5 100644 --- a/vslib/src/sai_vs_hostintf.cpp +++ b/vslib/src/sai_vs_hostintf.cpp @@ -225,6 +225,8 @@ void findBridgeVlanForPortVlan( // iterate via all bridge ports to find match on port id + bool bv_id_set = false; + for (auto it = objectHash.begin(); it != objectHash.end(); ++it) { sai_object_id_t bpid; @@ -289,6 +291,7 @@ void findBridgeVlanForPortVlan( sai_serialize_object_id(bridge_id).c_str(), attr.value.s32); bv_id = bridge_id; + bv_id_set = true; } else { @@ -315,6 +318,7 @@ void findBridgeVlanForPortVlan( if (vlan_id == attr.value.u16) { bv_id = vlan_oid; + bv_id_set = true; break; } } @@ -322,6 +326,13 @@ void findBridgeVlanForPortVlan( break; } + + if (!bv_id_set) + { + SWSS_LOG_WARN("failed to find bv_id for vlan %d and port_id %s", + vlan_id, + sai_serialize_object_id(port_id).c_str()); + } } bool getLagFromPort( @@ -541,6 +552,7 @@ void process_packet_for_fdb_event( return; } + // untagged port vlan (default is 1, but may change setting port attr) vlan_id = attr.value.u16; } } @@ -586,10 +598,17 @@ void process_packet_for_fdb_event( if (fi.fdb_entry.bv_id == SAI_NULL_OBJECT_ID) { + SWSS_LOG_WARN("skipping mac learn for %s, since BV_ID was not found for mac", + sai_serialize_fdb_entry(fi.fdb_entry).c_str()); + // bridge was not found, skip mac learning return; } + SWSS_LOG_INFO("inserting to fdb_info set: %s, vid: %d", + sai_serialize_fdb_entry(fi.fdb_entry).c_str(), + fi.vlan_id); + g_fdb_info_set.insert(fi); processFdbInfo(fi, SAI_FDB_EVENT_LEARNED);