Test upgrading from the latest published minor version #261

eastlondoner · 2020-06-15T20:44:20Z

Adds 2 new tests for upgrading from published neo4j docker images to the one just built.

tests upgrade within the same minor version e.g released 4.0 -> 4.0.7-SNAPSHOT
tests upgrade from the previous minor version in the same major version e.g. released 4.0 -> 4.1.0-SNAPSHOT

It seems that the existing DatabaseIO.java and TestUpgrade.java file were indented with tabs so, that got fixed at the same time.

Check groups when determining if a directory is writable

…oints

Renamed folders and merged source from https://github.com/neo-technology/new-docker-tests

….java

…rly.

…ning-to-stdout Send warning about chowning folder to stdout.

Send warning about chowning folder to stdout.

…j into test-refactor

We were using a Neo4jContainer from TestContainers rather than a GenericContainer and this seems to have caused the /data/dbms directory to get built unexpectedly. The data/dbms folder contains authentication information, and it turns out that Neo4jContainer was setting a default password even though NEO4J_AUTH=none is set, hence the folder appeared and out tests passed. I've added an initial password so that the folder is created and we can test permissions on it.

This reverts commit 4c002cf.

This reverts commit 03bc788.

…r inside a parent. renamed test folders so they sort by test rather than folder function

…ithout having to specify it

So I can merge host file system refactors.

src/test/java/com/neo4j/docker/TestUpgrade.java

jennyowen · 2020-06-16T07:55:20Z

src/test/java/com/neo4j/docker/TestUpgrade.java

+        if ( Instant.now().isBefore( Instant.parse( "2020-07-01T00:00:00.00Z" ) ) )
+        {
+            // This sort-of-hack is necessary when we cut a new minor branch before the previous minor branch has been published to dockerhub.
+            Assumptions.assumeTrue( Neo4jVersion.NEO4J_VERSION_420.isNewerThan( targetNeo4jVersion ) );
+        }


This is hacky. I think the question of "is this on a branch that's released already" should be captured in the Neo4jVersion class somehow. I think marking all patch=0 versions as unreleased (which would be true in our CI environment and in general) would be enough right?
We don't really need to worry too much about people updating from, say, 4.0.0 to 4.0.0. It's not a valid thing that users will ever do. Can't we just skip this test for if patch == 0?

For the main Neo4j Jars we say that going from 1 minor version to another is supported.
For clustering two adjacent minor versions are required to form a functioning cluster (necessary for no-downtime upgrades).
So my intention is to test that migration to x.y.0 from x.y-1 works.

The problem I'm trying to tackle is that there is a period (like now) when 4.2.0 exists internally (so these tests will be run in PRs etc.) but there is no published 4.1 to test upgrade from.

why not just do that kind of upgrade in a separate unit test rather than having over-complicated test logic here? Actually that would simplify the logic a lot since the latest patch version of a release is always tagged as the major.minor. EG neo4j:4.0 is always the latest 4.0 patch version.

I will separate so we will do:

patch upgrade tests I.e. from x.y (released) to current head (assuming current head patch version > 0)

Minor version upgrade test I.e. from x.(y-1) to current head.

For the latter there will still need to be a temporary hack for situations like at the moment where there is a 4.2 branch but no 4.1 release.

What’s going on should be much more transparent though

src/test/java/com/neo4j/docker/TestUpgrade.java

jennyowen · 2020-06-16T09:07:48Z

src/test/java/com/neo4j/docker/TestUpgrade.java

