Avoid port collisions in tests, start CassandraLauncher first #2804

octonato · 2020-04-06T15:41:08Z

Refs: #2778

octonato · 2020-04-06T15:42:24Z

We had an internal CassandraLauncher.scala that was not used. This is removed.

octonato · 2020-04-06T15:49:08Z

We had an internal CassandraLauncher.scala that was not used. This is removed.

I'm wrong about this. The internal CassandraLauncher is used in dev-mode. I will re-add it.

patriknw · 2020-04-06T16:48:33Z

.../test/scala/com/lightbend/lagom/javadsl/persistence/cassandra/CassandraPersistenceSpec.scala

@@ -34,13 +37,19 @@ class CassandraPersistenceSpec(system: ActorSystem) extends ActorSystemSpec(syst
  override def beforeAll(): Unit = {
    super.beforeAll()

+    // Join ourselves - needed because the Cassandra offset store uses cluster startup task
+    val cluster = Cluster(system)
+    cluster.join(cluster.selfAddress)


I don't understand why this would be needed? It must be better to have Cassandra running as early as possible so that any things in the test or Lagom itself can connect if they startup as part of the cluster formation.

Both write and read sides depend on cluter and therefore it's ok to start the cluter first.

my point is that if they start before Cassandra is started the initial connect to Cassandra may fail. That may cause other problems.

patriknw · 2020-04-06T16:49:18Z

.../test/scala/com/lightbend/lagom/javadsl/persistence/cassandra/CassandraPersistenceSpec.scala

+    val cluster = Cluster(system)
+    cluster.join(cluster.selfAddress)
+
+    // first ensure that this node is Up (ie: remote port is properly bound)


remote port is bound when the ActorSystem is started (before apply returns)

You are completely right about this. Somehow I convinced myself that the port would only bind when trying to form the cluster. This doesn't make any sense.

That's getting really puzzling.

The erroing test logs says:

Remoting started with transport [Artery tcp]; listening on address [akka://PersistentEntityTestDriverCompatSpec@127.0.0.1:34359] with UID [3966907252480615512] [info] PersistentEntityTestDriverCompatSpec: Starting Cassandra on port client port: 34359

So, artery bound on TCP port 34359 at actor system bootstrap. Fine.
Then Cassandra launcher tries to pick a free port and OS returns 34359, which then conflicts when we try to start the Cassandra client.
That doesn't make sense to me. Unless the actor system is not yet bootstrapped when we start the CassandraLauncher.

I can only explain it with the following order of events:

ActorSystem reserves port 34359 and releases the socket

CassandraLauncher reserves port 34359 and releases the socket

Artery starts on port 34359

CassandraLauncher starts and fails to bind on port 34359

(1 and 2 can be swapped)

Maybe forming the cluster before CassandraLauncher is exaggerated, but we should not try to reserve a port for Cassandra before we can confirm that Artery is running.

If that scenario is true there is something wrong with the initialization in Artery. It's supposed to bind (and block until completed) before ActorSystem.apply returns.

Oh, that's useful information. Although it makes it scarier now.

I found it!

In the init of the test, we reserve the Cassandra port using CassandraLauncher.randomPort (opens socket, pick port, close socket).

Then the ActorSystem bootstraps and take the same port. Artery bounds to it.

Then CassandraLauncher has the port already fixed and tries to bound to it.

We can see it here:
https://github.com/lagom/lagom/pull/2804/files#diff-06bb277ec92c7db81e68c50ae7478f00R28

ah, nice catch

octonato · 2020-04-07T09:20:05Z

This is ready for another review.

Search overall where we were using the CassandraLauncher and made sure that we start it as soon as possible and before the ActorSystem.

We do need to access randomPort in many places in order add it to the config, but better to only do it when Cassandra is already running. First start Cassandra, then read randomPort and then start the ActorSystem.

I'm confident that this and akka/akka-persistence-cassandra#765 will remove a lot of flakiness we have seen in the past. 🤞

patriknw

LGTM, that looks like the right thing to do

patriknw · 2020-04-07T09:31:42Z

changed title of PR

octonato · 2020-04-07T11:55:19Z

@Mergifyio backport 1.6.x 1.5.x

mergify · 2020-04-07T11:56:01Z

Command backport 1.6.x 1.5.x: failure

No backport have been created

Backport to branch 1.6.x failed

Cherry-pick of 090074a has failed:

On branch mergify/bp/1.6.x/pr-2804
Your branch is up to date with 'origin/1.6.x'.

You are currently cherry-picking commit 090074a50.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   persistence-cassandra/javadsl/src/test/java/com/lightbend/lagom/javadsl/persistence/PersistentEntityRefTest.java
	modified:   persistence-cassandra/javadsl/src/test/scala/com/lightbend/lagom/javadsl/persistence/cassandra/CassandraPersistenceSpec.scala
	modified:   persistence-cassandra/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/cassandra/PersistentEntityRefSpec.scala
	modified:   persistence-jdbc/javadsl/src/test/scala/com/lightbend/lagom/javadsl/persistence/jdbc/JdbcPersistenceSpec.scala
	modified:   persistence-jdbc/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/jdbc/JdbcPersistenceSpec.scala
	modified:   persistence-jdbc/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/slick/SlickPersistenceSpec.scala
	modified:   persistence/core/src/test/scala/com/lightbend/lagom/persistence/ActorSystemSpec.scala
	modified:   testkit/core/src/main/scala/com/lightbend/lagom/internal/testkit/CassandraTestServer.scala

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   persistence-cassandra/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/cassandra/CassandraPersistenceSpec.scala

Backport to branch 1.5.x failed

Cherry-pick of 090074a has failed:

On branch mergify/bp/1.5.x/pr-2804
Your branch is up to date with 'origin/1.5.x'.

You are currently cherry-picking commit 090074a50.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   persistence-cassandra/javadsl/src/test/java/com/lightbend/lagom/javadsl/persistence/PersistentEntityRefTest.java
	modified:   testkit/core/src/main/scala/com/lightbend/lagom/internal/testkit/CassandraTestServer.scala

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   persistence-cassandra/javadsl/src/test/scala/com/lightbend/lagom/javadsl/persistence/cassandra/CassandraPersistenceSpec.scala
	both modified:   persistence-cassandra/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/cassandra/CassandraPersistenceSpec.scala
	both modified:   persistence-cassandra/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/cassandra/PersistentEntityRefSpec.scala
	both modified:   persistence-jdbc/javadsl/src/test/scala/com/lightbend/lagom/javadsl/persistence/jdbc/JdbcPersistenceSpec.scala
	both modified:   persistence-jdbc/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/jdbc/JdbcPersistenceSpec.scala
	both modified:   persistence-jdbc/scaladsl/src/test/scala/com/lightbend/lagom/scaladsl/persistence/slick/SlickPersistenceSpec.scala
	both modified:   persistence/core/src/test/scala/com/lightbend/lagom/persistence/ActorSystemSpec.scala

Manual backport of #2804 on 1.6.x branch

Manual backport #2804 on 1.5.x branch

octonato force-pushed the cassandra-flaky-tests branch from 82b8cbb to 4d89265 Compare April 6, 2020 15:49

patriknw reviewed Apr 6, 2020

View reviewed changes

Start CassandraLauncher before ActorSystem (always)

090074a

octonato force-pushed the cassandra-flaky-tests branch from 4d89265 to 090074a Compare April 7, 2020 09:14

patriknw approved these changes Apr 7, 2020

View reviewed changes

octonato changed the title ~~Form Cluster before starting CassandraLauncher~~ Start CassandraLauncher before anything else Apr 7, 2020

patriknw changed the title ~~Start CassandraLauncher before anything else~~ Avoid port collisions in tests, start CassandraLauncher first Apr 7, 2020

mergify bot merged commit 2af24e0 into lagom:master Apr 7, 2020

octonato deleted the cassandra-flaky-tests branch April 7, 2020 11:55

mergify bot added a commit that referenced this pull request Apr 7, 2020

Merge pull request #2810 from renatocaval/backport-2804-on-1.6.x

69648a2

Manual backport of #2804 on 1.6.x branch

mergify bot added a commit that referenced this pull request Apr 7, 2020

Merge pull request #2811 from renatocaval/backport-2804-on-1.5.x

6479c91

Manual backport #2804 on 1.5.x branch

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid port collisions in tests, start CassandraLauncher first #2804

Avoid port collisions in tests, start CassandraLauncher first #2804

octonato commented Apr 6, 2020 •

edited

octonato commented Apr 6, 2020

octonato commented Apr 6, 2020

patriknw Apr 6, 2020

octonato Apr 6, 2020

patriknw Apr 7, 2020

patriknw Apr 6, 2020

octonato Apr 6, 2020 •

edited

octonato Apr 6, 2020

patriknw Apr 7, 2020

octonato Apr 7, 2020

octonato Apr 7, 2020

octonato Apr 7, 2020

patriknw Apr 7, 2020

octonato commented Apr 7, 2020 •

edited

patriknw left a comment

patriknw commented Apr 7, 2020

octonato commented Apr 7, 2020

mergify bot commented Apr 7, 2020

Avoid port collisions in tests, start CassandraLauncher first #2804

Avoid port collisions in tests, start CassandraLauncher first #2804

Conversation

octonato commented Apr 6, 2020 • edited

octonato commented Apr 6, 2020

octonato commented Apr 6, 2020

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

octonato Apr 6, 2020 • edited

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

octonato commented Apr 7, 2020 • edited

patriknw left a comment

Choose a reason for hiding this comment

patriknw commented Apr 7, 2020

octonato commented Apr 7, 2020

mergify bot commented Apr 7, 2020

octonato commented Apr 6, 2020 •

edited

octonato Apr 6, 2020 •

edited

octonato commented Apr 7, 2020 •

edited