Skip to content

Commit

Permalink
FMG label missed.
Browse files Browse the repository at this point in the history 
Transiently FMG label can be assigned to multiple tree, because of control node
restarts or flaps. In this case say tree-1 is using label A and it was supposed
to be withdrawn or staled. Re-use of label happens at CN and is assigned to
tree-2. XMPP messages for withdrawal of label A from tree-1 and adding to tree-2
can go in any order. So add for tree-2 is seen before withdrawal in problematic
case. This results in label A removed even though tree-2 is active user.

Solution:
Maintain a list of users for label. Label remains intact till list is not empty.
In above case tree-2 will be present in list even after tree-1 is withdrawn and
label remains intact.

Change-Id: Ia2bb22d0c958355a5c2295709fd5eb90c1a79a65
CLoses-bug: #1724114
  • Loading branch information
@manishsing
manishsing committed Feb 20, 2018
1 parent f85f076 commit 6f8d7b4
Show file tree
Hide file tree
Showing 5 changed files with 207 additions and 9 deletions.
11 changes: 8 additions & 3 deletions src/vnsw/agent/oper/bridge_route.cc
View file
Expand Up @@ -497,6 +497,7 @@ void BridgeRouteEntry::HandleMulticastLabel(const Agent *agent,
const AgentPath *local_peer_path,
const AgentPath *local_vm_peer_path,
bool del,
const std::string &vrf_name,
uint32_t *evpn_label) {
*evpn_label = MplsTable::kInvalidLabel;

Expand Down Expand Up @@ -547,8 +548,10 @@ void BridgeRouteEntry::HandleMulticastLabel(const Agent *agent,
local_vm_peer_path == NULL)
delete_label = true;
}
if (delete_label)
agent->mpls_table()->DeleteMcastLabel(path->label());
if (delete_label) {
agent->mpls_table()->DeleteMcastLabel(path->label(),
vrf()->GetName());
}

return;
}
Expand Down Expand Up @@ -675,6 +678,7 @@ bool BridgeRouteEntry::ReComputeMulticastPaths(AgentPath *path, bool del) {
local_peer_path,
local_vm_peer_path,
del,
vrf()->GetName(),
&evpn_label);

//all paths are gone so delete multicast_peer path as well
Expand Down Expand Up @@ -808,7 +812,8 @@ bool BridgeRouteEntry::ReComputeMulticastPaths(AgentPath *path, bool del) {
vrf()->GetName());
//Delete Old label, in case label has changed for same peer.
if (old_fabric_mpls_label != fabric_peer_path->label()) {
agent->mpls_table()->DeleteMcastLabel(old_fabric_mpls_label);
agent->mpls_table()->DeleteMcastLabel(old_fabric_mpls_label,
vrf()->GetName());
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/vnsw/agent/oper/bridge_route.h
View file
Expand Up @@ -125,7 +125,9 @@ class BridgeRouteEntry : public AgentRoute {
AgentPath *path,
const AgentPath *local_peer_path,
const AgentPath *local_vm_peer_path,
bool del, uint32_t *evpn_label);
bool del,
const std::string &vrf_name,
uint32_t *evpn_label);
bool ReComputeMulticastPaths(AgentPath *path, bool del);
AgentPath *FindEvpnPathUsingKeyData(const AgentRouteKey *key,
const AgentRouteData *data) const;
Expand Down
43 changes: 41 additions & 2 deletions src/vnsw/agent/oper/mpls.cc
View file
Expand Up @@ -90,13 +90,33 @@ bool MplsTable::ChangeHandler(MplsLabel *mpls, const DBRequest *req) {
nh = static_cast<NextHop *>
(agent()->nexthop_table()->FindActiveEntry(&key));
}

if (mpls->IsFabricMulticastReservedLabel()) {
assert(data->vrf_name_.length() != 0);
mpls->fmg_nh_list()[data->vrf_name_] = nh;
}

ret = ChangeNH(mpls, nh);

return ret;
}

bool MplsTable::Delete(DBEntry *entry, const DBRequest *req) {
MplsLabelData *data = static_cast<MplsLabelData *>(req->data.get());
MplsLabel *mpls = static_cast<MplsLabel *>(entry);

if (data && IsFabricMulticastLabel(mpls->label())) {
mpls->fmg_nh_list().erase(data->vrf_name_);
if (mpls->fmg_nh_list().empty() == false) {
if (ChangeNH(mpls, mpls->fmg_nh_list().begin()->second)) {
DBTablePartBase *tpart =
static_cast<DBTablePartition *>(GetTablePartition(mpls));
tpart->Notify(mpls);
}
return false;
}
}

mpls->SendObjectLog(this, AgentLogEvent::DELETE);
return true;
}
Expand Down Expand Up @@ -223,6 +243,12 @@ void MplsLabel::Delete(const Agent *agent, uint32_t label) {
DBRequest req;
req.oper = DBRequest::DB_ENTRY_DELETE;

MplsLabel *label_entry =
Agent::GetInstance()->mpls_table()->FindMplsLabelUsingKey(label);
if (agent->mpls_table()->IsFabricMulticastLabel(label))
assert(0);
if (label_entry && label_entry->GetType() == MplsLabel::MCAST_NH)
assert(0);
MplsLabelKey *key = new MplsLabelKey(MplsLabel::INVALID, label);
req.key.reset(key);
req.data.reset(NULL);
Expand Down Expand Up @@ -404,13 +430,21 @@ void MplsTable::CreateMcastLabel(uint32_t label,
return;
}

void MplsTable::DeleteMcastLabel(uint32_t src_label) {
void MplsTable::DeleteMcastLabel(uint32_t src_label,
const std::string &vrf_name) {
DBRequest req;
req.oper = DBRequest::DB_ENTRY_DELETE;

MplsLabel *label_entry =
Agent::GetInstance()->mpls_table()->FindMplsLabelUsingKey(src_label);
if (!label_entry) {
return;
}

MplsLabelKey *key = new MplsLabelKey(MplsLabel::MCAST_NH, src_label);
MplsLabelData *data = new MplsLabelData(vrf_name);
req.key.reset(key);
req.data.reset(NULL);
req.data.reset(data);

Process(req);
return;
Expand All @@ -431,3 +465,8 @@ bool MplsTable::IsFabricMulticastLabel(uint32_t label) const {
}
return false;
}

MplsLabel *MplsTable::FindMplsLabelUsingKey(uint32_t label) {
MplsLabelKey key(label);
return static_cast<MplsLabel *>(Find(&key, false));
}
19 changes: 16 additions & 3 deletions src/vnsw/agent/oper/mpls.h
View file
Expand Up @@ -72,13 +72,17 @@ class MplsLabel : AgentRefCount<MplsLabel>, public AgentDBEntry {
AgentLogEvent::type event) const;
void SyncDependentPath();
bool IsFabricMulticastReservedLabel() const;
std::map<std::string, NextHop *> &fmg_nh_list() {
return fmg_nh_list_;
}

private:
const Agent *agent_;
Type type_;
uint32_t label_;
bool free_label_;
NextHopRef nh_;
std::map<std::string, NextHop *> fmg_nh_list_;
friend class MplsTable;
DEPENDENCY_LIST(AgentRoute, MplsLabel, mpls_label_);
DISALLOW_COPY_AND_ASSIGN(MplsLabel);
Expand Down Expand Up @@ -119,19 +123,26 @@ class MplsLabelData : public AgentData {
MplsLabelData(COMPOSITETYPE type, bool policy,
ComponentNHKeyList &component_nh_key_list, std::string vrf_name) :
AgentData(), nh_key(new CompositeNHKey(type, policy,
component_nh_key_list, vrf_name)) {
component_nh_key_list, vrf_name)), vrf_name_(vrf_name) {
}

MplsLabelData(const std::string vrf_name, bool policy) :
AgentData(), nh_key(new VrfNHKey(vrf_name, policy, false)) {
AgentData(), nh_key(new VrfNHKey(vrf_name, policy, false)),
vrf_name_(vrf_name) {
}

MplsLabelData(const std::string &vrf_name) :
AgentData(), nh_key(NULL), vrf_name_(vrf_name) {
}

virtual ~MplsLabelData() {
if (nh_key) {
delete nh_key;
}
};

NextHopKey *nh_key;
std::string vrf_name_;
};

/////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -161,7 +172,8 @@ class MplsTable : public AgentDBTable {
COMPOSITETYPE type,
ComponentNHKeyList &component_nh_key_list,
const std::string vrf_name);
void DeleteMcastLabel(uint32_t src_label);
void DeleteMcastLabel(uint32_t src_label,
const std::string &vrf_name);

// Allocate and Free label from the label_table
uint32_t AllocLabel() {
Expand All @@ -182,6 +194,7 @@ class MplsTable : public AgentDBTable {
uint32_t index = label >> mpls_shift_bits_;
label_table_.Remove(index);
}
MplsLabel *FindMplsLabelUsingKey(uint32_t label);
MplsLabel *FindMplsLabel(size_t label) {
uint32_t index = label >> mpls_shift_bits_;
return label_table_.At(index);
Expand Down
139 changes: 139 additions & 0 deletions src/vnsw/agent/test/test_l2route.cc
View file
Expand Up @@ -1726,6 +1726,145 @@ TEST_F(RouteTest, label_in_evpn_mcast_path) {
bgp_peer.reset();
}

TEST_F(RouteTest, test_ntt) {
client->Reset();
AddVrf("vrf1");
AddVn("vn1", 1);
AddLink("virtual-network", "vn1", "routing-instance", "vrf1");
client->WaitForIdle();
//Add a peer and enqueue path add in multicast route.
BgpPeer *bgp_peer_ptr = CreateBgpPeer(Ip4Address(1), "BGP Peer1");
agent_->mpls_table()->ReserveMulticastLabel(4000, 5000, 0);
MulticastHandler *mc_handler = static_cast<MulticastHandler *>(agent_->
oper_db()->multicast());
TunnelOlist olist;
olist.push_back(OlistTunnelEntry(nil_uuid(), 10,
IpAddress::from_string("8.8.8.8").to_v4(),
TunnelType::MplsType()));
mc_handler->ModifyTorMembers(bgp_peer_ptr,
"vrf1",
olist,
10,
1);
client->WaitForIdle();

mc_handler->ModifyFabricMembers(Agent::GetInstance()->
multicast_tree_builder_peer(),
"vrf1",
IpAddress::from_string("255.255.255.255").to_v4(),
IpAddress::from_string("0.0.0.0").to_v4(),
4100, olist, 1);
client->WaitForIdle();
//MulticastGroupObject *obj =
// mc_handler->FindFloodGroupObject("vrf1");
//uint32_t evpn_label = obj->evpn_mpls_label();
//EXPECT_FALSE(FindMplsLabel(MplsLabel::MCAST_NH, evpn_label));

//Delete remote paths
mc_handler->ModifyFabricMembers(Agent::GetInstance()->
multicast_tree_builder_peer(),
"vrf1",
IpAddress::from_string("255.255.255.255").to_v4(),
IpAddress::from_string("0.0.0.0").to_v4(),
4100, olist,
ControllerPeerPath::kInvalidPeerIdentifier);
client->WaitForIdle();
TunnelOlist del_olist;
mc_handler->ModifyTorMembers(bgp_peer_ptr,
"vrf1",
del_olist,
10,
ControllerPeerPath::kInvalidPeerIdentifier);
client->WaitForIdle();
mc_handler->ModifyTorMembers(bgp_peer_ptr,
"vrf1",
olist,
10,
1);
client->WaitForIdle();
mc_handler->ModifyTorMembers(bgp_peer_ptr,
"vrf1",
del_olist,
10,
ControllerPeerPath::kInvalidPeerIdentifier);
client->WaitForIdle();

DeleteBgpPeer(bgp_peer_ptr);
client->WaitForIdle();
}

TEST_F(RouteTest, test_fmg_label_1) {
client->Reset();
AddVrf("vrf1");
AddVn("vn1", 1);
AddLink("virtual-network", "vn1", "routing-instance", "vrf1");
AddVrf("vrf2");
AddVn("vn2", 2);
AddLink("virtual-network", "vn2", "routing-instance", "vrf2");
client->WaitForIdle();
//Add a peer and enqueue path add in multicast route.
agent_->mpls_table()->ReserveMulticastLabel(4000, 5000, 0);
MulticastHandler *mc_handler = static_cast<MulticastHandler *>(agent_->
oper_db()->multicast());
TunnelOlist olist;
olist.push_back(OlistTunnelEntry(nil_uuid(), 10,
IpAddress::from_string("8.8.8.8").to_v4(),
TunnelType::MplsType()));
mc_handler->ModifyFabricMembers(Agent::GetInstance()->
multicast_tree_builder_peer(),
"vrf1",
IpAddress::from_string("255.255.255.255").to_v4(),
IpAddress::from_string("0.0.0.0").to_v4(),
4100, olist, 1);
client->WaitForIdle();
MplsLabel *label_entry =
Agent::GetInstance()->mpls_table()->FindMplsLabelUsingKey(4100);
const CompositeNH *cnh = dynamic_cast<const CompositeNH *>(label_entry->
nexthop());
EXPECT_TRUE(label_entry != NULL);
EXPECT_TRUE(cnh->vrf()->GetName() == "vrf1");
mc_handler->ModifyFabricMembers(Agent::GetInstance()->
multicast_tree_builder_peer(),
"vrf2",
IpAddress::from_string("255.255.255.255").to_v4(),
IpAddress::from_string("0.0.0.0").to_v4(),
4100, olist, 1);
client->WaitForIdle();
label_entry =
Agent::GetInstance()->mpls_table()->FindMplsLabelUsingKey(4100);
cnh = dynamic_cast<const CompositeNH *>(label_entry->nexthop());
EXPECT_TRUE(label_entry != NULL);

mc_handler->ModifyFabricMembers(Agent::GetInstance()->
multicast_tree_builder_peer(),
"vrf2",
IpAddress::from_string("255.255.255.255").to_v4(),
IpAddress::from_string("0.0.0.0").to_v4(),
4100, olist,
ControllerPeerPath::kInvalidPeerIdentifier);
client->WaitForIdle();
label_entry =
Agent::GetInstance()->mpls_table()->FindMplsLabelUsingKey(4100);
EXPECT_TRUE(label_entry != NULL);
//Start cleaning
//Delete remote paths
mc_handler->ModifyFabricMembers(Agent::GetInstance()->
multicast_tree_builder_peer(),
"vrf1",
IpAddress::from_string("255.255.255.255").to_v4(),
IpAddress::from_string("0.0.0.0").to_v4(),
4100, olist,
ControllerPeerPath::kInvalidPeerIdentifier);
client->WaitForIdle();
DelLink("virtual-network", "vn1", "routing-instance", "vrf1");
DelLink("virtual-network", "vn2", "routing-instance", "vrf2");
DelVrf("vrf1");
DelVn("vn1");
DelVrf("vrf2");
DelVn("vn2");
client->WaitForIdle();
}

int main(int argc, char *argv[]) {
::testing::InitGoogleTest(&argc, argv);
GETUSERARGS();
Expand Down

0 comments on commit 6f8d7b4

Please sign in to comment.