Handling service-instance reuse

When the neutron router is added and deleted continuously in a loop, the
service instance object in Agent is not properly handled. When service
instance is delete marked and re-add appears again, DB table invokes
OnChange on the DBEntry rather Add. Service instance code is not
handling this case and IFMapNode is set in DBEntry only as part
of ADD. This is leading to stale IFMapNode entry in DBEntry and leading
to crash when IFMap node graph is invoked as part of OnChange.

As a fix, even in OnChange, IFMapNode is set in DBEntry. As
service_instance is taking a intrusive pointer to IFMapNode, redundatnt
node_ member is removed from the object.

Also service_instance_test has been moved out of falky test.

closes-bug: #1473597
closes-bug: #1474273
partial-bug: #1465413

Conflicts:

	src/vnsw/agent/oper/service_instance.h

Change-Id: I56880a0d2b82f310c351ba292b7a54098b6ea880

src/vnsw/agent/oper/service_instance.cc

@@ -626,11 +626,13 @@ void ServiceInstance::CalculateProperties(
     DBGraph *graph, Properties *properties) {
     properties->Clear();
 
-    if (node_->IsDeleted()) {
+    IFMapNode *node = ifmap_node();
+
+    if (node->IsDeleted()) {
         return;
     }
 
-    FindAndSetTypes(graph, node_, properties);
+    FindAndSetTypes(graph, node, properties);
 
     /*
      * The vrouter agent is only interest in the properties of service
@@ -640,17 +642,17 @@ void ServiceInstance::CalculateProperties(
         return;
     }
 
-    IFMapNode *vm_node = FindAndSetVirtualMachine(graph, node_, properties);
+    IFMapNode *vm_node = FindAndSetVirtualMachine(graph, node, properties);
     if (vm_node == NULL) {
         return;
     }
 
     autogen::ServiceInstance *svc_instance =
-                 static_cast<autogen::ServiceInstance *>(node_->GetObject());
+                 static_cast<autogen::ServiceInstance *>(node->GetObject());
     FindAndSetInterfaces(graph, vm_node, svc_instance, properties);
 
     if (properties->service_type == LoadBalancer) {
-        FindAndSetLoadbalancer(graph, node_, properties);
+        FindAndSetLoadbalancer(graph, node, properties);
     }
 }
 
@@ -681,25 +683,46 @@ std::auto_ptr<DBEntry> ServiceInstanceTable::AllocEntry(
     return entry;
 }
 
-DBEntry *ServiceInstanceTable::Add(const DBRequest *request) {
-    ServiceInstance *svc_instance = new ServiceInstance();
-    svc_instance->SetKey(request->key.get());
+bool ServiceInstanceTable::HandleAddChange(ServiceInstance
+        *svc_instance, const DBRequest *request) {
+
+    if (!svc_instance || svc_instance->ifmap_node())
+        return false;
+
     ServiceInstanceCreate *data =
             static_cast<ServiceInstanceCreate *>(request->data.get());
-    svc_instance->set_node(data->node());
+    if (!data)
+        return false;
+
+    svc_instance->SetKey(request->key.get());
     assert(dependency_manager_);
     svc_instance->SetIFMapNodeState
         (dependency_manager_->SetState(data->node()));
     dependency_manager_->SetObject(data->node(), svc_instance);
 
+    ServiceInstance::Properties properties;
+    properties.Clear();
+    assert(graph_);
+    svc_instance->CalculateProperties(graph_, &properties);
+    svc_instance->set_properties(properties);
+    return true;
+}
+
+
+DBEntry *ServiceInstanceTable::Add(const DBRequest *request) {
+    ServiceInstance *svc_instance = new ServiceInstance();
+
+    assert(HandleAddChange(svc_instance, request));
     return svc_instance;
 }
 
 bool ServiceInstanceTable::Delete(DBEntry *entry, const DBRequest *request) {
     ServiceInstance *svc_instance  = static_cast<ServiceInstance *>(entry);
     assert(dependency_manager_);
-    dependency_manager_->SetObject(svc_instance->node(), NULL);
-    svc_instance->SetIFMapNodeState(NULL);
+    if (svc_instance->ifmap_node()) {
+        dependency_manager_->SetObject(svc_instance->ifmap_node(), NULL);
+        svc_instance->SetIFMapNodeState(NULL);
+    }
     return true;
 }
 
@@ -713,12 +736,8 @@ bool ServiceInstanceTable::OnChange(DBEntry *entry, const DBRequest *request) {
         ServiceInstanceUpdate *data =
                 static_cast<ServiceInstanceUpdate *>(request->data.get());
         svc_instance->set_properties(data->properties());
-    } else if (dynamic_cast<ServiceInstanceCreate*>(request->data.get()) != NULL) {
-        ServiceInstance::Properties properties;
-        properties.Clear();
-        assert(graph_);
-        svc_instance->CalculateProperties(graph_, &properties);
-        svc_instance->set_properties(properties);
+    } else {
+        HandleAddChange(svc_instance, request);
     }
     return true;
 }
@@ -766,7 +785,7 @@ void ServiceInstanceTable::ChangeEventHandler(IFMapNode *node, DBEntry *entry) {
      * Do not enqueue an ADD_CHANGE operation after the DELETE generated
      * by IFNodeToReq.
      */
-    if (svc_instance->node()->IsDeleted()) {
+    if (svc_instance->ifmap_node()->IsDeleted()) {
         return;
     }
 

src/vnsw/agent/oper/service_instance.h

@@ -109,10 +109,6 @@ class ServiceInstance : public AgentRefCount<ServiceInstance>,
      */
     void CalculateProperties(DBGraph *graph, Properties *properties);
 
-    void set_node(IFMapNode *node) { node_ = node; }
-
-    IFMapNode *node() { return node_; }
-
     void set_properties(const Properties &properties) {
         properties_ = properties;
     }
@@ -127,9 +123,15 @@ class ServiceInstance : public AgentRefCount<ServiceInstance>,
         ifmap_node_state_ref_ = ref;
     }
 
+    IFMapNode *ifmap_node() {
+        if (!ifmap_node_state_ref_)
+            return NULL;
+        IFMapNodeState *state = ifmap_node_state_ref_.get();
+        return state->node();
+    }
+
 private:
     boost::uuids::uuid uuid_;
-    IFMapNode *node_;
     Properties properties_;
     IFMapDependencyManager::IFMapNodePtr ifmap_node_state_ref_;
 
@@ -175,6 +177,7 @@ class ServiceInstanceTable : public AgentDBTable {
      * the dependency manager directly.
      */
     void ChangeEventHandler(IFMapNode *node, DBEntry *entry);
+    bool HandleAddChange(ServiceInstance *svc_instance, const DBRequest *key);
 
     DBGraph *graph_;
     IFMapDependencyManager *dependency_manager_;

src/vnsw/agent/test/SConscript

@@ -120,7 +120,7 @@ test_agent_route_walker = AgentEnv.MakeTestCmd(env, 'test_agent_route_walker',
 test_xmpp_hv = AgentEnv.MakeTestCmd(env, 'test_xmpp_hv', flaky_agent_suite)
 test_scale_walk = AgentEnv.MakeTestCmd(env, 'test_scale_walk', flaky_agent_suite)
 service_instance_test = AgentEnv.MakeTestCmd(env, 'service_instance_test',
-                                             flaky_agent_suite)
+                                             agent_suite)
 
 flaky_test = env.TestSuite('agent-flaky-test', flaky_agent_suite)
 env.Alias('controller/src/vnsw/agent:flaky_test', flaky_test)