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

feat: add autoware_control_center and autoware_node #84

Closed
wants to merge 5 commits into from

Conversation

xmfcx
Copy link
Contributor

@xmfcx xmfcx commented May 17, 2024

Description

Goals for the v1:

  • Lifecycle
  • Register/Deregister
    • List potential cases that are handled
  • Heartbeat monitoring
    • Error/Warning/Healthy states are part of the heartbeat
  • Basic ACC features
    • Report faulty nodes

Remaining tasks

Remaining items won't be handled in this PR.

  • Write down the AutowareNode.
    • Create tests for it.
      • Init / Shutdown
      • Register
        • AN is launched first
        • ACC is launched first
        • Duplicate registering with same name
      • Heartbeat
        • Success case
        • Dead upon being late
        • Alive upon coming back

Edge cases

Support:

  • ACC runs, AN runs, ACC dies, another ACC comes, AN should register to it.
  • AN dies, ACC still has an entry, after a grace period, it should deregister it automatically.

Related links

🖱️Click to expand

Continuation from:

Nav2 references

Autoware.Auto discussions

Tests performed

WIP

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@xmfcx xmfcx added the enhancement New feature or request label May 17, 2024
@xmfcx xmfcx self-assigned this May 17, 2024
@esteve
Copy link

esteve commented May 21, 2024

@xmfcx I found this about using Kubernetes to ensure a singleton instance:

https://medium.com/@fahed.dorgaa/kubernetes-singleton-pattern-84a88a06727a

And this for integrating Kubernetes with ROS 2:

https://github.com/fujitatomoya/ros_k8s

Hope this helps, let me know if I can help

@xmfcx
Copy link
Contributor Author

xmfcx commented May 21, 2024

@esteve I've read the medium post and it aims to solve the same problem I had. I've also read the slides and looked into implementations from https://github.com/fujitatomoya/ros_k8s as well.

Kubernetes, also known as K8s, is an open source system for automating deployment, scaling, and management of containerized applications.

But k8s works with containers and I would assume this would have meant that /autoware/control_center node would need to be in its own container? I don't know it too well. But if yes, this is not the solution I'm looking for. I would like to be able to run the entire Autoware in a single container or without any.

I think current solution is enough for now, since in the deployment stage, this would not occur anyways, it is just to prevent adding another thing to debug for developers by making sure there is only one instance of the /autoware/control_center.

@esteve
Copy link

esteve commented May 21, 2024

@xmfcx yeah, for something like this,, Kubernetes is overkill, I thought it was more about making sure that nodes in a distributed system were only instantiated only. But if it's a tool that runs locally, a lock and a file is more than enough.

@xmfcx xmfcx force-pushed the feat/add-autoware-control-center branch from 7628800 to a1b7090 Compare July 19, 2024 11:41
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 75.28302% with 131 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@df9cd36). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...utoware_control_center/src/control_center_node.cpp 74.10% 17 Missing and 12 partials ⚠️
...utoware_control_center/src/control_center_main.cpp 48.78% 12 Missing and 9 partials ⚠️
...toware_control_center/test/test_cc_registering.cpp 57.44% 3 Missing and 17 partials ⚠️
...autoware_control_center/test/test_cc_heartbeat.cpp 78.94% 3 Missing and 9 partials ⚠️
common/autoware_node/src/node.cpp 79.62% 5 Missing and 6 partials ⚠️
common/autoware_node/test/test_an_registering.cpp 80.00% 0 Missing and 10 partials ⚠️
...mmon/autoware_control_center/test/test_utility.hpp 85.00% 3 Missing and 6 partials ⚠️
common/autoware_node/test/test_an_heartbeat.cpp 87.17% 0 Missing and 5 partials ⚠️
...ommon/autoware_node/test/test_an_init_shutdown.cpp 81.48% 1 Missing and 4 partials ⚠️
common/autoware_test_node/src/test_node.cpp 0.00% 5 Missing ⚠️
... and 2 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #84   +/-   ##
=======================================
  Coverage        ?   75.28%           
=======================================
  Files           ?       13           
  Lines           ?      530           
  Branches        ?      318           
=======================================
  Hits            ?      399           
  Misses          ?       50           
  Partials        ?       81           
Flag Coverage Δ
differential 75.28% <75.28%> (?)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xmfcx xmfcx force-pushed the feat/add-autoware-control-center branch from a1b7090 to dc0fb55 Compare July 19, 2024 11:47
Copy link

stale bot commented Sep 29, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the stale label Sep 29, 2024
@mitsudome-r mitsudome-r mentioned this pull request Oct 29, 2024
14 tasks
Copy link

github-actions bot commented Nov 7, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@xmfcx xmfcx force-pushed the feat/add-autoware-control-center branch from 1c2748f to 185343f Compare November 17, 2024 22:59
@stale stale bot removed the stale label Nov 17, 2024
@xmfcx xmfcx force-pushed the feat/add-autoware-control-center branch from ce6afb8 to b61993c Compare November 27, 2024 12:38
@xmfcx xmfcx requested a review from mitsudome-r November 27, 2024 12:39
@xmfcx xmfcx marked this pull request as ready for review November 27, 2024 12:39
@xmfcx xmfcx force-pushed the feat/add-autoware-control-center branch 4 times, most recently from 5fd074a to 7acead0 Compare November 27, 2024 12:55
@xmfcx
Copy link
Contributor Author

xmfcx commented Nov 27, 2024

@mitsudome-r this is ready for review.

I will go and fix spell-check-differential issues now. (Copy the repo and configure it)
Then will look into cppcheck, it's not working properly.

xmfcx and others added 5 commits December 2, 2024 10:27
Signed-off-by: M. Fatih Cırıt <mfc@autoware.org>
Signed-off-by: M. Fatih Cırıt <mfc@autoware.org>
Signed-off-by: M. Fatih Cırıt <mfc@autoware.org>
Signed-off-by: M. Fatih Cırıt <mfc@autoware.org>
@xmfcx
Copy link
Contributor Author

xmfcx commented Dec 4, 2024

Thanks, it was fixed:

It seems to not care about the capitalization ✅

@youtalk
Copy link
Member

youtalk commented Dec 5, 2024

@xmfcx We would like to review autoware_node first and then review autoware_control_center next. Could you split this PR into 2 PRs?

@xmfcx
Copy link
Contributor Author

xmfcx commented Dec 5, 2024

@youtalk -san, these don't make much sense to be added individually:

  • autoware_control_center
  • autoware_control_center_msgs
  • autoware_node
  • autoware_test_node

And they have test dependencies to each other.

For example autoware_node has a <test_depend>autoware_control_center</test_depend> because in the test it communicates with it.

If I make separate PRs, the tests would fail.

@youtalk youtalk self-requested a review December 6, 2024 02:24
@youtalk
Copy link
Member

youtalk commented Dec 6, 2024

@xmfcx I would like to thoroughly review the source code of autoware.core, but I cannot review all 47 files and 2,463 added lines at once. If it is not possible to split by package, would it be possible to divide the PRs for each goal instead?

Goals for the v1:

  • Lifecycle
  • Register/Deregister
    • List potential cases that are handled
  • Heartbeat monitoring
    • Error/Warning/Healthy states are part of the heartbeat
  • Basic ACC features
    • Report faulty nodes

For example, we could start with an almost empty implementation of the Lifecycle, and then build on it step by step.
It might feel like extra effort until completion, but the review process will be faster, which I believe will ultimately lead to quicker overall completion.

@xmfcx
Copy link
Contributor Author

xmfcx commented Dec 6, 2024

Sure, let's go feature by feature 👍

@xmfcx xmfcx closed this Dec 6, 2024
@youtalk youtalk deleted the feat/add-autoware-control-center branch December 6, 2024 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants