Skip to content

Commit

Permalink
Backup temporary directory rotation fix
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@phughk
phughk committed Jun 25, 2018
1 parent 2c9d837 commit 406d401
Show file tree
Hide file tree
Showing 9 changed files with 251 additions and 14 deletions.
2 changes: 1 addition & 1 deletion community/kernel/src/test/java/org/neo4j/test/rule/EmbeddedDatabaseRule.java
View file
Expand Up @@ -71,7 +71,7 @@ protected GraphDatabaseFactory newFactory()
@Override
protected GraphDatabaseBuilder newBuilder( GraphDatabaseFactory factory )
{
return factory.newEmbeddedDatabaseBuilder( testDirectory.graphDbDir() );
return factory.newEmbeddedDatabaseBuilder( getStoreDir() );
}

@Override
Expand Down
1 change: 1 addition & 0 deletions enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupCopyService.java
View file
Expand Up @@ -70,6 +70,7 @@ public void moveBackupLocation( Path oldLocation, Path newLocation ) throws IOEx
{
moves.next().move( target );
}
oldLocation.toFile().delete();
}
catch ( IOException e )
{
Expand Down
10 changes: 4 additions & 6 deletions enterprise/backup/src/main/java/org/neo4j/backup/impl/BackupStrategyWrapper.java
View file
Expand Up @@ -74,8 +74,7 @@ Fallible<BackupStrategyOutcome> doBackup( OnlineBackupContext onlineBackupContex
return state;
}

private Fallible<BackupStrategyOutcome> performBackupWithoutLifecycle(
OnlineBackupContext onlineBackupContext )
private Fallible<BackupStrategyOutcome> performBackupWithoutLifecycle( OnlineBackupContext onlineBackupContext )
{
Path backupLocation = onlineBackupContext.getResolvedLocationFromName();
Path userSpecifiedBackupLocation = onlineBackupContext.getResolvedLocationFromName();
Expand Down Expand Up @@ -136,8 +135,7 @@ private void clearIdFiles( Path backupLocation )
* @param onlineBackupContext command line arguments, config etc.
* @return outcome of full backup
*/
private Fallible<BackupStageOutcome> fullBackupWithTemporaryFolderResolutions(
OnlineBackupContext onlineBackupContext )
private Fallible<BackupStageOutcome> fullBackupWithTemporaryFolderResolutions( OnlineBackupContext onlineBackupContext )
{
Path userSpecifiedBackupLocation = onlineBackupContext.getResolvedLocationFromName();
Path temporaryFullBackupLocation = backupCopyService.findAnAvailableLocationForNewFullBackup( userSpecifiedBackupLocation );
Expand All @@ -146,12 +144,12 @@ private Fallible<BackupStageOutcome> fullBackupWithTemporaryFolderResolutions(
Fallible<BackupStageOutcome> 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
{
Expand Down
59 changes: 59 additions & 0 deletions enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandCcIT.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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 )
Expand Down Expand Up @@ -342,6 +388,19 @@ private Cluster startCluster( String recordFormat ) throws Exception
return cluster;
}

private Cluster startCluster2( String recordFormat ) throws ExecutionException, InterruptedException
{
Map<String,String> 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
Expand Down
89 changes: 85 additions & 4 deletions enterprise/backup/src/test/java/org/neo4j/backup/impl/OnlineBackupCommandHaIT.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -143,15 +159,15 @@ 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,
runBackupTool( testDirectory.absolutePath(), "--from", "127.0.0.1:" + backupPort,
"--cc-report-dir=" + backupDir,
"--backup-dir=" + backupDir,
"--name=" + backupName ) );
assertEquals( getDbRepresentation(), getBackupDbRepresentation( backupName ) );
assertEquals( DbRepresentation.of( db ), getBackupDbRepresentation( backupName ) );
}

@Test
Expand Down Expand Up @@ -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() )
Expand All @@ -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 );
Expand All @@ -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 )
Expand Down
1 change: 1 addition & 0 deletions enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveAction.java
View file
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions enterprise/com/src/main/java/org/neo4j/com/storecopy/FileMoveProvider.java
View file
Expand Up @@ -126,7 +126,7 @@ private Stream<FileMoveAction> expandTraverseFiles( File dir, File basePath )
File base = basePath; // Capture effectively-final base path snapshot.
Stream<File> files = listing.stream().filter( this::isFile );
Stream<File> dirs = listing.stream().filter( this::isDirectory );
Stream<FileMoveAction> moveFiles = files.map( f -> copyFileCorrectly( f, base ) );
Stream<FileMoveAction> moveFiles = files.map( f -> moveFileCorrectly( f, base ) );
Stream<FileMoveAction> traverseDirectories = dirs.flatMap( d -> traverseForMoving( d, base ) );
return Stream.concat( moveFiles, traverseDirectories );
}
Expand Down Expand Up @@ -171,7 +171,7 @@ private List<File> 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() ) )
{
Expand Down

0 comments on commit 406d401

Please sign in to comment.