From 080dda0c6af5eb172e7690854326ba230d349388 Mon Sep 17 00:00:00 2001 From: Divakar Date: Tue, 26 May 2015 21:40:44 -0700 Subject: [PATCH] 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 | 79 +++++++++++++++++++++---- src/vnsw/agent/oper/instance_manager.h | 20 ++++++- 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/src/vnsw/agent/oper/instance_manager.cc b/src/vnsw/agent/oper/instance_manager.cc index 9195f1b2cc0..e66bb8b8395 100644 --- a/src/vnsw/agent/oper/instance_manager.cc +++ b/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,6 +245,9 @@ 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(); @@ -241,6 +255,7 @@ void InstanceManager::SigChldEventHandler(InstanceManagerChildEvent event) { 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(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::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 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::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::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() { diff --git a/src/vnsw/agent/oper/instance_manager.h b/src/vnsw/agent/oper/instance_manager.h index f4e034c5fcc..ad43ae26b7a 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); @@ -144,7 +145,8 @@ class InstanceManager { WorkQueue work_queue_; std::vector task_queues_; - std::map task_svc_instances_; + typedef std::map TaskSvcMap; + TaskSvcMap task_svc_instances_; std::map 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_;