Clearing service instance DB State after STOP command

If Delete of service instance is received before it was marked
unusable, StopServiceInstance command is issued, which invokes
the namespace script. And immediately after this, DB state is cleared.
Clearing of DB State can result in deleting of service instance
DBEntry before namespace script completion. SIGCHILD event after
script completion looks for service instance that triggered the
task, and results in crash as the service isntance is already deleted.
To avoid crash, the DBState is cleared after processing the SIGCHILD
event.

Testcases to follow in the next commit

Change-Id: I212063ede8f7df4b693771aa394c4536a76d8d20
closes-bug: #1453956

src/vnsw/agent/oper/instance_manager.cc

@@ -219,6 +219,17 @@ void InstanceManager::OnTaskTimeout(InstanceTaskQueue *task_queue) {
 }
 
 void InstanceManager::OnTaskTimeoutEventHandler(InstanceManagerChildEvent event) {
+
+    ServiceInstance *svc_instance = GetSvcInstance(event.task);
+    if (svc_instance && svc_instance->IsDeleted()) {
+        InstanceState *state = GetState(svc_instance);
+        if (state != NULL) {
+            ClearState(svc_instance);
+            delete state;
+        }
+        UnregisterSvcInstance(event.task);
+    }
+
     ScheduleNextTask(event.task_queue);
 }
 
