-
Notifications
You must be signed in to change notification settings - Fork 70
PoC: Separate mapper configuration from tedge.toml #3851
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
base: main
Are you sure you want to change the base?
PoC: Separate mapper configuration from tedge.toml #3851
Conversation
6930f5e to
723457a
Compare
|
The separation of mapper configs from the main Where the I hope that the TOML schema enforcement becomes easier for different clouds as there is a clear correlation between the names of the config files/directories and their types. WDYT? |
640e93e to
339e442
Compare
Robot Results
|
The reason why the current design defines the paths and associated cloud types in I'm not entirely sure how to handle different profiles at the moment. With the current design, I think a possible solution to this is that we make profile names required, so |
395bda5 to
037cd97
Compare
I have now implemented this |
An intermediate |
|
I like a lot the new way to configure mappers and profiles using independent files. However, it would be good to raise at least a warning when a non-profiled c8y configuration is given both in |
|
|
||
| /// Base mapper configuration with common fields and cloud-specific fields via generics | ||
| #[derive(Debug, Document)] | ||
| pub struct MapperConfig<T: SpecialisedCloudConfig> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this generic representation of a MapperConfig with the cloud specific type wrapped as a generic type is worth the complexity of all the indirections, esp with the current design that keeps each cloud mapper config in their own "named" files and directories. Why not let the mappers parse their config files into cloud specific mapper config types like C8yMapperConfig, AzMapperConfig and even a GenericMapperConfig directly rather than going thorugh tedge_config::mapper_config() that returns this generic type? These common fields like url, root_cert_path etc can be captured into a BaseMapperConfig (or even the GenericMapperConfig itself) and wrapped into the cloud specific ones as a flattened field so that each toml file can be directly parsed into one of these config types.
In my simplistic view, the TEdgeConfig struct doesn't really have to read all mapper configs and keep them cached, as every mapper instance is only interested in the common tedge configurations and the cloud config for its own type and profile. So, let the mappers parse the cloud config files that are relevant for its type and profile directly, rather than letting tedge_config parse everything into generic MapperConfig instances and have the mappers looking them up later on.
With such a scheme, the tedge_config layer just needs to act like the router between the cloud specific config types and their corresponding file locations (or TOML tables in the old config scheme), along with the magic for device.id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While looking at tedge_config only from the mapper process's PoV, may be I've oversimplified the requirements here, without fully appreciating the complexities of having a shared definition of the config structs between the tedge config cli utility as well as the consumers of the configs like the mapper.
7acd92e to
af371f1
Compare
4a773dd to
828690a
Compare
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
…ed mapper config Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
…per configs Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
828690a to
c254858
Compare
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
| tokio_stream::wrappers::ReadDirStream::new( | ||
| tokio::fs::read_dir(self.mappers_config_dir().join(format!("{ty}.d"))) | ||
| .await | ||
| .unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a complete mapper config hierarchy should not be mandatory.
$ sudo tedge-mapper c8y
2025-11-28T09:26:46.883035301Z INFO Runtime: Started
2025-11-28T09:26:46.883403123Z INFO Runtime: Running Signal-Handler-0
2025-11-28T09:26:46.883467269Z INFO Runtime: Running HealthMonitorActor-1
thread 'main' panicked at /home/didier/production/thin-edge.io/crates/common/tedge_config/src/tedge_toml/tedge_config.rs:316:30:
called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ fd . /etc/tedge/mappers
/etc/tedge/mappers/c8y.d/
/etc/tedge/mappers/c8y.d/foo.toml
/etc/tedge/mappers/c8y.toml
The fix is simple but should not be mandatory:
$ mkdir /etc/tedge/mappers/az.d
$ mkdir /etc/tedge/mappers/aws.d
$ sudo systemctl start tedge-mapper-c8y
$ sudo systemctl status tedge-mapper-c8y
Active: active (running) since Fri 2025-11-28 10:43:50 CET; 3min 29s ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this turned out to be a bug where we always consulted the c8y entry for where the configuration lies, so something was determining the az or aws configuration, discovering that c8y is migrated to the new format, and attempting to read the az/aws configuration from the new format. That's easy to fix.
However, this does raise an additional point about whether c8y.d must exist to read c8y.toml. I think the answer is "no, we should cope with the profiles directory not existing" (even if we automatically create it regardless of whether profiles are in use when performing a hypothetical automated migration from tedge.toml to separate mapper config in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, this does raise an additional point about whether
c8y.dmust exist to readc8y.toml. I think the answer is "no, we should cope with the profiles directory not existing"
I agree and also think thin-edge should not mandate a complete file hierarchy when a single configuration file is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now fixed the original bug, and improved the error message in the case of the default profile existing but not the profiled-configs directory. I'm still working on actually preventing the error entirely, though.
Proposed changes
An alternative solution to generalised mapper configuration (original proposal is in #3835).
This separates the mapper configurations entirely from tedge.toml.
TODO:
CloudConfigimplementations forTEdgeConfigReaderC8yetc.TryFrom<...> for BridgeConfigC8yParamsbridge.include.local_cleansessiona configuration for all cloudsbridge.include.local_cleansessionfor non c8y cloudsmappersconfig directory with a single source of truthmapper.urlshould be privatec8yand generalised configsderive(Deserialize)in mapper configtedge connectvalidations for unique proxy bind portsExample configuration
To use the feature, you need to create a mapper config file. In
/etc/tedge/mappers/c8y.toml, you add thec8yfields fromtedge.tomlIf you want to try it out,
tedge-mapper c8ywill read from the new configuration if it exists. For profiles, the config goes in/etc/tedge/mappers/c8y.d/profile-name.toml.Code changes
The bulk of the code this PR touches at the moment is in
mapper_config.rs. This is essentially extracting the mapper related configurations from tedge.toml, allowing us to deserialise the configuration. It doesn't add any support for editing the mapper configuration file, so for the moment this will need to be done by hand. I anticipate adding some CLI support for the mapper configuration file later, but this shouldn't have a major impact on the more critical changes made by this PR.Most of
mapper_config.rswas written by Claude. It could do with some restructuring/splitting out, my focus was to get the infrastructure in place to allow the existing mapper code to read the new configuration. The other changes, at least as of the first commit are to supporttedge-mapper c8y. As of the first commit,tedge-mapper c8ysources its configuration frommapper.config_path(with no profile set). I will need to add a compatibility layer later.Differences between the two proposals
The "mapper config in tedge.toml" proposal (#3835) stored the "specialised" (e.g. c8y-specific) configurations inside a
c8ysub-field, so you would set e.g.mapper.profiles.c8y.c8y.proxy.bind_port. This proposal keeps the existing hierarchy, so the cloud-specific fields are at the same level of the configuration as everything else in the toml file (e.g.proxy.bind.portabove is at the top-level like everything else).When deserialising the old configuration, the specialised mapper configuration was read as an enum. This is a pain when handling the configuration since you have to pass the specialised configuration around in addition to the mapper config that contains it. By comparison, the new approach uses generics to handle the specialised mapper configurations. This means the c8y mapper can check for the given profile that
mapper.type = "c8y", then deserialise with only the c8y specialisations supported.Previous solution:
This solution:
Types of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments