From fed0d74f972857fd1e9d41f1ea33794b81631fab Mon Sep 17 00:00:00 2001 From: Prabhjot Singh Sethi Date: Thu, 12 Feb 2015 00:08:47 -0800 Subject: [PATCH] Fix Tor-Agent crash for VRF Delete timeout Issue: ------ when a logical interface is associated with multiple VMIs and administrator removes one VMI from the list associated it results in deletion of existing VRF and re-addition again. By this time if there are any routes imported from TOR, they hold the reference to VRF and doesnot allow VRF to clean up, which results in a delete timeout. In usual scenarios VRF is deleted along with VN, so we didnot observe this issue earlier. Fix: ---- Maintain a VRF dependency list in UnicastMacLocalOvsdb table and trigger re-eval of entries on VRF delete. which removes the OvsdbEntry for unicast mac local and re-adds resulting in removing the VRF reference and move the entry to add-defer state to wait for new VRF object. This trigger of re-eval is triggered in a work queue to ensure the order of events such that re-eval kicks in after the vn_ovsdb_entry becomes in-active to hold the re-addition of route in Add defer state. Also adding fix for marking a logical entry as incomplete if logical interface to vmi association is removed to allow completion of logical switch delete. Change-Id: Ifdcd06266b1eeefb36a4fcd90f3f0d0e2e471527 Closes-Bug: 1420903 --- .../ovsdb_client/ovsdb_client_idl.cc | 13 +++++++ .../ovsdb_client/ovsdb_client_idl.h | 3 ++ .../ovsdb_client/unicast_mac_local_ovsdb.cc | 39 ++++++++++++++++++- .../ovsdb_client/unicast_mac_local_ovsdb.h | 16 ++++++-- .../ovsdb_client/vlan_port_binding_ovsdb.cc | 9 +++++ .../ovs_tor_agent/ovsdb_client/vn_ovsdb.cc | 14 ++++++- .../ovs_tor_agent/ovsdb_client/vn_ovsdb.h | 1 + 7 files changed, 90 insertions(+), 5 deletions(-) diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc index 30443744570..a35c5c872f8 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc @@ -219,6 +219,19 @@ void OvsdbClientIdl::DeleteTxn(struct ovsdb_idl_txn *txn) { ovsdb_wrapper_idl_txn_destroy(txn); } +// API to trigger ovs row del followed by add +// used by OvsdbEntry on catastrophic change event, which +// results in emulating a delete followed by add +void OvsdbClientIdl::NotifyDelAdd(struct ovsdb_idl_row *row) { + int i = ovsdb_wrapper_row_type(row); + if (i >= OvsdbClientIdl::OVSDB_TYPE_COUNT) + return; + if (callback_[i] != NULL) { + callback_[i](OvsdbClientIdl::OVSDB_DEL, row); + callback_[i](OvsdbClientIdl::OVSDB_ADD, row); + } +} + Ip4Address OvsdbClientIdl::tsn_ip() { return session_->tsn_ip(); } diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h index 31b2926c5af..4896ac99e3a 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h @@ -86,6 +86,9 @@ class OvsdbClientIdl { void DeleteTxn(struct ovsdb_idl_txn *txn); void Register(EntryType type, NotifyCB cb) {callback_[type] = cb;} void UnRegister(EntryType type) {callback_[type] = NULL;} + + // Notify Delete followed by add for a given ovsdb_idl_row + void NotifyDelAdd(struct ovsdb_idl_row *row); // Get TOR Service Node IP Ip4Address tsn_ip(); diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.cc index b09324e4df1..7d31a1c4174 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.cc +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.cc @@ -7,6 +7,7 @@ extern "C" { }; #include #include +#include #include #include #include @@ -55,7 +56,10 @@ bool UnicastMacLocalEntry::Add() { static_cast(l_table->Find(&l_key)); const PhysicalDeviceVn *dev_vn = static_cast(ls_entry->GetDBEntry()); + // Take vrf reference to genrate withdraw/delete route request vrf_ = dev_vn->vn()->GetVrf(); + // Add vrf dep in UnicastMacLocalOvsdb + table->vrf_dep_list_.insert(UnicastMacLocalOvsdb::VrfDepEntry(vrf_.get(), this)); vxlan_id_ = dev_vn->vxlan_id(); boost::system::error_code err; Ip4Address dest = Ip4Address::from_string(dest_ip_, err); @@ -67,7 +71,10 @@ bool UnicastMacLocalEntry::Delete() { UnicastMacLocalOvsdb *table = static_cast(table_); OVSDB_TRACE(Trace, "Deleting Route " + mac_ + " VN uuid " + logical_switch_name_ + " destination IP " + dest_ip_); - table->peer()->DeleteOvsRoute(vrf_, vxlan_id_, MacAddress(mac_)); + table->vrf_dep_list_.erase(UnicastMacLocalOvsdb::VrfDepEntry(vrf_.get(), this)); + table->peer()->DeleteOvsRoute(vrf_.get(), vxlan_id_, MacAddress(mac_)); + // remove vrf reference after deleting route + vrf_ = NULL; return true; } @@ -118,11 +125,16 @@ const std::string &UnicastMacLocalEntry::dest_ip() const { UnicastMacLocalOvsdb::UnicastMacLocalOvsdb(OvsdbClientIdl *idl, OvsPeer *peer) : OvsdbObject(idl), peer_(peer) { + vrf_reeval_queue_ = new WorkQueue( + TaskScheduler::GetInstance()->GetTaskId("Agent::KSync"), 0, + boost::bind(&UnicastMacLocalOvsdb::VrfReEval, this, _1)); idl->Register(OvsdbClientIdl::OVSDB_UCAST_MAC_LOCAL, boost::bind(&UnicastMacLocalOvsdb::Notify, this, _1, _2)); } UnicastMacLocalOvsdb::~UnicastMacLocalOvsdb() { + vrf_reeval_queue_->Shutdown(); + delete vrf_reeval_queue_; } OvsPeer *UnicastMacLocalOvsdb::peer() { @@ -147,11 +159,13 @@ void UnicastMacLocalOvsdb::Notify(OvsdbClientIdl::Op op, /* trigger delete if dest ip is not available */ if (op == OvsdbClientIdl::OVSDB_DEL || dest_ip == NULL) { if (entry != NULL) { + entry->ovs_entry_ = NULL; Delete(entry); } } else if (op == OvsdbClientIdl::OVSDB_ADD) { if (entry == NULL) { entry = static_cast(Create(&key)); + entry->ovs_entry_ = row; } } else { assert(0); @@ -165,6 +179,29 @@ KSyncEntry *UnicastMacLocalOvsdb::Alloc(const KSyncEntry *key, uint32_t index) { return entry; } +void UnicastMacLocalOvsdb::VrfReEvalEnqueue(VrfEntry *vrf) { + vrf_reeval_queue_->Enqueue(vrf); +} + +bool UnicastMacLocalOvsdb::VrfReEval(VrfEntryRef vrf) { + // iterate through dependency list and trigger del and add + VrfDepList::iterator it = + vrf_dep_list_.upper_bound(VrfDepEntry(vrf.get(), NULL)); + while (it != vrf_dep_list_.end()) { + if (it->first != vrf.get()) { + break; + } + UnicastMacLocalEntry *u_entry = it->second; + it++; + if (u_entry->ovs_entry() != NULL && client_idl() != NULL) { + // vrf re-eval for unicast mac local is a catastrophic change + // trigger NotifyAddDel on ovs row. + client_idl()->NotifyDelAdd(u_entry->ovs_entry()); + } + } + return true; +} + ///////////////////////////////////////////////////////////////////////////// // Sandesh routines ///////////////////////////////////////////////////////////////////////////// diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.h b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.h index d26591859b4..d89592211a3 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.h +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_local_ovsdb.h @@ -10,8 +10,12 @@ class OvsPeer; namespace OVSDB { +class UnicastMacLocalEntry; class UnicastMacLocalOvsdb : public OvsdbObject { public: + typedef std::pair VrfDepEntry; + typedef std::set VrfDepList; + UnicastMacLocalOvsdb(OvsdbClientIdl *idl, OvsPeer *peer); ~UnicastMacLocalOvsdb(); @@ -19,8 +23,14 @@ class UnicastMacLocalOvsdb : public OvsdbObject { void Notify(OvsdbClientIdl::Op op, struct ovsdb_idl_row *row); KSyncEntry *Alloc(const KSyncEntry *key, uint32_t index); + void VrfReEvalEnqueue(VrfEntry *vrf); + bool VrfReEval(VrfEntryRef vrf); + private: + friend class UnicastMacLocalEntry; OvsPeer *peer_; + VrfDepList vrf_dep_list_; + WorkQueue *vrf_reeval_queue_; DISALLOW_COPY_AND_ASSIGN(UnicastMacLocalOvsdb); }; @@ -47,9 +57,9 @@ class UnicastMacLocalEntry : public OvsdbEntry { std::string mac_; std::string logical_switch_name_; std::string dest_ip_; - // we don't need to hold reference to vrf since this pointer is valid, - // only once route is exported and will become invalid once its removed. - VrfEntry *vrf_; + // take reference to the vrf while exporting route, to assure sanity + // of vrf pointer even if Add route request fails, due to any reason + VrfEntryRef vrf_; uint32_t vxlan_id_; DISALLOW_COPY_AND_ASSIGN(UnicastMacLocalEntry); }; diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vlan_port_binding_ovsdb.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vlan_port_binding_ovsdb.cc index 7e64bbf0b50..9a333c1d3d0 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vlan_port_binding_ovsdb.cc +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vlan_port_binding_ovsdb.cc @@ -254,6 +254,15 @@ KSyncDBObject::DBFilterResp VlanPortBindingTable::DBEntryFilter( // Ignore entries other than VLanLogicalInterface. return DBFilterIgnore; } + + // Logical interface without vm interface is incomplete entry + // for ovsdb, trigger delete. + if (l_port->vm_interface() == NULL) { + OVSDB_TRACE(Trace, "VM Interface Unavialable, Deleting Logical " + "Port " + l_port->name()); + return DBFilterDelete; + } + // Since we need physical port name and device name as key, ignore entry // if physical port or device is not yet present. RemotePhysicalInterface *phy_intf = dynamic_cast diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.cc index 8a2332b4029..35a0d74cd6e 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.cc +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.cc @@ -5,11 +5,14 @@ extern "C" { #include }; -#include #include +#include #include +#include +#include + using OVSDB::OvsdbDBEntry; using OVSDB::VnOvsdbEntry; using OVSDB::VnOvsdbObject; @@ -19,12 +22,21 @@ VnOvsdbEntry::VnOvsdbEntry(VnOvsdbObject *table, } void VnOvsdbEntry::AddMsg(struct ovsdb_idl_txn *txn) { + VnEntry *vn = static_cast(GetDBEntry()); + vrf_ = vn->GetVrf(); } void VnOvsdbEntry::ChangeMsg(struct ovsdb_idl_txn *txn) { } void VnOvsdbEntry::DeleteMsg(struct ovsdb_idl_txn *txn) { + UnicastMacLocalOvsdb *uc_obj = + table_->client_idl()->unicast_mac_local_ovsdb(); + // Entries in Unicast Mac Local Table are dependent on vn/vrf + // on delete of this entry trigger Vrf re-eval for entries in + // Unicast Mac Local Table + uc_obj->VrfReEvalEnqueue(vrf_.get()); + vrf_ = NULL; } bool VnOvsdbEntry::Sync(DBEntry *db_entry) { diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.h b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.h index ce756fe8b4e..bc6039db95d 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.h +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vn_ovsdb.h @@ -44,6 +44,7 @@ class VnOvsdbEntry : public OvsdbDBEntry { private: friend class VnOvsdbObject; boost::uuids::uuid uuid_; + VrfEntryRef vrf_; DISALLOW_COPY_AND_ASSIGN(VnOvsdbEntry); }; };