diff --git a/modules/core/core-api/src/main/java/com/enonic/xp/node/DeleteSnapshotsResult.java b/modules/core/core-api/src/main/java/com/enonic/xp/node/DeleteSnapshotsResult.java index 5741f1e491e..11bf3bd43cd 100644 --- a/modules/core/core-api/src/main/java/com/enonic/xp/node/DeleteSnapshotsResult.java +++ b/modules/core/core-api/src/main/java/com/enonic/xp/node/DeleteSnapshotsResult.java @@ -13,9 +13,17 @@ public class DeleteSnapshotsResult extends AbstractImmutableEntitySet { + private final Set failedSnapshotNames; + private DeleteSnapshotsResult( final Builder builder ) { super( ImmutableSet.copyOf( builder.snapshotNames ) ); + this.failedSnapshotNames = ImmutableSet.copyOf( builder.failedSnapshotNames ); + } + + public Set getFailedSnapshotNames() + { + return failedSnapshotNames; } public static Builder create() @@ -27,6 +35,8 @@ public static class Builder { private final Set snapshotNames = new HashSet<>(); + private final Set failedSnapshotNames = new HashSet<>(); + public Builder add( final String snapshotName ) { @@ -34,12 +44,19 @@ public Builder add( final String snapshotName ) return this; } + @Deprecated public Builder addAll( final Collection snapshotNames ) { this.snapshotNames.addAll( snapshotNames ); return this; } + public Builder addFailed( final String snapshotName ) + { + this.failedSnapshotNames.add( snapshotName ); + return this; + } + public DeleteSnapshotsResult build() { return new DeleteSnapshotsResult( this ); diff --git a/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImpl.java b/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImpl.java index 3d20215a347..d6cdfc24420 100644 --- a/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImpl.java +++ b/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImpl.java @@ -2,7 +2,6 @@ import java.nio.file.Path; import java.time.Instant; -import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Set; @@ -316,46 +315,54 @@ private DeleteSnapshotsResult doDelete( final DeleteSnapshotParams params ) if ( !params.getSnapshotNames().isEmpty() ) { - builder.addAll( deleteByName( params.getSnapshotNames() ) ); + deleteByName( builder, params.getSnapshotNames() ); } if ( params.getBefore() != null ) { - builder.addAll( deleteByBefore( params.getBefore() ) ); + deleteByBefore( builder, params.getBefore() ); } return builder.build(); } - private Set deleteByBefore( final Instant before ) + private void deleteByBefore( final DeleteSnapshotsResult.Builder builder, final Instant before ) { - final Set deleted = new HashSet<>(); - final SnapshotResults snapshotResults = doListSnapshots(); for ( final SnapshotResult snapshotResult : snapshotResults ) { if ( snapshotResult.getTimestamp().isBefore( before ) ) { - doDeleteSnapshot( snapshotResult.getName() ); - deleted.add( snapshotResult.getName() ); + try + { + doDeleteSnapshot( snapshotResult.getName() ); + builder.add( snapshotResult.getName() ); + } + catch ( Exception e ) + { + LOG.error( "Snapshot delete failed: {}", snapshotResult.getName(), e ); + builder.addFailed( snapshotResult.getName() ); + } } } - - return deleted; } - private Set deleteByName( final Set snapshotNames ) + private void deleteByName( final DeleteSnapshotsResult.Builder builder, final Set snapshotNames ) { - final Set deletedNames = new HashSet<>(); - for ( final String name : snapshotNames ) { - doDeleteSnapshot( name ); - deletedNames.add( name ); + try + { + doDeleteSnapshot( name ); + builder.add( name ); + } + catch ( Exception e ) + { + LOG.error( "Snapshot delete failed: {}", name, e ); + builder.addFailed( name ); + } } - - return deletedNames; } private void doDeleteSnapshot( final String snapshotName ) diff --git a/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTask.java b/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTask.java index 80bf076a0ef..68dcd398682 100644 --- a/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTask.java +++ b/modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTask.java @@ -42,20 +42,13 @@ public VacuumTaskResult execute( final VacuumTaskParams params ) } final VacuumTaskResult.Builder builder = VacuumTaskResult.create().taskName( NAME ); - try - { - final DeleteSnapshotsResult deleteSnapshotsResult = snapshotService.delete( - DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() ); + final DeleteSnapshotsResult deleteSnapshotsResult = + snapshotService.delete( DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() ); - deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() ); + deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() ); + deleteSnapshotsResult.getFailedSnapshotNames().forEach( snapshot -> builder.failed() ); - return builder.build(); - } - catch ( Exception e ) - { - LOG.error( "Failed to vacuum snapshots", e ); - return builder.failed().build(); - } + return builder.build(); } @Override diff --git a/modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImplTest.java b/modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImplTest.java new file mode 100644 index 00000000000..14a250fd959 --- /dev/null +++ b/modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImplTest.java @@ -0,0 +1,120 @@ +package com.enonic.xp.repo.impl.elasticsearch.snapshot; + +import java.nio.file.Path; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.List; + +import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.action.ActionFuture; +import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesRequest; +import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse; +import org.elasticsearch.action.admin.cluster.snapshots.delete.DeleteSnapshotRequest; +import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest; +import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; +import org.elasticsearch.client.AdminClient; +import org.elasticsearch.client.Client; +import org.elasticsearch.client.ClusterAdminClient; +import org.elasticsearch.cluster.metadata.RepositoryMetaData; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.snapshots.SnapshotInfo; +import org.elasticsearch.snapshots.SnapshotState; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import com.enonic.xp.event.EventPublisher; +import com.enonic.xp.node.DeleteSnapshotParams; +import com.enonic.xp.node.DeleteSnapshotsResult; +import com.enonic.xp.repo.impl.config.RepoConfiguration; +import com.enonic.xp.repo.impl.index.IndexServiceInternal; +import com.enonic.xp.repo.impl.repository.RepositoryEntryService; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class SnapshotServiceImplTest +{ + private ClusterAdminClient clusterAdminClient; + + private SnapshotServiceImpl instance; + + @BeforeEach + void setUp() + { + final Client client = mock( Client.class ); + clusterAdminClient = mock( ClusterAdminClient.class ); + + when( client.admin() ).thenReturn( mock( AdminClient.class ) ); + when( client.admin().cluster() ).thenReturn( clusterAdminClient ); + + final RepoConfiguration configuration = mock( RepoConfiguration.class ); + when( configuration.getSnapshotsDir() ).thenReturn( Path.of( "tmp", "data" ) ); + + final RepositoryEntryService repositoryEntryService = mock( RepositoryEntryService.class ); + final EventPublisher eventPublisher = mock( EventPublisher.class ); + final IndexServiceInternal indexServiceInternal = mock( IndexServiceInternal.class ); + + instance = new SnapshotServiceImpl( client, configuration, repositoryEntryService, eventPublisher, indexServiceInternal ); + } + + @Test + void testDelete() + { + final RepositoryMetaData repositoryMetaData = new RepositoryMetaData( "enonic-xp-snapshot-repo", "fs", Settings.settingsBuilder() + .put( "compress", true ) + .put( "location", Path.of( "tmp", "data" ).toString() ) + .build() ); + + final GetRepositoriesResponse getRepositoriesResponse = mock( GetRepositoriesResponse.class ); + when( getRepositoriesResponse.repositories() ).thenReturn( List.of( repositoryMetaData ) ); + + final ActionFuture getRepositoryActionFuture = mock( ActionFuture.class ); + when( getRepositoryActionFuture.actionGet() ).thenReturn( getRepositoriesResponse ); + + when( clusterAdminClient.getRepositories( any( GetRepositoriesRequest.class ) ) ).thenReturn( getRepositoryActionFuture ); + + final Instant now = Instant.now(); + + final SnapshotInfo snapshot3 = mockSnapshot( "snapshot3", SnapshotState.SUCCESS, now.minus( 3, ChronoUnit.HOURS ) ); + final SnapshotInfo snapshot4 = mockSnapshot( "snapshot4", SnapshotState.PARTIAL, now.minus( 4, ChronoUnit.HOURS ) ); + final SnapshotInfo snapshot5 = mockSnapshot( "snapshot5", SnapshotState.SUCCESS, now.minus( 1, ChronoUnit.HOURS ) ); + + final GetSnapshotsResponse getSnapshotsResponse = mock( GetSnapshotsResponse.class ); + when( getSnapshotsResponse.getSnapshots() ).thenReturn( List.of( snapshot3, snapshot4, snapshot5 ) ); + + final ActionFuture getSnapshotsActionFuture = mock( ActionFuture.class ); + when( getSnapshotsActionFuture.actionGet() ).thenReturn( getSnapshotsResponse ); + + when( clusterAdminClient.getSnapshots( any( GetSnapshotsRequest.class ) ) ).thenReturn( getSnapshotsActionFuture ); + + when( clusterAdminClient.deleteSnapshot( any( DeleteSnapshotRequest.class ) ) ).thenReturn( mock( ActionFuture.class ) ); + when( clusterAdminClient.deleteSnapshot( + argThat( request -> "snapshot2".equals( request.snapshot() ) || "snapshot4".equals( request.snapshot() ) ) ) ).thenThrow( + new ElasticsearchException( "Failed to delete snapshot" ) ); + + final DeleteSnapshotsResult result = instance.delete( + DeleteSnapshotParams.create().add( "snapshot1" ).add( "snapshot2" ).before( now.minus( 2, ChronoUnit.HOURS ) ).build() ); + + assertEquals( 2, result.getSize() ); + assertTrue( result.getSet().contains( "snapshot1" ) ); + assertTrue( result.getSet().contains( "snapshot3" ) ); + assertEquals( 2, result.getFailedSnapshotNames().size() ); + assertTrue( result.getFailedSnapshotNames().contains( "snapshot2" ) ); + assertTrue( result.getFailedSnapshotNames().contains( "snapshot4" ) ); + } + + private static SnapshotInfo mockSnapshot( String name, SnapshotState state, Instant endTime ) + { + final SnapshotInfo snapshot = mock( SnapshotInfo.class ); + + when( snapshot.state() ).thenReturn( state ); + when( snapshot.name() ).thenReturn( name ); + when( snapshot.endTime() ).thenReturn( endTime.toEpochMilli() ); + + return snapshot; + } +} diff --git a/modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTaskTest.java b/modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTaskTest.java index 3d7f7bb3243..6d639b3c8a9 100644 --- a/modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTaskTest.java +++ b/modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTaskTest.java @@ -64,23 +64,4 @@ public void processed( final long count ) verify( snapshotService, times( 1 ) ).delete( any( DeleteSnapshotParams.class ) ); } - - @Test - void testFailed() - { - SnapshotService snapshotService = mock( SnapshotService.class ); - when( snapshotService.delete( any( DeleteSnapshotParams.class ) ) ).thenThrow( new RuntimeException() ); - - SnapshotsVacuumTask instance = new SnapshotsVacuumTask( snapshotService ); - - VacuumTaskResult result = instance.execute( VacuumTaskParams.create().ageThreshold( 60 * 1000 ).build() ); - - assertEquals( "SnapshotsVacuumTask", result.getTaskName() ); - assertEquals( 0, result.getProcessed() ); - assertEquals( 0, result.getInUse() ); - assertEquals( 0, result.getDeleted() ); - assertEquals( 1, result.getFailed() ); - - verify( snapshotService, times( 1 ) ).delete( any( DeleteSnapshotParams.class ) ); - } } diff --git a/modules/server/server-rest/src/main/java/com/enonic/xp/impl/server/rest/model/DeleteSnapshotsResultJson.java b/modules/server/server-rest/src/main/java/com/enonic/xp/impl/server/rest/model/DeleteSnapshotsResultJson.java index c5aecd97299..0923e3f6e10 100644 --- a/modules/server/server-rest/src/main/java/com/enonic/xp/impl/server/rest/model/DeleteSnapshotsResultJson.java +++ b/modules/server/server-rest/src/main/java/com/enonic/xp/impl/server/rest/model/DeleteSnapshotsResultJson.java @@ -8,18 +8,26 @@ public class DeleteSnapshotsResultJson { private final Set deletedSnapshots; - private DeleteSnapshotsResultJson( final Set deletedSnapshots ) + private final Set failedSnapshots; + + private DeleteSnapshotsResultJson( final Set deletedSnapshots, final Set failedSnapshots ) { this.deletedSnapshots = deletedSnapshots; + this.failedSnapshots = failedSnapshots; } public static DeleteSnapshotsResultJson from( final DeleteSnapshotsResult result ) { - return new DeleteSnapshotsResultJson( result.getSet() ); + return new DeleteSnapshotsResultJson( result.getSet(), result.getFailedSnapshotNames() ); } public Set getDeletedSnapshots() { return deletedSnapshots; } + + public Set getFailedSnapshots() + { + return failedSnapshots; + } } diff --git a/modules/server/server-rest/src/test/resources/com/enonic/xp/impl/server/rest/delete.json b/modules/server/server-rest/src/test/resources/com/enonic/xp/impl/server/rest/delete.json index 5d42d526639..69a481e4262 100644 --- a/modules/server/server-rest/src/test/resources/com/enonic/xp/impl/server/rest/delete.json +++ b/modules/server/server-rest/src/test/resources/com/enonic/xp/impl/server/rest/delete.json @@ -1,5 +1,6 @@ { "deletedSnapshots": [ "snapshot1" - ] -} \ No newline at end of file + ], + "failedSnapshots": [] +}