From 55debc26904da93ad276d4eb691f75356e81e90d Mon Sep 17 00:00:00 2001 From: Divakar Date: Fri, 19 Jun 2015 23:30:48 +0530 Subject: [PATCH] Update IFNode seq number even for Delete operation Lets say if Node A, Node B and link L1 exists between them with each entity's seq number being 1. After Xmpp channel flap we generate a new seq number for the subsequent config. After XMPP flap, if an update is receive for Node B, its seq number would be 2 with A and L1 being at seq 1. If Node A's delete is received before L1 delete, L1 would be pushed to Defer list. While pushing to defer list, the seq number of defer entry A->B is taken as 2 (as B's seq is 2) and B->A entry is taken as 1 (as A's seq is 1). This is leading to dissimilar seq number for A->B and B->A. When subsequntly add of Node A is received, while evaluating defer list, B->A is not considered as its seq is old seq leading to deletion of defer enty A->B but not B->A. This ends up in a single defer entry being present which is invalid. As a fix, when Node A's delete is received, the seq number is of A is updated before pushing its links to Defer list. While adding defer list, the seq number of Defer entry is taken from IFMapLink rather from end nodes. While accepting an update to a node in DB, and assert is added to ensure that update's seq number is same or higher than existing node. While evaluating Defer list, it is ensured that both direction entry's are removed rather a single. Change-Id: Ibb5a7469b80a596961baa3f11d52ee52ff9b1f10 closes-bug: #1465190 --- src/ifmap/ifmap_agent_table.cc | 51 +++++++--- src/vnsw/agent/test/test_cfg.cc | 165 +++++++++++++++++++++++++++++++- 2 files changed, 203 insertions(+), 13 deletions(-) diff --git a/src/ifmap/ifmap_agent_table.cc b/src/ifmap/ifmap_agent_table.cc index 3585f993d49..78993016a52 100644 --- a/src/ifmap/ifmap_agent_table.cc +++ b/src/ifmap/ifmap_agent_table.cc @@ -78,7 +78,11 @@ IFMapNode *IFMapAgentTable::EntryLocate(IFMapNode *node, RequestKey *req) { obj = node->GetObject(); assert(obj); + //We dont accept lesser sequence number updates + assert(obj->sequence_number() <= req->id_seq_num); + node->Remove(obj); + } else { auto_ptr key(AllocEntry(req)); node = const_cast( @@ -103,6 +107,8 @@ void IFMapAgentTable::HandlePendingLinks(IFMapNode *node) { assert(ltable != NULL); DBGraphVertex::adjacency_iterator iter; + bool origin_exists; + uint64_t seq; for (iter = node->begin(graph_); iter != node->end(graph_);) { right = static_cast(iter.operator->()); @@ -110,16 +116,18 @@ void IFMapAgentTable::HandlePendingLinks(IFMapNode *node) { edge = graph_->GetEdge(node, right); assert(edge); IFMapLink *l = static_cast(edge); + seq = l->sequence_number(IFMapOrigin::UNKNOWN, &origin_exists); + assert(origin_exists); // Create both the request keys auto_ptr req_key (new IFMapAgentLinkTable::RequestKey); req_key->left_key.id_name = node->name(); req_key->left_key.id_type = node->table()->Typename(); - req_key->left_key.id_seq_num = node->GetObject()->sequence_number(); + req_key->left_key.id_seq_num = seq; req_key->right_key.id_name = right->name(); req_key->right_key.id_type = right->table()->Typename(); - req_key->right_key.id_seq_num = right->GetObject()->sequence_number(); + req_key->right_key.id_seq_num = seq; req_key->metadata = l->metadata(); DBRequest req; @@ -158,6 +166,7 @@ void IFMapAgentTable::NotifyNode(IFMapNode *node) { // Process link-defer list based for the request. // If request is add, create left->right and right-left defer nodes // If request is delete, remove left->right and right->left defer nodes +// The sequence number is valid only in the DeferrendNode entry void IFMapAgentLinkTable::LinkDefAdd(DBRequest *request) { RequestKey *key = static_cast(request->key.get()); @@ -174,6 +183,7 @@ void IFMapAgentLinkTable::LinkDefAdd(DBRequest *request) { right = right_it->second; if (request->oper == DBRequest::DB_ENTRY_DELETE) { + //We need to delete the old sequence links as well // remove left->right entry if (left) { for(it = left->begin(); it != left->end(); it++) { @@ -295,6 +305,12 @@ void IFMapAgentTable::Input(DBTablePartition *partition, DBClient *client, return; } + obj = node->GetObject(); + //We dont accept lesser sequence number updates + assert(obj->sequence_number() <= key->id_seq_num); + + //Upate the sequence number even for deletion of node + obj->set_sequence_number(key->id_seq_num); DeleteNode(node); return; } @@ -308,8 +324,10 @@ void IFMapAgentTable::Input(DBTablePartition *partition, DBClient *client, //Set the sequence number of the object obj->set_sequence_number(key->id_seq_num); + node->Insert(obj); NotifyNode(node); + IFMapAgentLinkTable *link_table = static_cast( database()->FindTable(IFMAP_AGENT_LINK_DB_NAME)); link_table->EvalDefLink(key); @@ -495,11 +513,17 @@ void IFMapAgentLinkTable::EvalDefLink(IFMapTable::RequestKey *key) { continue; // Skip if right-node is not yet present - if (IFMapAgentTable::TableEntryLookup(database(), - &((*left_list_entry).node_key)) - == NULL) { + IFMapNode *node = IFMapAgentTable::TableEntryLookup(database(), + &((*left_list_entry).node_key)); + if (!node) + continue; + + //If the other end of the node is not from active control node, + //dont consider the link + IFMapObject *obj = node->GetObject(); + if (obj->sequence_number() < key->id_seq_num) continue; - } + // left->right entry found defer-list. Find the right->left entry LinkDefMap::iterator right_defmap_it = @@ -508,21 +532,24 @@ void IFMapAgentLinkTable::EvalDefLink(IFMapTable::RequestKey *key) { std::list *right_list = right_defmap_it->second; std::list::iterator right_it, right_list_entry; - for(right_it = right_list->begin(); right_it != right_list->end();) { - right_list_entry = right_it++; + for(right_it = right_list->begin(); right_it != + right_list->end(); right_it++) { // If link seq is older, dont consider the link. - if ((*right_list_entry).node_key.id_seq_num < key->id_seq_num) + if ((*right_it).node_key.id_seq_num < key->id_seq_num) continue; - if ((*right_list_entry).node_key.id_type == key->id_type && - (*right_list_entry).node_key.id_name == key->id_name) { + if ((*right_it).node_key.id_type == key->id_type && + (*right_it).node_key.id_name == key->id_name) { RemoveDefListEntry(&link_def_map_, right_defmap_it, - &right_list_entry); + &right_it); break; } } + //We should have removed something in the above iteration + assert(right_it != right_list->end()); + //Remove from deferred list before enqueing auto_ptr req_key (new RequestKey); req_key->left_key = *key; diff --git a/src/vnsw/agent/test/test_cfg.cc b/src/vnsw/agent/test/test_cfg.cc index 19bfb731614..57c698ceaf3 100644 --- a/src/vnsw/agent/test/test_cfg.cc +++ b/src/vnsw/agent/test/test_cfg.cc @@ -503,7 +503,6 @@ TEST_F(CfgTest, LinkReorderTest) { " testtest\n" " \n" ""); - IFMapTable *ftable = IFMapTable::FindTable(&db_, "foo"); ASSERT_TRUE(ftable!=NULL); @@ -2190,6 +2189,170 @@ TEST_F(CfgTest, NodeDelLinkMetadata) { EXPECT_EQ(link->metadata(), "foo-bar"); } +TEST_F(CfgTest, AssymetricLinkSeqTest) { + char buff[1500]; + sprintf(buff, + "\n" + " \n" + " \n" + " testfoo\n" + " \n" + " \n" + " testbar\n" + " \n" + " \n" + " \n" + " testfoo\n" + " \n" + " \n" + " testbar\n" + " \n" + ""); + + IFMapTable *ftable = IFMapTable::FindTable(&db_, "foo"); + ASSERT_TRUE(ftable!=NULL); + + IFMapTable *btable = IFMapTable::FindTable(&db_, "bar"); + ASSERT_TRUE(btable!=NULL); + + IFMapAgentLinkTable *ltable = static_cast( + db_.FindTable(IFMAP_AGENT_LINK_DB_NAME)); + ASSERT_TRUE(ltable!=NULL); + + //Add foo, bar and link between them with seq of 1 + pugi::xml_parse_result result = xdoc_.load(buff); + EXPECT_TRUE(result); + parser_->ConfigParse(xdoc_, 1); + WaitForIdle(); + + IFMapNode *TestFoo = ftable->FindNode("testfoo"); + ASSERT_TRUE(TestFoo !=NULL); + EXPECT_EQ("testfoo", TestFoo->name()); + + IFMapNode *TestBar = btable->FindNode("testbar"); + ASSERT_TRUE(TestBar !=NULL); + EXPECT_EQ("testbar", TestBar->name()); + + //Ensure that there is Link from Foo to Bar + for (DBGraphVertex::adjacency_iterator iter = TestFoo->begin(&graph_); + iter != TestFoo->end(&graph_); ++iter) { + TestBar = static_cast(iter.operator->()); + EXPECT_EQ("testbar", TestBar->name()); + } + + //Update only bar with seq 2 + sprintf(buff, + "\n" + " \n" + " testbar\n" + " \n" + "\n"); + result = xdoc_.load(buff); + EXPECT_TRUE(result); + parser_->ConfigParse(xdoc_, 2); + WaitForIdle(); + + TestFoo = ftable->FindNode("testfoo"); + ASSERT_TRUE(TestFoo !=NULL); + EXPECT_EQ("testfoo", TestFoo->name()); + + TestBar = btable->FindNode("testbar"); + ASSERT_TRUE(TestBar !=NULL); + EXPECT_EQ("testbar", TestBar->name()); + + //Ensure that there is Link from Foo to Bar even after seq update + for (DBGraphVertex::adjacency_iterator iter = TestFoo->begin(&graph_); + iter != TestFoo->end(&graph_); ++iter) { + TestBar = static_cast(iter.operator->()); + EXPECT_EQ("testbar", TestBar->name()); + } + + //Delete foo and ensure that bidifrectional link + //is moved to defer list + sprintf(buff, + "\n" + " \n" + " testfoo\n" + " \n" + "\n"); + result = xdoc_.load(buff); + EXPECT_TRUE(result); + parser_->ConfigParse(xdoc_, 2); + WaitForIdle(); + + + const IFMapAgentLinkTable::LinkDefMap &def_list = + ltable->GetLinkDefMap(); + EXPECT_EQ(def_list.size(), 2); + + TestFoo = ftable->FindNode("testfoo"); + ASSERT_TRUE(TestFoo == NULL); + + //Ensure that there is Link from Foo to Bar even after seq update + int cnt = 0; + for (DBGraphVertex::adjacency_iterator iter = TestBar->begin(&graph_); + iter != TestBar->end(&graph_); ++iter) { + cnt++; + } + EXPECT_EQ(cnt, 0); + + //Add the foo again but still link should not be added as + //link is of seq 1 + sprintf(buff, + "\n" + " \n" + " testfoo\n" + " \n" + "\n"); + result = xdoc_.load(buff); + EXPECT_TRUE(result); + parser_->ConfigParse(xdoc_, 2); + WaitForIdle(); + EXPECT_EQ(def_list.size(), 2); + + TestFoo = ftable->FindNode("testfoo"); + ASSERT_TRUE(TestFoo !=NULL); + EXPECT_EQ("testfoo", TestFoo->name()); + + cnt = 0; + for (DBGraphVertex::adjacency_iterator iter = TestBar->begin(&graph_); + iter != TestBar->end(&graph_); ++iter) { + cnt++; + } + EXPECT_EQ(cnt, 0); + + //Update the link with seq of 2 and ensure link is added + sprintf(buff, + "\n" + " \n" + " \n" + " testfoo\n" + " \n" + " \n" + " testbar\n" + " \n" + " \n" + ""); + result = xdoc_.load(buff); + EXPECT_TRUE(result); + parser_->ConfigParse(xdoc_, 2); + WaitForIdle(); + + for (DBGraphVertex::adjacency_iterator iter = TestFoo->begin(&graph_); + iter != TestFoo->end(&graph_); ++iter) { + TestBar = static_cast(iter.operator->()); + EXPECT_EQ("testbar", TestBar->name()); + } + //Defer list should still exist, as above link is directly added + EXPECT_EQ(def_list.size(), 2); + + //Run the stale timer and ensure defer list is cleaned + IFMapAgentStaleCleaner *cl = new IFMapAgentStaleCleaner(&db_, &graph_); + cl->StaleTimeout(2); + WaitForIdle(); + EXPECT_EQ(def_list.size(), 0); +} + int main(int argc, char **argv) { LoggingInit(); ::testing::InitGoogleTest(&argc, argv);