+        }
+        catch ( IOException e )
+        {
+            throw new RuntimeException( e );


why is the IOException being recast as a runtime exception? Also, doesn't the method signature need to declare that it throws an exception?

RuntimeExceptions are unchecked (don't need to be declared).

I used createDirectory in a stream processing step and those aren't allowed to throw checked exceptions - this is a just wrapping the checked IOException in a RuntimeException so I can use this method on a stream.

jennyowen · 2020-06-16T10:33:38Z

src/test/java/com/neo4j/docker/TestUpgrade.java

+        container.setWaitStrategy( Wait.forHttp( "/" ).forPort( 7474 ).forStatusCode( 200 ) );
+        container = container.withStartupTimeout( Duration.ofMinutes( 2 ) );


You can do this simpler like:

docker-neo4j/src/test/java/com/neo4j/docker/TestPasswords.java

Lines 35 to 38 in 03bc788

.waitingFor( Wait.forHttp( "/" )

.forPort( 7474 )

.forStatusCode( 200 )

.withStartupTimeout( Duration.ofSeconds( 90 ) ) );

jennyowen · 2020-06-16T11:26:43Z

src/test/java/com/neo4j/docker/TestUpgrade.java

+            validateConfig( db );
+
+            // validate that writes actually went to the mounted directories
+            writableMounts.forEach( ( containerMount, hostFolder ) -> assertDirectoryModifiedSince( hostFolder, startTime ) );


we already have tests that check that the container can write to a mounted folder, so I'm not sure what value this adds?

If I didn't include this check to check that mount paths worked. I include it because without checking if we changed how the mount paths worked (e.g. /data became /databases) this test would still pass (using /data) but it wouldn't be doing what I intended it to do which is persisting data on that mount.

but there's already a check that the data has been persisted:
db.verifyDataInContainer( user, password ); on line 146

That doesn’t check that the data is in the mounted directory - just that it’s in neo4j

In the second container, we don't write the data again before checking that the data is there. How else would the data be there unless it's reading from the mounted folder?

I’m checking that subsequent writes succeed and go there.
admittedly perhaps a niche concern.
It’s not just the data folder though - it’s all the writable folders so logs etc as well.

it's valid to check that we can still write to the database with the mounted folder since we might have introduced permissions changes (eg new uid!). I just don't think we need to query the file system to do that when we are also actually writing something to the database, since the database write will fail if something on the file system changed.

I stand by this. The writing of metrics to /metrics is broken in 3.5 if the volume is mounted without permissions even when running as root. That is detected by this check.

If I remove this check we do not detect that brokenness and this test passes when it should not.

jennyowen · 2020-06-16T11:42:36Z

src/test/java/com/neo4j/docker/TestUpgrade.java

+    private String dockerImageToUpgradeFrom( Neo4jVersion targetNeo4jVersion )
+    {
+        // The most recent minor release that we expect to already have been released.
+        var minorVersionToUpgradeFrom = targetNeo4jVersion.patch == 0 ? targetNeo4jVersion.minor - 1 : targetNeo4jVersion.minor;


the logic here confuses me. My understanding of these tests is that we're supposed to be testing upgrade of patch versions. So if the patch version is 0, we wouldn't ever need to upgrade within major/minor, right? The test is redundant if patch version = 0.

However there is confusing logic here and earlier with the 4.2 time checking hack, which seem to deal with the case of when patch = 0. Also I think this test will (in its current state) run on 4.1 even now before 4.1.0 is released?

We're working on different assumptions. I want this test to run on 4.1.0 to test 4.0 -> 4.1.0

My understanding is that for Neo4j going from one minor version to the next is expected to work without config changes.

that should be a separate unit test

this is split out into separate tests now.

src/test/java/com/neo4j/docker/TestUpgrade.java

…edures.

… dev version

…lts from queries, new createAndDeleteNode method to check writes AND format with spaces not tabs

…ctories in TestUpgrade

eastlondoner · 2020-06-23T07:00:46Z

src/test/java/com/neo4j/docker/utils/DatabaseIO.java

@@ -3,86 +3,111 @@
 import org.junit.jupiter.api.Assertions;


Sorry the diff here is large but the file was indented with tabs so I let the IDE re-indent the whole thing

Andrew Jefferson and others added 30 commits May 25, 2019 12:43

Check groups when determining if a directory is writable

5c6ad08

Merge pull request neo4j#169 from eastlondoner/fix-for-groups

edef1e2

Check groups when determining if a directory is writable

fixed minor typos in docker-entrypoint.sh

5e66363

patched 3.5 changes to 3.4 and 3.3

73770e2

adding back +x file permissions which got wiped when i patched entryp…

febc011

…oints

added extended group permission checks to 3.6 entrypoint

2478a45

added extended group permission checks to 4.0 entrypoint

5ec33f5

refactoring tests to use test-containers.

55ab1c3

Renamed folders and merged source from https://github.com/neo-technology/new-docker-tests

test mounting /data as specified user

8fcf613

minor refactoring of mounting tests

fe237f5

added logs and data mounting tests

a4f5821

refactored folder creation and mounting utilities out of TestMounting…

77c310b

….java

moved utilities folder to more sensible location

f75531f

more refactoring of test code

990d819

migrated 3.5 to debian slim base image

143c317

patched 3.5 base image changes to 3.4 and 3.3

710965a

Merge branch 'master' of github.com:neo4j/docker-neo4j

e3da938

changed base image on 3.6 and 4.0 to openjdk with debian slim

5fcde3b

made diagnostic message on logs folder check better

2345bf8

added switch around file permission improvments

e1a7b76

can now run java tests from makefile and/or maven

c6c91ef

deleted obsolete Makefile stuff. Refactored tests to use JUnit5 prope…

03f94a1

…rly.

added secure permissions flag testing for mounted folders

e12275d

removed commented out code

768b1a9

Send warning about chowning folder to stdout.

70b205c

Merge pull request neo4j#17 from neo-technology/dev-jgrobbel-send-war…

7597e69

…ning-to-stdout Send warning about chowning folder to stdout.

Send warning about chowning folder to stdout.

b4984d7

Merge pull request neo4j#173 from neo4j/rm-stderr-warning

6bb6c0f

Send warning about chowning folder to stdout.

added test for causal clustering

de8473b

Merge branch 'test-refactor' of github.com:neo-technology/docker-neo4…

551a7cb

…j into test-refactor

jennyowen added 7 commits May 6, 2020 12:52

Merge branch 'master' of github.com:neo4j/docker-neo4j

baec1f1

improved readwrite checks on mounted folders

d5c97b6

copied new docker source files for 4.2

bdf86b5

moved set password command to after configuration to fix a bug in aura

b0acb4f

explicitly set neo4j uid and gid to 7474

4c002cf

Revert "explicitly set neo4j uid and gid to 7474"

03bc788

This reverts commit 4c002cf.

eastlondoner requested a review from jennyowen June 15, 2020 20:44

eastlondoner force-pushed the upgrade-test branch from 768712b to c050d4e Compare June 15, 2020 20:48

jennyowen added 6 commits June 16, 2020 09:20

Revert "Revert "explicitly set neo4j uid and gid to 7474""

b65ad72

This reverts commit 03bc788.

started test for database dump

994bf19

made tests that mount 2 or more folders keep the temp folders togethe…

b09dd44

…r inside a parent. renamed test folders so they sort by test rather than folder function

added some notes to dump load test

3d75707

added host FS operation to create test folder under TEST_TMP_FOLDER w…

9e5927a

…ithout having to specify it

removed dump and load test

3f8cc0a

So I can merge host file system refactors.

jennyowen requested changes Jun 16, 2020

View reviewed changes

jennyowen and others added 6 commits June 18, 2020 10:25

Merge branch 'hardcode-uid-gid'

3d059c5

reformatting TestUpgrade.java to use spaces instead of tabs

15ba3e6

Use autoclosable drivers. Consume and return results from Cypher proc…

147c675

…edures.

Test upgrading from the latest published minor version to the current…

6cc7a54

… dev version

Improve DatabaseIO: use autoclosable drivers, consume and return resu…

f85e167

…lts from queries, new createAndDeleteNode method to check writes AND format with spaces not tabs

Rewrite UpgradeTest based on PR feedback

c2c7bbc

eastlondoner force-pushed the upgrade-test branch from 5bd10cc to c2c7bbc Compare June 22, 2020 15:48

Andrew Jefferson added 2 commits June 22, 2020 17:50

don't need a version const for 4.2

ea936c4

handle the difference between community and enterprise mountable dire…

6cb6586

…ctories in TestUpgrade

eastlondoner force-pushed the upgrade-test branch from 92b0c48 to 6cb6586 Compare June 23, 2020 06:39

eastlondoner commented Jun 23, 2020

View reviewed changes

jennyowen force-pushed the master branch 2 times, most recently from 1ce4aac to 7986a74 Compare June 10, 2021 13:54

jennyowen force-pushed the master branch from db907cd to 0256996 Compare November 3, 2022 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test upgrading from the latest published minor version #261

Test upgrading from the latest published minor version #261

eastlondoner commented Jun 15, 2020 •

edited

Loading

jennyowen Jun 16, 2020

eastlondoner Jun 16, 2020

jennyowen Jun 18, 2020

eastlondoner Jun 18, 2020

jennyowen Jun 16, 2020

eastlondoner Jun 16, 2020

jennyowen Jun 16, 2020

jennyowen Jun 16, 2020

eastlondoner Jun 16, 2020

jennyowen Jun 18, 2020

eastlondoner Jun 18, 2020

jennyowen Jun 18, 2020

eastlondoner Jun 18, 2020

jennyowen Jun 18, 2020

eastlondoner Jun 23, 2020

jennyowen Jun 16, 2020

eastlondoner Jun 16, 2020

jennyowen Jun 18, 2020

eastlondoner Jun 23, 2020

eastlondoner Jun 23, 2020

		container.setWaitStrategy( Wait.forHttp( "/" ).forPort( 7474 ).forStatusCode( 200 ) );
		container = container.withStartupTimeout( Duration.ofMinutes( 2 ) );

	.waitingFor( Wait.forHttp( "/" )
	.forPort( 7474 )
	.forStatusCode( 200 )
	.withStartupTimeout( Duration.ofSeconds( 90 ) ) );

Test upgrading from the latest published minor version #261

Are you sure you want to change the base?

Test upgrading from the latest published minor version #261

Conversation

eastlondoner commented Jun 15, 2020 • edited Loading

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

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

eastlondoner commented Jun 15, 2020 •

edited

Loading