Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 759: This will add the cleanup service #1236

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

bailaC
Copy link
Contributor

@bailaC bailaC commented Dec 18, 2023

@bailaC bailaC closed this Dec 18, 2023
@bailaC bailaC reopened this Dec 18, 2023
@bailaC
Copy link
Contributor Author

bailaC commented Dec 18, 2023

@destogl Please check the draft PR. I think the main logic of the transition is missing. I will check again at my end.

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: Patch coverage is 33.33333% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 47.45%. Comparing base (ff3177b) to head (4d4ed45).

❗ Current head 4d4ed45 differs from pull request most recent head 418e47a. Consider uploading reports for the commit 418e47a to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1236       +/-   ##
===========================================
- Coverage   75.89%   47.45%   -28.45%     
===========================================
  Files          41       41               
  Lines        3352     3595      +243     
  Branches     1926     1962       +36     
===========================================
- Hits         2544     1706      -838     
- Misses        417      459       +42     
- Partials      391     1430     +1039     
Flag Coverage Δ
unittests 47.45% <33.33%> (-28.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
.../include/controller_manager/controller_manager.hpp 18.18% <ø> (-13.97%) ⬇️
controller_manager/src/controller_manager.cpp 38.91% <33.33%> (-30.46%) ⬇️

... and 30 files with indirect coverage changes

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @bailaC,

After looking roughly at the cleanup_controller service, it seems to me similar to the unload_controller service that exist in the controller manager.

controller_interface::return_type ControllerManager::unload_controller(
const std::string & controller_name)
{
std::lock_guard<std::recursive_mutex> guard(rt_controllers_wrapper_.controllers_lock_);
std::vector<ControllerSpec> & to = rt_controllers_wrapper_.get_unused_list(guard);
const std::vector<ControllerSpec> & from = rt_controllers_wrapper_.get_updated_list(guard);
// Transfers the active controllers over, skipping the one to be removed and the active ones.
to = from;
auto found_it = std::find_if(
to.begin(), to.end(),
std::bind(controller_name_compare, std::placeholders::_1, controller_name));
if (found_it == to.end())
{
// Fails if we could not remove the controllers
to.clear();
RCLCPP_ERROR(
get_logger(),
"Could not unload controller with name '%s' because no controller with this name exists",
controller_name.c_str());
return controller_interface::return_type::ERROR;
}
auto & controller = *found_it;
if (is_controller_active(*controller.c))
{
to.clear();
RCLCPP_ERROR(
get_logger(), "Could not unload controller with name '%s' because it is still active",
controller_name.c_str());
return controller_interface::return_type::ERROR;
}
if (controller.c->is_async())
{
RCLCPP_DEBUG(
get_logger(), "Removing controller '%s' from the list of async controllers",
controller_name.c_str());
async_controller_threads_.erase(controller_name);
}
RCLCPP_DEBUG(get_logger(), "Cleanup controller");
// TODO(destogl): remove reference interface if chainable; i.e., add a separate method for
// cleaning-up controllers?
controller.c->get_node()->cleanup();
executor_->remove_node(controller.c->get_node()->get_node_base_interface());
to.erase(found_it);
// Destroys the old controllers list when the realtime thread is finished with it.
RCLCPP_DEBUG(get_logger(), "Realtime switches over to new controller list");
rt_controllers_wrapper_.switch_updated_list(guard);
std::vector<ControllerSpec> & new_unused_list = rt_controllers_wrapper_.get_unused_list(guard);
RCLCPP_DEBUG(get_logger(), "Destruct controller");
new_unused_list.clear();
RCLCPP_DEBUG(get_logger(), "Destruct controller finished");
RCLCPP_DEBUG(get_logger(), "Successfully unloaded controller '%s'", controller_name.c_str());
return controller_interface::return_type::OK;
}

What do you think?

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bailaC can you please write tests for the newly added code.

@destogl destogl marked this pull request as ready for review February 2, 2024 15:10
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the following changes, I want to know your opinion @destogl @bailaC on the following

Does it make sense to call the service alone unconfigure_controller instead of cleanup_controller (We can leave the rest of the methods without renaming as internally it's fine)?. Because it might give more or less the basic idea of the service in the context of controllers.

"Could not unload controller with name '%s' because no controller with this name exists",
controller_name.c_str());
return controller_interface::return_type::ERROR;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the above cleanup controller call should come after this, if not you get the error message of controller not found from cleanup rather than the unload controller


response->ok = cleanup_controller(request->name) == controller_interface::return_type::OK;

RCLCPP_DEBUG(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print the cleanup finished message if the response of the clean_controller method is OK. If not, better to print an error or leave the earlier error from within the method

cm_->get_loaded_controllers()[0].c->get_state().id());
result = call_service_and_wait(*client, request, srv_executor, true);
ASSERT_TRUE(result->ok) << "Controller cleaned in inactive state: " << request->name;
EXPECT_EQ(1u, cm_->get_loaded_controllers().size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EXPECT_EQ(1u, cm_->get_loaded_controllers().size());
EXPECT_EQ(
lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED,
cm_->get_loaded_controllers()[0].c->get_state().id());
EXPECT_EQ(1u, cm_->get_loaded_controllers().size());

it's good to check that the transition is successful

If not, in future we might face a situation that it will be returned ok, but never transitioned

cm_->get_loaded_controllers()[0].c->get_state().id());
result = call_service_and_wait(*client, request, srv_executor, true);
ASSERT_FALSE(result->ok) << "Controller can not be cleaned in active state: " << request->name;
EXPECT_EQ(1u, cm_->get_loaded_controllers().size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EXPECT_EQ(1u, cm_->get_loaded_controllers().size());
EXPECT_EQ(
lifecycle_msgs::msg::State::PRIMARY_STATE_ACTIVE,
cm_->get_loaded_controllers()[0].c->get_state().id());
EXPECT_EQ(1u, cm_->get_loaded_controllers().size());

// scenario: call the cleanup service when no controllers are loaded
// expected: it should return ERROR as no controllers will be found to cleanup
auto result = call_service_and_wait(*client, request, srv_executor);
ASSERT_FALSE(result->ok) << "Controller not loaded: " << request->name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ASSERT_FALSE(result->ok) << "Controller not loaded: " << request->name;
EXPECT_EQ(
lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED,
cm_->get_loaded_controllers()[0].c->get_state().id());
ASSERT_FALSE(result->ok) << "Controller not loaded: " << request->name;

Copy link
Contributor

mergify bot commented Feb 9, 2024

This pull request is in conflict. Could you fix it @bailaC?

Copy link
Contributor

mergify bot commented Apr 29, 2024

This pull request is in conflict. Could you fix it @bailaC?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants