Merge "ToR Agent to assure OVSDB route table sanity" into R2.21.x

src/vnsw/agent/ovs_tor_agent/ovsdb_client/ha_stale_dev_vn.cc

@@ -27,26 +27,28 @@ using namespace OVSDB;
 
 HaStaleDevVnEntry::HaStaleDevVnEntry(OvsdbDBObject *table,
         const boost::uuids::uuid &vn_uuid) : OvsdbDBEntry(table),
-    vn_uuid_(vn_uuid), l2_table_(NULL), oper_bridge_table_(NULL), dev_ip_(),
-    vn_name_(""), vxlan_id_(0) {
+    vn_uuid_(vn_uuid), l2_table_(NULL), old_l2_table_(NULL),
+    oper_bridge_table_(NULL), dev_ip_(), vn_name_(""), vxlan_id_(0) {
 }
 
 HaStaleDevVnEntry::~HaStaleDevVnEntry() {
     assert(l2_table_ == NULL);
+    assert(old_l2_table_ == NULL);
 }
 
-void HaStaleDevVnEntry::AddMsg(struct ovsdb_idl_txn *txn) {
-    // if table is scheduled for delete, delete the entry
+bool HaStaleDevVnEntry::Add() {
+    // if table is scheduled for delete, return from here
+    // and wait for delete callback
     if (table_->delete_scheduled()) {
-        DeleteMsg(txn);
-        return;
+        return true;
     }
 
+    bool ret = true;
     // On device ip, vxlan id or Agent Route Table pointer change delete
     // previous route table and add new with updated vxlan id
     if (l2_table_ != NULL &&
         l2_table_->GetDBTable() != oper_bridge_table_) {
-        DeleteMsg(txn);
+        ret = Delete();
     }
 
     // create route table and register
@@ -56,18 +58,24 @@ void HaStaleDevVnEntry::AddMsg(struct ovsdb_idl_txn *txn) {
 
     // trigger update params for L2 Table to pick new vxlan id / tor ip
     l2_table_->UpdateParams(this);
+    return ret;
 }
 
-void HaStaleDevVnEntry::ChangeMsg(struct ovsdb_idl_txn *txn) {
-    AddMsg(txn);
+bool HaStaleDevVnEntry::Change() {
+    return Add();
 }
 
-void HaStaleDevVnEntry::DeleteMsg(struct ovsdb_idl_txn *txn) {
+bool HaStaleDevVnEntry::Delete() {
     // delete route table.
     if (l2_table_ != NULL) {
         l2_table_->DeleteTable();
+        assert(old_l2_table_ == NULL);
+        old_l2_table_ = l2_table_;
         l2_table_ = NULL;
+        return false;
     }
+
+    return true;
 }
 
 bool HaStaleDevVnEntry::Sync(DBEntry *db_entry) {
@@ -128,6 +136,18 @@ KSyncEntry* HaStaleDevVnEntry::UnresolvedReference() {
     return NULL;
 }
 
+void HaStaleDevVnEntry::TriggerAck(HaStaleL2RouteTable *table) {
+    OvsdbDBObject *object = static_cast<OvsdbDBObject*>(GetObject());
+    assert(old_l2_table_ == table);
+    if (l2_table_ != NULL) {
+        old_l2_table_ = NULL;
+        object->NotifyEvent(this, KSyncEntry::ADD_ACK);
+    } else {
+        old_l2_table_ = NULL;
+        object->NotifyEvent(this, KSyncEntry::DEL_ACK);
+    }
+}
+
 Agent *HaStaleDevVnEntry::agent() const {
     const HaStaleDevVnTable *reflector_table =
         static_cast<const HaStaleDevVnTable *>(table_);

src/vnsw/agent/ovs_tor_agent/ovsdb_client/ha_stale_dev_vn.h

@@ -98,15 +98,17 @@ class HaStaleDevVnEntry : public OvsdbDBEntry {
     HaStaleDevVnEntry(OvsdbDBObject *table, const boost::uuids::uuid &vn_uuid);
     ~HaStaleDevVnEntry();
 
-    void AddMsg(struct ovsdb_idl_txn *);
-    void ChangeMsg(struct ovsdb_idl_txn *);
-    void DeleteMsg(struct ovsdb_idl_txn *);
+    bool Add();
+    bool Change();
+    bool Delete();
 
     bool Sync(DBEntry*);
     bool IsLess(const KSyncEntry&) const;
     std::string ToString() const {return "Ha Stale Dev VN Entry";}
     KSyncEntry* UnresolvedReference();
 
+    void TriggerAck(HaStaleL2RouteTable *table);
+
     HaStaleL2RouteTable *l2_table() const {return l2_table_;}
     const boost::uuids::uuid &vn_uuid() const { return vn_uuid_; }
 
@@ -125,6 +127,14 @@ class HaStaleDevVnEntry : public OvsdbDBEntry {
     friend class HaStaleDevVnTable;
     boost::uuids::uuid vn_uuid_;
     HaStaleL2RouteTable *l2_table_;
+    // hold pointer to old l2_table which is already
+    // scheduled for deletion, this delete can be
+    // triggered due to delete or change of entry
+    // so we hold the ksync state to wait for an Ack
+    // on table cleanup and move the ksync state
+    // machine when HaStaleL2RouteTable triggerack
+    // at the time of table delete complete
+    HaStaleL2RouteTable *old_l2_table_;
     AgentRouteTable *oper_bridge_table_;
     IpAddress dev_ip_;
     std::string vn_name_;

src/vnsw/agent/ovs_tor_agent/ovsdb_client/ha_stale_l2_route.cc

@@ -76,8 +76,7 @@ void HaStaleL2RouteEntry::StopStaleClearTimer() {
     if (time_stamp_ != 0) {
         HaStaleL2RouteTable *table =
             static_cast<HaStaleL2RouteTable *>(table_);
-        HaStaleDevVnEntry *dev_vn =
-            static_cast<HaStaleDevVnEntry *>(table->dev_vn_ref_.get());
+        HaStaleDevVnEntry *dev_vn = table->dev_vn_;
         HaStaleDevVnTable *dev_vn_table =
             static_cast<HaStaleDevVnTable*>(dev_vn->table());
         dev_vn_table->StaleClearDelEntry(time_stamp_, this);
@@ -101,8 +100,7 @@ void HaStaleL2RouteEntry::AddEvent() {
         // donot reexport the route if path preference is less than LOW
         return;
     }
-    HaStaleDevVnEntry *dev_vn =
-        static_cast<HaStaleDevVnEntry *>(table->dev_vn_ref_.get());
+    HaStaleDevVnEntry *dev_vn = table->dev_vn_;
     vxlan_id_ = table->vxlan_id_;
     dev_vn->route_peer()->AddOvsRoute(table->vrf_.get(), table->vxlan_id_,
                                       table->vn_name_, MacAddress(mac_),
@@ -128,8 +126,7 @@ void HaStaleL2RouteEntry::ChangeEvent() {
 void HaStaleL2RouteEntry::DeleteEvent() {
     HaStaleL2RouteTable *table =
         static_cast<HaStaleL2RouteTable *>(table_);
-    HaStaleDevVnEntry *dev_vn =
-        static_cast<HaStaleDevVnEntry *>(table->dev_vn_ref_.get());
+    HaStaleDevVnEntry *dev_vn = table->dev_vn_;
     OVSDB_TRACE(Trace, std::string("withdrawing Ha Stale route vrf ") +
                 table->vrf_->GetName() + ", VxlanId " +
                 integerToString(vxlan_id_) + ", MAC " + mac_);
@@ -149,8 +146,7 @@ bool HaStaleL2RouteEntry::Sync(DBEntry *db_entry) {
             if (time_stamp_ == 0) {
                 HaStaleL2RouteTable *table =
                     static_cast<HaStaleL2RouteTable *>(table_);
-                HaStaleDevVnEntry *dev_vn =
-                    static_cast<HaStaleDevVnEntry *>(table->dev_vn_ref_.get());
+                HaStaleDevVnEntry *dev_vn = table->dev_vn_;
                 HaStaleDevVnTable *dev_vn_table =
                     static_cast<HaStaleDevVnTable*>(dev_vn->table());
                 time_stamp_ = dev_vn_table->time_stamp();
@@ -202,13 +198,9 @@ void HaStaleL2RouteEntry::StaleClearCb() {
 HaStaleL2RouteTable::HaStaleL2RouteTable(
         HaStaleDevVnEntry *dev_vn, AgentRouteTable *table) :
     OvsdbDBObject(NULL, false), table_delete_ref_(this, table->deleter()),
-    state_(dev_vn->state()), dev_ip_(dev_vn->dev_ip().to_v4()),
-    vxlan_id_(dev_vn->vxlan_id()), vrf_(table->vrf_entry()),
-    vn_name_(dev_vn->vn_name()) {
-    HaStaleDevVnTable *dev_table =
-        static_cast<HaStaleDevVnTable*>(dev_vn->table());
-    HaStaleDevVnEntry dev_key(dev_table, boost::uuids::nil_uuid());
-    dev_vn_ref_ = dev_table->GetReference(&dev_key);
+    dev_vn_(dev_vn), state_(dev_vn->state()),
+    dev_ip_(dev_vn->dev_ip().to_v4()), vxlan_id_(dev_vn->vxlan_id()),
+    vrf_(table->vrf_entry()), vn_name_(dev_vn->vn_name()) {
     route_export_queue_ = new WorkQueue<HaStaleL2RouteEntry *>(
             TaskScheduler::GetInstance()->GetTaskId("Agent::KSync"), 0,
             boost::bind(&HaStaleL2RouteTable::ProcessExportEntry, this, _1));
@@ -284,9 +276,7 @@ KSyncDBObject::DBFilterResp HaStaleL2RouteTable::OvsdbDBEntryFilter(
 }
 
 Agent *HaStaleL2RouteTable::agent() const {
-    HaStaleDevVnEntry *dev_vn =
-        static_cast<HaStaleDevVnEntry *>(dev_vn_ref_.get());
-    return dev_vn->agent();
+    return dev_vn_->agent();
 }
 
 void HaStaleL2RouteTable::ManagedDelete() {
@@ -299,6 +289,9 @@ void HaStaleL2RouteTable::EmptyTable() {
     // unregister the object if emptytable is called with
     // object being scheduled for delete
     if (delete_scheduled()) {
+        // trigger Ack for Dev Vn entry and reset to NULL
+        dev_vn_->TriggerAck(this);
+        dev_vn_ = NULL;
         KSyncObjectManager::Unregister(this);
     }
 }

src/vnsw/agent/ovs_tor_agent/ovsdb_client/ha_stale_l2_route.h

@@ -51,10 +51,8 @@ class HaStaleL2RouteTable : public OvsdbDBObject {
     bool ProcessExportEntry(HaStaleL2RouteEntry *entry);
 
     LifetimeRef<HaStaleL2RouteTable> table_delete_ref_;
-    // take reference to a dummy dev_vn entry to hold peer till
-    // route table is deleted. route object cannot hold reference
-    // to dev_vn entry creating it, as it will cause cyclic dependency
-    KSyncEntry::KSyncEntryPtr dev_vn_ref_;
+    // DevVn entry will not be deleted till we del_ack for it
+    HaStaleDevVnEntry *dev_vn_;
     ConnectionStateEntry *state_;
     Ip4Address dev_ip_;
     uint32_t vxlan_id_;

src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_object.cc

@@ -43,7 +43,8 @@ void OvsdbObject::EmptyTable(void) {
 
 OvsdbDBObject::OvsdbDBObject(OvsdbClientIdl *idl,
                              bool init_stale_entry_cleanup) : KSyncDBObject(),
-    client_idl_(idl), walkid_(DBTableWalker::kInvalidWalkerId) {
+    client_idl_(idl), walkid_(DBTableWalker::kInvalidWalkerId),
+    delete_triggered_(false) {
     if (init_stale_entry_cleanup) {
         InitStaleEntryCleanup(*(idl->agent()->event_manager())->io_service(),
                               StaleEntryCleanupTimer, StaleEntryYeildTimer,
@@ -54,7 +55,7 @@ OvsdbDBObject::OvsdbDBObject(OvsdbClientIdl *idl,
 OvsdbDBObject::OvsdbDBObject(OvsdbClientIdl *idl, DBTable *tbl,
                              bool init_stale_entry_cleanup) :
     KSyncDBObject(tbl), client_idl_(idl),
-    walkid_(DBTableWalker::kInvalidWalkerId) {
+    walkid_(DBTableWalker::kInvalidWalkerId), delete_triggered_(false) {
     DBTableWalker *walker = client_idl_->agent()->db()->GetWalker();
     // Start a walker to get the entries which were already present,
     // when we register to the DB Table
@@ -108,6 +109,11 @@ void OvsdbDBObject::NotifyAddOvsdb(OvsdbDBEntry *key, struct ovsdb_idl_row *row)
         del_entry->NotifyAdd(row);
         // entry created by AllocOvsEntry should always be stale
         assert(del_entry->stale());
+        // if delete of table is already triggered delete the created
+        // stale entry immediately
+        if (delete_triggered_) {
+            Delete(del_entry);
+        }
     }
 }
 
@@ -140,6 +146,9 @@ Agent *OvsdbDBObject::agent() const {
 }
 
 void OvsdbDBObject::DeleteTable(void) {
+    // validate delete should not be triggered twice for an object
+    assert(!delete_triggered_);
+    delete_triggered_ = true;
     if (client_idl_ != NULL) {
         client_idl_->ksync_obj_manager()->Delete(this);
     } else {

src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_object.h

@@ -86,6 +86,7 @@ class OvsdbDBObject : public KSyncDBObject {
 private:
     friend class OvsdbDBEntry;
     DBTableWalker::WalkId walkid_;
+    bool delete_triggered_;
     DISALLOW_COPY_AND_ASSIGN(OvsdbDBObject);
 };
 };

src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_remote_ovsdb.cc

@@ -367,15 +367,9 @@ void UnicastMacRemoteEntry::ReleaseLocatorCreateReference() {
 }
 
 UnicastMacRemoteTable::UnicastMacRemoteTable(OvsdbClientIdl *idl,
-        const std::string &logical_switch_name) :
+        const std::string &logical_switch_name, VrfOvsdbEntry *vrf) :
     OvsdbDBObject(idl, true), logical_switch_name_(logical_switch_name),
-    table_delete_ref_(this, NULL) {
-}
-
-UnicastMacRemoteTable::UnicastMacRemoteTable(OvsdbClientIdl *idl,
-        AgentRouteTable *table, const std::string &logical_switch_name) :
-    OvsdbDBObject(idl, table, true), logical_switch_name_(logical_switch_name),
-    table_delete_ref_(this, table->deleter()) {
+    table_delete_ref_(this, NULL), vrf_(vrf) {
 }
 
 UnicastMacRemoteTable::~UnicastMacRemoteTable() {
@@ -463,6 +457,9 @@ void UnicastMacRemoteTable::EmptyTable() {
     // unregister the object if emptytable is called with
     // object being scheduled for delete
     if (delete_scheduled()) {
+        // trigger Ack for Vrf entry and reset to NULL
+        vrf_->TriggerAck(this);
+        vrf_ = NULL;
         KSyncObjectManager::Unregister(this);
     }
 }

src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_remote_ovsdb.h

@@ -14,13 +14,13 @@ class BridgeRouteEntry;
 
 namespace OVSDB {
 class VrfOvsdbObject;
+class VrfOvsdbEntry;
 
 class UnicastMacRemoteTable : public OvsdbDBObject {
 public:
     UnicastMacRemoteTable(OvsdbClientIdl *idl,
-                          const std::string &logical_switch_name);
-    UnicastMacRemoteTable(OvsdbClientIdl *idl, AgentRouteTable *table,
-                          const std::string &logical_switch_name);
+                          const std::string &logical_switch_name,
+                          VrfOvsdbEntry *vrf);
     virtual ~UnicastMacRemoteTable();
 
     virtual void OvsdbRegisterDBTable(DBTable *tbl);
@@ -42,6 +42,7 @@ class UnicastMacRemoteTable : public OvsdbDBObject {
 private:
     std::string logical_switch_name_;
     LifetimeRef<UnicastMacRemoteTable> table_delete_ref_;
+    VrfOvsdbEntry *vrf_;
     DISALLOW_COPY_AND_ASSIGN(UnicastMacRemoteTable);
 };
 

src/vnsw/agent/ovs_tor_agent/ovsdb_client/vrf_ovsdb.cc

@@ -35,35 +35,44 @@ VrfOvsdbEntry::~VrfOvsdbEntry() {
     assert(route_table_ == NULL);
 }
 
-void VrfOvsdbEntry::AddMsg(struct ovsdb_idl_txn *txn) {
-    // if table is scheduled for delete, delete the entry
+bool VrfOvsdbEntry::Add() {
+    // if table is scheduled for delete, return from here
+    // and wait for delete callback
     if (table_->delete_scheduled()) {
-        DeleteMsg(txn);
-        return;
+        return true;
     }
+
     // create route table and register
     if (route_table_ == NULL) {
         route_table_ = new UnicastMacRemoteTable(table_->client_idl(),
-                                                 logical_switch_name_);
+                                                 logical_switch_name_, this);
     }
 
     if (!stale() && route_table_->GetDBTable() == NULL &&
         oper_route_table_ != NULL) {
-        // TODO register for route table.
         route_table_->OvsdbRegisterDBTable(oper_route_table_);
     }
+    return true;
 }
 
-void VrfOvsdbEntry::ChangeMsg(struct ovsdb_idl_txn *txn) {
-    AddMsg(txn);
+bool VrfOvsdbEntry::Change() {
+    return Add();
 }
 
-void VrfOvsdbEntry::DeleteMsg(struct ovsdb_idl_txn *txn) {
+bool VrfOvsdbEntry::Delete() {
     // delete route table.
     if (route_table_ != NULL) {
         route_table_->DeleteTable();
-        route_table_ = NULL;
+        // while triggering a delete for UnicastMacRemoteTable, wait
+        // for table cleanup to complete to proceed with the KSync
+        // state machine, so that we maintain consistency for remote
+        // MAC table by not allowing two transient route table, one
+        // in process of deletion and other in process of addition
+        // which can lead to inconsistent state in OVSDB-server
+        return false;
     }
+
+    return true;
 }
 
 bool VrfOvsdbEntry::Sync(DBEntry *db_entry) {
@@ -87,6 +96,13 @@ KSyncEntry* VrfOvsdbEntry::UnresolvedReference() {
     return NULL;
 }
 
+void VrfOvsdbEntry::TriggerAck(UnicastMacRemoteTable *table) {
+    OvsdbDBObject *object = static_cast<OvsdbDBObject*>(GetObject());
+    assert(route_table_ == table);
+    route_table_ = NULL;
+    object->NotifyEvent(this, KSyncEntry::DEL_ACK);
+}
+
 VrfOvsdbObject::VrfOvsdbObject(OvsdbClientIdl *idl) : OvsdbDBObject(idl, true) {
     client_idl_->Register(OvsdbClientIdl::OVSDB_UCAST_MAC_REMOTE,
             boost::bind(&VrfOvsdbObject::OvsdbNotify, this, _1, _2));