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

Introduce actor-based thin-edge components #1622

Merged
merged 143 commits into from
Feb 15, 2023

Conversation

didier-wenzek
Copy link
Contributor

@didier-wenzek didier-wenzek commented Nov 30, 2022

Proposed changes

First step toward thin-edge Rust extensions

Types of changes

This PR provides:

  • A new crate, tedge_actors, that defines how to implement, compose and test thin-edge applications using independent computing units that cooperate through messages over in-memory channels.
  • A set of generic actors to support MQTT, HTTP, unix signals, inotify events, timers.
  • A set of Cumulocity actors to handle configuration and log management.
  • An executable that combines the features of the c8y-configuration-plugin and c8y-log-plugin, using the new actor design but with no external changes.

This PR has no impacts on the current thin-edge. The plan is to evolve concurrently the new and former design, porting the features progressively.

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@didier-wenzek didier-wenzek marked this pull request as draft November 30, 2022 13:56
@didier-wenzek didier-wenzek changed the title Rfc/tedge actor POC thin-edge Rust extensions and actors Nov 30, 2022
crates/bin/c8y_configuration_poc/src/http_ext.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_http_ext/src/lib.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/channels.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_http_ext/src/lib.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/actors.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/messages.rs Show resolved Hide resolved
crates/core/tedge_actors/src/messages.rs Show resolved Hide resolved
@didier-wenzek didier-wenzek force-pushed the rfc/tedge_actor branch 2 times, most recently from 82e8b48 to 4222c6e Compare December 14, 2022 16:48
@reubenmiller reubenmiller added the refactoring Developer value label Dec 16, 2022
Copy link
Contributor Author

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Still to be done:

  • Finalize the runtime.
  • Derive the implementation of the message boxes that are simple records of message boxes.
  • Fix fan_in_message_type! to accept arbitrary type expressions.
  • Derive the implementation of a handle (as C8YHttpProxy) from a list of (request, response) type pairs?
  • Consider to remove the ActorBuilder trait.
  • Find a way to avoid a call to runtime.spawn(actor) for each and every actor.
  • Can we have the message box owned by the actor instead of passing it as an argument to the run method?
  • Move the code to extract the body of an http response, from c8y_configuration_poc to tedge_http_ext.
  • Read C8YHttpConfig from tedge config.
  • Move file_system_ext under crates/extensions
  • Move c8y_http_proxy under crates/extensions
  • Move config_manager under crates/extensions/c8y_config_manager
  • Move log_manager under crates/extensions/c8y_log_manager

Cargo.toml Show resolved Hide resolved
}

/// A connection port to connect a message box under-connection to another box
pub trait MessageBoxPort<Request: Message, Response: Message> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea to improve this name?

  • MessageBoxSocket, since this a way to plug a peer actor mailbox to this message box?
  • ClientConnector, matching the name ServiceConnector, to stress this is to connect a client to a service?
  • ClientSocket ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This trait would need more documentation to better convey its purpose as it's not that straight-forward even after looking at the concrete implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, the documentation must crucially be improved. => I will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will rename this trait as ActorPlug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still looking for a better name.

  • ServiceConsumer or ServiceClient or ServiceUser might be good candidates as this trait defines the requirements of an actor for some service provided by another actor.
  • An alternative is also to deprecate this trait and use a combination of MessageSource + MessageSink .

This will be addressed as a follow-up task.

crates/core/tedge_actors/src/tasks.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_mqtt_ext/src/lib.rs Show resolved Hide resolved
crates/extensions/tedge_mqtt_ext/src/lib.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_mqtt_ext/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

A few general suggestions:

  1. It might be better if the existing tedge_actor APIs could be split into two: The core APIs which just contains the core concepts like Actor, MessageBox etc and and ext set which contains all the utility stuff like Service, SimpleMessgeBox, KeyedSender etc. This will make the API exploration lighter even for an outsider as otherwise he'll be a bit overwhelmed with so many abstraction, where not all are required to write simple actors.
  2. Error handling needs to be re-looked as many APIs expect ChannelErrors in the return type but a conversion of an actor specific error type to ChannelError is not straightforward. An anyhow::Error like approach would be easier for developers, which probably can be provided by just having a ChannelError enum variant that transparently wraps anyhow::Error

crates/extensions/tedge_http_ext/src/lib.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/handles.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/handles.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/channels.rs Show resolved Hide resolved
crates/core/tedge_actors/src/message_boxes.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
}

