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

NOISSUE - Make MQTT Broker Configurable #1904

Merged
merged 15 commits into from
Oct 19, 2023

Conversation

rodneyosodo
Copy link
Member

@rodneyosodo rodneyosodo commented Sep 1, 2023

Signed-off-by: Rodney Osodo socials@rodneyosodo.com

What does this do?

This PR makes the MQTT broker configurable either as VerneMQ or NATS. NATS is a lightweight and highly performant message broker that offers MQTT support, making it a suitable replacement for VerneMQ in this context.

Which issue(s) does this PR fix/relate to?

No issue

List any changes that modify/break current functionality

  1. Add docker profiles to switch between different modes. The available profiles are:

    • vernemq_nats: Uses vernemq as MQTT_BROKER and nats as MESSAGE_BROKER.
    • vernemq_rabbitmq: Uses vernemq as MQTT_BROKER and rabbitmq as MESSAGE_BROKER.
    • nats_nats: Uses nats as both MQTT_BROKER and MESSAGE_BROKER.
    • nats_rabbitmq: Uses nats as MQTT_BROKER and rabbitmq as MESSAGE_BROKER.
  2. Integration of NATS as the MQTT broker.

Have you included tests for your changes?

Tested manually

Did you document any new/modified functionality?

absmach/magistrala-docs#159

Notes

To be merged after https://github.com/mainflux/mainflux/pull/1903 and https://github.com/mainflux/mainflux/pull/1902

  • NATS follows MQTT v3.1.1 specification
  • Limitations regarding Nats MQTT support can be found here

How to test

  1. NATS as MQTT Broker and NATS as Message Broker

    MF_MESSAGE_BROKER_TYPE=nats MF_MQTT_BROKER_TYPE=nats make run up args="-d"
    MF_MESSAGE_BROKER_TYPE=nats MF_MQTT_BROKER_TYPE=nats make run_addons up args="-d"
  2. VerneMQ as MQTT Broker and NATS as Message Broker

    MF_MESSAGE_BROKER_TYPE=nats MF_MQTT_BROKER_TYPE=vernemq make run up args="-d"
    MF_MESSAGE_BROKER_TYPE=nats MF_MQTT_BROKER_TYPE=vernemq make run_addons up args="-d"
  3. NATS as MQTT Broker and RabbitMQ as Message Broker

    MF_MESSAGE_BROKER_TYPE=rabbitmq MF_MQTT_BROKER_TYPE=nats make run up args="-d"
    MF_MESSAGE_BROKER_TYPE=rabbitmq MF_MQTT_BROKER_TYPE=nats make run_addons up args="-d"
  4. VerneMQ as MQTT Broker and RabbitMQ as Message Broker

    MF_MESSAGE_BROKER_TYPE=rabbitmq MF_MQTT_BROKER_TYPE=vernemq make run up args="-d"
    MF_MESSAGE_BROKER_TYPE=rabbitmq MF_MQTT_BROKER_TYPE=vernemq make run_addons up args="-d"

@rodneyosodo rodneyosodo requested a review from a team as a code owner September 1, 2023 10:13
@rodneyosodo rodneyosodo marked this pull request as draft September 1, 2023 10:13
@rodneyosodo rodneyosodo force-pushed the noissue-replace-vernemq branch 2 times, most recently from a8c367f to aca105d Compare September 1, 2023 11:28
@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #1904 (e9432fa) into master (28f4965) will increase coverage by 0.00%.
Report is 19 commits behind head on master.
The diff coverage is 65.00%.

❗ Current head e9432fa differs from pull request most recent head 87c3384. Consider uploading reports for the commit 87c3384 to get more accurate results

@@           Coverage Diff           @@
##           master    #1904   +/-   ##
=======================================
  Coverage   67.14%   67.14%           
=======================================
  Files         120      120           
  Lines        9145     9144    -1     
=======================================
  Hits         6140     6140           
  Misses       2368     2368           
