From 4cc1ba5b3e80def08f08b8be27eaea015a105f57 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Wed, 15 Jun 2016 09:17:10 -0700 Subject: [PATCH] Ensure that reach and unreach items go in separate xmpp messages Change-Id: Idb0d59df3bd7258b79bb5525c5e05560de60abd8 Partial-Bug: 1591399 --- src/bgp/bgp_update_queue.cc | 8 +- src/bgp/bgp_update_queue.h | 9 +- src/bgp/test/bgp_xmpp_inetvpn_test.cc | 152 ++++++++++++++++++++++++++ src/bgp/xmpp_message_builder.cc | 1 + 4 files changed, 168 insertions(+), 2 deletions(-) diff --git a/src/bgp/bgp_update_queue.cc b/src/bgp/bgp_update_queue.cc index dd093fa02c2..cbc6472a02f 100644 --- a/src/bgp/bgp_update_queue.cc +++ b/src/bgp/bgp_update_queue.cc @@ -137,7 +137,13 @@ UpdateInfo *UpdateQueue::AttrNext(UpdateInfo *current_uinfo) { return NULL; } if (encoding_is_xmpp_) { - return next_uinfo; + const RibOutAttr &next_roattr = next_uinfo->roattr; + const RibOutAttr ¤t_roattr = current_uinfo->roattr; + if (next_roattr.IsReachable() == current_roattr.IsReachable()) { + return next_uinfo; + } else { + return NULL; + } } if (next_uinfo->roattr.attr() == current_uinfo->roattr.attr()) { return next_uinfo; diff --git a/src/bgp/bgp_update_queue.h b/src/bgp/bgp_update_queue.h index ab5faede5b9..67da82a0681 100644 --- a/src/bgp/bgp_update_queue.h +++ b/src/bgp/bgp_update_queue.h @@ -28,7 +28,14 @@ // struct UpdateByAttrCmp { bool operator()(const UpdateInfo &lhs, const UpdateInfo &rhs) const { - if (!lhs.roattr.is_xmpp()) { + if (lhs.roattr.is_xmpp()) { + if (lhs.roattr.IsReachable() < rhs.roattr.IsReachable()) { + return true; + } + if (lhs.roattr.IsReachable() > rhs.roattr.IsReachable()) { + return false; + } + } else { if (lhs.roattr.attr() < rhs.roattr.attr()) { return true; } diff --git a/src/bgp/test/bgp_xmpp_inetvpn_test.cc b/src/bgp/test/bgp_xmpp_inetvpn_test.cc index 75c8dc01fb7..5c389578531 100644 --- a/src/bgp/test/bgp_xmpp_inetvpn_test.cc +++ b/src/bgp/test/bgp_xmpp_inetvpn_test.cc @@ -1702,6 +1702,158 @@ TEST_F(BgpXmppInetvpn2ControlNodeTest, MultipleRouteAddDelete4) { agent_b_->SessionDown(); } +// +// Multiple routes are exchanged correctly. +// Reach and unreach items go in separate xmpp messages even though add and +// delete of routes is interleaved. +// +TEST_F(BgpXmppInetvpn2ControlNodeTest, MultipleRouteAddDelete5) { + string nh_str("192.168.1.1"); + Configure(); + 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()); + + // Verify table walk count for blue.inet.0. + BgpTable *blue_x = VerifyTableExists(bs_x_, "blue.inet.0"); + BgpTable *blue_y = VerifyTableExists(bs_y_, "blue.inet.0"); + TASK_UTIL_EXPECT_EQ(0, blue_x->walk_complete_count()); + TASK_UTIL_EXPECT_EQ(0, blue_y->walk_complete_count()); + task_util::WaitForIdle(); + + // Register to blue instance + agent_a_->Subscribe("blue", 1); + agent_b_->Subscribe("blue", 1); + task_util::WaitForIdle(); + + // Verify that subscribe completed. + TASK_UTIL_EXPECT_EQ(1, blue_x->walk_complete_count()); + TASK_UTIL_EXPECT_EQ(1, blue_y->walk_complete_count()); + task_util::WaitForIdle(); + + // Pause update generation on Y. + bs_y_->scheduling_group_manager()->DisableGroups(); + + // Add even routes from agent A. + for (int idx = 0; idx < kRouteCount; idx += 2) { + int lpref = idx + 1; + int med = idx + 1; + agent_a_->AddRoute("blue", BuildPrefix(idx), nh_str, lpref, med); + } + task_util::WaitForIdle(); + + // Verify that blue.inet.0 output queues have built up on Y. + VerifyOutputQueueDepth(bs_y_, kRouteCount / 2); + task_util::WaitForIdle(); + + // Resume update generation on Y. + bs_y_->scheduling_group_manager()->EnableGroups(); + task_util::WaitForIdle(); + + // Verify that blue.inet.0 output queues are drained on Y. + VerifyOutputQueueDepth(bs_y_, 0); + + // Verify that even routes showed up on agents A and B. + for (int idx = 0; idx < kRouteCount; idx += 2) { + VerifyRouteExists(agent_a_, "blue", BuildPrefix(idx), nh_str); + VerifyRouteExists(agent_b_, "blue", BuildPrefix(idx), nh_str); + } + + // Pause update generation on Y. + bs_y_->scheduling_group_manager()->DisableGroups(); + + // Add odd route and delete even routes from agent A. + for (int idx = 0; idx < kRouteCount; ++idx) { + if (idx % 2 == 1) { + int lpref = idx + 1; + int med = idx + 1; + agent_a_->AddRoute("blue", BuildPrefix(idx), nh_str, lpref, med); + } else { + agent_a_->DeleteRoute("blue", BuildPrefix(idx)); + } + } + task_util::WaitForIdle(); + + // Verify that blue.inet.0 output queues have built up on Y. + VerifyOutputQueueDepth(bs_y_, kRouteCount); + task_util::WaitForIdle(); + + // Resume update generation on Y. + bs_y_->scheduling_group_manager()->EnableGroups(); + task_util::WaitForIdle(); + + // Verify that blue.inet.0 output queues are drained on Y. + VerifyOutputQueueDepth(bs_y_, 0); + + // Verify odd routes exist and even routes are deleted at agents A and B. + for (int idx = 0; idx < kRouteCount; ++idx) { + if (idx % 2 == 1) { + VerifyRouteExists(agent_a_, "blue", BuildPrefix(idx), nh_str); + VerifyRouteExists(agent_b_, "blue", BuildPrefix(idx), nh_str); + } else { + VerifyRouteNoExists(agent_a_, "blue", BuildPrefix(idx)); + VerifyRouteNoExists(agent_b_, "blue", BuildPrefix(idx)); + } + } + + // Pause update generation on Y. + bs_y_->scheduling_group_manager()->DisableGroups(); + + // Delete odd routes from agent A. + for (int idx = 1; idx < kRouteCount; idx += 2) { + agent_a_->DeleteRoute("blue", BuildPrefix(idx)); + } + task_util::WaitForIdle(); + + // Verify that blue.inet.0 output queues have built up on Y. + VerifyOutputQueueDepth(bs_y_, kRouteCount / 2); + task_util::WaitForIdle(); + + // Resume update generation on Y. + bs_y_->scheduling_group_manager()->EnableGroups(); + task_util::WaitForIdle(); + + // Verify that blue.inet.0 output queues are drained on Y. + VerifyOutputQueueDepth(bs_y_, 0); + + // Verify all routes are deleted at agents A and B. + for (int idx = 0; idx < kRouteCount; ++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"); + + // Verify xmpp update counters. + const BgpXmppChannel *xc_b = + VerifyXmppChannelExists(cm_y_.get(), "agent-b"); + TASK_UTIL_EXPECT_EQ(0, xc_b->get_rx_route_reach()); + TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_b->get_tx_route_reach()); + TASK_UTIL_EXPECT_EQ(0, xc_b->get_rx_route_unreach()); + TASK_UTIL_EXPECT_EQ(0 + kRouteCount, xc_b->get_tx_route_unreach()); + + // Verify xmpp update message counters. + // agent-b: 16 (kRouteCount/BgpXmppMessage::kMaxReachCount) advertise + + // 2 (kRouteCount/BgpXmppMessage::kMaxUnreachCount) withdraw + TASK_UTIL_EXPECT_EQ(18, xc_b->get_tx_update()); + + // Close the sessions. + agent_a_->SessionDown(); + agent_b_->SessionDown(); +} + // // Generate big update message. // Each route has a very large number of attributes such that only a single diff --git a/src/bgp/xmpp_message_builder.cc b/src/bgp/xmpp_message_builder.cc index cdc6afef91b..bde9e6c73aa 100644 --- a/src/bgp/xmpp_message_builder.cc +++ b/src/bgp/xmpp_message_builder.cc @@ -156,6 +156,7 @@ void BgpXmppMessage::Start(const RibOutAttr *roattr, const BgpRoute *route) { } bool BgpXmppMessage::AddRoute(const BgpRoute *route, const RibOutAttr *roattr) { + assert(is_reachable_ == roattr->IsReachable()); if (is_reachable_ && num_reach_route_ >= kMaxReachCount) return false; if (!is_reachable_ && num_unreach_route_ >= kMaxUnreachCount)