Skip to content

Commit

Permalink
On deletion of AgentXmppchannel, BGP peer was not cleared properly.
Browse files Browse the repository at this point in the history 
Ideally every channel state change is responsible for cleaning up or creating
BGP peer. However with commit
6d845c1
above assumption will not be true.
To fix it artificially inject a channel down event on deletion of agent xmpp
channel. Since BGP peer is being manipulated also push the process of applying
discovery servers to controller work queue.

Change-Id: Ia733ed1061747153fbfb841f63813ad148ce6bfc
Closes-bug: 1460435
  • Loading branch information
@manishsing
manishsing committed Jun 8, 2015
1 parent d6fa249 commit 45df647
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 12 deletions.
38 changes: 36 additions & 2 deletions src/vnsw/agent/controller/controller_init.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ using namespace boost::asio;
SandeshTraceBufferPtr ControllerTraceBuf(SandeshTraceBufferCreate(
"Controller", 1000));

ControllerDiscoveryData::ControllerDiscoveryData(std::vector<DSResponse> resp) :
ControllerWorkQueueData(), discovery_response_(resp) {
}

VNController::VNController(Agent *agent)
: agent_(agent), multicast_sequence_number_(0),
unicast_cleanup_timer_(agent), multicast_cleanup_timer_(agent),
Expand Down Expand Up @@ -229,6 +233,22 @@ void VNController::DnsXmppServerDisConnect() {

}

//During delete of xmpp channel, check if BGP peer is deleted.
//If not agent never got a channel down state and is being removed
//as it is not part of discovery list.
//Artificially inject NOT_READY in agent xmpp channel.
void VNController::DeleteAgentXmppChannel(AgentXmppChannel *channel) {
if (!channel)
return;

BgpPeer *bgp_peer = channel->bgp_peer_id();
if (bgp_peer != NULL) {
AgentXmppChannel::HandleAgentXmppClientChannelEvent(channel,
xmps::NOT_READY);
}
delete channel;
}

//Trigger shutdown and cleanup of routes for the client
void VNController::DisConnect() {
XmppServerDisConnect();
Expand Down Expand Up @@ -302,11 +322,11 @@ void VNController::DisConnectControllerIfmapServer(uint8_t idx) {

//cleanup AgentXmppChannel
agent_->ResetAgentMcastLabelRange(idx);
delete agent_->controller_xmpp_channel(idx);
DeleteAgentXmppChannel(agent_->controller_xmpp_channel(idx));
agent_->set_controller_xmpp_channel(NULL, idx);

//cleanup AgentIfmapXmppChannel
delete agent_->ifmap_xmpp_channel(idx);
delete agent_->ifmap_xmpp_channel(idx);
agent_->set_ifmap_xmpp_channel(NULL, idx);

agent_->controller_ifmap_xmpp_init(idx)->Reset();
Expand All @@ -330,7 +350,13 @@ bool VNController::AgentXmppServerExists(const std::string &server_ip,
}

void VNController::ApplyDiscoveryXmppServices(std::vector<DSResponse> resp) {
ControllerDiscoveryDataType data(new ControllerDiscoveryData(resp));
ControllerWorkQueueDataType base_data =
boost::static_pointer_cast<ControllerWorkQueueData>(data);
work_queue_.Enqueue(base_data);
}

bool VNController::ApplyDiscoveryXmppServicesInternal(std::vector<DSResponse> resp) {
std::vector<DSResponse>::iterator iter;
int8_t count = -1;
for (iter = resp.begin(); iter != resp.end(); iter++) {
Expand Down Expand Up @@ -414,6 +440,7 @@ void VNController::ApplyDiscoveryXmppServices(std::vector<DSResponse> resp) {
}

XmppServerConnect();
return true;
}

AgentDnsXmppChannel *VNController::FindAgentDnsXmppChannel(
Expand Down Expand Up @@ -696,6 +723,13 @@ bool VNController::ControllerWorkQueueProcess(ControllerWorkQueueDataType data)
return ControllerPeerHeadlessAgentDelDone(derived_walk_done_data->
bgp_peer());
}
//Discovery response for servers
ControllerDiscoveryDataType discovery_data =
boost::dynamic_pointer_cast<ControllerDiscoveryData>(data);
if (discovery_data) {
return ApplyDiscoveryXmppServicesInternal(discovery_data->
discovery_response_);
}
return true;
}

Expand Down
16 changes: 14 additions & 2 deletions src/vnsw/agent/controller/controller_init.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,21 @@ class ControllerXmppData : public ControllerWorkQueueData {
DISALLOW_COPY_AND_ASSIGN(ControllerXmppData);
};

class ControllerDiscoveryData : public ControllerWorkQueueData {
public:
ControllerDiscoveryData(std::vector<DSResponse> resp);
virtual ~ControllerDiscoveryData() {}

std::vector<DSResponse> discovery_response_;
DISALLOW_COPY_AND_ASSIGN(ControllerDiscoveryData);
};

class VNController {
public:
typedef boost::shared_ptr<ControllerXmppData> ControllerXmppDataType;
typedef boost::shared_ptr<ControllerDeletePeerData> ControllerDeletePeerDataType;
typedef boost::shared_ptr<ControllerWorkQueueData> ControllerWorkQueueDataType;
typedef boost::shared_ptr<ControllerDiscoveryData> ControllerDiscoveryDataType;
typedef boost::shared_ptr<BgpPeer> BgpPeerPtr;
typedef std::list<boost::shared_ptr<BgpPeer> >::iterator BgpPeerIterator;
VNController(Agent *agent);
Expand All @@ -86,8 +96,8 @@ class VNController {
void XmppServerDisConnect();
void DnsXmppServerDisConnect();

void ApplyDiscoveryXmppServices(std::vector<DSResponse> resp);
void ApplyDiscoveryDnsXmppServices(std::vector<DSResponse> resp);
void ApplyDiscoveryXmppServices(std::vector<DSResponse> resp);
void ApplyDiscoveryDnsXmppServices(std::vector<DSResponse> resp);

void DisConnectControllerIfmapServer(uint8_t idx);
void DisConnectDnsServer(uint8_t idx);
Expand Down Expand Up @@ -129,6 +139,7 @@ class VNController {
bool XmppMessageProcess(ControllerXmppDataType data);
Agent *agent() {return agent_;}
void Enqueue(ControllerWorkQueueDataType data);
void DeleteAgentXmppChannel(AgentXmppChannel *ch);

private:
AgentXmppChannel *FindAgentXmppChannel(const std::string &server_ip);
Expand All @@ -137,6 +148,7 @@ class VNController {
const std::string MakeConnectionPrefix(bool is_dns) const;
bool AgentXmppServerExists(const std::string &server_ip,
std::vector<DSResponse> resp);
bool ApplyDiscoveryXmppServicesInternal(std::vector<DSResponse> resp);

Agent *agent_;
uint64_t multicast_sequence_number_;
Expand Down
2 changes: 2 additions & 0 deletions src/vnsw/agent/controller/controller_peer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ AgentXmppChannel::AgentXmppChannel(Agent *agent,
}

AgentXmppChannel::~AgentXmppChannel() {
BgpPeer *bgp_peer = bgp_peer_id_.get();
assert(bgp_peer == NULL);
channel_->UnRegisterReceive(xmps::BGP);
}

Expand Down
37 changes: 37 additions & 0 deletions src/vnsw/agent/test/test_route.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2124,6 +2124,43 @@ TEST_F(RouteTest, ArpRouteDelete) {
EXPECT_FALSE(FindNH(&key));
}

TEST_F(RouteTest, verify_channel_delete_results_in_path_delete) {
client->Reset();
struct PortInfo input[] = {
{"vnet1", 1, "1.1.1.1", "00:00:00:01:01:01", 1, 1},
};

IpamInfo ipam_info[] = {
{"1.1.1.0", 24, "1.1.1.200", true},
};
client->Reset();
CreateVmportEnv(input, 1, 0);
client->WaitForIdle();
AddIPAM("vn1", ipam_info, 1);
client->WaitForIdle();

BgpPeer *peer = CreateBgpPeer("127.0.0.1", "remote");
FillEvpnNextHop(peer, "vrf1", 1000, TunnelType::MplsType());
client->WaitForIdle();
//Get Channel and delete it.
AgentXmppChannel *ch = peer->GetBgpXmppPeer();
XmppChannelMock *xmpp_channel = static_cast<XmppChannelMock *>
(ch->GetXmppChannel());
delete ch;
delete xmpp_channel;
client->WaitForIdle();

client->Reset();
DelIPAM("vn1");
client->WaitForIdle();
FlushEvpnNextHop(peer, "vrf1", 0);
DeleteVmportEnv(input, 1, 1, 0);
client->WaitForIdle();
DeleteBgpPeer(NULL);
client->WaitForIdle();
}


int main(int argc, char *argv[]) {
::testing::InitGoogleTest(&argc, argv);
GETUSERARGS();
Expand Down
19 changes: 11 additions & 8 deletions src/vnsw/agent/test/test_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3127,11 +3127,12 @@ BgpPeer *CreateBgpPeer(const Ip4Address &addr, std::string name) {

void DeleteBgpPeer(Peer *peer) {
BgpPeer *bgp_peer = static_cast<BgpPeer *>(peer);
if (!bgp_peer)
return;

AgentXmppChannel *channel = bgp_peer->GetBgpXmppPeer();
AgentXmppChannel::HandleAgentXmppClientChannelEvent(channel, xmps::NOT_READY);
AgentXmppChannel *channel = NULL;
if (bgp_peer) {
channel = bgp_peer->GetBgpXmppPeer();
AgentXmppChannel::HandleAgentXmppClientChannelEvent(channel, xmps::NOT_READY);
}
client->WaitForIdle();
TaskScheduler::GetInstance()->Stop();
Agent::GetInstance()->controller()->unicast_cleanup_timer().cleanup_timer_->
Expand All @@ -3144,10 +3145,12 @@ void DeleteBgpPeer(Peer *peer) {
client->WaitForIdle();
Agent::GetInstance()->controller()->Cleanup();
client->WaitForIdle();
XmppChannelMock *xmpp_channel = static_cast<XmppChannelMock *>
(channel->GetXmppChannel());
delete channel;
delete xmpp_channel;
if (channel) {
XmppChannelMock *xmpp_channel = static_cast<XmppChannelMock *>
(channel->GetXmppChannel());
delete channel;
delete xmpp_channel;
}
}

void FillEvpnNextHop(BgpPeer *peer, std::string vrf_name,
Expand Down

0 comments on commit 45df647

Please sign in to comment.