Skip to content

Commit

Permalink
Allow BGP hold-time to be configured per BgpPeer
Browse files Browse the repository at this point in the history 
Change-Id: Iadebd68d2e8b68d398ee6634e229df9f194d2099
Closes-Bug: 1514497
  • Loading branch information
Nischal Sheth committed Nov 12, 2015
1 parent 4cf884e commit 24e067f
Show file tree
Hide file tree
Showing 9 changed files with 110 additions and 8 deletions.
3 changes: 3 additions & 0 deletions src/bgp/bgp_config_ifmap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ static void NeighborSetSessionAttributes(
attributes = local;
}
if (attributes != NULL) {
if (attributes->hold_time) {
neighbor->set_hold_time(attributes->hold_time);
}
BuildFamilyAttributesList(neighbor, attributes);
BuildKeyChain(neighbor, attributes->auth_data);
}
Expand Down
1 change: 1 addition & 0 deletions src/bgp/bgp_config_parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ static void RemovePeeringLinks(const string &instance,
static bool ParseSession(const string &identifier, const xml_node &node,
SessionMap *sessions) {
autogen::BgpSessionAttributes attr;
attr.Clear();
xml_attribute to = node.attribute("to");
assert(to);
assert(attr.XmlParse(node));
Expand Down
3 changes: 3 additions & 0 deletions src/bgp/bgp_peer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance,
membership_req_pending_(0),
defer_close_(false),
vpn_tables_registered_(false),
hold_time_(config->hold_time()),
local_as_(config->local_as()),
peer_as_(config->peer_as()),
local_bgp_id_(config->local_identifier()),
Expand Down Expand Up @@ -578,6 +579,8 @@ void BgpPeer::ConfigUpdate(const BgpNeighborConfig *config) {
clear_session = true;
}

hold_time_ = config->hold_time();

if (peer_as_ != config->peer_as()) {
peer_as_ = config->peer_as();
peer_info.set_peer_asn(peer_as_);
Expand Down
2 changes: 2 additions & 0 deletions src/bgp/bgp_peer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ class BgpPeer : public IPeer {
void clear_session();
BgpSession *session();

uint16_t hold_time() const { return hold_time_; }
as_t local_as() const { return local_as_; }
as_t peer_as() const { return peer_as_; }

Expand Down Expand Up @@ -365,6 +366,7 @@ class BgpPeer : public IPeer {
bool defer_close_;
bool vpn_tables_registered_;
std::vector<BgpProto::OpenMessage::Capability *> capabilities_;
uint16_t hold_time_;
as_t local_as_;
as_t peer_as_;
uint32_t local_bgp_id_; // network order
Expand Down
1 change: 1 addition & 0 deletions src/bgp/bgp_peer.sandesh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ struct ShowBgpNeighborConfig {
4: i32 autonomous_system;
5: string identifier;
6: string address;
14: i32 hold_time;
10: string last_change_at;
11: optional string auth_type;
12: optional list<string> auth_keys;
Expand Down
1 change: 1 addition & 0 deletions src/bgp/bgp_sandesh.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,7 @@ class ShowBgpNeighborConfigHandler {
nbr.set_identifier(peerid.to_string());
nbr.set_address(neighbor->peer_address().to_string());
nbr.set_address_families(neighbor->GetAddressFamilies());
nbr.set_hold_time(neighbor->hold_time());
nbr.set_last_change_at(
UTCUsecToString(neighbor->last_change_at()));
nbr.set_auth_type(neighbor->auth_data().KeyTypeToString());
Expand Down
6 changes: 5 additions & 1 deletion src/bgp/state_machine.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1651,7 +1651,11 @@ int StateMachine::GetConfiguredHoldTime() const {
return env_hold_time;
}

// Use the configured hold-time if available.
// Use the configured hold-time from peer if available.
if (peer_ && peer_->hold_time())
return peer_->hold_time();

// Use the configured hold-time from server if available.
if (peer_ && peer_->server()->hold_time())
return peer_->server()->hold_time();

Expand Down
98 changes: 91 additions & 7 deletions src/bgp/test/bgp_server_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ class BgpServerUnitTest : public ::testing::Test {
vector<string> families1 = vector<string>(),
vector<string> families2 = vector<string>(),
uint16_t hold_time1 = StateMachine::kHoldTime,
uint16_t hold_time2 = StateMachine::kHoldTime);
uint16_t hold_time2 = StateMachine::kHoldTime,
uint16_t nbr_hold_time1 = 0,
uint16_t nbr_hold_time2 = 0);
void SetupPeers(int peer_count, unsigned short port_a,
unsigned short port_b, bool verify_keepalives,
uint16_t as_num1, uint16_t as_num2,
Expand Down Expand Up @@ -216,6 +218,7 @@ class BgpServerUnitTest : public ::testing::Test {
string bgp_identifier1, string bgp_identifier2,
vector<string> families1, vector<string> families2,
uint16_t hold_time1, uint16_t hold_time2,
uint16_t nbr_hold_time1, uint16_t nbr_hold_time2,
bool delete_config,
vector<ConfigUTAuthKeyItem> auth_keys =
vector<ConfigUTAuthKeyItem>());
Expand Down Expand Up @@ -266,6 +269,7 @@ string BgpServerUnitTest::GetConfigStr(int peer_count,
string bgp_identifier1, string bgp_identifier2,
vector<string> families1, vector<string> families2,
uint16_t hold_time1, uint16_t hold_time2,
uint16_t nbr_hold_time1, uint16_t nbr_hold_time2,
bool delete_config, vector<ConfigUTAuthKeyItem> auth_keys) {
ostringstream config;

Expand Down Expand Up @@ -307,6 +311,8 @@ string BgpServerUnitTest::GetConfigStr(int peer_count,

for (int i = 0; i < peer_count; i++) {
config << "<session to='B'>";
if (nbr_hold_time1)
config << "<hold-time>" << nbr_hold_time1 << "</hold-time>";
config << "<address-families>";
for (vector<string>::const_iterator it = families2.begin();
it != families2.end(); ++it) {
Expand Down Expand Up @@ -351,6 +357,8 @@ string BgpServerUnitTest::GetConfigStr(int peer_count,

for (int i = 0; i < peer_count; i++) {
config << "<session to='A'>";
if (nbr_hold_time2)
config << "<hold-time>" << nbr_hold_time2 << "</hold-time>";
config << "<address-families>";
for (vector<string>::const_iterator it = families1.begin();
it != families1.end(); ++it) {
Expand Down Expand Up @@ -380,7 +388,7 @@ void BgpServerUnitTest::SetupPeers(BgpServerTest *server, int peer_count,
families1, families2,
StateMachine::kHoldTime,
StateMachine::kHoldTime,
delete_config);
0, 0, delete_config);
server->Configure(config);
task_util::WaitForIdle();
}
Expand All @@ -398,7 +406,7 @@ void BgpServerUnitTest::SetupPeers(BgpServerTest *server, int peer_count,
families1, families2,
StateMachine::kHoldTime,
StateMachine::kHoldTime,
delete_config, auth_keys);
0, 0, delete_config, auth_keys);
server->Configure(config);
task_util::WaitForIdle();
}
Expand All @@ -417,7 +425,7 @@ void BgpServerUnitTest::SetupPeers(int peer_count,
families1, families2,
StateMachine::kHoldTime,
StateMachine::kHoldTime,
false, auth_keys);
0, 0, false, auth_keys);
a_->Configure(config);
task_util::WaitForIdle();
b_->Configure(config);
Expand All @@ -430,14 +438,16 @@ void BgpServerUnitTest::SetupPeers(int peer_count,
string peer_address1, string peer_address2,
string bgp_identifier1, string bgp_identifier2,
vector<string> families1, vector<string> families2,
uint16_t hold_time1, uint16_t hold_time2) {
uint16_t hold_time1, uint16_t hold_time2,
uint16_t nbr_hold_time1, uint16_t nbr_hold_time2) {
string config = GetConfigStr(peer_count, port_a, port_b,
as_num1, as_num2,
as_num1, as_num2,
peer_address1, peer_address2,
bgp_identifier1, bgp_identifier2,
families1, families2,
hold_time1, hold_time2,
nbr_hold_time1, nbr_hold_time2,
false);
a_->Configure(config);
task_util::WaitForIdle();
Expand All @@ -456,7 +466,7 @@ void BgpServerUnitTest::SetupPeers(int peer_count,
vector<string>(), vector<string>(),
StateMachine::kHoldTime,
StateMachine::kHoldTime,
false);
0, 0, false);
a_->Configure(config);
task_util::WaitForIdle();
b_->Configure(config);
Expand Down Expand Up @@ -1546,7 +1556,7 @@ TEST_F(BgpServerUnitTest, DISABLED_ChangeBgpPort) {
"192.168.0.10", "192.168.0.11",
families_a, families_b,
StateMachine::kHoldTime, StateMachine::kHoldTime,
true);
0, 0, true);
b_->Configure(config);

for (int j = 0; j < peer_count; j++) {
Expand Down Expand Up @@ -1949,6 +1959,80 @@ TEST_F(BgpServerUnitTest, HoldTimeChange) {
}
}

TEST_F(BgpServerUnitTest, NeighborHoldTimeChange) {
int peer_count = 3;

vector<string> families_a;
vector<string> families_b;

BgpPeerTest::verbose_name(true);
SetupPeers(peer_count, a_->session_manager()->GetPort(),
b_->session_manager()->GetPort(), false,
BgpConfigManager::kDefaultAutonomousSystem,
BgpConfigManager::kDefaultAutonomousSystem,
"127.0.0.1", "127.0.0.1",
"192.168.0.10", "192.168.0.11",
families_a, families_b,
StateMachine::kHoldTime, StateMachine::kHoldTime,
10, 10);
VerifyPeers(peer_count);

for (int idx = 2; idx <= 9; ++idx) {

// Change the hold time and apply the new configuration.
SetupPeers(peer_count, a_->session_manager()->GetPort(),
b_->session_manager()->GetPort(), false,
BgpConfigManager::kDefaultAutonomousSystem,
BgpConfigManager::kDefaultAutonomousSystem,
"127.0.0.1", "127.0.0.1",
"192.168.0.10", "192.168.0.11",
families_a, families_b,
StateMachine::kHoldTime, StateMachine::kHoldTime,
10 * idx, 90);
VerifyPeers(peer_count);

// Established sessions should keep using the old hold time value.
for (int j = 0; j < peer_count; j++) {
string uuid = BgpConfigParser::session_uuid("A", "B", j + 1);
BgpPeer *peer_a =
a_->FindPeerByUuid(BgpConfigManager::kMasterInstance, uuid);
const StateMachine *sm_a = GetPeerStateMachine(peer_a);
TASK_UTIL_EXPECT_EQ(10 * (idx - 1), sm_a->hold_time());

BgpPeer *peer_b =
b_->FindPeerByUuid(BgpConfigManager::kMasterInstance, uuid);
const StateMachine *sm_b = GetPeerStateMachine(peer_b);
TASK_UTIL_EXPECT_EQ(10 * (idx - 1), sm_b->hold_time());
}

// Clear all the sessions by setting peers to admin down on A.
for (int j = 0; j < peer_count; j++) {
string uuid = BgpConfigParser::session_uuid("A", "B", j + 1);
BgpPeer *peer_a =
a_->FindPeerByUuid(BgpConfigManager::kMasterInstance, uuid);
peer_a->SetAdminState(true);
task_util::WaitForIdle();
peer_a->SetAdminState(false);
}

VerifyPeers(peer_count);

// Re-established sessions should use the updated hold time value.
for (int j = 0; j < peer_count; j++) {
string uuid = BgpConfigParser::session_uuid("A", "B", j + 1);
BgpPeer *peer_a =
a_->FindPeerByUuid(BgpConfigManager::kMasterInstance, uuid);
const StateMachine *sm_a = GetPeerStateMachine(peer_a);
TASK_UTIL_EXPECT_EQ(10 * idx, sm_a->hold_time());

BgpPeer *peer_b =
b_->FindPeerByUuid(BgpConfigManager::kMasterInstance, uuid);
const StateMachine *sm_b = GetPeerStateMachine(peer_b);
TASK_UTIL_EXPECT_EQ(10 * idx, sm_b->hold_time());
}
}
}

// Apply bgp neighbor configuration on only one side of the session and
// verify that the session does not come up.
TEST_F(BgpServerUnitTest, MissingPeerConfig) {
Expand Down
3 changes: 3 additions & 0 deletions src/schema/bgp_schema.xsd
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,12 @@
When the parameters are uni-directional the bgp-router element
specifies to which node the configuration applies. If missing
the attributes apply to both ends of the session.
A non-zero hold-time overrides the hold-time inherited from the
bgp-router configuration.
</xsd:documentation>
</xsd:annotation>
<xsd:element name='bgp-router' type='xsd:string'/>
<xsd:element name='hold-time' type='BgpHoldTime' default='0'/>
<xsd:element name='address-families' type='AddressFamilies'/>
<xsd:element name='auth-data' type='AuthenticationData'/>
<xsd:element name='family-attributes' type='BgpFamilyAttributes' maxOccurs='unbounded'/>
Expand Down

0 comments on commit 24e067f

Please sign in to comment.