diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index cefc2efbb1..1e62547b41 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -8,6 +8,7 @@ extern sai_object_id_t gSwitchId; +extern IntfsOrch *gIntfsOrch; extern NeighOrch *gNeighOrch; extern RouteOrch *gRouteOrch; extern NhgOrch *gNhgOrch; @@ -58,6 +59,8 @@ void NhgOrch::doTask(Consumer& consumer) string aliases; string weights; string mpls_nhs; + string nhgs; + bool is_recursive = false; /* Get group's next hop IPs and aliases */ for (auto i : kfvFieldsValues(t)) @@ -73,24 +76,102 @@ void NhgOrch::doTask(Consumer& consumer) if (fvField(i) == "mpls_nh") mpls_nhs = fvValue(i); - } - /* Split ips and alaises strings into vectors of tokens. */ + if (fvField(i) == "nexthop_group") + { + nhgs = fvValue(i); + if (!nhgs.empty()) + is_recursive = true; + } + } + /* A NHG should not have both regular(ip/alias) and recursive fields */ + if (is_recursive && (!ips.empty() || !aliases.empty())) + { + SWSS_LOG_ERROR("Nexthop group %s has both regular(ip/alias) and recursive fields", index.c_str()); + it = consumer.m_toSync.erase(it); + continue; + } + /* Split ips and aliases strings into vectors of tokens. */ vector ipv = tokenize(ips, ','); vector alsv = tokenize(aliases, ','); vector mpls_nhv = tokenize(mpls_nhs, ','); + vector nhgv = tokenize(nhgs, NHG_DELIMITER); /* Create the next hop group key. */ string nhg_str; - for (uint32_t i = 0; i < ipv.size(); i++) + /* Keeps track of any non-existing member of a recursive nexthop group */ + bool non_existent_member = false; + + if (is_recursive) { - if (i) nhg_str += NHG_DELIMITER; - if (!mpls_nhv.empty() && mpls_nhv[i] != "na") + SWSS_LOG_INFO("Adding recursive nexthop group %s with %s", index.c_str(), nhgs.c_str()); + + /* Reset the "nexthop_group" field and update it with only the existing members */ + nhgs = ""; + + /* Check if any of the members are a recursive or temporary nexthop group */ + bool invalid_member = false; + + for (auto& nhgm : nhgv) + { + const auto& nhgm_it = m_syncdNextHopGroups.find(nhgm); + if (nhgm_it == m_syncdNextHopGroups.end()) + { + SWSS_LOG_INFO("Member nexthop group %s in parent nhg %s not ready", + nhgm.c_str(), index.c_str()); + + non_existent_member = true; + continue; + } + if ((nhgm_it->second.nhg) && + (nhgm_it->second.nhg->isRecursive() || nhgm_it->second.nhg->isTemp())) + { + SWSS_LOG_ERROR("Invalid member nexthop group %s in parent nhg %s", + nhgm.c_str(), index.c_str()); + + invalid_member = true; + break; + } + /* Keep only the members which exist in the local cache */ + if (nhgs.empty()) + nhgs = nhgm; + else + nhgs += NHG_DELIMITER + nhgm; + } + if (invalid_member) { - nhg_str += mpls_nhv[i] + LABELSTACK_DELIMITER; + it = consumer.m_toSync.erase(it); + continue; + } + /* If no members are present */ + if (nhgs.empty()) + { + it++; + continue; + } + + /* Form nexthopgroup key with the nexthopgroup keys of available members */ + nhgv = tokenize(nhgs, NHG_DELIMITER); + + for (uint32_t i = 0; i < nhgv.size(); i++) + { + if (i) nhg_str += NHG_DELIMITER; + + nhg_str += m_syncdNextHopGroups.at(nhgv[i]).nhg->getKey().to_string(); + } + } + else + { + for (uint32_t i = 0; i < ipv.size(); i++) + { + if (i) nhg_str += NHG_DELIMITER; + if (!mpls_nhv.empty() && mpls_nhv[i] != "na") + { + nhg_str += mpls_nhv[i] + LABELSTACK_DELIMITER; + } + nhg_str += ipv[i] + NH_DELIMITER + alsv[i]; } - nhg_str += ipv[i] + NH_DELIMITER + alsv[i]; } NextHopGroupKey nhg_key = NextHopGroupKey(nhg_str, weights); @@ -98,6 +179,8 @@ void NhgOrch::doTask(Consumer& consumer) /* If the group does not exist, create one. */ if (nhg_it == m_syncdNextHopGroups.end()) { + SWSS_LOG_INFO("Create nexthop group %s with %s", index.c_str(), nhg_str.c_str()); + /* * If we've reached the NHG limit, we're going to create a temporary * group, represented by one of it's NH only until we have @@ -133,10 +216,21 @@ void NhgOrch::doTask(Consumer& consumer) else { auto nhg = std::make_unique(nhg_key, false); - success = nhg->sync(); + /* + * Mark the nexthop group as recursive so as to create a + * nexthop group object even if it has just one available path + */ + nhg->setRecursive(is_recursive); + + success = nhg->sync(); if (success) { + /* Keep the msg in loop if any member path is not available yet */ + if (is_recursive && non_existent_member) + { + success = false; + } m_syncdNextHopGroups.emplace(index, NhgEntry(std::move(nhg))); } } @@ -144,6 +238,8 @@ void NhgOrch::doTask(Consumer& consumer) /* If the group exists, update it. */ else { + SWSS_LOG_INFO("Update nexthop group %s with %s", index.c_str(), nhg_str.c_str()); + const auto& nhg_ptr = nhg_it->second.nhg; /* @@ -216,6 +312,12 @@ void NhgOrch::doTask(Consumer& consumer) else { success = nhg_ptr->update(nhg_key); + + /* Keep the msg in loop if any member path is not available yet */ + if (is_recursive && non_existent_member) + { + success = false; + } } } } @@ -367,7 +469,11 @@ sai_object_id_t NextHopGroupMember::getNhId() const sai_object_id_t nh_id = SAI_NULL_OBJECT_ID; - if (gNeighOrch->hasNextHop(m_key)) + if (m_key.isIntfNextHop()) + { + nh_id = gIntfsOrch->getRouterIntfsId(m_key.alias); + } + else if (gNeighOrch->hasNextHop(m_key)) { nh_id = gNeighOrch->getNextHopId(m_key); } @@ -484,7 +590,7 @@ NextHopGroupMember::~NextHopGroupMember() * Params: IN key - The next hop group's key. * Returns: Nothing. */ -NextHopGroup::NextHopGroup(const NextHopGroupKey& key, bool is_temp) : NhgCommon(key), m_is_temp(is_temp) +NextHopGroup::NextHopGroup(const NextHopGroupKey& key, bool is_temp) : NhgCommon(key), m_is_temp(is_temp), m_is_recursive(false) { SWSS_LOG_ENTER(); @@ -506,6 +612,7 @@ NextHopGroup& NextHopGroup::operator=(NextHopGroup&& nhg) SWSS_LOG_ENTER(); m_is_temp = nhg.m_is_temp; + m_is_recursive = nhg.m_is_recursive; NhgCommon::operator=(std::move(nhg)); @@ -532,11 +639,8 @@ bool NextHopGroup::sync() return true; } - /* - * If the group is temporary, the group ID will be the only member's NH - * ID. - */ - if (m_is_temp) + /* If the group is non-recursive with single member, the group ID will be the only member's NH ID */ + if (!isRecursive() && (m_members.size() == 1)) { const NextHopGroupMember& nhgm = m_members.begin()->second; sai_object_id_t nhid = nhgm.getNhId(); @@ -549,6 +653,12 @@ bool NextHopGroup::sync() else { m_id = nhid; + + auto nh_key = nhgm.getKey(); + if (nh_key.isIntfNextHop()) + gIntfsOrch->increaseRouterIntfsRefCount(nh_key.alias); + else + gNeighOrch->increaseNextHopRefCount(nh_key); } } else @@ -663,9 +773,21 @@ bool NextHopGroup::remove() { SWSS_LOG_ENTER(); - // If the group is temporary, there is nothing to be done - just reset the ID. - if (m_is_temp) + if (!isSynced()) { + return true; + } + // If the group is temporary or non-recursive, update the neigh or rif ref-count and reset the ID. + if (m_is_temp || + (!isRecursive() && m_members.size() == 1)) + { + const NextHopGroupMember& nhgm = m_members.begin()->second; + auto nh_key = nhgm.getKey(); + if (nh_key.isIntfNextHop()) + gIntfsOrch->decreaseRouterIntfsRefCount(nh_key.alias); + else + gNeighOrch->decreaseNextHopRefCount(nh_key); + m_id = SAI_NULL_OBJECT_ID; return true; } @@ -687,6 +809,9 @@ bool NextHopGroup::syncMembers(const std::set& nh_keys) { SWSS_LOG_ENTER(); + /* This method should not be called for single-membered non-recursive nexthop groups */ + assert(isRecursive() || (m_members.size() > 1)); + ObjectBulker nextHopGroupMemberBulker(sai_next_hop_group_api, gSwitchId, gMaxBulkSize); /* @@ -776,6 +901,23 @@ bool NextHopGroup::update(const NextHopGroupKey& nhg_key) { SWSS_LOG_ENTER(); + if (!isSynced() || + (!isRecursive() && (m_members.size() == 1 || nhg_key.getSize() == 1))) + { + bool was_synced = isSynced(); + bool was_temp = isTemp(); + *this = NextHopGroup(nhg_key, false); + + /* + * For temporary nexthop group being updated, set the recursive flag + * as it is expected to get promoted to multiple NHG + */ + setRecursive(was_temp); + + /* Sync the group only if it was synced before. */ + return (was_synced ? sync() : true); + } + /* Update the key. */ m_key = nhg_key; @@ -891,7 +1033,12 @@ bool NextHopGroup::validateNextHop(const NextHopKey& nh_key) { SWSS_LOG_ENTER(); - return syncMembers({nh_key}); + if (isRecursive() || (m_members.size() > 1)) + { + return syncMembers({nh_key}); + } + + return true; } /* @@ -905,5 +1052,10 @@ bool NextHopGroup::invalidateNextHop(const NextHopKey& nh_key) { SWSS_LOG_ENTER(); - return removeMembers({nh_key}); + if (isRecursive() || (m_members.size() > 1)) + { + return removeMembers({nh_key}); + } + + return true; } diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index 225d3ffaf2..d8a92e6131 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -54,7 +54,7 @@ class NextHopGroup : public NhgCommon& nh_keys) override; diff --git a/tests/test_nhg.py b/tests/test_nhg.py index 6647a8d0de..b9e125f760 100644 --- a/tests/test_nhg.py +++ b/tests/test_nhg.py @@ -84,6 +84,8 @@ def get_nhg_map_id(self, nhg_map_index): # Create a NHG fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), ('ifname', 'Ethernet0')]) + nhg_ps.set('_testnhg', fvs) + fvs = swsscommon.FieldValuePairs([('nexthop_group', '_testnhg')]) nhg_ps.set('testnhg', fvs) # Add a CBF NHG pointing to the given map @@ -98,6 +100,7 @@ def get_nhg_map_id(self, nhg_map_index): # Remove the added NHGs cbf_nhg_ps._del('testcbfnhg') nhg_ps._del('testnhg') + nhg_ps._del('_testnhg') self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, asic_nhgs_count) return None @@ -111,6 +114,7 @@ def get_nhg_map_id(self, nhg_map_index): # Remove the added NHGs cbf_nhg_ps._del('testcbfnhg') nhg_ps._del('testnhg') + nhg_ps._del('_testnhg') self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, asic_nhgs_count) return nhg_map_id @@ -1546,6 +1550,67 @@ def create_route_inexistent_nhg_test(): self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count) def test_nhgorch_nh_group(self, dvs, testlog): + # Test scenario: + # - create recursive nhg - rec_grp1 with two members - grp1 and grp2 only one of which exists + # - create singleton nhg grp2 and check if the rec_grp1 is updated with both the members + # - create a recursive nhg - rec_grp2 with another recursive nhg - rec_grp1 as member. Assert that the nhg is not created. + def create_recursive_nhg_test(): + # create next hop group in APPL DB + fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), ('ifname', 'Ethernet0')]) + self.nhg_ps.set("grp1", fvs) + + # create a recursive nexthop group with two members + fvs = swsscommon.FieldValuePairs([('nexthop_group', 'grp1,grp2')]) + self.nhg_ps.set("rec_grp1", fvs) + + # check if group was propagated to ASIC DB with the existing member + self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, self.asic_nhgs_count + 1) + assert self.nhg_exists('rec_grp1') + + # check if the existing member was propagated to ASIC DB + self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count + 1) + assert len(self.get_nhgm_ids('rec_grp1')) == 1 + + # add another singleton nexthop group - grp2 + fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.3'), ('ifname', 'Ethernet4')]) + self.nhg_ps.set("grp2", fvs) + + # check if both the members were propagated to ASIC DB + self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count + 2) + assert len(self.get_nhgm_ids('rec_grp1')) == 2 + + # update the recursive nexthop group with another member not yet existing + fvs = swsscommon.FieldValuePairs([('nexthop_group', 'grp1,grp2,grp3')]) + self.nhg_ps.set("rec_grp1", fvs) + + # check if only two members were propagated to ASIC DB + self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count + 2) + assert len(self.get_nhgm_ids('rec_grp1')) == 2 + + # add another singleton nexthop group - grp3 + fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.5'), ('ifname', 'Ethernet8')]) + self.nhg_ps.set("grp3", fvs) + + # check if all members were propagated to ASIC DB + self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count + 3) + assert len(self.get_nhgm_ids('rec_grp1')) == 3 + + # create a recursive nhg with another recursive nhg as member + fvs = swsscommon.FieldValuePairs([('nexthop_group', 'rec_grp1')]) + self.nhg_ps.set("rec_grp2", fvs) + + # check that the group was not propagated to ASIC DB + self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, self.asic_nhgs_count + 1) + assert not self.nhg_exists('rec_grp2') + + self.nhg_ps._del("rec_grp2") + self.nhg_ps._del("rec_grp1") + self.nhg_ps._del("grp1") + self.nhg_ps._del("grp2") + self.nhg_ps._del("grp3") + self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, self.asic_nhgs_count) + self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count) + # Test scenario: # - create NHG 'group1' and assert it is being added to ASIC DB along with its members def create_nhg_test(): @@ -1705,8 +1770,8 @@ def update_nhgm_count_test(): # Update the group to one NH only fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), ("ifname", "Ethernet0")]) self.nhg_ps.set("group1", fvs) - self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count + 1) - assert len(self.get_nhgm_ids('group1')) == 1 + self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count) + assert len(self.get_nhgm_ids('group1')) == 0 # Update the group to 2 NHs fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1,10.0.0.3'), ("ifname", "Ethernet0,Ethernet4")]) @@ -1716,6 +1781,7 @@ def update_nhgm_count_test(): self.init_test(dvs, 4) + create_recursive_nhg_test() create_nhg_test() create_route_nhg_test() link_flap_test() @@ -1838,9 +1904,11 @@ def data_validation_test(): # - update the CBF NHG reordering the members and assert the new details match def update_cbf_nhg_members_test(): # Create a NHG with a single next hop - fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), - ("ifname", "Ethernet0")]) + fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), ('ifname', 'Ethernet0')]) + self.nhg_ps.set("_group3", fvs) + fvs = swsscommon.FieldValuePairs([('nexthop_group','_group3')]) self.nhg_ps.set("group3", fvs) + self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, self.asic_nhgs_count + 3) # Create a CBF NHG @@ -2035,6 +2103,8 @@ def create_cbf_invalid_nhg_map_test(): self.cbf_nhg_ps._del('cbfgroup1') self.nhg_ps._del('group2') self.nhg_ps._del('group3') + self.nhg_ps._del('_group3') + self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, self.asic_nhgs_count) self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count)