/// A connection port to connect a message box under-connection to another box
pub trait MessageBoxPort<Request: Message, Response: Message> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This trait would need more documentation to better convey its purpose as it's not that straight-forward even after looking at the concrete implementations.

crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
didier-wenzek added a commit to didier-wenzek/thin-edge.io that referenced this pull request Jan 16, 2023
See: thin-edge#1622

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
crates/core/tedge_actors/src/actors.rs Show resolved Hide resolved
crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
crates/core/tedge_actors/src/builders.rs Outdated Show resolved Hide resolved
//! - input messages that the actor processes in turn,
//! - output messages produced by the actor.
//!
//! ```
Copy link
Contributor

Choose a reason for hiding this comment

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

This feature could be used to avoid this example code duplication. I wish it was a bit more mature to allow scraping smaller blocks of code from the source file as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the link. However, I'm not fully convinced, the author having no way to tell which example to include in the doc and where (beyond that the documented function is used in these examples).

}
}

/// Create a message box pair (mostly for testing purpose)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Create a message box pair (mostly for testing purpose)
/// Create a message box with a `DualChannel` connected to it that can be used to send/receive messages to/from the message box (mostly for testing purpose).

My opinion: For some reason I find a "message box-channel" duo easier to convey than a message box duo, where one is connected to another. Especially because of the inverted Input/Output types in the generic signature of SimpleMessageBox<Output, Input>. I understand why it is like that, but I find a DualChannel<Input, Output> far more natural. But, this is just a personal opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you have in mind with by a DualChannel<Input, Output> struct?

I understand that you want to grasp the concept of bi-directional channel; but, I don't see how you plan to use such a single struct while two independent actors that require a mutable access to it. A DualChannel must be a pair. Furthermore, one must be able to use specific message box types for each side: the types expected by the actors under test.

About the inverted input/output, note this is why I sometimes use request/response in such a context, along specific types as RequestResponseHandler that abstract this issue.

Compare:

type DualChannel<Input, Output> = (
      SimpleMessageBox<Output,Input>,
      ServiceMessageBox<Input,Output>
)

with

type ClientServiceChannel<Request, Response> = (
       RequestResponseHandler<Request, Response>,
       ServiceMessageBox<Request, Response>
)

In the former case, the input of one end is the output of the other and vise-versa. This is misleading. While in the later case the meanings of request and response is clear despite the RequesResponseHandler sends requests and receives responses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this comment still relevant?

@didier-wenzek didier-wenzek force-pushed the rfc/tedge_actor branch 2 times, most recently from 4f7baf2 to 2b7c958 Compare February 8, 2023 13:52
didier-wenzek and others added 21 commits February 15, 2023 14:04
This trait has to be implemented by all actor builders,
so the runtime can connect to the actors before sending those.
This connection will then be used to send requests to the actors
either globally (e.g for a shutdown) or specically (e.g. to disable a
specific feature).

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Due to a stupid mistake: awaiting for the future actor.run()

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
The connection to the runtime was simply missing

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Silent the warning. This needs to be fixed globally: thin-edge#1716
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Using `Any` there is no more need for a cache per client
of the timer actor.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
The mqtt_channel is the internal implementation and should not be exposed.
MqttMessage, Topic, TopicFilter, MqttConfig and MqttError types
are re-exported by tedge_mqtt_ext.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek didier-wenzek merged commit 53152a2 into thin-edge:main Feb 15, 2023
@didier-wenzek didier-wenzek deleted the rfc/tedge_actor branch February 15, 2023 13:22
didier-wenzek added a commit to didier-wenzek/thin-edge.io that referenced this pull request Mar 6, 2023
See: thin-edge#1622

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek added a commit to didier-wenzek/thin-edge.io that referenced this pull request Apr 27, 2023
See: thin-edge#1622

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek added a commit to didier-wenzek/thin-edge.io that referenced this pull request May 4, 2023
See: thin-edge#1622

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek added a commit to didier-wenzek/thin-edge.io that referenced this pull request Jun 5, 2023
See: thin-edge#1622

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek added a commit to didier-wenzek/thin-edge.io that referenced this pull request Jun 9, 2023
See: thin-edge#1622

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek added a commit to didier-wenzek/thin-edge.io that referenced this pull request Jun 9, 2023
See: thin-edge#1622

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Developer value
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants