Added tests for MQTT retained messages in various cluster/domain conf… #5082

levb · 2024-02-14T13:34:10Z

This is a Test- and CI-only PR. No server changes.

…igurations

.github/workflows/MQTT_test.yaml

levb · 2024-02-15T17:32:32Z

server/mqtt_ex_test_test.go

+			target := topo.makef(t)
+			t.Cleanup(target.Shutdown)
+
+			// TODO (levb): if we do not pre-initialize the streams and


NS: cc @derekcollison @kozlovic This test uncovered a rather inconsistent behavior in multi-domain scenarios. If there are 2 domains, A and B, and the MQTT JetStream assets are first initialized in A, B will only receive and replay retained messages after its own retained message assets are created. Touching the servers with an MQTT connection here ensures the success of the tests; otherwise A<->B fail 1/2 of the time.

Not quite sure if this is "by design".

I would think that if they have different domains, messages should not even be visible in the other domain, but I have to say I don't really understand that concept. What I notice is that you don't specify js_domain: in the mqtt{} block of the config, but if I try to add those, then the test fails early since the lookup of a stream fails with a timeout since the JS APIs are rewritten with the domain name, like $JS.HUBD.API.STREAM.INFO.$MQTT_sess and for some reason, the server does not listen to those, even though you have specified domain in the JS config too. I don't really see in JS code where the server listens to that...

you don't specify js_domain: in the mqtt{} block of the config

👍 I did not (and still do not) fully understand the intended functionality of this option, would love to add tests for it. Will read the code and try to understand better. The customer that was having issues did not have it set, so I did not prioritize it.

…-test-retained

#5082 was supposed to fix this, but it is not reviewed yet. `mqtt-test` got updated; and the `@latest` reference is causing the new, incompatible, version to be used. This PR will fix it, until #5082 is merged.

kozlovic

Some comments, but unfortunately regarding the domains, I am not sure how that works and can't really help there.

kozlovic · 2024-02-16T16:24:55Z

server/mqtt_ex_test_test.go