+ Partials      637      636    -1     
Files Coverage Δ
pkg/messaging/mqtt/pubsub.go 75.00% <80.00%> (+1.36%) ⬆️
mqtt/handler.go 91.93% <71.42%> (-0.07%) ⬇️
pkg/messaging/mqtt/publisher.go 23.80% <33.33%> (-1.20%) ⬇️
pkg/messaging/rabbitmq/pubsub.go 69.04% <60.00%> (+0.37%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rodneyosodo rodneyosodo marked this pull request as ready for review September 1, 2023 11:56
@rodneyosodo rodneyosodo marked this pull request as draft September 1, 2023 12:24
@rodneyosodo rodneyosodo force-pushed the noissue-replace-vernemq branch 4 times, most recently from da756a1 to 9ac365a Compare September 6, 2023 11:16
@rodneyosodo rodneyosodo changed the title NOISSUE - Replace VerneMQ With Nats NOISSUE - Make MQTT Broker Configurable Sep 6, 2023
@rodneyosodo rodneyosodo force-pushed the noissue-replace-vernemq branch 2 times, most recently from 81f7572 to 81eca50 Compare September 6, 2023 13:51
@rodneyosodo rodneyosodo marked this pull request as ready for review September 6, 2023 14:41
@@ -419,8 +419,6 @@ func TestPubSub(t *testing.T) {
assert.Equal(t, expectedMsg.Channel, receivedMsg.Channel, fmt.Sprintf("%s: expected %+v got %+v\n", tc.desc, &expectedMsg, receivedMsg))
assert.Equal(t, expectedMsg.Created, receivedMsg.Created, fmt.Sprintf("%s: expected %+v got %+v\n", tc.desc, &expectedMsg, receivedMsg))
assert.Equal(t, expectedMsg.Protocol, receivedMsg.Protocol, fmt.Sprintf("%s: expected %+v got %+v\n", tc.desc, &expectedMsg, receivedMsg))
assert.Equal(t, expectedMsg.Publisher, receivedMsg.Publisher, fmt.Sprintf("%s: expected %+v got %+v\n", tc.desc, &expectedMsg, receivedMsg))
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these removed

Copy link
Member Author

Choose a reason for hiding this comment

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

The published message only contains channel and payload

@@ -55,19 +55,19 @@ var channelRegExp = regexp.MustCompile(`^\/?channels\/([\w\-]+)\/messages(\/[^?]

// Event implements events.Event interface.
type handler struct {
publishers []messaging.Publisher
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove support for mutliple publishers

Copy link
Member Author

@rodneyosodo rodneyosodo Sep 19, 2023

Choose a reason for hiding this comment

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

It was not being utilized. We only used passed one publisher.

pkg/messaging/mqtt/pubsub.go Outdated Show resolved Hide resolved
@rodneyosodo rodneyosodo force-pushed the noissue-replace-vernemq branch 9 times, most recently from c60aeab to 3989996 Compare October 19, 2023 09:18
rodneyosodo and others added 15 commits October 19, 2023 20:59
Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The Makefile has been updated to support custom Docker profiles.
Previously, the Makefile only supported the default profiles for the
message broker and MQTT broker. Now, the Makefile allows for custom
profiles to be specified using environment variables. If the
MF_BROKER_TYPE or MF_MQTT_BROKER_TYPE variables are not set, the
default values "nats" and "nats" will be used, respectively. This
change enables more flexibility in configuring the Docker environment
for the project.

The `run` target has also been modified to use the correct broker
configuration file based on the MF_BROKER_TYPE variable. The sed
command in the `run` target now replaces the placeholder in the
docker/docker-compose.yml file with the appropriate broker
configuration file.

This commit improves the Makefile to support custom Docker profiles
and ensures the correct broker configuration file is used when
running the project.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The commit fixes an issue in the RabbitMQ pubsub implementation where the queue binding was not correctly set up. Instead of using the topic as the queue name, the commit now uses a unique client ID generated by combining the topic and subscriber ID. This ensures that each subscriber has its own dedicated queue. The commit also updates the queue binding to use the correct queue name.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The commit refactors the `edit_docker_config` function in the Makefile to improve readability and maintainability. The changes include:

- Removing unnecessary conditionals related to the `rabbitmq` broker

These changes ensure that the Docker configuration is correctly updated based on the specified MQTT broker type.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The MQTT_BROKER comment in the docker-compose.yml file has been updated to provide a more accurate description of its functionality. The comment now states that the MQTT_BROKER handles MQTT communication between MQTT adapters and the message broker, instead of Mainflux services. This change improves clarity and aligns with the actual purpose of the MQTT_BROKER.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The Makefile and Semaphore configuration files have been refactored to update the variable names related to the message broker type.

These changes ensure consistency and clarity in the codebase by using more descriptive variable names related to the message broker type.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Update the Docker profile configuration for nats_rabbitmq by replacing the NATS URL in the .env file with the correct value.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The MQTT QoS level in the pubsub.go file was set to 1, which is the
default level. However, since NATS supports up to QoS 1, I updated the
QoS level comment to reflect this.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The NewPublisher function in the pkg/messaging/mqtt/publisher.go file has been refactored to accept a new parameter, qos, which represents the Quality of Service level for MQTT message publishing. This change allows for more flexibility in configuring the MQTT publisher.

The NewPublisher function now has the following signature:

```go
func NewPublisher(address string, qos uint8, timeout time.Duration) (messaging.Publisher, error)
```

This change ensures that the MQTT publisher can be created with the desired QoS level, enhancing the reliability and delivery guarantees of the published messages.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
The test assertions in the pubsub_test.go file were incorrect. This commit fixes the assertions to properly compare the expected and received message values.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Signed-off-by: rodneyosodo <blackd0t@protonmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit 790f8a6 into absmach:master Oct 19, 2023
1 of 2 checks passed
@rodneyosodo rodneyosodo deleted the noissue-replace-vernemq branch October 22, 2024 08:13
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.

4 participants