Skip to content

Commit

Permalink
Route present wih no paths.
Browse files Browse the repository at this point in the history 
Problem:
When only stale paths in headless mode were present and CN goes down, then walk
was issued to squash all of them. However after squashing it didnt check if
route has no paths and delete the same. So route was present with no paths.

Solution:
Re-org the squashing code in walk to go via proper removal of path and
check for zero path to delete route.

Closes-bug: #1562961

Conflicts:
	src/vnsw/agent/test/test_l2route.cc

Conflicts:
	src/vnsw/agent/test/test_l2route.cc

Conflicts:
	src/vnsw/agent/test/test_l2route.cc

Double path delete request in succession.

Problem:
In stale path cleanup, if no stale path was found function used to clean last
path seen. This resulted in not related path getting deleted. In case of OVS
delete the path to be deleted was already gone because of bug and table deleted,
resulting in ceash.

Solution:
Delete if relevant path is found.
Closes-bug: #1571598

Change-Id: I25bd7cec4c0774d0a041286c15af99bc5a2d1ada
  • Loading branch information
@manishsing
manishsing committed Apr 19, 2016
1 parent b0358a5 commit bf49b3c
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 20 deletions.
54 changes: 35 additions & 19 deletions src/vnsw/agent/oper/agent_route.cc
View file
Expand Up @@ -146,7 +146,7 @@ bool AgentRouteTable::DeleteAllBgpPath(DBTablePartBase *part,
DeletePathFromPeer(part, route, path);
to_be_deleted_path_list_it++;
}
route->SquashStalePaths(NULL);
SquashStalePaths(route, NULL);
}
return true;
}
Expand Down Expand Up @@ -255,7 +255,10 @@ void AgentRouteTable::DeletePathFromPeer(DBTablePartBase *part,
// For active peers reflector will remove the route but for
// non active peers explicitly squash the paths.
if (peer && (peer->GetType() != Peer::BGP_PEER)) {
rt->SquashStalePaths(NULL);
path = rt->FindStalePath();
if (path) {
rt->RemovePath(path);
}
}

// Delete route if no more paths
Expand Down Expand Up @@ -519,6 +522,23 @@ void AgentRouteTable::NotifyEntry(AgentRoute *e) {
tpart->Notify(e);
}

void AgentRouteTable::SquashStalePaths(AgentRoute *route,
const AgentPath *exception_path) {
Route::PathList::iterator it = route->GetPathList().begin();
AgentPath *path = NULL;
while (it != route->GetPathList().end()) {
path = static_cast<AgentPath *>(it.operator->());
// Delete all stale path except for the path sent(exception_path)
if (path->is_stale() && (path != exception_path)) {
// Since we squash stales, at any point of time there should be only
// one stale other than exception_path in list
DeletePathFromPeer(route->get_table_partition(), route, path);
return;
}
it++;
}
}

uint32_t AgentRoute::GetActiveLabel() const {
return GetActivePath()->label();
};
Expand Down Expand Up @@ -581,6 +601,18 @@ AgentPath *AgentRoute::FindLocalVmPortPath() const {
return NULL;
}

AgentPath *AgentRoute::FindStalePath() {
Route::PathList::iterator it = GetPathList().begin();
while (it != GetPathList().end()) {
AgentPath *path = static_cast<AgentPath *>(it.operator->());
if (path->is_stale()) {
return path;
}
it++;
}
return NULL;
}

void AgentRoute::DeletePathInternal(AgentPath *path) {
AgentRouteTable *table = static_cast<AgentRouteTable *>(get_table());
table->DeletePathFromPeer(get_table_partition(), this, path);
Expand Down Expand Up @@ -616,22 +648,6 @@ AgentPath *AgentRoute::FindPath(const Peer *peer) const {
return NULL;
}

void AgentRoute::SquashStalePaths(const AgentPath *exception_path) {
Route::PathList::iterator it = GetPathList().begin();

while (it != GetPathList().end()) {
// Delete all stale path except for the path sent(exception_path)
AgentPath *path = static_cast<AgentPath *>(it.operator->());
if (path->is_stale() && (path != exception_path)) {
// Since we squash stales, at any point of time there should be only
// one stale other than exception_path in list
RemovePath(path);
return;
}
it++;
}
}

// First path in list is always treated as active path.
const AgentPath *AgentRoute::GetActivePath() const {
return static_cast<const AgentPath *>(front());
Expand Down Expand Up @@ -753,7 +769,7 @@ void AgentRouteTable::StalePathFromPeer(DBTablePartBase *part, AgentRoute *rt,
if (rt && (rt->IsDeleted() == false)) {
path->set_is_stale(true);
// Remove all stale path except the path received
rt->SquashStalePaths(path);
SquashStalePaths(rt, path);
rt->GetPathList().sort(&AgentRouteTable::PathSelection);
rt->Sync();
part->Notify(rt);
Expand Down
3 changes: 2 additions & 1 deletion src/vnsw/agent/oper/agent_route.h
View file
Expand Up @@ -181,6 +181,7 @@ class AgentRouteTable : public RouteTable {
//Stale path handling
void StalePathFromPeer(DBTablePartBase *part, AgentRoute *rt,
const Peer *peer);
void SquashStalePaths(AgentRoute *rt, const AgentPath *path);

private:
class DeleteActor;
Expand Down Expand Up @@ -253,6 +254,7 @@ class AgentRoute : public Route {
uint32_t vrf_id() const;

AgentPath *FindLocalVmPortPath() const;
AgentPath *FindStalePath();
const AgentPath *GetActivePath() const;
const NextHop *GetActiveNextHop() const;
const std::string &dest_vn_name() const;
Expand All @@ -262,7 +264,6 @@ class AgentRoute : public Route {
void ResyncTunnelNextHop();
bool HasUnresolvedPath();
bool Sync(void);
void SquashStalePaths(const AgentPath *path);

//TODO Move dependantroutes and nh to inet4
void UpdateDependantRoutes();// analogous to updategatewayroutes
Expand Down
96 changes: 96 additions & 0 deletions src/vnsw/agent/test/test_l2route.cc
View file
Expand Up @@ -1545,6 +1545,102 @@ TEST_F(RouteTest, add_local_peer_and_then_vm) {
bgp_peer.reset();
}

//Bug# 1562961
TEST_F(RouteTest, StalePathDeleteRouteDelete) {
struct PortInfo input[] = {
{"vnet1", 1, "1.1.1.10", "00:00:01:01:01:10", 1, 1},
};

client->Reset();
CreateVmportEnv(input, 1);
client->WaitForIdle();

//Add a peer and keep a reference of same.
BgpPeer *bgp_peer_ptr = CreateBgpPeer(Ip4Address(1), "BGP Peer1");
boost::shared_ptr<BgpPeer> bgp_peer =
bgp_peer_ptr->GetBgpXmppPeer()->bgp_peer_id_ref();

BridgeTunnelRouteAdd(bgp_peer_ptr, vrf_name_, TunnelType::MplsType(),
server1_ip_, MplsTable::kStartLabel + 10,
remote_vm_mac_, remote_vm_ip4_, 32);
client->WaitForIdle();

EvpnRouteEntry *rt = EvpnRouteGet(vrf_name_, remote_vm_mac_,
remote_vm_ip4_, 0);
EXPECT_TRUE(rt != NULL);
AgentPath *path = rt->FindPath(bgp_peer_ptr);
path->set_is_stale(true);
DelVrf(vrf_name_.c_str());
client->WaitForIdle();
rt = EvpnRouteGet(vrf_name_, remote_vm_mac_,
remote_vm_ip4_, 0);
EXPECT_TRUE(rt == NULL);

DeleteBgpPeer(bgp_peer.get());
client->WaitForIdle();
DeleteVmportEnv(input, 1, true);
client->WaitForIdle();
//Release VRF reference
bgp_peer.reset();
client->WaitForIdle();
}

class SetupTask : public Task {
public:
SetupTask(RouteTest *test, std::string name) :
Task((TaskScheduler::GetInstance()->
GetTaskId("db::DBTable")), 0), test_(test),
test_name_(name) {
}

virtual bool Run() {
if (test_name_ == "SquashPathTest_1") {
char local_vm_mac_str_[100];
MacAddress local_vm_mac_;
Ip4Address local_vm_ip4_;
char local_vm_ip4_str_[100];
strcpy(local_vm_ip4_str_, "1.1.1.10");
local_vm_ip4_ = Ip4Address::from_string(local_vm_ip4_str_);
strcpy(local_vm_mac_str_, "00:00:01:01:01:10");
local_vm_mac_ = MacAddress::FromString(local_vm_mac_str_);
EvpnRouteEntry *rt = EvpnRouteGet("vrf1",
local_vm_mac_,
local_vm_ip4_,
0);
const VrfEntry *vrf = VrfGet("vrf1");
vrf->GetEvpnRouteTable()->SquashStalePaths(rt, NULL);
}
return true;
}
std::string Description() const { return "SetupTask"; }
private:
RouteTest *test_;
std::string test_name_;
};

//Bug# 1571598
TEST_F(RouteTest, SquashPathTest_1) {
struct PortInfo input[] = {
{"vnet1", 1, "1.1.1.10", "00:00:01:01:01:10", 1, 1},
};

client->Reset();
CreateVmportEnv(input, 1);
client->WaitForIdle();

EvpnRouteEntry *rt = EvpnRouteGet("vrf1", local_vm_mac_, local_vm_ip4_, 0);
EXPECT_TRUE(rt != NULL);
EXPECT_TRUE(rt->GetActivePath() != NULL);
SetupTask * task = new SetupTask(this, "SquashPathTest_1");
TaskScheduler::GetInstance()->Enqueue(task);
client->WaitForIdle();
rt = EvpnRouteGet("vrf1", local_vm_mac_, local_vm_ip4_, 0);
EXPECT_TRUE(rt != NULL);

DeleteVmportEnv(input, 1, true);
client->WaitForIdle();
}

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

0 comments on commit bf49b3c

Please sign in to comment.