Spawner waits for services

[tylerjw]

Signed-off-by: Tyler Weaver tyler@picknik.ai

Resolves #682

To send us a pull request, please:

• Fork the repository.
• Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
• Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
• Commit to your fork using clear commit messages.
• Send a pull request, answering any default questions in the pull request interface.
• Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

Testing

Locally I'm getting two test failures with this. I would appreciate any help in resolving these test failures:

    [----------] Global test environment tear-down
    [==========] 6 tests from 1 test suite ran. (49366 ms total)
    [  PASSED  ] 4 tests.
    [  FAILED  ] 2 tests, listed below:
    [  FAILED  ] TestLoadController.spawner_test_type_in_param
    [  FAILED  ] TestLoadController.spawner_test_type_in_arg
    
     2 FAILED TESTS
    -- run_test.py: return code 1
    -- run_test.py: inject classname prefix into gtest result file '/home/tyler/code/l0/ws_target/build/controller_manager/test_results/controller_manager/test_spawner_unspawner.gtest.xml'
    -- run_test.py: verify result file '/home/tyler/code/l0/ws_target/build/controller_manager/test_results/controller_manager/test_spawner_unspawner.gtest.xml'
  >>>
build/controller_manager/test_results/controller_manager/test_spawner_unspawner.gtest.xml: 6 tests, 0 errors, 2 failures, 0 skipped
- controller_manager.TestLoadController spawner_test_type_in_param
  <<< failure message
    /home/tyler/code/l0/ws_target/src/ros2_control/controller_manager/test/test_spawner_unspawner.cpp:96
    Expected equality of these values:
      call_spawner("ctrl_1 -c test_controller_manager")
        Which is: 256
      0
    /home/tyler/code/l0/ws_target/src/ros2_control/controller_manager/test/test_spawner_unspawner.cpp:98
    Expected equality of these values:
      cm_->get_loaded_controllers().size()
        Which is: 0
      1ul
        Which is: 1
  >>>
- controller_manager.TestLoadController spawner_test_type_in_arg
  <<< failure message
    /home/tyler/code/l0/ws_target/src/ros2_control/controller_manager/test/test_spawner_unspawner.cpp:127
    Expected equality of these values:
      call_spawner( "ctrl_2 -c test_controller_manager -t " + std::string(test_controller::TEST_CONTROLLER_CLASS_NAME))
        Which is: 256
      0
    /home/tyler/code/l0/ws_target/src/ros2_control/controller_manager/test/test_spawner_unspawner.cpp:133
    Expected equality of these values:
      cm_->get_loaded_controllers().size()
        Which is: 0
      1ul
        Which is: 1
  >>>

[tylerjw]

I also have no idea how to test this fixes the race condition other than my local environment. I was not able to produce a minimal example that showed the race condition however reading the code it seems obvious to me that there is a race condition.

As to unit testing these new functions I don't know where to start with writing python unit tests. If someone would like to point me to an example I'd be happy to try write unit tests for these new functions (and the old one that it doesn't look like anyone tested).

[destogl]

This looks good, but I am just not sure if this is not to complicate code for the task since there are many function injections. Probably an additional for-loop for checking the services would also do.

Other than that, having an output with “wait” notification as previously existed would be useful.

Regarding tests, you can check this file for spawner tests.

[tylerjw]

It seems that the Galactic binary job (galactic, testing) is experiencing the same test failures I am locally seeing. I'll try running tests without this change and see if they pass.

[tylerjw]

Other than that, having an output with “wait” notification as previously existed would be useful.

Is that log actually useful? You did catch that I removed it.

[tylerjw]

Regarding tests, you can check this file for spawner tests.

I did look at that and it seems that it only tests the spawner from the outside. Is there some way to ask the controller_manager to start without services? That might be a valid way to write a black box test like what is shown here that would verify this code does what you expect.

[tylerjw]

Is there some way you think this would be clearer with for loops? I started implementing it that way and found that I had a bunch of copy-pasta between checking for the node to exist and checking for the services to exist so I tried to separate the algorithmic code and the checks it was doing.

[tylerjw]

I put the log back in the wait_for function so you get a log when it is waiting for the controller_manager to initialize and cleaned up that if statement with an assignment expression.

[tylerjw]

I fixed the logging some more to troubleshoot this and it actually doesn't seem to work for me. Maybe something about galactic is broken? If I run ros2 node info /controller_manager as it is initializing I can see it before the services exist and after they have come up and they are all there. If I run my code here and watch the debug statements the logger that is logging client_names_and_types always prints and empty list.

[destogl]

Maybe something about galactic is broken?

Consider that you are actually submitting this against rolling, which is correct. We would back-port this than to galactic.

[tylerjw]

Consider that you are actually submitting this against rolling, which is correct. We would back-port this than to galactic.

I know, I just can't test this on rolling because rolling is so broken right now.

[tylerjw]

I believe this now works. I am still unsure how to test it.

[bmagyar]

This'd require some heavy mocking to test and I think the utility of that would be questionable. Probably the best way forward is trial by fire

[bmagyar]

Nitpick: I found writing Python with type annotations very maintainable, perhaps that'd help the readability but that could be a good first issue for anyone, not necessarily needed in this PR

[erickisos]

In general, it looks good to me!

thank you for bringing us this approach, it's amazing!!

[tylerjw]

@erickisos I accepted your changes but it looks like they aren't passing the formatter check. Would you mind pushing a change that fixes that or would you like me to?

[erickisos]

@erickisos I accepted your changes but it looks like they aren't passing the formatter check. Would you mind pushing a change that fixes that or would you like me to?

Sorry @tylerjw , let me check!

[destogl]

@Mergifyio backport galactic

[mergify]

backport galactic

✅ Backports have been created

• #697 Spawner waits for services (backport #683) has been created for branch galactic