MQTT twin/ topic support for Cumulocity inventory updates

[reubenmiller]

Proposed changes

PR to communicate the proposed mapping of the /twin/ topics including integration tests. This PR can be used for the actual implementation.

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

#2279

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
321	0	3	321	100	50m53.260999999s

[didier-wenzek]

It would be really nice to use this PR for pushing the implementation along docs and tests.

I see two points that really needs to be clarified:

• How this feature is combined with the inventory fragments extracted from tedge-config and inventory.json
• The local consolidation of all the fragments. It's a good idea to persist this aggregated view, but I would really avoid to use the former inventory.json file for that. We need to be clear what is the source of truth: the persisted view of the inventory must only be edited by the mapper after data collected from the config and over MQTT.

[didier-wenzek]

Nice to have tests along the specs ♥

This one makes me understand why a type fragment is an issue as already defined by tedge config get device.type. But, don't we have to address with a broader view these conflicts with legacy features (inventory.json and tedge config)?

[albinsuresh]

The inventory.json contents can be parsed and any direct key-value pairs can be published to the meta/key topic, with the value as the payload. The same can be extended to tedge config as well, where the value would be just be the plain value instead of a JSON object, as you suggested.

[reubenmiller]

We could go the layered approach and first parse the inventory.json file and use that as the base model, and then apply any changes from the MQTT topics. The inventory.json is good for being able to hardcode static information into a build, however we could also just phase the file out (behind a feature flag or something).

[albinsuresh]

@reubenmiller So, no need to re-publish the contents of the file as twin messages to the broker? Without that, the other components relying on the broker for the inventory information will never get that "base model", right? Unless they all parse that file.

[codecov]

Codecov Report

Merging #2280 (d5f35fc) into main (f811fad) will increase coverage by 0.1%.
The diff coverage is 86.5%.

Additional details and impacted files

Files	Coverage Δ
.../tedge_config/src/tedge_config_cli/tedge_config.rs	81.8% <100.0%> (ø)
crates/core/tedge_api/src/mqtt_topics.rs	88.2% <100.0%> (+0.1%)	⬆️
crates/extensions/c8y_mapper_ext/src/lib.rs	87.5% <ø> (ø)
crates/extensions/c8y_mapper_ext/src/tests.rs	92.6% <98.9%> (+0.2%)	⬆️
crates/extensions/c8y_mapper_ext/src/config.rs	48.8% <60.0%> (+0.4%)	⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs	79.6% <81.8%> (-0.1%)	⬇️
crates/extensions/c8y_mapper_ext/src/inventory.rs	82.7% <82.7%> (ø)

... and 5 files with indirect coverage changes

[didier-wenzek]

I still have to review inventory.rs. So far things look really nice.

I'm missing doc comments in inventory.rs though. This makes things a bit harder to understand/check as one has to compare each implementation with its uses.

[didier-wenzek]

I really like how things are structured, the inventory.json file being translated into a sequence of twin data messages which are then translated into c8y fragments exactly as any other twin data.

I have some questions about the protected keys (name and type):

• These keys are not protected when read from the inventory.json file.
• c8y_Agent seems to be the key needing to be protected form override, not name.

Once these two points clarified, I will be happy to approve.

[didier-wenzek]

Why is the name property forbidden? Looking at the code, c8y_Agent is the fragment that must be protected from overriding.

[albinsuresh]

The name and type fields are forbidden here because they're already part of the entity registration message. If someone wants to update these, they must send another registration message with the updated values. Allowing them to update the same data via both these endpoints will might be confusing especially in the broker as we may have different name and type in the device registration topic as well as in the twin/... topics.

I didn't really understand why you think c8y_Agent must not be overridden?

[didier-wenzek]

The name and type fields are forbidden here because they're already part of the entity registration message. If someone wants to update these, they must send another registration message with the updated values. Allowing them to update the same data via both these endpoints will might be confusing especially in the broker as we may have different name and type in the device registration topic as well as in the twin/... topics.

Okay

I didn't really understand why you think c8y_Agent must not be overridden?

What if a user publish its own fragment on te/device/main///twin/c8y_Agent ?

[albinsuresh]

What if a user publish its own fragment on te/device/main///twin/c8y_Agent

I don't really have a use-case for someone wanting to update that fragment for the thin-edge device, as we set that. But I don't have any strong arguments to close that option either. So, will need @reubenmiller 's advise on this. For child devices, we probably wouldn't want to prevent them from setting it. For services, this fragment probably doesn't even make any sense.

[didier-wenzek]

What if a user publish its own fragment on te/device/main///twin/c8y_Agent

I don't really have a use-case for someone wanting to update that fragment for the thin-edge device, as we set that. But I don't have any strong arguments to close that option either. So, will need @reubenmiller 's advise on this. For child devices, we probably wouldn't want to prevent them from setting it. For services, this fragment probably doesn't even make any sense.

Indeed, this would make no sense, but could be done by a user who fails to notice that this is already done by thin-edge as for the type. I that case there will be inconsistencies.

[didier-wenzek]

Approved. Nice feature and implementation!

[albinsuresh]

Test Plan

The new twin topics and their mapping contracts are documented in the mqtt-api.md and c8y-mapper.md docs included in this PR. The basic feature is already covered in the system tests updated in the same PR: child_device_telemetry.robot and thin-edge_device_telemetry.robot. Tests for clearing inventory fragments will be added in a follow-up PR soon, as some Cumulocity library updates are required for it.

[gligorisaev]

QA has thoroughly checked the feature and here are the results:

• Test for ticket exists in the test suite.
• tests/RobotFramework/tests/cumulocity/telemetry/child_device_telemetry.robot
• tests/RobotFramework/tests/cumulocity/telemetry/thin-edge_device_telemetry.robot
• tests/RobotFramework/tests/tedge/call_tedge_config_list.robot
• QA has tested the bug and could not reproduce it anymore.