From 43c5719f224b5ba72d4bc93fdf02abc1aba7657b Mon Sep 17 00:00:00 2001 From: Rio Zhu Date: Fri, 26 Mar 2021 20:22:43 +0000 Subject: [PATCH 1/6] Optimized GRPC channel status checking logic;\nadded temporary fix for dhcp operation type CREATE/UPDATE --- src/comm/aca_grpc.cpp | 6 ++- src/dhcp/aca_dhcp_state_handler.cpp | 65 +++++++++++++++++++---------- 2 files changed, 47 insertions(+), 24 deletions(-) diff --git a/src/comm/aca_grpc.cpp b/src/comm/aca_grpc.cpp index d6bfa435..8a8d7487 100644 --- a/src/comm/aca_grpc.cpp +++ b/src/comm/aca_grpc.cpp @@ -41,8 +41,10 @@ HostRequestReply GoalStateProvisionerImpl::RequestGoalStates(HostRequest *reques { grpc::ClientContext ctx; alcor::schema::HostRequestReply reply; - - if (chan_->GetState(false) != grpc_connectivity_state::GRPC_CHANNEL_READY) { + + grpc_connectivity_state currentChanState = chan_->GetState(true); + if (currentChanState == grpc_connectivity_state::GRPC_CHANNEL_TRANSIENT_FAILURE || + +currentChanState == grpc_connectivity_state::GRPC_CHANNEL_SHUTDOWN) { ACA_LOG_INFO("%s, it is: [%d]\n", "Channel state is not READY", chan_->GetState(false)); reply.mutable_operation_statuses()->Add(); reply.mutable_operation_statuses()->at(0).set_operation_status(OperationStatus::FAILURE); diff --git a/src/dhcp/aca_dhcp_state_handler.cpp b/src/dhcp/aca_dhcp_state_handler.cpp index 14b80086..c9f91747 100644 --- a/src/dhcp/aca_dhcp_state_handler.cpp +++ b/src/dhcp/aca_dhcp_state_handler.cpp @@ -53,20 +53,16 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem(const DHCPState current_D int overall_rc = EXIT_SUCCESS; ulong culminative_dataplane_programming_time = 0; ulong culminative_network_configuration_time = 0; - auto operation_start = chrono::steady_clock::now(); - DHCPConfiguration current_DhcpConfiguration = current_DhcpState.configuration(); stDhcpCfg.mac_address = current_DhcpConfiguration.mac_address(); stDhcpCfg.ipv4_address = current_DhcpConfiguration.ipv4_address(); stDhcpCfg.ipv6_address = current_DhcpConfiguration.ipv6_address(); stDhcpCfg.port_host_name = current_DhcpConfiguration.port_host_name(); - string subnet_id = current_DhcpConfiguration.subnet_id(); for (int i = 0; i < parsed_struct.subnet_states_size(); i++) { SubnetState current_SubnetState = parsed_struct.subnet_states(i); SubnetConfiguration current_SubnetConfiguration = current_SubnetState.configuration(); - if (subnet_id == current_SubnetConfiguration.id()) { stDhcpCfg.gateway_address = current_SubnetConfiguration.gateway().ip_address(); stDhcpCfg.subnet_mask = @@ -80,8 +76,20 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem(const DHCPState current_D break; } } - - switch (current_DhcpState.operation_type()) { + alcor::schema::OperationType current_operation_type = + current_DhcpState.operation_type(); + if (current_operation_type == alcor::schema::OperationType::UPDATE) { + for (int i = 0; i < parsed_struct.port_states().size(); i++) { + if (parsed_struct.port_states().at(i).configuration().mac_address() == + current_DhcpConfiguration.mac_address() && + !parsed_struct.port_states().at(i).configuration().device_id().empty() && + !parsed_struct.port_states().at(i).configuration().device_owner().empty()) { + current_operation_type = alcor::schema::OperationType::CREATE; + break; + } + } + } + switch (current_operation_type) { case OperationType::CREATE: overall_rc = this->dhcp_programming_if->add_dhcp_entry(&stDhcpCfg); break; @@ -96,20 +104,29 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem(const DHCPState current_D overall_rc = EXIT_FAILURE; break; } - auto operation_end = chrono::steady_clock::now(); - auto operation_total_time = cast_to_microseconds(operation_end - operation_start).count(); - aca_goal_state_handler::Aca_Goal_State_Handler::get_instance().add_goal_state_operation_status( gsOperationReply, current_DhcpConfiguration.id(), DHCP, current_DhcpState.operation_type(), overall_rc, culminative_dataplane_programming_time, culminative_network_configuration_time, operation_total_time); - return overall_rc; } +auto operation_end = chrono::steady_clock::now(); + +auto operation_total_time = + cast_to_microseconds(operation_end - operation_start).count(); + +aca_goal_state_handler::Aca_Goal_State_Handler::get_instance().add_goal_state_operation_status( + gsOperationReply, current_DhcpConfiguration.id(), DHCP, + current_DhcpState.operation_type(), overall_rc, culminative_dataplane_programming_time, + culminative_network_configuration_time, operation_total_time); + +return overall_rc; +} + int Aca_Dhcp_State_Handler::update_dhcp_states(GoalState &parsed_struct, GoalStateOperationReply &gsOperationReply) { @@ -144,23 +161,17 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem_v2(const DHCPState curren int overall_rc = EXIT_SUCCESS; ulong culminative_dataplane_programming_time = 0; ulong culminative_network_configuration_time = 0; - auto operation_start = chrono::steady_clock::now(); - DHCPConfiguration current_DhcpConfiguration = current_DhcpState.configuration(); stDhcpCfg.mac_address = current_DhcpConfiguration.mac_address(); stDhcpCfg.ipv4_address = current_DhcpConfiguration.ipv4_address(); stDhcpCfg.ipv6_address = current_DhcpConfiguration.ipv6_address(); stDhcpCfg.port_host_name = current_DhcpConfiguration.port_host_name(); - string subnet_id = current_DhcpConfiguration.subnet_id(); - auto subnetStateFound = parsed_struct.subnet_states().find(subnet_id); - if (subnetStateFound != parsed_struct.subnet_states().end()) { SubnetState current_SubnetState = subnetStateFound->second; SubnetConfiguration current_SubnetConfiguration = current_SubnetState.configuration(); - stDhcpCfg.gateway_address = current_SubnetConfiguration.gateway().ip_address(); stDhcpCfg.subnet_mask = aca_convert_cidr_to_netmask(current_SubnetConfiguration.cidr()); @@ -174,9 +185,23 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem_v2(const DHCPState curren subnet_id.c_str()); overall_rc = EXIT_FAILURE; } - + alcor::schema::OperationType current_operation_type = + current_DhcpState.operation_type(); + if (current_operation_type == alcor::schema::OperationType::UPDATE) { + for (auto &port_state : parsed_struct.port_states()) { + // port_state.first is the key (resource_id) + // port_state.second is the value (PortState) + if (port_state.second.configuration().mac_address() == + current_DhcpConfiguration.mac_address() && + !port_state.second.configuration().device_id().empty() && + !port_state.second.configuration().device_owner().empty()) { + current_operation_type = alcor::schema::OperationType::CREATE; + break; + } + } + } if (overall_rc == EXIT_SUCCESS) { - switch (current_DhcpState.operation_type()) { + switch (current_operation_type) { case OperationType::CREATE: overall_rc = this->dhcp_programming_if->add_dhcp_entry(&stDhcpCfg); break; @@ -191,17 +216,13 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem_v2(const DHCPState curren overall_rc = EXIT_FAILURE; } } - auto operation_end = chrono::steady_clock::now(); - auto operation_total_time = cast_to_microseconds(operation_end - operation_start).count(); - aca_goal_state_handler::Aca_Goal_State_Handler::get_instance().add_goal_state_operation_status( gsOperationReply, current_DhcpConfiguration.id(), DHCP, current_DhcpState.operation_type(), overall_rc, culminative_dataplane_programming_time, culminative_network_configuration_time, operation_total_time); - return overall_rc; } From 8931f06685a6ca0b72c58cfea8499940afaa95f5 Mon Sep 17 00:00:00 2001 From: Rio Zhu Date: Fri, 26 Mar 2021 21:25:49 +0000 Subject: [PATCH 2/6] Removed extra code --- src/dhcp/aca_dhcp_state_handler.cpp | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/dhcp/aca_dhcp_state_handler.cpp b/src/dhcp/aca_dhcp_state_handler.cpp index c9f91747..b783890d 100644 --- a/src/dhcp/aca_dhcp_state_handler.cpp +++ b/src/dhcp/aca_dhcp_state_handler.cpp @@ -114,19 +114,6 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem(const DHCPState current_D return overall_rc; } -auto operation_end = chrono::steady_clock::now(); - -auto operation_total_time = - cast_to_microseconds(operation_end - operation_start).count(); - -aca_goal_state_handler::Aca_Goal_State_Handler::get_instance().add_goal_state_operation_status( - gsOperationReply, current_DhcpConfiguration.id(), DHCP, - current_DhcpState.operation_type(), overall_rc, culminative_dataplane_programming_time, - culminative_network_configuration_time, operation_total_time); - -return overall_rc; -} - int Aca_Dhcp_State_Handler::update_dhcp_states(GoalState &parsed_struct, GoalStateOperationReply &gsOperationReply) { From 62af3451fa5ebc69e39c509edef177dd448b0ab2 Mon Sep 17 00:00:00 2001 From: Rio Zhu Date: Fri, 26 Mar 2021 21:53:35 +0000 Subject: [PATCH 3/6] added logs in port state checking --- src/dhcp/aca_dhcp_state_handler.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/dhcp/aca_dhcp_state_handler.cpp b/src/dhcp/aca_dhcp_state_handler.cpp index b783890d..a4020644 100644 --- a/src/dhcp/aca_dhcp_state_handler.cpp +++ b/src/dhcp/aca_dhcp_state_handler.cpp @@ -78,8 +78,16 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem(const DHCPState current_D } alcor::schema::OperationType current_operation_type = current_DhcpState.operation_type(); + ACA_LOG_INFO("%s, port_states size: %d", "Searching in port states\n", + parsed_struct.port_states().size()); + if (current_operation_type == alcor::schema::OperationType::UPDATE) { for (int i = 0; i < parsed_struct.port_states().size(); i++) { + ACA_LOG_INFO( + "Port %d MAC: %s, device_ID: %s, device_owner: %s", i, + (parsed_struct.port_states().at(i).configuration().mac_address()).c_str(), + (parsed_struct.port_states().at(i).configuration().device_id()).c_str(), + (parsed_struct.port_states().at(i).configuration().device_owner()).c_str()); if (parsed_struct.port_states().at(i).configuration().mac_address() == current_DhcpConfiguration.mac_address() && !parsed_struct.port_states().at(i).configuration().device_id().empty() && From 8dd89d046aeb2fc7e73c276f75c7168c7911fa3a Mon Sep 17 00:00:00 2001 From: Rio Zhu Date: Fri, 26 Mar 2021 22:27:45 +0000 Subject: [PATCH 4/6] Removed GRPC related code --- src/comm/aca_grpc.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/comm/aca_grpc.cpp b/src/comm/aca_grpc.cpp index 8a8d7487..98150ef0 100644 --- a/src/comm/aca_grpc.cpp +++ b/src/comm/aca_grpc.cpp @@ -42,9 +42,7 @@ HostRequestReply GoalStateProvisionerImpl::RequestGoalStates(HostRequest *reques grpc::ClientContext ctx; alcor::schema::HostRequestReply reply; - grpc_connectivity_state currentChanState = chan_->GetState(true); - if (currentChanState == grpc_connectivity_state::GRPC_CHANNEL_TRANSIENT_FAILURE || - +currentChanState == grpc_connectivity_state::GRPC_CHANNEL_SHUTDOWN) { + if (chan_->GetState(false) != grpc_connectivity_state::GRPC_CHANNEL_READY) { ACA_LOG_INFO("%s, it is: [%d]\n", "Channel state is not READY", chan_->GetState(false)); reply.mutable_operation_statuses()->Add(); reply.mutable_operation_statuses()->at(0).set_operation_status(OperationStatus::FAILURE); From c9904e9791054a15f5249195a31df19fb4e69395 Mon Sep 17 00:00:00 2001 From: Rio Zhu Date: Fri, 26 Mar 2021 22:39:56 +0000 Subject: [PATCH 5/6] Added new line at the end of logs --- src/dhcp/aca_dhcp_state_handler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dhcp/aca_dhcp_state_handler.cpp b/src/dhcp/aca_dhcp_state_handler.cpp index a4020644..30069fbd 100644 --- a/src/dhcp/aca_dhcp_state_handler.cpp +++ b/src/dhcp/aca_dhcp_state_handler.cpp @@ -78,13 +78,13 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem(const DHCPState current_D } alcor::schema::OperationType current_operation_type = current_DhcpState.operation_type(); - ACA_LOG_INFO("%s, port_states size: %d", "Searching in port states\n", + ACA_LOG_INFO("%s, port_states size: %d\n", "Searching in port states\n", parsed_struct.port_states().size()); if (current_operation_type == alcor::schema::OperationType::UPDATE) { for (int i = 0; i < parsed_struct.port_states().size(); i++) { ACA_LOG_INFO( - "Port %d MAC: %s, device_ID: %s, device_owner: %s", i, + "Port %d MAC: %s, device_ID: %s, device_owner: %s\n", i, (parsed_struct.port_states().at(i).configuration().mac_address()).c_str(), (parsed_struct.port_states().at(i).configuration().device_id()).c_str(), (parsed_struct.port_states().at(i).configuration().device_owner()).c_str()); From 513a8c908d298ce0872fa792c197b69172847661 Mon Sep 17 00:00:00 2001 From: Rio Zhu Date: Mon, 29 Mar 2021 16:43:39 +0000 Subject: [PATCH 6/6] Added comments, and optimized logic, with the suggestions from @er1ctheOne --- src/dhcp/aca_dhcp_state_handler.cpp | 42 +++++++++++++++--------- src/dp_abstraction/aca_dataplane_ovs.cpp | 6 ++++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/dhcp/aca_dhcp_state_handler.cpp b/src/dhcp/aca_dhcp_state_handler.cpp index 30069fbd..9f89e5d4 100644 --- a/src/dhcp/aca_dhcp_state_handler.cpp +++ b/src/dhcp/aca_dhcp_state_handler.cpp @@ -85,18 +85,24 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem(const DHCPState current_D for (int i = 0; i < parsed_struct.port_states().size(); i++) { ACA_LOG_INFO( "Port %d MAC: %s, device_ID: %s, device_owner: %s\n", i, - (parsed_struct.port_states().at(i).configuration().mac_address()).c_str(), - (parsed_struct.port_states().at(i).configuration().device_id()).c_str(), - (parsed_struct.port_states().at(i).configuration().device_owner()).c_str()); - if (parsed_struct.port_states().at(i).configuration().mac_address() == + (parsed_struct.port_states(i).configuration().mac_address()).c_str(), + (parsed_struct.port_states(i).configuration().device_id()).c_str(), + (parsed_struct.port_states(i).configuration().device_owner()).c_str()); + if (parsed_struct.port_states(i).configuration().mac_address() == current_DhcpConfiguration.mac_address() && - !parsed_struct.port_states().at(i).configuration().device_id().empty() && - !parsed_struct.port_states().at(i).configuration().device_owner().empty()) { + !parsed_struct.port_states(i).configuration().device_id().empty() && + !parsed_struct.port_states(i).configuration().device_owner().empty()) { current_operation_type = alcor::schema::OperationType::CREATE; break; } } } + /* + In the current test environment, Nova may send DHCP UPDATE, when it tries to + CREATE a new port, we are now adding this logic to the ACA as a temporary + fix, if this logic is proved to be successful, this logic may be moved to the + Port Manger/Dataplane Manager. + */ switch (current_operation_type) { case OperationType::CREATE: overall_rc = this->dhcp_programming_if->add_dhcp_entry(&stDhcpCfg); @@ -182,17 +188,21 @@ int Aca_Dhcp_State_Handler::update_dhcp_state_workitem_v2(const DHCPState curren } alcor::schema::OperationType current_operation_type = current_DhcpState.operation_type(); + /* + In the current test environment, Nova may send DHCP UPDATE, when it tries to + CREATE a new port, we are now adding this logic to the ACA as a temporary + fix, if this logic is proved to be successful, this logic may be moved to the + Port Manger/Dataplane Manager. + */ if (current_operation_type == alcor::schema::OperationType::UPDATE) { - for (auto &port_state : parsed_struct.port_states()) { - // port_state.first is the key (resource_id) - // port_state.second is the value (PortState) - if (port_state.second.configuration().mac_address() == - current_DhcpConfiguration.mac_address() && - !port_state.second.configuration().device_id().empty() && - !port_state.second.configuration().device_owner().empty()) { - current_operation_type = alcor::schema::OperationType::CREATE; - break; - } + auto target_port_state = + parsed_struct.port_states().find(current_DhcpConfiguration.mac_address()); + // target_port_state.first is the key (resource_id) + // target_port_state.second is the value (PortState) + if (target_port_state != parsed_struct.port_states().end() && + !target_port_state->second.configuration().device_id().empty() && + !target_port_state->second.configuration().device_owner().empty()) { + current_operation_type = alcor::schema::OperationType::CREATE; } } if (overall_rc == EXIT_SUCCESS) { diff --git a/src/dp_abstraction/aca_dataplane_ovs.cpp b/src/dp_abstraction/aca_dataplane_ovs.cpp index 14160d4c..87656687 100644 --- a/src/dp_abstraction/aca_dataplane_ovs.cpp +++ b/src/dp_abstraction/aca_dataplane_ovs.cpp @@ -216,6 +216,12 @@ int ACA_Dataplane_OVS::update_port_state_workitem(const PortState current_PortSt } alcor::schema::OperationType current_operation_type = current_PortState.operation_type(); + /* + In the current test environment, Nova may send PORT UPDATE, when it tries to + CREATE a new port, when the device_id and device_owner are not empty, we are + now adding this logic to the ACA as a temporary fix, if this logic is proved + to be successful, this logic may be moved to the Port Manger/Dataplane Manager. + */ if (current_PortState.operation_type() == OperationType::UPDATE) { if (!current_PortConfiguration.device_id().empty() && !current_PortConfiguration.device_owner().empty()) {