Skip to content

Commit 5054495

Browse files
venkit-nexthopvenkit-nexthop
authored
With ZMQ enabled between fpmsyncd to orchagent, default routes are se… (#3979)
* With ZMQ enabled between fpmsyncd to orchagent, default routes are set to DROP Issue# #3978 Recently ZMQ was enabled in sonic-mgmt (refer to sonic-net/sonic-mgmt@111e635). fpmsyncd sends a DEL+SET for every route. This was coalesced by the producerStateTable infra. But with ZMQ enabled, this coalescing does not happen. As a result, orchagent gets a DEL+SET for every route. This works well normally. But for default routes, there is a bug in orchagent, where it adds a DROP action when it receives the DEL. But when the subsequent SET is received, it does not reset it to FORWARD action. This is due to a bug in its checking code. * ZMQ configuration does not work when mgmt VRF is configured
1 parent 6b976e9 commit 5054495

File tree

8 files changed

+447
-95
lines changed

8 files changed

+447
-95
lines changed

‎fpmsyncd/routesync.cpp‎

Lines changed: 28 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ void RouteSync::setRouteWithWarmRestart(FieldValueTupleWrapperBase & fvw,
174174
}
175175
else
176176
{
177-
m_warmStartHelper.insertRefreshMap(fvw.KeyOpFieldsValuesTupleVector()[1]);
177+
m_warmStartHelper.insertRefreshMap(fvw.KeyOpFieldsValuesTupleVector()[0]);
178178
}
179179
}
180180

