From 406d401af1820bba0d6b645b985e8b9b8ede88d9 Mon Sep 17 00:00:00 2001 From: Przemek Hugh Kaznowski Date: Mon, 4 Jun 2018 17:31:49 +0100 Subject: [PATCH] Backup temporary directory rotation fix FileMoveActions on non page cache files are copied. This is evident when backups are made to a temporary directory and then renamed when successful. This change fixes file move actions to always move and not copy. --- .../neo4j/test/rule/EmbeddedDatabaseRule.java | 2 +- .../neo4j/backup/impl/BackupCopyService.java | 1 + .../backup/impl/BackupStrategyWrapper.java | 10 +- .../backup/impl/OnlineBackupCommandCcIT.java | 59 +++++++++++ .../backup/impl/OnlineBackupCommandHaIT.java | 89 ++++++++++++++++- .../neo4j/com/storecopy/FileMoveAction.java | 1 + .../neo4j/com/storecopy/FileMoveProvider.java | 4 +- .../com/storecopy/FileMoveActionTest.java | 98 +++++++++++++++++++ .../test/java/org/neo4j/util/TestHelpers.java | 1 - 9 files changed, 251 insertions(+), 14 deletions(-) create mode 100644 enterprise/com/src/test/java/org/neo4j/com/storecopy/FileMoveActionTest.java diff --git a/community/kernel/src/test/java/org/neo4j/test/rule/EmbeddedDatabaseRule.java b/community/kernel/src/test/java/org/neo4j/test/rule/EmbeddedDatabaseRule.java index 3116c9ed07e03..a53df8ccbb1e2 100644 --- a/community/kernel/src/test/java/org/neo4j/test/rule/EmbeddedDatabaseRule.java +++ b/community/kernel/src/test/java/org/neo4j/test/rule/EmbeddedDatabaseRule.java @@ -71,7 +71,7 @@ protected GraphDatabaseFactory newFactory() @Override protected GraphDatabaseBuilder newBuilder( GraphDatabaseFactory factory ) { - return factory.newEmbeddedDatabaseBuilder( testDirectory.graphDbDir() ); + return factory.newEmbeddedDatabaseBuilder( getStoreDir() ); } @Override diff --git a/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupCopyService.java b/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupCopyService.java index 33f2cffc48612..a2e073a3e6cb1 100644 --- a/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupCopyService.java +++ b/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupCopyService.java @@ -70,6 +70,7 @@ public void moveBackupLocation( Path oldLocation, Path newLocation ) throws IOEx { moves.next().move( target ); } + oldLocation.toFile().delete(); } catch ( IOException e ) { diff --git a/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupStrategyWrapper.java b/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupStrategyWrapper.java index 9850692457958..271937c412228 100644 --- a/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupStrategyWrapper.java +++ b/enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupStrategyWrapper.java @@ -74,8 +74,7 @@ Fallible doBackup( OnlineBackupContext onlineBackupContex return state; } - private Fallible performBackupWithoutLifecycle( - OnlineBackupContext onlineBackupContext ) + private Fallible performBackupWithoutLifecycle( OnlineBackupContext onlineBackupContext ) { Path backupLocation = onlineBackupContext.getResolvedLocationFromName(); Path userSpecifiedBackupLocation = onlineBackupContext.getResolvedLocationFromName(); @@ -136,8 +135,7 @@ private void clearIdFiles( Path backupLocation ) * @param onlineBackupContext command line arguments, config etc. * @return outcome of full backup */ - private Fallible fullBackupWithTemporaryFolderResolutions( - OnlineBackupContext onlineBackupContext ) + private Fallible fullBackupWithTemporaryFolderResolutions( OnlineBackupContext onlineBackupContext ) { Path userSpecifiedBackupLocation = onlineBackupContext.getResolvedLocationFromName(); Path temporaryFullBackupLocation = backupCopyService.findAnAvailableLocationForNewFullBackup( userSpecifiedBackupLocation ); @@ -146,12 +144,12 @@ private Fallible fullBackupWithTemporaryFolderResolutions( Fallible state = backupStrategy.performFullBackup( temporaryFullBackupLocation, config, address ); // NOTE temporaryFullBackupLocation can be equal to desired - boolean aBackupAlreadyExisted = userSpecifiedBackupLocation.equals( temporaryFullBackupLocation ); + boolean backupWasMadeToATemporaryLocation = !userSpecifiedBackupLocation.equals( temporaryFullBackupLocation ); if ( BackupStageOutcome.SUCCESS.equals( state.getState() ) ) { backupRecoveryService.recoverWithDatabase( temporaryFullBackupLocation, pageCache, config ); - if ( !aBackupAlreadyExisted ) + if ( backupWasMadeToATemporaryLocation ) { try { diff --git a/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandCcIT.java b/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandCcIT.java index 339504ae7ba14..577af9fe1aebc 100644 --- a/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandCcIT.java +++ b/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandCcIT.java @@ -40,7 +40,10 @@ import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; import org.neo4j.causalclustering.ClusterHelper; @@ -49,6 +52,8 @@ import org.neo4j.causalclustering.core.consensus.roles.Role; import org.neo4j.causalclustering.discovery.Cluster; import org.neo4j.causalclustering.discovery.CoreClusterMember; +import org.neo4j.causalclustering.discovery.IpFamily; +import org.neo4j.causalclustering.discovery.SharedDiscoveryServiceFactory; import org.neo4j.causalclustering.helpers.CausalClusteringTestHelpers; import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.kernel.configuration.Config; @@ -64,8 +69,10 @@ import org.neo4j.util.TestHelpers; import static java.lang.String.format; +import static java.util.Collections.emptyMap; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeFalse; @@ -269,6 +276,45 @@ public void reportsProgress() throws Exception assertTrue( output.contains( "Finished receiving index snapshots" ) ); } + @Test + public void backupRenamesWork() throws Exception + { + // given a prexisting backup from a different store + String backupName = "preexistingBackup_" + recordFormat; + Cluster cluster = startCluster( recordFormat ); + String firstBackupAddress = CausalClusteringTestHelpers.transactionAddress( clusterLeader( cluster ).database() ); + + assertEquals( 0, runBackupToolFromOtherJvmToGetExitCode( + "--from", firstBackupAddress, + "--cc-report-dir=" + backupDir, + "--backup-dir=" + backupDir, + "--name=" + backupName ) ); + DbRepresentation firstDatabaseRepresentation = DbRepresentation.of( clusterLeader( cluster ).database() ); + + // and a different database + Cluster cluster2 = startCluster2( recordFormat ); + DbRepresentation secondDatabaseRepresentation = DbRepresentation.of( clusterLeader( cluster2 ).database() ); + assertNotEquals( firstDatabaseRepresentation, secondDatabaseRepresentation ); + String secondBackupAddress = CausalClusteringTestHelpers.transactionAddress( clusterLeader( cluster2 ).database() ); + + // when backup is performed + assertEquals( 0, runBackupToolFromOtherJvmToGetExitCode( + "--from", secondBackupAddress, + "--cc-report-dir=" + backupDir, + "--backup-dir=" + backupDir, + "--name=" + backupName ) ); + cluster2.shutdown(); + + // then the new backup has the correct name + assertEquals( secondDatabaseRepresentation, getBackupDbRepresentation( backupName, backupDir ) ); + + // and the old backup is in a renamed location + assertEquals( firstDatabaseRepresentation, getBackupDbRepresentation( backupName + ".err.0", backupDir ) ); + + // and the data isn't equal (sanity check) + assertNotEquals( firstDatabaseRepresentation, secondDatabaseRepresentation ); + } + static PrintStream wrapWithNormalOutput( PrintStream normalOutput, PrintStream nullAbleOutput ) { if ( nullAbleOutput == null ) @@ -342,6 +388,19 @@ private Cluster startCluster( String recordFormat ) throws Exception return cluster; } + private Cluster startCluster2( String recordFormat ) throws ExecutionException, InterruptedException + { + Map sharedParams = new HashMap<>( ); + sharedParams.put( GraphDatabaseSettings.record_format.name(), recordFormat ); + Cluster cluster = + new Cluster( testDirectory.directory( "cluster-b_" + recordFormat ), 3, 0, new SharedDiscoveryServiceFactory(), sharedParams, emptyMap(), + sharedParams, emptyMap(), + recordFormat, IpFamily.IPV4, false ); + cluster.start(); + createSomeData( cluster ); + return cluster; + } + public static DbRepresentation createSomeData( Cluster cluster ) { try diff --git a/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandHaIT.java b/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandHaIT.java index 65827aea6d5c2..909f0cbf40972 100644 --- a/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandHaIT.java +++ b/enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandHaIT.java @@ -48,6 +48,8 @@ import org.neo4j.graphdb.Node; import org.neo4j.graphdb.RelationshipType; import org.neo4j.graphdb.Transaction; +import org.neo4j.graphdb.factory.GraphDatabaseBuilder; +import org.neo4j.graphdb.factory.GraphDatabaseFactory; import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.io.fs.DefaultFileSystemAbstraction; import org.neo4j.kernel.configuration.Config; @@ -66,6 +68,7 @@ import static org.hamcrest.Matchers.greaterThan; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeFalse; @@ -110,6 +113,19 @@ private static void createSomeData( GraphDatabaseService db ) } } + private static void createSpecificNodePair( GraphDatabaseService db, String name ) + { + try ( Transaction tx = db.beginTx() ) + { + Node left = db.createNode(); + left.setProperty( "name", name + "Left" ); + Node right = db.createNode(); + right.setProperty( "name", name + "Right" ); + right.createRelationshipTo( left, RelationshipType.withName( "KNOWS" ) ); + tx.success(); + } + } + @Before public void resetTasks() { @@ -143,7 +159,7 @@ public void makeSureBackupCanBePerformedWithCustomPort() throws Exception "--cc-report-dir=" + backupDir, "--backup-dir=" + backupDir, "--name=" + backupName ) ); - assertEquals( getDbRepresentation(), getBackupDbRepresentation( backupName ) ); + assertEquals( DbRepresentation.of( db ), getBackupDbRepresentation( backupName ) ); createSomeData( db ); assertEquals( 0, @@ -151,7 +167,7 @@ public void makeSureBackupCanBePerformedWithCustomPort() throws Exception "--cc-report-dir=" + backupDir, "--backup-dir=" + backupDir, "--name=" + backupName ) ); - assertEquals( getDbRepresentation(), getBackupDbRepresentation( backupName ) ); + assertEquals( DbRepresentation.of( db ), getBackupDbRepresentation( backupName ) ); } @Test @@ -289,6 +305,48 @@ public void reportsProgress() throws Exception assertFalse( output.contains( "Finished receiving index snapshots" ) ); } + @Test + public void backupRenamesWork() throws Exception + { + // given a prexisting backup from a different store + String backupName = "preexistingBackup_" + recordFormat; + int firstBackupPort = PortAuthority.allocatePort(); + startDb( firstBackupPort ); + createSpecificNodePair( db, "first" ); + + assertEquals( 0, runSameJvm( backupDir, backupName, + "--from", "127.0.0.1:" + firstBackupPort, + "--cc-report-dir=" + backupDir, + "--protocol=common", + "--backup-dir=" + backupDir, + "--name=" + backupName ) ); + DbRepresentation firstDatabaseRepresentation = DbRepresentation.of( db ); + + // and a different database + int secondBackupPort = PortAuthority.allocatePort(); + GraphDatabaseService db2 = createDb2( secondBackupPort ); + createSpecificNodePair( db2, "second" ); + DbRepresentation secondDatabaseRepresentation = DbRepresentation.of( db2 ); + + // when backup is performed + assertEquals( 0, runSameJvm(backupDir, backupName, + "--from", "127.0.0.1:" + secondBackupPort, + "--cc-report-dir=" + backupDir, + "--backup-dir=" + backupDir, + "--protocol=common", + "--name=" + backupName ) ); + + // then the new backup has the correct name + assertEquals( secondDatabaseRepresentation, getBackupDbRepresentation( backupName ) ); + + // and the old backup is in a renamed location + assertEquals( firstDatabaseRepresentation, getBackupDbRepresentation( backupName + ".err.0" ) ); + + // and the data isn't equal (sanity check) + assertNotEquals( firstDatabaseRepresentation, secondDatabaseRepresentation ); + db2.shutdown(); + } + private void repeatedlyPopulateDatabase( GraphDatabaseService db, AtomicBoolean continueFlagReference ) { while ( continueFlagReference.get() ) @@ -313,7 +371,21 @@ private void createIndexes( GraphDatabaseService db ) } } + private GraphDatabaseService createDb2( Integer backupPort ) + { + File storeDir = testDirectory.directory("graph-db-2"); + GraphDatabaseFactory factory = new GraphDatabaseFactory(); + GraphDatabaseBuilder builder = factory.newEmbeddedDatabaseBuilder( storeDir ); + builder.setConfig( OnlineBackupSettings.online_backup_server, "0.0.0.0:" + backupPort ); + return builder.newGraphDatabase(); + } + private void startDb( Integer backupPort ) + { + startDb( db, backupPort ); + } + + private void startDb( EmbeddedDatabaseRule db, Integer backupPort ) { db.setConfig( GraphDatabaseSettings.record_format, recordFormat ); db.setConfig( OnlineBackupSettings.online_backup_enabled, Settings.TRUE ); @@ -333,9 +405,18 @@ private static int runBackupTool( File neo4jHome, String... args ) return runBackupToolFromOtherJvmToGetExitCode( neo4jHome, args ); } - private DbRepresentation getDbRepresentation() + private static int runSameJvm( File home, String name, String... args ) { - return DbRepresentation.of( db ); + try + { + new OnlineBackupCommandBuilder().withRawArgs( args ).backup( home, name ); + return 0; + } + catch ( Exception e ) + { + e.printStackTrace(); + return 1; + } } private DbRepresentation getBackupDbRepresentation( String name ) diff --git a/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveAction.java b/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveAction.java index da15bd6a41004..27cdefa8d8b49 100644 --- a/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveAction.java +++ b/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveAction.java @@ -74,6 +74,7 @@ public void move( File toDir, CopyOption... copyOptions ) throws IOException Path resolvedPath = toDir.toPath().resolve( relativePath ); Files.createDirectories( resolvedPath.getParent() ); Files.copy( originalPath, resolvedPath, copyOptions ); + Files.delete( originalPath ); } @Override diff --git a/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveProvider.java b/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveProvider.java index 02620d250f60d..62e04a8d93af1 100644 --- a/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveProvider.java +++ b/enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveProvider.java @@ -126,7 +126,7 @@ private Stream expandTraverseFiles( File dir, File basePath ) File base = basePath; // Capture effectively-final base path snapshot. Stream files = listing.stream().filter( this::isFile ); Stream dirs = listing.stream().filter( this::isDirectory ); - Stream moveFiles = files.map( f -> copyFileCorrectly( f, base ) ); + Stream moveFiles = files.map( f -> moveFileCorrectly( f, base ) ); Stream traverseDirectories = dirs.flatMap( d -> traverseForMoving( d, base ) ); return Stream.concat( moveFiles, traverseDirectories ); } @@ -171,7 +171,7 @@ private List listFiles( File dir ) * contains the logic for handling files between * the 2 systems */ - private FileMoveAction copyFileCorrectly( File fileToMove, File basePath ) + private FileMoveAction moveFileCorrectly( File fileToMove, File basePath ) { if ( fileMoveActionInformer.shouldBeManagedByPageCache( fileToMove.getName() ) ) { diff --git a/enterprise/com/src/test/java/org/neo4j/com/storecopy/FileMoveActionTest.java b/enterprise/com/src/test/java/org/neo4j/com/storecopy/FileMoveActionTest.java new file mode 100644 index 0000000000000..08ba010f9fe93 --- /dev/null +++ b/enterprise/com/src/test/java/org/neo4j/com/storecopy/FileMoveActionTest.java @@ -0,0 +1,98 @@ +/* + * Copyright (c) 2002-2018 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j Enterprise Edition. The included source + * code can be redistributed and/or modified under the terms of the + * GNU AFFERO GENERAL PUBLIC LICENSE Version 3 + * (http://www.fsf.org/licensing/licenses/agpl-3.0.html) with the + * Commons Clause, as found in the associated LICENSE.txt file. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * Neo4j object code can be licensed independently from the source + * under separate terms from the AGPL. Inquiries can be directed to: + * licensing@neo4j.com + * + * More information is also available at: + * https://neo4j.com/licensing/ + */ +package org.neo4j.com.storecopy; + +import org.junit.Rule; +import org.junit.Test; + +import java.io.File; +import java.io.IOException; +import java.nio.file.StandardOpenOption; + +import org.neo4j.io.fs.DefaultFileSystemAbstraction; +import org.neo4j.io.fs.FileSystemAbstraction; +import org.neo4j.io.pagecache.PageCache; +import org.neo4j.kernel.configuration.Config; +import org.neo4j.kernel.impl.pagecache.ConfigurableStandalonePageCacheFactory; +import org.neo4j.test.rule.TestDirectory; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class FileMoveActionTest +{ + @Rule + public final TestDirectory testDirectory = TestDirectory.testDirectory(); + + private FileSystemAbstraction fileSystemAbstraction = new DefaultFileSystemAbstraction(); + private Config config = Config.defaults(); + + @Test + public void pageCacheFilesMovedDoNotLeaveOriginal() throws IOException + { + // given + PageCache pageCache = aPageCache(); + + // and + File pageCacheFile = new File( "page-cache-file" ); + pageCache.map( pageCacheFile, 100, StandardOpenOption.CREATE ); + + // when + File targetRename = new File( "target" ); + FileMoveAction.copyViaPageCache( pageCacheFile, pageCache ).move( targetRename ); + + // then + assertFalse( pageCacheFile.exists() ); + assertTrue( targetRename.exists() ); + } + + @Test + public void nonPageCacheFilesMovedDoNotLeaveOriginal() throws IOException + { + // given + File baseDirectory = testDirectory.directory(); + File sourceDirectory = new File( baseDirectory, "source" ); + File targetDirectory = new File( baseDirectory, "target" ); + File sourceFile = new File( sourceDirectory, "theFileName" ); + File targetFile = new File( targetDirectory, "theFileName" ); + sourceFile.getParentFile().mkdirs(); + targetDirectory.mkdirs(); + + // and sanity check + assertTrue( sourceFile.createNewFile() ); + assertTrue( sourceFile.exists() ); + assertFalse( targetFile.exists() ); + + // when + FileMoveAction.copyViaFileSystem( sourceFile, sourceDirectory ).move( targetDirectory ); + + // then + assertTrue( targetFile.exists() ); + assertFalse( sourceFile.exists() ); + } + + private PageCache aPageCache() + { + return ConfigurableStandalonePageCacheFactory.createPageCache( fileSystemAbstraction, config ); + } +} diff --git a/enterprise/kernel/src/test/java/org/neo4j/util/TestHelpers.java b/enterprise/kernel/src/test/java/org/neo4j/util/TestHelpers.java index e9e96eaaee367..922d8e2115f31 100644 --- a/enterprise/kernel/src/test/java/org/neo4j/util/TestHelpers.java +++ b/enterprise/kernel/src/test/java/org/neo4j/util/TestHelpers.java @@ -107,6 +107,5 @@ public static String backupAddressHa( GraphDatabaseAPI graphDatabase ) return hostnamePort.toString(); } - }