PHPC-1184: Add alternative topologies to Travis CI #871

derickr · 2018-07-02T15:25:49Z

No description provided.

jmikola · 2018-07-11T17:11:59Z

tests/apm/monitoring-commandFailed-001.phpt

-started: drop
-failed: drop
+started: geoNear
+failed: geoNear


geoNear is deprecated (PM-939). I think a more robust solution would just be to issue an aggregate with an unsupported pipeline operator (e.g. [ { "$unsupported": 1 } ]) or an update with a similarly unsupported modifier.

OK, will see if I can fix that.

jmikola · 2018-07-11T17:14:40Z

tests/utils/tools.php

+ * @return bool
+ * @throws RuntimeException
+ */
+function command_works($uri, $command)


Any reason not to type-hint $command here?

Alternatively, allow it to take any array|object and then wrap it in a Command as needed. This is what PHPLIB's DatabaseCommand helper does. It would save you from needing to import and construct MongoDB\Driver\Command from skip_if_sleep_command_unavailable(), and anything else that may use this in the future.

OK, will fix that too.

jmikola · 2018-07-11T17:17:24Z

tests/standalone/connectiontimeoutexception-001.phpt

@@ -5,6 +5,7 @@ ConnectionTimeoutException: exceeding sockettimeoutms
 <?php skip_if_not_live(); ?>
 <?php skip_if_not_clean(); ?>
 <?php skip_if_test_commands_disabled(); ?>
+<?php skip_if_sleep_command_unavailable(); ?>


Since we already have a skip for disabled test commands on the line above, what is the case where sleep isn't supported? Unfortunately, it looks like the command is no longer in our own manual, so I've linked to one of the Chinese translations (cloned from an older version).

https://jira.mongodb.org/browse/SERVER-36066

Interesting. I wonder if that's related to fail points on mongos not being supported. Depending on the outcome of that ticket, it may make sense to simply skip this test for mongos and leave /* See: SERVER-36066 */ as a comment between the skip function and ?>.

The work for skipping this properly (based on the sleep function existing) is already done, so I don't see a reason to remove that again.

If I understand skip_if_sleep_command_unavailable() correctly, it's going to introduce a one-second delay if the server does support the command. If this is also the only place that function would be used, I think the skip is preferable.

I think the best improvement might actually be modifying skip_if_test_commands_disabled() to also skip if the primary server is a mongos node, since it doesn't seem to support any test commands (despite reporting otherwise when we query its configuration).

That will also prove more useful down the line if we inevitably add more tests that want to set fail points.

I'm actually curious how the following tests pass on mongos:

tests/standalone/executiontimeoutexception-001.phpt

tests/standalone/executiontimeoutexception-002.phpt

The other two tests that set fail points are currently disabled:

tests/cursor/cursor-getmore-005.phpt

tests/cursor/cursor-getmore-006.phpt

I can't explain why those two tests actually work, but I checked, and they indeed fail with operation exceeded time limit, even when talking to mongos. I can't reproduce this effect in the shell though.

And also, setting the failpoint on mongos succeeds:

mongos> db.runCommand( { configureFailPoint: 'maxTimeAlwaysTimeOut', mode: 'alwaysOn' } ); { "ok" : 1, "operationTime" : Timestamp(1531733041, 1), "$clusterTime" : { "clusterTime" : Timestamp(1531733041, 1), "signature" : { "hash" : BinData(0,"AAAAAAAAAAAAAAAAAAAAAAAAAAA="), "keyId" : NumberLong(0) } } }

Ah, so it is a case of this specific test command not working on mongos. Works for me, then.

jmikola · 2018-07-11T17:20:11Z

tests/apm/overview.phpt

    ["code"]=>
-    int(26)%A
+    int(8)%A


The fact that this still reports error code 8 ("UnknownError") instead of 26 ("NamespaceNotFound") is more evidence that geoNear is not being actively maintained and we should probably avoid using it for any testing.

c.f. https://github.com/mongodb/mongo/blob/master/src/mongo/base/error_codes.err

jmikola · 2018-07-11T17:21:37Z

scripts/presets/travis/replica_sets/replicaset.json

@@ -12,8 +12,7 @@
                "nssize": 1,
                "port": 3000,
                "bind_ip_all": true,
-                "smallfiles": true,
-                "setParameter": {"enableTestCommands": 1}
+                "smallfiles": true


Why are you removing enableTestCommand?

I'm curious if this is the reason you needed to introduce skip_if_sleep_command_unavailable(). Perhaps there's a bug in skip_if_test_commands_disabled()?

jmikola · 2018-07-13T16:23:52Z

.travis.yml

-  - export PATH=${PWD}/mongodb-linux-x86_64-${SERVER_VERSION}/bin/:${PATH}
+  - wget http://fastdl.mongodb.org/linux/mongodb-linux-x86_64-ubuntu1404-${SERVER_VERSION}.tgz
+  - tar zxf mongodb-linux-x86_64-ubuntu1404-${SERVER_VERSION}.tgz
+  - export PATH=${PWD}/mongodb-linux-x86_64-ubuntu1404-${SERVER_VERSION}/bin:${PATH}


Was there a problem with the standard Linux builds? Is it that they ship without SSL support?

I'm just curious if we might run into issues with the build team no longer supplying 14.04 builds for newer releases down the line (assuming Travis doesn't upgrade to 16.04 sooner).

Yes, the standard ones ship without SSL support :-/ And yes, we will run into issues when we stop supporting 14.04 and Travis doesn't upgrade :-/

jmikola · 2018-07-13T16:25:33Z

.travis.yml

  - ulimit -a
  - ulimit -c unlimited || true
  - .travis.scripts/before_script.sh

 script:
+  - export MONGODB_URI=`cat /tmp/uri.txt`
+  - echo $MONGODB_URI


setup_mo.sh already prints the URI (prefixed with "MongoDB Test URI: "). Why bother doing so again?

Originally, I added the echo here because for some reason the environment variable didn't propagate between steps (i.e., before_script and script). I then later added the export to fix this, and the echo stayed for testing. I am inclined to keep it.

jmikola · 2018-07-13T16:30:32Z

scripts/presets/travis/replica_sets/replicaset.json

+                "nssize": 1,
+                "port": 3000,
+                "bind_ip_all": true,
+                "smallfiles": true


Per mongodb-labs/mongo-orchestration#247 (comment), I think we can remove nssize, smallfiles, and noprealloc from all of these configs.

Only the STANDALONE_OLD case under the 3.0.x server version will use MMAPv1 (where noprealloc might still apply); however, I'm not convinced it's worth optimizing for that case (especially since it's not even a replica set). Having these servers use /tmp as their DB path should be enough optimization, since I would expect that to be a RAM disk.

I had previously removed noprealloc from our non-Travis configs in 2ef765a but stupidly added the others in 9a83bde. Perhaps we can remove all three options across the board (for these Travis configs and the non-Travis configs).

I've made a ticket for it, to do this as a low priority thing later: https://jira.mongodb.org/browse/PHPC-1240

jmikola · 2018-07-13T16:38:03Z

scripts/presets/travis/replica_sets/replicaset.json

+            "procParams": {
+                "dbpath": "/tmp/REPLICASET/3000/",
+                "ipv6": true,
+                "logappend": true,


Is this option important given that the Travis build boxes are ephemeral?

It was useful for Travis in debug mode.

jmikola · 2018-07-13T16:39:14Z

scripts/presets/travis/replica_sets/replicaset.json

+                "ipv6": true,
+                "logappend": true,
+                "logpath": "/tmp/REPLICASET/3000/mongod.log",
+                "journal": true,


I expect this was copied over from our non-Travis configs. It looks like storage.journal.enabled defaults to true only on 64-bit platforms, so it's probably sensible to keep this set explicitly in case.

jmikola · 2018-07-13T16:41:17Z

scripts/presets/travis/sharded_clusters/cluster.json

@@ -0,0 +1,89 @@
+{
+    "configsvrs": [ {
+		"members" : [


These JSON files appear to be using a mix of four-space and tab indents. I'd suggest standardizing on four-space indents, as I did for our non-Travis configs in 8b19fcb.

It was only this one... → fixed.

jmikola · 2018-07-13T16:41:55Z

tests/cursor/bug1050-001.phpt

@@ -57,7 +57,7 @@ if (isset($document)) {
 <?php exit(0); ?>
 --EXPECTF--
 Executing command took 0.%d seconds
-Rewinding cursor took 1.%d seconds
+Rewinding cursor took 0.%r(4|5)%r%d seconds


Good catch. 👍

jmikola · 2018-07-13T16:43:06Z

tests/connect/bug1045.phpt

@@ -12,7 +12,7 @@ PHPC-1045: Segfault if username is not provided for SCRAM-SHA-1 authMechanism
 require_once __DIR__ . "/../utils/basic.inc";

 // URI does not support auth, but that is not necessary for the test
-$m = new MongoDB\Driver\Manager(URI, ['authMechanism' => 'SCRAM-SHA-1', 'ssl' => false]);
+$m = new MongoDB\Driver\Manager(URI, ['authMechanism' => 'SCRAM-SHA-1']);


Is the "URI does not support auth" comment above still relevant, given that we may be testing against a server using authentication?

Perhaps this should be changed to: "URI may or may not support auth, but that..."

jmikola · 2018-07-13T16:52:58Z

tests/standalone/connectiontimeoutexception-001.phpt

@@ -5,6 +5,7 @@ ConnectionTimeoutException: exceeding sockettimeoutms
 <?php skip_if_not_live(); ?>
 <?php skip_if_not_clean(); ?>
 <?php skip_if_test_commands_disabled(); ?>
+<?php skip_if_sleep_command_unavailable(); ?>


If I understand skip_if_sleep_command_unavailable() correctly, it's going to introduce a one-second delay if the server does support the command. If this is also the only place that function would be used, I think the skip is preferable.

I think the best improvement might actually be modifying skip_if_test_commands_disabled() to also skip if the primary server is a mongos node, since it doesn't seem to support any test commands (despite reporting otherwise when we query its configuration).

That will also prove more useful down the line if we inevitably add more tests that want to set fail points.

I'm actually curious how the following tests pass on mongos:

tests/standalone/executiontimeoutexception-001.phpt

tests/standalone/executiontimeoutexception-002.phpt

The other two tests that set fail points are currently disabled:

tests/cursor/cursor-getmore-005.phpt

tests/cursor/cursor-getmore-006.phpt

jmikola

LGTM, although there still appear to be two build failures in https://travis-ci.org/mongodb/mongo-php-driver/builds/404364466.

derickr force-pushed the PHPC-1184-alternative-topologies-travis branch 17 times, most recently from 2b74723 to b63fb69 Compare July 3, 2018 14:12

derickr force-pushed the PHPC-1184-alternative-topologies-travis branch from b63fb69 to e36f03f Compare July 10, 2018 13:46

jmikola reviewed Jul 11, 2018

View reviewed changes

derickr force-pushed the PHPC-1184-alternative-topologies-travis branch 2 times, most recently from 709e29d to 36ce628 Compare July 12, 2018 09:54

derickr changed the base branch from master to v1.5 July 12, 2018 10:57

derickr force-pushed the PHPC-1184-alternative-topologies-travis branch 4 times, most recently from 4a2b3f6 to b42d739 Compare July 13, 2018 15:28

derickr changed the title ~~[WIP] Try out starting our own versions and SSL~~ PHPC-1184: Add alternative topologies to Travis CI Jul 13, 2018

derickr requested a review from jmikola July 13, 2018 15:49

jmikola reviewed Jul 13, 2018

View reviewed changes

derickr added 2 commits July 16, 2018 10:30

PHPC-1184: Add alternative topologies to Travis

3678fa5

Disable test case for PHPC-431 until PHPC-1239 is resolved

7ae04b2

derickr force-pushed the PHPC-1184-alternative-topologies-travis branch from b42d739 to 7ae04b2 Compare July 16, 2018 09:30

jmikola approved these changes Jul 16, 2018

View reviewed changes

derickr merged commit 7ae04b2 into mongodb:v1.5 Jul 16, 2018

derickr added a commit that referenced this pull request Jul 16, 2018

Merged pull request #871

9afcf4a

PHPC-1184: Add alternative topologies to Travis CI #871

PHPC-1184: Add alternative topologies to Travis CI #871

Uh oh!

Conversation

derickr commented Jul 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!