Comment 3 for bug 1454380

Revision history for this message
Tapan Karwa (tkarwa) wrote :

This core seems to be a simple case of a link getting revived. Consider a link add, followed by delete, followed by another add.

When we delete the link, we will unlink it from its nodes, remove it from the graph and mark it deleted.
When the exporter gets the delete, he will call
 state->RemoveDependency();
 state->ClearValid();
 EnqueueDelete(link, state);

Now, consider that before the update_list is drained, we get a second link add. So, state is non-NULL and state->update_list() is not empty. But, the dependencies are gone and valid has been cleared.

The ifmap_server_table code will do this when it gets the second add:

54 string link_name = LinkKey(metadata, left, right); << will create the name
55 IFMapLink *link = FindLink(link_name); << will find it since its still in the link table
56 if (link) {
57 assert(link->IsDeleted());
58 link->ClearDelete(); << revive the link
59 link->set_last_change_at_to_now();
60 partition->Change(link);
61 }

Note, at this point, state exists. But, the state's dependencies are gone.

Now, when the exporter gets this add,

 IFMapLinkState *state = static_cast<IFMapLinkState *>(entry_state);
 ….
 if (state == NULL) {
           state = new IFMapLinkState(link);
           entry->SetState(table, tinfo->id(), state);
           s_left = NodeStateLocate(link->left());
           s_right = NodeStateLocate(link->right());
           add_link = true;
       } else {
           assert(state->HasDependency()); <<< asserts here….
           s_left = state->left();
           s_right = state->right();
       }

The reason we are seeing this problem now is because we recently changed the code in IFMapLinkTable::AddLink() to fix another issue and that fix is invalidating the assert in the exporter.

Consider that the second add. We won't find glink below since its not in the graph.

346 IFMapLink *glink =
347 static_cast<IFMapLink *>(graph_->GetEdge(first, second));
348 if (glink == NULL) {
349 DBGraph::Edge edge = graph_->Link(first, second);
350 LinkNodeAdd(edge, first, second, data->metadata,
351 key->id_seq_num, data->origin);
352 }

Then, in the older code, we would call FindLink() with the new edge. FindLink() will not find it since the edge is a new edge. It will go do the 'else'.

57 IFMapLink *link = FindLink(edge); <<< earlier we would use the edge to do the lookup
56 if (link) { <<< we would NOT find it since the link has been removed from the graph
57 assert(link->IsDeleted());
58 link->ClearDelete();
59 link->set_last_change_at_to_now();
60 partition->Change(link);
61 } else {
62 link = new IFMapLink(link_name); <<< earlier we would do this
63 partition->Add(link);
64 }

In the new code, FindLink() will find the link since its still in the partition and we are looking up by name. We will go do the 'if' which will finally end up with the exporter receiving a change and asserting.

54 string link_name = LinkKey(metadata, left, right); << will create the name
55 IFMapLink *link = FindLink(link_name); << will find it since its still in the link table
56 if (link) {
57 assert(link->IsDeleted());
58 link->ClearDelete(); << revive the node
59 link->set_last_change_at_to_now();
60 partition->Change(link);
61 }