+	for _, c := range t.clusters {
+		c.shutdown()
+	}
+	for _, s := range t.servers {


See my comment in Reload()

kozlovic · 2024-02-16T16:25:57Z

server/mqtt_ex_test_test.go

+	tb.Helper()
+
+	for _, c := range t.clusters {
+		c.restartAllSamePorts()


This a mistake that I use to make: restartAllSamePorts() does NOTHING if the cluster has not been shutdown first. Basically that code goes through servers in the cluster and restart IF they are not currently running. So you need to add c.shutdown() prior to that.

And adding that shows that TestMQTTExRetainedMessages/cluster are now failing...

Sorry, should call c.stopAll(), not c.shutdown() since in that case the config/store would be wiped out.

We need after the restart to add something like:

for _, stream := range []string{mqttSessStreamName, mqttStreamName, mqttQoS2IncomingMsgsStreamName, mqttOutStreamName, mqttRetainedMsgsStreamName} { c.waitOnStreamLeader("ONE", stream) }

Without something like that, then it is possible that a connect fails because one of the stream lookup fails with a timeout (since after a restart, it may take some time to elect a leader for those streams).

Ouch... fixing.

kozlovic · 2024-02-16T16:26:09Z

server/mqtt_ex_test_test.go

+
+	for i, s := range t.servers {
+		o := s.getOpts()
+		s.Shutdown()


Is it that mqttExTarget will either have t.clusters or t.servers set but not both? Or at least that t.servers will not be servers that are in t.clusters?

It's the case at the moment, but you're right that it may not be the case in the future (a cluster with a single leaf node immediately comes to mind).

TBH, this was just a "minimal viable" implementation for the current requirements, I didn't want to cut/paste nor to go too abstract here (it's just an "extra" test for the time being). If we keep using MQTTEx going forward more then yes, the "target" implementation should be refactored.

server/mqtt_ex_test_test.go

kozlovic · 2024-02-16T16:34:39Z

server/mqtt_ex_test_test.go

+	sub  []mqttExDial
+}
+
+func TestMQTTExCompliance(t *testing.T) {


We need to be able to exclude this file from running the normal tests, say from server repo something like:

go test -race -v -run=TestMQTT ./server

Now fails if you don't have all the things that you do in GithubActions setup. So you may want to use some build flags or something to exclude this file. Maybe something like:

//go:build !skip_mqtt_tests && run_mqtt_ex // +build !skip_mqtt_tests,run_mqtt_ex

Of course, that means adding -tags=run_mqtt_ex in the go command to run those tests.

I thought about it, but passed on a separate build tag. All the MQTTEx tests (should) do a t.Skip if mqtt-test is not installed in CI, so it's ok to invoke them in the main tag?

Right. I did not realize that this is what was happening. I installed mqtt-test, etc.. and added it to my path, but then when I was trying to run the "normal" MQTT tests, those 2 would run and I thought that was new.

ok, will separate.

server/mqtt_ex_test_test.go

kozlovic · 2024-02-16T17:16:39Z

server/mqtt_ex_test_test.go

+			target := topo.makef(t)
+			t.Cleanup(target.Shutdown)
+
+			// TODO (levb): if we do not pre-initialize the streams and


I would think that if they have different domains, messages should not even be visible in the other domain, but I have to say I don't really understand that concept. What I notice is that you don't specify js_domain: in the mqtt{} block of the config, but if I try to add those, then the test fails early since the lookup of a stream fails with a timeout since the JS APIs are rewritten with the domain name, like $JS.HUBD.API.STREAM.INFO.$MQTT_sess and for some reason, the server does not listen to those, even though you have specified domain in the JS config too. I don't really see in JS code where the server listens to that...

server/mqtt_ex_test_test.go

kozlovic · 2024-02-16T18:42:35Z

server/mqtt_ex_test_test.go

+	tb.Helper()
+
+	for _, c := range t.clusters {
+		c.restartAllSamePorts()


Sorry, should call c.stopAll(), not c.shutdown() since in that case the config/store would be wiped out.

…-test-retained

kozlovic

Still need to make sure that the cluster is really restarted. And I have found then that tests are quite flappy. I think you need to have to maybe use testMQTTConnectRetry() with a clean session and dummy ID (or no ID specified) until it succeeds before having the real test in mqttexRunTest(). Just waiting on the cluster to be ready proved not enough in tests I was running last week.

kozlovic · 2024-02-20T16:38:56Z

server/mqtt_ex_test_test.go

+	sub  []mqttExDial
+}
+
+func TestMQTTExCompliance(t *testing.T) {


Right. I did not realize that this is what was happening. I installed mqtt-test, etc.. and added it to my path, but then when I was trying to run the "normal" MQTT tests, those 2 would run and I thought that was new.

levb · 2024-02-20T16:47:50Z

Still need to make sure that the cluster is really restarted.

@kozlovic sorry my commit message was non-descriptive, I squashed from another branch. Anyway, I am still working on the restart a another potential issue I ran into, will mark you for review when done.

… init

Added tests for MQTT retained messages in various cluster/domain conf…

40aaa59

…igurations

levb requested a review from a team as a code owner February 14, 2024 13:34

levb requested review from wallyqs and Jarema February 14, 2024 13:42

levb commented Feb 14, 2024

View reviewed changes

.github/workflows/MQTT_test.yaml Outdated Show resolved Hide resolved

levb added 2 commits February 15, 2024 09:04

point to mqtt-test@v0.1

28dc3f7

point to mqtt-test@v0.1.0

23229f5

levb commented Feb 15, 2024

View reviewed changes

Merge branch 'main' of github.com:nats-io/nats-server into lev-mqttex…

daa8158

…-test-retained

levb mentioned this pull request Feb 15, 2024

Fixed incorrect mqtt-test reference (mqttex tests failure) #5094

Merged

kozlovic requested changes Feb 16, 2024

View reviewed changes

levb added 2 commits February 20, 2024 08:23

Merge branch 'main' of github.com:nats-io/nats-server into lev-mqttex…

1e0999d

…-test-retained

PR feedback

086fe59

kozlovic requested changes Feb 20, 2024

View reviewed changes

levb added 2 commits February 20, 2024 11:18

PR feedback: better Reload (and better use of it in the test)

4b73737

removed verbose test naming, adjustments to when to reload and how to…

843b19a

… init

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for MQTT retained messages in various cluster/domain conf… #5082

Added tests for MQTT retained messages in various cluster/domain conf… #5082

levb commented Feb 14, 2024 •

edited

levb Feb 15, 2024 •

edited

kozlovic Feb 16, 2024

levb Feb 20, 2024 •

edited

kozlovic left a comment

kozlovic Feb 16, 2024

kozlovic Feb 16, 2024

kozlovic Feb 16, 2024

kozlovic Feb 16, 2024

kozlovic Feb 16, 2024

levb Feb 17, 2024

kozlovic Feb 16, 2024

levb Feb 17, 2024

kozlovic Feb 16, 2024

levb Feb 17, 2024

kozlovic Feb 20, 2024

levb Feb 20, 2024

kozlovic Feb 16, 2024

kozlovic Feb 16, 2024

kozlovic left a comment

kozlovic Feb 20, 2024

levb commented Feb 20, 2024

Added tests for MQTT retained messages in various cluster/domain conf… #5082

Are you sure you want to change the base?

Added tests for MQTT retained messages in various cluster/domain conf… #5082

Conversation

levb commented Feb 14, 2024 • edited

levb Feb 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb Feb 20, 2024 • edited

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb commented Feb 20, 2024

levb commented Feb 14, 2024 •

edited

levb Feb 15, 2024 •

edited

levb Feb 20, 2024 •

edited