diff --git a/src/bgp/bgp_message_builder.cc b/src/bgp/bgp_message_builder.cc index 5cef86f52f0..221b247813a 100644 --- a/src/bgp/bgp_message_builder.cc +++ b/src/bgp/bgp_message_builder.cc @@ -5,16 +5,19 @@ #include "bgp/bgp_message_builder.h" #include "base/parse_object.h" +#include "bgp/bgp_log.h" #include "bgp/bgp_route.h" #include "net/bgp_af.h" -BgpMessage::BgpMessage() { +using std::auto_ptr; + +BgpMessage::BgpMessage(const BgpTable *table) : table_(table), datalen_(0) { } BgpMessage::~BgpMessage() { } -void BgpMessage::StartReach(const RibOutAttr *roattr, const BgpRoute *route) { +bool BgpMessage::StartReach(const RibOutAttr *roattr, const BgpRoute *route) { BgpProto::Update update; const BgpAttr *attr = roattr->attr(); @@ -103,42 +106,56 @@ void BgpMessage::StartReach(const RibOutAttr *roattr, const BgpRoute *route) { new BgpMpNlri(BgpAttribute::MPReachNlri, route->Afi(), route->Safi(), nh); update.path_attributes.push_back(nlri); - if (route) { - BgpProtoPrefix *prefix = new BgpProtoPrefix; - route->BuildProtoPrefix(prefix, attr, roattr->label()); - nlri->nlri.push_back(prefix); - num_reach_route_++; + BgpProtoPrefix *prefix = new BgpProtoPrefix; + route->BuildProtoPrefix(prefix, attr, roattr->label()); + nlri->nlri.push_back(prefix); + + int result = + BgpProto::Encode(&update, data_, sizeof(data_), &encode_offsets_); + if (result <= 0) { + BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL, + "Error encoding reach message for route " << route->ToString() << + " in table " << (table_ ? table_->name() : "unknown")); + table_->server()->increment_message_build_error(); + return false; } - datalen_ = BgpProto::Encode(&update, data_, sizeof(data_), - &encode_offsets_); - assert(datalen_ > 0); + num_reach_route_++; + datalen_ = result; + return true; } -void BgpMessage::StartUnreach(const BgpRoute *route) { +bool BgpMessage::StartUnreach(const BgpRoute *route) { BgpProto::Update update; BgpMpNlri *nlri = new BgpMpNlri(BgpAttribute::MPUnreachNlri, route->Afi(), route->Safi()); update.path_attributes.push_back(nlri); - if (route) { - BgpProtoPrefix *prefix = new BgpProtoPrefix; - route->BuildProtoPrefix(prefix); - nlri->nlri.push_back(prefix); - num_unreach_route_++; + BgpProtoPrefix *prefix = new BgpProtoPrefix; + route->BuildProtoPrefix(prefix); + nlri->nlri.push_back(prefix); + + int result = + BgpProto::Encode(&update, data_, sizeof(data_), &encode_offsets_); + if (result <= 0) { + BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_WARN, BGP_LOG_FLAG_ALL, + "Error encoding unreach message for route " << route->ToString() << + " in table " << (table_ ? table_->name() : "unknown")); + table_->server()->increment_message_build_error(); + return false; } - datalen_ = BgpProto::Encode(&update, data_, sizeof(data_), - &encode_offsets_); - assert(datalen_ > 0); + num_unreach_route_++; + datalen_ = result; + return true; } -void BgpMessage::Start(const RibOutAttr *roattr, const BgpRoute *route) { +bool BgpMessage::Start(const RibOutAttr *roattr, const BgpRoute *route) { if (roattr->IsReachable()) { - StartReach(roattr, route); + return StartReach(roattr, route); } else { - StartUnreach(route); + return StartUnreach(route); } } @@ -205,9 +222,12 @@ const uint8_t *BgpMessage::GetData(IPeerUpdate *ipeer_update, size_t *lenp) { Message *BgpMessageBuilder::Create(const BgpTable *table, const RibOutAttr *roattr, const BgpRoute *route) const { - BgpMessage *msg = new BgpMessage(); - msg->Start(roattr, route); - return msg; + auto_ptr msg(new BgpMessage(table)); + if (msg->Start(roattr, route)) { + return msg.release(); + } else { + return NULL; + } } BgpMessageBuilder BgpMessageBuilder::instance_; diff --git a/src/bgp/bgp_message_builder.h b/src/bgp/bgp_message_builder.h index 34f564e0720..977170fc981 100644 --- a/src/bgp/bgp_message_builder.h +++ b/src/bgp/bgp_message_builder.h @@ -10,21 +10,23 @@ class BgpMessage : public Message { public: - BgpMessage(); + BgpMessage(const BgpTable *table = NULL); virtual ~BgpMessage(); - void Start(const RibOutAttr *roattr, const BgpRoute *route); + bool Start(const RibOutAttr *roattr, const BgpRoute *route); virtual bool AddRoute(const BgpRoute *route, const RibOutAttr *roattr); virtual void Finish(); virtual const uint8_t *GetData(IPeerUpdate *ipeer_update, size_t *lenp); private: - void StartReach(const RibOutAttr *roattr, const BgpRoute *route); - void StartUnreach(const BgpRoute *route); + bool StartReach(const RibOutAttr *roattr, const BgpRoute *route); + bool StartUnreach(const BgpRoute *route); bool UpdateLength(const char *tag, int size, int delta); + const BgpTable *table_; EncodeOffsets encode_offsets_; uint8_t data_[BgpProto::kMaxMessageSize]; size_t datalen_; + DISALLOW_COPY_AND_ASSIGN(BgpMessage); }; diff --git a/src/bgp/bgp_ribout_updates.cc b/src/bgp/bgp_ribout_updates.cc index 97ec35aa45e..a37d0a256b8 100644 --- a/src/bgp/bgp_ribout_updates.cc +++ b/src/bgp/bgp_ribout_updates.cc @@ -105,20 +105,31 @@ bool RibOutUpdates::DequeueCommon(UpdateMarker *marker, RouteUpdate *rt_update, continue; } - // Generate the update and merge additional updates into that message. + // Generate the update, merge additional updates into that message and + // send it message to the target RibPeerSet. + // + // In the rare case that the first route and it's attributes don't fit + // into the message, clear the target bits in the UpdateInfo to ensure + // that the UpdateQueue doesn't get wedged. However, don't update the + // history bits in the RouteUpdate since the message did not get sent. + // + // The Create routine has the responsibility of logging an error and + // incrementing any counters. + RibPeerSet msg_blocked; + bool msg_sent = false; auto_ptr message( builder_->Create(table, &uinfo->roattr, rt_update->route())); - UpdatePack(rt_update->queue_id(), message.get(), uinfo, msgset); - message->Finish(); - - // Send the message to the target RibPeerSet. - RibPeerSet msg_blocked; - UpdateSend(message.get(), msgset, &msg_blocked); + if (message.get() != NULL) { + UpdatePack(rt_update->queue_id(), message.get(), uinfo, msgset); + message->Finish(); + UpdateSend(message.get(), msgset, &msg_blocked); + msg_sent = true; + } // Reset bits in the UpdateInfo. Note that this has already been done // via UpdatePack for all the other UpdateInfo elements that we packed // into this message. - bool empty = ClearAdvertisedBits(rt_update, uinfo, msgset); + bool empty = ClearAdvertisedBits(rt_update, uinfo, msgset, msg_sent); if (empty) { rt_update->RemoveUpdateInfo(uinfo); } @@ -371,7 +382,7 @@ void RibOutUpdates::UpdatePack(int queue_id, Message *message, // // If the RouteUpdate itself is now empty i.e. there are no more // UpdateInfo elements associated with it, we can get rid of it. - bool empty = ClearAdvertisedBits(update.get(), uinfo, msgset); + bool empty = ClearAdvertisedBits(update.get(), uinfo, msgset, true); if (empty && update->RemoveUpdateInfo(uinfo)) { ClearUpdate(&update); } @@ -499,11 +510,13 @@ void RibOutUpdates::ClearUpdate(RouteUpdatePtr *update) { // Return true if the UpdateInfo was removed from the set container. // bool RibOutUpdates::ClearAdvertisedBits(RouteUpdate *rt_update, - UpdateInfo *uinfo, const RibPeerSet &isect) { + UpdateInfo *uinfo, const RibPeerSet &isect, bool update_history) { CHECK_CONCURRENCY("bgp::SendTask"); + if (update_history) { + rt_update->UpdateHistory(ribout_, &uinfo->roattr, isect); + } uinfo->target.Reset(isect); - rt_update->UpdateHistory(ribout_, &uinfo->roattr, isect); bool empty = uinfo->target.empty(); if (empty) { UpdateQueue *queue = queue_vec_[rt_update->queue_id()]; diff --git a/src/bgp/bgp_ribout_updates.h b/src/bgp/bgp_ribout_updates.h index 958cb1d322c..6f68e3006e6 100644 --- a/src/bgp/bgp_ribout_updates.h +++ b/src/bgp/bgp_ribout_updates.h @@ -84,8 +84,8 @@ class RibOutUpdates { // Remove the advertised bits on an update. This updates the history // information. Returns true if the UpdateInfo should be deleted. bool ClearAdvertisedBits(RouteUpdate *rt_update, UpdateInfo *uinfo, - const RibPeerSet &bits); - + const RibPeerSet &bits, bool update_history); + void StoreHistory(RouteUpdate *rt_update); void ClearState(RouteUpdate *rt_update); void ClearUpdate(RouteUpdatePtr *update); diff --git a/src/bgp/bgp_server.h b/src/bgp/bgp_server.h index 9578efe5779..0f7b06e50f8 100644 --- a/src/bgp/bgp_server.h +++ b/src/bgp/bgp_server.h @@ -123,6 +123,9 @@ class BgpServer { LifetimeActor *deleter(); boost::asio::io_service *ioservice(); + void increment_message_build_error() const { ++message_build_error_; } + uint64_t message_build_error() const { return message_build_error_; } + int RegisterASNUpdateCallback(ASNUpdateCb callback); void UnregisterASNUpdateCallback(int listener); void NotifyASNUpdate(as_t old_asn); @@ -182,6 +185,8 @@ class BgpServer { boost::scoped_ptr config_mgr_; boost::scoped_ptr updater_; + mutable uint64_t message_build_error_; + DISALLOW_COPY_AND_ASSIGN(BgpServer); }; diff --git a/src/bgp/test/bgp_xmpp_inetvpn_test.cc b/src/bgp/test/bgp_xmpp_inetvpn_test.cc index a886c218fd2..6f25988f149 100644 --- a/src/bgp/test/bgp_xmpp_inetvpn_test.cc +++ b/src/bgp/test/bgp_xmpp_inetvpn_test.cc @@ -14,6 +14,7 @@ #include "control-node/control_node.h" #include "control-node/test/network_agent_mock.h" #include "io/test/event_manager_test.h" +#include "ifmap/test/ifmap_test_util.h" #include "schema/xmpp_unicast_types.h" #include "testing/gunit.h" #include "xmpp/xmpp_factory.h" @@ -198,6 +199,48 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test { bs_y_->Configure(config); } + void AddRouteTarget(BgpServerTestPtr server, const string name, + const string target) { + TASK_UTIL_EXPECT_NE(static_cast(NULL), + server->routing_instance_mgr()->GetRoutingInstance(name)); + + ifmap_test_util::IFMapMsgLink(server->config_db(), + "routing-instance", name, + "route-target", target, + "instance-target"); + } + + void RemoveRouteTarget(BgpServerTestPtr server, const string name, + const string target) { + TASK_UTIL_EXPECT_NE(static_cast(NULL), + server->routing_instance_mgr()->GetRoutingInstance(name)); + + ifmap_test_util::IFMapMsgUnlink(server->config_db(), + "routing-instance", name, + "route-target", target, + "instance-target"); + } + + size_t GetExportRouteTargetListSize(BgpServerTestPtr server, + const string &instance) { + TASK_UTIL_EXPECT_NE(static_cast(NULL), + server->routing_instance_mgr()->GetRoutingInstance(instance)); + RoutingInstance *rti = + server->routing_instance_mgr()->GetRoutingInstance(instance); + task_util::WaitForIdle(); + return rti->GetExportList().size(); + } + + size_t GetImportRouteTargetListSize(BgpServerTestPtr server, + const string &instance) { + TASK_UTIL_EXPECT_NE(static_cast(NULL), + server->routing_instance_mgr()->GetRoutingInstance(instance)); + RoutingInstance *rti = + server->routing_instance_mgr()->GetRoutingInstance(instance); + task_util::WaitForIdle(); + return rti->GetImportList().size(); + } + bool CheckEncap(autogen::TunnelEncapsulationListType &rt_encap, const string &encap) { if (rt_encap.tunnel_encapsulation.size() != 1) @@ -229,7 +272,7 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test { } void VerifyRouteExists(test::NetworkAgentMockPtr agent, string net, - string prefix, string nexthop, int local_pref, + string prefix, string nexthop, int local_pref = 100, const string &encap = string(), const string &origin_vn = string(), const vector sgids = vector()) { TASK_UTIL_EXPECT_TRUE(CheckRoute( @@ -259,6 +302,13 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test { boost::scoped_ptr cm_y_; }; +static string BuildPrefix(uint32_t idx) { + assert(idx <= 65535); + string prefix = string("10.1.") + + integerToString(idx / 255) + "." + integerToString(idx % 255) + "/32"; + return prefix; +} + // // Added route has expected local preference. // @@ -411,6 +461,166 @@ TEST_F(BgpXmppInetvpn2ControlNodeTest, RouteFlap) { agent_b_->SessionDown(); } +// +// Generate big update message. +// Each route has a very large number of attributes such that only a single +// route fits into each update. +// +TEST_F(BgpXmppInetvpn2ControlNodeTest, BigUpdateMessage1) { + Configure(); + task_util::WaitForIdle(); + + // Add a bunch of route targets to blue instance. + static const int kRouteTargetCount = 496; + for (int idx = 0; idx < kRouteTargetCount; ++idx) { + string target = string("target:1:") + integerToString(10000 + idx); + AddRouteTarget(bs_x_, "blue", target); + task_util::WaitForIdle(); + } + TASK_UTIL_EXPECT_EQ(1 + kRouteTargetCount, + GetExportRouteTargetListSize(bs_x_, "blue")); + TASK_UTIL_EXPECT_EQ(1 + kRouteTargetCount, + GetImportRouteTargetListSize(bs_x_, "blue")); + task_util::WaitForIdle(); + + // Create XMPP Agent A connected to XMPP server X. + agent_a_.reset( + new test::NetworkAgentMock(&evm_, "agent-a", xs_x_->GetPort(), + "127.0.0.1", "127.0.0.1")); + TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished()); + + // Create XMPP Agent B connected to XMPP server Y. + agent_b_.reset( + new test::NetworkAgentMock(&evm_, "agent-b", xs_y_->GetPort(), + "127.0.0.2", "127.0.0.2")); + TASK_UTIL_EXPECT_TRUE(agent_b_->IsEstablished()); + + // Register to blue instance + agent_a_->Subscribe("blue", 1); + agent_b_->Subscribe("blue", 1); + + // Add routes from agent A. + for (int idx = 0; idx < 4; ++idx) { + agent_a_->AddRoute("blue", BuildPrefix(idx), "192.168.1.1"); + } + + // Verify that routes showed up on agents A and B. + for (int idx = 0; idx < 4; ++idx) { + VerifyRouteExists(agent_a_, "blue", BuildPrefix(idx), "192.168.1.1"); + VerifyRouteExists(agent_b_, "blue", BuildPrefix(idx), "192.168.1.1"); + } + + // Delete routes from agent A. + for (int idx = 0; idx < 4; ++idx) { + agent_a_->DeleteRoute("blue", BuildPrefix(idx)); + } + + // Verify that routes are deleted at agents A and B. + for (int idx = 0; idx < 4; ++idx) { + VerifyRouteNoExists(agent_a_, "blue", BuildPrefix(idx)); + VerifyRouteNoExists(agent_b_, "blue", BuildPrefix(idx)); + } + + // Unregister to blue instance + agent_a_->Unsubscribe("blue"); + agent_b_->Unsubscribe("blue"); + + // Close the sessions. + agent_a_->SessionDown(); + agent_b_->SessionDown(); + + TASK_UTIL_EXPECT_EQ(0, bs_x_->message_build_error()); +} + +// +// Generate big update message. +// Each route has a very large number of attributes such that even a single +// route doesn't fit into an update. +// +TEST_F(BgpXmppInetvpn2ControlNodeTest, BigUpdateMessage2) { + Configure(); + task_util::WaitForIdle(); + + // Add extra route targets to blue instance. + static const int kRouteTargetCount = 512; + for (int idx = 0; idx < kRouteTargetCount; ++idx) { + string target = string("target:1:") + integerToString(10000 + idx); + AddRouteTarget(bs_x_, "blue", target); + task_util::WaitForIdle(); + } + TASK_UTIL_EXPECT_EQ(1 + kRouteTargetCount, + GetExportRouteTargetListSize(bs_x_, "blue")); + TASK_UTIL_EXPECT_EQ(1 + kRouteTargetCount, + GetImportRouteTargetListSize(bs_x_, "blue")); + task_util::WaitForIdle(); + + // Create XMPP Agent A connected to XMPP server X. + agent_a_.reset( + new test::NetworkAgentMock(&evm_, "agent-a", xs_x_->GetPort(), + "127.0.0.1", "127.0.0.1")); + TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished()); + + // Create XMPP Agent B connected to XMPP server Y. + agent_b_.reset( + new test::NetworkAgentMock(&evm_, "agent-b", xs_y_->GetPort(), + "127.0.0.2", "127.0.0.2")); + TASK_UTIL_EXPECT_TRUE(agent_b_->IsEstablished()); + + // Register to blue instance + agent_a_->Subscribe("blue", 1); + agent_b_->Subscribe("blue", 1); + task_util::WaitForIdle(); + + // Add routes from agent A. + for (int idx = 0; idx < 4; ++idx) { + agent_a_->AddRoute("blue", BuildPrefix(idx), "192.168.1.1"); + } + + // Verify that we're unable to build the messages. + TASK_UTIL_EXPECT_EQ(4, bs_x_->message_build_error()); + + // Verify that routes showed up on agent A, but not on B. + for (int idx = 0; idx < 4; ++idx) { + VerifyRouteExists(agent_a_, "blue", BuildPrefix(idx), "192.168.1.1"); + VerifyRouteNoExists(agent_b_, "blue", BuildPrefix(idx)); + } + + // Remove extra route targets from blue instance. + for (int idx = 0; idx < kRouteTargetCount; ++idx) { + string target = string("target:1:") + integerToString(10000 + idx); + RemoveRouteTarget(bs_x_, "blue", target); + task_util::WaitForIdle(); + } + TASK_UTIL_EXPECT_EQ(1, GetExportRouteTargetListSize(bs_x_, "blue")); + TASK_UTIL_EXPECT_EQ(1, GetImportRouteTargetListSize(bs_x_, "blue")); + task_util::WaitForIdle(); + + // Verify that routes showed up on agents A and B. + for (int idx = 0; idx < 4; ++idx) { + VerifyRouteExists(agent_a_, "blue", BuildPrefix(idx), "192.168.1.1"); + VerifyRouteExists(agent_b_, "blue", BuildPrefix(idx), "192.168.1.1"); + } + + // Delete routes from agent A. + for (int idx = 0; idx < 4; ++idx) { + agent_a_->DeleteRoute("blue", BuildPrefix(idx)); + } + + // Verify that routes are deleted at agents A and B. + for (int idx = 0; idx < 4; ++idx) { + VerifyRouteNoExists(agent_a_, "blue", BuildPrefix(idx)); + VerifyRouteNoExists(agent_b_, "blue", BuildPrefix(idx)); + } + + // Unregister to blue instance + agent_a_->Unsubscribe("blue"); + agent_b_->Unsubscribe("blue"); + + // Close the sessions. + agent_a_->SessionDown(); + agent_b_->SessionDown(); +} + TEST_F(BgpXmppInetvpn2ControlNodeTest, TunnelEncap) { Configure(); task_util::WaitForIdle();