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

feat: enable mosquitto persistence by default #3232

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reubenmiller
Copy link
Contributor

@reubenmiller reubenmiller commented Nov 6, 2024

Proposed changes

Open Questions

  • Should the logic be moved to the install script so that it can check if the user has configured the persistence true or not and configured the path (with the correct permissions set).

Enable persistence as thin-edge.io heavily relies on retained messages and if mosquitto persistence is not configured, then it can lead to unexpected behavior.

For example (assuming mosquitto persistence is not enabled), if the tedge-agent has already started, and then the user runs tedge connect c8y, then any previously retained message published by tedge-agent will not be republished, and the tedge-mapper-c8y will not know of the previously published retained messages...this leads to supported operations not being sent to the cloud.

Given the impact of not having mosquitto persistence configured, it should not be left up to the user to configure it.

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

@reubenmiller reubenmiller changed the title feat: enable mosquitto persistence feat: enable mosquitto persistence by default Nov 6, 2024
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...s/core/tedge/src/bridge/common_mosquitto_config.rs 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@reubenmiller reubenmiller added the theme:mqtt Theme: mqtt and mosquitto related topics label Nov 6, 2024
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
529 0 2 529 100 1h41m1.404084s

@reubenmiller
Copy link
Contributor Author

Though I'm just reviewing one aspect regarding the persistence_location (https://mosquitto.org/man/mosquitto-conf-5.html).

If non is defined it will choose the current location, but this may be problematic as I don't see the mosquitto service defining the default working directory, which means the current working directory maybe somewhere that the mosquitto user does not have permission to write to.

persistence_location path
The path where the persistence database should be stored. If not given, then the current directory is used.

@albinsuresh @didier-wenzek Any thoughts on this? Should we try to set the default path ourselves? Or even consider creating our own mosquitto configuration package?

Comment on lines +113 to +115
// Enable persistence as thin-edge.io heavily relies on retained messages and
// if mosquitto persistence is not configured, then it can lead to unexpected behavior
writeln!(writer, "persistence true")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware that the file /etc/tedge/mosquitto-conf/tedge-mosquitto.conf is populated only if there is at least one cloud connected.

I would consider to improve the tedge refresh-bridges command to populate this file even if there is no bridge configured. I.e. moving these 2 lines before that one

@albinsuresh
Copy link
Contributor

albinsuresh commented Nov 8, 2024

@albinsuresh @didier-wenzek Any thoughts on this? Should we try to set the default path ourselves? Or even consider creating our own mosquitto configuration package?

Can we not write a new config file with these persistence settings into the /etc/mosquitto/conf.d directory once tedge is installed or as part of tedge init and not wait for tedge connect? Yes, there is the risk of conflicting persistence settings, if the user already has persistence turned on in some other configuration file. But, we can at least check the main /etc/mosquitto/mosquitto.conf file to see if it is already turned ON, before adding ours.

@reubenmiller
Copy link
Contributor Author

@albinsuresh @didier-wenzek Any thoughts on this? Should we try to set the default path ourselves? Or even consider creating our own mosquitto configuration package?

Can we not write a new config file with these persistence settings into the /etc/mosquitto/conf.d directory once tedge is installed or as part of tedge init and not wait for tedge connect? Yes, there is the risk of conflicting persistence settings, if the user already has persistence turned on in some other configuration file. But, we can at least check the main /etc/mosquitto/mosquitto.conf file to see if it is already turned ON, before adding ours.

Ok, I'm going to move the state of this PR to draft until we work out a way forward.

@reubenmiller reubenmiller marked this pull request as draft November 12, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:mqtt Theme: mqtt and mosquitto related topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants