C8y mapping of entity registration messages

[albinsuresh]

Proposed changes

Cumulocity mapping for:

• Child device registration
• Service registration
• Nested child device/service registration
• System tests

Deferred for later:

• Inventory updates for custom fragments
• Auto-registration for services
• Avoid resending all entity creation messages to c8y on restart

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

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

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
294	0	3	294	100	1h6m35.549999999s

[reubenmiller]

@albinsuresh I've been trying out the new settings and things mostly work without the below minor exceptions:

• name is used instead of the displayName property defined in the MQTT spec. I'm ok with using name over displayName however then we need to update the spec to the implementation
• The Cumulocity IoT external identities for both the child devices and services are missing the common name prefix (which reduces naming conflicts between child devices on different tedge instances).
  • child-device: device:TST_jawbone_direct_wood -> <common_name>:device:TST_jawbone_direct_wood
  • service: device:TST_jawbone_direct_wood:service:custom-app -> <common_name>:device:TST_jawbone_direct_wood:service:custom-app

[codecov]

Codecov Report

Merging #2266 (cdb1143) into main (bbdc417) will increase coverage by 0.4%.
Report is 8 commits behind head on main.
The diff coverage is 91.8%.

Additional details and impacted files

Files	Coverage Δ
.../tedge_config/src/tedge_config_cli/tedge_config.rs	89.8% <100.0%> (ø)
crates/core/c8y_api/src/smartrest/inventory.rs	100.0% <100.0%> (ø)
crates/core/tedge_api/src/event.rs	95.1% <100.0%> (ø)
crates/core/tedge_api/src/mqtt_topics.rs	82.5% <100.0%> (+0.6%)	⬆️
crates/extensions/c8y_mapper_ext/src/config.rs	53.9% <100.0%> (+0.4%)	⬆️
crates/extensions/c8y_mapper_ext/src/error.rs	0.0% <ø> (ø)
crates/extensions/c8y_mapper_ext/src/serializer.rs	81.2% <100.0%> (ø)
crates/core/c8y_api/src/smartrest/topic.rs	88.0% <93.7%> (+1.7%)	⬆️
crates/extensions/c8y_mapper_ext/src/log_upload.rs	76.5% <86.6%> (-0.1%)	⬇️
crates/core/tedge_api/src/entity_store.rs	96.4% <98.1%> (+5.5%)	⬆️
... and 4 more

... and 18 files with indirect coverage changes

[albinsuresh]

• name is used instead of the displayName property defined in the MQTT spec. I'm ok with using name over displayName however then we need to update the spec to the implementation

Stuck with name over displayName as thin-edge on its own doesn't really have a "display" aspect to it by default, and that word felt more appropriate for the UI aspects of the cloud. So, name sounds more "agnostics" to my ears ;) And it was more consistent with type as well. Updated the specs accordingly.

• The Cumulocity IoT external identities for both the child devices and services are missing the common name prefix (which reduces naming conflicts between child devices on different tedge instances).

  • child-device: device:TST_jawbone_direct_wood -> <common_name>:device:TST_jawbone_direct_wood
  • service: device:TST_jawbone_direct_wood:service:custom-app -> <common_name>:device:TST_jawbone_direct_wood:service:custom-app

Fixed by appending the the main device's device_id to the generated external IDs of entities.

[didier-wenzek]

The duplications with the EntityStore must be resolved.

[Bravo555]

No issues with the code, just requesting some additional comments

[didier-wenzek]

The main comments from my previous review have been addressed. I still have some comments - notably about the entity_store.update() method.

[didier-wenzek]

One needs to check what are the impacts of using the new external ids on the c8y topics.

[didier-wenzek]

This is the drawback of using short child device names in path: one has to assume a default topic scheme.

Ideally, I would use the same hierarchy for paths as for MQTT. i.e. with a device directory and there a sub directory per child plus a main directory. Or an a/b/c/d tree if the user prefer its own schema.

[albinsuresh]

This is the drawback of using short child device names in path: one has to assume a default topic scheme.

Since we're doing this for a legacy mechanism which assumed such things, isn't safe to make the assumptions here as well for the sake of consistency?

Ideally, I would use the same hierarchy for paths as for MQTT. i.e. with a device directory and there a sub directory per child plus a main directory. Or an a/b/c/d tree if the user prefer its own schema.

Yeah, we could do that if we are going to continue supporting this file system based entity creation mechanism for much longer. But, my understanding was that it's not gonna be around for much longer and hence there's no point in extending that further by supporting hierarchical nested child devices and all. So, if we really want to extend this API, we could do that in a follow-up PR.

[didier-wenzek]

we could do that in a follow-up PR

Sure. The scope of this PR is already unexpectedly growing. We need to focus on what is definitely wrong. We will later address this kind of inconsistencies.

[albinsuresh]

Follow-up tasks:

• Sanitise external_id provided via @id value in the registration message
• Review file system based child device registration API. Should the directory name be used as-is as the child device external id? How to support addition of more operations when someone is already using the default topic scheme and hence the default external_id generation logic. How to support this along with custom topic schemes?
• Avoid re-processing of auto-registered entity registration messages
• Avoid re-processing of all the retained entity registration messages on mapper restart. Use file system persistence to identify the delta? Or re-use the message sync mechanism of alarms (too complex)?
• Simplify smartrest::topic module by keeping publish_topic_from_ancestors and removing redundant helpers
• Remove the hard-coded MQTT_ROOT that is used by EntityRegistrationMessage::new().
• Create entity registration messages for child devices and services present in the cloud, but not registered locally, on mapper startup (syncing cloud state to local)

[didier-wenzek]

Approved. Thank you!