From 06b38bcdbfe6773adf3c41f416d5af0ebf47f7c9 Mon Sep 17 00:00:00 2001 From: Divakar Date: Sun, 31 May 2015 23:33:00 -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. Also, the config filters for missing tables, which treat the NULL uuid as node deletes is also added Change-Id: I406f9e3d70768151e4390955153bc3176d3da99f closes-bug: #1453956 --- src/vnsw/agent/cfg/cfg_filter.cc | 19 ++++++++ src/vnsw/agent/cfg/cfg_init.cc | 20 ++++++++- src/vnsw/agent/cfg/cfg_init.h | 17 +++++++ src/vnsw/agent/oper/instance_manager.cc | 59 ++++++++++++++++++++----- src/vnsw/agent/oper/instance_manager.h | 20 ++++++++- 5 files changed, 122 insertions(+), 13 deletions(-) diff --git a/src/vnsw/agent/cfg/cfg_filter.cc b/src/vnsw/agent/cfg/cfg_filter.cc index a328bbef626..716680148a6 100644 --- a/src/vnsw/agent/cfg/cfg_filter.cc +++ b/src/vnsw/agent/cfg/cfg_filter.cc @@ -81,9 +81,28 @@ void CfgFilter::Init() { agent_cfg_->cfg_acl_table()->RegisterPreFilter (boost::bind(&CfgFilter::CheckProperty, this, _1, _2, _3, AccessControlList::ID_PERMS)); + + agent_cfg_->cfg_loadbalancer_table()->RegisterPreFilter + (boost::bind(&CfgFilter::CheckProperty, this, _1, _2, _3, + LoadbalancerPool::ID_PERMS)); + + agent_cfg_->cfg_service_instance_table()->RegisterPreFilter + (boost::bind(&CfgFilter::CheckProperty, this, _1, _2, _3, + ServiceInstance::ID_PERMS)); + + agent_cfg_->cfg_security_group_table()->RegisterPreFilter + (boost::bind(&CfgFilter::CheckProperty, this, _1, _2, _3, + SecurityGroup::ID_PERMS)); + } void CfgFilter::Shutdown() { agent_cfg_->cfg_vm_table()->RegisterPreFilter(NULL); agent_cfg_->cfg_vn_table()->RegisterPreFilter(NULL); + agent_cfg_->cfg_vm_interface_table()->RegisterPreFilter(NULL); + agent_cfg_->cfg_acl_table()->RegisterPreFilter(NULL); + agent_cfg_->cfg_loadbalancer_table()->RegisterPreFilter(NULL); + agent_cfg_->cfg_service_instance_table()->RegisterPreFilter(NULL); + agent_cfg_->cfg_security_group_table()->RegisterPreFilter(NULL); + } diff --git a/src/vnsw/agent/cfg/cfg_init.cc b/src/vnsw/agent/cfg/cfg_init.cc index 321829dd93c..cac58e7db1c 100644 --- a/src/vnsw/agent/cfg/cfg_init.cc +++ b/src/vnsw/agent/cfg/cfg_init.cc @@ -123,9 +123,9 @@ void AgentConfig::RegisterDBClients(DB *db) { cfg_listener_->Register("virtual-network-network-ipam", boost::bind(&VnTable::IpamVnSync, _1), -1); cfg_listener_->Register("network-ipam", boost::bind(&DomainConfig::IpamSync, - agent_->domain_config_table(), _1), NetworkIpam::ID_PERMS); + agent_->domain_config_table(), _1), -1); cfg_listener_->Register("virtual-DNS", boost::bind(&DomainConfig::VDnsSync, - agent_->domain_config_table(), _1), VirtualDns::ID_PERMS); + agent_->domain_config_table(), _1), -1); cfg_listener_->Register ("virtual-machine-interface-routing-instance", boost::bind(&InterfaceTable::VmInterfaceVrfSync, @@ -203,6 +203,22 @@ void AgentConfig::RegisterDBClients(DB *db) { "interface-route-table"))); assert(cfg_route_table_); + cfg_loadbalancer_table_ = (static_cast + (IFMapTable::FindTable(agent_->db(), + "loadbalancer_pool"))); + assert(cfg_loadbalancer_table_); + + cfg_service_instance_table_ = (static_cast + (IFMapTable::FindTable(agent_->db(), + "service_instance"))); + assert(cfg_service_instance_table_); + + cfg_security_group_table_ = (static_cast + (IFMapTable::FindTable(agent_->db(), + "security_group"))); + assert(cfg_security_group_table_); + + cfg_interface_client_->Init(); } diff --git a/src/vnsw/agent/cfg/cfg_init.h b/src/vnsw/agent/cfg/cfg_init.h index eadaf6e8213..8ba47b676de 100644 --- a/src/vnsw/agent/cfg/cfg_init.h +++ b/src/vnsw/agent/cfg/cfg_init.h @@ -57,6 +57,18 @@ class AgentConfig { return cfg_service_template_table_; } + IFMapAgentTable *cfg_loadbalancer_table() const { + return cfg_loadbalancer_table_; + } + + IFMapAgentTable *cfg_service_instance_table() const { + return cfg_service_instance_table_; + } + + IFMapAgentTable *cfg_security_group_table() const { + return cfg_security_group_table_; + } + Agent *agent() const { return agent_; } CfgFilter *cfg_filter() const { return cfg_filter_.get(); } CfgListener *cfg_listener() const { return cfg_listener_.get(); } @@ -106,6 +118,11 @@ class AgentConfig { IFMapAgentTable *cfg_vm_port_vrf_table_; IFMapAgentTable *cfg_route_table_; IFMapAgentTable *cfg_service_template_table_; + IFMapAgentTable *cfg_loadbalancer_table_; + IFMapAgentTable *cfg_service_instance_table_; + IFMapAgentTable *cfg_security_group_table_; + + DISALLOW_COPY_AND_ASSIGN(AgentConfig); }; diff --git a/src/vnsw/agent/oper/instance_manager.cc b/src/vnsw/agent/oper/instance_manager.cc index 5e14771efd2..fa01a0b856b 100644 --- a/src/vnsw/agent/oper/instance_manager.cc +++ b/src/vnsw/agent/oper/instance_manager.cc @@ -215,13 +215,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; } @@ -316,6 +319,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)); @@ -332,6 +352,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); } @@ -420,6 +441,11 @@ void InstanceManager::ScheduleNextTask(InstanceTaskQueue *task_queue) { 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 { @@ -431,7 +457,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; @@ -441,15 +467,23 @@ 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 = - task_svc_instances_.begin(); + 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; } @@ -459,11 +493,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; } @@ -636,14 +675,14 @@ void InstanceManager::EventObserver( InstanceState *state = GetState(svc_instance); if (svc_instance->IsDeleted()) { - UnregisterSvcInstance(svc_instance); if (state) { if (GetLastCmdType(svc_instance) == Start) { StopNetNS(svc_instance, state); + SetLastCmdType(svc_instance, Stop); } - ClearState(svc_instance); - delete state; + if (DeleteState(svc_instance)) + return; } ClearLastCmdType(svc_instance); } else { @@ -723,7 +762,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 31273d8f803..acbe4e765e8 100644 --- a/src/vnsw/agent/oper/instance_manager.h +++ b/src/vnsw/agent/oper/instance_manager.h @@ -104,6 +104,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); @@ -138,7 +139,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_; @@ -207,6 +209,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: @@ -215,6 +232,7 @@ class InstanceState : public DBState { std::string errors_; std::string cmd_; int status_type_; + int tasks_running_; ServiceInstance::Properties properties_;