Skip to content

Commit

Permalink
Support idle-timeout after prefix-limit is exceeded
Browse files Browse the repository at this point in the history 
If a session is terminated because the configured prefix-limit is
exceeded, do not allow it to get established till the configured
idle-timeout is exceeded.

Any significant change to the bgp peering, which would cause the
session to reset, results in the idle timer being cancelled and
the session being allowed to re-establish. Of course, the session
would still be subject to the configured prefix-limit after it
comes up again.

Change-Id: I007ada064df9b93352a4c50ff0511083331bc55d
Closes-Bug: 1697345
  • Loading branch information
Nischal Sheth committed Jul 20, 2017
1 parent b32b60c commit b1f41b0
Show file tree
Hide file tree
Showing 9 changed files with 196 additions and 14 deletions.
4 changes: 3 additions & 1 deletion src/bgp/bgp_config.h
View file
Expand Up @@ -87,12 +87,13 @@ class AuthenticationData {
//
struct BgpFamilyAttributesConfig {
explicit BgpFamilyAttributesConfig(const std::string &family)
: family(family), loop_count(0), prefix_limit(0) {
: family(family), loop_count(0), prefix_limit(0), idle_timeout(0) {
}

std::string family;
uint8_t loop_count;
uint32_t prefix_limit;
uint32_t idle_timeout;
};

//
Expand All @@ -104,6 +105,7 @@ struct BgpFamilyAttributesConfigCompare {
KEY_COMPARE(lhs.family, rhs.family);
KEY_COMPARE(lhs.loop_count, rhs.loop_count);
KEY_COMPARE(lhs.prefix_limit, rhs.prefix_limit);
KEY_COMPARE(lhs.idle_timeout, rhs.idle_timeout);
return 0;
}
};
Expand Down
2 changes: 2 additions & 0 deletions src/bgp/bgp_config_ifmap.cc
View file
Expand Up @@ -205,6 +205,8 @@ static void BuildFamilyAttributesList(BgpNeighborConfig *neighbor,
family_config.address_family);
family_attributes.loop_count = family_config.loop_count;
family_attributes.prefix_limit = family_config.prefix_limit.maximum;
family_attributes.idle_timeout =
family_config.prefix_limit.idle_timeout;
family_attributes_list.push_back(family_attributes);
family_set.insert(family_config.address_family);
}
Expand Down
41 changes: 39 additions & 2 deletions src/bgp/bgp_peer.cc
View file
Expand Up @@ -209,6 +209,7 @@ BgpPeerFamilyAttributes::BgpPeerFamilyAttributes(
loop_count = config->loop_count();
}
prefix_limit = family_config.prefix_limit;
idle_timeout = family_config.idle_timeout;

