From 00f58c86b96e08c8a58466b3a5577abf112ccde2 Mon Sep 17 00:00:00 2001 From: Anatol Sialitski Date: Tue, 10 Dec 2024 11:27:44 +0100 Subject: [PATCH] No error/warning on failed snapshot deletion #10676 --- .../enonic/xp/node/DeleteSnapshotsResult.java | 1 + .../enonic/xp/node/RemoveSnapshotsResult.java | 60 +++++++++ .../enonic/xp/snapshot/SnapshotService.java | 4 + .../snapshot/SnapshotServiceImpl.java | 56 +++++--- .../vacuum/snapshots/SnapshotsVacuumTask.java | 24 +--- .../snapshot/SnapshotServiceImplTest.java | 120 ++++++++++++++++++ .../snapshots/SnapshotsVacuumTaskTest.java | 27 +--- .../snapshot/SnapshotServiceImplTest.java | 2 +- 8 files changed, 234 insertions(+), 60 deletions(-) create mode 100644 modules/core/core-api/src/main/java/com/enonic/xp/node/RemoveSnapshotsResult.java create mode 100644 modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImplTest.java 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..2ddcaa5e88a 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 @@ -9,6 +9,7 @@ import com.enonic.xp.annotation.PublicApi; import com.enonic.xp.support.AbstractImmutableEntitySet; +@Deprecated @PublicApi public class DeleteSnapshotsResult extends AbstractImmutableEntitySet diff --git a/modules/core/core-api/src/main/java/com/enonic/xp/node/RemoveSnapshotsResult.java b/modules/core/core-api/src/main/java/com/enonic/xp/node/RemoveSnapshotsResult.java new file mode 100644 index 00000000000..e8e7432a842 --- /dev/null +++ b/modules/core/core-api/src/main/java/com/enonic/xp/node/RemoveSnapshotsResult.java @@ -0,0 +1,60 @@ +package com.enonic.xp.node; + +import java.util.Set; + +import com.google.common.collect.ImmutableSet; + +import com.enonic.xp.annotation.PublicApi; + +@PublicApi +public final class RemoveSnapshotsResult +{ + private final Set snapshotNames; + + private final Set failedSnapshotNames; + + private RemoveSnapshotsResult( final Builder builder ) + { + this.snapshotNames = builder.snapshotNames.build(); + this.failedSnapshotNames = builder.failedSnapshotNames.build(); + } + + public Set getSnapshotNames() + { + return snapshotNames; + } + + public Set getFailedSnapshotNames() + { + return failedSnapshotNames; + } + + public static Builder create() + { + return new Builder(); + } + + public static final class Builder + { + private final ImmutableSet.Builder snapshotNames = ImmutableSet.builder(); + + private final ImmutableSet.Builder failedSnapshotNames = ImmutableSet.builder(); + + public Builder addProcessed( final String snapshotName ) + { + this.snapshotNames.add( snapshotName ); + return this; + } + + public Builder addFailed( final String snapshotName ) + { + this.failedSnapshotNames.add( snapshotName ); + return this; + } + + public RemoveSnapshotsResult build() + { + return new RemoveSnapshotsResult( this ); + } + } +} diff --git a/modules/core/core-api/src/main/java/com/enonic/xp/snapshot/SnapshotService.java b/modules/core/core-api/src/main/java/com/enonic/xp/snapshot/SnapshotService.java index 2e1cd661333..92c4652fc43 100644 --- a/modules/core/core-api/src/main/java/com/enonic/xp/snapshot/SnapshotService.java +++ b/modules/core/core-api/src/main/java/com/enonic/xp/snapshot/SnapshotService.java @@ -2,6 +2,7 @@ import com.enonic.xp.node.DeleteSnapshotParams; import com.enonic.xp.node.DeleteSnapshotsResult; +import com.enonic.xp.node.RemoveSnapshotsResult; import com.enonic.xp.node.RestoreParams; import com.enonic.xp.node.RestoreResult; import com.enonic.xp.node.SnapshotParams; @@ -14,7 +15,10 @@ public interface SnapshotService RestoreResult restore( RestoreParams restoreParams ); + @Deprecated DeleteSnapshotsResult delete( DeleteSnapshotParams params ); + RemoveSnapshotsResult remove( DeleteSnapshotParams params ); + SnapshotResults list(); } 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..7dbe879c61d 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; @@ -33,6 +32,7 @@ import com.enonic.xp.event.EventPublisher; import com.enonic.xp.node.DeleteSnapshotParams; import com.enonic.xp.node.DeleteSnapshotsResult; +import com.enonic.xp.node.RemoveSnapshotsResult; import com.enonic.xp.node.RestoreParams; import com.enonic.xp.node.RestoreResult; import com.enonic.xp.node.SnapshotParams; @@ -197,8 +197,18 @@ private SnapshotResults doListSnapshots() return SnapshotResultsFactory.create( getSnapshotsResponse ); } + @Deprecated @Override public DeleteSnapshotsResult delete( final DeleteSnapshotParams params ) + { + return NodeHelper.runAsAdmin( () -> { + final RemoveSnapshotsResult result = doDelete( params ); + return DeleteSnapshotsResult.create().addAll( result.getSnapshotNames() ).build(); + } ); + } + + @Override + public RemoveSnapshotsResult remove( final DeleteSnapshotParams params ) { return NodeHelper.runAsAdmin( () -> doDelete( params ) ); } @@ -310,52 +320,60 @@ private SnapshotInfo getSnapshot( final String snapshotName ) } } - private DeleteSnapshotsResult doDelete( final DeleteSnapshotParams params ) + private RemoveSnapshotsResult doDelete( final DeleteSnapshotParams params ) { - final DeleteSnapshotsResult.Builder builder = DeleteSnapshotsResult.create(); + final RemoveSnapshotsResult.Builder builder = RemoveSnapshotsResult.create(); 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 RemoveSnapshotsResult.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.addProcessed( 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 RemoveSnapshotsResult.Builder builder, final Set snapshotNames ) { - final Set deletedNames = new HashSet<>(); - for ( final String name : snapshotNames ) { - doDeleteSnapshot( name ); - deletedNames.add( name ); + try + { + doDeleteSnapshot( name ); + builder.addProcessed( 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..1e77d98a2ba 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 @@ -5,11 +5,9 @@ import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import com.enonic.xp.node.DeleteSnapshotParams; -import com.enonic.xp.node.DeleteSnapshotsResult; +import com.enonic.xp.node.RemoveSnapshotsResult; import com.enonic.xp.repo.impl.vacuum.VacuumTask; import com.enonic.xp.repo.impl.vacuum.VacuumTaskParams; import com.enonic.xp.snapshot.SnapshotService; @@ -19,8 +17,6 @@ public class SnapshotsVacuumTask implements VacuumTask { - private static final Logger LOG = LoggerFactory.getLogger( SnapshotsVacuumTask.class ); - private static final int ORDER = 400; private static final String NAME = "SnapshotsVacuumTask"; @@ -42,20 +38,14 @@ 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() ); - deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() ); + final RemoveSnapshotsResult deleteSnapshotsResult = + snapshotService.remove( DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() ); - return builder.build(); - } - catch ( Exception e ) - { - LOG.error( "Failed to vacuum snapshots", e ); - return builder.failed().build(); - } + deleteSnapshotsResult.getSnapshotNames().forEach( snapshot -> builder.processed() ); + deleteSnapshotsResult.getFailedSnapshotNames().forEach( snapshot -> builder.failed() ); + + 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..cce2a36b9cf --- /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.RemoveSnapshotsResult; +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 testRemove() + { + final RepositoryMetaData repositoryMetaData = new RepositoryMetaData( "enonic-xp-snapshot-repo", "fs", Settings.settingsBuilder() + .put( "compress", true ) + .put( "location", "\\tmp\\data" ) + .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 RemoveSnapshotsResult result = instance.remove( + DeleteSnapshotParams.create().add( "snapshot1" ).add( "snapshot2" ).before( now.minus( 2, ChronoUnit.HOURS ) ).build() ); + + assertEquals( 2, result.getSnapshotNames().size() ); + assertTrue( result.getSnapshotNames().contains( "snapshot1" ) ); + assertTrue( result.getSnapshotNames().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..d461bb2327b 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 @@ -3,7 +3,7 @@ import org.junit.jupiter.api.Test; import com.enonic.xp.node.DeleteSnapshotParams; -import com.enonic.xp.node.DeleteSnapshotsResult; +import com.enonic.xp.node.RemoveSnapshotsResult; import com.enonic.xp.repo.impl.vacuum.VacuumTaskParams; import com.enonic.xp.snapshot.SnapshotService; import com.enonic.xp.vacuum.VacuumListener; @@ -22,8 +22,8 @@ public class SnapshotsVacuumTaskTest void test() { SnapshotService snapshotService = mock( SnapshotService.class ); - when( snapshotService.delete( any( DeleteSnapshotParams.class ) ) ).thenReturn( - DeleteSnapshotsResult.create().add( "snapshot" ).build() ); + when( snapshotService.remove( any( DeleteSnapshotParams.class ) ) ).thenReturn( + RemoveSnapshotsResult.create().addProcessed( "snapshot" ).build() ); SnapshotsVacuumTask instance = new SnapshotsVacuumTask( snapshotService ); @@ -62,25 +62,6 @@ public void processed( final long count ) assertEquals( 0, result.getDeleted() ); assertEquals( 0, result.getFailed() ); - 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 ) ); + verify( snapshotService, times( 1 ) ).remove( any( DeleteSnapshotParams.class ) ); } } diff --git a/modules/itest/itest-core/src/test/java/com/enonic/xp/core/snapshot/SnapshotServiceImplTest.java b/modules/itest/itest-core/src/test/java/com/enonic/xp/core/snapshot/SnapshotServiceImplTest.java index 783ecc1e3fc..abbef06ec66 100644 --- a/modules/itest/itest-core/src/test/java/com/enonic/xp/core/snapshot/SnapshotServiceImplTest.java +++ b/modules/itest/itest-core/src/test/java/com/enonic/xp/core/snapshot/SnapshotServiceImplTest.java @@ -252,7 +252,7 @@ private void doRestoreInvalidSnapshot() this.snapshotService.snapshot( SnapshotParams.create().snapshotName( "my-snapshot" ).build() ); - this.snapshotService.delete( DeleteSnapshotParams.create().add( "my-snapshot" ).build() ); + this.snapshotService.remove( DeleteSnapshotParams.create().add( "my-snapshot" ).build() ); this.repositoryService.deleteRepository( DeleteRepositoryParams.from( newRepoId ) );