From abdb26d0b61b759089b3572648394d16413e06ee Mon Sep 17 00:00:00 2001 From: Divakar Date: Wed, 27 May 2015 20:59:31 -0700 Subject: [PATCH] Clearing service instance DB State after STOP command If Delete of service instance is received before it was marked unusable, to stop the namespace, StopServiceInstance command is issued, which invokes the deletion of 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. Also corresponding to the service instance, there can be many tasks queued for execution. To identify how any SIGCHILDS to wait for, running tasks cound is maintained in DBState. This could is manipulated everytime the task is created, destroyed, error handled corresponding to the service instance. Testcases to follow in the next commit Change-Id: I4b9853de271b8ee54016bb91b7a2e199cbce6b0d closes-bug: #1449166 closes-bug: #1453956 --- src/vnsw/agent/oper/instance_manager.cc | 52 ++++++++++++++++++++++--- src/vnsw/agent/oper/instance_manager.h | 17 ++++++++ 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/src/vnsw/agent/oper/instance_manager.cc b/src/vnsw/agent/oper/instance_manager.cc index d7c591cf145..fcf45d942fc 100644 --- a/src/vnsw/agent/oper/instance_manager.cc +++ b/src/vnsw/agent/oper/instance_manager.cc @@ -234,13 +234,16 @@ 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; } @@ -258,6 +261,7 @@ void InstanceManager::OnErrorEventHandler(InstanceManagerChildEvent event) { if (state != NULL) { state->set_errors(event.errors); } + ScheduleNextTask(event.task_queue); } bool InstanceManager::DequeueEvent(InstanceManagerChildEvent event) { @@ -335,6 +339,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 +372,7 @@ void InstanceManager::StateClear() { if (state != NULL) { entry->ClearState(agent_->service_instance_table(), si_listener_); delete state; + ClearLastCmdType(static_cast(entry)); } next = partition->GetNext(entry); } @@ -445,11 +467,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(); @@ -471,6 +499,9 @@ ServiceInstance *InstanceManager::GetSvcInstance(InstanceTask *task) const { void InstanceManager::RegisterSvcInstance(InstanceTask *task, ServiceInstance *svc_instance) { task_svc_instances_.insert(std::make_pair(task, svc_instance)); + InstanceState *state = GetState(svc_instance); + assert(state); + state->incr_tasks_running(); } ServiceInstance *InstanceManager::UnregisterSvcInstance(InstanceTask *task) { @@ -479,6 +510,9 @@ ServiceInstance *InstanceManager::UnregisterSvcInstance(InstanceTask *task) { 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 +522,16 @@ ServiceInstance *InstanceManager::UnregisterSvcInstance(InstanceTask *task) { } void InstanceManager::UnregisterSvcInstance(ServiceInstance *svc_instance) { + + InstanceState *state = GetState(svc_instance); + assert(state); + std::map::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 +668,15 @@ 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); } - ClearState(svc_instance); - delete state; + if (DeleteState(svc_instance)) { + return; + } } ClearLastCmdType(svc_instance); } else { @@ -717,7 +757,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() { diff --git a/src/vnsw/agent/oper/instance_manager.h b/src/vnsw/agent/oper/instance_manager.h index 035142b349a..d587695d449 100644 --- a/src/vnsw/agent/oper/instance_manager.h +++ b/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); @@ -215,6 +216,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 +239,7 @@ class InstanceState : public DBState { std::string errors_; std::string cmd_; int status_type_; + int tasks_running_; ServiceInstance::Properties properties_;