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

Move tedge_to_te_converter to tedge-mapper #2272

Closed
wants to merge 1 commit into from

Conversation

Bravo555
Copy link
Contributor

Proposed changes

Having tedge_to_te_converter be a part of tedge-agent introduces 2 problems:

  1. If tedge-agent were to be disabled, then MEA topics would no longer be mapped. As tedge-agent is not mandatory for the connection to the cloud to be established, it's reasonable to assume a user who doesn't need the functionality tedge-agent provides to disable it, at which point the converter would not work.

  2. tedge-agent is not able to map its own health status messages after it's turned off (which it reasonably could be). Once tedge-agent is updated to work with new health topics while maintaining backward compatibility by converting new health messages back to old topic scheme tedge/health/..., one could see an MQTT message log like this:

    (tedge-agent starts)
    - PUB te/device/main/service/tedge-agent/status/health "UP" (published by tedge-agent)
    - PUB tedge/health/tedge-agent "UP" (published by tedge_to_te_converter)
    (tedge-agent exits)
    - PUB te/device/main/service/tedge-agent/status/health "DOWN" (published by tedge-agent as last will message)
    - (DOWN message on tedge/health/tedge-agent NOT published by tedge_to_te_converter, because it's exited)
    

    leading to an unconsistent state where under a new topic scheme the service is DOWN, but under old topic scheme it's still UP.

    Having in mind that service status is reported by tedge-mapper to the cloud, and tedge-mapper still uses old health topics, it would be good to have it work while the transistion to new topics is still ongoing.

As such, the topic converter was moved into tedge-mapper, because it's the main thin-edge component, responsible for the connection to the cloud and configuration of mosquitto. Moving topic converter solves both above issues because 1. without connecting to the cloud mosquitto is disabled anyway, and 2. after tedge-mapper is disabled, the topic conversion and service status reporting both stop, so there is no opportunity for any inconsistency.

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

Having tedge_to_te_converter be a part of tedge-agent introduces 2
problems:

1. If tedge-agent were to be disabled, then MEA topics would no longer
   be mapped. As tedge-agent is not mandatory for the connection to the
   cloud to be established, it's reasonable to assume a user who doesn't
   need the functionality tedge-agent provides to disable it, at which
   point the converter would not work.
2. tedge-agent is not able to map its own health status messages after
   it's turned off (which it reasonably could be). Once tedge-agent is
   updated to work with new health topics while maintaining backward
   compatibility by converting new health messages back to old topic
   scheme `tedge/health/...`, one could see an MQTT message log like
   this:

   ```
   (tedge-agent starts)
   - PUB te/device/main/service/tedge-agent/status/health "UP" (published by tedge-agent)
   - PUB tedge/health/tedge-agent "UP" (published by tedge_to_te_converter)
   (tedge-agent exits)
   - PUB te/device/main/service/tedge-agent/status/health "DOWN" (published by tedge-agent as last will message)
   (DOWN message on tedge/health/tedge-agent NOT published by tedge_to_te_converter, because it's exited)
   ```

   leading to an unconsistent state where under a new topic scheme the
   service is DOWN, but under old topic scheme it's still UP.

   Having in mind that service status is reported by tedge-mapper to the
   cloud, and tedge-mapper still uses old health topics, it would be
   good to have it work while the transistion to new topics is still
   ongoing.

As such, the topic converter was moved into tedge-mapper, because it's
the main thin-edge component, responsible for the connection to the
cloud and configuration of mosquitto. Moving topic converter solves both
above issues because 1. without connecting to the cloud mosquitto is
disabled anyway, and 2. after tedge-mapper is disabled, the topic
conversion and service status reporting both stop, so there is no
opportunity for any inconsistency.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@github-actions
Copy link
Contributor

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
269 4 5 273 98.53 54m58.566999999s

Failed Tests

Name Message ⏱️ Duration Suite
Prerequisite Child DeviceNotFound 38.290 s Firmware Operation Child Device
Child device firmware update ValueError: Unable to perform POST request. Status: 422 Response: {"error":"undefined/validationError","message":"Following mandatory fields should be included: deviceId","info":"https://www.cumulocity.com/guides/reference/rest-implementation//#a-name-error-reporting-a-error-reporting&#34;} 1.243 s Firmware Operation Child Device
Child device firmware update with cache ValueError: Unable to perform POST request. Status: 422 Response: {"error":"undefined/validationError","message":"Following mandatory fields should be included: deviceId","info":"https://www.cumulocity.com/guides/reference/rest-implementation//#a-name-error-reporting-a-error-reporting&#34;} 1.394 s Firmware Operation Child Device
Converter and file transfer service are not running on a child device Matching messages is less than minimum. wanted: 1 got: 0 messages: [] 31.454 s Tedge Agent

Copy link
Contributor

@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.

The code is correct (i.e. moving the topic converter from the agent to the mapper).
So I'm okay to have this PR merged. However, the decision is up to @reubenmiller, as this is impacts the users.

That said, I agree with the first argument for this change (the agent might not be used on all installations) but not with the second one (the issue on tedge/health). I think what is wrong is #2271: the converter should not convert te topics to tedge topics, as the plan is to deprecate the latter.

Copy link
Contributor

@PradeepKiruvale PradeepKiruvale left a comment

Choose a reason for hiding this comment

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

Code changes look good to me. But, I will leave it to @didier-wenzek & @reubenmiller to decide where to put it, either in mapper or agent.

@Bravo555
Copy link
Contributor Author

Closing because as @reubenmiller said, it's possible to have multiple mappers running, which make a thin-edge instance connected to multiple clouds simultaneously, in which case moving the converter to the mapper would result in duplicating messages.

@Bravo555 Bravo555 closed this Sep 20, 2023
@Bravo555 Bravo555 deleted the move-converter-to-mapper branch August 1, 2024 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants