Skip to content

Commit

Permalink
* If arp route is deleted twice, then we arp nexthop was not
Browse files Browse the repository at this point in the history 
  getting deleted, since upon each arp route delete we enqueue
  a request to modify nexthop to invalid and this might
  result in arp entry creation, hence use operation RESYNC
  instead of add/change
* Check for vrf while parsing on subnet address change, since
  we might modify next vrf subnet routes if prefixx matches
Test case for same
Closes-bug:#1423502
Change-Id: I09854932ef6045398ba25301f921840edf41a603
  • Loading branch information
@naveen-n
naveen-n committed Feb 19, 2015
1 parent 54e50e1 commit a65d557
Show file tree
Hide file tree
Showing 4 changed files with 170 additions and 12 deletions.
10 changes: 9 additions & 1 deletion src/vnsw/agent/oper/inet_unicast_route.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,15 @@ InetUnicastAgentRouteTable::ArpRoute(DBRequest::DBOperation op,
const SecurityGroupList &sg) {
Agent *agent = Agent::GetInstance();
DBRequest nh_req(DBRequest::DB_ENTRY_ADD_CHANGE);
nh_req.key.reset(new ArpNHKey(nexthop_vrf_name, ip, policy));
ArpNHKey *nh_key = new ArpNHKey(nexthop_vrf_name, ip, policy);
if (op == DBRequest::DB_ENTRY_DELETE) {
//In case of delete we want to set the
//nexthop as invalid, hence use resync operation
//We dont want the nexthop to created again
//in case of duplicate delete
nh_key->sub_op_ = AgentKey::RESYNC;
}
nh_req.key.reset(nh_key);
ArpNHData *arp_data = new ArpNHData(mac,
static_cast<InterfaceKey *>(intf.GetDBRequestKey().release()),
resolved);
Expand Down
16 changes: 6 additions & 10 deletions src/vnsw/agent/services/arp_proto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,10 @@ void ArpDBState::UpdateArpRoutes(const InetUnicastRouteEntry *rt) {
ArpProto::ArpIterator start_iter =
vrf_state_->arp_proto->FindUpperBoundArpEntry(start_key);

if (start_iter->first.vrf != rt->vrf()) {
return;
}


while (IsIp4SubnetMember(Ip4Address(start_iter->first.ip),
while (start_iter != vrf_state_->arp_proto->arp_cache().end() &&
start_iter->first.vrf == rt->vrf() &&
IsIp4SubnetMember(Ip4Address(start_iter->first.ip),
rt->addr().to_v4(), plen)) {
start_iter->second->Resync(policy_, vn_, sg_list_);
start_iter++;
Expand All @@ -226,11 +224,9 @@ void ArpDBState::Delete(const InetUnicastRouteEntry *rt) {
ArpProto::ArpIterator start_iter =
vrf_state_->arp_proto->FindUpperBoundArpEntry(start_key);

if (start_iter->first.vrf != rt->vrf()) {
return;
}

while (IsIp4SubnetMember(Ip4Address(start_iter->first.ip),
while (start_iter != vrf_state_->arp_proto->arp_cache().end() &&
start_iter->first.vrf == rt->vrf() &&
IsIp4SubnetMember(Ip4Address(start_iter->first.ip),
rt->addr().to_v4(), plen)) {
ArpProto::ArpIterator tmp = start_iter++;
if (tmp->second->DeleteArpRoute()) {
Expand Down
132 changes: 132 additions & 0 deletions src/vnsw/agent/services/test/arp_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,14 @@ TEST_F(ArpTest, DISABLED_SubnetResolveWithoutPolicy) {
EXPECT_TRUE(VmPortFind(8));
client->Reset();

AddPhysicalDevice(agent->host_name().c_str(), 1);
AddPhysicalInterface("pi1", 1, "pid1");
AddLogicalInterface("lp1", 1, "lp1", 1);
AddLink("physical-router", "prouter1", "physical-interface", "pi1");
AddLink("logical-interface", "lp1", "physical-interface", "pi1");
AddLink("virtual-machine-interface", "vnet8", "logical-interface", "lp1");
client->WaitForIdle();

//Add a link to interface subnet and ensure resolve route is added
AddSubnetType("subnet", 1, "8.1.1.0", 24);
AddLink("virtual-machine-interface", input1[0].name,
Expand Down Expand Up @@ -831,6 +839,14 @@ TEST_F(ArpTest, DISABLED_SubnetResolveWithSg) {
EXPECT_TRUE(VmPortFind(8));
client->Reset();

AddPhysicalDevice(agent->host_name().c_str(), 1);
AddPhysicalInterface("pi1", 1, "pid1");
AddLogicalInterface("lp1", 1, "lp1", 1);
AddLink("physical-router", "prouter1", "physical-interface", "pi1");
AddLink("logical-interface", "lp1", "physical-interface", "pi1");
AddLink("virtual-machine-interface", "vnet8", "logical-interface", "lp1");
client->WaitForIdle();

AddSg("sg1", 1);
AddAcl("acl1", 1);
AddLink("security-group", "sg1", "access-control-list", "acl1");
Expand Down Expand Up @@ -864,6 +880,7 @@ TEST_F(ArpTest, DISABLED_SubnetResolveWithSg) {
DelLink("security-group", "sg1", "access-control-list", "acl1");
DelAcl("acl1");
DelNode("security-group", "sg1");
DelLink("virtual-machine-interface", "vnet8", "security-group", "sg1");
client->WaitForIdle();
DeleteVmportEnv(input1, 1, true, 1);
client->WaitForIdle();
Expand All @@ -876,6 +893,121 @@ TEST_F(ArpTest, DISABLED_SubnetResolveWithSg) {
client->Reset();
}

//Check leaked routes dont get updated
//when original route changes
TEST_F(ArpTest, DISABLED_SubnetResolveWithSg1) {
struct PortInfo input1[] = {
{"vnet8", 8, "8.1.1.1", "00:00:00:01:01:01", 1, 1},
{"vnet9", 9, "9.1.1.1", "00:00:00:01:01:02", 2, 2}
};

client->Reset();
//Create VM interface with policy
CreateVmportWithEcmp(input1, 2, 1);
client->WaitForIdle();
EXPECT_TRUE(VmPortActive(input1, 0));
EXPECT_TRUE(VmPortFind(8));
client->Reset();

AddPhysicalDevice(agent->host_name().c_str(), 1);
AddPhysicalInterface("pi1", 1, "pid1");
AddLogicalInterface("lp1", 1, "lp1", 1);
AddLink("physical-router", "prouter1", "physical-interface", "pi1");
AddLink("logical-interface", "lp1", "physical-interface", "pi1");
AddLink("virtual-machine-interface", "vnet8", "logical-interface", "lp1");
client->WaitForIdle();

VmInterface *vintf8 = VmInterfaceGet(8);
VmInterface *vintf9 = VmInterfaceGet(9);

AddSg("sg1", 1);
AddAcl("acl1", 1);
AddLink("security-group", "sg1", "access-control-list", "acl1");
client->WaitForIdle();
//Add a link to interface subnet and ensure resolve route is added
AddSubnetType("subnet", 1, "8.1.1.0", 24);
AddLink("virtual-machine-interface", input1[0].name,
"subnet", "subnet");
client->WaitForIdle();
EXPECT_TRUE(VmPortActive(input1, 0));
EXPECT_TRUE(RouteFind("vrf1", "8.1.1.0", 24));

VmInterfaceKey key(AgentKey::ADD_DEL_CHANGE, MakeUuid(8), "vnet8");
Ip4Address subnet = Ip4Address::from_string("8.1.1.0");
InetUnicastAgentRouteTable::AddResolveRoute(vintf9->peer(), "vrf2",
subnet, 24, key, 0, false,
"vn1", SecurityGroupList());
client->WaitForIdle();

Ip4Address sip = Ip4Address::from_string("9.1.1.1");
Ip4Address dip = Ip4Address::from_string("8.1.1.2");
Ip4Address dip1 = Ip4Address::from_string("8.1.1.3");
SendArpReq(vintf9->id(), vintf9->vrf()->vrf_id(), sip.to_ulong(), dip.to_ulong());
SendArpReq(vintf9->id(), vintf9->vrf()->vrf_id(), sip.to_ulong(), dip1.to_ulong());
client->WaitForIdle();
WAIT_FOR(500, 1000,
Agent::GetInstance()->GetArpProto()->GetArpCacheSize() == 5);

WAIT_FOR(500, 1000, RouteFind("vrf2", "8.1.1.2", 32) == true);
WAIT_FOR(500, 1000, RouteFind("vrf2", "8.1.1.3", 32) == true);
AddLink("virtual-machine-interface", "vnet8", "security-group", "sg1");
client->WaitForIdle();

//Verify that route update on vrf1 doesnt reevaluate vrf2 arp routes
AgentRoute *rt = RouteGet("vrf2", dip, 32);
EXPECT_TRUE(rt->GetActiveNextHop()->GetType() == NextHop::ARP);
EXPECT_TRUE(rt->GetActivePath()->dest_vn_name() == "vn1");
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 0);
rt = RouteGet("vrf2", dip1, 32);
EXPECT_TRUE(rt->GetActiveNextHop()->GetType() == NextHop::ARP);
EXPECT_TRUE(rt->GetActivePath()->dest_vn_name() == "vn1");
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 0);

rt = RouteGet("vrf1", dip, 32);
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 1);
rt = RouteGet("vrf1", dip1, 32);
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 1);

//Resync the same on leaked vrf
InetUnicastAgentRouteTable::AddResolveRoute(vintf9->peer(), "vrf2",
subnet, 24, key, 0, false,
"vn1", SecurityGroupList(1));
client->WaitForIdle();

rt = RouteGet("vrf2", dip, 32);
EXPECT_TRUE(rt->GetActiveNextHop()->GetType() == NextHop::ARP);
EXPECT_TRUE(rt->GetActivePath()->dest_vn_name() == "vn1");
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 1);

rt = RouteGet("vrf2", dip1, 32);
EXPECT_TRUE(rt->GetActiveNextHop()->GetType() == NextHop::ARP);
EXPECT_TRUE(rt->GetActivePath()->dest_vn_name() == "vn1");
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 1);

rt = RouteGet("vrf1", dip, 32);
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 1);
rt = RouteGet("vrf1", dip1, 32);
EXPECT_TRUE(rt->GetActivePath()->sg_list().size() == 1);

DelLink("virtual-machine-interface", input1[0].name,
"subnet", "subnet");
DelLink("security-group", "sg1", "access-control-list", "acl1");
DelAcl("acl1");
DelNode("security-group", "sg1");
InetUnicastAgentRouteTable::DeleteReq(vintf9->peer(), "vrf2",
subnet, 24, NULL);
client->WaitForIdle();
DeleteVmportEnv(input1, 2, true, 1);
client->WaitForIdle();
WAIT_FOR(500, 1000, RouteFind("vrf1", "8.1.1.2", 32) == false);
EXPECT_EQ(1U, Agent::GetInstance()->GetArpProto()->GetArpCacheSize());

EXPECT_FALSE(VmPortFind(8));
WAIT_FOR(100, 1000, (Agent::GetInstance()->interface_table()->Find(&key, true)
== NULL));
client->Reset();
}

TEST_F(ArpTest, IntfArpReqTest_1) {
struct PortInfo input[] = {
{"vnet1", 1, "1.1.1.1", "00:00:00:01:01:01", 1, 1},
Expand Down
24 changes: 23 additions & 1 deletion src/vnsw/agent/test/test_route.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ class RouteTest : public ::testing::Test {
TestRouteTable table3(3);
WAIT_FOR(100, 1000, (table3.Size() == 0));
EXPECT_EQ(table3.Size(), 0U);

VrfDelReq(vrf_name_.c_str());
client->WaitForIdle();
WAIT_FOR(100, 1000, (VrfFind(vrf_name_.c_str()) != true));
Expand Down Expand Up @@ -2042,6 +2041,29 @@ TEST_F(RouteTest, Dhcp_mode_toggled_ipam) {
client->WaitForIdle();
}

//Double delete ARP route and verify that ARP NH
//get deleted, since we always enqueu RESYNC for arp NH change
//from ARP route deletiong path
TEST_F(RouteTest, ArpRouteDelete) {
ArpNHKey key(Agent::GetInstance()->fabric_vrf_name(), server1_ip_, false);

AddArp(server1_ip_.to_string().c_str(), "0a:0b:0c:0d:0e:0f", eth_name_.c_str());
client->WaitForIdle();
EXPECT_TRUE(FindNH(&key));
EXPECT_TRUE(GetNH(&key)->IsValid() == true);

//Delete Remote VM route
DelArp(server1_ip_.to_string(), "0a:0b:0c:0d:0e:0f", eth_name_.c_str());
client->WaitForIdle();
EXPECT_FALSE(RouteFind(vrf_name_, server1_ip_, 32));

DelArp(server1_ip_.to_string(), "0a:0b:0c:0d:0e:0f", eth_name_.c_str());
client->WaitForIdle();
EXPECT_FALSE(RouteFind(vrf_name_, server1_ip_, 32));

EXPECT_FALSE(FindNH(&key));
}

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

0 comments on commit a65d557

Please sign in to comment.