@@ -923,39 +923,17 @@ bool RouteSync::getSrv6SteerRouteNextHop(struct nlmsghdr *h, int received_bytes,
923923
vector<FieldValueTuple>
924924
RouteTableFieldValueTupleWrapper::fieldValueTupleVector() {
925925
vector<FieldValueTuple> fvVector;
926-
if (protocol != string()) {
927-
fvVector.push_back(FieldValueTuple("protocol", protocol.c_str()));
928-
}
929-
if (blackhole != string()) {
930-
fvVector.push_back(FieldValueTuple("blackhole", blackhole.c_str()));
931-
}
932-
if (nexthop != string()) {
933-
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
934-
}
935-
if (ifname != string()) {
936-
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
937-
}
938-
if (nexthop_group != string()) {
939-
fvVector.push_back(FieldValueTuple("nexthop_group", nexthop_group.c_str()));
940-
}
941-
if (mpls_nh != string()) {
942-
fvVector.push_back(FieldValueTuple("mpls_nh", mpls_nh.c_str()));
943-
}
944-
if (weight != string()) {
945-
fvVector.push_back(FieldValueTuple("weight", weight.c_str()));
946-
}
947-
if (vni_label != string()) {
948-
fvVector.push_back(FieldValueTuple("vni_label", vni_label.c_str()));
949-
}
950-
if (router_mac != string()) {
951-
fvVector.push_back(FieldValueTuple("router_mac", router_mac.c_str()));
952-
}
953-
if (segment != string()) {
954-
fvVector.push_back(FieldValueTuple("segment", segment.c_str()));
955-
}
956-
if (seg_src != string()) {
957-
fvVector.push_back(FieldValueTuple("seg_src", seg_src.c_str()));
958-
}
926+
fvVector.push_back(FieldValueTuple("protocol", protocol.c_str()));
927+
fvVector.push_back(FieldValueTuple("blackhole", blackhole.c_str()));
928+
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
929+
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
930+
fvVector.push_back(FieldValueTuple("nexthop_group", nexthop_group.c_str()));
931+
fvVector.push_back(FieldValueTuple("mpls_nh", mpls_nh.c_str()));
932+
fvVector.push_back(FieldValueTuple("weight", weight.c_str()));
933+
fvVector.push_back(FieldValueTuple("vni_label", vni_label.c_str()));
934+
fvVector.push_back(FieldValueTuple("router_mac", router_mac.c_str()));
935+
fvVector.push_back(FieldValueTuple("segment", segment.c_str()));
936+
fvVector.push_back(FieldValueTuple("seg_src", seg_src.c_str()));
959937
// Return value optimization will avoid copy of the following vector
960938
return fvVector;
961939
}
@@ -965,24 +943,12 @@ RouteTableFieldValueTupleWrapper::fieldValueTupleVector() {
965943
vector<FieldValueTuple>
966944
LabelRouteTableFieldValueTupleWrapper::fieldValueTupleVector() {
967945
vector<FieldValueTuple> fvVector;
968-
if (protocol != string()) {
969-
fvVector.push_back(FieldValueTuple("protocol", protocol.c_str()));
970-
}
971-
if (blackhole != string()) {
972-
fvVector.push_back(FieldValueTuple("blackhole", blackhole.c_str()));
973-
}
974-
if (nexthop != string()) {
975-
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
976-
}
977-
if (ifname != string()) {
978-
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
979-
}
980-
if (mpls_nh != string()) {
981-
fvVector.push_back(FieldValueTuple("mpls_nh", mpls_nh.c_str()));
982-
}
983-
if (mpls_pop != string()) {
984-
fvVector.push_back(FieldValueTuple("mpls_pop", mpls_pop.c_str()));
985-
}
946+
fvVector.push_back(FieldValueTuple("protocol", protocol.c_str()));
947+
fvVector.push_back(FieldValueTuple("blackhole", blackhole.c_str()));
948+
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
949+
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
950+
fvVector.push_back(FieldValueTuple("mpls_nh", mpls_nh.c_str()));
951+
fvVector.push_back(FieldValueTuple("mpls_pop", mpls_pop.c_str()));
986952
return fvVector;
987953
}
988954

@@ -991,12 +957,8 @@ LabelRouteTableFieldValueTupleWrapper::fieldValueTupleVector() {
991957
vector<FieldValueTuple>
992958
VnetRouteTableFieldValueTupleWrapper::fieldValueTupleVector() {
993959
vector<FieldValueTuple> fvVector;
994-
if (nexthop != string()) {
995-
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
996-
}
997-
if (ifname != string()) {
998-
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
999-
}
960+
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
961+
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
1000962
return fvVector;
1001963
}
1002964

@@ -1005,9 +967,7 @@ VnetRouteTableFieldValueTupleWrapper::fieldValueTupleVector() {
1005967
vector<FieldValueTuple>
1006968
VnetTunnelTableFieldValueTupleWrapper::fieldValueTupleVector() {
1007969
vector<FieldValueTuple> fvVector;
1008-
if (endpoint != string()) {
1009-
fvVector.push_back(FieldValueTuple("endpoint", endpoint.c_str()));
1010-
}
970+
fvVector.push_back(FieldValueTuple("endpoint", endpoint.c_str()));
1011971
return fvVector;
1012972
}
1013973

@@ -1016,15 +976,9 @@ VnetTunnelTableFieldValueTupleWrapper::fieldValueTupleVector() {
1016976
vector<FieldValueTuple>
1017977
NextHopGroupTableFieldValueTupleWrapper::fieldValueTupleVector() {
1018978
vector<FieldValueTuple> fvVector;
1019-
if (nexthop != string()) {
1020-
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
1021-
}
1022-
if (ifname != string()) {
1023-
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
1024-
}
1025-
if (weight != string()) {
1026-
fvVector.push_back(FieldValueTuple("weight", weight.c_str()));
1027-
}
979+
fvVector.push_back(FieldValueTuple("nexthop", nexthop.c_str()));
980+
fvVector.push_back(FieldValueTuple("ifname", ifname.c_str()));
981+
fvVector.push_back(FieldValueTuple("weight", weight.c_str()));
1028982
return fvVector;
1029983
}
1030984

@@ -1033,15 +987,9 @@ NextHopGroupTableFieldValueTupleWrapper::fieldValueTupleVector() {
1033987
vector<FieldValueTuple>
1034988
Srv6MySidTableFieldValueTupleWrapper::fieldValueTupleVector() {
1035989
vector<FieldValueTuple> fvVector;
1036-
if (action != string()) {
1037-
fvVector.push_back(FieldValueTuple("action", action.c_str()));
1038-
}
1039-
if (vrf != string()) {
1040-
fvVector.push_back(FieldValueTuple("vrf", vrf.c_str()));
1041-
}
1042-
if (adj != string()) {
1043-
fvVector.push_back(FieldValueTuple("adj", adj.c_str()));
1044-
}
990+
fvVector.push_back(FieldValueTuple("action", action.c_str()));
991+
fvVector.push_back(FieldValueTuple("vrf", vrf.c_str()));
992+
fvVector.push_back(FieldValueTuple("adj", adj.c_str()));
1045993
return fvVector;
1046994
}
1047995

@@ -1050,9 +998,7 @@ Srv6MySidTableFieldValueTupleWrapper::fieldValueTupleVector() {
1050998
vector<FieldValueTuple>
1051999
Srv6SidListTableFieldValueTupleWrapper::fieldValueTupleVector() {
10521000
vector<FieldValueTuple> fvVector;
1053-
if (path != string()) {
1054-
fvVector.push_back(FieldValueTuple("path", path.c_str()));
1055-
}
1001+
fvVector.push_back(FieldValueTuple("path", path.c_str()));
10561002
return fvVector;
10571003
}
10581004

‎fpmsyncd/routesync.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ class FieldValueTupleWrapperBase {
5959
// then we would like to atomically cleanup earlier fields and set the new
6060
// fields in the hash-set in redis.
6161
vector<KeyOpFieldsValuesTuple> kfvVector;
62-
kfvVector.push_back(KeyOpFieldsValuesTuple {key.c_str(), "DEL", {}});
6362
auto fvVector = fieldValueTupleVector();
6463
kfvVector.push_back(KeyOpFieldsValuesTuple {key.c_str(), "SET", fvVector});
6564
return kfvVector;
@@ -83,7 +82,7 @@ class RouteTableFieldValueTupleWrapper : public FieldValueTupleWrapperBase {
8382
vector<FieldValueTuple> fieldValueTupleVector() override;
8483

8584
string protocol = string();
86-
string blackhole = string();
85+
string blackhole = string("false");
8786
string nexthop = string();
8887
string ifname = string();
8988
string nexthop_group = string();
@@ -107,7 +106,7 @@ class LabelRouteTableFieldValueTupleWrapper : public FieldValueTupleWrapperBase
107106
vector<FieldValueTuple> fieldValueTupleVector() override;
108107

109108
string protocol = string();
110-
string blackhole = string();
109+
string blackhole = string("false");
111110
string nexthop = string();
112111
string ifname = string();
113112
string mpls_nh = string();

‎orchagent/bulker.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -778,6 +778,12 @@ class EntityBulker
778778
return removing_entries.find(entry) != removing_entries.end();
779779
}
780780

781+
bool bulk_entry_pending_removal_or_set(const Te& entry) const
782+
{
783+
return removing_entries.find(entry) != removing_entries.end() ||
784+
setting_entries.find(entry) != setting_entries.end();
785+
}
786+
781787
private:
782788
std::unordered_map< // A map of
783789
Te, // entry ->

‎orchagent/main.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -546,7 +546,7 @@ int main(int argc, char **argv)
546546
else
547547
{
548548
SWSS_LOG_NOTICE("The ZMQ channel on the northbound side of orchagent has been initialized: %s, %s", zmq_server_address.c_str(), vrf.c_str());
549-
zmq_server = create_zmq_server(zmq_server_address, vrf);
549+
zmq_server = create_zmq_server(zmq_server_address);
550550
}
551551

552552
// Get switch_type

‎orchagent/routeorch.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1060,11 +1060,15 @@ void RouteOrch::doTask(ConsumerBase& consumer)
10601060
* Check if the route does not exist or needs to be updated or
10611061
* if the route is using a temporary next hop group owned by
10621062
* NhgOrch.
1063+
* With default routes, there may be a setting_entries present in the
1064+
* bulker due to a previous DEL event, where we automatically add a
1065+
* DROP action. So one of the check below (bulk_entry_pending_removal_or_set)
1066+
* checks for both removal and set entries.
10631067
*/
10641068
else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() ||
10651069
m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() ||
10661070
m_syncdRoutes.at(vrf_id).at(ip_prefix) != RouteNhg(nhg, ctx.nhg_index, ctx.context_index) ||
1067-
gRouteBulker.bulk_entry_pending_removal(route_entry) ||
1071+
gRouteBulker.bulk_entry_pending_removal_or_set(route_entry) ||
10681072
ctx.using_temp_nhg)
10691073
{
10701074
if (addRoute(ctx, nhg))

0 commit comments

Comments
 (0)