@@ -234,13 +245,17 @@ void InstanceManager::SigChldEventHandler(InstanceManagerChildEvent event) {
          if (!task_queue->Empty()) {
              InstanceTask *task = task_queue->Front();
              if (task->pid() == event.pid) {
+
+                 //Get the sevice instance first, to delete the state later
+                 ServiceInstance* svc_instance = GetSvcInstance(task);
                  UpdateStateStatusType(task, event.status);
 
                  task_queue->Pop();
                  delete task;
 
                  task_queue->StopTimer();
 
+                 DeleteState(svc_instance);
                  ScheduleNextTask(task_queue);
                  return;
              }
@@ -257,7 +272,13 @@ void InstanceManager::OnErrorEventHandler(InstanceManagerChildEvent event) {
     InstanceState *state = GetState(svc_instance);
     if (state != NULL) {
        state->set_errors(event.errors);
+       if (svc_instance->IsDeleted()) {
+           ClearState(svc_instance);
+           delete state;
+           UnregisterSvcInstance(event.task);
+       }
     }
+    ScheduleNextTask(event.task_queue);
 }
 
 bool InstanceManager::DequeueEvent(InstanceManagerChildEvent event) {
@@ -335,6 +356,23 @@ void InstanceManager::ClearState(ServiceInstance *svc_instance) {
     svc_instance->ClearState(agent_->service_instance_table(), si_listener_);
 }
 
+bool InstanceManager::DeleteState(ServiceInstance *svc_instance) {
+
+    if (!svc_instance || !svc_instance->IsDeleted()) {
+        return false;
+    }
+
+    InstanceState *state = GetState(svc_instance);
+    if (state && !state->tasks_running()) {
+        ClearState(svc_instance);
+        delete state;
+        ClearLastCmdType(svc_instance);
+        return true;
+    }
+
+    return false;
+}
+
 void InstanceManager::InitSigHandler(AgentSignal *signal) {
     signal->RegisterChildHandler(
         boost::bind(&InstanceManager::HandleSigChild, this, _1, _2, _3, _4));
@@ -351,6 +389,7 @@ void InstanceManager::StateClear() {
         if (state != NULL) {
             entry->ClearState(agent_->service_instance_table(), si_listener_);
             delete state;
+            ClearLastCmdType(static_cast<ServiceInstance *>(entry));
         }
         next = partition->GetNext(entry);
     }
@@ -445,11 +484,17 @@ void InstanceManager::ScheduleNextTask(InstanceTaskQueue *task_queue) {
 
             if (delay > (netns_timeout_ * 2)) {
                task->Terminate();
-
                task_queue->StopTimer();
                task_queue->Pop();
+
+               ServiceInstance* svc_instance = GetSvcInstance(task);
+               if (state && svc_instance)
+                   state->decr_tasks_running();
+
                task_svc_instances_.erase(task);
 
+               DeleteState(svc_instance);
+
                delete task;
             } else {
                task->Stop();
@@ -460,7 +505,7 @@ void InstanceManager::ScheduleNextTask(InstanceTaskQueue *task_queue) {
 }
 
 ServiceInstance *InstanceManager::GetSvcInstance(InstanceTask *task) const {
-    std::map<InstanceTask *, ServiceInstance*>::const_iterator iter =
+    TaskSvcMap::const_iterator iter =
                     task_svc_instances_.find(task);
     if (iter != task_svc_instances_.end()) {
         return iter->second;
@@ -470,15 +515,24 @@ ServiceInstance *InstanceManager::GetSvcInstance(InstanceTask *task) const {
 
 void InstanceManager::RegisterSvcInstance(InstanceTask *task,
                                           ServiceInstance *svc_instance) {
-    task_svc_instances_.insert(std::make_pair(task, svc_instance));
+    pair<TaskSvcMap::iterator, bool> result =
+         task_svc_instances_.insert(std::make_pair(task, svc_instance));
+    assert(result.second);
+
+    InstanceState *state = GetState(svc_instance);
+    assert(state);
+    state->incr_tasks_running();
 }
 
 ServiceInstance *InstanceManager::UnregisterSvcInstance(InstanceTask *task) {
-    for (std::map<InstanceTask *, ServiceInstance*>::iterator iter =
+    for (TaskSvcMap::iterator iter =
                     task_svc_instances_.begin();
          iter != task_svc_instances_.end(); ++iter) {
         if (task == iter->first) {
             ServiceInstance *svc_instance = iter->second;
+            InstanceState *state = GetState(svc_instance);
+            assert(state);
+            state->decr_tasks_running();
             task_svc_instances_.erase(iter);
             return svc_instance;
         }
@@ -488,11 +542,16 @@ ServiceInstance *InstanceManager::UnregisterSvcInstance(InstanceTask *task) {
 }
 
 void InstanceManager::UnregisterSvcInstance(ServiceInstance *svc_instance) {
-    std::map<InstanceTask *, ServiceInstance*>::iterator iter =
+
+    InstanceState *state = GetState(svc_instance);
+    assert(state);
+
+    TaskSvcMap::iterator iter =
         task_svc_instances_.begin();
     while(iter != task_svc_instances_.end()) {
         if (svc_instance == iter->second) {
             task_svc_instances_.erase(iter++);
+            state->decr_tasks_running();
         } else {
             ++iter;
         }
@@ -629,14 +688,14 @@ void InstanceManager::EventObserver(
 
     InstanceState *state = GetState(svc_instance);
     if (svc_instance->IsDeleted()) {
-        UnregisterSvcInstance(svc_instance);
         if (state) {
             if (GetLastCmdType(svc_instance) == Start) {
                 StopServiceInstance(svc_instance, state);
+                SetLastCmdType(svc_instance, Stop);
+            }
+            if (DeleteState(svc_instance)) {
+                return;
             }
-
-            ClearState(svc_instance);
-            delete state;
         }
         ClearLastCmdType(svc_instance);
     } else {
@@ -716,7 +775,7 @@ void InstanceManager::SetNamespaceStorePath(std::string path) {
  * InstanceState class
  */
 InstanceState::InstanceState() : DBState(),
-        pid_(0), status_(0), status_type_(0) {
+        pid_(0), status_(0), status_type_(0), tasks_running_(0) {
 }
 
 void InstanceState::Clear() {

src/vnsw/agent/oper/instance_manager.h

@@ -110,6 +110,7 @@ class InstanceManager {
     InstanceState *GetState(InstanceTask* task) const;
     void SetState(ServiceInstance *svc_instance, InstanceState *state);
     void ClearState(ServiceInstance *svc_instance);
+    bool DeleteState(ServiceInstance *svc_instance);
     void UpdateStateStatusType(InstanceTask* task, int status);
 
     void SetLastCmdType(ServiceInstance *svc_instance, int last_cmd_type);
@@ -144,7 +145,8 @@ class InstanceManager {
     WorkQueue<InstanceManagerChildEvent> work_queue_;
 
     std::vector<InstanceTaskQueue *> task_queues_;
-    std::map<InstanceTask *, ServiceInstance *> task_svc_instances_;
+    typedef std::map<InstanceTask *, ServiceInstance *> TaskSvcMap;
+    TaskSvcMap task_svc_instances_;
     std::map<std::string, int> last_cmd_types_;
     std::string loadbalancer_config_path_;
     std::string namespace_store_path_;
@@ -215,6 +217,21 @@ class InstanceState : public DBState {
         return status_type_;
     }
 
+    int tasks_running() const {
+        return tasks_running_;
+    }
+
+    int incr_tasks_running() {
+        return ++tasks_running_;
+    }
+
+    int decr_tasks_running() {
+        tasks_running_--;
+        assert(!(tasks_running_ < 0));
+        return tasks_running_;
+    }
+
+
     void Clear();
 
  private:
@@ -223,6 +240,7 @@ class InstanceState : public DBState {
     std::string errors_;
     std::string cmd_;
     int status_type_;
+    int tasks_running_;
 
     ServiceInstance::Properties properties_;