if (config->router_type() == "bgpaas-client") {
if (family_config.family == "inet") {
Expand Down Expand Up @@ -455,6 +456,11 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance,
trigger_(boost::bind(&BgpPeer::ResumeClose, this),
TaskScheduler::GetInstance()->GetTaskId("bgp::StateMachine"),
GetTaskInstance()),
prefix_limit_idle_timer_(
TimerManager::CreateTimer(*server->ioservice(),
"BGP prefix limit idle timer",
TaskScheduler::GetInstance()->GetTaskId("bgp::StateMachine"),
GetTaskInstance())),
prefix_limit_trigger_(boost::bind(&BgpPeer::CheckPrefixLimits, this),
TaskScheduler::GetInstance()->GetTaskId("bgp::StateMachine"),
GetTaskInstance()),
Expand Down Expand Up @@ -756,6 +762,7 @@ bool BgpPeer::CheckPrefixLimits() {
if (family_primary_path_count_[idx] <= family_attributes->prefix_limit)
continue;
Clear(BgpProto::Notification::MaxPrefixes);
StartPrefixLimitIdleTimer(family_attributes->idle_timeout * 1000);
break;
}
return true;
Expand Down Expand Up @@ -925,6 +932,7 @@ void BgpPeer::ConfigUpdate(const BgpNeighborConfig *config) {

// Send the UVE as appropriate.
if (admin_down_changed || clear_session) {
StopPrefixLimitIdleTimer();
BGPPeerInfoSend(peer_info);
}
}
Expand Down Expand Up @@ -960,6 +968,7 @@ void BgpPeer::PostCloseRelease() {
index_ = -1;
}
TimerManager::DeleteTimer(keepalive_timer_);
TimerManager::DeleteTimer(prefix_limit_idle_timer_);

for (Address::Family family = Address::UNSPEC;
family < Address::NUM_FAMILIES;
Expand Down Expand Up @@ -1104,10 +1113,12 @@ const string BgpPeer::GetStateName() const {
}

BgpSession *BgpPeer::CreateSession() {
if (PrefixLimitIdleTimerRunning())
return NULL;

TcpSession *session = server_->session_manager()->CreateSession();
if (session == NULL) {
if (session == NULL)
return NULL;
}

// Set valid keys, if any, in the socket.
SetSessionSocketAuthKey(session);
Expand Down Expand Up @@ -1743,6 +1754,30 @@ void BgpPeer::UpdatePrimaryPathCount(int count, Address::Family family) const {
prefix_limit_trigger_.Set();
}

void BgpPeer::StartPrefixLimitIdleTimer(uint32_t plim_idle_time_msecs) {
prefix_limit_idle_timer_->Start(plim_idle_time_msecs,
boost::bind(&BgpPeer::PrefixLimitIdleTimerExpired, this),
boost::bind(&BgpPeer::PrefixLimitIdleTimerErrorHandler, this, _1, _2));
}

void BgpPeer::StopPrefixLimitIdleTimer() {
prefix_limit_idle_timer_->Cancel();
}

bool BgpPeer::PrefixLimitIdleTimerRunning() const {
return prefix_limit_idle_timer_->running();
}

bool BgpPeer::PrefixLimitIdleTimerExpired() {
return false;
}

void BgpPeer::PrefixLimitIdleTimerErrorHandler(string error_name,
string error_message) {
BGP_LOG_PEER_CRITICAL(Timer, this, BGP_LOG_FLAG_ALL, BGP_PEER_DIR_NA,
"Timer error: " << error_name << " " << error_message);
}

void BgpPeer::EndOfRibTimerErrorHandler(string error_name,
string error_message) {
BGP_LOG_PEER_CRITICAL(Timer, this, BGP_LOG_FLAG_ALL, BGP_PEER_DIR_NA,
Expand Down Expand Up @@ -2109,6 +2144,8 @@ void BgpPeer::FillBgpNeighborFamilyAttributes(BgpNeighborResp *nbr) const {
family_attributes_list_[idx]->loop_count);
show_family_attributes.set_prefix_limit(
family_attributes_list_[idx]->prefix_limit);
show_family_attributes.set_idle_timeout(
family_attributes_list_[idx]->idle_timeout);
if (!family_attributes_list_[idx]->gateway_address.is_unspecified()) {
show_family_attributes.set_gateway_address(
family_attributes_list_[idx]->gateway_address.to_string());
Expand Down
10 changes: 10 additions & 0 deletions src/bgp/bgp_peer.h
View file
Expand Up @@ -52,6 +52,7 @@ struct BgpPeerFamilyAttributes {
const BgpFamilyAttributesConfig &family_config);
uint8_t loop_count;
uint32_t prefix_limit;
uint32_t idle_timeout;
IpAddress gateway_address;
};

Expand All @@ -64,6 +65,7 @@ struct BgpPeerFamilyAttributesCompare {
if (lhs && rhs) {
KEY_COMPARE(lhs->loop_count, rhs->loop_count);
KEY_COMPARE(lhs->prefix_limit, rhs->prefix_limit);
KEY_COMPARE(lhs->idle_timeout, rhs->idle_timeout);
KEY_COMPARE(lhs->gateway_address, rhs->gateway_address);
} else {
KEY_COMPARE(lhs, rhs);
Expand Down Expand Up @@ -136,6 +138,7 @@ class BgpPeer : public IPeer {

void StartKeepaliveTimer();
bool KeepaliveTimerRunning();
bool PrefixLimitIdleTimerRunning() const;
void SetSendReady();

// thread: io::ReaderTask
Expand Down Expand Up @@ -364,6 +367,12 @@ class BgpPeer : public IPeer {
void StopKeepaliveTimerUnlocked();
bool KeepaliveTimerExpired();

void StartPrefixLimitIdleTimer(uint32_t plim_idle_time_msecs);
void StopPrefixLimitIdleTimer();
bool PrefixLimitIdleTimerExpired();
void PrefixLimitIdleTimerErrorHandler(std::string error_name,
std::string error_message);

RibExportPolicy BuildRibExportPolicy(Address::Family family) const;
void StartEndOfRibReceiveTimer(Address::Family family);
bool EndOfRibReceiveTimerExpired(Address::Family family);
Expand Down Expand Up @@ -423,6 +432,7 @@ class BgpPeer : public IPeer {
// Global peer index
int index_;
TaskTrigger trigger_;
Timer *prefix_limit_idle_timer_;
mutable TaskTrigger prefix_limit_trigger_;

// The mutex is used to protect the session, keepalive timer and the
Expand Down
2 changes: 2 additions & 0 deletions src/bgp/bgp_peer.sandesh
View file
Expand Up @@ -43,6 +43,7 @@ struct ShowBgpNeighborFamily {
1: string family;
2: u32 loop_count;
3: u32 prefix_limit;
5: i32 idle_timeout;
4: string gateway_address;
}

Expand Down Expand Up @@ -631,6 +632,7 @@ struct ShowBgpNeighborFamilyConfig {
1: string family;
2: i32 loop_count;
3: i32 prefix_limit;
5: i32 idle_timeout;
4: string gateway_address;
}

Expand Down
8 changes: 8 additions & 0 deletions src/bgp/bgp_session_manager.cc
View file
Expand Up @@ -207,6 +207,14 @@ bool BgpSessionManager::ProcessSession(BgpSession *session) {
return true;
}

// Ignore if the peer's prefix limit idle timer is running.
if (peer->PrefixLimitIdleTimerRunning()) {
session->SendNotification(BgpProto::Notification::Cease,
BgpProto::Notification::MaxPrefixes);
DeleteSession(session);
return true;
}

// Ignore if this peer is being closed.
if (peer->IsCloseInProgress()) {
session->SendNotification(BgpProto::Notification::Cease,
Expand Down
1 change: 1 addition & 0 deletions src/bgp/bgp_show_config.cc
View file
Expand Up @@ -357,6 +357,7 @@ static void FillBgpNeighborConfigInfo(ShowBgpNeighborConfig *sbnc,
sbnfc.set_family(family_config.family);
sbnfc.set_loop_count(family_config.loop_count);
sbnfc.set_prefix_limit(family_config.prefix_limit);
sbnfc.set_idle_timeout(family_config.idle_timeout);
if (family_config.family == "inet") {
IpAddress address = neighbor->gateway_address(Address::INET);
if (!address.is_unspecified())
Expand Down
128 changes: 120 additions & 8 deletions src/bgp/test/bgp_ip_test.cc
View file
Expand Up @@ -79,6 +79,7 @@ static const char *cfg_template = "\
<address-family>%s</address-family>\
<prefix-limit>\
<maximum>%d</maximum>\
<idle-timeout>%d</idle-timeout>\
</prefix-limit>\
</family-attributes>\
</session>\
Expand All @@ -93,6 +94,7 @@ static const char *cfg_template = "\
<address-family>%s</address-family>\
<prefix-limit>\
<maximum>%d</maximum>\
<idle-timeout>%d</idle-timeout>\
</prefix-limit>\
</family-attributes>\
</session>\
Expand Down Expand Up @@ -146,13 +148,15 @@ class BgpIpTest : public ::testing::Test {
delete peer2_;
}

void Configure(int prefix_limit = 0) {
void Configure(int prefix_limit = 0, int idle_timeout = 0) {
char config[4096];
snprintf(config, sizeof(config), cfg_template,
bs_x_->session_manager()->GetPort(),
GetFamily() == Address::INET ? "inet" : "inet6", prefix_limit,
GetFamily() == Address::INET ? "inet" : "inet6",
prefix_limit, idle_timeout,
bs_y_->session_manager()->GetPort(),
GetFamily() == Address::INET ? "inet" : "inet6", prefix_limit);
GetFamily() == Address::INET ? "inet" : "inet6",
prefix_limit, idle_timeout);
bs_x_->Configure(config);
bs_y_->Configure(config);
}
Expand Down Expand Up @@ -554,13 +558,15 @@ TYPED_TEST(BgpIpTest, MultiplePrefixMultipath) {
}

//
// Configure a prefix limit and a 0 idle timeout.
// Add multiple prefixes such that the prefix limit configured on Y for
// the peering to X is exceeded. Verify that the peering keeps flapping.
// Then increase the prefix limit and verify that all the expected paths
// exist on Y. Then lower the limit again and verify that the peering
// keeps flapping again.
// the peering to X is exceeded.
// Verify that the peering keeps flapping.
// Then increase the prefix limit and verify the peering gets re-established
// and all the expected paths exist on Y.
// Then lower the limit again and verify that the peering keeps flapping.
//
TYPED_TEST(BgpIpTest, PrefixLimit) {
TYPED_TEST(BgpIpTest, PrefixLimit1) {
BgpServerTestPtr bs_x = this->bs_x_;
BgpServerTestPtr bs_y = this->bs_y_;
PeerMock *peer1 = this->peer1_;
Expand Down Expand Up @@ -621,6 +627,112 @@ TYPED_TEST(BgpIpTest, PrefixLimit) {
}
}

//
// Configure a prefix limit and a very high idle timeout.
// Add multiple prefixes such that the prefix limit configured on Y for
// the peering to X is exceeded
// Verify that the peering flaps once and doesn't get re-established.
// Then increase the prefix limit and verify that that the peering gets
// re-established and all the expected paths exist on Y.
//
TYPED_TEST(BgpIpTest, PrefixLimit2) {
BgpServerTestPtr bs_x = this->bs_x_;
BgpServerTestPtr bs_y = this->bs_y_;
PeerMock *peer1 = this->peer1_;
PeerMock *peer2 = this->peer2_;
BgpPeer *peer_xy = this->peer_xy_;
BgpPeer *peer_yx = this->peer_yx_;
const string &master = this->master_;

this->Configure(DB::PartitionCount() * 2 - 1, 3600);
task_util::WaitForIdle();

for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->AddRoute(bs_x, peer2, master, this->BuildPrefix(idx),
this->BuildNextHopAddress(peer2->ToString()));
this->AddRoute(bs_x, peer1, master, this->BuildPrefix(idx),
this->BuildNextHopAddress(peer1->ToString()));
}
for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
peer2, this->BuildNextHopAddress(peer2->ToString()));
this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
peer1, this->BuildNextHopAddress(peer1->ToString()));
}

usleep(3000000);
TASK_UTIL_EXPECT_EQ(1, peer_yx->flap_count());
TASK_UTIL_EXPECT_EQ(1, peer_xy->flap_count());

this->Configure(DB::PartitionCount() * 2, 3600);
task_util::WaitForIdle();

for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
peer2, this->BuildNextHopAddress(peer2->ToString()));
this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
peer1, this->BuildNextHopAddress(peer1->ToString()));
this->VerifyPathExists(bs_y, master, this->BuildPrefix(idx),
peer_yx, this->BuildNextHopAddress(peer1->ToString()));
this->VerifyPathNoExists(bs_y, master, this->BuildPrefix(idx),
peer_yx, this->BuildNextHopAddress(peer2->ToString()));
}

for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->DeleteRoute(bs_x, peer1, master, this->BuildPrefix(idx));
this->DeleteRoute(bs_x, peer2, master, this->BuildPrefix(idx));
}
for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->VerifyRouteNoExists(bs_x, master, this->BuildPrefix(idx));
this->VerifyRouteNoExists(bs_y, master, this->BuildPrefix(idx));
}
}

//
// Configure a prefix limit and a small non-zero idle timeout.
// Add multiple prefixes such that the prefix limit configured on Y for
// the peering to X is exceeded.
// Verify that the peering keeps flapping.
//
TYPED_TEST(BgpIpTest, PrefixLimit3) {
BgpServerTestPtr bs_x = this->bs_x_;
BgpServerTestPtr bs_y = this->bs_y_;
PeerMock *peer1 = this->peer1_;
PeerMock *peer2 = this->peer2_;
BgpPeer *peer_xy = this->peer_xy_;
BgpPeer *peer_yx = this->peer_yx_;
const string &master = this->master_;

this->Configure(DB::PartitionCount() * 2 - 1, 1);
task_util::WaitForIdle();

for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->AddRoute(bs_x, peer2, master, this->BuildPrefix(idx),
this->BuildNextHopAddress(peer2->ToString()));
this->AddRoute(bs_x, peer1, master, this->BuildPrefix(idx),
this->BuildNextHopAddress(peer1->ToString()));
}
for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
peer2, this->BuildNextHopAddress(peer2->ToString()));
this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
peer1, this->BuildNextHopAddress(peer1->ToString()));
}

usleep(3000000);
TASK_UTIL_EXPECT_TRUE(peer_yx->flap_count() > 3);
TASK_UTIL_EXPECT_TRUE(peer_xy->flap_count() > 3);

for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->DeleteRoute(bs_x, peer1, master, this->BuildPrefix(idx));
this->DeleteRoute(bs_x, peer2, master, this->BuildPrefix(idx));
}
for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
this->VerifyRouteNoExists(bs_x, master, this->BuildPrefix(idx));
this->VerifyRouteNoExists(bs_y, master, this->BuildPrefix(idx));
}
}

class TestEnvironment : public ::testing::Environment {
virtual ~TestEnvironment() { }
};
Expand Down

0 comments on commit b1f41b0

Please sign in to comment.